Re: [RFC PATCH ima-evm-utils 04/11] Deprecate IMA signature version 1

[Petr Vorel <pvorel@suse.cz>]

> The original IMA file signatures were based on a SHA1 hash.  Kernel
> support for other hash algorithms was subsequently upstreamed.  Deprecate
> "--rsa" support.

> Define "--enable-sigv1" option to configure signature v1 support.

LGTM, few minor comments below.

Reviewed-by: Petr Vorel <pvorel@suse.cz>

Kind regards,
Petr

...
> +++ b/configure.ac
...

>  static int cmd_convert(struct command *cmd)
>  {
> +#if CONFIG_SIGV1
>  	char *inkey;
>  	unsigned char _pub[1024], *pub = _pub;
>  	int len, err = 0;
> @@ -1033,6 +1033,8 @@ static int cmd_convert(struct command *cmd)

>  	RSA_free(key);
>  	return err;
> +#endif
> +	return 77;
What is this this magic number? EBADFD?
Well, git grep shows many places with 77, so it's just a tip for next cleanup :).

...
>  	log_info("Importing public key %s from file %s into keyring %d\n", name, inkey, id);
> @@ -2598,7 +2605,8 @@ static void usage(void)
>  		"  -d, --imahash      make IMA hash\n"
>  		"  -f, --sigfile      store IMA signature in .sig file instead of xattr\n"
>  		"      --xattr-user   store xattrs in user namespace (for testing purposes)\n"
> -		"      --rsa          use RSA key type and signing scheme v1\n"
> +
nit: was this blank line intentional?
> +		"      --rsa          use RSA key type and signing scheme v1 (deprecated)\n"
>  		"  -k, --key          path to signing key (default: /etc/keys/{privkey,pubkey}_evm.pem)\n"
>  		"                     or a pkcs11 URI\n"
>  		"      --keyid n      overwrite signature keyid with a 32-bit value in hex (for signing)\n"
> @@ -2637,8 +2645,8 @@ static void usage(void)
>  struct command cmds[] = {
>  	{"--version", NULL, 0, ""},
>  	{"help", cmd_help, 0, "<command>"},
> -	{"import", cmd_import, 0, "[--rsa] pubkey keyring", "Import public key into the keyring.\n"},
> -	{"convert", cmd_convert, 0, "key", "convert public key into the keyring.\n"},
> +	{"import", cmd_import, 0, "[--rsa] pubkey keyring", "Import public key into the keyring. (deprecated)\n"},
> +	{"convert", cmd_convert, 0, "key", "convert public key into the keyring. (deprecated)\n"},
>  	{"sign", cmd_sign_evm, 0, "[-r] [--imahash | --imasig ] [--key key] [--pass [password] file", "Sign file metadata.\n"},
>  	{"verify", cmd_verify_evm, 0, "file", "Verify EVM signature (for debugging).\n"},
>  	{"ima_sign", cmd_sign_ima, 0, "[--sigfile] [--key key] [--pass [password] file", "Make file content signature.\n"},
...
> +++ b/src/libimaevm.c
...

> +#if CONFIG_SIGV1
>  static RSA *read_priv_key(const char *keyfile, const char *keypass)
>  {
> +	RSA *key = NULL;
nit: NULL is safe, I wonder if it is necessary (was needed before).
>  	EVP_PKEY *pkey;
> -	RSA *key;

>  	pkey = read_priv_pkey(keyfile, keypass);
>  	if (!pkey)
> @@ -1018,10 +1034,12 @@ static int get_hash_algo_v1(const char *algo)

>  	return -1;
>  }
> +#endif