Hi Lorenz,
On 2026-09-17T06:55:42, Lorenz Kofler <[email protected]> wrote:
> binman: pre-load: Allow signing with an OpenSSL engine
>
> The pre-load entry type imports a private key from a file and signs with
> Cryptodome, so the key must be readable as a file. Keys held in an HSM
> or on a smart card cannot be used. The FIT entry type is not affected as
> it delegates signing to mkimage, which signs through the OpenSSL engine
> API.
>
> Add an optional 'engine' property. When set, sign with openssl using
> that engine and interpret 'key-name' as the engine-specific key
> identifier, instead of reading a key from 'pre-load-key-path'.
>
> For the pkcs11 engine, expand a 'key-name' that is not already a PKCS#11
> URI to:
>
> pkcs11:object=<key-name>;type=private
>
> With an engine, binman never loads the key itself and so cannot check
> its size before signing. Check the length of the resulting RSA signature
> instead, which is the modulus size either way.
> [...]
>
> tools/binman/btool/openssl.py | 28 ++++++++++++++
> tools/binman/etype/pre_load.py | 85
> ++++++++++++++++++++++++++++++++++++------
> tools/binman/ftest.py | 12 ++++--
> 3 files changed, 111 insertions(+), 14 deletions(-)
> + def _sign_engine(self, data, hash_name, padding_name):
> + """Sign data with an OpenSSL engine
> +
> + Args:
> + data (bytes): Data to sign
> + hash_name (str): Hash algorithm to use
> + padding_name (str): Padding to use
> +
> + Returns:
> + bytes: Signature, or zeroes if openssl is missing
> + """
> + key_id = self.key_name
> + if self.engine == 'pkcs11' and not key_id.startswith('pkcs11:'):
> + key_id = f'pkcs11:object={key_id};type=private'
If key-name is absent, self.key_name is None and this crashes with an
unhelpful AttributeError from startswith(). Please can you validate
that key_name is set when engine is used and raise via self.Raise().
> diff --git a/tools/binman/etype/pre_load.py b/tools/binman/etype/pre_load.py
> @@ -78,6 +98,7 @@ class Entry_pre_load(Entry_collection):
> + self.engine = fdt_util.GetString(self._node, 'engine')
Embedding the pkcs11 URI expansion in the etype ties this generic
engine knob to one specific engine. If another engine with different
URI conventions is added later, the etype grows another special case.
I'd prefer the etype pass key-name through unchanged and require the
user to supply a full PKCS#11 URI - or move the munging into
openssl.sign() where the engine-specific knowledge lives. What do you
think?
> diff --git a/tools/binman/etype/pre_load.py b/tools/binman/etype/pre_load.py
> @@ -115,7 +167,7 @@
> + # An engine does not expose its key, so infer its size from the
> signature
> + if self.engine and len(sig) != RSAS[sign_name]:
> + self.Raise("The key " + self.key_name + " don't have the
> expected size")
Worth a short comment noting that _sign_engine() returns a zero-filled
placeholder of exactly RSAS[sign_name] bytes when openssl is missing,
so this check silently passes and the missing-bintool path is
preserved. The placeholder-length coupling is not obvious.
> diff --git a/tools/binman/ftest.py b/tools/binman/ftest.py
> @@ -5903,12 +5903,18 @@ fdt fdtmap Extract the
> devicetree blob from the fdtmap
> + with unittest.mock.patch.object(
> + pre_load.Entry_pre_load, '_sign',
> + side_effect=AssertionError('signing should not be
> attempted')):
> + with self.assertRaisesRegex(ValueError,
> + "don't have the expected size"):
> + self._DoReadFileDtb('security/pre_load_invalid_key.dts',
> + entry_args=entry_args)
This belongs in patch 1, where _sign() is introduced and
_CreateHeader() is reshuffled - the test would have caught a
regression in that refactor. Please can you move it there and keep
patch 2 focused on the engine change?
Regards,
Simon
