Re: [PATCH v3 3/4] crypto: keytoc: Split env-provided full keyspec on spaces

[Jonas Rebmann <jre@pengutronix.de>]

Hello Marco,

On 2026-03-17 01:00, Marco Felsch wrote:
> On 26-03-16, Jonas Rebmann wrote:
>> keytoc/CONFIG_CRYPTO_PUBLIC_KEYS can work with a complete keyspec
>> provided by an environment variable as opposed to providing single URIs.
>> This would be a very useful feature if it could also provide any number
>> of keys. Kconfig however provides keytoc with regular keyspecs already
>> split at spaces so without furhter measures, the env variable can only
>> be expanded into a single key.
>>
>> If a complete argument is provided via __ENV, split it at any space
>> character that is not escaped with a backslash in front of it.  An
>> actual backslash in a path needs to be escape with another backslash.
> 
> I find your commit message a bit hard to read to be honest. I had to
> read it twice to see what you actually want to add. So it could be
> shorten to:
> 
> Add support to provide multiple keyspec entries via a single environemnt
> (__ENV__) variable.

No. Earlier iterations would only support a single environment variable
but we decided that this behavior was too inconsistent, hence that all
positionals on keytoc should be handled in the same way, and that a
single positional shouldn't be treated as a special case. This means any
positional can be provided as an environment variable, unlike the RFC
version of this patch that would only handle a single environment
variable.

> Furthermore please adapt the Kconfig help message to keep it in sync.

Could you point me to a part of the Kconfig help text that would be
outdated by this? My intention was to implement the expected behavior
given the current Kconfig help text, and I just double-checked.

>>
>> Signed-off-by: Jonas Rebmann <jre@pengutronix.de>
>> ---
>>   scripts/keytoc.c | 55 ++++++++++++++++++++++++++++++++++++++++++-------------
>>   1 file changed, 42 insertions(+), 13 deletions(-)
>>
>> diff --git a/scripts/keytoc.c b/scripts/keytoc.c
>> index 1b99393fdc..c451ee081a 100644
>> --- a/scripts/keytoc.c
>> +++ b/scripts/keytoc.c
>> @@ -10,6 +10,8 @@
>>   
>>   #pragma GCC diagnostic ignored "-Wdeprecated-declarations" /* ENGINE deprecated in OpenSSL 3.0 */
>>   
>> +#include "include/string_util.h"
>> +
>>   #include <stdio.h>
>>   #include <string.h>
>>   #include <time.h>
>> @@ -787,6 +789,11 @@ static bool parse_info(char *p, struct keyinfo *out)
>>   
>>   static bool parse_keyspec(const char *keyspec, struct keyinfo *out)
>>   {
>> +	if (!strncmp(keyspec, "pkcs11:", 7)) { /* legacy format of pkcs11 URI */
>> +		out->path = strdup(keyspec);
>> +		return true;
>> +	}
>> +
>>   	char *sep, *spec;
>>   
>>   	spec = strdup(keyspec);
>> @@ -817,7 +824,7 @@ int main(int argc, char *argv[])
>>   {
>>   	int argi, opt, ret;
>>   	char *outfile = NULL;
>> -	int keycount;
>> +	size_t keycount, num_positionals;
> 
> Why size_t?

It used to be correct that keycount is an int because it was bounded by
argc. After this change, that is not the case anymore. keycount is an
index to an arbitrary-sized array hence size_t is correct.

Not that I'm expecting over 2147483647 to be a use case here, this is
solely correctness.

>>   	struct keyinfo *keylist;
>>   
>>   	outfilep = stdout;
>> @@ -853,22 +860,44 @@ int main(int argc, char *argv[])
>>   		exit(1);
>>   	}
>>   
>> -	keycount = argc - optind;
>> -	keylist = calloc(sizeof(struct keyinfo), keycount);
>>   
>> -	for (argi = 0; argi < keycount; argi++) {
>> -		const char *keyspec = try_resolve_env(argv[optind + argi]);
>> -		struct keyinfo *info = &keylist[argi];
>> +	num_positionals = argc - optind;
>> +	keycount = num_positionals;
>>   
>> -		if (!keyspec)
>> -			exit(1);
>> +	keylist = calloc(keycount, sizeof(*keylist));
>> +
> 
> No newline required. Furthermore I would prefer a list now since this
> would be much easier to read and to handle (without the below
> reallocarray).

I disagree.

On v2 you asked me to reduce the changes per patch to the bare minimum.

Since this used to be an array and I'm leaving it an array. Converting
it to a linked list, regardless of which variant one finds more
pleasing, would bloat this commit. The conversion to a linked list could
be done in a separate commit but I personally don't find reallocarray
problematic the context, especially given that the array is only ever
resized in the case that was previously broken (that is complete
multi-key keyspec in an environment variable).

>> +	if (!keylist)
>> +		enomem_exit("push");
>> +
>> +	int listi = 0;
>> +
>> +	for (argi = 0; argi < num_positionals; argi++) {
>> +		char *arg = strdup(argv[optind + argi]);
>> +		char *resolved = try_resolve_env(arg);
> 		^
> Why do you drop the 'const' here? Furthermore, why do you strdup()?

try_resolve_env does not return const, if that's what you mean by "drop
the 'const'". It returns either its non-const input or the non-const
return value of getenv. Are you suggesting it should be made const? It
could be.

The strdup of arg is not technically needed here given argv is non-const
and honestly I just wrongly assumed argv as const when I strudped here,
and maybe it should be. But with non-const argv it's not technically
needed. Do you want me to drop it?

If you're referring to the strdup of the result of try_resolve_env; If
it did expand an env variable, it needs to be duplicated before
splitting via strsep_unescaped or a successive getenv call on the same
variable name would unexpectedly return variable contents modified by
the later strsep_unescaped. If this bothers you, I could move that
strdup into try_resolve_env but for the other usages of try_resolve_env
that would not be neccessary.

> 
>> -		if (!strncmp(keyspec, "pkcs11:", 7)) { // legacy format of pkcs11 URI
>> -			info->path = strdup(keyspec);
>> +		if (arg == resolved) {
>> +			keylist[listi].path = arg;
>> +			listi++;
>>   		} else {
>> -			if (!parse_keyspec(keyspec, info)) {
>> -				fprintf(stderr, "invalid keyspec %i: %s\n", optind, keyspec);
>> -				exit(1);
>> +			char *keyspecs = strdup(resolved);
>> +			char *keyspec;
>> +
>> +			/* Keyspec given as env Variable,
>> +			 * remove it and add an arbitrary number of keyspecs from its contents
>> +			 */
>> +			keycount--;
>> +			while ((keyspec = strsep_unescaped(&keyspecs, " ", NULL))) {
>> +				keycount++;
>> +				keylist = reallocarray(keylist, keycount, sizeof(*keylist));
>> +				if (!keylist)
>> +					enomem_exit("realloc keylist");
>> +				bzero(keylist + (keycount - 1), sizeof(*keylist));
>> +				if (!parse_keyspec(keyspec, &keylist[listi])) {
>> +					fprintf(stderr, "invalid keyspec %i: %s\n", optind,
>> +						keyspec);
>> +					exit(1);
>> +				}
>> +				listi++;
>>   			}
> 
> Did you tested that all the other usecases are not broken by this
> change?

That's a too broad question to answer but Patch 4/4 explains backwards
compatibility of this change to the best of my knowledge.

> Regards,
>    Marco
> 
>>   		}
>>   	}
>>
>> -- 
>> 2.53.0.308.g50d063e335
>>
>>
>>
> 

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