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

[Jerome Forissier <jerome.forissier@linaro.org>]

On 3/10/25 14:04, Ilias Apalodimas wrote:
> On Mon, 10 Mar 2025 at 14:48, Jerome Forissier
> <jerome.forissier@linaro.org> wrote:
>>
>>
>>
>> On 3/10/25 13:38, Ilias Apalodimas wrote:
>>> On Mon, 10 Mar 2025 at 14:13, Jerome Forissier
>>> <jerome.forissier@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").
> 
> So right now if  a user defines a builtin cert it will be used on the
> first download.

Yes.

> If after that he defines a memory one, that will be
> used on the next download,

Yes.

> but if he unloads it shouldn't the built in
> be restired automatically?

If he unloads with "wget cacert 0 0" then it means clear certificates, so no,
the builtin should not be restored IMO. To restore them one needs to run
"wget cacert builtin".

I think it is cleaner to keep the same meaning for "wget cacert 0 0" whether or
not builtin certificates are enabled. It's a matter of consistency.

Thanks,
-- 
Jerome


> 
> Thanks
> /Ilias
>>
>> 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