[PATCH v4 0/6] UEFI Capsule - PKCS11 Support

[PATCH v4 0/6] UEFI Capsule - PKCS11 Support

[Wojciech Dubowik]

Add support for pkcs11 URI's when generating UEFI capsules and
accept URI's for certificate in dts capsule nodes.
Example:
export PKCS11_MODULE_PATH=<pkcs11 provider path>/libsofthsm2.so
tools/mkeficapsule --monotonic-count 1 \
 --private-key "pkcs11:token=EX;object=capsule;type=private;pin-source=pin.txt" \
 --certificate "pkcs11:token=EX;object=capsule;type=cert;pin-source=pin.txt" \
 --index 1 \
 --guid XXXXXXXX-XXXX-XXXX-XXXX-XXXXXXXX \
 "capsule-payload" \
 "capsule.cap
Signed-off-by: Wojciech Dubowik <Wojciech.Dubowik@mt.com>
---
Changes in v4:
* adapt mkeficapsule python support to dump detached signature
  for authenticated capsules
* verify detached capsule signature with openssl after generation
* use p11-kit to figure out location of softhsm2 library
* fix missing long option for dumping signatures in mkeficapsule
Changes in v3:
* fix write file encoding, env setting and extra line in binman test
  after review
Changes in v2:
* allow mixed file/pkcs11 URI as key specification in mkeficapsule
* fix logic for accepting pkcs11 URI in binman device tree sections
* add binman test for UEFI capsule signature where private key comes
  from softHSM
---
Wojciech Dubowik (6):
  tools: mkeficapsule: Add support for pkcs11
  binman: Accept pkcs11 URI tokens for capsule updates
  tools: Fix long option --dump_sig in mkeficapsule
  binman: Add dump signarture option to mkeficapsule
  binman: DTS: Add dump-signature option for capsules
  test: binman: Add test for pkcs11 signed capsule

 tools/binman/btool/mkeficapsule.py            |   6 +-
 tools/binman/entries.rst                      |   2 +
 tools/binman/etype/efi_capsule.py             |  13 +-
 tools/binman/ftest.py                         |  53 +++++++++
 .../binman/test/351_capsule_signed_pkcs11.dts |  22 ++++
 tools/mkeficapsule.c                          | 111 ++++++++++++++----
 6 files changed, 177 insertions(+), 30 deletions(-)
 create mode 100644 tools/binman/test/351_capsule_signed_pkcs11.dts

-- 
2.47.3

[PATCH v4 1/6] tools: mkeficapsule: Add support for pkcs11

[Wojciech Dubowik]

With pkcs11 support it's now possible to specify keys
with URI format. To use this feature the filename must
begin "pkcs11:.." and have valid URI pointing to certificate
and private key in HSM.

The environment variable PKCS11_MODULE_PATH must point to the
right pkcs11 provider i.e. with softhsm:
export PKCS11_MODULE_PATH=<path>/libsofthsm2.so

Example command line:
tools/mkeficapsule --monotonic-count 1 \
 --private-key "pkcs11:token=EX;object=capsule;type=private;pin-source=pin.txt" \
 --certificate "pkcs11:token=EX;object=capsule;type=cert;pin-source=pin.txt" \
 --index 1 \
 --guid XXXXXXXX-XXXX-XXXX-XXXX-XXXXXXXX \
 "capsule-payload" \
 "capsule.cap"

Signed-off-by: Wojciech Dubowik <Wojciech.Dubowik@mt.com>
---
 tools/mkeficapsule.c | 110 +++++++++++++++++++++++++++++++++----------
 1 file changed, 84 insertions(+), 26 deletions(-)

diff --git a/tools/mkeficapsule.c b/tools/mkeficapsule.c
index 0f41cdb64f54..a61557b73bef 100644
--- a/tools/mkeficapsule.c
+++ b/tools/mkeficapsule.c
@@ -228,21 +228,54 @@ 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;
 
-	ret = read_bin_file(ctx->cert_file, &cert.data, &file_size);
-	if (ret < 0)
-		return -1;
-	if (file_size > UINT_MAX)
-		return -1;
-	cert.size = file_size;
+	if (!strncmp(ctx->cert_file, "pkcs11:", 7))
+		pkcs11_cert = true;
 
-	ret = read_bin_file(ctx->key_file, &key.data, &file_size);
-	if (ret < 0)
-		return -1;
-	if (file_size > UINT_MAX)
-		return -1;
-	key.size = file_size;
+	if (!strncmp(ctx->key_file, "pkcs11:", 7))
+		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)
+			return -1;
+		if (file_size > UINT_MAX)
+			return -1;
+		cert.size = file_size;
+	}
+
+	if (!pkcs11_key) {
+		ret = read_bin_file(ctx->key_file, &key.data, &file_size);
+		if (ret < 0)
+			return -1;
+		if (file_size > UINT_MAX)
+			return -1;
+		key.size = file_size;
+	}
 
 	/*
 	 * For debugging,
@@ -265,22 +298,42 @@ static int create_auth_data(struct auth_context *ctx)
 		return -1;
 	}
 
-	/* load a private key */
-	ret = gnutls_privkey_import_x509_raw(pkey, &key, GNUTLS_X509_FMT_PEM,
-					     0, 0);
-	if (ret < 0) {
-		fprintf(stderr,
-			"error in gnutls_privkey_import_x509_raw(): %s\n",
-			gnutls_strerror(ret));
-		return -1;
+	/* 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");
+			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) {
+			fprintf(stderr, "error in gnutls_x509_crt_import(): %s\n",
+				gnutls_strerror(ret));
+			return -1;
+		}
 	}
 
-	/* load x509 certificate */
-	ret = gnutls_x509_crt_import(x509, &cert, GNUTLS_X509_FMT_PEM);
-	if (ret < 0) {
-		fprintf(stderr, "error in gnutls_x509_crt_import(): %s\n",
-			gnutls_strerror(ret));
-		return -1;
+	/* 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));
+			return -1;
+		}
+	} else {
+		ret = gnutls_privkey_import_x509_raw(pkey, &key, GNUTLS_X509_FMT_PEM,
+						     0, 0);
+		if (ret < 0) {
+			fprintf(stderr,
+				"error in gnutls_privkey_import_x509_raw(): %s\n",
+				gnutls_strerror(ret));
+			return -1;
+		}
 	}
 
 	/* generate a PKCS #7 structure */
@@ -349,6 +402,11 @@ static int create_auth_data(struct auth_context *ctx)
 	 *   gnutls_free(signature.data);
 	 */
 
+	if (pkcs11_cert || pkcs11_key) {
+		gnutls_global_deinit();
+		gnutls_pkcs11_deinit();
+	}
+
 	return 0;
 }
 
-- 
2.47.3

[PATCH v4 2/6] binman: Accept pkcs11 URI tokens for capsule updates

[Wojciech Dubowik]

With pkcs11 support in mkeficapsule we can now accept URI
tokens and not only files.

Signed-off-by: Wojciech Dubowik <Wojciech.Dubowik@mt.com>
Reviewed-by: Simon Glass <simon.glass@canonical.com>
---
 tools/binman/etype/efi_capsule.py | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/tools/binman/etype/efi_capsule.py b/tools/binman/etype/efi_capsule.py
index 9f06cc88e6e5..3b30c12ea514 100644
--- a/tools/binman/etype/efi_capsule.py
+++ b/tools/binman/etype/efi_capsule.py
@@ -125,10 +125,14 @@ class Entry_efi_capsule(Entry_section):
         private_key = ''
         public_key_cert = ''
         if self.auth:
-            if not os.path.isabs(self.private_key):
+            if not os.path.isabs(self.private_key) and not 'pkcs11:' in self.private_key:
                 private_key =  tools.get_input_filename(self.private_key)
-            if not os.path.isabs(self.public_key_cert):
+            if not os.path.isabs(self.public_key_cert) and not 'pkcs11:' in self.public_key_cert:
                 public_key_cert = tools.get_input_filename(self.public_key_cert)
+            if 'pkcs11:' in self.private_key:
+                private_key = self.private_key
+            if 'pkcs11:' in self.public_key_cert:
+                public_key_cert = self.public_key_cert
         data, payload, uniq = self.collect_contents_to_file(
             self._entries.values(), 'capsule_in')
         outfile = self._filename if self._filename else 'capsule.%s' % uniq
-- 
2.47.3

[PATCH v4 3/6] tools: Fix long option --dump_sig in mkeficapsule

[Wojciech Dubowik]

Only short option has been present.

Signed-off-by: Wojciech Dubowik <Wojciech.Dubowik@mt.com>
---
 tools/mkeficapsule.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/tools/mkeficapsule.c b/tools/mkeficapsule.c
index a61557b73bef..a89f11713d67 100644
--- a/tools/mkeficapsule.c
+++ b/tools/mkeficapsule.c
@@ -56,6 +56,7 @@ static struct option options[] = {
 	{"fw-revert", no_argument, NULL, 'R'},
 	{"capoemflag", required_argument, NULL, 'o'},
 	{"dump-capsule", no_argument, NULL, 'D'},
+	{"dump_sig", no_argument, NULL, 'd'},
 	{"help", no_argument, NULL, 'h'},
 	{NULL, 0, NULL, 0},
 };
-- 
2.47.3

[PATCH v4 4/6] binman: Add dump signarture option to mkeficapsule

[Wojciech Dubowik]

It will be used to capsule signature verification.

Signed-off-by: Wojciech Dubowik <Wojciech.Dubowik@mt.com>
---
 tools/binman/btool/mkeficapsule.py | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/tools/binman/btool/mkeficapsule.py b/tools/binman/btool/mkeficapsule.py
index f7e5a8868495..f4b594b3faef 100644
--- a/tools/binman/btool/mkeficapsule.py
+++ b/tools/binman/btool/mkeficapsule.py
@@ -38,7 +38,8 @@ class Bintoolmkeficapsule(bintool.Bintool):
 
     def generate_capsule(self, image_index, image_guid, hardware_instance,
                          payload, output_fname, priv_key, pub_key,
-                         monotonic_count=0, version=0, oemflags=0):
+                         monotonic_count=0, version=0, oemflags=0,
+                         dump_sig=0):
         """Generate a capsule through commandline-provided parameters
 
         Args:
@@ -53,6 +54,7 @@ class Bintoolmkeficapsule(bintool.Bintool):
             monotonic_count (int): Count used when signing an image
             version (int): Image version (Optional)
             oemflags (int): Optional 16 bit OEM flags
+            dump_sig (int): Dump signature to a file (Optional)
 
         Returns:
             str: Tool output
@@ -73,6 +75,8 @@ class Bintoolmkeficapsule(bintool.Bintool):
                 f'--private-key={priv_key}',
                 f'--certificate={pub_key}'
             ]
+        if dump_sig:
+            args += [f'--dump_sig']
 
         args += [
             payload,
-- 
2.47.3

[PATCH v4 5/6] binman: DTS: Add dump-signature option for capsules

[Wojciech Dubowik]

Mkeficapsule can dump signature for signed capsules. It can
be used in test to validate signature i.e. with openssl.
Add an entry for device tree node.

Signed-off-by: Wojciech Dubowik <Wojciech.Dubowik@mt.com>
---
 tools/binman/entries.rst          | 2 ++
 tools/binman/etype/efi_capsule.py | 5 ++++-
 2 files changed, 6 insertions(+), 1 deletion(-)

diff --git a/tools/binman/entries.rst b/tools/binman/entries.rst
index a81fcbd3891f..1dace2087a2a 100644
--- a/tools/binman/entries.rst
+++ b/tools/binman/entries.rst
@@ -552,6 +552,8 @@ Properties / Entry arguments:
     - public-key-cert: Path to PEM formatted .crt public key certificate
       file. Mandatory property for generating signed capsules.
     - oem-flags - OEM flags to be passed through capsule header.
+    - dump-signature: Instruct mkeficapsule to write signature data to
+      a separete file. It might be used to verify capsule authentication.
 
 Since this is a subclass of Entry_section, all properties of the parent
 class also apply here. Except for the properties stated as mandatory, the
diff --git a/tools/binman/etype/efi_capsule.py b/tools/binman/etype/efi_capsule.py
index 3b30c12ea514..01d56723b98c 100644
--- a/tools/binman/etype/efi_capsule.py
+++ b/tools/binman/etype/efi_capsule.py
@@ -101,6 +101,7 @@ class Entry_efi_capsule(Entry_section):
         self.private_key = ''
         self.public_key_cert = ''
         self.auth = 0
+        self.dump_signature = False
 
     def ReadNode(self):
         super().ReadNode()
@@ -111,6 +112,7 @@ class Entry_efi_capsule(Entry_section):
         self.hardware_instance = fdt_util.GetInt(self._node, 'hardware-instance')
         self.monotonic_count = fdt_util.GetInt(self._node, 'monotonic-count')
         self.oem_flags = fdt_util.GetInt(self._node, 'oem-flags')
+        self.dump_signature = fdt_util.GetBool(self._node, 'dump-signature')
 
         self.private_key = fdt_util.GetString(self._node, 'private-key')
         self.public_key_cert = fdt_util.GetString(self._node, 'public-key-cert')
@@ -150,7 +152,8 @@ class Entry_efi_capsule(Entry_section):
                                                  public_key_cert,
                                                  self.monotonic_count,
                                                  self.fw_version,
-                                                 self.oem_flags)
+                                                 self.oem_flags,
+                                                 self.dump_signature)
         if ret is not None:
             return tools.read_file(capsule_fname)
         else:
-- 
2.47.3

[PATCH v4 6/6] test: binman: Add test for pkcs11 signed capsule

[Wojciech Dubowik]

Test pkcs11 URI support for UEFI capsule generation. For
simplicity only private key is defined in binman section
as softhsm tool doesn't support certificate import (yet).

Signed-off-by: Wojciech Dubowik <Wojciech.Dubowik@mt.com>
Reviewed-by: Simon Glass <simon.glass@canonical.com>
---
 tools/binman/ftest.py                         | 53 +++++++++++++++++++
 .../binman/test/351_capsule_signed_pkcs11.dts | 22 ++++++++
 2 files changed, 75 insertions(+)
 create mode 100644 tools/binman/test/351_capsule_signed_pkcs11.dts

diff --git a/tools/binman/ftest.py b/tools/binman/ftest.py
index 21ec48d86fd1..a005a167e414 100644
--- a/tools/binman/ftest.py
+++ b/tools/binman/ftest.py
@@ -7,6 +7,7 @@
 #    python -m unittest func_test.TestFunctional.testHelp
 
 import collections
+import configparser
 import glob
 import gzip
 import hashlib
@@ -7532,6 +7533,58 @@ fdt         fdtmap                Extract the devicetree blob from the fdtmap
 
         self._CheckCapsule(data, signed_capsule=True)
 
+    def testPkcs11SignedCapsuleGen(self):
+        """Test generation of EFI capsule (with PKCS11)"""
+        data = tools.read_file(self.TestFile("key.key"))
+        private_key = self._MakeInputFile("key.key", data)
+        data = tools.read_file(self.TestFile("key.pem"))
+        cert_file = self._MakeInputFile("key.crt", data)
+
+        softhsm2_util = bintool.Bintool.create('softhsm2_util')
+        self._CheckBintool(softhsm2_util)
+
+        prefix = "testPkcs11SignedCapsuleGen."
+        # Configure SoftHSMv2
+        data = tools.read_file(self.TestFile('340_softhsm2.conf'))
+        softhsm2_conf = self._MakeInputFile(f'{prefix}softhsm2.conf', data)
+        softhsm2_tokens_dir = self._MakeInputDir(f'{prefix}softhsm2.tokens')
+        tools.write_file(softhsm2_conf, data +
+                         f'\ndirectories.tokendir = \
+                         {softhsm2_tokens_dir}\n'.encode("utf-8"))
+
+        p11_kit_config = configparser.ConfigParser()
+        out = tools.run('p11-kit', 'print-config')
+        p11_kit_config.read_string(out)
+        softhsm2_lib = p11_kit_config['softhsm2']['module']
+
+        os.environ['SOFTHSM2_CONF'] = softhsm2_conf
+        tools.run('softhsm2-util', '--init-token', '--free', '--label',
+                  'U-Boot token', '--pin', '1111', '--so-pin',
+                  '222222')
+        tools.run('softhsm2-util', '--import', private_key, '--token',
+                  'U-Boot token', '--label', 'test_key', '--id', '999999',
+                  '--pin', '1111')
+
+        os.environ['PKCS11_MODULE_PATH'] = softhsm2_lib
+        data = self._DoReadFile('351_capsule_signed_pkcs11.dts')
+
+        self._CheckCapsule(data, signed_capsule=True)
+
+        # Verify signed capsule
+        hdr = self._GetCapsuleHeaders(data)
+        monotonic_count = hdr['EFI_FIRMWARE_IMAGE_AUTH.MONOTONIC_COUNT']
+
+        with open(self._indir + '/capsule_input.bin', 'ab') as f:
+            f.write(struct.pack('<Q', int(monotonic_count, 16)))
+
+        try:
+            tools.run('openssl', 'smime', '-verify', '-inform', 'DER',
+                      '-in', tools.get_output_dir() + '/capsule.efi-capsule.p7',
+                      '-content', self._indir + '/capsule_input.bin',
+                      '-CAfile', cert_file, '-no_check_time')
+        except ValueError:
+            self.assertIn('UEFI Capsule verification failed')
+
     def testCapsuleGenVersionSupport(self):
         """Test generation of EFI capsule with version support"""
         data = self._DoReadFile('313_capsule_version.dts')
diff --git a/tools/binman/test/351_capsule_signed_pkcs11.dts b/tools/binman/test/351_capsule_signed_pkcs11.dts
new file mode 100644
index 000000000000..ae93bf83936f
--- /dev/null
+++ b/tools/binman/test/351_capsule_signed_pkcs11.dts
@@ -0,0 +1,22 @@
+// SPDX-License-Identifier: GPL-2.0+
+
+/dts-v1/;
+
+/ {
+	binman {
+		efi-capsule {
+			image-index = <0x1>;
+			/* Image GUID for testing capsule update */
+			image-guid = "binman-test";
+			hardware-instance = <0x0>;
+			monotonic-count = <0x1>;
+			dump-signature;
+			private-key = "pkcs11:token=U-Boot%20token;object=test_key;type=private;pin-value=1111";
+			public-key-cert = "key.crt";
+
+			blob {
+				filename = "capsule_input.bin";
+			};
+		};
+	};
+};
-- 
2.47.3

Re: [PATCH v4 5/6] binman: DTS: Add dump-signature option for capsules

[Quentin Schulz]

Hi Wojciech,

On 1/20/26 9:12 AM, Wojciech Dubowik wrote:
> Mkeficapsule can dump signature for signed capsules. It can
> be used in test to validate signature i.e. with openssl.
> Add an entry for device tree node.
> 
> Signed-off-by: Wojciech Dubowik <Wojciech.Dubowik@mt.com>
> ---
>   tools/binman/entries.rst          | 2 ++
>   tools/binman/etype/efi_capsule.py | 5 ++++-
>   2 files changed, 6 insertions(+), 1 deletion(-)
> 
> diff --git a/tools/binman/entries.rst b/tools/binman/entries.rst
> index a81fcbd3891f..1dace2087a2a 100644
> --- a/tools/binman/entries.rst
> +++ b/tools/binman/entries.rst
> @@ -552,6 +552,8 @@ Properties / Entry arguments:
>       - public-key-cert: Path to PEM formatted .crt public key certificate
>         file. Mandatory property for generating signed capsules.
>       - oem-flags - OEM flags to be passed through capsule header.
> +    - dump-signature: Instruct mkeficapsule to write signature data to

S/Instruct/Optional boolean (default: false). Instruct

> +      a separete file. It might be used to verify capsule authentication.
>   

s/separete/separate/

Can we predict where the file will be located and how it'll be named?

>   Since this is a subclass of Entry_section, all properties of the parent
>   class also apply here. Except for the properties stated as mandatory, the
> diff --git a/tools/binman/etype/efi_capsule.py b/tools/binman/etype/efi_capsule.py
> index 3b30c12ea514..01d56723b98c 100644
> --- a/tools/binman/etype/efi_capsule.py
> +++ b/tools/binman/etype/efi_capsule.py
> @@ -101,6 +101,7 @@ class Entry_efi_capsule(Entry_section):

Please update the docstring so it's in sync with tools/binman/entries.rst.

Cheers,
Quentin

Re: [PATCH v4 4/6] binman: Add dump signarture option to mkeficapsule

[Quentin Schulz]

Hi Wojciech,

On 1/20/26 9:12 AM, Wojciech Dubowik wrote:
> It will be used to capsule signature verification.
> 
> Signed-off-by: Wojciech Dubowik <Wojciech.Dubowik@mt.com>
> ---
>   tools/binman/btool/mkeficapsule.py | 6 +++++-
>   1 file changed, 5 insertions(+), 1 deletion(-)
> 
> diff --git a/tools/binman/btool/mkeficapsule.py b/tools/binman/btool/mkeficapsule.py
> index f7e5a8868495..f4b594b3faef 100644
> --- a/tools/binman/btool/mkeficapsule.py
> +++ b/tools/binman/btool/mkeficapsule.py
> @@ -38,7 +38,8 @@ class Bintoolmkeficapsule(bintool.Bintool):
>   
>       def generate_capsule(self, image_index, image_guid, hardware_instance,
>                            payload, output_fname, priv_key, pub_key,
> -                         monotonic_count=0, version=0, oemflags=0):
> +                         monotonic_count=0, version=0, oemflags=0,
> +                         dump_sig=0):

This is clearly a boolean flag both in tools/mkeficapsule.c, the 
dump-signature boolean property added in the next patch and how it's 
stored in Entry_efi_capsule object, so please make this a bool.

>           """Generate a capsule through commandline-provided parameters
>   
>           Args:
> @@ -53,6 +54,7 @@ class Bintoolmkeficapsule(bintool.Bintool):
>               monotonic_count (int): Count used when signing an image
>               version (int): Image version (Optional)
>               oemflags (int): Optional 16 bit OEM flags
> +            dump_sig (int): Dump signature to a file (Optional)

Ditto.

Also, specify the default value (false).

Cheers,
Quentin

Re: [PATCH v4 3/6] tools: Fix long option --dump_sig in mkeficapsule

[Quentin Schulz]

Hi Wojciech,

On 1/20/26 9:12 AM, Wojciech Dubowik wrote:
> Only short option has been present.
> 

Actually.... it's been there, just under dump-sig and not dump_sig.

So we have the choice of either fixing the usage string and the manpage 
to use dump-sig instead of dump_sig or support both.

I would personally be in favor of patching the usage string and manpage 
so we don't have long options with an underscore.. it's odd to me.

Regardless of your choice, we need

Fixes: 16abff246b40 ("tools: mkeficapsule: add firmware image signing")

in the commit log.

If you keep dump_sig, I would really recommend to put it next to 
dump-sig and explain why we have both (we need to keep dump-sig for free 
backward compatibility).

Cheers,
Quentin

Re: [PATCH v4 6/6] test: binman: Add test for pkcs11 signed capsule

[Quentin Schulz]

Hi Wojciech,

On 1/20/26 9:12 AM, Wojciech Dubowik wrote:
> Test pkcs11 URI support for UEFI capsule generation. For
> simplicity only private key is defined in binman section
> as softhsm tool doesn't support certificate import (yet).
> 
> Signed-off-by: Wojciech Dubowik <Wojciech.Dubowik@mt.com>
> Reviewed-by: Simon Glass <simon.glass@canonical.com>
> ---
>   tools/binman/ftest.py                         | 53 +++++++++++++++++++
>   .../binman/test/351_capsule_signed_pkcs11.dts | 22 ++++++++
>   2 files changed, 75 insertions(+)
>   create mode 100644 tools/binman/test/351_capsule_signed_pkcs11.dts
> 
> diff --git a/tools/binman/ftest.py b/tools/binman/ftest.py
> index 21ec48d86fd1..a005a167e414 100644
> --- a/tools/binman/ftest.py
> +++ b/tools/binman/ftest.py
> @@ -7,6 +7,7 @@
>   #    python -m unittest func_test.TestFunctional.testHelp
>   
>   import collections
> +import configparser
>   import glob
>   import gzip
>   import hashlib
> @@ -7532,6 +7533,58 @@ fdt         fdtmap                Extract the devicetree blob from the fdtmap
>   
>           self._CheckCapsule(data, signed_capsule=True)
>   
> +    def testPkcs11SignedCapsuleGen(self):
> +        """Test generation of EFI capsule (with PKCS11)"""
> +        data = tools.read_file(self.TestFile("key.key"))
> +        private_key = self._MakeInputFile("key.key", data)
> +        data = tools.read_file(self.TestFile("key.pem"))
> +        cert_file = self._MakeInputFile("key.crt", data)
> +
> +        softhsm2_util = bintool.Bintool.create('softhsm2_util')
> +        self._CheckBintool(softhsm2_util)
> +
> +        prefix = "testPkcs11SignedCapsuleGen."
> +        # Configure SoftHSMv2
> +        data = tools.read_file(self.TestFile('340_softhsm2.conf'))
> +        softhsm2_conf = self._MakeInputFile(f'{prefix}softhsm2.conf', data)
> +        softhsm2_tokens_dir = self._MakeInputDir(f'{prefix}softhsm2.tokens')
> +        tools.write_file(softhsm2_conf, data +
> +                         f'\ndirectories.tokendir = \
> +                         {softhsm2_tokens_dir}\n'.encode("utf-8"))
> +

data is already in softhsm2_conf due to calling
self._MakeInputFile(f'{prefix}softhsm2.conf', data)
you can simply do the same I did in testFitSignPKCS11Simple and append 
to the file. Or you can append to data before you call _MakeInputFile().

> +        p11_kit_config = configparser.ConfigParser()
> +        out = tools.run('p11-kit', 'print-config')

Please create a bintool and call CheckBintool so we can skip the test if 
p11-kit isn't installed. It'll be a bit awkward because there's no 
--version, --help, to print the help text but not have a non-zero exit code.

> +        p11_kit_config.read_string(out)
> +        softhsm2_lib = p11_kit_config['softhsm2']['module']
> +

We probably want to have some try..except here, or a more forgiving 
.get('softhsm2', {}).get('module')
and fail the test if we don't get a path?

> +        os.environ['SOFTHSM2_CONF'] = softhsm2_conf

This is wrong, you'll be messing up with the environment of all tests 
being run in the same thread. You must use the "with 
unittest.mock.patch.dict('os.environ'," implementation I used in 
testFitSignPKCS11Simple.

> +        tools.run('softhsm2-util', '--init-token', '--free', '--label',
> +                  'U-Boot token', '--pin', '1111', '--so-pin',
> +                  '222222')
> +        tools.run('softhsm2-util', '--import', private_key, '--token',
> +                  'U-Boot token', '--label', 'test_key', '--id', '999999',
> +                  '--pin', '1111')
> +
> +        os.environ['PKCS11_MODULE_PATH'] = softhsm2_lib

Same issue as with SOFTHSM2_CONF, you must mock the environment and not 
write to it.

> +        data = self._DoReadFile('351_capsule_signed_pkcs11.dts')
> +
> +        self._CheckCapsule(data, signed_capsule=True)
> +
> +        # Verify signed capsule
> +        hdr = self._GetCapsuleHeaders(data)
> +        monotonic_count = hdr['EFI_FIRMWARE_IMAGE_AUTH.MONOTONIC_COUNT']
> +
> +        with open(self._indir + '/capsule_input.bin', 'ab') as f:

Please use self._MakeInputFile() and prefix it with the name of the 
method so there's no possible name clash.

> +            f.write(struct.pack('<Q', int(monotonic_count, 16)))
> +
> +        try:
> +            tools.run('openssl', 'smime', '-verify', '-inform', 'DER',

This means you depend on openssl being present. Should we maybe do 
something like

openssl = bintool.Bintool.create('openssl')
self._CheckBintool(openssl)
[...]
openssl.run_cmd(['smime', ...])

to skip the test if it isn't installed?

> +                      '-in', tools.get_output_dir() + '/capsule.efi-capsule.p7',

tools.get_output_filename('capsule.efi-capsule.p7')

> +                      '-content', self._indir + '/capsule_input.bin',

Reuse the path created by self._MakeInputFile().

> +                      '-CAfile', cert_file, '-no_check_time')
> +        except ValueError:
> +            self.assertIn('UEFI Capsule verification failed')
> +

I don't think this is valid. 
https://docs.python.org/3/library/unittest.html#unittest.TestCase.assertIn 
states you need 2 (or three) arguments, only one's provided here.

I'm assuming we don't need the try..except since the raised Exception 
will just stop (and fail) the execution of the test but continue with 
the other ones? Can you check (locally) by purposefully using the wrong 
key for example?

Cheers,
Quentin

Re: EXTERNAL - [PATCH v4 6/6] test: binman: Add test for pkcs11 signed capsule

[Wojciech Dubowik]

On Tue, Jan 20, 2026 at 04:53:04PM +0100, Quentin Schulz wrote:
Hello Quentin,
> Hi Wojciech,
> 
> On 1/20/26 9:12 AM, Wojciech Dubowik wrote:
> > Test pkcs11 URI support for UEFI capsule generation. For
> > simplicity only private key is defined in binman section
> > as softhsm tool doesn't support certificate import (yet).
> > 
> > Signed-off-by: Wojciech Dubowik <Wojciech.Dubowik@mt.com>
> > Reviewed-by: Simon Glass <simon.glass@canonical.com>
> > ---
> >   tools/binman/ftest.py                         | 53 +++++++++++++++++++
> >   .../binman/test/351_capsule_signed_pkcs11.dts | 22 ++++++++
> >   2 files changed, 75 insertions(+)
> >   create mode 100644 tools/binman/test/351_capsule_signed_pkcs11.dts
> > 
> > diff --git a/tools/binman/ftest.py b/tools/binman/ftest.py
> > index 21ec48d86fd1..a005a167e414 100644
> > --- a/tools/binman/ftest.py
> > +++ b/tools/binman/ftest.py
> > @@ -7,6 +7,7 @@
> >   #    python -m unittest func_test.TestFunctional.testHelp
> >   import collections
> > +import configparser
> >   import glob
> >   import gzip
> >   import hashlib
> > @@ -7532,6 +7533,58 @@ fdt         fdtmap                Extract the devicetree blob from the fdtmap
> >           self._CheckCapsule(data, signed_capsule=True)
> > +    def testPkcs11SignedCapsuleGen(self):
> > +        """Test generation of EFI capsule (with PKCS11)"""
> > +        data = tools.read_file(self.TestFile("key.key"))
> > +        private_key = self._MakeInputFile("key.key", data)
> > +        data = tools.read_file(self.TestFile("key.pem"))
> > +        cert_file = self._MakeInputFile("key.crt", data)
> > +
> > +        softhsm2_util = bintool.Bintool.create('softhsm2_util')
> > +        self._CheckBintool(softhsm2_util)
> > +
> > +        prefix = "testPkcs11SignedCapsuleGen."
> > +        # Configure SoftHSMv2
> > +        data = tools.read_file(self.TestFile('340_softhsm2.conf'))
> > +        softhsm2_conf = self._MakeInputFile(f'{prefix}softhsm2.conf', data)
> > +        softhsm2_tokens_dir = self._MakeInputDir(f'{prefix}softhsm2.tokens')
> > +        tools.write_file(softhsm2_conf, data +
> > +                         f'\ndirectories.tokendir = \
> > +                         {softhsm2_tokens_dir}\n'.encode("utf-8"))
> > +
> 
> data is already in softhsm2_conf due to calling
> self._MakeInputFile(f'{prefix}softhsm2.conf', data)
> you can simply do the same I did in testFitSignPKCS11Simple and append to
> the file. Or you can append to data before you call _MakeInputFile().
> 
> > +        p11_kit_config = configparser.ConfigParser()
> > +        out = tools.run('p11-kit', 'print-config')
> 
> Please create a bintool and call CheckBintool so we can skip the test if
> p11-kit isn't installed. It'll be a bit awkward because there's no
> --version, --help, to print the help text but not have a non-zero exit code.
> 
> > +        p11_kit_config.read_string(out)
> > +        softhsm2_lib = p11_kit_config['softhsm2']['module']
> > +
> 
> We probably want to have some try..except here, or a more forgiving
> .get('softhsm2', {}).get('module')
> and fail the test if we don't get a path?
> 
> > +        os.environ['SOFTHSM2_CONF'] = softhsm2_conf
> 
> This is wrong, you'll be messing up with the environment of all tests being
> run in the same thread. You must use the "with
> unittest.mock.patch.dict('os.environ'," implementation I used in
> testFitSignPKCS11Simple.

Well, I have done so in my V2 but has been commented as wrong by the
first reviewer. I will restore it back.

> 
> > +        tools.run('softhsm2-util', '--init-token', '--free', '--label',
> > +                  'U-Boot token', '--pin', '1111', '--so-pin',
> > +                  '222222')
> > +        tools.run('softhsm2-util', '--import', private_key, '--token',
> > +                  'U-Boot token', '--label', 'test_key', '--id', '999999',
> > +                  '--pin', '1111')
> > +
> > +        os.environ['PKCS11_MODULE_PATH'] = softhsm2_lib
> 
> Same issue as with SOFTHSM2_CONF, you must mock the environment and not
> write to it.
> 
> > +        data = self._DoReadFile('351_capsule_signed_pkcs11.dts')
> > +
> > +        self._CheckCapsule(data, signed_capsule=True)
> > +
> > +        # Verify signed capsule
> > +        hdr = self._GetCapsuleHeaders(data)
> > +        monotonic_count = hdr['EFI_FIRMWARE_IMAGE_AUTH.MONOTONIC_COUNT']
> > +
> > +        with open(self._indir + '/capsule_input.bin', 'ab') as f:
> 
> Please use self._MakeInputFile() and prefix it with the name of the method
> so there's no possible name clash.
> 
> > +            f.write(struct.pack('<Q', int(monotonic_count, 16)))
> > +
> > +        try:
> > +            tools.run('openssl', 'smime', '-verify', '-inform', 'DER',
> 
> This means you depend on openssl being present. Should we maybe do something
> like
> 
> openssl = bintool.Bintool.create('openssl')
> self._CheckBintool(openssl)
> [...]
> openssl.run_cmd(['smime', ...])
> 
> to skip the test if it isn't installed?
> 
> > +                      '-in', tools.get_output_dir() + '/capsule.efi-capsule.p7',
> 
> tools.get_output_filename('capsule.efi-capsule.p7')
> 
> > +                      '-content', self._indir + '/capsule_input.bin',
> 
> Reuse the path created by self._MakeInputFile().
> 
> > +                      '-CAfile', cert_file, '-no_check_time')
> > +        except ValueError:
> > +            self.assertIn('UEFI Capsule verification failed')
> > +
> 
> I don't think this is valid.
> https://docs.python.org/3/library/unittest.html#unittest.TestCase.assertIn
> states you need 2 (or three) argumentis, only one's provided here.
> 
> I'm assuming we don't need the try..except since the raised Exception will
> just stop (and fail) the execution of the test but continue with the other
> ones? Can you check (locally) by purposefully using the wrong key for
> example?

It has worked (failed) with try/except. I had to add no time check as the certificate
used for uefi capsules tests has expired in 2024. Maybe somebody can refresh
it one day. I will try other methods to see if we still get meaningful error
messages.

I will try to handle issues you mentioned also in other threads this/next week.

Cheers,
Wojtek
> 
> Cheers,
> Quentin

Re: EXTERNAL - [PATCH v4 6/6] test: binman: Add test for pkcs11 signed capsule

[Quentin Schulz]

Hi Wojciech,

On 1/21/26 1:43 PM, Wojciech Dubowik wrote:
> On Tue, Jan 20, 2026 at 04:53:04PM +0100, Quentin Schulz wrote:
> Hello Quentin,
>> Hi Wojciech,
>>
>> On 1/20/26 9:12 AM, Wojciech Dubowik wrote:
[...]
>>> +        os.environ['SOFTHSM2_CONF'] = softhsm2_conf
>>
>> This is wrong, you'll be messing up with the environment of all tests being
>> run in the same thread. You must use the "with
>> unittest.mock.patch.dict('os.environ'," implementation I used in
>> testFitSignPKCS11Simple.
> 
> Well, I have done so in my V2 but has been commented as wrong by the
> first reviewer. I will restore it back.
> 

Indeed, I see Simon asked you to do this in v2 and I missed it. It isn't 
how we should be doing it.

This is likely fine on its own because there's only one test that is now 
modifying os.environ's SOFTHSM2_CONF but this will be a problem next 
time a test wants to modify it. I actually hit this issue when 
developing the PKCS11 fit signing tests as I had two tests modifying the 
environment.

The only trace of it left is the changelog in 
https://lore.kernel.org/u-boot/20251121-binman-engine-v3-0-b80180aaa783@cherry.de/

"""
- fixed issues due to modification of the environment in tests failing
   other tests, by using unittest.mock.patch.dict() on os.environ as
   suggested by the unittest.mock doc,
"""

and you can check the diff between the v2 and v3 to check I used to 
modify the env directly but now mock it instead.

Sorry for not catching this, should have answered to Simon in the v2.

[...]

>>> +                      '-CAfile', cert_file, '-no_check_time')
>>> +        except ValueError:
>>> +            self.assertIn('UEFI Capsule verification failed')
>>> +
>>
>> I don't think this is valid.
>> https://docs.python.org/3/library/unittest.html#unittest.TestCase.assertIn
>> states you need 2 (or three) argumentis, only one's provided here.
>>
>> I'm assuming we don't need the try..except since the raised Exception will
>> just stop (and fail) the execution of the test but continue with the other
>> ones? Can you check (locally) by purposefully using the wrong key for
>> example?
> 
> It has worked (failed) with try/except. I had to add no time check as the certificate
Thanks for the heads up, @Simon any chance you can update the 
certificate so we're fine for the decades to come?

Cheers,
Quentin

Re: EXTERNAL - [PATCH v4 6/6] test: binman: Add test for pkcs11 signed capsule

[Simon Glass]

Hi,

On Thu, 22 Jan 2026 at 02:06, Quentin Schulz <quentin.schulz@cherry.de> wrote:
>
> Hi Wojciech,
>
> On 1/21/26 1:43 PM, Wojciech Dubowik wrote:
> > On Tue, Jan 20, 2026 at 04:53:04PM +0100, Quentin Schulz wrote:
> > Hello Quentin,
> >> Hi Wojciech,
> >>
> >> On 1/20/26 9:12 AM, Wojciech Dubowik wrote:
> [...]
> >>> +        os.environ['SOFTHSM2_CONF'] = softhsm2_conf
> >>
> >> This is wrong, you'll be messing up with the environment of all tests being
> >> run in the same thread. You must use the "with
> >> unittest.mock.patch.dict('os.environ'," implementation I used in
> >> testFitSignPKCS11Simple.
> >
> > Well, I have done so in my V2 but has been commented as wrong by the
> > first reviewer. I will restore it back.
> >
>
> Indeed, I see Simon asked you to do this in v2 and I missed it. It isn't
> how we should be doing it.
>
> This is likely fine on its own because there's only one test that is now
> modifying os.environ's SOFTHSM2_CONF but this will be a problem next
> time a test wants to modify it. I actually hit this issue when
> developing the PKCS11 fit signing tests as I had two tests modifying the
> environment.
>
> The only trace of it left is the changelog in
> https://lore.kernel.org/u-boot/20251121-binman-engine-v3-0-b80180aaa783@cherry.de/
>
> """
> - fixed issues due to modification of the environment in tests failing
>    other tests, by using unittest.mock.patch.dict() on os.environ as
>    suggested by the unittest.mock doc,
> """
>
> and you can check the diff between the v2 and v3 to check I used to
> modify the env directly but now mock it instead.
>
> Sorry for not catching this, should have answered to Simon in the v2.

In practice we try to set values for various which are important, so
future tests should explicitly delete the var if needed. But I am OK
with mocking, instead.

>
> [...]
>
> >>> +                      '-CAfile', cert_file, '-no_check_time')
> >>> +        except ValueError:
> >>> +            self.assertIn('UEFI Capsule verification failed')
> >>> +
> >>
> >> I don't think this is valid.
> >> https://docs.python.org/3/library/unittest.html#unittest.TestCase.assertIn
> >> states you need 2 (or three) argumentis, only one's provided here.
> >>
> >> I'm assuming we don't need the try..except since the raised Exception will
> >> just stop (and fail) the execution of the test but continue with the other
> >> ones? Can you check (locally) by purposefully using the wrong key for
> >> example?
> >
> > It has worked (failed) with try/except. I had to add no time check as the certificate
> Thanks for the heads up, @Simon any chance you can update the
> certificate so we're fine for the decades to come?

OK I sent a patch.

Regards,
Simon

Re: EXTERNAL - [PATCH v4 6/6] test: binman: Add test for pkcs11 signed capsule

[Quentin Schulz]

Hi Simon,

On 1/22/26 11:46 PM, Simon Glass wrote:
> Hi,
> 
> On Thu, 22 Jan 2026 at 02:06, Quentin Schulz <quentin.schulz@cherry.de> wrote:
>>
>> Hi Wojciech,
>>
>> On 1/21/26 1:43 PM, Wojciech Dubowik wrote:
>>> On Tue, Jan 20, 2026 at 04:53:04PM +0100, Quentin Schulz wrote:
>>> Hello Quentin,
>>>> Hi Wojciech,
>>>>
>>>> On 1/20/26 9:12 AM, Wojciech Dubowik wrote:
>> [...]
>>>>> +        os.environ['SOFTHSM2_CONF'] = softhsm2_conf
>>>>
>>>> This is wrong, you'll be messing up with the environment of all tests being
>>>> run in the same thread. You must use the "with
>>>> unittest.mock.patch.dict('os.environ'," implementation I used in
>>>> testFitSignPKCS11Simple.
>>>
>>> Well, I have done so in my V2 but has been commented as wrong by the
>>> first reviewer. I will restore it back.
>>>
>>
>> Indeed, I see Simon asked you to do this in v2 and I missed it. It isn't
>> how we should be doing it.
>>
>> This is likely fine on its own because there's only one test that is now
>> modifying os.environ's SOFTHSM2_CONF but this will be a problem next
>> time a test wants to modify it. I actually hit this issue when
>> developing the PKCS11 fit signing tests as I had two tests modifying the
>> environment.
>>
>> The only trace of it left is the changelog in
>> https://lore.kernel.org/u-boot/20251121-binman-engine-v3-0-b80180aaa783@cherry.de/
>>
>> """
>> - fixed issues due to modification of the environment in tests failing
>>     other tests, by using unittest.mock.patch.dict() on os.environ as
>>     suggested by the unittest.mock doc,
>> """
>>
>> and you can check the diff between the v2 and v3 to check I used to
>> modify the env directly but now mock it instead.
>>
>> Sorry for not catching this, should have answered to Simon in the v2.
> 
> In practice we try to set values for various which are important, so
> future tests should explicitly delete the var if needed. But I am OK

This is not working. See this very simple example (too lazy to use 
threading.Lock so synchronization done via time.sleep instead):

"""
#!/usr/bin/env python3

import os
import time
import threading


def thread_func(n):
     if n == 1:
         time.sleep(1)
     print(f'Thread {n} read environ var FOO={os.environ["FOO"]}')
     if n == 1:
         time.sleep(1)
     print(f'Thread {n} set environ var FOO to foo{n}')
     os.environ['FOO'] = f'foo{n}'
     if n == 0:
         time.sleep(5)
     print(f'Thread {n} read environ var FOO={os.environ["FOO"]}')
     if n == 0:
         print(f'Thread {n} removes environ var FOO')
         del os.environ["FOO"]
     else:
         time.sleep(10)
         print(f'Thread {n} read environ var FOO={os.environ.get("FOO")}')


threads = []

os.environ["FOO"] = "foo"

for i in range(0, 2):
     t = threading.Thread(target=thread_func, args=(i,))
     threads.append(t)

for t in threads:
     t.start()

for t in threads:
     t.join()

"""

This results in:

"""
Thread 0 read environ var FOO=foo
Thread 0 set environ var FOO to foo0
Thread 1 read environ var FOO=foo0
Thread 1 set environ var FOO to foo1
Thread 1 read environ var FOO=foo1
Thread 0 read environ var FOO=foo1
Thread 0 removes environ var FOO
Thread 1 read environ var FOO=None
"""

You see that modification made to os.environ in a different thread 
impacts the other threads. A test should definitely NOT modify anything 
for another test, especially not when it's already running.

So now, I implemented mocking instead (like in my tests for PKCS11 in 
tools/binman/ftest.py) because I know it works.

See:

"""
#!/usr/bin/env python3

import os
import time
import threading
import unittest.mock


def thread_func(n):
     if n == 1:
         time.sleep(1)
     print(f'Thread {n} read environ var FOO={os.environ.get("FOO")}')
     if n == 1:
         time.sleep(1)
     with unittest.mock.patch.dict('os.environ',
                                   {'FOO': f'foo{n}'}):
         print(f'Thread {n} set environ var FOO to foo{n}')
         if n == 0:
             time.sleep(5)
         print(f'Thread {n} read mocked environ var 
FOO={os.environ.get("FOO")}')
         if n == 1:
             time.sleep(6)
     print(f'Thread {n} read environ var FOO={os.environ.get("FOO")}')


threads = []

for i in range(0, 2):
     t = threading.Thread(target=thread_func, args=(i,))
     threads.append(t)

for t in threads:
     t.start()

for t in threads:
     t.join()
"""

Lo and behold, it.... does NOT work???????

I get:

"""
Thread 0 read environ var FOO=None
Thread 0 set environ var FOO to foo0
Thread 1 read environ var FOO=foo0
Thread 1 set environ var FOO to foo1
Thread 1 read mocked environ var FOO=foo1
Thread 0 read mocked environ var FOO=foo1
Thread 0 read environ var FOO=None
Thread 1 read environ var FOO=foo0
"""

I've read that os.environ isn't thread-safe, due to setenv() in glibc 
not being thread-safe: 
https://sourceware.org/glibc/manual/latest/html_node/Environment-Access.html#Environment-Access-1

"""
Modifications of environment variables are not allowed in multi-threaded 
programs.
"""

I'm not sure if this applies to any other Python implementation than 
CPython? But that is likely the one that most people are using.

So... In short, I'm at a loss, no clue how to fix this (if it is even 
fixable). The obvious answer is "spawn multiple processes instead of 
multiple threads" but I can guarantee you, you don't want to be going 
this route as multiprocessing is a lot of headaches in Python. We could 
have the Python thread spawn a subprocess which has a different 
environment if we wanted to (via the `env` command for example), but 
that means not using binman Python API, rather its CLI. We could have 
bintools accept an environment dict that needs to be passed via the 
`env` command or the `env` kwargs of subprocess.Popen().

Headaches, headaches.

Quentin

Re: EXTERNAL - [PATCH v4 6/6] test: binman: Add test for pkcs11 signed capsule

[Simon Glass]

Hi Quentin,

On Tue, 27 Jan 2026 at 00:43, Quentin Schulz <quentin.schulz@cherry.de> wrote:
>
> Hi Simon,
>
> On 1/22/26 11:46 PM, Simon Glass wrote:
> > Hi,
> >
> > On Thu, 22 Jan 2026 at 02:06, Quentin Schulz <quentin.schulz@cherry.de> wrote:
> >>
> >> Hi Wojciech,
> >>
> >> On 1/21/26 1:43 PM, Wojciech Dubowik wrote:
> >>> On Tue, Jan 20, 2026 at 04:53:04PM +0100, Quentin Schulz wrote:
> >>> Hello Quentin,
> >>>> Hi Wojciech,
> >>>>
> >>>> On 1/20/26 9:12 AM, Wojciech Dubowik wrote:
> >> [...]
> >>>>> +        os.environ['SOFTHSM2_CONF'] = softhsm2_conf
> >>>>
> >>>> This is wrong, you'll be messing up with the environment of all tests being
> >>>> run in the same thread. You must use the "with
> >>>> unittest.mock.patch.dict('os.environ'," implementation I used in
> >>>> testFitSignPKCS11Simple.
> >>>
> >>> Well, I have done so in my V2 but has been commented as wrong by the
> >>> first reviewer. I will restore it back.
> >>>
> >>
> >> Indeed, I see Simon asked you to do this in v2 and I missed it. It isn't
> >> how we should be doing it.
> >>
> >> This is likely fine on its own because there's only one test that is now
> >> modifying os.environ's SOFTHSM2_CONF but this will be a problem next
> >> time a test wants to modify it. I actually hit this issue when
> >> developing the PKCS11 fit signing tests as I had two tests modifying the
> >> environment.
> >>
> >> The only trace of it left is the changelog in
> >> https://lore.kernel.org/u-boot/20251121-binman-engine-v3-0-b80180aaa783@cherry.de/
> >>
> >> """
> >> - fixed issues due to modification of the environment in tests failing
> >>     other tests, by using unittest.mock.patch.dict() on os.environ as
> >>     suggested by the unittest.mock doc,
> >> """
> >>
> >> and you can check the diff between the v2 and v3 to check I used to
> >> modify the env directly but now mock it instead.
> >>
> >> Sorry for not catching this, should have answered to Simon in the v2.
> >
> > In practice we try to set values for various which are important, so
> > future tests should explicitly delete the var if needed. But I am OK
>
> This is not working. See this very simple example (too lazy to use
> threading.Lock so synchronization done via time.sleep instead):
>
> """
> #!/usr/bin/env python3
>
> import os
> import time
> import threading
>
>
> def thread_func(n):
>      if n == 1:
>          time.sleep(1)
>      print(f'Thread {n} read environ var FOO={os.environ["FOO"]}')
>      if n == 1:
>          time.sleep(1)
>      print(f'Thread {n} set environ var FOO to foo{n}')
>      os.environ['FOO'] = f'foo{n}'
>      if n == 0:
>          time.sleep(5)
>      print(f'Thread {n} read environ var FOO={os.environ["FOO"]}')
>      if n == 0:
>          print(f'Thread {n} removes environ var FOO')
>          del os.environ["FOO"]
>      else:
>          time.sleep(10)
>          print(f'Thread {n} read environ var FOO={os.environ.get("FOO")}')
>
>
> threads = []
>
> os.environ["FOO"] = "foo"
>
> for i in range(0, 2):
>      t = threading.Thread(target=thread_func, args=(i,))
>      threads.append(t)
>
> for t in threads:
>      t.start()
>
> for t in threads:
>      t.join()
>
> """
>
> This results in:
>
> """
> Thread 0 read environ var FOO=foo
> Thread 0 set environ var FOO to foo0
> Thread 1 read environ var FOO=foo0
> Thread 1 set environ var FOO to foo1
> Thread 1 read environ var FOO=foo1
> Thread 0 read environ var FOO=foo1
> Thread 0 removes environ var FOO
> Thread 1 read environ var FOO=None
> """
>
> You see that modification made to os.environ in a different thread
> impacts the other threads. A test should definitely NOT modify anything
> for another test, especially not when it's already running.
>
> So now, I implemented mocking instead (like in my tests for PKCS11 in
> tools/binman/ftest.py) because I know it works.
>
> See:
>
> """
> #!/usr/bin/env python3
>
> import os
> import time
> import threading
> import unittest.mock
>
>
> def thread_func(n):
>      if n == 1:
>          time.sleep(1)
>      print(f'Thread {n} read environ var FOO={os.environ.get("FOO")}')
>      if n == 1:
>          time.sleep(1)
>      with unittest.mock.patch.dict('os.environ',
>                                    {'FOO': f'foo{n}'}):
>          print(f'Thread {n} set environ var FOO to foo{n}')
>          if n == 0:
>              time.sleep(5)
>          print(f'Thread {n} read mocked environ var
> FOO={os.environ.get("FOO")}')
>          if n == 1:
>              time.sleep(6)
>      print(f'Thread {n} read environ var FOO={os.environ.get("FOO")}')
>
>
> threads = []
>
> for i in range(0, 2):
>      t = threading.Thread(target=thread_func, args=(i,))
>      threads.append(t)
>
> for t in threads:
>      t.start()
>
> for t in threads:
>      t.join()
> """
>
> Lo and behold, it.... does NOT work???????
>
> I get:
>
> """
> Thread 0 read environ var FOO=None
> Thread 0 set environ var FOO to foo0
> Thread 1 read environ var FOO=foo0
> Thread 1 set environ var FOO to foo1
> Thread 1 read mocked environ var FOO=foo1
> Thread 0 read mocked environ var FOO=foo1
> Thread 0 read environ var FOO=None
> Thread 1 read environ var FOO=foo0
> """
>
> I've read that os.environ isn't thread-safe, due to setenv() in glibc
> not being thread-safe:
> https://sourceware.org/glibc/manual/latest/html_node/Environment-Access.html#Environment-Access-1
>
> """
> Modifications of environment variables are not allowed in multi-threaded
> programs.
> """
>
> I'm not sure if this applies to any other Python implementation than
> CPython? But that is likely the one that most people are using.
>
> So... In short, I'm at a loss, no clue how to fix this (if it is even
> fixable). The obvious answer is "spawn multiple processes instead of
> multiple threads" but I can guarantee you, you don't want to be going
> this route as multiprocessing is a lot of headaches in Python. We could
> have the Python thread spawn a subprocess which has a different
> environment if we wanted to (via the `env` command for example), but
> that means not using binman Python API, rather its CLI. We could have
> bintools accept an environment dict that needs to be passed via the
> `env` command or the `env` kwargs of subprocess.Popen().
>
> Headaches, headaches.

I would argue that requiring a particular environment var for a test
is not a great idea. Better to pass an argument through the call stack
and have the external program run with a new environment containing
what is needed.

Regards,
Simon

Re: EXTERNAL - [PATCH v4 6/6] test: binman: Add test for pkcs11 signed capsule

[Wojciech Dubowik]

On Tue, Jan 27, 2026 at 04:00:44PM +1300, Simon Glass wrote:
Hi Simon,
> Hi Quentin,
> 
> On Tue, 27 Jan 2026 at 00:43, Quentin Schulz <quentin.schulz@cherry.de> wrote:
> >
> > Hi Simon,
> >
> > On 1/22/26 11:46 PM, Simon Glass wrote:
> > > Hi,
> > >
> > > On Thu, 22 Jan 2026 at 02:06, Quentin Schulz <quentin.schulz@cherry.de> wrote:
> > >>
> > >> Hi Wojciech,
> > >>
> > >> On 1/21/26 1:43 PM, Wojciech Dubowik wrote:
> > >>> On Tue, Jan 20, 2026 at 04:53:04PM +0100, Quentin Schulz wrote:
> > >>> Hello Quentin,
> > >>>> Hi Wojciech,
> > >>>>
> > >>>> On 1/20/26 9:12 AM, Wojciech Dubowik wrote:
> > >> [...]
> > >>>>> +        os.environ['SOFTHSM2_CONF'] = softhsm2_conf
> > >>>>
> > >>>> This is wrong, you'll be messing up with the environment of all tests being
> > >>>> run in the same thread. You must use the "with
> > >>>> unittest.mock.patch.dict('os.environ'," implementation I used in
> > >>>> testFitSignPKCS11Simple.
> > >>>
> > >>> Well, I have done so in my V2 but has been commented as wrong by the
> > >>> first reviewer. I will restore it back.
> > >>>
> > >>
> > >> Indeed, I see Simon asked you to do this in v2 and I missed it. It isn't
> > >> how we should be doing it.
> > >>
> > >> This is likely fine on its own because there's only one test that is now
> > >> modifying os.environ's SOFTHSM2_CONF but this will be a problem next
> > >> time a test wants to modify it. I actually hit this issue when
> > >> developing the PKCS11 fit signing tests as I had two tests modifying the
> > >> environment.
> > >>
> > >> The only trace of it left is the changelog in
> > >> https://lore.kernel.org/u-boot/20251121-binman-engine-v3-0-b80180aaa783@cherry.de/
> > >>
> > >> """
> > >> - fixed issues due to modification of the environment in tests failing
> > >>     other tests, by using unittest.mock.patch.dict() on os.environ as
> > >>     suggested by the unittest.mock doc,
> > >> """
> > >>
> > >> and you can check the diff between the v2 and v3 to check I used to
> > >> modify the env directly but now mock it instead.
> > >>
> > >> Sorry for not catching this, should have answered to Simon in the v2.
> > >
> > > In practice we try to set values for various which are important, so
> > > future tests should explicitly delete the var if needed. But I am OK
> >
> > This is not working. See this very simple example (too lazy to use
> > threading.Lock so synchronization done via time.sleep instead):
> >
> > """
> > #!/usr/bin/env python3
> >
> > import os
> > import time
> > import threading
> >
> >
> > def thread_func(n):
> >      if n == 1:
> >          time.sleep(1)
> >      print(f'Thread {n} read environ var FOO={os.environ["FOO"]}')
> >      if n == 1:
> >          time.sleep(1)
> >      print(f'Thread {n} set environ var FOO to foo{n}')
> >      os.environ['FOO'] = f'foo{n}'
> >      if n == 0:
> >          time.sleep(5)
> >      print(f'Thread {n} read environ var FOO={os.environ["FOO"]}')
> >      if n == 0:
> >          print(f'Thread {n} removes environ var FOO')
> >          del os.environ["FOO"]
> >      else:
> >          time.sleep(10)
> >          print(f'Thread {n} read environ var FOO={os.environ.get("FOO")}')
> >
> >
> > threads = []
> >
> > os.environ["FOO"] = "foo"
> >
> > for i in range(0, 2):
> >      t = threading.Thread(target=thread_func, args=(i,))
> >      threads.append(t)
> >
> > for t in threads:
> >      t.start()
> >
> > for t in threads:
> >      t.join()
> >
> > """
> >
> > This results in:
> >
> > """
> > Thread 0 read environ var FOO=foo
> > Thread 0 set environ var FOO to foo0
> > Thread 1 read environ var FOO=foo0
> > Thread 1 set environ var FOO to foo1
> > Thread 1 read environ var FOO=foo1
> > Thread 0 read environ var FOO=foo1
> > Thread 0 removes environ var FOO
> > Thread 1 read environ var FOO=None
> > """
> >
> > You see that modification made to os.environ in a different thread
> > impacts the other threads. A test should definitely NOT modify anything
> > for another test, especially not when it's already running.
> >
> > So now, I implemented mocking instead (like in my tests for PKCS11 in
> > tools/binman/ftest.py) because I know it works.
> >
> > See:
> >
> > """
> > #!/usr/bin/env python3
> >
> > import os
> > import time
> > import threading
> > import unittest.mock
> >
> >
> > def thread_func(n):
> >      if n == 1:
> >          time.sleep(1)
> >      print(f'Thread {n} read environ var FOO={os.environ.get("FOO")}')
> >      if n == 1:
> >          time.sleep(1)
> >      with unittest.mock.patch.dict('os.environ',
> >                                    {'FOO': f'foo{n}'}):
> >          print(f'Thread {n} set environ var FOO to foo{n}')
> >          if n == 0:
> >              time.sleep(5)
> >          print(f'Thread {n} read mocked environ var
> > FOO={os.environ.get("FOO")}')
> >          if n == 1:
> >              time.sleep(6)
> >      print(f'Thread {n} read environ var FOO={os.environ.get("FOO")}')
> >
> >
> > threads = []
> >
> > for i in range(0, 2):
> >      t = threading.Thread(target=thread_func, args=(i,))
> >      threads.append(t)
> >
> > for t in threads:
> >      t.start()
> >
> > for t in threads:
> >      t.join()
> > """
> >
> > Lo and behold, it.... does NOT work???????
> >
> > I get:
> >
> > """
> > Thread 0 read environ var FOO=None
> > Thread 0 set environ var FOO to foo0
> > Thread 1 read environ var FOO=foo0
> > Thread 1 set environ var FOO to foo1
> > Thread 1 read mocked environ var FOO=foo1
> > Thread 0 read mocked environ var FOO=foo1
> > Thread 0 read environ var FOO=None
> > Thread 1 read environ var FOO=foo0
> > """
> >
> > I've read that os.environ isn't thread-safe, due to setenv() in glibc
> > not being thread-safe:
> > https://sourceware.org/glibc/manual/latest/html_node/Environment-Access.html#Environment-Access-1
> >
> > """
> > Modifications of environment variables are not allowed in multi-threaded
> > programs.
> > """
> >
> > I'm not sure if this applies to any other Python implementation than
> > CPython? But that is likely the one that most people are using.
> >
> > So... In short, I'm at a loss, no clue how to fix this (if it is even
> > fixable). The obvious answer is "spawn multiple processes instead of
> > multiple threads" but I can guarantee you, you don't want to be going
> > this route as multiprocessing is a lot of headaches in Python. We could
> > have the Python thread spawn a subprocess which has a different
> > environment if we wanted to (via the `env` command for example), but
> > that means not using binman Python API, rather its CLI. We could have
> > bintools accept an environment dict that needs to be passed via the
> > `env` command or the `env` kwargs of subprocess.Popen().
> >
> > Headaches, headaches.
> 
> I would argue that requiring a particular environment var for a test
> is not a great idea. Better to pass an argument through the call stack
> and have the external program run with a new environment containing
> what is needed.
Would it be ok to call softhsm utils with softhsm_conf like
SOFTHSM2_CONF=... softhsm2-util? Then we don't have to mock environment.
I would still have to set SOFTHSM install dir location with the env variable
for mkeficapsule in binman generation. I guess it doesn't affect other
tests as we don't need to change it.

Regards,
Wojtek

> 
> Regards,
> Simon

Re: EXTERNAL - [PATCH v4 6/6] test: binman: Add test for pkcs11 signed capsule

[Simon Glass]

Hi Wojciech,

On Tue, 3 Feb 2026 at 21:18, Wojciech Dubowik <Wojciech.Dubowik@mt.com> wrote:
>
> On Tue, Jan 27, 2026 at 04:00:44PM +1300, Simon Glass wrote:
> Hi Simon,
> > Hi Quentin,
> >
> > On Tue, 27 Jan 2026 at 00:43, Quentin Schulz <quentin.schulz@cherry.de> wrote:
> > >
> > > Hi Simon,
> > >
> > > On 1/22/26 11:46 PM, Simon Glass wrote:
> > > > Hi,
> > > >
> > > > On Thu, 22 Jan 2026 at 02:06, Quentin Schulz <quentin.schulz@cherry.de> wrote:
> > > >>
> > > >> Hi Wojciech,
> > > >>
> > > >> On 1/21/26 1:43 PM, Wojciech Dubowik wrote:
> > > >>> On Tue, Jan 20, 2026 at 04:53:04PM +0100, Quentin Schulz wrote:
> > > >>> Hello Quentin,
> > > >>>> Hi Wojciech,
> > > >>>>
> > > >>>> On 1/20/26 9:12 AM, Wojciech Dubowik wrote:
> > > >> [...]
> > > >>>>> +        os.environ['SOFTHSM2_CONF'] = softhsm2_conf
> > > >>>>
> > > >>>> This is wrong, you'll be messing up with the environment of all tests being
> > > >>>> run in the same thread. You must use the "with
> > > >>>> unittest.mock.patch.dict('os.environ'," implementation I used in
> > > >>>> testFitSignPKCS11Simple.
> > > >>>
> > > >>> Well, I have done so in my V2 but has been commented as wrong by the
> > > >>> first reviewer. I will restore it back.
> > > >>>
> > > >>
> > > >> Indeed, I see Simon asked you to do this in v2 and I missed it. It isn't
> > > >> how we should be doing it.
> > > >>
> > > >> This is likely fine on its own because there's only one test that is now
> > > >> modifying os.environ's SOFTHSM2_CONF but this will be a problem next
> > > >> time a test wants to modify it. I actually hit this issue when
> > > >> developing the PKCS11 fit signing tests as I had two tests modifying the
> > > >> environment.
> > > >>
> > > >> The only trace of it left is the changelog in
> > > >> https://lore.kernel.org/u-boot/20251121-binman-engine-v3-0-b80180aaa783@cherry.de/
> > > >>
> > > >> """
> > > >> - fixed issues due to modification of the environment in tests failing
> > > >>     other tests, by using unittest.mock.patch.dict() on os.environ as
> > > >>     suggested by the unittest.mock doc,
> > > >> """
> > > >>
> > > >> and you can check the diff between the v2 and v3 to check I used to
> > > >> modify the env directly but now mock it instead.
> > > >>
> > > >> Sorry for not catching this, should have answered to Simon in the v2.
> > > >
> > > > In practice we try to set values for various which are important, so
> > > > future tests should explicitly delete the var if needed. But I am OK
> > >
> > > This is not working. See this very simple example (too lazy to use
> > > threading.Lock so synchronization done via time.sleep instead):
> > >
> > > """
> > > #!/usr/bin/env python3
> > >
> > > import os
> > > import time
> > > import threading
> > >
> > >
> > > def thread_func(n):
> > >      if n == 1:
> > >          time.sleep(1)
> > >      print(f'Thread {n} read environ var FOO={os.environ["FOO"]}')
> > >      if n == 1:
> > >          time.sleep(1)
> > >      print(f'Thread {n} set environ var FOO to foo{n}')
> > >      os.environ['FOO'] = f'foo{n}'
> > >      if n == 0:
> > >          time.sleep(5)
> > >      print(f'Thread {n} read environ var FOO={os.environ["FOO"]}')
> > >      if n == 0:
> > >          print(f'Thread {n} removes environ var FOO')
> > >          del os.environ["FOO"]
> > >      else:
> > >          time.sleep(10)
> > >          print(f'Thread {n} read environ var FOO={os.environ.get("FOO")}')
> > >
> > >
> > > threads = []
> > >
> > > os.environ["FOO"] = "foo"
> > >
> > > for i in range(0, 2):
> > >      t = threading.Thread(target=thread_func, args=(i,))
> > >      threads.append(t)
> > >
> > > for t in threads:
> > >      t.start()
> > >
> > > for t in threads:
> > >      t.join()
> > >
> > > """
> > >
> > > This results in:
> > >
> > > """
> > > Thread 0 read environ var FOO=foo
> > > Thread 0 set environ var FOO to foo0
> > > Thread 1 read environ var FOO=foo0
> > > Thread 1 set environ var FOO to foo1
> > > Thread 1 read environ var FOO=foo1
> > > Thread 0 read environ var FOO=foo1
> > > Thread 0 removes environ var FOO
> > > Thread 1 read environ var FOO=None
> > > """
> > >
> > > You see that modification made to os.environ in a different thread
> > > impacts the other threads. A test should definitely NOT modify anything
> > > for another test, especially not when it's already running.
> > >
> > > So now, I implemented mocking instead (like in my tests for PKCS11 in
> > > tools/binman/ftest.py) because I know it works.
> > >
> > > See:
> > >
> > > """
> > > #!/usr/bin/env python3
> > >
> > > import os
> > > import time
> > > import threading
> > > import unittest.mock
> > >
> > >
> > > def thread_func(n):
> > >      if n == 1:
> > >          time.sleep(1)
> > >      print(f'Thread {n} read environ var FOO={os.environ.get("FOO")}')
> > >      if n == 1:
> > >          time.sleep(1)
> > >      with unittest.mock.patch.dict('os.environ',
> > >                                    {'FOO': f'foo{n}'}):
> > >          print(f'Thread {n} set environ var FOO to foo{n}')
> > >          if n == 0:
> > >              time.sleep(5)
> > >          print(f'Thread {n} read mocked environ var
> > > FOO={os.environ.get("FOO")}')
> > >          if n == 1:
> > >              time.sleep(6)
> > >      print(f'Thread {n} read environ var FOO={os.environ.get("FOO")}')
> > >
> > >
> > > threads = []
> > >
> > > for i in range(0, 2):
> > >      t = threading.Thread(target=thread_func, args=(i,))
> > >      threads.append(t)
> > >
> > > for t in threads:
> > >      t.start()
> > >
> > > for t in threads:
> > >      t.join()
> > > """
> > >
> > > Lo and behold, it.... does NOT work???????
> > >
> > > I get:
> > >
> > > """
> > > Thread 0 read environ var FOO=None
> > > Thread 0 set environ var FOO to foo0
> > > Thread 1 read environ var FOO=foo0
> > > Thread 1 set environ var FOO to foo1
> > > Thread 1 read mocked environ var FOO=foo1
> > > Thread 0 read mocked environ var FOO=foo1
> > > Thread 0 read environ var FOO=None
> > > Thread 1 read environ var FOO=foo0
> > > """
> > >
> > > I've read that os.environ isn't thread-safe, due to setenv() in glibc
> > > not being thread-safe:
> > > https://sourceware.org/glibc/manual/latest/html_node/Environment-Access.html#Environment-Access-1
> > >
> > > """
> > > Modifications of environment variables are not allowed in multi-threaded
> > > programs.
> > > """
> > >
> > > I'm not sure if this applies to any other Python implementation than
> > > CPython? But that is likely the one that most people are using.
> > >
> > > So... In short, I'm at a loss, no clue how to fix this (if it is even
> > > fixable). The obvious answer is "spawn multiple processes instead of
> > > multiple threads" but I can guarantee you, you don't want to be going
> > > this route as multiprocessing is a lot of headaches in Python. We could
> > > have the Python thread spawn a subprocess which has a different
> > > environment if we wanted to (via the `env` command for example), but
> > > that means not using binman Python API, rather its CLI. We could have
> > > bintools accept an environment dict that needs to be passed via the
> > > `env` command or the `env` kwargs of subprocess.Popen().
> > >
> > > Headaches, headaches.
> >
> > I would argue that requiring a particular environment var for a test
> > is not a great idea. Better to pass an argument through the call stack
> > and have the external program run with a new environment containing
> > what is needed.
> Would it be ok to call softhsm utils with softhsm_conf like
> SOFTHSM2_CONF=... softhsm2-util? Then we don't have to mock environment.
> I would still have to set SOFTHSM install dir location with the env variable
> for mkeficapsule in binman generation. I guess it doesn't affect other
> tests as we don't need to change it.
>

So long as it works it seems fine to me.

Regards,
Simon

Re: EXTERNAL - [PATCH v4 6/6] test: binman: Add test for pkcs11 signed capsule

[Quentin Schulz]

Hi Wojciech,

On 2/3/26 9:17 AM, Wojciech Dubowik wrote:
> On Tue, Jan 27, 2026 at 04:00:44PM +1300, Simon Glass wrote:
> Hi Simon,
>> Hi Quentin,
>>
>> On Tue, 27 Jan 2026 at 00:43, Quentin Schulz <quentin.schulz@cherry.de> wrote:
>>>
>>> Hi Simon,
>>>
>>> On 1/22/26 11:46 PM, Simon Glass wrote:
>>>> Hi,
>>>>
>>>> On Thu, 22 Jan 2026 at 02:06, Quentin Schulz <quentin.schulz@cherry.de> wrote:
>>>>>
>>>>> Hi Wojciech,
>>>>>
>>>>> On 1/21/26 1:43 PM, Wojciech Dubowik wrote:
>>>>>> On Tue, Jan 20, 2026 at 04:53:04PM +0100, Quentin Schulz wrote:
>>>>>> Hello Quentin,
>>>>>>> Hi Wojciech,
>>>>>>>
>>>>>>> On 1/20/26 9:12 AM, Wojciech Dubowik wrote:
>>>>> [...]
>>>>>>>> +        os.environ['SOFTHSM2_CONF'] = softhsm2_conf
>>>>>>>
>>>>>>> This is wrong, you'll be messing up with the environment of all tests being
>>>>>>> run in the same thread. You must use the "with
>>>>>>> unittest.mock.patch.dict('os.environ'," implementation I used in
>>>>>>> testFitSignPKCS11Simple.
>>>>>>
>>>>>> Well, I have done so in my V2 but has been commented as wrong by the
>>>>>> first reviewer. I will restore it back.
>>>>>>
>>>>>
>>>>> Indeed, I see Simon asked you to do this in v2 and I missed it. It isn't
>>>>> how we should be doing it.
>>>>>
>>>>> This is likely fine on its own because there's only one test that is now
>>>>> modifying os.environ's SOFTHSM2_CONF but this will be a problem next
>>>>> time a test wants to modify it. I actually hit this issue when
>>>>> developing the PKCS11 fit signing tests as I had two tests modifying the
>>>>> environment.
>>>>>
>>>>> The only trace of it left is the changelog in
>>>>> https://lore.kernel.org/u-boot/20251121-binman-engine-v3-0-b80180aaa783@cherry.de/
>>>>>
>>>>> """
>>>>> - fixed issues due to modification of the environment in tests failing
>>>>>      other tests, by using unittest.mock.patch.dict() on os.environ as
>>>>>      suggested by the unittest.mock doc,
>>>>> """
>>>>>
>>>>> and you can check the diff between the v2 and v3 to check I used to
>>>>> modify the env directly but now mock it instead.
>>>>>
>>>>> Sorry for not catching this, should have answered to Simon in the v2.
>>>>
>>>> In practice we try to set values for various which are important, so
>>>> future tests should explicitly delete the var if needed. But I am OK
>>>
>>> This is not working. See this very simple example (too lazy to use
>>> threading.Lock so synchronization done via time.sleep instead):
>>>
>>> """
>>> #!/usr/bin/env python3
>>>
>>> import os
>>> import time
>>> import threading
>>>
>>>
>>> def thread_func(n):
>>>       if n == 1:
>>>           time.sleep(1)
>>>       print(f'Thread {n} read environ var FOO={os.environ["FOO"]}')
>>>       if n == 1:
>>>           time.sleep(1)
>>>       print(f'Thread {n} set environ var FOO to foo{n}')
>>>       os.environ['FOO'] = f'foo{n}'
>>>       if n == 0:
>>>           time.sleep(5)
>>>       print(f'Thread {n} read environ var FOO={os.environ["FOO"]}')
>>>       if n == 0:
>>>           print(f'Thread {n} removes environ var FOO')
>>>           del os.environ["FOO"]
>>>       else:
>>>           time.sleep(10)
>>>           print(f'Thread {n} read environ var FOO={os.environ.get("FOO")}')
>>>
>>>
>>> threads = []
>>>
>>> os.environ["FOO"] = "foo"
>>>
>>> for i in range(0, 2):
>>>       t = threading.Thread(target=thread_func, args=(i,))
>>>       threads.append(t)
>>>
>>> for t in threads:
>>>       t.start()
>>>
>>> for t in threads:
>>>       t.join()
>>>
>>> """
>>>
>>> This results in:
>>>
>>> """
>>> Thread 0 read environ var FOO=foo
>>> Thread 0 set environ var FOO to foo0
>>> Thread 1 read environ var FOO=foo0
>>> Thread 1 set environ var FOO to foo1
>>> Thread 1 read environ var FOO=foo1
>>> Thread 0 read environ var FOO=foo1
>>> Thread 0 removes environ var FOO
>>> Thread 1 read environ var FOO=None
>>> """
>>>
>>> You see that modification made to os.environ in a different thread
>>> impacts the other threads. A test should definitely NOT modify anything
>>> for another test, especially not when it's already running.
>>>
>>> So now, I implemented mocking instead (like in my tests for PKCS11 in
>>> tools/binman/ftest.py) because I know it works.
>>>
>>> See:
>>>
>>> """
>>> #!/usr/bin/env python3
>>>
>>> import os
>>> import time
>>> import threading
>>> import unittest.mock
>>>
>>>
>>> def thread_func(n):
>>>       if n == 1:
>>>           time.sleep(1)
>>>       print(f'Thread {n} read environ var FOO={os.environ.get("FOO")}')
>>>       if n == 1:
>>>           time.sleep(1)
>>>       with unittest.mock.patch.dict('os.environ',
>>>                                     {'FOO': f'foo{n}'}):
>>>           print(f'Thread {n} set environ var FOO to foo{n}')
>>>           if n == 0:
>>>               time.sleep(5)
>>>           print(f'Thread {n} read mocked environ var
>>> FOO={os.environ.get("FOO")}')
>>>           if n == 1:
>>>               time.sleep(6)
>>>       print(f'Thread {n} read environ var FOO={os.environ.get("FOO")}')
>>>
>>>
>>> threads = []
>>>
>>> for i in range(0, 2):
>>>       t = threading.Thread(target=thread_func, args=(i,))
>>>       threads.append(t)
>>>
>>> for t in threads:
>>>       t.start()
>>>
>>> for t in threads:
>>>       t.join()
>>> """
>>>
>>> Lo and behold, it.... does NOT work???????
>>>
>>> I get:
>>>
>>> """
>>> Thread 0 read environ var FOO=None
>>> Thread 0 set environ var FOO to foo0
>>> Thread 1 read environ var FOO=foo0
>>> Thread 1 set environ var FOO to foo1
>>> Thread 1 read mocked environ var FOO=foo1
>>> Thread 0 read mocked environ var FOO=foo1
>>> Thread 0 read environ var FOO=None
>>> Thread 1 read environ var FOO=foo0
>>> """
>>>
>>> I've read that os.environ isn't thread-safe, due to setenv() in glibc
>>> not being thread-safe:
>>> https://sourceware.org/glibc/manual/latest/html_node/Environment-Access.html#Environment-Access-1
>>>
>>> """
>>> Modifications of environment variables are not allowed in multi-threaded
>>> programs.
>>> """
>>>
>>> I'm not sure if this applies to any other Python implementation than
>>> CPython? But that is likely the one that most people are using.
>>>
>>> So... In short, I'm at a loss, no clue how to fix this (if it is even
>>> fixable). The obvious answer is "spawn multiple processes instead of
>>> multiple threads" but I can guarantee you, you don't want to be going
>>> this route as multiprocessing is a lot of headaches in Python. We could
>>> have the Python thread spawn a subprocess which has a different
>>> environment if we wanted to (via the `env` command for example), but
>>> that means not using binman Python API, rather its CLI. We could have
>>> bintools accept an environment dict that needs to be passed via the
>>> `env` command or the `env` kwargs of subprocess.Popen().
>>>
>>> Headaches, headaches.
>>
>> I would argue that requiring a particular environment var for a test
>> is not a great idea. Better to pass an argument through the call stack
>> and have the external program run with a new environment containing
>> what is needed.
> Would it be ok to call softhsm utils with softhsm_conf like
> SOFTHSM2_CONF=... softhsm2-util? Then we don't have to mock environment.
> I would still have to set SOFTHSM install dir location with the env variable
> for mkeficapsule in binman generation. I guess it doesn't affect other
> tests as we don't need to change it.
> 

I would recommend to hook this to command.run_pipe(..., env=env, ...) in 
tools/binman/bintool.py:Bintool:run_cmd_result() if possible, such that 
it isn't bintool-specific.

We already override the env with the path to our tools, so we could 
simply extend it. I guess we could have a Bintool:set_env() or 
add_to_env() and store the result in some variable (have 
tools.get_env_with_path() in it by default in the object init?) and have 
run_cmd_result() use the bintool object's self.env dict insted of 
calling tools.get_env_with_path()? Then in ftest.py, after b = 
bintool.Bintool.create(), b.add_to_env({'SOFTHSM2_CONF': '...'})?

I'm not sure, maybe there's a reason tools.get_env_with_path() is called 
at the moment the command needs to be run and not when the bintool is 
created, in which case maybe store extra_env and update the env returned 
by tools.get_env_with_path() with that?

Cheers,
Quentin

Re: EXTERNAL - [PATCH v4 6/6] test: binman: Add test for pkcs11 signed capsule

[Quentin Schulz]

Hi Wojciech, Simon,

On 2/4/26 10:49 AM, Quentin Schulz wrote:
> Hi Wojciech,
> 
> On 2/3/26 9:17 AM, Wojciech Dubowik wrote:
>> On Tue, Jan 27, 2026 at 04:00:44PM +1300, Simon Glass wrote:
>> Hi Simon,
>>> Hi Quentin,
>>>
>>> On Tue, 27 Jan 2026 at 00:43, Quentin Schulz 
>>> <quentin.schulz@cherry.de> wrote:
>>>>
>>>> Hi Simon,
>>>>
>>>> On 1/22/26 11:46 PM, Simon Glass wrote:
>>>>> Hi,
>>>>>
>>>>> On Thu, 22 Jan 2026 at 02:06, Quentin Schulz 
>>>>> <quentin.schulz@cherry.de> wrote:
>>>>>>
>>>>>> Hi Wojciech,
>>>>>>
>>>>>> On 1/21/26 1:43 PM, Wojciech Dubowik wrote:
>>>>>>> On Tue, Jan 20, 2026 at 04:53:04PM +0100, Quentin Schulz wrote:
>>>>>>> Hello Quentin,
>>>>>>>> Hi Wojciech,
>>>>>>>>
>>>>>>>> On 1/20/26 9:12 AM, Wojciech Dubowik wrote:
>>>>>> [...]
>>>>>>>>> +        os.environ['SOFTHSM2_CONF'] = softhsm2_conf
>>>>>>>>
>>>>>>>> This is wrong, you'll be messing up with the environment of all 
>>>>>>>> tests being
>>>>>>>> run in the same thread. You must use the "with
>>>>>>>> unittest.mock.patch.dict('os.environ'," implementation I used in
>>>>>>>> testFitSignPKCS11Simple.
>>>>>>>
>>>>>>> Well, I have done so in my V2 but has been commented as wrong by the
>>>>>>> first reviewer. I will restore it back.
>>>>>>>
>>>>>>
>>>>>> Indeed, I see Simon asked you to do this in v2 and I missed it. It 
>>>>>> isn't
>>>>>> how we should be doing it.
>>>>>>
>>>>>> This is likely fine on its own because there's only one test that 
>>>>>> is now
>>>>>> modifying os.environ's SOFTHSM2_CONF but this will be a problem next
>>>>>> time a test wants to modify it. I actually hit this issue when
>>>>>> developing the PKCS11 fit signing tests as I had two tests 
>>>>>> modifying the
>>>>>> environment.
>>>>>>
>>>>>> The only trace of it left is the changelog in
>>>>>> https://eur02.safelinks.protection.outlook.com/? 
>>>>>> url=https%3A%2F%2Flore.kernel.org%2Fu-boot%2F20251121-binman- 
>>>>>> engine-v3-0- 
>>>>>> b80180aaa783%40cherry.de%2F&data=05%7C02%7Cquentin.schulz%40cherry.de%7C4be38001079e40572d5708de63d2c7a6%7C5e0e1b5221b54e7b83bb514ec460677e%7C0%7C0%7C639057954135737078%7CUnknown%7CTWFpbGZsb3d8eyJFbXB0eU1hcGkiOnRydWUsIlYiOiIwLjAuMDAwMCIsIlAiOiJXaW4zMiIsIkFOIjoiTWFpbCIsIldUIjoyfQ%3D%3D%7C0%7C%7C%7C&sdata=B5%2Bx5iUO54EpovAyDBUCwuq5NgcfcejIoD6Unhdoa%2FM%3D&reserved=0
>>>>>>
>>>>>> """
>>>>>> - fixed issues due to modification of the environment in tests 
>>>>>> failing
>>>>>>      other tests, by using unittest.mock.patch.dict() on 
>>>>>> os.environ as
>>>>>>      suggested by the unittest.mock doc,
>>>>>> """
>>>>>>
>>>>>> and you can check the diff between the v2 and v3 to check I used to
>>>>>> modify the env directly but now mock it instead.
>>>>>>
>>>>>> Sorry for not catching this, should have answered to Simon in the v2.
>>>>>
>>>>> In practice we try to set values for various which are important, so
>>>>> future tests should explicitly delete the var if needed. But I am OK
>>>>
>>>> This is not working. See this very simple example (too lazy to use
>>>> threading.Lock so synchronization done via time.sleep instead):
>>>>
>>>> """
>>>> #!/usr/bin/env python3
>>>>
>>>> import os
>>>> import time
>>>> import threading
>>>>
>>>>
>>>> def thread_func(n):
>>>>       if n == 1:
>>>>           time.sleep(1)
>>>>       print(f'Thread {n} read environ var FOO={os.environ["FOO"]}')
>>>>       if n == 1:
>>>>           time.sleep(1)
>>>>       print(f'Thread {n} set environ var FOO to foo{n}')
>>>>       os.environ['FOO'] = f'foo{n}'
>>>>       if n == 0:
>>>>           time.sleep(5)
>>>>       print(f'Thread {n} read environ var FOO={os.environ["FOO"]}')
>>>>       if n == 0:
>>>>           print(f'Thread {n} removes environ var FOO')
>>>>           del os.environ["FOO"]
>>>>       else:
>>>>           time.sleep(10)
>>>>           print(f'Thread {n} read environ var 
>>>> FOO={os.environ.get("FOO")}')
>>>>
>>>>
>>>> threads = []
>>>>
>>>> os.environ["FOO"] = "foo"
>>>>
>>>> for i in range(0, 2):
>>>>       t = threading.Thread(target=thread_func, args=(i,))
>>>>       threads.append(t)
>>>>
>>>> for t in threads:
>>>>       t.start()
>>>>
>>>> for t in threads:
>>>>       t.join()
>>>>
>>>> """
>>>>
>>>> This results in:
>>>>
>>>> """
>>>> Thread 0 read environ var FOO=foo
>>>> Thread 0 set environ var FOO to foo0
>>>> Thread 1 read environ var FOO=foo0
>>>> Thread 1 set environ var FOO to foo1
>>>> Thread 1 read environ var FOO=foo1
>>>> Thread 0 read environ var FOO=foo1
>>>> Thread 0 removes environ var FOO
>>>> Thread 1 read environ var FOO=None
>>>> """
>>>>
>>>> You see that modification made to os.environ in a different thread
>>>> impacts the other threads. A test should definitely NOT modify anything
>>>> for another test, especially not when it's already running.
>>>>
>>>> So now, I implemented mocking instead (like in my tests for PKCS11 in
>>>> tools/binman/ftest.py) because I know it works.
>>>>
>>>> See:
>>>>
>>>> """
>>>> #!/usr/bin/env python3
>>>>
>>>> import os
>>>> import time
>>>> import threading
>>>> import unittest.mock
>>>>
>>>>
>>>> def thread_func(n):
>>>>       if n == 1:
>>>>           time.sleep(1)
>>>>       print(f'Thread {n} read environ var FOO={os.environ.get("FOO")}')
>>>>       if n == 1:
>>>>           time.sleep(1)
>>>>       with unittest.mock.patch.dict('os.environ',
>>>>                                     {'FOO': f'foo{n}'}):
>>>>           print(f'Thread {n} set environ var FOO to foo{n}')
>>>>           if n == 0:
>>>>               time.sleep(5)
>>>>           print(f'Thread {n} read mocked environ var
>>>> FOO={os.environ.get("FOO")}')
>>>>           if n == 1:
>>>>               time.sleep(6)
>>>>       print(f'Thread {n} read environ var FOO={os.environ.get("FOO")}')
>>>>
>>>>
>>>> threads = []
>>>>
>>>> for i in range(0, 2):
>>>>       t = threading.Thread(target=thread_func, args=(i,))
>>>>       threads.append(t)
>>>>
>>>> for t in threads:
>>>>       t.start()
>>>>
>>>> for t in threads:
>>>>       t.join()
>>>> """
>>>>
>>>> Lo and behold, it.... does NOT work???????
>>>>
>>>> I get:
>>>>
>>>> """
>>>> Thread 0 read environ var FOO=None
>>>> Thread 0 set environ var FOO to foo0
>>>> Thread 1 read environ var FOO=foo0
>>>> Thread 1 set environ var FOO to foo1
>>>> Thread 1 read mocked environ var FOO=foo1
>>>> Thread 0 read mocked environ var FOO=foo1
>>>> Thread 0 read environ var FOO=None
>>>> Thread 1 read environ var FOO=foo0
>>>> """
>>>>
>>>> I've read that os.environ isn't thread-safe, due to setenv() in glibc
>>>> not being thread-safe:
>>>> https://eur02.safelinks.protection.outlook.com/? 
>>>> url=https%3A%2F%2Fsourceware.org%2Fglibc%2Fmanual%2Flatest%2Fhtml_node%2FEnvironment-Access.html%23Environment-Access-1&data=05%7C02%7Cquentin.schulz%40cherry.de%7C4be38001079e40572d5708de63d2c7a6%7C5e0e1b5221b54e7b83bb514ec460677e%7C0%7C0%7C639057954135756872%7CUnknown%7CTWFpbGZsb3d8eyJFbXB0eU1hcGkiOnRydWUsIlYiOiIwLjAuMDAwMCIsIlAiOiJXaW4zMiIsIkFOIjoiTWFpbCIsIldUIjoyfQ%3D%3D%7C0%7C%7C%7C&sdata=K5LZPB7BkLnUsLU%2BFFK4Bmp1vOXqF8WiSryObldGBAY%3D&reserved=0
>>>>
>>>> """
>>>> Modifications of environment variables are not allowed in multi- 
>>>> threaded
>>>> programs.
>>>> """
>>>>
>>>> I'm not sure if this applies to any other Python implementation than
>>>> CPython? But that is likely the one that most people are using.
>>>>
>>>> So... In short, I'm at a loss, no clue how to fix this (if it is even
>>>> fixable). The obvious answer is "spawn multiple processes instead of
>>>> multiple threads" but I can guarantee you, you don't want to be going

We are already doing this actually and this is what I was missing.

When using binman in parallel (the default when not passing -P1), we 
create a ConcurrentTestSuite whose make_tests function "pointer" 
parameter is fork_for_tests() from concurrencytest. What this does is 
collect all tests to run from all test suites, divide them equally to 
the number of processes then fork the main process that many times. 
Before forking, concurrencytest creates a pipe for communication between 
the child process and main process. The main process keeps track of all 
those pipes and generate a TestCase list (one per child process) that 
ConcurrentTestSuite then consumes. This will then create a thread per 
TestCase. This essentially means that:

- tests are run in different processes,
- but each process runs multiple tests sequentially (not in parallel, 
not in separate threads),
- threads are used to get the output of tests from a different process 
but that's just implementation details,

As such, we don't have to care about thread-safety as that is only used 
by concurrencytest for communication purposes.

However, this does mean that we absolutely need to clean up after 
ourselves. This is the reason why modifications to os.environ were 
breaking stuff in other tests for me, not because of thread-unsafe code, 
but because sequential tests inherit the dirty laundry from the previous 
test and I didn't clean things up properly. This explains why using 
unittest.mock.patch() actually fixed things up, because it's handled by 
the context manager anyway.

So we have the option of using either a contextmanager 
(unittest.mock.patch) for setting the environment and not having to care 
about cleaning it up afterwards. Or have the code snippet setting the 
environment and what uses it in a big try:finally which then cleans up 
before leaving the test.

I don't think propagating the env through Binman (see _DoBinman() which 
runs binman like a Python module thus inheriting the current 
environment) will make things easier or more readable, so please ignore 
the rest of the mail I answer to.

The TL;DR is: no thread-safety issue in tools/binman because we don't 
use threads. Use unittest.mock.patch contextmanager, or write to 
os.environ and undo it before exiting the test (and this needs to be 
done in a try:finally block to avoid exiting early).

Cheers,
Quentin