Re: [PATCH v4 2/3] ima-evm-utils: Allow manual setting keyid from a cert file

[Vitaly Chikunov <vt@altlinux.org>]

Stefan,

On Wed, May 05, 2021 at 10:24:48PM -0400, Stefan Berger wrote:
> On 5/5/21 9:07 PM, Vitaly Chikunov wrote:
> > On Thu, May 06, 2021 at 03:54:53AM +0300, Vitaly Chikunov wrote:
> > > On Wed, May 05, 2021 at 07:13:39PM -0400, Stefan Berger wrote:
> > > > On 5/5/21 2:48 AM, Vitaly Chikunov wrote:
> > > > > Allow user to specify `--keyid @/path/to/cert.pem' to extract keyid from
> > > > > SKID of the certificate file. PEM or DER format is auto-detected.
> > > > > 
> > > > > `--keyid' option is reused instead of adding a new option (like possible
> > > > > `--cert') to signify to the user it's only keyid extraction and nothing
> > > > > more.
> > > > > 
> > > > > This commit creates ABI change for libimaevm, due to adding new function
> > > > > ima_read_keyid(). Newer clients cannot work with older libimaevm.
> > > > > Together with previous commit it creates backward-incompatible ABI
> > > > > change, thus soname should be incremented on release.
> > > > > 
> > > > > Signed-off-by: Vitaly Chikunov <vt@altlinux.org>
> > > > > ---
> > > > >    README                 |  1 +
> > > > >    src/evmctl.c           | 22 ++++++++++---
> > > > >    src/imaevm.h           |  1 +
> > > > >    src/libimaevm.c        | 85 ++++++++++++++++++++++++++++++++++++++++++++++++++
> > > > >    tests/sign_verify.test |  1 +
> > > > >    5 files changed, 105 insertions(+), 5 deletions(-)
> > > > > 
> > > > > +/**
> > > > > + * ima_read_keyid() - Read 32-bit keyid from the cert file.
> > > > > + * @certfile:	File possibly containing certificate in DER/PEM format.
> > > > > + * @keyid:	Output keyid in network order.
> > > > > + *
> > > > > + * Try to read keyid from Subject Key Identifier (SKID) of certificate.
> > > > > + * Autodetect if cert is in PEM or DER encoding.
> > > > > + *
> > > > > + * Return: -1 (ULONG_MAX) on error;
> > > > > + *         32-bit keyid as unsigned integer in host order.
> > > > That's confusing, two times the same result, one time in host order, on time
> > > > in network order. Why not just one return value in host order?
> > > Pointer API is similar to calc_keyid_v2().
> > > 
> > > Do you sugegst to change calc_keyid_v2() API too?
> > > 
> > > To introduce non-confusing API that contradict other parts of API would
> > > be more confusing than it already is.
> > Maybe we could change this libimaevm API:
> > 
> >    void calc_keyid_v2(uint32_t *keyid, char *str, EVP_PKEY *pkey);
> > 
> > to
> > 
> >    void calc_keyid_v2(uint8_t *keyid, char *str, EVP_PKEY *pkey);
> 
> 
> I think it's better to leave it... :-(

OK.

> 
> > 
> > To signal to the user that there it's not just uint32_t, but some byte
> > array (possible in network order). This would not even break ABI, only
> > API. (But, we breaking ABI with this patch set anyway.)
> 
> You mean we are breaking it by introducing this extensions here?

As described in commit messages - adding new API function breaks
compatibility of new clients with old lib, and adding keyid into
libimaevm_params breaks compatibility of new lib with old clients.
Thus, major ABI change.

> 
> @ -196,6 +196,7 @@ struct libimaevm_params {
>  	const char *hash_algo;
>  	const char *keyfile;
>  	const char *keypass;
> +	uint32_t keyid;		/* keyid overriding value, unless 0. */
>  };
> 
> > 
> > Thanks,
> >