[PATCH v2 4/6] net: lwip: add support for built-in root certificates

[Jerome Forissier]

Hi Ilias,

On 3/9/25 12:33, Ilias Apalodimas wrote:
> Hi Jerome
> 
> On Wed, 5 Mar 2025 at 16:27, Jerome Forissier
> <jerome.forissier at 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