On Mon, 10 Mar 2025 at 14:48, Jerome Forissier <[email protected]> wrote: > > > > On 3/10/25 13:38, Ilias Apalodimas wrote: > > On Mon, 10 Mar 2025 at 14:13, Jerome Forissier > > <[email protected]> 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. If after that he defines a memory one, that will be used on the next download, but if he unloads it shouldn't the built in be restired automatically? 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
