I initially ignored this test because I didn't like the implementation anyway, but I disagree with some of Sean's comments here so I wanted to add on what I think.
On 08/04/2022 22:26, Ivan Mikhaylov wrote:
On Fri, 2022-04-08 at 11:39 -0400, Sean Anderson wrote:
On 3/21/22 5:43 PM, Ivan Mikhaylov wrote:
Add the test which provides sequence of actions: 1. create the image from binman dts 2. create public and private keys 3. add public key into dtb with fdt_add_pubkey 4. sign image with new sign option 5. check with fit_check_sign
Signed-off-by: Ivan Mikhaylov ivan.mikhaylov@siemens.com
tools/binman/ftest.py | 42 +++++++++++++++++++ tools/binman/test/225_fit_sign.dts | 67 ++++++++++++++++++++++++++++++ 2 files changed, 109 insertions(+) create mode 100644 tools/binman/test/225_fit_sign.dts
diff --git a/tools/binman/ftest.py b/tools/binman/ftest.py index 8f00db6945..8a3d3720c4 100644 --- a/tools/binman/ftest.py +++ b/tools/binman/ftest.py @@ -3088,6 +3088,48 @@ class TestFunctional(unittest.TestCase): self.assertEqual(len(U_BOOT_DATA), entry.contents_size) self.assertEqual(len(U_BOOT_DATA), entry.size) + def testSignSimple(self): + """Test signing a single file"""
+ data = self._DoReadFileRealDtb('225_fit_sign.dts')
+ updated_fname = tools.GetOutputFilename('image- updated.bin') + tools.WriteFile(updated_fname, data)
+ outdir = os.path.join(self._indir, 'extract') + einfos = control.ExtractEntries(updated_fname, None, outdir, [])
+ dtb = tools.GetOutputFilename('source.dtb') + private_key = tools.GetOutputFilename('test_key.key') + public_key = tools.GetOutputFilename('test_key.crt') + fit = tools.GetOutputFilename('fit.fit') + key_dir = tools.GetOutputDir()
+ def check_sign(fit, key):
please inline this, since it is only called once
+ try: + tools.Run('fit_check_sign', '-k', key, '-f', fit) + except: + return False
you can just do a bare tools.Run() and if an exception is raised it will cause the test to fail.
Ok, good to know.
It will cause a test 'error' instead of a 'failure'. You should keep the try-except, but call self.fail(...) in the except case with a reasonable error message. Nesting multiple try blocks is a bit ugly, so you could keep the single-use function.
It would be even better if you added more cases testing various signing conditions, with this as a helper function in the outer scope that all of them can use.
You could extend it to check if the signature nodes in the updated fdtmap match the arguments passed to 'binman sign' (but your current implementation doesn't update those nodes).
+ return True
+ is_signed = False + try: + tools.Run('openssl', 'req', '-batch' , '-newkey', 'rsa:4096', + '-sha256', '-new', '-nodes', '-x509', '- keyout', + private_key, '-out', public_key) + tools.Run('fdt_add_pubkey', '-a', 'sha256,rsa4096', '- k', key_dir, + '-n', 'test_key', dtb) + with test_util.capture_sys_output() as (stdout, stderr): + # do sign with private key + self._DoBinman('sign', '-i', updated_fname, '-k', private_key, + '-a', 'sha256,rsa4096', '-f', fit, 'fit') + is_signed = check_sign(fit, dtb) + finally: + shutil.rmtree(key_dir)
+ self.assertEqual(is_signed, True)
so no need for this assert here
def _RunReplaceCmd(self, entry_name, data, decomp=True, allow_resize=True, dts='132_replace.dts'): """Replace an entry in an image
New tests usually are added at the end of this file. IIRC they should be ordered by their test dts' number.
diff --git a/tools/binman/test/225_fit_sign.dts b/tools/binman/test/225_fit_sign.dts new file mode 100644 index 0000000000..2bfa826106 --- /dev/null +++ b/tools/binman/test/225_fit_sign.dts @@ -0,0 +1,67 @@ +// SPDX-License-Identifier: GPL-2.0+
+/dts-v1/;
+/ { + #address-cells = <1>; + #size-cells = <1>;
+ binman {
I don't really understand what's going on in this test case.
Binman mocks most entries' data during tests, the test images aren't meaningful or usable beyond what's in the python test case code. In this case, we only need a 'fit' entry with a signature we can run 'binman sign' on, and a 'fdtmap' so we can recognize that the built image has that 'fit' entry in it. The rest is just fluff.
+ size = <0x100000>; + allow-repack;
+ u-boot { + };
+ fit { + description = "U-Boot"; + offset = <0x10000>; + images { + u-boot-1 { + description = "U-Boot"; + type = "standalone"; + arch = "arm64"; + os = "u-boot"; + compression = "none"; + hash-1 { + algo = "sha256"; + };
Shouldn't there be some kind of data here?
Nope.
You could add a 'u-boot' entry here. It's weird seeing a hash/signature for empty data.
+ };
+ fdt-1 {
Maybe use things like @fdt-SEQ, as seen in tools/binman/test/170_fit_fdt.dts?
I'll check, thanks.
If you do so, I would prefer that as an additional test case. No need to replace this one.
+ description = "test.dtb"; + type = "flat_dt"; + arch = "arm64"; + compression = "none"; + hash-1 { + algo = "sha256"; + }; + u-boot-spl-dtb { + }; + };
+ };
+ configurations { + default = "conf-1"; + conf-1 { + description = "u-boot with fdt"; + firmware = "u-boot-1"; + fdt = "fdt-1"; + signature-1 { + algo = "sha256,rsa4096"; + key-name-hint = "test_key"; + sign-images = "firmware", "fdt"; + };
+ }; + }; + };
+ u-boot-nodtb { + };
Why do we have U-Boot again here without a device tree?
It is done by mistake, same as for 'u-boot' on the top, I'll get rid off both of them.
Doesn't really matter IMO. A lot of test images have the entry-under-test sandwiched between other entries like this anyway.
+ fdtmap { + }; + }; +};
What is U-Boot supposed to be verified by? Shouldn't this package SPL? I guess that is out of scope?
Only thing which should be verified here, that is FIT container is signed with fit_check_sign utility.
Thanks.