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@xxxxxxxxxxxx>
> > > > > ---
> > > > > 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,
> >
