On Mon, 10 Mar 2025 at 14:13, Jerome Forissier
<jerome.foriss...@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?
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
