Re: [PATCH v2 2/2] keytoc: fix env provided keyspec handling

[Marco Felsch <m.felsch@pengutronix.de>]

On 25-08-06, Sascha Hauer wrote:
> On Tue, Aug 05, 2025 at 10:07:54PM +0200, Marco Felsch wrote:
> > On 25-08-05, Sascha Hauer wrote:
> > > +Cc Basti
> > > 
> > > On Thu, Jul 24, 2025 at 03:50:56PM +0200, Marco Felsch wrote:
> > > > Currently the env provided keyspec is resolved during the gen_key() step
> > > > by making use of the try_resolve_env(). This is wrong because it will
> > > > set the complete <hint>:<key> keyspec string for the 'keyname' and 'path'.
> > > > 
> > > > To fix this the resolve step must happen during the main-loop as first step
> > > > because the main-loop is doing the 'keyname' and 'path' split already.
> > > > 
> > > > Signed-off-by: Marco Felsch <m.felsch@pengutronix.de>
> > > > ---
> > > >  scripts/keytoc.c | 13 +++++--------
> > > >  1 file changed, 5 insertions(+), 8 deletions(-)
> > > > 
> > > > diff --git a/scripts/keytoc.c b/scripts/keytoc.c
> > > > index 617317d6607e..105fa4b5413d 100644
> > > > --- a/scripts/keytoc.c
> > > > +++ b/scripts/keytoc.c
> > > > @@ -615,10 +615,6 @@ static int gen_key(const char *keyname, const char *path)
> > > >  	char *tmp, *key_name_c;
> > > >  
> > > >  	/* key name handling */
> > > > -	keyname = try_resolve_env(keyname);
> > > > -	if (!keyname)
> > > > -		exit(1);
> > > > -
> > > >  	tmp = key_name_c = strdup(keyname);
> > > >  
> > > >  	while (*tmp) {
> > > > @@ -628,10 +624,6 @@ static int gen_key(const char *keyname, const char *path)
> > > >  	}
> > > >  
> > > >  	/* path/URI handling */
> > > > -	path = try_resolve_env(path);
> > > > -	if (!path)
> > > > -		exit(1);
> > > > -
> > > 
> > > This was introduced by Basti in 685cc602e0ad ("keytoc: allow __ENV__
> > > lookup for keyname hint") to support:
> > > 
> > > CONFIG_CRYPTO_PUBLIC_KEYS="__ENV__FIT_KEY_NAME_HINT:__ENV__FIT_KEY"
> > 
> > Right, albeit I don't see why there should be two separate environment
> > variables for specifying one keyspec entry.
> > 
> > > I believe this patch breaks this usecase.
> > 
> > Of course :-/ but at the moment the simple use-case:
> > 
> > __ENV__FITKEY -> FITKEY="hint:<key>"
> > 
> > is broken. IMHO having two separate ENV variables is rather counter
> > intuitive. I also found myself in the following situation:
> > 
> > __ENV__FITKEYS -> FITKEYS="hint:<key> hint-2:<key-2> <key-3>"
> > 
> > This is not supported as well. So we should really define and document
> > the ENV behavior.
> 
> Let's recap the different possibilities. Plain without __ENV__
> substitution we would have:
> 
>   CONFIG_CRYPTO_PUBLIC_KEYS="pkcs11:object=foo"
>   CONFIG_CRYPTO_PUBLIC_KEYS="myhint:pkcs11:object=foo"
>   CONFIG_CRYPTO_PUBLIC_KEYS="myhint:pkcs11:object=foo /foobar/baz.der"
>   CONFIG_CRYPTO_PUBLIC_KEYS="myhint:pkcs11:object=foo myotherhint:/foobar/baz.der"
> 
> With Bastis patch we also support:
> 
>   myhint="myhint"
>   myname="pkcs11:object=foo"
>   CONFIG_CRYPTO_PUBLIC_KEYS="__ENV__myhint:__ENV__myname"
> 
> Now you want to support:
> 
>   mykeys="myhint:pkcs11:object=foo"
>   CONFIG_CRYPTO_PUBLIC_KEYS="__ENV__mykeys"
> 
> This would be easy to do by adding the call to try_resolve_env() in the
> loop like you did. You could keep the calls to try_resolve_env() in
> gen_key() in place to still support Bastis usecase.

Sure, too easy that I didn't thought about keeping the code. Albeit I
still find the use-case of having it separate not really intuitive. The
integration could still have two variables <hint> and <key> which is
afterwards combined to <hintkey>.

Anyway, I will keep Bastis use-case :)

> The other thing you like to support is:
> 
>   mykeys="myhint:pkcs11:object=foo myotherhint:/foobar/baz.der"
>   CONFIG_CRYPTO_PUBLIC_KEYS="__ENV__mykeys"
> 
> This is more complicated. Right now we assume one single key in each
> argv[] element and consequently call gen_key() for each element. With
> the above an argv[] element can expand to multiple keys and we would
> have to split this up first.
> 
> One way would be to do all __ENV__ substitutions upfront.
> 
> Not sure if it's worth the hassle though.

This is a use-case I would like to see implemented. Imagine the
following:

 * generic defconfig set CONFIG_CRYPTO_PUBLIC_KEYS="__ENV__FITKEYS"
 * platform-a: export FITKEYS="hint:keyspec"
 * platform-b: export FITKEYS="hint1:keyspec1 keyspec2 hint3:keyspec3"

This would make the integration easier since no adaption of the .config
is required. Without the support for the above, platform-b would need to
adapt the .config first:
 * platform-b: CONFIG_CRYPTO_PUBLIC_KEYS="__ENV__FITKEYS __ENV__FITKEY2 __ENV__FITKEY3"
 * platform-b: export FITKEYS="hint1:keyspec1"
 	       export FITKEYS2="keyspec2"
	       export FITKEYS3="hint3:keyspec3"

Regards,
  Marco


> 
> Sascha
> 
> -- 
> Pengutronix e.K.                           |                             |
> Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
> 31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
> Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |
>