| Message ID | 20250305142650.2966738-5-jerome.forissier@linaro.org |
|---|---|
| State | New |
| Headers | show |
| Series | net: lwip: root certificates | expand |
Hi Jerome On Wed, 5 Mar 2025 at 16:27, Jerome Forissier <jerome.forissier@linaro.org> wrote: > [...] > @@ -304,28 +304,34 @@ static int set_auth(enum auth_mode auth) > > return CMD_RET_SUCCESS; > } > +#endif > > -static int set_cacert(char * const saddr, char * const ssz) > +#if CONFIG_IS_ENABLED(WGET_BUILTIN_CACERT) > +extern const char builtin_cacert[]; > +extern const size_t builtin_cacert_size; > +static bool cacert_initialized; > +#endif These are better off under WGET_CACERT || WGET_BUILTIN_CACERT ? > + > +#if CONFIG_IS_ENABLED(WGET_CACERT) || CONFIG_IS_ENABLED(WGET_BUILTIN_CACERT) > +static int _set_cacert(const void *addr, size_t sz) > { > mbedtls_x509_crt crt; > - ulong addr, sz; > + void *p; > int ret; > > if (cacert) > free(cacert); > > - addr = hextoul(saddr, NULL); > - sz = hextoul(ssz, NULL); > - > if (!addr) { > cacert = NULL; > cacert_size = 0; > return CMD_RET_SUCCESS; > } > > - cacert = malloc(sz); > - if (!cacert) > + p = malloc(sz); > + if (!p) > return CMD_RET_FAILURE; > + cacert = p; > cacert_size = sz; > > memcpy(cacert, (void *)addr, sz); > @@ -340,10 +346,32 @@ static int set_cacert(char * const saddr, char * const ssz) > return CMD_RET_FAILURE; > } > > +#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? [...] Cheers /Ilias
Hi Ilias, On 3/9/25 12:33, Ilias Apalodimas wrote: > Hi Jerome > > On Wed, 5 Mar 2025 at 16:27, Jerome Forissier > <jerome.forissier@linaro.org> wrote: >> > > [...] > >> @@ -304,28 +304,34 @@ static int set_auth(enum auth_mode auth) >> >> return CMD_RET_SUCCESS; >> } >> +#endif >> >> -static int set_cacert(char * const saddr, char * const ssz) >> +#if CONFIG_IS_ENABLED(WGET_BUILTIN_CACERT) >> +extern const char builtin_cacert[]; >> +extern const size_t builtin_cacert_size; >> +static bool cacert_initialized; >> +#endif > > These are better off under WGET_CACERT || WGET_BUILTIN_CACERT ? No, because one can build with BUILTIN_CACERT=y and CACERT=n (the latter is for the "wget cacert" command, which is not mandatory for built-in certs). > >> + >> +#if CONFIG_IS_ENABLED(WGET_CACERT) || CONFIG_IS_ENABLED(WGET_BUILTIN_CACERT) >> +static int _set_cacert(const void *addr, size_t sz) >> { >> mbedtls_x509_crt crt; >> - ulong addr, sz; >> + void *p; >> int ret; >> >> if (cacert) >> free(cacert); >> >> - addr = hextoul(saddr, NULL); >> - sz = hextoul(ssz, NULL); >> - >> if (!addr) { >> cacert = NULL; >> cacert_size = 0; >> return CMD_RET_SUCCESS; >> } >> >> - cacert = malloc(sz); >> - if (!cacert) >> + p = malloc(sz); >> + if (!p) >> return CMD_RET_FAILURE; >> + cacert = p; >> cacert_size = sz; >> >> memcpy(cacert, (void *)addr, sz); >> @@ -340,10 +346,32 @@ static int set_cacert(char * const saddr, char * const ssz) >> return CMD_RET_FAILURE; >> } >> >> +#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. Cheers,
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. 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
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? Cheers,
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? 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
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"). Thanks,
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. 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
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,
On Mon, 10 Mar 2025 at 15:48, Jerome Forissier <jerome.forissier@linaro.org> wrote: > > > > 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. 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.apalodimas@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
diff --git a/cmd/Kconfig b/cmd/Kconfig index d469217c0ea..312bf94d4e8 100644 --- a/cmd/Kconfig +++ b/cmd/Kconfig @@ -2185,6 +2185,20 @@ config WGET_CACERT Adds the "cacert" sub-command to wget to provide root certificates to the HTTPS engine. Must be in DER format. +config WGET_BUILTIN_CACERT + bool "Built-in CA certificates" + depends on WGET_HTTPS + select BUILD_BIN2C + +config WGET_BUILTIN_CACERT_PATH + string "Path to root certificates" + depends on WGET_BUILTIN_CACERT + default "cacert.crt" + help + Set this to the path to a DER-encoded X509 file containing + Certification Authority certificates, a.k.a. root certificates, for + the purpose of authenticating HTTPS connections. + endif # if CMD_NET config CMD_PXE diff --git a/cmd/net-lwip.c b/cmd/net-lwip.c index 1152c94a6dc..58c10fbec7d 100644 --- a/cmd/net-lwip.c +++ b/cmd/net-lwip.c @@ -41,6 +41,10 @@ U_BOOT_CMD(wget, 4, 1, do_wget, " - provide CA certificates (0 0 to remove current)" "\nwget cacert none|optional|required\n" " - set server certificate verification mode (default: optional)" +#if defined(CONFIG_WGET_BUILTIN_CACERT) + "\nwget cacert builtin\n" + " - use the builtin CA certificates" +#endif #endif ); #endif diff --git a/net/lwip/Makefile b/net/lwip/Makefile index 79dd6b3fb50..950c5316bb9 100644 --- a/net/lwip/Makefile +++ b/net/lwip/Makefile @@ -6,3 +6,9 @@ obj-$(CONFIG_CMD_DNS) += dns.o obj-$(CONFIG_CMD_PING) += ping.o obj-$(CONFIG_CMD_TFTPBOOT) += tftp.o obj-$(CONFIG_WGET) += wget.o + +ifeq (y,$(CONFIG_WGET_BUILTIN_CACERT)) +$(obj)/builtin_cacert.c: $(CONFIG_WGET_BUILTIN_CACERT_PATH:"%"=%) FORCE + $(call if_changed,bin2c,builtin_cacert) +obj-y += builtin_cacert.o +endif diff --git a/net/lwip/wget.c b/net/lwip/wget.c index c22843ee10d..ec098148835 100644 --- a/net/lwip/wget.c +++ b/net/lwip/wget.c @@ -304,28 +304,34 @@ static int set_auth(enum auth_mode auth) return CMD_RET_SUCCESS; } +#endif -static int set_cacert(char * const saddr, char * const ssz) +#if CONFIG_IS_ENABLED(WGET_BUILTIN_CACERT) +extern const char builtin_cacert[]; +extern const size_t builtin_cacert_size; +static bool cacert_initialized; +#endif + +#if CONFIG_IS_ENABLED(WGET_CACERT) || CONFIG_IS_ENABLED(WGET_BUILTIN_CACERT) +static int _set_cacert(const void *addr, size_t sz) { mbedtls_x509_crt crt; - ulong addr, sz; + void *p; int ret; if (cacert) free(cacert); - addr = hextoul(saddr, NULL); - sz = hextoul(ssz, NULL); - if (!addr) { cacert = NULL; cacert_size = 0; return CMD_RET_SUCCESS; } - cacert = malloc(sz); - if (!cacert) + p = malloc(sz); + if (!p) return CMD_RET_FAILURE; + cacert = p; cacert_size = sz; memcpy(cacert, (void *)addr, sz); @@ -340,10 +346,32 @@ static int set_cacert(char * const saddr, char * const ssz) return CMD_RET_FAILURE; } +#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 + ca = cacert; + ca_sz = cacert_size; if (cacert_auth_mode == AUTH_REQUIRED) { if (!ca || !ca_sz) { @@ -455,6 +490,10 @@ int do_wget(struct cmd_tbl *cmdtp, int flag, int argc, char * const argv[]) if (argc == 4 && !strncmp(argv[1], "cacert", strlen("cacert"))) return set_cacert(argv[2], argv[3]); if (argc == 3 && !strncmp(argv[1], "cacert", strlen("cacert"))) { +#if CONFIG_IS_ENABLED(WGET_BUILTIN_CACERT) + if (!strncmp(argv[2], "builtin", strlen("builtin"))) + return set_cacert_builtin(); +#endif if (!strncmp(argv[2], "none", strlen("none"))) return set_auth(AUTH_NONE); if (!strncmp(argv[2], "optional", strlen("optional")))
Introduce Kconfig symbols WGET_BUILTIN_CACERT and WGET_BUILTIN_CACERT_PATH to provide root certificates at build time. Usage example: wget -O cacert.crt https://cacerts.digicert.com/DigiCertTLSECCP384RootG5.crt make qemu_arm64_lwip_defconfig echo CONFIG_WGET_BUILTIN_CACERT=y >>.config echo CONFIG_WGET_BUILTIN_CACERT_PATH=cacert.crt >>.config make olddefconfig make -j$(nproc) CROSS_COMPILE="ccache aarch64-linux-gnu-" qemu-system-aarch64 -M virt -nographic -cpu max \ -object rng-random,id=rng0,filename=/dev/urandom \ -device virtio-rng-pci,rng=rng0 -bios u-boot.bin => dhcp # HTTPS transfer using the builtin CA certificates => wget https://digicert-tls-ecc-p384-root-g5.chain-demos.digicert.com/ 1867 bytes transferred in 1 ms (1.8 MiB/s) Bytes transferred = 1867 (74b hex) Signed-off-by: Jerome Forissier <jerome.forissier@linaro.org> --- cmd/Kconfig | 14 ++++++++++++ cmd/net-lwip.c | 4 ++++ net/lwip/Makefile | 6 +++++ net/lwip/wget.c | 57 +++++++++++++++++++++++++++++++++++++++-------- 4 files changed, 72 insertions(+), 9 deletions(-)