[PATCH] Implement PKCS#11 opaque keys support through OpenSSL pkcs11 engine

[PATCH] Implement PKCS#11 opaque keys support through OpenSSL pkcs11 engine

[Aleksander Adamowski]

PKCS#11 API allows us to use opaque keys confined in hardware security
modules (HSMs) and similar hardware tokens without direct access to the
key material, providing logical separation of the keys from the
cryptographic operations performed using them.

This commit allows using the popular libp11 pkcs11 module for the
OpenSSL library with `fsverity` so that direct access to a private key
file isn't necessary to sign files.

The user needs to supply the path to the engine shared library
(typically libp11 shared object file) and the PKCS#11 module library (a
shared object file specific to the given hardware token).

Additionally, the existing `key` argument can be used to pass an
optional token-specific key identifier (instead of a private key file
name) for tokens that can contain multiple keys.

Test evidence with a hardware PKCS#11 token:

  $ echo test > dummy
  $ ./fsverity sign dummy dummy.sig \
    --pkcs11-engine=/usr/lib64/engines-1.1/libpkcs11.so \
    --pkcs11-module=/usr/local/lib64/pkcs11_module.so \
    --cert=test-pkcs11-cert.pem && echo OK;
  Signed file 'dummy'
  (sha256:c497326752e21b3992b57f7eff159102d474a97d972dc2c2d99d23e0f5fbdb65)
  OK

Test evidence for regression check (checking that regular file-based key
signing still works):

  $ ./fsverity sign dummy dummy.sig --key=key.pem --cert=cert.pem && \
    echo  OK;
  Signed file 'dummy'
  (sha256:c497326752e21b3992b57f7eff159102d474a97d972dc2c2d99d23e0f5fbdb65)
  OK

Signed-off-by: Aleksander Adamowski <olo@fb.com>
---
 include/libfsverity.h |  9 ++++-
 lib/sign_digest.c     | 94 +++++++++++++++++++++++++++++++++++++++++++
 programs/cmd_sign.c   | 57 ++++++++++++++++++++++++++
 programs/fsverity.c   |  6 ++-
 programs/fsverity.h   |  2 +
 5 files changed, 166 insertions(+), 2 deletions(-)

diff --git a/include/libfsverity.h b/include/libfsverity.h
index 6cefa2b..4b27b3a 100644
--- a/include/libfsverity.h
+++ b/include/libfsverity.h
@@ -82,8 +82,15 @@ struct libfsverity_digest {
 };
 
 struct libfsverity_signature_params {
-	const char *keyfile;		/* path to key file (PEM format) */
+	const char *keyfile; /* path to key file (PEM format), optional,
+				conflicts with pkcs11 */
 	const char *certfile;		/* path to certificate (PEM format) */
+#ifndef OPENSSL_IS_BORINGSSL
+	const char *pkcs11_engine;	/* path to PKCS#11 engine .so, optional,
+					   conflicts with *keyfile */
+	const char *pkcs11_module;	/* path to PKCS#11 module .so, optional,
+					   conflicts with *keyfile */
+#endif
 	uint64_t reserved1[8];		/* must be 0 */
 	uintptr_t reserved2[8];		/* must be 0 */
 };
diff --git a/lib/sign_digest.c b/lib/sign_digest.c
index 9a35256..1874541 100644
--- a/lib/sign_digest.c
+++ b/lib/sign_digest.c
@@ -17,6 +17,9 @@
 #include <openssl/err.h>
 #include <openssl/pem.h>
 #include <openssl/pkcs7.h>
+#ifndef OPENSSL_IS_BORINGSSL
+#include <openssl/engine.h>
+#endif
 #include <string.h>
 
 static int print_openssl_err_cb(const char *str,
@@ -214,6 +217,58 @@ out:
 
 #else /* OPENSSL_IS_BORINGSSL */
 
+static ENGINE *get_engine(const char *pkcs11_engine, const char *pkcs11_module)
+{
+	ENGINE *engine;
+
+	ENGINE_load_dynamic();
+	engine = ENGINE_by_id("dynamic");
+	if (!engine) {
+		error_msg_openssl(
+		    "failed to initialize OpenSSL PKCS#11 engine");
+		return NULL;
+	}
+	if (!ENGINE_ctrl_cmd_string(engine, "SO_PATH", pkcs11_engine, 0) ||
+	    !ENGINE_ctrl_cmd_string(engine, "ID", "pkcs11", 0) ||
+	    !ENGINE_ctrl_cmd_string(engine, "LIST_ADD", "1", 0) ||
+	    !ENGINE_ctrl_cmd_string(engine, "LOAD", NULL, 0) ||
+	    !ENGINE_ctrl_cmd_string(engine, "MODULE_PATH", pkcs11_module, 0) ||
+	    !ENGINE_init(engine)) {
+		error_msg_openssl(
+		    "failed to initialize OpenSSL PKCS#11 engine");
+		ENGINE_free(engine);
+		return NULL;
+	}
+	/*
+	 * engine now holds a functional reference after ENGINE_init(), free
+	 * the structural reference from ENGINE_by_id()
+	 */
+	ENGINE_free(engine);
+	return engine;
+}
+
+static int read_pkcs11_token(ENGINE *engine, const char *key_id,
+			     EVP_PKEY **pkey_ret)
+{
+	EVP_PKEY *pkey;
+	int err;
+
+	pkey = ENGINE_load_private_key(engine, key_id, NULL, NULL);
+	if (!pkey) {
+		error_msg_openssl(
+		    "failed to load private key from PKCS#11 token");
+		err = -EINVAL;
+		goto out;
+	} else {
+		*pkey_ret = pkey;
+		err = 0;
+	}
+out:
+	// Free the functional reference obtained from ENGINE_init()
+	ENGINE_finish(engine);
+	return err;
+}
+
 static BIO *new_mem_buf(const void *buf, size_t size)
 {
 	BIO *bio;
@@ -334,10 +389,20 @@ libfsverity_sign_digest(const struct libfsverity_digest *digest,
 		return -EINVAL;
 	}
 
+#ifdef OPENSSL_IS_BORINGSSL
 	if (!sig_params->keyfile || !sig_params->certfile) {
 		libfsverity_error_msg("keyfile and certfile must be specified");
+	}
+#else
+	if ((!sig_params->keyfile &&
+	     (!sig_params->pkcs11_engine || !sig_params->pkcs11_module)) ||
+	    !sig_params->certfile) {
+		libfsverity_error_msg(
+		    "(keyfile or pkcs11_engine and pkcs11_module) and certfile "
+		    "must be specified");
 		return -EINVAL;
 	}
+#endif
 
 	if (!libfsverity_mem_is_zeroed(sig_params->reserved1,
 				       sizeof(sig_params->reserved1)) ||
@@ -353,9 +418,38 @@ libfsverity_sign_digest(const struct libfsverity_digest *digest,
 		return -EINVAL;
 	}
 
+#ifdef OPENSSL_IS_BORINGSSL
 	err = read_private_key(sig_params->keyfile, &pkey);
 	if (err)
 		goto out;
+#else
+	if (sig_params->pkcs11_engine && sig_params->pkcs11_module) {
+		ENGINE *engine;
+		engine = get_engine(sig_params->pkcs11_engine,
+				    sig_params->pkcs11_module);
+		if (!engine) {
+			err = -EINVAL;
+			goto out;
+		}
+		/*
+		 * We overload the keyfile arg as an optional PKCS#11 key
+		 * identifier (NULL will cause engine to use the default key
+		 * from the token)
+		 */
+		err = read_pkcs11_token(engine, sig_params->keyfile, &pkey);
+		if (err)
+			goto out;
+	} else if (sig_params->keyfile) {
+		err = read_private_key(sig_params->keyfile, &pkey);
+		if (err)
+			goto out;
+
+	} else {
+		libfsverity_error_msg("Either private key or both pkcs11 "
+				      "params need to be provided");
+		return -EINVAL;
+	}
+#endif
 
 	err = read_certificate(sig_params->certfile, &cert);
 	if (err)
diff --git a/programs/cmd_sign.c b/programs/cmd_sign.c
index 81a4ddc..81de18c 100644
--- a/programs/cmd_sign.c
+++ b/programs/cmd_sign.c
@@ -32,8 +32,16 @@ static const struct option longopts[] = {
 	{"salt",	    required_argument, NULL, OPT_SALT},
 	{"out-merkle-tree", required_argument, NULL, OPT_OUT_MERKLE_TREE},
 	{"out-descriptor",  required_argument, NULL, OPT_OUT_DESCRIPTOR},
+#ifdef OPENSSL_IS_BORINGSSL
 	{"key",		    required_argument, NULL, OPT_KEY},
+#else
+	{"key",		    optional_argument, NULL, OPT_KEY},
+#endif
 	{"cert",	    required_argument, NULL, OPT_CERT},
+#ifndef OPENSSL_IS_BORINGSSL
+	{"pkcs11-engine",	    optional_argument, NULL, OPT_PKCS11_ENGINE},
+	{"pkcs11-module",	    optional_argument, NULL, OPT_PKCS11_MODULE},
+#endif
 	{NULL, 0, NULL, 0}
 };
 
@@ -66,8 +74,34 @@ int fsverity_cmd_sign(const struct fsverity_command *cmd,
 				error_msg("--key can only be specified once");
 				goto out_usage;
 			}
+#ifndef OPENSSL_IS_BORINGSSL
+			if (sig_params.pkcs11_engine != NULL ||
+			    sig_params.pkcs11_module != NULL) {
+				error_msg("--key cannot be specified when "
+					  "PKCS#11 is in use");
+				goto out_usage;
+			}
+#endif
 			sig_params.keyfile = optarg;
 			break;
+#ifndef OPENSSL_IS_BORINGSSL
+		case OPT_PKCS11_ENGINE:
+			if (sig_params.keyfile != NULL) {
+				error_msg("--pkcs11-engine cannot be specified "
+					  "when on-disk --key is in use");
+				goto out_usage;
+			}
+			sig_params.pkcs11_engine = optarg;
+			break;
+		case OPT_PKCS11_MODULE:
+			if (sig_params.keyfile != NULL) {
+				error_msg("--pkcs11-module cannot be specified "
+					  "when on-disk --key is in use");
+				goto out_usage;
+			}
+			sig_params.pkcs11_module = optarg;
+			break;
+#endif
 		case OPT_CERT:
 			if (sig_params.certfile != NULL) {
 				error_msg("--cert can only be specified once");
@@ -86,12 +120,35 @@ int fsverity_cmd_sign(const struct fsverity_command *cmd,
 	if (argc != 2)
 		goto out_usage;
 
+#ifdef OPENSSL_IS_BORINGSSL
 	if (sig_params.keyfile == NULL) {
 		error_msg("Missing --key argument");
 		goto out_usage;
 	}
 	if (sig_params.certfile == NULL)
 		sig_params.certfile = sig_params.keyfile;
+#else
+	if (sig_params.keyfile == NULL && sig_params.pkcs11_engine == NULL &&
+	    sig_params.pkcs11_module == NULL) {
+		error_msg("Missing --key argument or a pair of --pkcs11-engine "
+			  "and --pkcs11-module");
+		goto out_usage;
+	}
+	if (sig_params.certfile == NULL) {
+		if (sig_params.keyfile == NULL) {
+			error_msg(
+			    "--cert must be specified when PKCS#11 is used");
+			goto out_usage;
+		}
+		sig_params.certfile = sig_params.keyfile;
+	}
+	if ((sig_params.pkcs11_engine == NULL) !=
+	    (sig_params.pkcs11_module == NULL)) {
+		error_msg("Both --pkcs11-engine and --pkcs11-module must be "
+			  "specified when used");
+		goto out_usage;
+	}
+#endif
 
 	if (!open_file(&file, argv[0], O_RDONLY, 0))
 		goto out_err;
diff --git a/programs/fsverity.c b/programs/fsverity.c
index f6aff3a..485aade 100644
--- a/programs/fsverity.c
+++ b/programs/fsverity.c
@@ -58,7 +58,11 @@ static const struct fsverity_command {
 		.func = fsverity_cmd_sign,
 		.short_desc = "Sign a file for fs-verity",
 		.usage_str =
-"    fsverity sign FILE OUT_SIGFILE --key=KEYFILE\n"
+"    fsverity sign FILE OUT_SIGFILE [--key=KEYFILE]\n"
+#ifndef OPENSSL_IS_BORINGSSL
+"               [--pkcs11-engine=PATH_TO_OPENSSL_ENGINE]\n"
+"               [--pkcs11-module=PATH_TO_OPENSSL_MODULE]\n"
+#endif
 "               [--hash-alg=HASH_ALG] [--block-size=BLOCK_SIZE] [--salt=SALT]\n"
 "               [--out-merkle-tree=FILE] [--out-descriptor=FILE]\n"
 "               [--cert=CERTFILE]\n"
diff --git a/programs/fsverity.h b/programs/fsverity.h
index fe24087..eb5ba33 100644
--- a/programs/fsverity.h
+++ b/programs/fsverity.h
@@ -33,6 +33,8 @@ enum {
 	OPT_OUT_MERKLE_TREE,
 	OPT_SALT,
 	OPT_SIGNATURE,
+	OPT_PKCS11_ENGINE,
+	OPT_PKCS11_MODULE,
 };
 
 struct fsverity_command;
-- 
2.30.2

Re: [PATCH] Implement PKCS#11 opaque keys support through OpenSSL pkcs11 engine

[Eric Biggers]

Hi Aleksander,

First, can you make sure to include "[fsverity-utils PATCH]" in the subject like
the fsverity-utils README file suggests?  I almost missed this patch as it
initially didn't look relevant to me.

I'm not particularly familiar with the OpenSSL PKCS#11 engine, but this patch
looks reasonable at a high level (assuming that you really want to use the
kernel's built-in fs-verity signature verification support -- I've been trying
to encourage people to do userspace signature verification instead).  Some
comments on the implementation below.

On Wed, Aug 25, 2021 at 05:13:46PM -0700, Aleksander Adamowski wrote:
> PKCS#11 API allows us to use opaque keys confined in hardware security
> modules (HSMs) and similar hardware tokens without direct access to the
> key material, providing logical separation of the keys from the
> cryptographic operations performed using them.
> 
> This commit allows using the popular libp11 pkcs11 module for the
> OpenSSL library with `fsverity` so that direct access to a private key
> file isn't necessary to sign files.
> 
> The user needs to supply the path to the engine shared library
> (typically libp11 shared object file) and the PKCS#11 module library (a
> shared object file specific to the given hardware token).
> 
> Additionally, the existing `key` argument can be used to pass an
> optional token-specific key identifier (instead of a private key file
> name) for tokens that can contain multiple keys.
> 
> Test evidence with a hardware PKCS#11 token:
> 
>   $ echo test > dummy
>   $ ./fsverity sign dummy dummy.sig \
>     --pkcs11-engine=/usr/lib64/engines-1.1/libpkcs11.so \
>     --pkcs11-module=/usr/local/lib64/pkcs11_module.so \
>     --cert=test-pkcs11-cert.pem && echo OK;
>   Signed file 'dummy'
>   (sha256:c497326752e21b3992b57f7eff159102d474a97d972dc2c2d99d23e0f5fbdb65)
>   OK

Please update the man page (man/fsverity.1.md) to document these new options.

> 
> Test evidence for regression check (checking that regular file-based key
> signing still works):
> 
>   $ ./fsverity sign dummy dummy.sig --key=key.pem --cert=cert.pem && \
>     echo  OK;
>   Signed file 'dummy'
>   (sha256:c497326752e21b3992b57f7eff159102d474a97d972dc2c2d99d23e0f5fbdb65)
>   OK
> 
> Signed-off-by: Aleksander Adamowski <olo@fb.com>
> ---
>  include/libfsverity.h |  9 ++++-
>  lib/sign_digest.c     | 94 +++++++++++++++++++++++++++++++++++++++++++
>  programs/cmd_sign.c   | 57 ++++++++++++++++++++++++++
>  programs/fsverity.c   |  6 ++-
>  programs/fsverity.h   |  2 +
>  5 files changed, 166 insertions(+), 2 deletions(-)
> 
> diff --git a/include/libfsverity.h b/include/libfsverity.h
> index 6cefa2b..4b27b3a 100644
> --- a/include/libfsverity.h
> +++ b/include/libfsverity.h
> @@ -82,8 +82,15 @@ struct libfsverity_digest {
>  };
>  
>  struct libfsverity_signature_params {
> -	const char *keyfile;		/* path to key file (PEM format) */
> +	const char *keyfile; /* path to key file (PEM format), optional,
> +				conflicts with pkcs11 */

This comment is incorrect, as your code uses keyfile even in the pkcs11 case.

Also, keyfile is only optional in the pkcs11 case.  Please write a comment that
clearly explains which parameters must be specified and when.

>  	const char *certfile;		/* path to certificate (PEM format) */
> +#ifndef OPENSSL_IS_BORINGSSL
> +	const char *pkcs11_engine;	/* path to PKCS#11 engine .so, optional,
> +					   conflicts with *keyfile */
> +	const char *pkcs11_module;	/* path to PKCS#11 module .so, optional,
> +					   conflicts with *keyfile */
> +#endif
>  	uint64_t reserved1[8];		/* must be 0 */
>  	uintptr_t reserved2[8];		/* must be 0 */
>  };

Inserting these new fields into the middle of the struct breaks the library ABI.
They need to be placed in reserved2 instead, and the size of reserved2 needs to
be decreased accordingly.

> +static ENGINE *get_engine(const char *pkcs11_engine, const char *pkcs11_module)
> +{

Calling this get_pkcs11_engine() would be clearer.

> +static int read_pkcs11_token(ENGINE *engine, const char *key_id,
> +			     EVP_PKEY **pkey_ret)
> +{
> +	EVP_PKEY *pkey;
> +	int err;
> +
> +	pkey = ENGINE_load_private_key(engine, key_id, NULL, NULL);
> +	if (!pkey) {
> +		error_msg_openssl(
> +		    "failed to load private key from PKCS#11 token");
> +		err = -EINVAL;
> +		goto out;
> +	} else {
> +		*pkey_ret = pkey;
> +		err = 0;
> +	}
> +out:
> +	// Free the functional reference obtained from ENGINE_init()
> +	ENGINE_finish(engine);
> +	return err;
> +}
> +
>  static BIO *new_mem_buf(const void *buf, size_t size)
>  {
>  	BIO *bio;
> @@ -334,10 +389,20 @@ libfsverity_sign_digest(const struct libfsverity_digest *digest,
>  		return -EINVAL;
>  	}
>  
> +#ifdef OPENSSL_IS_BORINGSSL
>  	if (!sig_params->keyfile || !sig_params->certfile) {
>  		libfsverity_error_msg("keyfile and certfile must be specified");
> +	}
> +#else
> +	if ((!sig_params->keyfile &&
> +	     (!sig_params->pkcs11_engine || !sig_params->pkcs11_module)) ||
> +	    !sig_params->certfile) {
> +		libfsverity_error_msg(
> +		    "(keyfile or pkcs11_engine and pkcs11_module) and certfile "
> +		    "must be specified");
>  		return -EINVAL;
>  	}
> +#endif
>  
>  	if (!libfsverity_mem_is_zeroed(sig_params->reserved1,
>  				       sizeof(sig_params->reserved1)) ||
> @@ -353,9 +418,38 @@ libfsverity_sign_digest(const struct libfsverity_digest *digest,
>  		return -EINVAL;
>  	}
>  
> +#ifdef OPENSSL_IS_BORINGSSL
>  	err = read_private_key(sig_params->keyfile, &pkey);
>  	if (err)
>  		goto out;
> +#else
> +	if (sig_params->pkcs11_engine && sig_params->pkcs11_module) {
> +		ENGINE *engine;
> +		engine = get_engine(sig_params->pkcs11_engine,
> +				    sig_params->pkcs11_module);
> +		if (!engine) {
> +			err = -EINVAL;
> +			goto out;
> +		}
> +		/*
> +		 * We overload the keyfile arg as an optional PKCS#11 key
> +		 * identifier (NULL will cause engine to use the default key
> +		 * from the token)
> +		 */
> +		err = read_pkcs11_token(engine, sig_params->keyfile, &pkey);
> +		if (err)
> +			goto out;
> +	} else if (sig_params->keyfile) {
> +		err = read_private_key(sig_params->keyfile, &pkey);
> +		if (err)
> +			goto out;
> +
> +	} else {
> +		libfsverity_error_msg("Either private key or both pkcs11 "
> +				      "params need to be provided");
> +		return -EINVAL;
> +	}
> +#endif

There are several bugs here.  The !OPENSSL_IS_BORINGSSL case no longer returns
an error if sig_params->keyfile or sig_params->certfile is unset, and it doesn't
return an error if the pkcs11 parameters are set.  Also, either case fails to
return an error if pkcs11_engine is set but not pkcs11_module, or vice versa.

I think it would be best to create a new function get_private_key() that just
handles getting the private key correctly.  Something like the following:

static int
get_private_key(const struct libfsverity_signature_params *sig_params,
		EVP_PKEY **pkey_ret)
{
	if (sig_params->pkcs11_engine || sig_params->pkcs11_module) {
#ifdef OPENSSL_IS_BORINGSSL
		libfsverity_error_msg("BoringSSL doesn't support PKCS#11 feature");
		return -EINVAL;
#else
		ENGINE *engine;

		if (!sig_params->pkcs11_engine) {
			libfsverity_error_msg("missing PKCS#11 engine parameter");
			return -EINVAL;
		}
		if (!sig_params->pkcs11_module) {
			libfsverity_error_msg("missing PKCS#11 module parameter");
			return -EINVAL;
		}
		engine = get_pkcs11_engine(sig_params->pkcs11_engine,
					   sig_params->pkcs11_module);
		if (!engine)
			return -EINVAL;
		/*
		 * We overload the keyfile parameter as an optional PKCS#11 key
		 * identifier.  NULL will cause the engine to use the default
		 * key from the token.
		 */
		*pkey_ret = ENGINE_load_private_key(engine, sig_params->keyfile,
						    NULL, NULL);
		ENGINE_finish(engine);
		if (!*pkey_ret) {
			error_msg_openssl("failed to load private key from PKCS#11 token");
			return -EINVAL;
		}
		return 0;
#endif
	}
	if (!sig_params->keyfile) {
		error_msg_openssl("missing keyfile parameter (or PKCS11 parameters)");
		return -EINVAL;
	}
	return read_private_key(sig_params->keyfile, pkey_ret);
}


The NULL check of certfile can be moved into read_certificate().

Then libfsverity_sign_digest() itself would be pretty simple:

	err = read_certificate(sig_params->certfile, &cert);
	if (err)
		goto out;

	err = get_private_key(sig_params, &pkey);
	if (err)
		goto out;

> diff --git a/programs/cmd_sign.c b/programs/cmd_sign.c
> index 81a4ddc..81de18c 100644
> --- a/programs/cmd_sign.c
> +++ b/programs/cmd_sign.c
> @@ -32,8 +32,16 @@ static const struct option longopts[] = {
>  	{"salt",	    required_argument, NULL, OPT_SALT},
>  	{"out-merkle-tree", required_argument, NULL, OPT_OUT_MERKLE_TREE},
>  	{"out-descriptor",  required_argument, NULL, OPT_OUT_DESCRIPTOR},
> +#ifdef OPENSSL_IS_BORINGSSL
>  	{"key",		    required_argument, NULL, OPT_KEY},
> +#else
> +	{"key",		    optional_argument, NULL, OPT_KEY},
> +#endif

The --key option still requires an argument in both cases, so it should still be
"required_argument".  I think what you're trying to say is that the --key option
is not required in the !OPENSSL_IS_BORINGSSL case.  That's something different;
it's handled in the code that processes the options.

Another thing to keep in mind is that the 'fsverity' binary might not know
whether 'libfsverity' is linked to BoringSSL or OpenSSL.  So any use of
OPENSSL_IS_BORINGSSL should be avoided in the programs/ directory.

>  	{"cert",	    required_argument, NULL, OPT_CERT},
> +#ifndef OPENSSL_IS_BORINGSSL
> +	{"pkcs11-engine",	    optional_argument, NULL, OPT_PKCS11_ENGINE},
> +	{"pkcs11-module",	    optional_argument, NULL, OPT_PKCS11_MODULE},
> +#endif
>  	{NULL, 0, NULL, 0}
>  };

Likewise, using OPENSSL_IS_BORINGSSL isn't valid here.

> +#ifndef OPENSSL_IS_BORINGSSL
> +		case OPT_PKCS11_ENGINE:
> +			if (sig_params.keyfile != NULL) {
> +				error_msg("--pkcs11-engine cannot be specified "
> +					  "when on-disk --key is in use");
> +				goto out_usage;
> +			}
> +			sig_params.pkcs11_engine = optarg;
> +			break;
> +		case OPT_PKCS11_MODULE:
> +			if (sig_params.keyfile != NULL) {
> +				error_msg("--pkcs11-module cannot be specified "
> +					  "when on-disk --key is in use");
> +				goto out_usage;
> +			}
> +			sig_params.pkcs11_module = optarg;
> +			break;
> +#endif

This seems to contradict elsewhere in the patch where keyfile is used even in
the PKCS#11 case.

Also as mentioned above, it isn't really valid to use OPENSSL_IS_BORINGSSL here.

- Eric

Re: [PATCH] Implement PKCS#11 opaque keys support through OpenSSL pkcs11 engine

[Aleksander Adamowski]

Hi Eric!

Thanks for the quick response to the patch!

My replies inline below:

On Thursday, August 26, 2021 12:11 PM, Eric Biggers wrote:

> First, can you make sure to include "[fsverity-utils PATCH]" in the subject like
> the fsverity-utils README file suggests?  I almost missed this patch as it
> initially didn't look relevant to me.

Sure! Apologies for missing that part. The V2 of the patch will be appropriately tagged.
 
> I'm not particularly familiar with the OpenSSL PKCS#11 engine, but this patch
> looks reasonable at a high level (assuming that you really want to use the
> kernel's built-in fs-verity signature verification support -- I've been trying
> to encourage people to do userspace signature verification instead).

We are currently going forward with in-kernel sig verification (and btrfs), but
I'd love to hear more about the userspace support you mention.

> Some
> comments on the implementation below.
> 

> This comment is incorrect, as your code uses keyfile even in the pkcs11 case.
> 
> Also, keyfile is only optional in the pkcs11 case.  Please write a comment that
> clearly explains which parameters must be specified and when.

Yes, you are entirely right about this. The decision to use the --key argument
as an optional PKCS#11 key identified was a later afterthought and I forgot
to update some related pieces of code.

Will fix in the next patch, as well as address your other comments.

Re: [PATCH] Implement PKCS#11 opaque keys support through OpenSSL pkcs11 engine

[Eric Biggers]

On Sat, Aug 28, 2021 at 01:03:10AM +0000, Aleksander Adamowski wrote:
> Hi Eric!
> > I'm not particularly familiar with the OpenSSL PKCS#11 engine, but this patch
> > looks reasonable at a high level (assuming that you really want to use the
> > kernel's built-in fs-verity signature verification support -- I've been trying
> > to encourage people to do userspace signature verification instead).
> 
> We are currently going forward with in-kernel sig verification (and btrfs), but
> I'd love to hear more about the userspace support you mention.
> 

Well, there isn't much to explain about it.  Userspace could just store whatever
signature it wants to in a separate file or in an xattr, and verify it at the
same time it checks the fs-verity bit which it must already be doing.  Then
there's no need for PKCS#7 or RSA in the kernel, and any signature algorithms
could be used -- not just the ones the kernel supports.  Also no need for
PKCS#7; something simpler could be used.

In retrospect I probably shouldn't have implemented the in-kernel signature
verification at all, as now everyone wants to use it even though it's a bad
design and was just meant as a proof of concept.  They see it and think "I want
signatures, so I'll use it", without considering better ways to do signatures.

- Eric