[PATCH 3/3] binman: add test for sign option

[Ivan Mikhaylov]

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.

> 
> > +            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
> > 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.
> 
> > +               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.

> 
> > +                               };
> > +
> > +                               fdt-1 {
> 
> Maybe use things like @fdt-SEQ, as seen in
> tools/binman/test/170_fit_fdt.dts?

I'll check, thanks.

> 
> > +                                       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.

> 
> > +               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.