Hi Yan, On 2026-10-12T10:47:59, Yan WANG <[email protected]> wrote: > binman: Add PKCS#11 signing support to pkcs11_tool > > Implement the sign() method in the pkcs11_tool btool to enable > HSM-based cryptographic signing using the PKCS#11 standard. > This provides the core PKCS#11 interface needed for secure signing > with hardware security modules in binman workflows.
Please can you say more about the design? For example, the token is selected by label, serial or slot index, and the mechanism is checked against the token's list before signing. Please also mention the new public method find_slot_by_serial(). > > Signed-off-by: Yan WANG <[email protected]> > > tools/binman/btool/pkcs11_tool.py | 275 ++++++++++++++++++++++++++++++++++++++ > 1 file changed, 275 insertions(+) > diff --git a/tools/binman/btool/pkcs11_tool.py > b/tools/binman/btool/pkcs11_tool.py > @@ -3,7 +3,12 @@ > from binman import bintool > +from u_boot_pylib import tools This import is unused. > diff --git a/tools/binman/btool/pkcs11_tool.py > b/tools/binman/btool/pkcs11_tool.py > @@ -22,6 +27,276 @@ class Bintoolpkcs11_tool(bintool.Bintool): > + args = ['--module', module, '--list-slots'] > + result = self.run_cmd(*args) > + > + # Parse output line by line to find matching serial > + lines = result.stdout.split('\n') run_cmd() returns the stdout string, or None if the tool is missing, not a CommandResult. So this raises AttributeError the first time anyone uses hsm-token-serial. The test in patch 2 misses this because it mocks run_cmd() to return a namedtuple. Please can you use run_cmd_result() here, or treat the return value as a string and mock it as one in the test? > diff --git a/tools/binman/btool/pkcs11_tool.py > b/tools/binman/btool/pkcs11_tool.py > @@ -22,6 +27,276 @@ class Bintoolpkcs11_tool(bintool.Bintool): > + elif algo_family in ('EC', 'ECDSA'): > + return f'ECDSA-{hash_upper}' The pre-load entry only handles RSA, and no test in the series produces an ECDSA signature that U-Boot then verifies. I suggest supporting only RSA until EC has a user and a test. Also, nothing checks hash_algo, so a typo only shows up as a confusing 'not in the allowed mechanisms' error, or as a pkcs11-tool error if the mechanism list can't be read. What do you think? > diff --git a/tools/binman/btool/pkcs11_tool.py > b/tools/binman/btool/pkcs11_tool.py > @@ -22,6 +27,276 @@ class Bintoolpkcs11_tool(bintool.Bintool): > + result = self.run_cmd_result(*args, raise_on_error=False) > + if result is None or result.return_code != 0: > + return # Cannot verify; proceed and let pkcs11-tool report > errors The check is silently skipped in several cases, and pkcs11-tool reports the same error anyway if the mechanism is missing. Do we need _verify_mechanism() at all? It costs an extra tool invocation per signature (two per pre-load entry), and parsing the output is fragile. If you keep it, please at least log something with tout when it is skipped. > diff --git a/tools/binman/btool/pkcs11_tool.py > b/tools/binman/btool/pkcs11_tool.py > @@ -22,6 +27,276 @@ class Bintoolpkcs11_tool(bintool.Bintool): > + # Write data to temporary file (pkcs11-tool requires file input) > + input_fd, input_file = tempfile.mkstemp(suffix='.bin', > + prefix='binman_sign_') Other btools normally take filenames. The etype can write its data into the output directory with tools.get_output_filename() and tools.write_file(). The file then stays there for debugging and is cleaned up with everything else. Please can you change sign() to take an input filename? It could perhaps also take an output filename via --output-file, rather than reading the signature from stdout. That removes the mkstemp()/unlink() handling here. > diff --git a/tools/binman/btool/pkcs11_tool.py > b/tools/binman/btool/pkcs11_tool.py > @@ -22,6 +27,276 @@ class Bintoolpkcs11_tool(bintool.Bintool): > + # Add PIN and login > + if pin: > + args += ['--pin', pin] > + args += ['--login'] Two things here. First, a PIN on the command line is visible to anyone who can read /proc on the build machine. I believe newer OpenSC versions accept '--pin env:VAR', but please check which version added it. If so, it would be better to pass the PIN that way, or at least to mention the risk in the documentation. Second, with no PIN, --login makes pkcs11-tool prompt on the terminal, so a non-interactive build hangs. Should this raise an error instead, unless the token has a protected authentication path? Regards, Simon
