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

[Sascha Hauer <s.hauer@pengutronix.de>]

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.

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.

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 |