[PATCH v2 1/2] keytoc: make keyspec a const char pointer

[PATCH v2 1/2] keytoc: make keyspec a const char pointer

[Marco Felsch]

Currently the code is working on the keypsec directly, this has the
drawback of the already existing freep variable to handle optional
key-names accordingly.

Delcare the keyspec as const and never operate on in directly to keep it
as it is and instead always alloc path and keyname, so the freep
variable can be dropped. For the split operation a simple helper was
added.

This prepares the code to fix the keyspec env handling which is done by
the next commit.

Signed-off-by: Marco Felsch <m.felsch@pengutronix.de>
---
 scripts/keytoc.c | 54 +++++++++++++++++++++++++++++++++---------------
 1 file changed, 37 insertions(+), 17 deletions(-)

diff --git a/scripts/keytoc.c b/scripts/keytoc.c
index c92465707f65..617317d6607e 100644
--- a/scripts/keytoc.c
+++ b/scripts/keytoc.c
@@ -653,6 +653,34 @@ static int gen_key(const char *keyname, const char *path)
 	return ret;
 }
 
+static void get_name_path(const char *keyspec, char **keyname, char **path)
+{
+	char *sep, *spec;
+
+	spec = strdup(keyspec);
+	if (!spec)
+		enomem_exit(__func__);
+
+	/* Split <key-hint>:<key-path> pair, <key-hint> is optional */
+	sep = strchr(spec, ':');
+	if (!sep) {
+		*path = spec;
+		return;
+	}
+
+	*sep = 0;
+	*keyname = strdup(spec);
+	if (!*keyname)
+		enomem_exit(__func__);
+
+	sep++;
+	*path = strdup(sep);
+	if (!*path)
+		enomem_exit(__func__);
+
+	free(spec);
+}
+
 int main(int argc, char *argv[])
 {
 	int i, opt, ret;
@@ -705,35 +733,27 @@ int main(int argc, char *argv[])
 	}
 
 	for (i = optind; i < argc; i++) {
-		char *keyspec = argv[i];
+		const char *keyspec = argv[i];
 		char *keyname = NULL;
-		char *path, *freep = NULL;
+		char *path = NULL;
 
-		if (!strncmp(keyspec, "pkcs11:", 7)) {
-			path = keyspec;
-		} else {
-			path = strchr(keyspec, ':');
-			if (path) {
-				*path = 0;
-				path++;
-				keyname = keyspec;
-			} else {
-				path = keyspec;
-			}
-		}
+		if (!strncmp(keyspec, "pkcs11:", 7))
+			path = strdup(keyspec);
+		else
+			get_name_path(keyspec, &keyname, &path);
 
 		if (!keyname) {
-			ret = asprintf(&freep, "key_%d", keynum++);
+			ret = asprintf(&keyname, "key_%d", keynum++);
 			if (ret < 0)
 				enomem_exit("asprintf");
-			keyname = freep;
 		}
 
 		ret = gen_key(keyname, path);
 		if (ret)
 			exit(1);
 
-		free(freep);
+		free(keyname);
+		free(path);
 	}
 
 	if (dts) {
-- 
2.39.5

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

[Marco Felsch]

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);
-
 	if (!strncmp(path, "pkcs11:", 7)) {
 		ret = engine_get_pub_key(path, &key);
 		if (ret)
@@ -737,6 +729,11 @@ int main(int argc, char *argv[])
 		char *keyname = NULL;
 		char *path = NULL;
 
+		/* Check if the keyspec is provided by the ENV first */
+		keyspec = try_resolve_env(keyspec);
+		if (!keyspec)
+			exit(1);
+
 		if (!strncmp(keyspec, "pkcs11:", 7))
 			path = strdup(keyspec);
 		else
-- 
2.39.5

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

[Sascha Hauer]

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

I believe this patch breaks this usecase.

Sascha

>  	if (!strncmp(path, "pkcs11:", 7)) {
>  		ret = engine_get_pub_key(path, &key);
>  		if (ret)
> @@ -737,6 +729,11 @@ int main(int argc, char *argv[])
>  		char *keyname = NULL;
>  		char *path = NULL;
>  
> +		/* Check if the keyspec is provided by the ENV first */
> +		keyspec = try_resolve_env(keyspec);
> +		if (!keyspec)
> +			exit(1);
> +
>  		if (!strncmp(keyspec, "pkcs11:", 7))
>  			path = strdup(keyspec);
>  		else
> -- 
> 2.39.5
> 
> 
> 

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

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

[Marco Felsch]

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.

Regards,
  Marco


> 
> Sascha
> 
> >  	if (!strncmp(path, "pkcs11:", 7)) {
> >  		ret = engine_get_pub_key(path, &key);
> >  		if (ret)
> > @@ -737,6 +729,11 @@ int main(int argc, char *argv[])
> >  		char *keyname = NULL;
> >  		char *path = NULL;
> >  
> > +		/* Check if the keyspec is provided by the ENV first */
> > +		keyspec = try_resolve_env(keyspec);
> > +		if (!keyspec)
> > +			exit(1);
> > +
> >  		if (!strncmp(keyspec, "pkcs11:", 7))
> >  			path = strdup(keyspec);
> >  		else
> > -- 
> > 2.39.5
> > 
> > 
> > 
> 
> -- 
> 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 |
> 

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

[Sascha Hauer]

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 |

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

[Marco Felsch]

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