On 3/10/25 14:04, Ilias Apalodimas wrote: > 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. 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
