Re: [PATCH v5 0/3] ima-evm-utils: Add --keyid option

[Stefan Berger <stefanb@linux.ibm.com>]

On 5/6/21 9:43 PM, Vitaly Chikunov wrote:
> Stefan,
>
> On Thu, May 06, 2021 at 04:10:25PM -0400, Stefan Berger wrote:
>> On 5/5/21 11:46 PM, Vitaly Chikunov wrote:
>>> Allow user to set signature's keyid using `--keyid' option. Keyid should
>>> correspond to SKID in certificate. When keyid is calculated using SHA-1
>>> in libimaevm it may mismatch keyid extracted by the kernel from SKID of
>>> certificate (the way public key is presented to the kernel), thus making
>>> signatures not verifiable. This may happen when certificate is using non
>>> SHA-1 SKID (see rfc7093) or just 'unique number' (see rfc5280 4.2.1.2).
>>> As a last resort user may specify arbitrary keyid using the new option.
>>> Certificate @filename could be used instead of the hex number. And,
>>> third option is to read keyid from the cert appended to the key file.
>>>
>>> These commits create backward incompatible ABI change for libimaevm,
>>>    thus soname should be incremented on release.
>> I hope this will not be forgotten about. Maybe it should be part of this
>> series here?
> https://www.gnu.org/software/libtool/manual/html_node/Updating-version-info.html
>
>    "Update the version information only immediately before a public
>    release of your software."
>
> I believe we should follow this.

As long as the maintainers are not forgetting about it...


One other thing is the naming of the function you are adding to the 
library. Here are the last few changes to imaevm.h:

+int imaevm_hash_algo_from_sig(unsigned char *sig);
+const char *imaevm_hash_algo_by_id(int algo);


@@ -204,12 +206,12 @@ struct RSA_ASN1_template {
  #define        NUM_PCRS 20
  #define DEFAULT_PCR 10

-extern struct libevm_params params;
+extern struct libimaevm_params imaevm_params;

-void do_dump(FILE *fp, const void *ptr, int len, bool cr);
-void dump(const void *ptr, int len);
+void imaevm_do_hexdump(FILE *fp, const void *ptr, int len, bool cr);
+void imaevm_hexdump(const void *ptr, int len);
  int ima_calc_hash(const char *file, uint8_t *hash);
-int get_hash_algo(const char *algo);
+int imaevm_get_hash_algo(const char *algo);
  RSA *read_pub_key(const char *keyfile, int x509);
  EVP_PKEY *read_pub_pkey(const char *keyfile, int x509);


It looks like the author (actually you) tried to establish some sort of 
namespace for the function with the prefix 'imaevm_'. Maybe the newly 
added one should also have that prefix?