Hi Ilias,
On 3/9/25 12:33, Ilias Apalodimas wrote:
> Hi Jerome
>
> On Wed, 5 Mar 2025 at 16:27, Jerome Forissier
> <jerome.foriss...@linaro.org> wrote:
>>
>
> [...]
>
>> @@ -304,28 +304,34 @@ static int set_auth(enum auth_mode auth)
>>
>> return CMD_RET_SUCCESS;
>> }
>> +#endif
>>
>> -static int set_cacert(char * const saddr, char * const ssz)
>> +#if CONFIG_IS_ENABLED(WGET_BUILTIN_CACERT)
>> +extern const char builtin_cacert[];
>> +extern const size_t builtin_cacert_size;
>> +static bool cacert_initialized;
>> +#endif
>
> These are better off under WGET_CACERT || WGET_BUILTIN_CACERT ?
No, because one can build with BUILTIN_CACERT=y and CACERT=n (the latter is for
the "wget cacert" command, which is not mandatory for built-in certs).
>
>> +
>> +#if CONFIG_IS_ENABLED(WGET_CACERT) || CONFIG_IS_ENABLED(WGET_BUILTIN_CACERT)
>> +static int _set_cacert(const void *addr, size_t sz)
>> {
>> mbedtls_x509_crt crt;
>> - ulong addr, sz;
>> + void *p;
>> int ret;
>>
>> if (cacert)
>> free(cacert);
>>
>> - addr = hextoul(saddr, NULL);
>> - sz = hextoul(ssz, NULL);
>> -
>> if (!addr) {
>> cacert = NULL;
>> cacert_size = 0;
>> return CMD_RET_SUCCESS;
>> }
>>
>> - cacert = malloc(sz);
>> - if (!cacert)
>> + p = malloc(sz);
>> + if (!p)
>> return CMD_RET_FAILURE;
>> + cacert = p;
>> cacert_size = sz;
>>
>> memcpy(cacert, (void *)addr, sz);
>> @@ -340,10 +346,32 @@ static int set_cacert(char * const saddr, char * const
>> ssz)
>> return CMD_RET_FAILURE;
>> }
>>
>> +#if CONFIG_IS_ENABLED(WGET_BUILTIN_CACERT)
>> + cacert_initialized = true;
>> +#endif
>> return CMD_RET_SUCCESS;
>> }
>> +
>> +#if CONFIG_IS_ENABLED(WGET_BUILTIN_CACERT)
>> +static int set_cacert_builtin(void)
>> +{
>> + return _set_cacert(builtin_cacert, builtin_cacert_size);
>> +}
>> #endif
>>
>> +#if CONFIG_IS_ENABLED(WGET_CACERT)
>> +static int set_cacert(char * const saddr, char * const ssz)
>> +{
>> + ulong addr, sz;
>> +
>> + addr = hextoul(saddr, NULL);
>> + sz = hextoul(ssz, NULL);
>> +
>> + return _set_cacert((void *)addr, sz);
>> +}
>> +#endif
>> +#endif /* CONFIG_WGET_CACERT || CONFIG_WGET_BUILTIN_CACERT */
>> +
>> static int wget_loop(struct udevice *udev, ulong dst_addr, char *uri)
>> {
>> #if CONFIG_IS_ENABLED(WGET_HTTPS)
>> @@ -373,8 +401,15 @@ static int wget_loop(struct udevice *udev, ulong
>> dst_addr, char *uri)
>> memset(&conn, 0, sizeof(conn));
>> #if CONFIG_IS_ENABLED(WGET_HTTPS)
>> if (is_https) {
>> - char *ca = cacert;
>> - size_t ca_sz = cacert_size;
>> + char *ca;
>> + size_t ca_sz;
>> +
>> +#if CONFIG_IS_ENABLED(WGET_BUILTIN_CACERT)
>> + if (!cacert_initialized)
>> + set_cacert_builtin();
>> +#endif
>
> The code and the rest of the patch seems fine, but the builtin vs
> downloaded cert is a bit confusing here.
> Since the built-in cert always gets initialized in the wget loop it
> would overwrite any certificates that are downloaded in memory?
The built-in certs are enabled only when cacert_initialized is false.
set_cacert_builtin() will set it to true (via _set_cacert()), so it will
happen only once. Note also that any successful "wget cacert" command
will also do the same. So effectively these two lines enable the
built-in certificates by default, that's all they do.
Cheers,
--
Jerome
>
> [...]
>
> Cheers
> /Ilias
