Re: [PATCH v7 3/3] ima-evm-utils: Read keyid from the cert appended to the key file

[Vitaly Chikunov <vt@altlinux.org>]

Mimi,

On Mon, Jul 05, 2021 at 04:04:52PM -0400, Mimi Zohar wrote:
> On Thu, 2021-07-01 at 04:13 +0300, Vitaly Chikunov wrote:
> > 
> > +/**
> > + * read_keyid_from_key() - Read 32-bit keyid from the key file
> > + * @keyid_be:   Pointer to 32-bit value in network order (BE, unaligned).
> > + * @keyfile:    PEM file with private key with optionally appended x509 cert.
> > + * Return:      0 on success and keyid_be is written;
> > + *              -1 on error, logged error message, and keyid_be isn't written.
> > + */
> > +static int read_keyid_from_key(uint32_t *keyid_be, const char *keyfile)
> 
> (With  the new option "--keyid-from-cert" is this patch really still
> needed?)

Yes. Key+cert is a nice option and should be handy for users. Key is
stored together with the cert which will verify them. Otherwise key
format doesn't store keyid which is error prone.

> The function name is a bit off.  Both imaevm_read_keyid() and this
> function are getting the keyid from a cert.  There's also quite a bit
> of code duplication between them.  Refactoring the code might help. 
> For example, perhaps imaevm_read_keyid() could be a wrapper for
> read_keyid_from_cert().

They have important difference too. Thus, its hard to refactor them
into nested function that looked good and simple. This is third attempt.
And it's like solving a unsolvable puzzle.

imaevm_read_keyid() reads from cert-only file where cert could be PEM or
DER encoded. It's failure if there is no cert, because user intended to
read a cert.

imaevm_read_keyid() reads from private key optionally combined with a
cert (both are PEM-only). It's not failure if there is no cert.

We want to save "duplicated" call to PEM_read_X509() but really it
causes more convoluted internal logic.

I tried to de-duplicate them as much as possible while remaining
understandability.

> Otherwise renaming this function to read_keyid_from_keyfile(),
> read_appended_keyid() or read_appended_keyid_from_cert(), which is
> really wordy, would be better.

Ok. read_keyid_from_keyfile looks good.

> 
> thanks,
> 
> Mimi