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

[Jerome Forissier]

On 3/10/25 13:38, Ilias Apalodimas wrote:
> On Mon, 10 Mar 2025 at 14:13, Jerome Forissier
> <jerome.forissier at linaro.org> wrote:
>>
>>
>>
>> On 3/10/25 12:52, Ilias Apalodimas wrote:
>>> Hi Jerome,
>>>
>>> [...]
>>>
>>>
>>>>>>
>>>>>> +#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.
>>>
>>> Ok, so if you download a cert in memory and have u-boot with a builtin
>>> certificate, then the memory one will be overwritten in the first run.
>>
>> No, because the downloaded cert must have be made active via "wget cacert
>> <addr> <size>", which will set cacert_initialized to true, and thus the
>> built-in certs won't overwrite them. Or did I miss something?
> 
> Nop I did, when reading the patch. But should the command that clears
> the downloaded cert set cacert_initialized; to false now?

It's probably easier if it does not, so that "wget cacert 0 0" really clears
the certs. We have a command to restore the built-in ones ("wget cacert
builtin").

Thanks,
-- 
Jerome

> 
> Thanks
> /Ilias
>>
>> Cheers,
>> --
>> Jerome
>>
>>> This is not easy to solve, I was trying to think of ways to make the
>>> functionality clearer to users.
>>>
>>> Cheers
>>> /Ilias
>>>>
>>>> Cheers,
>>>> --
>>>> Jerome
>>>>
>>>>>
>>>>> [...]
>>>>>
>>>>> Cheers
>>>>> /Ilias