On 22/03/2022 00:43, Ivan Mikhaylov wrote:

Introduce proof of concept for binman's new option which provides sign and replace sections in binary images.

Usage as example:

from: mkimage -G privateky -r -o sha256,rsa4096 -F fit binman replace -i flash.bin -f fit.fit fit

to: binman sign -i flash.bin -k privatekey -a sha256,rsa4096 -f fit.fit fit

This shouldn't need the fit.fit input. It can be extracted from the image as you said in the cover letter. But I think instead of doing "extract -> sign with mkimage -> replace", signing should be implemented in the entry types as a new method. This way we can support other entry-specific ways to sign things (see vblock for an example).

Signed-off-by: Ivan Mikhaylov ivan.mikhaylov@siemens.com

tools/binman/cmdline.py | 13 +++++++++++++ tools/binman/control.py | 26 +++++++++++++++++++++++++- 2 files changed, 38 insertions(+), 1 deletion(-)

diff --git a/tools/binman/cmdline.py b/tools/binman/cmdline.py index 0626b850f4..1a25f95ff1 100644 --- a/tools/binman/cmdline.py +++ b/tools/binman/cmdline.py @@ -160,6 +160,19 @@ controlled by a description in the board device tree.''' replace_parser.add_argument('paths', type=str, nargs='*', help='Paths within file to replace (wildcard)')

• sign_parser = subparsers.add_parser('sign',

•                                       help='Sign entries in image')
  

• sign_parser.add_argument('-a', '--algo', type=str, required=True,

•                            help='Hash algorithm e.g. sha256,rsa4096')
  

• sign_parser.add_argument('-f', '--file', type=str, required=True,

•                            help='Input filename to sign')
  

• sign_parser.add_argument('-i', '--image', type=str, required=True,

•                            help='Image filename to update')
  

• sign_parser.add_argument('-k', '--key', type=str, required=True,

•                            help='Private key file for signing')
  

• sign_parser.add_argument('paths', type=str, nargs='*',

•                            help='Paths within file to sign (wildcard)')
  

If we want to support signing entry types other than FIT, I guess we need to base things on "EntryArg"s to make the arguments more dynamic, like named-by-arg blobs do.

 test_parser = subparsers.add_parser('test', help='Run tests')
 test_parser.add_argument('-P', '--processes', type=int,
     help='set number of processes to use for running tests')

diff --git a/tools/binman/control.py b/tools/binman/control.py index a179f78129..7595ea7776 100644 --- a/tools/binman/control.py +++ b/tools/binman/control.py @@ -19,6 +19,7 @@ from binman import cbfs_util from binman import elf from patman import command from patman import tout +from patman import tools

# List of images we plan to create # Make this global so that it can be referenced from tests @@ -434,6 +435,26 @@ def ReplaceEntries(image_fname, input_fname, indir, entry_paths, AfterReplace(image, allow_resize=allow_resize, write_map=write_map) return image

+def MkimageSign(privatekey_fname, algo, input_fname):

• tools.Run('mkimage', '-G', privatekey_fname, '-r', '-o', algo, '-F', input_fname)

+def SignEntries(image_fname, input_fname, privatekey_fname, algo, entry_paths):

• """Sign and replace the data from one or more entries from input files
• Args:

•    image_fname: Image filename to process
  

•    input_fname: Single input filename to use if replacing one file, None
  

•        otherwise
  

•    algo: Hashing algorithm
  

•    privatekey_fname: Private key filename
  

• Returns:

•    List of EntryInfo records that were signed and replaced
  

• """
• MkimageSign(privatekey_fname, algo, input_fname)
• return ReplaceEntries(image_fname, input_fname, None, entry_paths)

I wrote a bit above, but what I mean is the mkimage call would go into a new method in etype/fit.py, and SignEntries() would be something like ReplaceEntries() but end up calling that method instead of WriteData().

def PrepareImagesAndDtbs(dtb_fname, select_images, update_fdt, use_expanded): """Prepare the images to be processed and select the device tree @@ -627,7 +648,7 @@ def Binman(args): from binman.image import Image from binman import state

• if args.cmd in ['ls', 'extract', 'replace', 'tool']:

• if args.cmd in ['ls', 'extract', 'replace', 'tool', 'sign']: try: tout.init(args.verbosity) tools.prepare_output_dir(None)

@@ -643,6 +664,9 @@ def Binman(args): do_compress=not args.compressed, allow_resize=not args.fix_size, write_map=args.map)

•        if args.cmd == 'sign':
  

•            SignEntries(args.image, args.file, args.key, args.algo, args.paths)
  

•        if args.cmd == 'tool':
             tools.set_tool_paths(args.toolpath)
             if args.list:
  

On 06/04/2022 23:28, Ivan Mikhaylov wrote:

On Tue, 2022-04-05 at 21:54 +0300, Alper Nebi Yasak wrote:

...

On 22/03/2022 00:43, Ivan Mikhaylov wrote:

...

Introduce proof of concept for binman's new option which provides sign and replace sections in binary images.

Usage as example:

from: mkimage -G privateky -r -o sha256,rsa4096 -F fit binman replace -i flash.bin -f fit.fit fit

to: binman sign -i flash.bin -k privatekey -a sha256,rsa4096 -f fit.fit fit

This shouldn't need the fit.fit input. It can be extracted from the image as you said in the cover letter. But I think instead of doing "extract -> sign with mkimage -> replace", signing should be implemented in the entry types as a new method. This way we can support other entry-specific ways to sign things (see vblock for an example).

I have both of these things already: 1.extract->sign->replace binman sign -i flash.bin -k privatekey -a sha256,rsa4096 fit 2.sign->replace binman sign -i flash.bin -k privatekey -a sha256,rsa4096 -f fit.fit fit

Just waited for review to see in which direction should I move.

I've the case when I need only FIT image sign and replace after that. I think they both fit in 'sign' option. Okay about new method, so we talking about just one call like 'SignEntries' without mkimage and other steps with tools?

See below...

...

...

Signed-off-by: Ivan Mikhaylov ivan.mikhaylov@siemens.com

tools/binman/cmdline.py | 13 +++++++++++++  tools/binman/control.py | 26 +++++++++++++++++++++++++-  2 files changed, 38 insertions(+), 1 deletion(-)

diff --git a/tools/binman/cmdline.py b/tools/binman/cmdline.py index 0626b850f4..1a25f95ff1 100644 --- a/tools/binman/cmdline.py +++ b/tools/binman/cmdline.py @@ -160,6 +160,19 @@ controlled by a description in the board device tree.'''      replace_parser.add_argument('paths', type=str, nargs='*',                                  help='Paths within file to replace (wildcard)')   +    sign_parser = subparsers.add_parser('sign', +                                           help='Sign entries in image') +    sign_parser.add_argument('-a', '--algo', type=str, required=True, +                                help='Hash algorithm e.g. sha256,rsa4096') +    sign_parser.add_argument('-f', '--file', type=str, required=True, +                                help='Input filename to sign') +    sign_parser.add_argument('-i', '--image', type=str, required=True, +                                help='Image filename to update') +    sign_parser.add_argument('-k', '--key', type=str, required=True, +                                help='Private key file for signing') +    sign_parser.add_argument('paths', type=str, nargs='*', +                                help='Paths within file to sign (wildcard)')

If we want to support signing entry types other than FIT, I guess we need to base things on "EntryArg"s to make the arguments more dynamic, like named-by-arg blobs do.

Should we care about such possibility in terms of these patches?

There's already vblock and pre-load entry types where 'sign' is a valid thing to do. If we add a 'binman sign', it's reasonable to expect it to work on those as well. But these 'algo' and 'key' arguments don't directly apply to vblock, its signing interface is a bit weird and needs different arguments.

Personally, I'm not sure if I'd use 'binman sign' as I like to clean-build things, so the argument mismatch is not that big of a concern for me.

...

...

test_parser = subparsers.add_parser('test', help='Run tests')      test_parser.add_argument('-P', '--processes', type=int,          help='set number of processes to use for running tests') diff --git a/tools/binman/control.py b/tools/binman/control.py index a179f78129..7595ea7776 100644 --- a/tools/binman/control.py +++ b/tools/binman/control.py @@ -19,6 +19,7 @@ from binman import cbfs_util  from binman import elf  from patman import command  from patman import tout +from patman import tools    # List of images we plan to create  # Make this global so that it can be referenced from tests @@ -434,6 +435,26 @@ def ReplaceEntries(image_fname, input_fname, indir, entry_paths,      AfterReplace(image, allow_resize=allow_resize, write_map=write_map)      return image   +def MkimageSign(privatekey_fname, algo, input_fname): +    tools.Run('mkimage', '-G', privatekey_fname, '-r', '-o', algo, '-F', input_fname)

+def SignEntries(image_fname, input_fname, privatekey_fname, algo, entry_paths): +    """Sign and replace the data from one or more entries from input files

+    Args: +        image_fname: Image filename to process +        input_fname: Single input filename to use if replacing one file, None +            otherwise +        algo: Hashing algorithm +        privatekey_fname: Private key filename

+    Returns: +        List of EntryInfo records that were signed and replaced +    """

+    MkimageSign(privatekey_fname, algo, input_fname)

+    return ReplaceEntries(image_fname, input_fname, None, entry_paths)

I wrote a bit above, but what I mean is the mkimage call would go into a new method in etype/fit.py, and SignEntries() would be something like ReplaceEntries() but end up calling that method instead of WriteData().

Yeap, I agree, as I said above about 'how it should be'. Do you think it should be like additional implementation for mkimage in etype/fit.py which should be used in SignEntries inside? Something like: def SignEntries(params): fit = SignFIT(params) return ReplaceEntries(params with fit)

Don't call ReplaceEntries(). Try to copy what it does, but don't call entry.WriteData() either. Replacing FIT sections is broken on master for now, it tries to rebuild itself to the original specification. I'm planning to fix it, but the way I'm thinking of is demoting the entry to 'blob-ext' when replaced. It wouldn't be nice if that happened also when we're signing an entry.

I think SignEntries would be more like (untested):

def SignEntries(image_fname, entry_paths, ...): image_fname = os.path.abspath(image_fname) image = Image.FromFile(image_fname) state.PrepareFromLoadedData(image) image.LoadData()

for entry_path in entry_paths: entry = image.FindEntryPath(entry_path) entry.UpdateSignatures(...) # TODO

ProcessImage(image, update_fdt=True, write_map=False, get_contents=False, allow_resize=False)

Then you'd implement UpdateSignatures in fit.py to configure the entry to use the given params. I'm not sure how exactly, I thought you'd call mkimage there, but it might be necessary to edit the underlying binman control dtb to change the signature nodes.

Anyways, waiting for Simon's review.

Thanks.

Hi Ivan,

On Tue, 6 Sept 2022 at 07:27, Ivan Mikhaylov fr0st61te@gmail.com wrote:

On Thu, 2022-04-07 at 01:22 +0300, Alper Nebi Yasak wrote:

...

On 06/04/2022 23:28, Ivan Mikhaylov wrote:

...

On Tue, 2022-04-05 at 21:54 +0300, Alper Nebi Yasak wrote:

...

On 22/03/2022 00:43, Ivan Mikhaylov wrote:

...

Introduce proof of concept for binman's new option which provides sign and replace sections in binary images.

Usage as example:

from: mkimage -G privateky -r -o sha256,rsa4096 -F fit binman replace -i flash.bin -f fit.fit fit

to: binman sign -i flash.bin -k privatekey -a sha256,rsa4096 -f fit.fit fit

This shouldn't need the fit.fit input. It can be extracted from the image as you said in the cover letter. But I think instead of doing "extract -> sign with mkimage -> replace", signing should be implemented in the entry types as a new method. This way we can support other entry-specific ways to sign things (see vblock for an example).

I have both of these things already: 1.extract->sign->replace binman sign -i flash.bin -k privatekey -a sha256,rsa4096 fit 2.sign->replace binman sign -i flash.bin -k privatekey -a sha256,rsa4096 -f fit.fit fit

Just waited for review to see in which direction should I move.

I've the case when I need only FIT image sign and replace after that. I think they both fit in 'sign' option. Okay about new method, so we talking about just one call like 'SignEntries' without mkimage and other steps with tools?

See below...

...

...

...

Signed-off-by: Ivan Mikhaylov ivan.mikhaylov@siemens.com

tools/binman/cmdline.py | 13 +++++++++++++ tools/binman/control.py | 26 +++++++++++++++++++++++++- 2 files changed, 38 insertions(+), 1 deletion(-)

diff --git a/tools/binman/cmdline.py b/tools/binman/cmdline.py index 0626b850f4..1a25f95ff1 100644 --- a/tools/binman/cmdline.py +++ b/tools/binman/cmdline.py @@ -160,6 +160,19 @@ controlled by a description in the board device tree.''' replace_parser.add_argument('paths', type=str, nargs='*', help='Paths within file to replace (wildcard)')

• sign_parser = subparsers.add_parser('sign',

•                                       help='Sign entries
  

in image')

• sign_parser.add_argument('-a', '--algo', type=str,

required=True,

•                            help='Hash algorithm e.g.
  

sha256,rsa4096')

• sign_parser.add_argument('-f', '--file', type=str,

required=True,

•                            help='Input filename to sign')
  

• sign_parser.add_argument('-i', '--image', type=str,

required=True,

•                            help='Image filename to
  

update')

• sign_parser.add_argument('-k', '--key', type=str,

required=True,

•                            help='Private key file for
  

signing')

• sign_parser.add_argument('paths', type=str, nargs='*',

•                            help='Paths within file to
  

sign (wildcard)')

If we want to support signing entry types other than FIT, I guess we need to base things on "EntryArg"s to make the arguments more dynamic, like named-by-arg blobs do.

Should we care about such possibility in terms of these patches?

There's already vblock and pre-load entry types where 'sign' is a valid thing to do. If we add a 'binman sign', it's reasonable to expect it to work on those as well. But these 'algo' and 'key' arguments don't directly apply to vblock, its signing interface is a bit weird and needs different arguments.

Personally, I'm not sure if I'd use 'binman sign' as I like to clean-build things, so the argument mismatch is not that big of a concern for me.

...

...

...

 test_parser = subparsers.add_parser('test', help='Run

tests') test_parser.add_argument('-P', '--processes', type=int, help='set number of processes to use for running tests') diff --git a/tools/binman/control.py b/tools/binman/control.py index a179f78129..7595ea7776 100644 --- a/tools/binman/control.py +++ b/tools/binman/control.py @@ -19,6 +19,7 @@ from binman import cbfs_util from binman import elf from patman import command from patman import tout +from patman import tools

# List of images we plan to create # Make this global so that it can be referenced from tests @@ -434,6 +435,26 @@ def ReplaceEntries(image_fname, input_fname, indir, entry_paths, AfterReplace(image, allow_resize=allow_resize, write_map=write_map) return image

+def MkimageSign(privatekey_fname, algo, input_fname):

• tools.Run('mkimage', '-G', privatekey_fname, '-r', '-o',

algo, '-F', input_fname)

+def SignEntries(image_fname, input_fname, privatekey_fname, algo, entry_paths):

• """Sign and replace the data from one or more entries from

input files

• Args:

•    image_fname: Image filename to process
  

•    input_fname: Single input filename to use if replacing
  

one file, None

•        otherwise
  

•    algo: Hashing algorithm
  

•    privatekey_fname: Private key filename
  

• Returns:

•    List of EntryInfo records that were signed and
  

replaced

• """
• MkimageSign(privatekey_fname, algo, input_fname)
• return ReplaceEntries(image_fname, input_fname, None,

entry_paths)

I wrote a bit above, but what I mean is the mkimage call would go into a new method in etype/fit.py, and SignEntries() would be something like ReplaceEntries() but end up calling that method instead of WriteData().

Yeap, I agree, as I said above about 'how it should be'. Do you think it should be like additional implementation for mkimage in etype/fit.py which should be used in SignEntries inside? Something like: def SignEntries(params): fit = SignFIT(params) return ReplaceEntries(params with fit)

Don't call ReplaceEntries(). Try to copy what it does, but don't call entry.WriteData() either. Replacing FIT sections is broken on master for now, it tries to rebuild itself to the original specification. I'm planning to fix it, but the way I'm thinking of is demoting the entry to 'blob-ext' when replaced. It wouldn't be nice if that happened also when we're signing an entry.

I think SignEntries would be more like (untested):

def SignEntries(image_fname, entry_paths, ...): image_fname = os.path.abspath(image_fname) image = Image.FromFile(image_fname) state.PrepareFromLoadedData(image) image.LoadData()

for entry_path in entry_paths:
    entry = image.FindEntryPath(entry_path)
    entry.UpdateSignatures(...)  # TODO

ProcessImage(image, update_fdt=True, write_map=False,
             get_contents=False, allow_resize=False)

Then you'd implement UpdateSignatures in fit.py to configure the entry to use the given params. I'm not sure how exactly, I thought you'd call mkimage there, but it might be necessary to edit the underlying binman control dtb to change the signature nodes.

Alper, this way works but until 74d3b231(binman: Create FIT subentries in the FIT section, not its parent) commit. self.data or self.GetData() is None for entry which need to be signed after this commit. Any ideas how to get the data of fit entry?

Section data comes from the BuildSectionData() method, so you could try calling that.

See also collect_contents_to_file()

Regards, Simon

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.

...

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

On 4/10/22 6:37 PM, Alper Nebi Yasak wrote:

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.

I was primarily confused by the duplicate/extraneous binaries also present. In addition to being tests, these device trees also function as examples, so I think it is desirable for them to be somewhat sane.

--Sean