On Mon, 10 Mar 2025 at 15:48, Jerome Forissier
<jerome.foriss...@linaro.org> wrote:
>
>
>
> On 3/10/25 14:04, Ilias Apalodimas wrote:
> > On Mon, 10 Mar 2025 at 14:48, Jerome Forissier
> > <jerome.foriss...@linaro.org> wrote:
> >>
> >>
> >>
> >> On 3/10/25 13:38, Ilias Apalodimas wrote:
> >>> 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?
> >>
> >> 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.
Fair enough. It's mostly a matter of design, I was thinking to limit
the load/unload only on the memory downloaded certs and always restore
the built in if present. But I think what we have here is fine as well
Reviewed-by: Ilias Apalodimas <ilias.apalodi...@linaro.org>
>
> 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
