[PATCH v2 5/5] test_vboot.py: include test of fdt_add_pubkey tool

[Ivan Mikhaylov]

On Thu, 2023-03-16 at 07:59 -0600, Simon Glass wrote:
> Hi Ivan,
> 
> On Wed, 15 Mar 2023 at 19:17, Ivan Mikhaylov <fr0st61te at gmail.com>
> wrote:
> > 
> > On Fri, 2023-03-10 at 17:46 -0800, Simon Glass wrote:
> > > Hi Ivan,
> > > 
> > > On Tue, 7 Mar 2023 at 14:13, Ivan Mikhaylov <fr0st61te at gmail.com>
> > > wrote:
> > > > 
> > > > From: Roman Kopytin <Roman.Kopytin at kaspersky.com>
> > > > 
> > > > Signed-off-by: Roman Kopytin <Roman.Kopytin at kaspersky.com>
> > > > Cc: Rasmus Villemoes <rasmus.villemoes at prevas.dk>
> > > > ---
> > > >  test/py/tests/test_vboot.py | 8 ++++++++
> > > >  1 file changed, 8 insertions(+)
> > > > 
> > > > diff --git a/test/py/tests/test_vboot.py
> > > > b/test/py/tests/test_vboot.py
> > > > index e3e7ca4b21..956b8fcd43 100644
> > > > --- a/test/py/tests/test_vboot.py
> > > > +++ b/test/py/tests/test_vboot.py
> > > > @@ -313,6 +313,13 @@ def test_vboot(u_boot_console, name,
> > > > sha_algo,
> > > > padding, sign_options, required,
> > > > 
> > > >          util.run_and_log(cons, [fit_check_sign, '-f', fit, '-
> > > > k',
> > > > dtb])
> > > > 
> > > > +        # Create a fresh .dtb without the public keys
> > > > +        dtc('sandbox-u-boot.dts')
> > > > +        # Then add the dev key via the fdt_add_pubkey tool
> > > > +        util.run_and_log(cons, [fdt_add_pubkey, '-a',
> > > > '%s,rsa2048'
> > > > % sha_algo,
> > > > +                                '-k', tmpdir, '-n', 'dev', '-
> > > > r',
> > > > 'conf', dtb])
> > > > +        util.run_and_log(cons, [fit_check_sign, '-f', fit, '-
> > > > k',
> > > > dtb])
> > > > +
> > > >          if full_test:
> > > >              # Make sure that U-Boot checks that the config is
> > > > in
> > > > the list of
> > > >              # hashed nodes. If it isn't, a security bypass is
> > > > possible.
> > > > @@ -500,6 +507,7 @@ def test_vboot(u_boot_console, name,
> > > > sha_algo,
> > > > padding, sign_options, required,
> > > >      mkimage = cons.config.build_dir + '/tools/mkimage'
> > > >      binman = cons.config.source_dir + '/tools/binman/binman'
> > > >      fit_check_sign = cons.config.build_dir +
> > > > '/tools/fit_check_sign'
> > > > +    fdt_add_pubkey = cons.config.build_dir +
> > > > '/tools/fdt_add_pubkey'
> > > >      dtc_args = '-I dts -O dtb -i %s' % tmpdir
> > > >      dtb = '%ssandbox-u-boot.dtb' % tmpdir
> > > >      sig_node = '/configurations/conf-1/signature'
> > > > --
> > > > 2.39.1
> > > > 
> > > 
> > > Unfortunately this test fails on sandbox:
> > > 
> > > https://source.denx.de/u-boot/custodians/u-boot-dm/-/jobs/591975
> > > 
> > > I think it would be better to put it in its own test (perhaps in
> > > the
> > > same file) so we are not doing it on every test run. Also you
> > > could
> > > check (in a very basic way) that it adds the key correctly since
> > > we
> > > don't really need another test of the logic of doing that. We are
> > > just
> > > checking that your tool calls that logic correctly.
> > > 
> > > I'll drop this one when applying, for now. Please take a look.
> > > 
> > > Regards,
> > > Simon
> > 
> > Simon, does it look ok? Test for test_vboot is passed fine.
> 
> Please see my message before:
> 
> > Unfortunately this test fails on sandbox:
> > 
> > https://source.denx.de/u-boot/custodians/u-boot-dm/-/jobs/591975
> > 
> > I think it would be better to put it in its own test (perhaps in
> > the
> > same file) so we are not doing it on every test run. Also you could
> > check (in a very basic way) that it adds the key correctly since we
> > don't really need another test of the logic of doing that. We are
> > just
> > checking that your tool calls that logic correctly.
> > 
> > I'll drop this one when applying, for now. Please take a look.
> 
> I have not applied this patch due to the problem.
> 
> Regards,
> Simon
> 

Simon, but I've changed the test and put it in previous note, maybe you
didn't notice, I did what you asked:
- made own test "test_fdt_add_pubkey"
- simple check that with clear dtb you can add keys with fdt_add_pubkey
  and check with fit_check_sign with signed fit.

I've checked that change with sandbox and it passes test_vboot well.

Need I re-submit that patch or whole series?

Thanks.

> 
> 
> > 
> > Thanks.
> > 
> > From 5484d525d4950b064adf1204f5bf055229c942ac Mon Sep 17 00:00:00
> > 2001
> > From: Roman Kopytin <Roman.Kopytin at kaspersky.com>
> > Date: Thu, 11 Nov 2021 11:15:12 +0300
> > Subject: [PATCH v3] test_vboot.py: include test of fdt_add_pubkey
> > tool
> > 
> > Signed-off-by: Roman Kopytin <Roman.Kopytin at kaspersky.com>
> > Signed-off-by: Ivan Mikhaylov <fr0st61te at gmail.com>
> > Cc: Rasmus Villemoes <rasmus.villemoes at prevas.dk>
> > ---
> >  test/py/tests/test_vboot.py | 33 +++++++++++++++++++++++++++++++++
> >  1 file changed, 33 insertions(+)
> > 
> > diff --git a/test/py/tests/test_vboot.py
> > b/test/py/tests/test_vboot.py
> > index e3e7ca4b21..5ae622fe21 100644
> > --- a/test/py/tests/test_vboot.py
> > +++ b/test/py/tests/test_vboot.py
> > @@ -491,6 +491,37 @@ def test_vboot(u_boot_console, name, sha_algo,
> > padding, sign_options, required,
> >          # Check that the boot fails if the global signature is not
> > provided
> >          run_bootm(sha_algo, 'global image signature', 'signature
> > is
> > mandatory', False)
> > 
> > +    def test_fdt_add_pubkey(sha_algo, padding, sign_options):
> > +        """Test fdt_add_pubkey utility with given hash algorithm
> > and
> > padding.
> > +
> > +        This function tests if fdt_add_pubkey utility may add
> > public
> > keys into dtb.
> > +
> > +        Args:
> > +            sha_algo: Either 'sha1' or 'sha256', to select the
> > algorithm to use
> > +            padding: Either '' or '-pss', to select the padding to
> > use
> > for the
> > +                    rsa signature algorithm.
> > +            sign_options: Options to mkimage when signing a fit
> > image.
> > +        """
> > +
> > +        # Create a fresh .dtb without the public keys
> > +        dtc('sandbox-u-boot.dts')
> > +        make_fit('sign-configs-%s%s.its' % (sha_algo, padding))
> > +
> > +        # Sign images with our dev keys
> > +        sign_fit(sha_algo, sign_options)
> > +
> > +        # Create a fresh .dtb without the public keys
> > +        dtc('sandbox-u-boot.dts')
> > +
> > +        cons.log.action('%s: Test fdt_add_pubkey with signed
> > configuration' % sha_algo)
> > +        # Then add the dev key via the fdt_add_pubkey tool
> > +        util.run_and_log(cons, [fdt_add_pubkey, '-a', '%s,%s' %
> > ('sha256' if algo_arg else sha_algo, \
> > +                                'rsa3072' if sha_algo == 'sha384'
> > else
> > 'rsa2048'),
> > +                                '-k', tmpdir, '-n', 'dev', '-r',
> > 'conf', dtb])
> > +
> > +        # Check with fit_check_sign that FIT is signed with key
> > +        util.run_and_log(cons, [fit_check_sign, '-f', fit, '-k',
> > dtb])
> > +
> >      cons = u_boot_console
> >      tmpdir = os.path.join(cons.config.result_dir, name) + '/'
> >      if not os.path.exists(tmpdir):
> > @@ -500,6 +531,7 @@ def test_vboot(u_boot_console, name, sha_algo,
> > padding, sign_options, required,
> >      mkimage = cons.config.build_dir + '/tools/mkimage'
> >      binman = cons.config.source_dir + '/tools/binman/binman'
> >      fit_check_sign = cons.config.build_dir +
> > '/tools/fit_check_sign'
> > +    fdt_add_pubkey = cons.config.build_dir +
> > '/tools/fdt_add_pubkey'
> >      dtc_args = '-I dts -O dtb -i %s' % tmpdir
> >      dtb = '%ssandbox-u-boot.dtb' % tmpdir
> >      sig_node = '/configurations/conf-1/signature'
> > @@ -516,6 +548,7 @@ def test_vboot(u_boot_console, name, sha_algo,
> > padding, sign_options, required,
> >      with open(evil_kernel, 'wb') as fd:
> >          fd.write(500 * b'\x01')
> > 
> > +    test_fdt_add_pubkey(sha_algo, padding, sign_options)
> >      try:
> >          # We need to use our own device tree file. Remember to
> > restore
> > it
> >          # afterwards.
> > --
> > 2.39.1
> > 
> >