Hi Yan, On 2026-10-12T10:47:59, Yan WANG <[email protected]> wrote: > binman: Add test for pkcs11_tool sign method > > Exercise the three new methods added to the pkcs11_tool btool: > sign(), _build_mechanism() and _verify_mechanism().
Patch 1 adds four methods, and this patch also tests find_slot_by_serial(). Please can you squash this into patch 1 so the code and its tests land together? Binman expects full test coverage, and patch 1 on its own leaves the new btool code untested. > > Signed-off-by: Yan WANG <[email protected]> > > tools/binman/bintool_test.py | 374 +++++++++++++++++++++++++++++++++++++++++++ > 1 file changed, 374 insertions(+) > diff --git a/tools/binman/bintool_test.py b/tools/binman/bintool_test.py > @@ -354,6 +364,370 @@ class TestBintool(unittest.TestCase): > + # Test finding a slot with matching serial > + slot_output = """Available slots: > +Slot 0 (0x123): SoftHSM slot ID 0x123 > + token label : test-token > + token manufacturer : SoftHSM project > + token model : SoftHSM v2 > + token flags : login required, rng, token initialized, PIN > initialized > + hardware version : 2.6 > + firmware version : 2.6 > + Token serial num : abc123xyz This doesn't match what pkcs11-tool prints. I ran 'pkcs11-tool --module /usr/lib/softhsm/libsofthsm2.so --list-slots' against a SoftHSM v2.6 install with one initialised token 't1' and one empty slot, and got: Slot 0 (0x23aea6bb): SoftHSM slot ID 0x23aea6bb token label : t1 ... firmware version : 2.6 serial num : 4b16583ea3aea6bb pin min/max : 4/255 The real tool prints 'serial num', not 'token serial num'. So find_slot_by_serial() in patch 1 never matches, and hsm-token-serial cannot work. The test only passes because the sample output was written to fit the parser. Please can you fix the parser and take the test data from real pkcs11-tool output, including the 'pin min/max' line and an uninitialised slot? A token-serial case in the SoftHSM functional test would also catch this. Also please use ' for strings, rather than " (fix this globally) > diff --git a/tools/binman/bintool_test.py b/tools/binman/bintool_test.py > @@ -354,6 +364,370 @@ class TestBintool(unittest.TestCase): > + def fake_run_result(*args, **kwargs): > + if '--sign' in args: > + self.assertIn('--slot-index', args) > + self.assertIn('0', args) > + return _CmdResult(0, fake_sig, b'') These checks are weak. assertIn('0', args) passes if '0' appears anywhere in the arguments, and if the --sign call never happens, nothing is checked at all. The same fake result is also returned for --list-mechanisms. So _verify_mechanism() parses the signature bytes as a mechanism list and quietly skips the check. Please can you record the arguments of the sign call, check that it happened, and check pairs directly, e.g. args[args.index('--slot-index') + 1] == '0'? The same applies to the key_id, PIN and token_serial tests. BTW the test should also check that --login is passed. > diff --git a/tools/binman/bintool_test.py b/tools/binman/bintool_test.py > @@ -354,6 +364,370 @@ class TestBintool(unittest.TestCase): > + def fake_unlink(path): > + if 'binman_sign_' in path: > + raise OSError("Permission denied") > + return original_unlink(path) This leaves a binman_sign_*.bin file in /tmp on every run. Please can you record the path in fake_unlink() and remove it with original_unlink() at the end of the test? Also strings should use ' not " (please fix globally) > diff --git a/tools/binman/bintool_test.py b/tools/binman/bintool_test.py > @@ -354,6 +364,370 @@ class TestBintool(unittest.TestCase): > + # Previously hard-coded combinations still resolve correctly > + self.assertEqual('SHA384-RSA-PKCS', > + btool._build_mechanism('sha384', 'rsa4096', > 'pkcs1v15')) > + > + # Novel (non-table) combination: any hash works with RSA These comments refer to an earlier lookup table that isn't in the tree, so they will confuse readers. Please can you drop the 'previously' and 'non-table' wording? The first case also duplicates the SHA384 and RSA4096 checks above. Regards, Simon
