Hi Yan, On 2026-10-12T10:47:59, Yan WANG <[email protected]> wrote: > binman: Add functional test for pre-load HSM signing > > Add end-to-end functional test for HSM-based signing of pre-load > headers. The test verifies that binman can use PKCS#11 libraries > to sign data via hardware security modules and can generate pre-load > headers with HSM signatures.
This describes patch 4. This patch adds error-path tests and a couple of file-based tests, and none of them signs anything with an HSM. Please can you rewrite the subject and message to say what this patch adds, e.g. 'binman: test: Add error-path tests for pre-load signing'? The missing key-name test is about the file-based path, so it belongs in the refactoring patch suggested on patch 3. > > Signed-off-by: Yan WANG <[email protected]> > > tools/binman/ftest.py | 76 ++++++++++++++++++++++ > .../test/security/pre_load_default_padding.dts | 23 +++++++ > .../test/security/pre_load_hsm_invalid_padding.dts | 27 ++++++++ > .../test/security/pre_load_hsm_missing_library.dts | 27 ++++++++ > .../test/security/pre_load_hsm_missing_tool.dts | 27 ++++++++ > .../test/security/pre_load_missing_key_name.dts | 23 +++++++ > 6 files changed, 203 insertions(+) > diff --git a/tools/binman/ftest.py b/tools/binman/ftest.py > @@ -5994,6 +5994,82 @@ fdt fdtmap Extract the > devicetree blob from the fdtmap > + def testPreLoadHsmMissingLibrary(self): > + """Test HSM signing with missing hsm-library property""" > + with self.assertRaises(ValueError) as exc: > + self._DoReadFile('security/pre_load_hsm_missing_library.dts') > + self.assertIn('hsm-library is required for HSM signing', > + str(exc.exception)) > + > + def testPreLoadHsmInvalidPadding(self): > + """Test HSM signing with invalid padding""" > + with self.assertRaises(ValueError) as exc: > + self._DoReadFile('security/pre_load_hsm_invalid_padding.dts') > + self.assertIn('Unsupported padding scheme', str(exc.exception)) In _SignData(), the pkcs11_tool.is_present() check runs before the hsm-library and padding checks. So on a machine without pkcs11-tool, both tests get 'pkcs11-tool btool not available' and fail, but the binman tests have to pass on a plain build host. I suggest moving the config checks in patch 3 ahead of the tool check. Better still, do them in ReadNode() so a bad DT fails early. Otherwise these tests need to mock is_present(), as the later ones do. > diff --git a/tools/binman/ftest.py b/tools/binman/ftest.py > @@ -5994,6 +5994,82 @@ fdt fdtmap Extract the > devicetree blob from the fdtmap > + def testPreLoadHsmMissingTool(self): > + """Test HSM signing when pkcs11_tool is not available""" > + try: > + with self.assertRaises(ValueError) as exc: > + self._DoTestFile('security/pre_load_hsm_missing_tool.dts', > + force_missing_bintools='pkcs11-tool') > + self.assertIn('pkcs11-tool btool not available', > str(exc.exception)) > + finally: > + bintool.Bintool.set_missing_list(None) This bakes in patch 3's behaviour of raising an error when the tool is missing. Other etypes (fit, mkimage, gbb, efi_capsule) produce fake data instead, so -M lets the build complete and binman reports the missing tool at the end. Once pre_load does the same, please change this test to match the existing missing-tool tests, e.g. the futility one in testGbbMissing. The try/finally isn't needed, since control.Binman() calls set_missing_list() on every run and the other force_missing_bintools tests don't reset it. Also, the continuation line isn't aligned with the open parenthesis. > diff --git a/tools/binman/ftest.py b/tools/binman/ftest.py > @@ -5994,6 +5994,82 @@ fdt fdtmap Extract the > devicetree blob from the fdtmap > + def testPreLoadHsmSignError(self): > + """Test HSM signing with pkcs11-tool ValueError""" > + from binman.btool import pkcs11_tool > + > + def mock_sign_value_error(*args, **kwargs): > + raise ValueError("Mock HSM error") > + > + with unittest.mock.patch.object(pkcs11_tool.Bintoolpkcs11_tool, > 'is_present', > + return_value=True): > + with unittest.mock.patch.object(pkcs11_tool.Bintoolpkcs11_tool, > 'sign', > + > side_effect=mock_sign_value_error): > + with self.assertRaises(ValueError) as exc: > + > self._DoReadFile('security/pre_load_hsm_missing_tool.dts') > + self.assertIn('HSM signing failed', str(exc.exception)) This test and testPreLoadHsmSignRuntimeError differ only in the exception type. Please can you merge them into one test that loops over (ValueError, RuntimeError)? You can pass side_effect=ValueError('Mock HSM error') directly, without the nested function, and a single 'with' statement can apply both patches. Please move the import to the top of the file with the others. Please also rename pre_load_hsm_missing_tool.dts to something generic like pre_load_hsm.dts, since several tests use it and only one is about a missing tool. The patch.object() lines also go over 80 columns. > diff --git a/tools/binman/ftest.py b/tools/binman/ftest.py > @@ -5994,6 +5994,82 @@ fdt fdtmap Extract the > devicetree blob from the fdtmap > + def testPreLoadDefaultPadding(self): > + """Test pre-load with no padding-name (should default to pkcs-1.5)""" > + entry_args = { > + 'pre-load-key-path': os.path.join(self._binman_dir, 'test'), > + } > + data = self._DoReadFileDtb('security/pre_load_default_padding.dts', > + entry_args=entry_args, > + > extra_indirs=[os.path.join(self._binman_dir, 'test')])[0] This duplicates testPreLoad. pre_load.dts already omits padding-name, and testPreLoad checks the result with _CheckPreload(), whose default padding is pkcs-1.5. This version only checks the header fields, so it wouldn't notice the wrong padding. Please can you drop this test and pre_load_default_padding.dts? Regards, Simon
