[PATCH v2 0/6] net: lwip: root certificates

[PATCH v2 0/6] net: lwip: root certificates

[Jerome Forissier]

This series adds support for HTTP server authentication using root (CA)
certificates.

As a first step, the wget command is extended to support a sub-command:
cacert <addr> <size>. The memory region shall contain the CA
certificates. With this, it is possible to load the certificates from
storage or get them from the network for example, which is convenient
for testing at least. The Kconfig symbol for this feature is
WGET_CACERT=y.

Then new Kconfig symbols are added to support providing the certificates
at build time as a DER encoded X509 collection: WGET_BUILTIN_CACERT=y
and WGET_BUILTIN_CACERT_PATH=<some path>.

An example of how to use this feature is given in patch "doc: cmd: wget:
document cacert subcommand".

Changes in v2:
- Drop PEM support. It is unnecessary since conversion from PEM to DER
is easy, and PEM takes more space in memory. Another reason is the ugly
hack requiring to allocate one more byte for the null terminator in
case the file is PEM (therefore, text).
- Add 'wget cacert none|optional|required'
- Replace '#if defined CONFIG_X' with '#if CONFIG_IS_ENABLED(X)' in
net/lwip/wget.c Do NOT replace '#if defined(CONFIG_X)' in net/net-lwip.c
for consistency with how other symbols are treated in that file.
- Make builtin_cacert const
- Make cacert_size static
- BUILTIN_CACERT selects BUILD_BIN2C
- Add documentation in doc/cmd/wget.rst
- Apply review tags

Jerome Forissier (6):
  net: lwip: extend wget to support CA (root) certificates
  lwip: tls: enforce checking of server certificates based on CA
    availability
  lwip: tls: warn when no CA exists amd log certificate validation
    errors
  net: lwip: add support for built-in root certificates
  doc: cmd: wget: document cacert subcommand
  configs: qemu_arm64_lwip_defconfig: enable WGET_CACERT

 cmd/Kconfig                                   |  22 +++
 cmd/net-lwip.c                                |  21 ++-
 configs/qemu_arm64_lwip_defconfig             |   1 +
 doc/usage/cmd/wget.rst                        |  82 +++++++++-
 .../src/apps/altcp_tls/altcp_tls_mbedtls.c    |   9 +-
 .../lwip/apps/altcp_tls_mbedtls_opts.h        |   6 -
 net/lwip/Makefile                             |   6 +
 net/lwip/wget.c                               | 141 +++++++++++++++++-
 8 files changed, 273 insertions(+), 15 deletions(-)

-- 
2.43.0

[PATCH v2 1/6] net: lwip: extend wget to support CA (root) certificates

[Jerome Forissier]

Add the "cacert" (Certification Authority certificates) subcommand to
wget to pass root certificates to the code handling the HTTPS protocol.
The subcommand is enabled by the WGET_CACERT Kconfig symbol.

Usage example:

 => dhcp
 # Download some root certificates (note: not authenticated!)
 => wget https://cacerts.digicert.com/DigiCertTLSECCP384RootG5.crt
 # Provide root certificates
 => wget cacert $fileaddr $filesize
 # Enforce verification (it is optional by default)
 => wget cacert required
 # Forget the root certificates
 => wget cacert 0 0
 # Disable verification
 => wget cacert none

Signed-off-by: Jerome Forissier <jerome.forissier@linaro.org>
---
 cmd/Kconfig     |   8 ++++
 cmd/net-lwip.c  |  17 ++++++--
 net/lwip/wget.c | 102 ++++++++++++++++++++++++++++++++++++++++++++++--
 3 files changed, 121 insertions(+), 6 deletions(-)

diff --git a/cmd/Kconfig b/cmd/Kconfig
index 8dd42571abc..d469217c0ea 100644
--- a/cmd/Kconfig
+++ b/cmd/Kconfig
@@ -2177,6 +2177,14 @@ config WGET_HTTPS
 	help
 	  Enable TLS over http for wget.
 
+config WGET_CACERT
+	bool "wget cacert"
+	depends on CMD_WGET
+	depends on WGET_HTTPS
+	help
+	  Adds the "cacert" sub-command to wget to provide root certificates
+	  to the HTTPS engine. Must be in DER format.
+
 endif  # if CMD_NET
 
 config CMD_PXE
diff --git a/cmd/net-lwip.c b/cmd/net-lwip.c
index 0fd446ecb20..1152c94a6dc 100644
--- a/cmd/net-lwip.c
+++ b/cmd/net-lwip.c
@@ -27,9 +27,20 @@ U_BOOT_CMD(dns, 3, 1, do_dns, "lookup the IP of a hostname",
 #endif
 
 #if defined(CONFIG_CMD_WGET)
-U_BOOT_CMD(wget, 3, 1, do_wget,
-	   "boot image via network using HTTP/HTTPS protocol",
+U_BOOT_CMD(wget, 4, 1, do_wget,
+	   "boot image via network using HTTP/HTTPS protocol"
+#if defined(CONFIG_WGET_CACERT)
+	   "\nwget cacert - configure wget root certificates"
+#endif
+	   ,
 	   "[loadAddress] url\n"
-	   "wget [loadAddress] [host:]path"
+	   "wget [loadAddress] [host:]path\n"
+	   "    - load file"
+#if defined(CONFIG_WGET_CACERT)
+	   "\nwget cacert <address> <length>\n"
+	   "    - provide CA certificates (0 0 to remove current)"
+	   "\nwget cacert none|optional|required\n"
+	   "    - set server certificate verification mode (default: optional)"
+#endif
 );
 #endif
diff --git a/net/lwip/wget.c b/net/lwip/wget.c
index 14f27d42998..c22843ee10d 100644
--- a/net/lwip/wget.c
+++ b/net/lwip/wget.c
@@ -285,9 +285,68 @@ static err_t httpc_headers_done_cb(httpc_state_t *connection, void *arg, struct
 	return ERR_OK;
 }
 
+#if CONFIG_IS_ENABLED(WGET_HTTPS)
+enum auth_mode {
+	AUTH_NONE,
+	AUTH_OPTIONAL,
+	AUTH_REQUIRED,
+};
+
+static char *cacert;
+static size_t cacert_size;
+static enum auth_mode cacert_auth_mode = AUTH_OPTIONAL;
+#endif
+
+#if CONFIG_IS_ENABLED(WGET_CACERT)
+static int set_auth(enum auth_mode auth)
+{
+	cacert_auth_mode = auth;
+
+	return CMD_RET_SUCCESS;
+}
+
+static int set_cacert(char * const saddr, char * const ssz)
+{
+	mbedtls_x509_crt crt;
+	ulong addr, sz;
+	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)
+		return CMD_RET_FAILURE;
+	cacert_size = sz;
+
+	memcpy(cacert, (void *)addr, sz);
+
+	mbedtls_x509_crt_init(&crt);
+	ret = mbedtls_x509_crt_parse(&crt, cacert, cacert_size);
+	if (ret) {
+		printf("Could not parse certificates (%d)\n", ret);
+		free(cacert);
+		cacert = NULL;
+		cacert_size = 0;
+		return CMD_RET_FAILURE;
+	}
+
+	return CMD_RET_SUCCESS;
+}
+#endif
+
 static int wget_loop(struct udevice *udev, ulong dst_addr, char *uri)
 {
-#if defined CONFIG_WGET_HTTPS
+#if CONFIG_IS_ENABLED(WGET_HTTPS)
 	altcp_allocator_t tls_allocator;
 #endif
 	httpc_connection_t conn;
@@ -312,11 +371,34 @@ static int wget_loop(struct udevice *udev, ulong dst_addr, char *uri)
 		return -1;
 
 	memset(&conn, 0, sizeof(conn));
-#if defined CONFIG_WGET_HTTPS
+#if CONFIG_IS_ENABLED(WGET_HTTPS)
 	if (is_https) {
+		char *ca = cacert;
+		size_t ca_sz = cacert_size;
+
+		if (cacert_auth_mode == AUTH_REQUIRED) {
+			if (!ca || !ca_sz) {
+				printf("Error: cacert authentication mode is "
+				       "'required' but no CA certificates "
+				       "given\n");
+				return CMD_RET_FAILURE;
+		       }
+		} else if (cacert_auth_mode == AUTH_NONE) {
+			ca = NULL;
+			ca_sz = 0;
+		} else if (cacert_auth_mode == AUTH_OPTIONAL) {
+			/*
+			 * Nothing to do, this is the default behavior of
+			 * altcp_tls to check server certificates against CA
+			 * certificates when the latter are provided and proceed
+			 * with no verification if not.
+			 */
+		}
+
 		tls_allocator.alloc = &altcp_tls_alloc;
 		tls_allocator.arg =
-			altcp_tls_create_config_client(NULL, 0, ctx.server_name);
+			altcp_tls_create_config_client(ca, ca_sz,
+						       ctx.server_name);
 
 		if (!tls_allocator.arg) {
 			log_err("error: Cannot create a TLS connection\n");
@@ -369,6 +451,20 @@ int do_wget(struct cmd_tbl *cmdtp, int flag, int argc, char * const argv[])
 	ulong dst_addr;
 	char nurl[1024];
 
+#if CONFIG_IS_ENABLED(WGET_CACERT)
+	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 (!strncmp(argv[2], "none", strlen("none")))
+			return set_auth(AUTH_NONE);
+		if (!strncmp(argv[2], "optional", strlen("optional")))
+			return set_auth(AUTH_OPTIONAL);
+		if (!strncmp(argv[2], "required", strlen("required")))
+			return set_auth(AUTH_REQUIRED);
+		return CMD_RET_USAGE;
+	}
+#endif
+
 	if (argc < 2 || argc > 3)
 		return CMD_RET_USAGE;
 
-- 
2.43.0

[PATCH v2 2/6] lwip: tls: enforce checking of server certificates based on CA availability

[Jerome Forissier]

Instead of relying on some build time configuration to determine if
server certificates need to be checked against CA certificates, do it
based on the availability of such certificates. If no CA is configured
then no check can succeed; on the other hand if we have CA certs then
we should not ignore them. It is always possible to remove the CA certs
(via 'wget cacert 0 0') to force an HTTPS download that would fail
certificate validation.

Signed-off-by: Jerome Forissier <jerome.forissier@linaro.org>
Reviewed-by: Ilias Apalodimas <ilias.apalodimas@linaro.org>
---
 lib/lwip/lwip/src/apps/altcp_tls/altcp_tls_mbedtls.c        | 3 ++-
 .../lwip/src/include/lwip/apps/altcp_tls_mbedtls_opts.h     | 6 ------
 2 files changed, 2 insertions(+), 7 deletions(-)

diff --git a/lib/lwip/lwip/src/apps/altcp_tls/altcp_tls_mbedtls.c b/lib/lwip/lwip/src/apps/altcp_tls/altcp_tls_mbedtls.c
index 46421588fef..fa3d1d74fed 100644
--- a/lib/lwip/lwip/src/apps/altcp_tls/altcp_tls_mbedtls.c
+++ b/lib/lwip/lwip/src/apps/altcp_tls/altcp_tls_mbedtls.c
@@ -786,6 +786,7 @@ altcp_tls_create_config(int is_server, u8_t cert_count, u8_t pkey_count, int hav
   int ret;
   struct altcp_tls_config *conf;
   mbedtls_x509_crt *mem;
+  int authmode = have_ca ? MBEDTLS_SSL_VERIFY_REQUIRED : MBEDTLS_SSL_VERIFY_NONE;
 
   if (TCP_WND < MBEDTLS_SSL_IN_CONTENT_LEN || TCP_WND < MBEDTLS_SSL_OUT_CONTENT_LEN) {
     LWIP_DEBUGF(ALTCP_MBEDTLS_DEBUG|LWIP_DBG_LEVEL_SERIOUS,
@@ -840,7 +841,7 @@ altcp_tls_create_config(int is_server, u8_t cert_count, u8_t pkey_count, int hav
     altcp_mbedtls_free_config(conf);
     return NULL;
   }
-  mbedtls_ssl_conf_authmode(&conf->conf, ALTCP_MBEDTLS_AUTHMODE);
+  mbedtls_ssl_conf_authmode(&conf->conf, authmode);
 
   mbedtls_ssl_conf_rng(&conf->conf, mbedtls_ctr_drbg_random, &altcp_tls_entropy_rng->ctr_drbg);
 #if ALTCP_MBEDTLS_LIB_DEBUG != LWIP_DBG_OFF
diff --git a/lib/lwip/lwip/src/include/lwip/apps/altcp_tls_mbedtls_opts.h b/lib/lwip/lwip/src/include/lwip/apps/altcp_tls_mbedtls_opts.h
index e41301c061c..71aa5993935 100644
--- a/lib/lwip/lwip/src/include/lwip/apps/altcp_tls_mbedtls_opts.h
+++ b/lib/lwip/lwip/src/include/lwip/apps/altcp_tls_mbedtls_opts.h
@@ -100,12 +100,6 @@
 #define ALTCP_MBEDTLS_SESSION_TICKET_TIMEOUT_SECONDS  (60 * 60 * 24)
 #endif
 
-/** Certificate verification mode: MBEDTLS_SSL_VERIFY_NONE, MBEDTLS_SSL_VERIFY_OPTIONAL (default),
- * MBEDTLS_SSL_VERIFY_REQUIRED (recommended)*/
-#ifndef ALTCP_MBEDTLS_AUTHMODE
-#define ALTCP_MBEDTLS_AUTHMODE                        MBEDTLS_SSL_VERIFY_OPTIONAL
-#endif
-
 #endif /* LWIP_ALTCP */
 
 #endif /* LWIP_HDR_ALTCP_TLS_OPTS_H */
-- 
2.43.0

[PATCH v2 3/6] lwip: tls: warn when no CA exists amd log certificate validation errors

[Jerome Forissier]

Using HTTPS without root (CA) certificates is a security issue. Print a
warning in this case. Also, when certificate verification fail, print
an additional message because "HTTP client error 4" is not very
informative (4 is HTTPC_RESULT_ERR_CLOSED).

Signed-off-by: Jerome Forissier <jerome.forissier@linaro.org>
Reviewed-by: Ilias Apalodimas <ilias.apalodimas@linaro.org>
---
 lib/lwip/lwip/src/apps/altcp_tls/altcp_tls_mbedtls.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/lib/lwip/lwip/src/apps/altcp_tls/altcp_tls_mbedtls.c b/lib/lwip/lwip/src/apps/altcp_tls/altcp_tls_mbedtls.c
index fa3d1d74fed..ef51a5ac168 100644
--- a/lib/lwip/lwip/src/apps/altcp_tls/altcp_tls_mbedtls.c
+++ b/lib/lwip/lwip/src/apps/altcp_tls/altcp_tls_mbedtls.c
@@ -298,6 +298,9 @@ altcp_mbedtls_lower_recv_process(struct altcp_pcb *conn, altcp_mbedtls_state_t *
     if (ret != 0) {
       LWIP_DEBUGF(ALTCP_MBEDTLS_DEBUG, ("mbedtls_ssl_handshake failed: %d\n", ret));
       /* handshake failed, connection has to be closed */
+      if (ret == MBEDTLS_ERR_X509_CERT_VERIFY_FAILED) {
+        printf("Certificate verification failed\n");
+      }
       if (conn->err) {
         conn->err(conn->arg, ERR_CLSD);
       }
@@ -841,6 +844,9 @@ altcp_tls_create_config(int is_server, u8_t cert_count, u8_t pkey_count, int hav
     altcp_mbedtls_free_config(conf);
     return NULL;
   }
+  if (authmode == MBEDTLS_SSL_VERIFY_NONE) {
+     printf("WARNING: no CA certificates, HTTPS connections not authenticated\n");
+  }
   mbedtls_ssl_conf_authmode(&conf->conf, authmode);
 
   mbedtls_ssl_conf_rng(&conf->conf, mbedtls_ctr_drbg_random, &altcp_tls_entropy_rng->ctr_drbg);
-- 
2.43.0

[PATCH v2 4/6] net: lwip: add support for built-in root certificates

[Jerome Forissier]

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(-)

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")))
-- 
2.43.0

[PATCH v2 5/6] doc: cmd: wget: document cacert subcommand

[Jerome Forissier]

Document the 'wget cacert' subcommand which allows to configure root
(CA) certificates for HTTPS.

Signed-off-by: Jerome Forissier <jerome.forissier@linaro.org>
---
 doc/usage/cmd/wget.rst | 82 ++++++++++++++++++++++++++++++++++++++++--
 1 file changed, 80 insertions(+), 2 deletions(-)

diff --git a/doc/usage/cmd/wget.rst b/doc/usage/cmd/wget.rst
index 48bedf1e845..cc82e495a29 100644
--- a/doc/usage/cmd/wget.rst
+++ b/doc/usage/cmd/wget.rst
@@ -12,7 +12,9 @@ Synopsis
 ::
 
     wget [address] [host:]path
-    wget [address] url          # lwIP only
+    wget [address] url                  # lwIP only
+    wget cacert none|optional|required  # lwIP only
+    wget cacert <address> <size>        # lwIP only
 
 
 Description
@@ -54,6 +56,32 @@ address
 url
     HTTP or HTTPS URL, that is: http[s]://<host>[:<port>]/<path>.
 
+The cacert (stands for 'Certification Authority certificates') subcommand is
+used to provide root certificates for the purpose of HTTPS authentication. It
+also allows to enable or disable authentication.
+
+wget cacert <address> <size>
+
+address
+    memory address of the root certificates in X509 DER format
+
+size
+    the size of the root certificates
+
+wget cacert none|optional|required
+
+none
+    certificate verification is disabled. HTTPS is used without any server
+    authentication (unsafe)
+optional
+    certificate verification is enabled provided root certificates have been
+    provided via wget cacert <addr> <size> or wget cacert builtin. Otherwise
+    HTTPS is used without any server authentication (unsafe).
+required
+    certificate verification is mandatory. If no root certificates have been
+    configured, HTTPS transfers will fail.
+
+
 Examples
 --------
 
@@ -97,11 +125,61 @@ In the example the following steps are executed:
    1694892032 bytes transferred in 492181 ms (3.3 MiB/s)
    Bytes transferred = 1694892032 (65060000 hex)
 
+Here is an example showing how to configure built-in root certificates as
+well as providing some at run time. In this example it is assumed that
+CONFIG_WGET_BUILTIN_CACERT_PATH=DigiCertTLSRSA4096RootG5.crt downloaded from
+https://cacerts.digicert.com/DigiCertTLSRSA4096RootG5.crt.
+
+::
+
+   # Make sure IP is configured
+   => dhcp
+   # When built-in certificates are configured, authentication is mandatory
+   # (i.e., "wget cacert required"). Use a test server...
+   => wget https://digicert-tls-rsa4096-root-g5.chain-demos.digicert.com/
+   1864 bytes transferred in 1 ms (1.8 MiB/s)
+   Bytes transferred = 1864 (748 hex)
+   # Another server not signed against Digicert will fail
+   => wget https://www.google.com/
+   Certificate verification failed
+
+   HTTP client error 4
+   # Disable authentication to allow the command to proceed anyways
+   => wget cacert none
+   => wget https://www.google.com/
+   WARNING: no CA certificates, HTTPS connections not authenticated
+   16683 bytes transferred in 15 ms (1.1 MiB/s)
+   Bytes transferred = 16683 (412b hex)
+   # Force verification but unregister the CA certificates
+   => wget cacert required
+   => wget cacert 0 0
+   # Unsurprisingly, download fails
+   => wget https://digicert-tls-rsa4096-root-g5.chain-demos.digicert.com/
+   Error: cacert authentication mode is 'required' but no CA certificates given
+   # Get the same certificates as above from the network
+   => wget cacert none
+   => wget https://cacerts.digicert.com/DigiCertTLSRSA4096RootG5.crt
+   WARNING: no CA certificates, HTTPS connections not authenticated
+   1386 bytes transferred in 1 ms (1.3 MiB/s)
+   Bytes transferred = 1386 (56a hex)
+   # Register them and force authentication
+   => wget cacert $fileaddr $filesize
+   => wget cacert required
+   # Authentication is operational again
+   => wget https://digicert-tls-rsa4096-root-g5.chain-demos.digicert.com/
+   1864 bytes transferred in 1 ms (1.8 MiB/s)
+   Bytes transferred = 1864 (748 hex)
+   # The builtin certificates can be restored at any time
+   => wget cacert builtin
+
 Configuration
 -------------
 
 The command is only available if CONFIG_CMD_WGET=y.
-To enable lwIP support set CONFIG_NET_LWIP=y.
+To enable lwIP support set CONFIG_NET_LWIP=y. In this case, root certificates
+support can be enabled via CONFIG_WGET_BUILTIN_CACERT=y
+CONFIG_WGET_BUILTIN_CACERT_PATH=<some path> (for built-in certificates) and/or
+CONFIG_WGET_CACERT=y (for the wget cacert command).
 
 TCP Selective Acknowledgments in the legacy network stack can be enabled via
 CONFIG_PROT_TCP_SACK=y. This will improve the download speed. Selective
-- 
2.43.0

[PATCH v2 6/6] configs: qemu_arm64_lwip_defconfig: enable WGET_CACERT

[Jerome Forissier]

Enable the "wget cacert" command.

Signed-off-by: Jerome Forissier <jerome.forissier@linaro.org>
Reviewed-by: Ilias Apalodimas <ilias.apalodimas@linaro.org>
---
 configs/qemu_arm64_lwip_defconfig | 1 +
 1 file changed, 1 insertion(+)

diff --git a/configs/qemu_arm64_lwip_defconfig b/configs/qemu_arm64_lwip_defconfig
index 754c770c33f..814e98729a3 100644
--- a/configs/qemu_arm64_lwip_defconfig
+++ b/configs/qemu_arm64_lwip_defconfig
@@ -8,3 +8,4 @@ CONFIG_CMD_DNS=y
 CONFIG_CMD_WGET=y
 CONFIG_EFI_HTTP_BOOT=y
 CONFIG_WGET_HTTPS=y
+CONFIG_WGET_CACERT=y
-- 
2.43.0

Re: [PATCH v2 1/6] net: lwip: extend wget to support CA (root) certificates

[Heinrich Schuchardt]

On 05.03.25 15:26, Jerome Forissier wrote:
> Add the "cacert" (Certification Authority certificates) subcommand to
> wget to pass root certificates to the code handling the HTTPS protocol.
> The subcommand is enabled by the WGET_CACERT Kconfig symbol.
>
> Usage example:
>
>   => dhcp
>   # Download some root certificates (note: not authenticated!)
>   => wget https://cacerts.digicert.com/DigiCertTLSECCP384RootG5.crt
>   # Provide root certificates
>   => wget cacert $fileaddr $filesize
>   # Enforce verification (it is optional by default)
>   => wget cacert required
>   # Forget the root certificates
>   => wget cacert 0 0
>   # Disable verification
>   => wget cacert none
>
> Signed-off-by: Jerome Forissier <jerome.forissier@linaro.org>
> ---
>   cmd/Kconfig     |   8 ++++
>   cmd/net-lwip.c  |  17 ++++++--
>   net/lwip/wget.c | 102 ++++++++++++++++++++++++++++++++++++++++++++++--
>   3 files changed, 121 insertions(+), 6 deletions(-)
>
> diff --git a/cmd/Kconfig b/cmd/Kconfig
> index 8dd42571abc..d469217c0ea 100644
> --- a/cmd/Kconfig
> +++ b/cmd/Kconfig
> @@ -2177,6 +2177,14 @@ config WGET_HTTPS
>   	help
>   	  Enable TLS over http for wget.
>
> +config WGET_CACERT
> +	bool "wget cacert"
> +	depends on CMD_WGET
> +	depends on WGET_HTTPS
> +	help
> +	  Adds the "cacert" sub-command to wget to provide root certificates
> +	  to the HTTPS engine. Must be in DER format.
> +

Shouldn't we build CA certs into U-Boot?
Downloading certs from unsafe media is not a good replacement.

Best regards

Heinrich

>   endif  # if CMD_NET
>
>   config CMD_PXE
> diff --git a/cmd/net-lwip.c b/cmd/net-lwip.c
> index 0fd446ecb20..1152c94a6dc 100644
> --- a/cmd/net-lwip.c
> +++ b/cmd/net-lwip.c
> @@ -27,9 +27,20 @@ U_BOOT_CMD(dns, 3, 1, do_dns, "lookup the IP of a hostname",
>   #endif
>
>   #if defined(CONFIG_CMD_WGET)
> -U_BOOT_CMD(wget, 3, 1, do_wget,
> -	   "boot image via network using HTTP/HTTPS protocol",
> +U_BOOT_CMD(wget, 4, 1, do_wget,
> +	   "boot image via network using HTTP/HTTPS protocol"
> +#if defined(CONFIG_WGET_CACERT)
> +	   "\nwget cacert - configure wget root certificates"
> +#endif
> +	   ,
>   	   "[loadAddress] url\n"
> -	   "wget [loadAddress] [host:]path"
> +	   "wget [loadAddress] [host:]path\n"
> +	   "    - load file"
> +#if defined(CONFIG_WGET_CACERT)
> +	   "\nwget cacert <address> <length>\n"
> +	   "    - provide CA certificates (0 0 to remove current)"
> +	   "\nwget cacert none|optional|required\n"
> +	   "    - set server certificate verification mode (default: optional)"
> +#endif
>   );
>   #endif
> diff --git a/net/lwip/wget.c b/net/lwip/wget.c
> index 14f27d42998..c22843ee10d 100644
> --- a/net/lwip/wget.c
> +++ b/net/lwip/wget.c
> @@ -285,9 +285,68 @@ static err_t httpc_headers_done_cb(httpc_state_t *connection, void *arg, struct
>   	return ERR_OK;
>   }
>
> +#if CONFIG_IS_ENABLED(WGET_HTTPS)
> +enum auth_mode {
> +	AUTH_NONE,
> +	AUTH_OPTIONAL,
> +	AUTH_REQUIRED,
> +};
> +
> +static char *cacert;
> +static size_t cacert_size;
> +static enum auth_mode cacert_auth_mode = AUTH_OPTIONAL;
> +#endif
> +
> +#if CONFIG_IS_ENABLED(WGET_CACERT)
> +static int set_auth(enum auth_mode auth)
> +{
> +	cacert_auth_mode = auth;
> +
> +	return CMD_RET_SUCCESS;
> +}
> +
> +static int set_cacert(char * const saddr, char * const ssz)
> +{
> +	mbedtls_x509_crt crt;
> +	ulong addr, sz;
> +	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)
> +		return CMD_RET_FAILURE;
> +	cacert_size = sz;
> +
> +	memcpy(cacert, (void *)addr, sz);
> +
> +	mbedtls_x509_crt_init(&crt);
> +	ret = mbedtls_x509_crt_parse(&crt, cacert, cacert_size);
> +	if (ret) {
> +		printf("Could not parse certificates (%d)\n", ret);
> +		free(cacert);
> +		cacert = NULL;
> +		cacert_size = 0;
> +		return CMD_RET_FAILURE;
> +	}
> +
> +	return CMD_RET_SUCCESS;
> +}
> +#endif
> +
>   static int wget_loop(struct udevice *udev, ulong dst_addr, char *uri)
>   {
> -#if defined CONFIG_WGET_HTTPS
> +#if CONFIG_IS_ENABLED(WGET_HTTPS)
>   	altcp_allocator_t tls_allocator;
>   #endif
>   	httpc_connection_t conn;
> @@ -312,11 +371,34 @@ static int wget_loop(struct udevice *udev, ulong dst_addr, char *uri)
>   		return -1;
>
>   	memset(&conn, 0, sizeof(conn));
> -#if defined CONFIG_WGET_HTTPS
> +#if CONFIG_IS_ENABLED(WGET_HTTPS)
>   	if (is_https) {
> +		char *ca = cacert;
> +		size_t ca_sz = cacert_size;
> +
> +		if (cacert_auth_mode == AUTH_REQUIRED) {
> +			if (!ca || !ca_sz) {
> +				printf("Error: cacert authentication mode is "
> +				       "'required' but no CA certificates "
> +				       "given\n");
> +				return CMD_RET_FAILURE;
> +		       }
> +		} else if (cacert_auth_mode == AUTH_NONE) {
> +			ca = NULL;
> +			ca_sz = 0;
> +		} else if (cacert_auth_mode == AUTH_OPTIONAL) {
> +			/*
> +			 * Nothing to do, this is the default behavior of
> +			 * altcp_tls to check server certificates against CA
> +			 * certificates when the latter are provided and proceed
> +			 * with no verification if not.
> +			 */
> +		}
> +
>   		tls_allocator.alloc = &altcp_tls_alloc;
>   		tls_allocator.arg =
> -			altcp_tls_create_config_client(NULL, 0, ctx.server_name);
> +			altcp_tls_create_config_client(ca, ca_sz,
> +						       ctx.server_name);
>
>   		if (!tls_allocator.arg) {
>   			log_err("error: Cannot create a TLS connection\n");
> @@ -369,6 +451,20 @@ int do_wget(struct cmd_tbl *cmdtp, int flag, int argc, char * const argv[])
>   	ulong dst_addr;
>   	char nurl[1024];
>
> +#if CONFIG_IS_ENABLED(WGET_CACERT)
> +	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 (!strncmp(argv[2], "none", strlen("none")))
> +			return set_auth(AUTH_NONE);
> +		if (!strncmp(argv[2], "optional", strlen("optional")))
> +			return set_auth(AUTH_OPTIONAL);
> +		if (!strncmp(argv[2], "required", strlen("required")))
> +			return set_auth(AUTH_REQUIRED);
> +		return CMD_RET_USAGE;
> +	}
> +#endif
> +
>   	if (argc < 2 || argc > 3)
>   		return CMD_RET_USAGE;
>

Re: [PATCH v2 1/6] net: lwip: extend wget to support CA (root) certificates

[Jerome Forissier]

Hi Heinrich,

On 3/5/25 16:07, Heinrich Schuchardt wrote:
> On 05.03.25 15:26, Jerome Forissier wrote:
>> Add the "cacert" (Certification Authority certificates) subcommand to
>> wget to pass root certificates to the code handling the HTTPS protocol.
>> The subcommand is enabled by the WGET_CACERT Kconfig symbol.
>>
>> Usage example:
>>
>>   => dhcp
>>   # Download some root certificates (note: not authenticated!)
>>   => wget https://cacerts.digicert.com/DigiCertTLSECCP384RootG5.crt
>>   # Provide root certificates
>>   => wget cacert $fileaddr $filesize
>>   # Enforce verification (it is optional by default)
>>   => wget cacert required
>>   # Forget the root certificates
>>   => wget cacert 0 0
>>   # Disable verification
>>   => wget cacert none
>>
>> Signed-off-by: Jerome Forissier <jerome.forissier@linaro.org>
>> ---
>>   cmd/Kconfig     |   8 ++++
>>   cmd/net-lwip.c  |  17 ++++++--
>>   net/lwip/wget.c | 102 ++++++++++++++++++++++++++++++++++++++++++++++--
>>   3 files changed, 121 insertions(+), 6 deletions(-)
>>
>> diff --git a/cmd/Kconfig b/cmd/Kconfig
>> index 8dd42571abc..d469217c0ea 100644
>> --- a/cmd/Kconfig
>> +++ b/cmd/Kconfig
>> @@ -2177,6 +2177,14 @@ config WGET_HTTPS
>>       help
>>         Enable TLS over http for wget.
>>
>> +config WGET_CACERT
>> +    bool "wget cacert"
>> +    depends on CMD_WGET
>> +    depends on WGET_HTTPS
>> +    help
>> +      Adds the "cacert" sub-command to wget to provide root certificates
>> +      to the HTTPS engine. Must be in DER format.
>> +
> 
> Shouldn't we build CA certs into U-Boot?
> Downloading certs from unsafe media is not a good replacement.

That's the purpose of patch 4/6 [1]. Although downloading may still be a
valid option when used with hash verification as I mentioned in a reply to
Ilias in v1 [2].

[1] https://lists.denx.de/pipermail/u-boot/2025-March/582567.html
[2] https://lists.denx.de/pipermail/u-boot/2025-February/582102.html

Best,
-- 
Jerome

> 
> Best regards
> 
> Heinrich
> 
>>   endif  # if CMD_NET
>>
>>   config CMD_PXE
>> diff --git a/cmd/net-lwip.c b/cmd/net-lwip.c
>> index 0fd446ecb20..1152c94a6dc 100644
>> --- a/cmd/net-lwip.c
>> +++ b/cmd/net-lwip.c
>> @@ -27,9 +27,20 @@ U_BOOT_CMD(dns, 3, 1, do_dns, "lookup the IP of a hostname",
>>   #endif
>>
>>   #if defined(CONFIG_CMD_WGET)
>> -U_BOOT_CMD(wget, 3, 1, do_wget,
>> -       "boot image via network using HTTP/HTTPS protocol",
>> +U_BOOT_CMD(wget, 4, 1, do_wget,
>> +       "boot image via network using HTTP/HTTPS protocol"
>> +#if defined(CONFIG_WGET_CACERT)
>> +       "\nwget cacert - configure wget root certificates"
>> +#endif
>> +       ,
>>          "[loadAddress] url\n"
>> -       "wget [loadAddress] [host:]path"
>> +       "wget [loadAddress] [host:]path\n"
>> +       "    - load file"
>> +#if defined(CONFIG_WGET_CACERT)
>> +       "\nwget cacert <address> <length>\n"
>> +       "    - provide CA certificates (0 0 to remove current)"
>> +       "\nwget cacert none|optional|required\n"
>> +       "    - set server certificate verification mode (default: optional)"
>> +#endif
>>   );
>>   #endif
>> diff --git a/net/lwip/wget.c b/net/lwip/wget.c
>> index 14f27d42998..c22843ee10d 100644
>> --- a/net/lwip/wget.c
>> +++ b/net/lwip/wget.c
>> @@ -285,9 +285,68 @@ static err_t httpc_headers_done_cb(httpc_state_t *connection, void *arg, struct
>>       return ERR_OK;
>>   }
>>
>> +#if CONFIG_IS_ENABLED(WGET_HTTPS)
>> +enum auth_mode {
>> +    AUTH_NONE,
>> +    AUTH_OPTIONAL,
>> +    AUTH_REQUIRED,
>> +};
>> +
>> +static char *cacert;
>> +static size_t cacert_size;
>> +static enum auth_mode cacert_auth_mode = AUTH_OPTIONAL;
>> +#endif
>> +
>> +#if CONFIG_IS_ENABLED(WGET_CACERT)
>> +static int set_auth(enum auth_mode auth)
>> +{
>> +    cacert_auth_mode = auth;
>> +
>> +    return CMD_RET_SUCCESS;
>> +}
>> +
>> +static int set_cacert(char * const saddr, char * const ssz)
>> +{
>> +    mbedtls_x509_crt crt;
>> +    ulong addr, sz;
>> +    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)
>> +        return CMD_RET_FAILURE;
>> +    cacert_size = sz;
>> +
>> +    memcpy(cacert, (void *)addr, sz);
>> +
>> +    mbedtls_x509_crt_init(&crt);
>> +    ret = mbedtls_x509_crt_parse(&crt, cacert, cacert_size);
>> +    if (ret) {
>> +        printf("Could not parse certificates (%d)\n", ret);
>> +        free(cacert);
>> +        cacert = NULL;
>> +        cacert_size = 0;
>> +        return CMD_RET_FAILURE;
>> +    }
>> +
>> +    return CMD_RET_SUCCESS;
>> +}
>> +#endif
>> +
>>   static int wget_loop(struct udevice *udev, ulong dst_addr, char *uri)
>>   {
>> -#if defined CONFIG_WGET_HTTPS
>> +#if CONFIG_IS_ENABLED(WGET_HTTPS)
>>       altcp_allocator_t tls_allocator;
>>   #endif
>>       httpc_connection_t conn;
>> @@ -312,11 +371,34 @@ static int wget_loop(struct udevice *udev, ulong dst_addr, char *uri)
>>           return -1;
>>
>>       memset(&conn, 0, sizeof(conn));
>> -#if defined CONFIG_WGET_HTTPS
>> +#if CONFIG_IS_ENABLED(WGET_HTTPS)
>>       if (is_https) {
>> +        char *ca = cacert;
>> +        size_t ca_sz = cacert_size;
>> +
>> +        if (cacert_auth_mode == AUTH_REQUIRED) {
>> +            if (!ca || !ca_sz) {
>> +                printf("Error: cacert authentication mode is "
>> +                       "'required' but no CA certificates "
>> +                       "given\n");
>> +                return CMD_RET_FAILURE;
>> +               }
>> +        } else if (cacert_auth_mode == AUTH_NONE) {
>> +            ca = NULL;
>> +            ca_sz = 0;
>> +        } else if (cacert_auth_mode == AUTH_OPTIONAL) {
>> +            /*
>> +             * Nothing to do, this is the default behavior of
>> +             * altcp_tls to check server certificates against CA
>> +             * certificates when the latter are provided and proceed
>> +             * with no verification if not.
>> +             */
>> +        }
>> +
>>           tls_allocator.alloc = &altcp_tls_alloc;
>>           tls_allocator.arg =
>> -            altcp_tls_create_config_client(NULL, 0, ctx.server_name);
>> +            altcp_tls_create_config_client(ca, ca_sz,
>> +                               ctx.server_name);
>>
>>           if (!tls_allocator.arg) {
>>               log_err("error: Cannot create a TLS connection\n");
>> @@ -369,6 +451,20 @@ int do_wget(struct cmd_tbl *cmdtp, int flag, int argc, char * const argv[])
>>       ulong dst_addr;
>>       char nurl[1024];
>>
>> +#if CONFIG_IS_ENABLED(WGET_CACERT)
>> +    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 (!strncmp(argv[2], "none", strlen("none")))
>> +            return set_auth(AUTH_NONE);
>> +        if (!strncmp(argv[2], "optional", strlen("optional")))
>> +            return set_auth(AUTH_OPTIONAL);
>> +        if (!strncmp(argv[2], "required", strlen("required")))
>> +            return set_auth(AUTH_REQUIRED);
>> +        return CMD_RET_USAGE;
>> +    }
>> +#endif
>> +
>>       if (argc < 2 || argc > 3)
>>           return CMD_RET_USAGE;
>>
> 

Re: [PATCH v2 1/6] net: lwip: extend wget to support CA (root) certificates

[Ilias Apalodimas]

Hi Jerome, Heinrich

On Wed, 5 Mar 2025 at 17:13, Jerome Forissier
<jerome.forissier@linaro.org> wrote:
>
> Hi Heinrich,
>
> On 3/5/25 16:07, Heinrich Schuchardt wrote:
> > On 05.03.25 15:26, Jerome Forissier wrote:
> >> Add the "cacert" (Certification Authority certificates) subcommand to
> >> wget to pass root certificates to the code handling the HTTPS protocol.
> >> The subcommand is enabled by the WGET_CACERT Kconfig symbol.
> >>
> >> Usage example:
> >>
> >>   => dhcp
> >>   # Download some root certificates (note: not authenticated!)
> >>   => wget https://cacerts.digicert.com/DigiCertTLSECCP384RootG5.crt
> >>   # Provide root certificates
> >>   => wget cacert $fileaddr $filesize
> >>   # Enforce verification (it is optional by default)
> >>   => wget cacert required
> >>   # Forget the root certificates
> >>   => wget cacert 0 0
> >>   # Disable verification
> >>   => wget cacert none
> >>
> >> Signed-off-by: Jerome Forissier <jerome.forissier@linaro.org>
> >> ---
> >>   cmd/Kconfig     |   8 ++++
> >>   cmd/net-lwip.c  |  17 ++++++--
> >>   net/lwip/wget.c | 102 ++++++++++++++++++++++++++++++++++++++++++++++--
> >>   3 files changed, 121 insertions(+), 6 deletions(-)
> >>
> >> diff --git a/cmd/Kconfig b/cmd/Kconfig
> >> index 8dd42571abc..d469217c0ea 100644
> >> --- a/cmd/Kconfig
> >> +++ b/cmd/Kconfig
> >> @@ -2177,6 +2177,14 @@ config WGET_HTTPS
> >>       help
> >>         Enable TLS over http for wget.
> >>
> >> +config WGET_CACERT
> >> +    bool "wget cacert"
> >> +    depends on CMD_WGET
> >> +    depends on WGET_HTTPS
> >> +    help
> >> +      Adds the "cacert" sub-command to wget to provide root certificates
> >> +      to the HTTPS engine. Must be in DER format.
> >> +
> >
> > Shouldn't we build CA certs into U-Boot?
> > Downloading certs from unsafe media is not a good replacement.
>
> That's the purpose of patch 4/6 [1]. Although downloading may still be a
> valid option when used with hash verification as I mentioned in a reply to
> Ilias in v1 [2].
>

FWIW I think this still makes sense for peopke that don't want or can
not add the cert in the u-boot binary, but can add a signed script to
download it on the fly

Reviewed-by: Ilias Apalodimas <ilias.apalodimas@linaro.org>

Re: [PATCH v2 1/6] net: lwip: extend wget to support CA (root) certificates

[Ilias Apalodimas]

On Sun, 9 Mar 2025 at 12:58, Ilias Apalodimas
<ilias.apalodimas@linaro.org> wrote:
>
> Hi Jerome, Heinrich
>
> On Wed, 5 Mar 2025 at 17:13, Jerome Forissier
> <jerome.forissier@linaro.org> wrote:
> >
> > Hi Heinrich,
> >
> > On 3/5/25 16:07, Heinrich Schuchardt wrote:
> > > On 05.03.25 15:26, Jerome Forissier wrote:
> > >> Add the "cacert" (Certification Authority certificates) subcommand to
> > >> wget to pass root certificates to the code handling the HTTPS protocol.
> > >> The subcommand is enabled by the WGET_CACERT Kconfig symbol.
> > >>
> > >> Usage example:
> > >>
> > >>   => dhcp
> > >>   # Download some root certificates (note: not authenticated!)
> > >>   => wget https://cacerts.digicert.com/DigiCertTLSECCP384RootG5.crt
> > >>   # Provide root certificates
> > >>   => wget cacert $fileaddr $filesize
> > >>   # Enforce verification (it is optional by default)
> > >>   => wget cacert required
> > >>   # Forget the root certificates
> > >>   => wget cacert 0 0
> > >>   # Disable verification
> > >>   => wget cacert none
> > >>
> > >> Signed-off-by: Jerome Forissier <jerome.forissier@linaro.org>
> > >> ---
> > >>   cmd/Kconfig     |   8 ++++
> > >>   cmd/net-lwip.c  |  17 ++++++--
> > >>   net/lwip/wget.c | 102 ++++++++++++++++++++++++++++++++++++++++++++++--
> > >>   3 files changed, 121 insertions(+), 6 deletions(-)
> > >>
> > >> diff --git a/cmd/Kconfig b/cmd/Kconfig
> > >> index 8dd42571abc..d469217c0ea 100644
> > >> --- a/cmd/Kconfig
> > >> +++ b/cmd/Kconfig
> > >> @@ -2177,6 +2177,14 @@ config WGET_HTTPS
> > >>       help
> > >>         Enable TLS over http for wget.
> > >>
> > >> +config WGET_CACERT
> > >> +    bool "wget cacert"
> > >> +    depends on CMD_WGET
> > >> +    depends on WGET_HTTPS
> > >> +    help
> > >> +      Adds the "cacert" sub-command to wget to provide root certificates
> > >> +      to the HTTPS engine. Must be in DER format.
> > >> +
> > >
> > > Shouldn't we build CA certs into U-Boot?
> > > Downloading certs from unsafe media is not a good replacement.
> >
> > That's the purpose of patch 4/6 [1]. Although downloading may still be a
> > valid option when used with hash verification as I mentioned in a reply to
> > Ilias in v1 [2].
> >
>
> FWIW I think this still makes sense for peopke that don't want or can
> not add the cert in the u-boot binary, but can add a signed script to
> download it on the fly
>
> Reviewed-by: Ilias Apalodimas <ilias.apalodimas@linaro.org>

This still stands, but there are a few warning/errors on the entire
patchset. Can you address them and send a v3?

Thanks
/Ilias

Re: [PATCH v2 4/6] net: lwip: add support for built-in root certificates

[Ilias Apalodimas]

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

Re: [PATCH v2 5/6] doc: cmd: wget: document cacert subcommand

[Ilias Apalodimas]

On Wed, 5 Mar 2025 at 16:27, Jerome Forissier
<jerome.forissier@linaro.org> wrote:
>
> Document the 'wget cacert' subcommand which allows to configure root
> (CA) certificates for HTTPS.
>
> Signed-off-by: Jerome Forissier <jerome.forissier@linaro.org>
> ---
>  doc/usage/cmd/wget.rst | 82 ++++++++++++++++++++++++++++++++++++++++--
>  1 file changed, 80 insertions(+), 2 deletions(-)
>
> diff --git a/doc/usage/cmd/wget.rst b/doc/usage/cmd/wget.rst
> index 48bedf1e845..cc82e495a29 100644
> --- a/doc/usage/cmd/wget.rst
> +++ b/doc/usage/cmd/wget.rst
> @@ -12,7 +12,9 @@ Synopsis
>  ::
>
>      wget [address] [host:]path
> -    wget [address] url          # lwIP only
> +    wget [address] url                  # lwIP only
> +    wget cacert none|optional|required  # lwIP only
> +    wget cacert <address> <size>        # lwIP only
>
>
>  Description
> @@ -54,6 +56,32 @@ address
>  url
>      HTTP or HTTPS URL, that is: http[s]://<host>[:<port>]/<path>.
>
> +The cacert (stands for 'Certification Authority certificates') subcommand is
> +used to provide root certificates for the purpose of HTTPS authentication. It
> +also allows to enable or disable authentication.
> +
> +wget cacert <address> <size>
> +
> +address
> +    memory address of the root certificates in X509 DER format
> +
> +size
> +    the size of the root certificates
> +
> +wget cacert none|optional|required
> +
> +none
> +    certificate verification is disabled. HTTPS is used without any server
> +    authentication (unsafe)
> +optional
> +    certificate verification is enabled provided root certificates have been
> +    provided via wget cacert <addr> <size> or wget cacert builtin. Otherwise
> +    HTTPS is used without any server authentication (unsafe).
> +required
> +    certificate verification is mandatory. If no root certificates have been
> +    configured, HTTPS transfers will fail.
> +
> +
>  Examples
>  --------
>
> @@ -97,11 +125,61 @@ In the example the following steps are executed:
>     1694892032 bytes transferred in 492181 ms (3.3 MiB/s)
>     Bytes transferred = 1694892032 (65060000 hex)
>
> +Here is an example showing how to configure built-in root certificates as
> +well as providing some at run time. In this example it is assumed that
> +CONFIG_WGET_BUILTIN_CACERT_PATH=DigiCertTLSRSA4096RootG5.crt downloaded from
> +https://cacerts.digicert.com/DigiCertTLSRSA4096RootG5.crt.
> +
> +::
> +
> +   # Make sure IP is configured
> +   => dhcp
> +   # When built-in certificates are configured, authentication is mandatory
> +   # (i.e., "wget cacert required"). Use a test server...
> +   => wget https://digicert-tls-rsa4096-root-g5.chain-demos.digicert.com/
> +   1864 bytes transferred in 1 ms (1.8 MiB/s)
> +   Bytes transferred = 1864 (748 hex)
> +   # Another server not signed against Digicert will fail
> +   => wget https://www.google.com/
> +   Certificate verification failed
> +
> +   HTTP client error 4
> +   # Disable authentication to allow the command to proceed anyways
> +   => wget cacert none
> +   => wget https://www.google.com/
> +   WARNING: no CA certificates, HTTPS connections not authenticated
> +   16683 bytes transferred in 15 ms (1.1 MiB/s)
> +   Bytes transferred = 16683 (412b hex)
> +   # Force verification but unregister the CA certificates
> +   => wget cacert required
> +   => wget cacert 0 0
> +   # Unsurprisingly, download fails
> +   => wget https://digicert-tls-rsa4096-root-g5.chain-demos.digicert.com/
> +   Error: cacert authentication mode is 'required' but no CA certificates given
> +   # Get the same certificates as above from the network
> +   => wget cacert none
> +   => wget https://cacerts.digicert.com/DigiCertTLSRSA4096RootG5.crt
> +   WARNING: no CA certificates, HTTPS connections not authenticated
> +   1386 bytes transferred in 1 ms (1.3 MiB/s)
> +   Bytes transferred = 1386 (56a hex)
> +   # Register them and force authentication
> +   => wget cacert $fileaddr $filesize
> +   => wget cacert required
> +   # Authentication is operational again
> +   => wget https://digicert-tls-rsa4096-root-g5.chain-demos.digicert.com/
> +   1864 bytes transferred in 1 ms (1.8 MiB/s)
> +   Bytes transferred = 1864 (748 hex)
> +   # The builtin certificates can be restored at any time
> +   => wget cacert builtin
> +
>  Configuration
>  -------------
>
>  The command is only available if CONFIG_CMD_WGET=y.
> -To enable lwIP support set CONFIG_NET_LWIP=y.
> +To enable lwIP support set CONFIG_NET_LWIP=y. In this case, root certificates
> +support can be enabled via CONFIG_WGET_BUILTIN_CACERT=y
> +CONFIG_WGET_BUILTIN_CACERT_PATH=<some path> (for built-in certificates) and/or
> +CONFIG_WGET_CACERT=y (for the wget cacert command).
>
>  TCP Selective Acknowledgments in the legacy network stack can be enabled via
>  CONFIG_PROT_TCP_SACK=y. This will improve the download speed. Selective
> --
> 2.43.0
>
Acked-by: Ilias Apalodimas <ilias.apalodimas@linaro.org>

Re: [PATCH v2 4/6] net: lwip: add support for built-in root certificates

[Jerome Forissier]

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,
-- 
Jerome

> 
> [...]
> 
> Cheers
> /Ilias

Re: [PATCH v2 1/6] net: lwip: extend wget to support CA (root) certificates

[Jerome Forissier]

On 3/9/25 12:00, Ilias Apalodimas wrote:
> On Sun, 9 Mar 2025 at 12:58, Ilias Apalodimas
> <ilias.apalodimas@linaro.org> wrote:
>>
>> Hi Jerome, Heinrich
>>
>> On Wed, 5 Mar 2025 at 17:13, Jerome Forissier
>> <jerome.forissier@linaro.org> wrote:
>>>
>>> Hi Heinrich,
>>>
>>> On 3/5/25 16:07, Heinrich Schuchardt wrote:
>>>> On 05.03.25 15:26, Jerome Forissier wrote:
>>>>> Add the "cacert" (Certification Authority certificates) subcommand to
>>>>> wget to pass root certificates to the code handling the HTTPS protocol.
>>>>> The subcommand is enabled by the WGET_CACERT Kconfig symbol.
>>>>>
>>>>> Usage example:
>>>>>
>>>>>   => dhcp
>>>>>   # Download some root certificates (note: not authenticated!)
>>>>>   => wget https://cacerts.digicert.com/DigiCertTLSECCP384RootG5.crt
>>>>>   # Provide root certificates
>>>>>   => wget cacert $fileaddr $filesize
>>>>>   # Enforce verification (it is optional by default)
>>>>>   => wget cacert required
>>>>>   # Forget the root certificates
>>>>>   => wget cacert 0 0
>>>>>   # Disable verification
>>>>>   => wget cacert none
>>>>>
>>>>> Signed-off-by: Jerome Forissier <jerome.forissier@linaro.org>
>>>>> ---
>>>>>   cmd/Kconfig     |   8 ++++
>>>>>   cmd/net-lwip.c  |  17 ++++++--
>>>>>   net/lwip/wget.c | 102 ++++++++++++++++++++++++++++++++++++++++++++++--
>>>>>   3 files changed, 121 insertions(+), 6 deletions(-)
>>>>>
>>>>> diff --git a/cmd/Kconfig b/cmd/Kconfig
>>>>> index 8dd42571abc..d469217c0ea 100644
>>>>> --- a/cmd/Kconfig
>>>>> +++ b/cmd/Kconfig
>>>>> @@ -2177,6 +2177,14 @@ config WGET_HTTPS
>>>>>       help
>>>>>         Enable TLS over http for wget.
>>>>>
>>>>> +config WGET_CACERT
>>>>> +    bool "wget cacert"
>>>>> +    depends on CMD_WGET
>>>>> +    depends on WGET_HTTPS
>>>>> +    help
>>>>> +      Adds the "cacert" sub-command to wget to provide root certificates
>>>>> +      to the HTTPS engine. Must be in DER format.
>>>>> +
>>>>
>>>> Shouldn't we build CA certs into U-Boot?
>>>> Downloading certs from unsafe media is not a good replacement.
>>>
>>> That's the purpose of patch 4/6 [1]. Although downloading may still be a
>>> valid option when used with hash verification as I mentioned in a reply to
>>> Ilias in v1 [2].
>>>
>>
>> FWIW I think this still makes sense for peopke that don't want or can
>> not add the cert in the u-boot binary, but can add a signed script to
>> download it on the fly
>>
>> Reviewed-by: Ilias Apalodimas <ilias.apalodimas@linaro.org>
> 
> This still stands, but there are a few warning/errors on the entire
> patchset. Can you address them and send a v3?

Which warnings/errors? Which config? I see none with qemu_arm64_lwip_defconfig.

Thanks,
-- 
Jerome

> 
> Thanks
> /Ilias

Re: [PATCH v2 1/6] net: lwip: extend wget to support CA (root) certificates

[Ilias Apalodimas]

[...]

> >>>
> >>
> >> FWIW I think this still makes sense for peopke that don't want or can
> >> not add the cert in the u-boot binary, but can add a signed script to
> >> download it on the fly
> >>
> >> Reviewed-by: Ilias Apalodimas <ilias.apalodimas@linaro.org>
> >
> > This still stands, but there are a few warning/errors on the entire
> > patchset. Can you address them and send a v3?
>
> Which warnings/errors? Which config? I see none with qemu_arm64_lwip_defconfig.

Ah my bad, it's checkpatch that has all that

Cheers
/Ilias
>
> Thanks,
> --
> Jerome
>
> >
> > Thanks
> > /Ilias

Re: [PATCH v2 4/6] net: lwip: add support for built-in root certificates

[Ilias Apalodimas]

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

Re: [PATCH v2 1/6] net: lwip: extend wget to support CA (root) certificates

[Jerome Forissier]

On 3/10/25 12:49, Ilias Apalodimas wrote:
> [...]
> 
>>>>>
>>>>
>>>> FWIW I think this still makes sense for peopke that don't want or can
>>>> not add the cert in the u-boot binary, but can add a signed script to
>>>> download it on the fly
>>>>
>>>> Reviewed-by: Ilias Apalodimas <ilias.apalodimas@linaro.org>
>>>
>>> This still stands, but there are a few warning/errors on the entire
>>> patchset. Can you address them and send a v3?
>>
>> Which warnings/errors? Which config? I see none with qemu_arm64_lwip_defconfig.
> 
> Ah my bad, it's checkpatch that has all that

Yes, I think they're all false positives, or rather things we want to ignore
for consistency with existing code.

Thanks,
-- 
Jerome

> 
> Cheers
> /Ilias
>>
>> Thanks,
>> --
>> Jerome
>>
>>>
>>> Thanks
>>> /Ilias

Re: [PATCH v2 4/6] net: lwip: add support for built-in root certificates

[Jerome Forissier]

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,
-- 
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

Re: [PATCH v2 4/6] net: lwip: add support for built-in root certificates

[Ilias Apalodimas]

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

Re: [PATCH v2 4/6] net: lwip: add support for built-in root certificates

[Jerome Forissier]

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,
-- 
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

Re: [PATCH v2 4/6] net: lwip: add support for built-in root certificates

[Ilias Apalodimas]

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

Re: [PATCH v2 4/6] net: lwip: add support for built-in root certificates

[Jerome Forissier]

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,
-- 
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

Re: [PATCH v2 4/6] net: lwip: add support for built-in root certificates

[Ilias Apalodimas]

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