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

[Ivan Mikhaylov]

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?

> 
> > Signed-off-by: Ivan Mikhaylov <ivan.mikhaylov at 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?

> 
> >      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)

Anyways, waiting for Simon's review.

Thanks.