[PATCH 3/3] binman: add test for sign option
Alper Nebi Yasak
alpernebiyasak at gmail.com
Mon Apr 11 00:37:56 CEST 2022
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 at 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.
More information about the U-Boot
mailing list