linux-fscrypt.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [fsverity-utils PATCH v2] Implement PKCS#11 opaque keys support through OpenSSL pkcs11 engine
@ 2021-08-28  1:30 Aleksander Adamowski
  2021-09-08 22:44 ` Eric Biggers
  0 siblings, 1 reply; 7+ messages in thread
From: Aleksander Adamowski @ 2021-08-28  1:30 UTC (permalink / raw)
  To: linux-fscrypt; +Cc: 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 |  6 ++-
 lib/sign_digest.c     | 96 ++++++++++++++++++++++++++++++++++++++++---
 man/fsverity.1.md     | 23 ++++++++++-
 programs/cmd_sign.c   | 31 ++++++++++++++
 programs/fsverity.c   |  4 +-
 programs/fsverity.h   |  2 +
 6 files changed, 151 insertions(+), 11 deletions(-)

diff --git a/include/libfsverity.h b/include/libfsverity.h
index 6cefa2b..4b34f43 100644
--- a/include/libfsverity.h
+++ b/include/libfsverity.h
@@ -82,10 +82,12 @@ 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 */
 	const char *certfile;		/* path to certificate (PEM format) */
 	uint64_t reserved1[8];		/* must be 0 */
-	uintptr_t reserved2[8];		/* must be 0 */
+	const char *pkcs11_engine;	/* path to PKCS#11 engine .so, optional */
+	const char *pkcs11_module;	/* path to PKCS#11 module .so, optional */
+	uintptr_t reserved2[6];		/* must be 0 */
 };
 
 struct libfsverity_metadata_callbacks {
diff --git a/lib/sign_digest.c b/lib/sign_digest.c
index 9a35256..f0830f3 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,
@@ -81,6 +84,10 @@ static int read_certificate(const char *certfile, X509 **cert_ret)
 	X509 *cert;
 	int err;
 
+	if (!certfile) {
+		libfsverity_error_msg("certfile must be specified");
+	}
+
 	errno = 0;
 	bio = BIO_new_file(certfile, "r");
 	if (!bio) {
@@ -214,6 +221,37 @@ out:
 
 #else /* OPENSSL_IS_BORINGSSL */
 
+static ENGINE *get_pkcs11_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 BIO *new_mem_buf(const void *buf, size_t size)
 {
 	BIO *bio;
@@ -317,6 +355,57 @@ out:
 
 #endif /* !OPENSSL_IS_BORINGSSL */
 
+/* Get a private key - either off disk or PKCS#11 token */
+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);
+}
+
 LIBEXPORT int
 libfsverity_sign_digest(const struct libfsverity_digest *digest,
 			const struct libfsverity_signature_params *sig_params,
@@ -334,11 +423,6 @@ libfsverity_sign_digest(const struct libfsverity_digest *digest,
 		return -EINVAL;
 	}
 
-	if (!sig_params->keyfile || !sig_params->certfile) {
-		libfsverity_error_msg("keyfile and certfile must be specified");
-		return -EINVAL;
-	}
-
 	if (!libfsverity_mem_is_zeroed(sig_params->reserved1,
 				       sizeof(sig_params->reserved1)) ||
 	    !libfsverity_mem_is_zeroed(sig_params->reserved2,
@@ -353,7 +437,7 @@ libfsverity_sign_digest(const struct libfsverity_digest *digest,
 		return -EINVAL;
 	}
 
-	err = read_private_key(sig_params->keyfile, &pkey);
+	err = get_private_key(sig_params, &pkey);
 	if (err)
 		goto out;
 
diff --git a/man/fsverity.1.md b/man/fsverity.1.md
index e1007f5..f44aeb0 100644
--- a/man/fsverity.1.md
+++ b/man/fsverity.1.md
@@ -169,8 +169,27 @@ Options accepted by **fsverity sign**:
 :   Same as for **fsverity digest**.
 
 **\-\-key**=*KEYFILE*
-:   Specifies the file that contains the private key, in PEM format.  This
-    option is required.
+:   Specifies the file that contains the private key, in PEM format.  If any
+    PKCS#11 options are used, it can be used instead to specify the key
+    identifier in the form of PKCS#11 URI.  This option is required when
+    private key is read from disk and optional when using a PKCS#11 token.
+
+**\-\-pkcs11-engine**=*SOFILE*
+:   Specifies the path to the OpenSSL engine library to be used, when a PKCS#11
+    cryptographic token is used instead of a private key file. Typically it
+    will be a path to the libp11 .so file.  This option is required when
+    **\-\-pkcs11-module** is used.
+
+    Note that this option is only supported with classical OpenSSL, and not
+    BoringSSL.
+
+**\-\-pkcs11-module**=*SOFILE*
+:   Specifies the path to the token-specific module library, when a PKCS#11
+    cryptographic token is used instead of a private key file.  This option is
+    required when **\-\-pkcs11-engine** is used.
+
+    Note that this option is only supported with classical OpenSSL, and not
+    BoringSSL.
 
 **\-\-out-descriptor**=*FILE*
 :   Same as for **fsverity digest**.
diff --git a/programs/cmd_sign.c b/programs/cmd_sign.c
index 81a4ddc..0d502c9 100644
--- a/programs/cmd_sign.c
+++ b/programs/cmd_sign.c
@@ -34,6 +34,8 @@ static const struct option longopts[] = {
 	{"out-descriptor",  required_argument, NULL, OPT_OUT_DESCRIPTOR},
 	{"key",		    required_argument, NULL, OPT_KEY},
 	{"cert",	    required_argument, NULL, OPT_CERT},
+	{"pkcs11-engine",	    required_argument, NULL, OPT_PKCS11_ENGINE},
+	{"pkcs11-module",	    required_argument, NULL, OPT_PKCS11_MODULE},
 	{NULL, 0, NULL, 0}
 };
 
@@ -68,6 +70,12 @@ int fsverity_cmd_sign(const struct fsverity_command *cmd,
 			}
 			sig_params.keyfile = optarg;
 			break;
+		case OPT_PKCS11_ENGINE:
+			sig_params.pkcs11_engine = optarg;
+			break;
+		case OPT_PKCS11_MODULE:
+			sig_params.pkcs11_module = optarg;
+			break;
 		case OPT_CERT:
 			if (sig_params.certfile != NULL) {
 				error_msg("--cert can only be specified once");
@@ -86,12 +94,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..a4e8f5b 100644
--- a/programs/fsverity.c
+++ b/programs/fsverity.c
@@ -58,7 +58,9 @@ 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"
+"               [--pkcs11-engine=PATH_TO_OPENSSL_ENGINE]\n"
+"               [--pkcs11-module=PATH_TO_OPENSSL_MODULE]\n"
 "               [--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


^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [fsverity-utils PATCH v2] Implement PKCS#11 opaque keys support through OpenSSL pkcs11 engine
  2021-08-28  1:30 [fsverity-utils PATCH v2] Implement PKCS#11 opaque keys support through OpenSSL pkcs11 engine Aleksander Adamowski
@ 2021-09-08 22:44 ` Eric Biggers
  2021-09-08 23:32   ` Aleksander Adamowski
  0 siblings, 1 reply; 7+ messages in thread
From: Eric Biggers @ 2021-09-08 22:44 UTC (permalink / raw)
  To: Aleksander Adamowski; +Cc: linux-fscrypt

On Fri, Aug 27, 2021 at 06:30:37PM -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.

Sorry, I didn't notice that you had already sent out a new version of this
patch.  Is this version intended to address all my comments?  Some of the
comments I made don't seem to have been fully addressed.

- Eric

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [fsverity-utils PATCH v2] Implement PKCS#11 opaque keys support through OpenSSL pkcs11 engine
  2021-09-08 22:44 ` Eric Biggers
@ 2021-09-08 23:32   ` Aleksander Adamowski
  2021-09-08 23:48     ` Eric Biggers
  0 siblings, 1 reply; 7+ messages in thread
From: Aleksander Adamowski @ 2021-09-08 23:32 UTC (permalink / raw)
  To: Eric Biggers; +Cc: linux-fscrypt

On Wednesday, September 8, 2021 3:44 PM, Eric Biggers wrote:
> Sorry, I didn't notice that you had already sent out a new version of this
> patch.  Is this version intended to address all my comments?  Some of the
> comments I made don't seem to have been fully addressed.

Hi!

Yes, the patch was intended to address all of your previous comment.

I went over it to double check and noticed that I somehow left one OPENSSL_IS_BORINGSSL ifdef in programs/cmd_sign.c.

I will remove it in V3. As far as I can tell, all of your other comments are already addressed, unless I'm still missing something?

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [fsverity-utils PATCH v2] Implement PKCS#11 opaque keys support through OpenSSL pkcs11 engine
  2021-09-08 23:32   ` Aleksander Adamowski
@ 2021-09-08 23:48     ` Eric Biggers
  2021-09-09  0:20       ` Aleksander Adamowski
  0 siblings, 1 reply; 7+ messages in thread
From: Eric Biggers @ 2021-09-08 23:48 UTC (permalink / raw)
  To: Aleksander Adamowski; +Cc: linux-fscrypt

On Wed, Sep 08, 2021 at 11:32:29PM +0000, Aleksander Adamowski wrote:
> On Wednesday, September 8, 2021 3:44 PM, Eric Biggers wrote:
> > Sorry, I didn't notice that you had already sent out a new version of this
> > patch.  Is this version intended to address all my comments?  Some of the
> > comments I made don't seem to have been fully addressed.
> 
> Hi!
> 
> Yes, the patch was intended to address all of your previous comment.
> 
> I went over it to double check and noticed that I somehow left one OPENSSL_IS_BORINGSSL ifdef in programs/cmd_sign.c.
> 
> I will remove it in V3. As far as I can tell, all of your other comments are already addressed, unless I'm still missing something?

Regarding struct libfsverity_signature_params, I wrote "Please write a comment
that clearly explains which parameters must be specified and when.".

Also I mentioned "The !OPENSSL_IS_BORINGSSL case no longer returns an error if
sig_params->keyfile or sig_params->certfile is unset".  That wasn't addressed
for sig_params->certfile.

- Eric

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [fsverity-utils PATCH v2] Implement PKCS#11 opaque keys support through OpenSSL pkcs11 engine
  2021-09-08 23:48     ` Eric Biggers
@ 2021-09-09  0:20       ` Aleksander Adamowski
  2021-09-09  0:28         ` Eric Biggers
  0 siblings, 1 reply; 7+ messages in thread
From: Aleksander Adamowski @ 2021-09-09  0:20 UTC (permalink / raw)
  To: Eric Biggers; +Cc: linux-fscrypt

On Wednesday, September 8, 2021 4:48 PM, Eric Biggers wrote:
> Regarding struct libfsverity_signature_params, I wrote "Please write a comment
> that clearly explains which parameters must be specified and when.".

Got it. I assumed that the detailed explanation in the manpage covering the
same parameters would be sufficient, as repeating it in struct comments would
make the information redundant and require reformatting that part to multi-line
comments.

I can add it to the struct comments, but this will mean I'll need to change
them to multi-line comments (above each struct member) and add empty lines
between members (following the same commenting style as in struct
libfsverity_merkle_tree_params). Are you okay with that change?

> Also I mentioned "The !OPENSSL_IS_BORINGSSL case no longer returns an error if
> sig_params->keyfile or sig_params->certfile is unset".  That wasn't addressed
> for sig_params->certfile.

Ah, I see. In my patch V2, after your suggestion, there's a new NULL check for
certfile in lib/sign_digest.c:87 that I intended as a replacement for the
previous check in lib/sign_digest.c:337. I think it's a better place for that
check, as it's in the place of actual use.

Do you want me to place that check back in the pre-check logic in
libfsverity_sign_digest()?

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [fsverity-utils PATCH v2] Implement PKCS#11 opaque keys support through OpenSSL pkcs11 engine
  2021-09-09  0:20       ` Aleksander Adamowski
@ 2021-09-09  0:28         ` Eric Biggers
  2021-09-09  0:55           ` Aleksander Adamowski
  0 siblings, 1 reply; 7+ messages in thread
From: Eric Biggers @ 2021-09-09  0:28 UTC (permalink / raw)
  To: Aleksander Adamowski; +Cc: linux-fscrypt

On Thu, Sep 09, 2021 at 12:20:35AM +0000, Aleksander Adamowski wrote:
> On Wednesday, September 8, 2021 4:48 PM, Eric Biggers wrote:
> > Regarding struct libfsverity_signature_params, I wrote "Please write a comment
> > that clearly explains which parameters must be specified and when.".
> 
> Got it. I assumed that the detailed explanation in the manpage covering the
> same parameters would be sufficient, as repeating it in struct comments would
> make the information redundant and require reformatting that part to multi-line
> comments.
> 
> I can add it to the struct comments, but this will mean I'll need to change
> them to multi-line comments (above each struct member) and add empty lines
> between members (following the same commenting style as in struct
> libfsverity_merkle_tree_params). Are you okay with that change?

The fsverity.1 man page documents the fsverity tool, whereas the comments in
libfsverity.h document the interface to libfsverity.  These are different
things, so the documentation for one doesn't really apply to the other, unless
they explicitly reference each other.  (Which you can do if it would avoid
redundancy.  The point is, if the documentation is somewhere else, people
actually need to be told where to find it.)

It's fine to reformat the comments and code if necessary.

> 
> > Also I mentioned "The !OPENSSL_IS_BORINGSSL case no longer returns an error if
> > sig_params->keyfile or sig_params->certfile is unset".  That wasn't addressed
> > for sig_params->certfile.
> 
> Ah, I see. In my patch V2, after your suggestion, there's a new NULL check for
> certfile in lib/sign_digest.c:87 that I intended as a replacement for the
> previous check in lib/sign_digest.c:337. I think it's a better place for that
> check, as it's in the place of actual use.
> 
> Do you want me to place that check back in the pre-check logic in
> libfsverity_sign_digest()?

Your patch has:

	if (!certfile) {
		libfsverity_error_msg("certfile must be specified");
	}

It needs to be:

	if (!certfile) {
		libfsverity_error_msg("certfile must be specified");
		return -EINVAL;
	}

- Eric

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [fsverity-utils PATCH v2] Implement PKCS#11 opaque keys support through OpenSSL pkcs11 engine
  2021-09-09  0:28         ` Eric Biggers
@ 2021-09-09  0:55           ` Aleksander Adamowski
  0 siblings, 0 replies; 7+ messages in thread
From: Aleksander Adamowski @ 2021-09-09  0:55 UTC (permalink / raw)
  To: Eric Biggers; +Cc: linux-fscrypt

On Wednesday, September 8, 2021 5:28 PM, Eric Biggers wrote:
>It's fine to reformat the comments and code if necessary.

Will do.

> It needs to be:
> 
>         if (!certfile) {
>                 libfsverity_error_msg("certfile must be specified");
>                 return -EINVAL;
>         }

{facepalm}

Argh, will fix that in V4 !
How did I miss that. Thanks for catching it!

^ permalink raw reply	[flat|nested] 7+ messages in thread

end of thread, other threads:[~2021-09-09  0:55 UTC | newest]

Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-08-28  1:30 [fsverity-utils PATCH v2] Implement PKCS#11 opaque keys support through OpenSSL pkcs11 engine Aleksander Adamowski
2021-09-08 22:44 ` Eric Biggers
2021-09-08 23:32   ` Aleksander Adamowski
2021-09-08 23:48     ` Eric Biggers
2021-09-09  0:20       ` Aleksander Adamowski
2021-09-09  0:28         ` Eric Biggers
2021-09-09  0:55           ` Aleksander Adamowski

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).