Hi Quentin, > From: Quentin Schulz <[email protected]> > > This adds support for using an OpenSSL engine for signing a FIT image. > To use it, one should set the fit,engine property at the FIT node level > with the engine to use. This will in turn call mkimage with the -N > option. > > The -k argument to mkimage can be specified via fit,engine-keydir. This > is especially useful for pkcs11 engine to specify slots, token label, > etc...
Thanks a lot, v2 of the patch series is really an improvement for the PKCS#11 use case. I was not aware that there was already code that could automatically prefix 'pkcs11:object=' to the key-name-hint. (Yes, I know, it is part of the existing documentation, and I have read that, but that just shows how much I miss even when the reading docs ...) > The -k argument to mkimage can be specified by setting fit,engine-keydir > property, if missing no -k argument will be passed Small nitpick: the above two paragraphs seem repetitive, how about the following? The -k argument to mkimage can be specified via fit,engine-keydir. If fit,engine-keydir is not specified, no -k argument will be passed to mkimage. The attribute fit,engine-keydir is especially useful for a pkcs11 engine to specify slots, token label, etc... > As far as I could tell, mkimage encrypts and signs a FIT in one go, thus > the -k argument applies to both signing and encrypting. Considering we > reuse the -k argument for two different meanings (info to pass to the > engine when using an engine otherwise the directory where keys are > stored), we cannot reasonably encrypt using local keys and signing with > an engine, hence the enforced check. I believe it should be possible to > support encrypting and signing with the same engine (using different > key pairs of course, via different key-name-hint likely), but this is > left for the next person to implement. > This is why the property is named fit,engine and not fit,sign-engine. > Ditto for fit,engine-keydir. > > The public key (with .crt extension) is still required if it needs to be > embedded in the SPL DTB for example. We could probably support > retrieving the public key from an engine, but this is a change to make > to fdt_add_pubkey.c. > > Signed-off-by: Quentin Schulz <[email protected]> > --- > tools/binman/entries.rst | 45 +++++++++++++++++++++++++++++++--- > tools/binman/etype/fit.py | 61 > +++++++++++++++++++++++++++++++++++++++++++---- > 2 files changed, 99 insertions(+), 7 deletions(-) > > diff --git a/tools/binman/entries.rst b/tools/binman/entries.rst > index 8922d6cd070..fb17157cd79 100644 > --- a/tools/binman/entries.rst > +++ b/tools/binman/entries.rst > @@ -885,9 +885,10 @@ The top-level 'fit' node supports the following special > properties: > > fit,sign > Enable signing FIT images via mkimage as described in > - verified-boot.rst. If the property is found, the private keys path > - is detected among binman include directories and passed to mkimage > - via -k flag. All the keys required for signing FIT must be > + verified-boot.rst. > + If the property is found and fit,engine is not set, the private > + keys path is detected among binman include directories and passed to > + mkimage via -k flag. All the keys required for signing FIT must be > available at time of signing and must be located in single include > directory. > > @@ -898,6 +899,44 @@ The top-level 'fit' node supports the following special > properties: > required for encrypting the FIT must be available at the time of > encrypting and must be located in a single include directory. > > + Incompatible with fit,engine. > + > + fit,engine > + Indicates the OpenSSL engine to use for signing the FIT image. This > + is passed to mkimage via the `-N` flag. Example:: > + > + fit,engine = "my-engine"; > + > + A `-k` argument for mkimage may be passed via `fit,engine-keydir`. > + > + When `fit,engine` is set to `pkcs11`, the value of > + `fit,engine-keydir` (if present) is concatenated with `;object=` and > + the value of `key-name-hint`, and passed as-is to the OpenSSL engine > + API. If the value of `fit,engine-keydir` contains `object=` or > + `id=`, only the value of `fit,engine-keydir` is passed as-is to the > + OpenSSL engine API. If no `fit,engine-keydir` is present, > + `pkcs11:object=` is concatenated with the value of `key-name-hint`, > + and passed as-is to the OpenSSL engine API. This description is quite convoluted. I would propose to describe it as a list, something like the following: When `fit,engine` is set to `pkcs11`, the following cases are distinguished regarding the value of `fit,engine-keydir`: - If `fit,engine-keydir` is not present, value of `key-name-hint` is prefixed with `pkcs11:object=`, and then passed as-is to the OpenSSL engine API. PKCS#11 id: `pkcs11:object=<key-name-hint>` - If `fit,engine-keydir` is present, and its value contains either `object=` or `id=`, then the value of `fit,engine-keydir` is passed as-is to theOpenSSL engine API. The value of `fit,engine-keydir` has to start with `pkcs11:`. PKCS#11 id: `<fit,engine-keydir>` - If `fit,engine-keydir` is present, but its value does not contain either `object=` or `id=`, then the value of `fit,engine-keydir` is concatenated with `;object=` and the value of `key-name-hint`, and passed as-is to the OpenSSL engine API. The value of `fit,engine-keydir` has to start with `pkcs11:`. PKCS#11 id: `<fit,engine-keydir>; object=<key-name-hint>` In all of the above cases, the value of `<key-name-hint>` is used for verification to find the matching public key. > + For all other values of `fit,engine`, the value of > + `fit,engine-keydir` (if present) is concatenated with the value of > + `key-name-hint` and passed as-is to the OpenSSL engine API. > + > + Depends on fit,sign. > + > + Incompatible with fit,encrypt. > + > + fit,engine-keydir > + Indicates the `-k` argument to pass to mkimage if an OpenSSL engine > + is to be used for signing the FIT image. Example:: > + > + fit,engine-keydir = "pkcs11:model=xxx;manufacturer=xxx"; > + > + Read `fit,engine` documentation for more info on special cases when > + using `pkcs11` as engine. > + > + Depends on fit,engine. > + > Substitutions > ~~~~~~~~~~~~~ > > diff --git a/tools/binman/etype/fit.py b/tools/binman/etype/fit.py > index db40479d30e..6547d0314b9 100644 > --- a/tools/binman/etype/fit.py > +++ b/tools/binman/etype/fit.py > @@ -104,9 +104,10 @@ class Entry_fit(Entry_section): > > fit,sign > Enable signing FIT images via mkimage as described in > - verified-boot.rst. If the property is found, the private keys > path > - is detected among binman include directories and passed to > mkimage > - via -k flag. All the keys required for signing FIT must be > + verified-boot.rst. > + If the property is found and fit,engine is not set, the private > + keys path is detected among binman include directories and > passed to > + mkimage via -k flag. All the keys required for signing FIT must > be > available at time of signing and must be located in single > include > directory. > > @@ -117,6 +118,44 @@ class Entry_fit(Entry_section): > required for encrypting the FIT must be available at the time of > encrypting and must be located in a single include directory. > > + Incompatible with fit,engine. > + > + fit,engine > + Indicates the OpenSSL engine to use for signing the FIT image. > This > + is passed to mkimage via the `-N` flag. Example:: > + > + fit,engine = "my-engine"; > + > + A `-k` argument for mkimage may be passed via > `fit,engine-keydir`. > + > + When `fit,engine` is set to `pkcs11`, the value of > + `fit,engine-keydir` (if present) is concatenated with `;object=` > and > + the value of `key-name-hint`, and passed as-is to the OpenSSL > engine > + API. If the value of `fit,engine-keydir` contains `object=` or > + `id=`, only the value of `fit,engine-keydir` is passed as-is to > the > + OpenSSL engine API. If no `fit,engine-keydir` is present, > + `pkcs11:object=` is concatenated with the value of > `key-name-hint`, > + and passed as-is to the OpenSSL engine API. Same as above. > + > + For all other values of `fit,engine`, the value of > + `fit,engine-keydir` (if present) is concatenated with the value > of > + `key-name-hint` and passed as-is to the OpenSSL engine API. > + > + Depends on fit,sign. > + > + Incompatible with fit,encrypt. > + > + fit,engine-keydir > + Indicates the `-k` argument to pass to mkimage if an OpenSSL > engine > + is to be used for signing the FIT image. Example:: > + > + fit,engine-keydir = "pkcs11:model=xxx;manufacturer=xxx"; > + > + Read `fit,engine` documentation for more info on special cases > when > + using `pkcs11` as engine. > + > + Depends on fit,engine. > + > Substitutions > ~~~~~~~~~~~~~ > > @@ -620,7 +659,21 @@ class Entry_fit(Entry_section): > args.update({'align': fdt_util.fdt32_to_cpu(align.value)}) > if (self._fit_props.get('fit,sign') is not None or > self._fit_props.get('fit,encrypt') is not None): > - args.update({'keys_dir': self._get_keys_dir(data)}) > + engine = None > + if self._fit_props.get('fit,sign') is not None: > + engine_prop = self._fit_props.get('fit,engine') > + if engine_prop is not None: > + engine = engine_prop.value > + args.update({'engine': engine}) > + engine_keydir_prop = self._fit_props.get('fit,engine-keydir') > + if engine_keydir_prop is not None: > + engine_keydir = engine_keydir_prop.value > + if engine_keydir: > + args.update({'keys_dir': engine_keydir}) > + if engine is None: > + args.update({'keys_dir': self._get_keys_dir(data)}) > + elif self._fit_props.get('fit,encrypt') is not None: > + self.Raise('fit,engine currently does not support > encryption') > if self.mkimage.run(reset_timestamp=True, output_fname=output_fname, > **args) is None: > if not self.GetAllowMissing(): Tested-by: Wolfgang Wallner <[email protected]> I did test the 3 cases listed in the description of fit,engine: Test 1: No fit,engine-keydir: fit,sign; fit,engine = "pkcs11"; key-name-hint = "<object-id-of-my-key>"; Test 2: fit,engine-keydir present, and contains the string 'object=': fit,sign; fit,engine = "pkcs11"; fit,engine-keydir = "pkcs11:object=<object-id-of-my-key>"; key-name-hint = "MyTestKey"; Test 3: fit,engine-keydir present, but does not contain'object=' of 'id=': fit,sign; fit,engine = "pkcs11"; fit,engine-keydir = "pkcs11:serial=<serial-of-my-key>"; key-name-hint = "<object-id-of-my-key>"; I think the suggestions above would improve the clarity, but even if you choose to keep the text as is would be fine for me. Both code + documentation: Reviewed-by: Wolfgang Wallner <[email protected]> regards, Wolfgang
