Hi Fabio,
On 7/21/26 4:13 PM, Fabio Estevam wrote:
From: Wojciech Dubowik <[email protected]>
Some distributions, such as OpenEmbedded, build GnuTLS without
PKCS #11 support, which causes mkeficapsule to fail at link time.
Detect whether GnuTLS was built with p11-kit support and enable
PKCS #11 functionality only when available. Emit a build warning
when mkeficapsule is built without this functionality.
When PKCS #11 is available, rely on GnuTLS and p11-kit for automatic
provider discovery, allowing PKCS #11 URIs to be used without the
PKCS11_MODULE_PATH environment variable.
Document the use of PKCS #11 URIs for private keys and certificates.
Suggested-by: Tom Rini <[email protected]>
Cc: Franz Schnyder <[email protected]>
Signed-off-by: Wojciech Dubowik <[email protected]>
Signed-off-by: Fabio Estevam <[email protected]>
Please list between Wojciech and your Signed-off-by what you changed
compared to their series.
---
Sorry for the resend. I forgot to CC the list.
Only built-tested.
Changes since v8:
- Removed Quentin's and Simon's review tags due to the significant changes
introduced in this version.
- Warn when mkeficapsule is built without PKCS#11 support.
- Rely on GnuTLS and p11-kit for automatic PKCS#11 provider discovery,
removing PKCS11_MODULE_PATH and manual initialization/deinitialization.
- Improve user-facing errors for PKCS#11 certificate and key imports.
- Document the use of PKCS#11 URIs in mkeficapsule.1 and the UEFI
documentation.
- Remove PKCS11_MODULE_PATH from the SoftHSM test to verify automatic
provider discovery through GnuTLS and p11-kit.
NACK.
You're trying to do too many unrelated things at once.
- The doc updates are unrelated to the pkcs11 support being compiled
out. It's a nice addition but needs to be its own separate commit.
- I don't understand why we need to warn that we're building without
pkcs11 support. Do we need to warn for anything that isn't supported?
We're working on removing support for OpenSSL engines on select OpenSSL
versions, should I warn when we're building mkimage? I really think this
is unnecessary noise. I guess we could have the usage of mkeficapsule be
different based on whether PKCS11 support is enabled or not.
- We should not remove PKCS11_MODULE_PATH in a patch making pkcs11
optional. It is currently supported, there's no reason to decide to
remove it. Yes it was undocumented, but maybe it's used. A separate
patch is definitely warranted for that if this is something we want to
do. This patch really should only gate pkcs11 support if gnutls is
compiled without support, that's it, anything else must be a separate
patch and justified separately.
The "urgency" is with disabling pkcs11 support in mkeficapsule when
gnutls is built without pkcs11 support, due to OpenEmbedded having it
disabled by default. So it's fine if we have separate series to avoid
blocking the gating of pkcs11 support with other patches for which
nitpicking/bikeshedding might still happen.
doc/develop/uefi/uefi.rst | 13 ++++++
doc/mkeficapsule.1 | 25 ++++++++--
tools/Makefile | 9 ++++
tools/binman/ftest.py | 3 +-
tools/mkeficapsule.c | 98 +++++++++++++++++++++++----------------
5 files changed, 102 insertions(+), 46 deletions(-)
diff --git a/doc/develop/uefi/uefi.rst b/doc/develop/uefi/uefi.rst
index 3ca22b572a92..cf7f638f2c23 100644
--- a/doc/develop/uefi/uefi.rst
+++ b/doc/develop/uefi/uefi.rst
@@ -597,6 +597,19 @@ and used by the steps highlighted below.
[--fit | --raw | --guid <guid-string] \
<image_blob> <capsule_file_name>
+If ``mkeficapsule`` was built with PKCS #11 support, the private key and
+certificate arguments can instead be PKCS #11 URIs. GnuTLS automatically
+discovers PKCS #11 providers configured through p11-kit. For example:
+
+.. code-block:: console
+
+ $ mkeficapsule --monotonic-count 1 \
+ --private-key 'pkcs11:token=U-Boot;object=capsule-key;type=private' \
+ --certificate 'pkcs11:token=U-Boot;object=capsule-cert;type=cert' \
+ --index 1 --instance 0 \
+ [--fit | --raw | --guid <guid-string] \
+ <image_blob> <capsule_file_name>
+
Anti-rollback Protection
************************
diff --git a/doc/mkeficapsule.1 b/doc/mkeficapsule.1
index ed296730eeb9..dfa9b805ca32 100644
--- a/doc/mkeficapsule.1
+++ b/doc/mkeficapsule.1
@@ -108,12 +108,12 @@ With signing,
are all mandatory.
.TP
-.BI "-p\fR,\fB --private-key " private-key-file
-Specify signer's private key file in PEM
+.BI "-p\fR,\fB --private-key " private-key-file-or-URI
+Specify the signer's private key as a PEM file or PKCS #11 URI.
.TP
-.BI "-c\fR,\fB --certificate " certificate-file
-Specify signer's certificate file in EFI certificate list format
+.BI "-c\fR,\fB --certificate " certificate-file-or-URI
+Specify the signer's certificate as a PEM file or PKCS #11 URI.
.TP
.BI "-m\fR,\fB --monotonic-count " count
@@ -124,6 +124,23 @@ at every firmware update.
.B "-d\fR,\fB --dump-sig"
Dump signature data into <capsule-file-name>.p7 file
+.SH "PKCS #11"
+If
+.B mkeficapsule
+was built with PKCS #11 support, a PKCS #11 URI can be used in place of the
+private key or certificate filename. PKCS #11 providers configured through
+p11-kit are discovered automatically by GnuTLS.
+
Should we add a way for the user to know whether mkeficapsule was
compiled with pkcs11 support? Otherwise how will they know (not sure
"try and it'll fail if it isn't supported" is the best UX :) ).
+For example:
+
+.EX
+mkeficapsule --monotonic-count 1 \\
+ --private-key 'pkcs11:token=U-Boot;object=capsule-key;type=private' \\
+ --certificate 'pkcs11:token=U-Boot;object=capsule-cert;type=cert' \\
+ --index 1 --instance 0 --guid <guid-string> \\
+ <image-blob> <capsule-file>
+.EE
+
.SH "GUIDGEN OPTIONS"
.TP
diff --git a/tools/Makefile b/tools/Makefile
index 1a5f425ecdaa..c156af9fc5e9 100644
--- a/tools/Makefile
+++ b/tools/Makefile
@@ -271,6 +271,15 @@ mkeficapsule-objs := generated/lib/uuid.o \
$(LIBFDT_OBJS) \
mkeficapsule.o
hostprogs-always-$(CONFIG_TOOLS_MKEFICAPSULE) += mkeficapsule
+ifeq ($(CONFIG_TOOLS_MKEFICAPSULE),y)
+GNUTLS_SUPPORTS_P11KIT = $(shell pkg-config --libs gnutls
--print-requires-private \
+ 2> /dev/null | grep p11-kit-1)
+ifeq ($(GNUTLS_SUPPORTS_P11KIT),p11-kit-1)
+HOSTCFLAGS_mkeficapsule.o += -DMKEFICAPSULE_PKCS11
+else
+$(warning Building mkeficapsule without PKCS#11 support)
+endif
+endif
include tools/fwumdata_src/fwumdata.mk
diff --git a/tools/binman/ftest.py b/tools/binman/ftest.py
index bf98b268ac15..d8f4d4ab2439 100644
--- a/tools/binman/ftest.py
+++ b/tools/binman/ftest.py
@@ -7588,8 +7588,7 @@ fdt fdtmap Extract the devicetree
blob from the fdtmap
self.assertIsNotNone(softhsm2_lib)
with unittest.mock.patch.dict('os.environ',
- {'SOFTHSM2_CONF': softhsm2_conf,
- 'PKCS11_MODULE_PATH': softhsm2_lib}):
+ {'SOFTHSM2_CONF': softhsm2_conf}):
softhsm2_util.run_cmd('--init-token', '--free', '--label',
'U-Boot token', '--pin', '1111',
'--so-pin', '222222')
diff --git a/tools/mkeficapsule.c b/tools/mkeficapsule.c
index ec640c57e8a5..b81c295f91e5 100644
--- a/tools/mkeficapsule.c
+++ b/tools/mkeficapsule.c
@@ -81,8 +81,8 @@ static void print_usage_mkeficapsule(void)
"\t-i, --index <index> update image index\n"
"\t-I, --instance <instance> update hardware instance\n"
"\t-v, --fw-version <version> firmware version\n"
- "\t-p, --private-key <privkey file> private key file\n"
- "\t-c, --certificate <cert file> signer's certificate
file\n"
+ "\t-p, --private-key <file|URI> private key file or PKCS#11
URI\n"
+ "\t-c, --certificate <file|URI> certificate file or PKCS#11
URI\n"
Maybe we could also add (PEM) after "file" so the user knows what format
to use? (if that really is PEM, I haven't checked myself).
"\t-m, --monotonic-count <count> monotonic count\n"
"\t-d, --dump-sig dump signature to <output
file>.p7\n"
"\t-A, --fw-accept firmware accept capsule, requires GUID, no image
blob\n"
@@ -207,6 +207,58 @@ static int write_capsule_file(FILE *f, void *data, size_t
size, const char *msg)
return 0;
}
+#ifdef MKEFICAPSULE_PKCS11
+static int import_pkcs11_crt(gnutls_x509_crt_t *x509, struct auth_context *ctx)
+{
+ gnutls_pkcs11_obj_t *obj_list;
+ unsigned int obj_list_size = 0;
+ int ret;
+
+ ret = gnutls_pkcs11_obj_list_import_url4(&obj_list, &obj_list_size,
+ ctx->cert_file, 0);
+ if (ret < 0) {
+ fprintf(stderr, "Failed to import PKCS#11 certificate: %s\n",
+ gnutls_strerror(ret));
+ return ret;
+ }
+ if (!obj_list_size) {
+ fprintf(stderr,
+ "Failed to import PKCS#11 certificate: no matching
object\n");
+ return -1;
+ }
+
+ gnutls_x509_crt_import_pkcs11(*x509, obj_list[0]);
+
+ return 0;
+}
+
+static int import_pkcs11_key(gnutls_privkey_t *pkey, struct auth_context *ctx)
+{
+ int ret;
+
+ ret = gnutls_privkey_import_pkcs11_url(*pkey, ctx->key_file);
+ if (ret < 0)
+ fprintf(stderr, "Failed to import PKCS#11 private key: %s\n",
+ gnutls_strerror(ret));
+
+ return ret;
+}
+#else
+static int import_pkcs11_crt(gnutls_x509_crt_t *x509, struct auth_context *ctx)
+{
+ fprintf(stderr,
+ "Cannot import PKCS#11 certificate: support is disabled\n");
+ return -1;
+}
+
+static int import_pkcs11_key(gnutls_privkey_t *pkey, struct auth_context *ctx)
+{
+ fprintf(stderr,
+ "Cannot import PKCS#11 private key: support is disabled\n");
+ return -1;
+}
+#endif
+
/**
* create_auth_data - compose authentication data in capsule
* @auth_context: Pointer to authentication context
@@ -229,9 +281,6 @@ static int create_auth_data(struct auth_context *ctx)
gnutls_pkcs7_t pkcs7;
gnutls_datum_t data;
gnutls_datum_t signature;
- gnutls_pkcs11_obj_t *obj_list;
- unsigned int obj_list_size = 0;
- const char *lib;
int ret;
bool pkcs11_cert = false;
bool pkcs11_key = false;
@@ -242,24 +291,6 @@ static int create_auth_data(struct auth_context *ctx)
if (!strncmp(ctx->key_file, "pkcs11:", strlen("pkcs11:")))
pkcs11_key = true;
- if (pkcs11_cert || pkcs11_key) {
- lib = getenv("PKCS11_MODULE_PATH");
- if (!lib) {
- fprintf(stdout,
- "PKCS11_MODULE_PATH not set in the
environment\n");
- return -1;
- }
-
- gnutls_pkcs11_init(GNUTLS_PKCS11_FLAG_MANUAL, NULL);
- gnutls_global_init();
-
- ret = gnutls_pkcs11_add_provider(lib, "trusted");
- if (ret < 0) {
- fprintf(stdout, "Failed to add pkcs11 provider\n");
- return -1;
- }
- }
-
if (!pkcs11_cert) {
ret = read_bin_file(ctx->cert_file, &cert.data, &file_size);
if (ret < 0)
@@ -301,14 +332,9 @@ static int create_auth_data(struct auth_context *ctx)
/* load x509 certificate */
if (pkcs11_cert) {
- ret = gnutls_pkcs11_obj_list_import_url4(&obj_list,
&obj_list_size,
- ctx->cert_file, 0);
- if (ret < 0 || obj_list_size == 0) {
- fprintf(stdout, "Failed to import crt_file URI
objects\n");
+ ret = import_pkcs11_crt(&x509, ctx);
+ if (ret < 0)
return -1;
- }
-
- gnutls_x509_crt_import_pkcs11(x509, obj_list[0]);
} else {
ret = gnutls_x509_crt_import(x509, &cert, GNUTLS_X509_FMT_PEM);
if (ret < 0) {
@@ -320,12 +346,9 @@ static int create_auth_data(struct auth_context *ctx)
/* load a private key */
if (pkcs11_key) {
- ret = gnutls_privkey_import_pkcs11_url(pkey, ctx->key_file);
- if (ret < 0) {
- fprintf(stderr, "error in %d: %s\n", __LINE__,
- gnutls_strerror(ret));
+ ret = import_pkcs11_key(&pkey, ctx);
+ if (ret < 0)
return -1;
- }
} else {
ret = gnutls_privkey_import_x509_raw(pkey, &key,
GNUTLS_X509_FMT_PEM,
0, 0);
@@ -403,11 +426,6 @@ static int create_auth_data(struct auth_context *ctx)
* gnutls_free(signature.data);
*/
- if (pkcs11_cert || pkcs11_key) {
- gnutls_global_deinit();
- gnutls_pkcs11_deinit();
- }
-
Where's this gone? in v8 you used to have
if (pkcs11_cert || pkcs11_key)
pkcs11_deinit();
with an empty function when pkcs11 support is disabled but it's gone
from v9.
Cheers,
Quentin
