| Message ID | 20210505064843.111900-4-vt@altlinux.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | ima-evm-utils: Add --keyid option | expand |
On 5/5/21 2:48 AM, Vitaly Chikunov wrote: > Allow to have certificate appended to the private key of `--key' > specified (PEM) file (for v2 signing) to facilitate reading of keyid > from the associated cert. This will allow users to have private and > public key as a single file. There is no check that public key form the > cert matches associated private key. > > Signed-off-by: Vitaly Chikunov <vt@altlinux.org> > --- > README | 3 +++ > src/libimaevm.c | 8 +++++--- > 2 files changed, 8 insertions(+), 3 deletions(-) > > diff --git a/README b/README > index 0e1f6ba..ea11bde 100644 > --- a/README > +++ b/README > @@ -127,6 +127,9 @@ for signing and importing the key. > Second key format uses X509 DER encoded public key certificates and uses asymmetric key support > in the kernel (since kernel 3.9). CONFIG_INTEGRITY_ASYMMETRIC_KEYS must be enabled (default). > > +For v2 signatures x509 certificate with the public key could be appended to the private > +key (both are in PEM format) to properly determine its Subject Key Identifier (SKID). > + > > Integrity keyrings > ---------------- > diff --git a/src/libimaevm.c b/src/libimaevm.c > index a22d9bb..ac4bb46 100644 > --- a/src/libimaevm.c > +++ b/src/libimaevm.c > @@ -1017,10 +1017,12 @@ static int sign_hash_v2(const char *algo, const unsigned char *hash, > return -1; > } > > - if (imaevm_params.keyid) > + if (imaevm_params.keyid) { > hdr->keyid = htonl(imaevm_params.keyid); > - else > - calc_keyid_v2(&hdr->keyid, name, pkey); > + } else { > + if (_ima_read_keyid(keyfile, &hdr->keyid, KEYID_FILE_PEM_KEY) == ULONG_MAX) > + calc_keyid_v2(&hdr->keyid, name, pkey); > + } It might be convenient here to just write the result in network byte order into the header but for a library API I find it not so nice, but then there's calc_keyid_v2 that does that already... I just wouldn't expect that these parameter are in big endian order already, I would expect them in native order. So maybe ima_read_keyid should just return ULONG_MAX or the keyid in host order and it call _ima_read_keyid() with a NULL pointer or a dummy variable as keyid that the library API caller doesn't see. Stefan > > st = "EVP_PKEY_CTX_new"; > if (!(ctx = EVP_PKEY_CTX_new(pkey, NULL)))
Stefan, On Wed, May 05, 2021 at 07:29:17PM -0400, Stefan Berger wrote: > > On 5/5/21 2:48 AM, Vitaly Chikunov wrote: > > Allow to have certificate appended to the private key of `--key' > > specified (PEM) file (for v2 signing) to facilitate reading of keyid > > from the associated cert. This will allow users to have private and > > public key as a single file. There is no check that public key form the > > cert matches associated private key. > > > > Signed-off-by: Vitaly Chikunov <vt@altlinux.org> > > --- > > README | 3 +++ > > src/libimaevm.c | 8 +++++--- > > 2 files changed, 8 insertions(+), 3 deletions(-) > > > > diff --git a/README b/README > > index 0e1f6ba..ea11bde 100644 > > --- a/README > > +++ b/README > > @@ -127,6 +127,9 @@ for signing and importing the key. > > Second key format uses X509 DER encoded public key certificates and uses asymmetric key support > > in the kernel (since kernel 3.9). CONFIG_INTEGRITY_ASYMMETRIC_KEYS must be enabled (default). > > +For v2 signatures x509 certificate with the public key could be appended to the private > > +key (both are in PEM format) to properly determine its Subject Key Identifier (SKID). > > + > > Integrity keyrings > > ---------------- > > diff --git a/src/libimaevm.c b/src/libimaevm.c > > index a22d9bb..ac4bb46 100644 > > --- a/src/libimaevm.c > > +++ b/src/libimaevm.c > > @@ -1017,10 +1017,12 @@ static int sign_hash_v2(const char *algo, const unsigned char *hash, > > return -1; > > } > > - if (imaevm_params.keyid) > > + if (imaevm_params.keyid) { > > hdr->keyid = htonl(imaevm_params.keyid); > > - else > > - calc_keyid_v2(&hdr->keyid, name, pkey); > > + } else { > > + if (_ima_read_keyid(keyfile, &hdr->keyid, KEYID_FILE_PEM_KEY) == ULONG_MAX) > > + calc_keyid_v2(&hdr->keyid, name, pkey); > > + } > > It might be convenient here to just write the result in network byte order > into the header but for a library API I find it not so nice, but then > there's calc_keyid_v2 that does that already... I just wouldn't expect that > these parameter are in big endian order already, I would expect them in > native order. I expect them in network order, similar to how calc_keyid_v2() already writes it one line below. So, why should we mix orders? Both functions write keyids, so it's not like completely different parts of API. Also, it's documented that ima_read_keyid() writes to the pointer in network order (and returns integer in host order), so I don't see the problem. Thus, I would prefer not to follow this suggestion. Thanks, > So maybe ima_read_keyid should just return ULONG_MAX or the > keyid in host order and it call _ima_read_keyid() with a NULL pointer or a > dummy variable as keyid that the library API caller doesn't see. > > Stefan > > > > st = "EVP_PKEY_CTX_new"; > > if (!(ctx = EVP_PKEY_CTX_new(pkey, NULL)))
Stefan, On Thu, May 06, 2021 at 03:29:32AM +0300, Vitaly Chikunov wrote: > On Wed, May 05, 2021 at 07:29:17PM -0400, Stefan Berger wrote: > > > > On 5/5/21 2:48 AM, Vitaly Chikunov wrote: > > > Allow to have certificate appended to the private key of `--key' > > > specified (PEM) file (for v2 signing) to facilitate reading of keyid > > > from the associated cert. This will allow users to have private and > > > public key as a single file. There is no check that public key form the > > > cert matches associated private key. > > > > > > Signed-off-by: Vitaly Chikunov <vt@altlinux.org> > > > --- > > > README | 3 +++ > > > src/libimaevm.c | 8 +++++--- > > > 2 files changed, 8 insertions(+), 3 deletions(-) > > > > > > diff --git a/README b/README > > > index 0e1f6ba..ea11bde 100644 > > > --- a/README > > > +++ b/README > > > @@ -127,6 +127,9 @@ for signing and importing the key. > > > Second key format uses X509 DER encoded public key certificates and uses asymmetric key support > > > in the kernel (since kernel 3.9). CONFIG_INTEGRITY_ASYMMETRIC_KEYS must be enabled (default). > > > +For v2 signatures x509 certificate with the public key could be appended to the private > > > +key (both are in PEM format) to properly determine its Subject Key Identifier (SKID). > > > + > > > Integrity keyrings > > > ---------------- > > > diff --git a/src/libimaevm.c b/src/libimaevm.c > > > index a22d9bb..ac4bb46 100644 > > > --- a/src/libimaevm.c > > > +++ b/src/libimaevm.c > > > @@ -1017,10 +1017,12 @@ static int sign_hash_v2(const char *algo, const unsigned char *hash, > > > return -1; > > > } > > > - if (imaevm_params.keyid) > > > + if (imaevm_params.keyid) { > > > hdr->keyid = htonl(imaevm_params.keyid); > > > - else > > > - calc_keyid_v2(&hdr->keyid, name, pkey); > > > + } else { > > > + if (_ima_read_keyid(keyfile, &hdr->keyid, KEYID_FILE_PEM_KEY) == ULONG_MAX) > > > + calc_keyid_v2(&hdr->keyid, name, pkey); > > > + } > > > > It might be convenient here to just write the result in network byte order > > into the header but for a library API I find it not so nice, but then > > there's calc_keyid_v2 that does that already... I just wouldn't expect that > > these parameter are in big endian order already, I would expect them in > > native order. > > I expect them in network order, similar to how calc_keyid_v2() already > writes it one line below. So, why should we mix orders? Both functions > write keyids, so it's not like completely different parts of API. Also, > it's documented that ima_read_keyid() writes to the pointer in network > order (and returns integer in host order), so I don't see the > problem. Thus, I would prefer not to follow this suggestion. > > Thanks, > > > So maybe ima_read_keyid should just return ULONG_MAX or the > > keyid in host order and it call _ima_read_keyid() with a NULL pointer or a > > dummy variable as keyid that the library API caller doesn't see. So there will be exported uint32_t ima_read_keyid(char *certfile); and internal static bool _read_keyid(char *certfile, uint32_t *keyid) ima_read_keyid will wrap read_keyid and return keyid via intermediate variable. Right? > > > > Stefan > > > > > > > st = "EVP_PKEY_CTX_new"; > > > if (!(ctx = EVP_PKEY_CTX_new(pkey, NULL)))
On 5/5/21 8:53 PM, Vitaly Chikunov wrote: > Stefan, > > On Thu, May 06, 2021 at 03:29:32AM +0300, Vitaly Chikunov wrote: >> On Wed, May 05, 2021 at 07:29:17PM -0400, Stefan Berger wrote: >>> On 5/5/21 2:48 AM, Vitaly Chikunov wrote: >>>> Allow to have certificate appended to the private key of `--key' >>>> specified (PEM) file (for v2 signing) to facilitate reading of keyid >>>> from the associated cert. This will allow users to have private and >>>> public key as a single file. There is no check that public key form the >>>> cert matches associated private key. >>>> >>>> Signed-off-by: Vitaly Chikunov <vt@altlinux.org> >>>> --- >>>> README | 3 +++ >>>> src/libimaevm.c | 8 +++++--- >>>> 2 files changed, 8 insertions(+), 3 deletions(-) >>>> >>>> diff --git a/README b/README >>>> index 0e1f6ba..ea11bde 100644 >>>> --- a/README >>>> +++ b/README >>>> @@ -127,6 +127,9 @@ for signing and importing the key. >>>> Second key format uses X509 DER encoded public key certificates and uses asymmetric key support >>>> in the kernel (since kernel 3.9). CONFIG_INTEGRITY_ASYMMETRIC_KEYS must be enabled (default). >>>> +For v2 signatures x509 certificate with the public key could be appended to the private >>>> +key (both are in PEM format) to properly determine its Subject Key Identifier (SKID). >>>> + >>>> Integrity keyrings >>>> ---------------- >>>> diff --git a/src/libimaevm.c b/src/libimaevm.c >>>> index a22d9bb..ac4bb46 100644 >>>> --- a/src/libimaevm.c >>>> +++ b/src/libimaevm.c >>>> @@ -1017,10 +1017,12 @@ static int sign_hash_v2(const char *algo, const unsigned char *hash, >>>> return -1; >>>> } >>>> - if (imaevm_params.keyid) >>>> + if (imaevm_params.keyid) { >>>> hdr->keyid = htonl(imaevm_params.keyid); >>>> - else >>>> - calc_keyid_v2(&hdr->keyid, name, pkey); >>>> + } else { >>>> + if (_ima_read_keyid(keyfile, &hdr->keyid, KEYID_FILE_PEM_KEY) == ULONG_MAX) >>>> + calc_keyid_v2(&hdr->keyid, name, pkey); >>>> + } >>> It might be convenient here to just write the result in network byte order >>> into the header but for a library API I find it not so nice, but then >>> there's calc_keyid_v2 that does that already... I just wouldn't expect that >>> these parameter are in big endian order already, I would expect them in >>> native order. >> I expect them in network order, similar to how calc_keyid_v2() already >> writes it one line below. So, why should we mix orders? Both functions >> write keyids, so it's not like completely different parts of API. Also, >> it's documented that ima_read_keyid() writes to the pointer in network >> order (and returns integer in host order), so I don't see the >> problem. Thus, I would prefer not to follow this suggestion. >> >> Thanks, >> >>> So maybe ima_read_keyid should just return ULONG_MAX or the >>> keyid in host order and it call _ima_read_keyid() with a NULL pointer or a >>> dummy variable as keyid that the library API caller doesn't see. > So there will be exported > uint32_t ima_read_keyid(char *certfile); > and internal > static bool _read_keyid(char *certfile, uint32_t *keyid) > > ima_read_keyid will wrap read_keyid and return keyid via intermediate > variable. Right? Yes like this. 0 returned from ima_read_keyid would indicate that nothing was found but could with a chance of 1/2^-32 be also a valid key id. Though we already declare it invalid in 1/3 when parsing a 0 from command line.
diff --git a/README b/README index 0e1f6ba..ea11bde 100644 --- a/README +++ b/README @@ -127,6 +127,9 @@ for signing and importing the key. Second key format uses X509 DER encoded public key certificates and uses asymmetric key support in the kernel (since kernel 3.9). CONFIG_INTEGRITY_ASYMMETRIC_KEYS must be enabled (default). +For v2 signatures x509 certificate with the public key could be appended to the private +key (both are in PEM format) to properly determine its Subject Key Identifier (SKID). + Integrity keyrings ---------------- diff --git a/src/libimaevm.c b/src/libimaevm.c index a22d9bb..ac4bb46 100644 --- a/src/libimaevm.c +++ b/src/libimaevm.c @@ -1017,10 +1017,12 @@ static int sign_hash_v2(const char *algo, const unsigned char *hash, return -1; } - if (imaevm_params.keyid) + if (imaevm_params.keyid) { hdr->keyid = htonl(imaevm_params.keyid); - else - calc_keyid_v2(&hdr->keyid, name, pkey); + } else { + if (_ima_read_keyid(keyfile, &hdr->keyid, KEYID_FILE_PEM_KEY) == ULONG_MAX) + calc_keyid_v2(&hdr->keyid, name, pkey); + } st = "EVP_PKEY_CTX_new"; if (!(ctx = EVP_PKEY_CTX_new(pkey, NULL)))
Allow to have certificate appended to the private key of `--key' specified (PEM) file (for v2 signing) to facilitate reading of keyid from the associated cert. This will allow users to have private and public key as a single file. There is no check that public key form the cert matches associated private key. Signed-off-by: Vitaly Chikunov <vt@altlinux.org> --- README | 3 +++ src/libimaevm.c | 8 +++++--- 2 files changed, 8 insertions(+), 3 deletions(-)