diff mbox series

[v2,4/6] net: lwip: Enable https:// support for wget

Message ID 20241024112449.1362319-5-ilias.apalodimas@linaro.org
State Superseded
Headers show
Series Enable https for wget | expand

Commit Message

Ilias Apalodimas Oct. 24, 2024, 11:24 a.m. UTC 
With the recent changes of lwip & mbedTLS we can now download from
https:// urls instead of just http://.
Adjust our wget lwip version parsing to support both URLs.
While at it adjust the default TCP window for QEMU since https seems to
require at least 16384

Signed-off-by: Ilias Apalodimas <ilias.apalodimas@linaro.org>
---
 cmd/Kconfig      | 19 ++++++++++++
 net/lwip/Kconfig |  2 +-
 net/lwip/wget.c  | 78 +++++++++++++++++++++++++++++++++++++++++++-----
 3 files changed, 91 insertions(+), 8 deletions(-)

Comments

Ilias Apalodimas Oct. 24, 2024, 2:04 p.m. UTC | #1 
Replying to myself here but

On Thu, 24 Oct 2024 at 14:25, Ilias Apalodimas
<ilias.apalodimas@linaro.org> wrote:
>
> With the recent changes of lwip & mbedTLS we can now download from
> https:// urls instead of just http://.
> Adjust our wget lwip version parsing to support both URLs.
> While at it adjust the default TCP window for QEMU since https seems to
> require at least 16384
>
> Signed-off-by: Ilias Apalodimas <ilias.apalodimas@linaro.org>
> ---
>  cmd/Kconfig      | 19 ++++++++++++
>  net/lwip/Kconfig |  2 +-
>  net/lwip/wget.c  | 78 +++++++++++++++++++++++++++++++++++++++++++-----
>  3 files changed, 91 insertions(+), 8 deletions(-)
>
> diff --git a/cmd/Kconfig b/cmd/Kconfig
> index 3ee70f31b142..1d90a884e2c1 100644
> --- a/cmd/Kconfig
> +++ b/cmd/Kconfig
> @@ -2126,6 +2126,25 @@ config CMD_WGET
>           wget is a simple command to download kernel, or other files,
>           from a http server over TCP.
>
> +config WGET_HTTPS
> +       bool "wget https"
> +       depends on CMD_WGET
> +       depends on PROT_TCP_LWIP
> +       depends on MBEDTLS_LIB
> +        select SHA256
> +        select RSA
> +        select ASYMMETRIC_KEY_TYPE
> +        select ASYMMETRIC_PUBLIC_KEY_SUBTYPE
> +        select X509_CERTIFICATE_PARSER
> +        select PKCS7_MESSAGE_PARSER
> +       select MBEDTLS_LIB_CRYPTO
> +       select MBEDTLS_LIB_TLS
> +       select RSA_VERIFY_WITH_PKEY
> +       select X509_CERTIFICATE_PARSER
> +       select PKCS7_MESSAGE_PARSER
> +       help
> +         Enable TLS over http for wget.
> +
>  endif  # if CMD_NET
>
>  config CMD_PXE
> diff --git a/net/lwip/Kconfig b/net/lwip/Kconfig
> index 8a67de4cf335..a9ae9bf7fa2a 100644
> --- a/net/lwip/Kconfig
> +++ b/net/lwip/Kconfig
> @@ -37,7 +37,7 @@ config PROT_UDP_LWIP
>
>  config LWIP_TCP_WND
>         int "Value of TCP_WND"
> -       default 8000 if ARCH_QEMU
> +       default 32768 if ARCH_QEMU
>         default 3000000
>         help
>           Default value for TCP_WND in the lwIP configuration
> diff --git a/net/lwip/wget.c b/net/lwip/wget.c
> index b495ebd1aa96..1a2ecdcddf34 100644
> --- a/net/lwip/wget.c
> +++ b/net/lwip/wget.c
> @@ -7,13 +7,17 @@
>  #include <efi_loader.h>
>  #include <image.h>
>  #include <lwip/apps/http_client.h>
> +#include "lwip/altcp_tls.h"
>  #include <lwip/timeouts.h>
> +#include <rng.h>
>  #include <mapmem.h>
>  #include <net.h>
>  #include <time.h>
> +#include <dm/uclass.h>
>
>  #define SERVER_NAME_SIZE 200
>  #define HTTP_PORT_DEFAULT 80
> +#define HTTPS_PORT_DEFAULT 443
>  #define PROGRESS_PRINT_STEP_BYTES (100 * 1024)
>
>  enum done_state {
> @@ -32,18 +36,53 @@ struct wget_ctx {
>         enum done_state done;
>  };
>
> +bool wget_validate_uri(char *uri);
> +
> +int mbedtls_hardware_poll(void *data, unsigned char *output, size_t len,
> +                         size_t *olen)
> +{
> +       struct udevice *dev;
> +       u64 rng = 0;
> +       int ret;
> +
> +       *olen = 0;
> +
> +       ret = uclass_get_device(UCLASS_RNG, 0, &dev);
> +       if (ret) {
> +               log_err("Failed to get an rng: %d\n", ret);
> +               return ret;
> +       }
> +       ret = dm_rng_read(dev, &rng, sizeof(rng));
> +       if (ret)
> +               return ret;
> +
> +       memcpy(output, &rng, len);
> +       *olen = sizeof(rng);

I just realized I forgot to address this. I'll fix it on v3, but the
rest of the code is ok to review

Thanks
/Ilias
> +
> +       return 0;
> +}
> +
>  static int parse_url(char *url, char *host, u16 *port, char **path)
>  {
>         char *p, *pp;
>         long lport;
> +       size_t prefix_len = 0;
> +
> +       if (!wget_validate_uri(url)) {
> +               log_err("Invalid URL. Use http(s)://\n");
> +               return -EINVAL;
> +       }
>
> +       *port = HTTP_PORT_DEFAULT;
> +       prefix_len = strlen("http://");
>         p = strstr(url, "http://");
>         if (!p) {
> -               log_err("only http:// is supported\n");
> -               return -EINVAL;
> +               p = strstr(url, "https://");
> +               prefix_len = strlen("https://");
> +               *port = HTTPS_PORT_DEFAULT;
>         }
>
> -       p += strlen("http://");
> +       p += prefix_len;
>
>         /* Parse hostname */
>         pp = strchr(p, ':');
> @@ -67,9 +106,8 @@ static int parse_url(char *url, char *host, u16 *port, char **path)
>                 if (lport > 65535)
>                         return -EINVAL;
>                 *port = (u16)lport;
> -       } else {
> -               *port = HTTP_PORT_DEFAULT;
>         }
> +
>         if (*pp != '/')
>                 return -EINVAL;
>         *path = pp;
> @@ -210,6 +248,9 @@ static void httpc_result_cb(void *arg, httpc_result_t httpc_result,
>  static int wget_loop(struct udevice *udev, ulong dst_addr, char *uri)
>  {
>         char server_name[SERVER_NAME_SIZE];
> +#if defined CONFIG_WGET_HTTPS
> +       altcp_allocator_t tls_allocator;
> +#endif
>         httpc_connection_t conn;
>         httpc_state_t *state;
>         struct netif *netif;
> @@ -232,6 +273,22 @@ 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 (port == HTTPS_PORT_DEFAULT) {
> +               tls_allocator.alloc = &altcp_tls_alloc;
> +               tls_allocator.arg =
> +                       altcp_tls_create_config_client(NULL, 0, server_name);
> +
> +               if (!tls_allocator.arg) {
> +                       log_err("error: Cannot create a TLS connection\n");
> +                       net_lwip_remove_netif(netif);
> +                       return -1;
> +               }
> +
> +               conn.altcp_allocator = &tls_allocator;
> +       }
> +#endif
> +
>         conn.result_fn = httpc_result_cb;
>         ctx.path = path;
>         if (httpc_get_file_dns(server_name, port, path, &conn, httpc_recv_cb,
> @@ -316,6 +373,7 @@ bool wget_validate_uri(char *uri)
>         char c;
>         bool ret = true;
>         char *str_copy, *s, *authority;
> +       size_t prefix_len = 0;
>
>         for (c = 0x1; c < 0x21; c++) {
>                 if (strchr(uri, c)) {
> @@ -323,15 +381,21 @@ bool wget_validate_uri(char *uri)
>                         return false;
>                 }
>         }
> +
>         if (strchr(uri, 0x7f)) {
>                 log_err("invalid character is used\n");
>                 return false;
>         }
>
> -       if (strncmp(uri, "http://", 7)) {
> -               log_err("only http:// is supported\n");
> +       if (!strncmp(uri, "http://", strlen("http://"))) {
> +               prefix_len = strlen("http://");
> +       } else if (!strncmp(uri, "https://", strlen("https://"))) {
> +               prefix_len = strlen("https://");
> +       } else {
> +               log_err("only http(s):// is supported\n");
>                 return false;
>         }
> +
>         str_copy = strdup(uri);
>         if (!str_copy)
>                 return false;
> --
> 2.45.2
>
Ilias Apalodimas Oct. 24, 2024, 2:38 p.m. UTC | #2 
On Thu, 24 Oct 2024 at 17:04, Ilias Apalodimas
<ilias.apalodimas@linaro.org> wrote:
>
> Replying to myself here but
>
> On Thu, 24 Oct 2024 at 14:25, Ilias Apalodimas
> <ilias.apalodimas@linaro.org> wrote:
> >
> > With the recent changes of lwip & mbedTLS we can now download from
> > https:// urls instead of just http://.
> > Adjust our wget lwip version parsing to support both URLs.
> > While at it adjust the default TCP window for QEMU since https seems to
> > require at least 16384
> >
> > Signed-off-by: Ilias Apalodimas <ilias.apalodimas@linaro.org>
> > ---
> >  cmd/Kconfig      | 19 ++++++++++++
> >  net/lwip/Kconfig |  2 +-
> >  net/lwip/wget.c  | 78 +++++++++++++++++++++++++++++++++++++++++++-----
> >  3 files changed, 91 insertions(+), 8 deletions(-)
> >
> > diff --git a/cmd/Kconfig b/cmd/Kconfig
> > index 3ee70f31b142..1d90a884e2c1 100644
> > --- a/cmd/Kconfig
> > +++ b/cmd/Kconfig
> > @@ -2126,6 +2126,25 @@ config CMD_WGET
> >           wget is a simple command to download kernel, or other files,
> >           from a http server over TCP.
> >
> > +config WGET_HTTPS
> > +       bool "wget https"
> > +       depends on CMD_WGET
> > +       depends on PROT_TCP_LWIP
> > +       depends on MBEDTLS_LIB
> > +        select SHA256
> > +        select RSA
> > +        select ASYMMETRIC_KEY_TYPE
> > +        select ASYMMETRIC_PUBLIC_KEY_SUBTYPE
> > +        select X509_CERTIFICATE_PARSER
> > +        select PKCS7_MESSAGE_PARSER
> > +       select MBEDTLS_LIB_CRYPTO
> > +       select MBEDTLS_LIB_TLS
> > +       select RSA_VERIFY_WITH_PKEY
> > +       select X509_CERTIFICATE_PARSER
> > +       select PKCS7_MESSAGE_PARSER
> > +       help
> > +         Enable TLS over http for wget.
> > +
> >  endif  # if CMD_NET
> >
> >  config CMD_PXE
> > diff --git a/net/lwip/Kconfig b/net/lwip/Kconfig
> > index 8a67de4cf335..a9ae9bf7fa2a 100644
> > --- a/net/lwip/Kconfig
> > +++ b/net/lwip/Kconfig
> > @@ -37,7 +37,7 @@ config PROT_UDP_LWIP
> >
> >  config LWIP_TCP_WND
> >         int "Value of TCP_WND"
> > -       default 8000 if ARCH_QEMU
> > +       default 32768 if ARCH_QEMU
> >         default 3000000
> >         help
> >           Default value for TCP_WND in the lwIP configuration
> > diff --git a/net/lwip/wget.c b/net/lwip/wget.c
> > index b495ebd1aa96..1a2ecdcddf34 100644
> > --- a/net/lwip/wget.c
> > +++ b/net/lwip/wget.c
> > @@ -7,13 +7,17 @@
> >  #include <efi_loader.h>
> >  #include <image.h>
> >  #include <lwip/apps/http_client.h>
> > +#include "lwip/altcp_tls.h"
> >  #include <lwip/timeouts.h>
> > +#include <rng.h>
> >  #include <mapmem.h>
> >  #include <net.h>
> >  #include <time.h>
> > +#include <dm/uclass.h>
> >
> >  #define SERVER_NAME_SIZE 200
> >  #define HTTP_PORT_DEFAULT 80
> > +#define HTTPS_PORT_DEFAULT 443
> >  #define PROGRESS_PRINT_STEP_BYTES (100 * 1024)
> >
> >  enum done_state {
> > @@ -32,18 +36,53 @@ struct wget_ctx {
> >         enum done_state done;
> >  };
> >
> > +bool wget_validate_uri(char *uri);
> > +
> > +int mbedtls_hardware_poll(void *data, unsigned char *output, size_t len,
> > +                         size_t *olen)
> > +{
> > +       struct udevice *dev;
> > +       u64 rng = 0;
> > +       int ret;
> > +
> > +       *olen = 0;
> > +
> > +       ret = uclass_get_device(UCLASS_RNG, 0, &dev);
> > +       if (ret) {
> > +               log_err("Failed to get an rng: %d\n", ret);
> > +               return ret;
> > +       }
> > +       ret = dm_rng_read(dev, &rng, sizeof(rng));
> > +       if (ret)
> > +               return ret;
> > +
> > +       memcpy(output, &rng, len);
> > +       *olen = sizeof(rng);
>
> I just realized I forgot to address this. I'll fix it on v3, but the
> rest of the code is ok to review

So, the correct thing to do here is
memcpy(output, &rng, len);
*olen = LWIP_MIN(len, sizeof(rng)); <-- mbedTLS sets len to 128, so
although sizeof(rng); is ok for now we can't rely on that


>
> Thanks
> /Ilias
> > +
> > +       return 0;
> > +}
> > +
> >  static int parse_url(char *url, char *host, u16 *port, char **path)
> >  {
> >         char *p, *pp;
> >         long lport;
> > +       size_t prefix_len = 0;
> > +
> > +       if (!wget_validate_uri(url)) {
> > +               log_err("Invalid URL. Use http(s)://\n");
> > +               return -EINVAL;
> > +       }
> >
> > +       *port = HTTP_PORT_DEFAULT;
> > +       prefix_len = strlen("http://");
> >         p = strstr(url, "http://");
> >         if (!p) {
> > -               log_err("only http:// is supported\n");
> > -               return -EINVAL;
> > +               p = strstr(url, "https://");
> > +               prefix_len = strlen("https://");
> > +               *port = HTTPS_PORT_DEFAULT;
> >         }
> >
> > -       p += strlen("http://");
> > +       p += prefix_len;
> >
> >         /* Parse hostname */
> >         pp = strchr(p, ':');
> > @@ -67,9 +106,8 @@ static int parse_url(char *url, char *host, u16 *port, char **path)
> >                 if (lport > 65535)
> >                         return -EINVAL;
> >                 *port = (u16)lport;
> > -       } else {
> > -               *port = HTTP_PORT_DEFAULT;
> >         }
> > +
> >         if (*pp != '/')
> >                 return -EINVAL;
> >         *path = pp;
> > @@ -210,6 +248,9 @@ static void httpc_result_cb(void *arg, httpc_result_t httpc_result,
> >  static int wget_loop(struct udevice *udev, ulong dst_addr, char *uri)
> >  {
> >         char server_name[SERVER_NAME_SIZE];
> > +#if defined CONFIG_WGET_HTTPS
> > +       altcp_allocator_t tls_allocator;
> > +#endif
> >         httpc_connection_t conn;
> >         httpc_state_t *state;
> >         struct netif *netif;
> > @@ -232,6 +273,22 @@ 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 (port == HTTPS_PORT_DEFAULT) {
> > +               tls_allocator.alloc = &altcp_tls_alloc;
> > +               tls_allocator.arg =
> > +                       altcp_tls_create_config_client(NULL, 0, server_name);
> > +
> > +               if (!tls_allocator.arg) {
> > +                       log_err("error: Cannot create a TLS connection\n");
> > +                       net_lwip_remove_netif(netif);
> > +                       return -1;
> > +               }
> > +
> > +               conn.altcp_allocator = &tls_allocator;
> > +       }
> > +#endif
> > +
> >         conn.result_fn = httpc_result_cb;
> >         ctx.path = path;
> >         if (httpc_get_file_dns(server_name, port, path, &conn, httpc_recv_cb,
> > @@ -316,6 +373,7 @@ bool wget_validate_uri(char *uri)
> >         char c;
> >         bool ret = true;
> >         char *str_copy, *s, *authority;
> > +       size_t prefix_len = 0;
> >
> >         for (c = 0x1; c < 0x21; c++) {
> >                 if (strchr(uri, c)) {
> > @@ -323,15 +381,21 @@ bool wget_validate_uri(char *uri)
> >                         return false;
> >                 }
> >         }
> > +
> >         if (strchr(uri, 0x7f)) {
> >                 log_err("invalid character is used\n");
> >                 return false;
> >         }
> >
> > -       if (strncmp(uri, "http://", 7)) {
> > -               log_err("only http:// is supported\n");
> > +       if (!strncmp(uri, "http://", strlen("http://"))) {
> > +               prefix_len = strlen("http://");
> > +       } else if (!strncmp(uri, "https://", strlen("https://"))) {
> > +               prefix_len = strlen("https://");
> > +       } else {
> > +               log_err("only http(s):// is supported\n");
> >                 return false;
> >         }
> > +
> >         str_copy = strdup(uri);
> >         if (!str_copy)
> >                 return false;
> > --
> > 2.45.2
> >
Jerome Forissier Nov. 6, 2024, 1:50 p.m. UTC | #3 
Hi Ilias,

On 10/24/24 12:24, Ilias Apalodimas wrote:
> With the recent changes of lwip & mbedTLS we can now download from
> https:// urls instead of just http://.
> Adjust our wget lwip version parsing to support both URLs.
> While at it adjust the default TCP window for QEMU since https seems to
> require at least 16384
> 
> Signed-off-by: Ilias Apalodimas <ilias.apalodimas@linaro.org>
> ---
>  cmd/Kconfig      | 19 ++++++++++++
>  net/lwip/Kconfig |  2 +-
>  net/lwip/wget.c  | 78 +++++++++++++++++++++++++++++++++++++++++++-----
>  3 files changed, 91 insertions(+), 8 deletions(-)
> 
> diff --git a/cmd/Kconfig b/cmd/Kconfig
> index 3ee70f31b142..1d90a884e2c1 100644
> --- a/cmd/Kconfig
> +++ b/cmd/Kconfig
> @@ -2126,6 +2126,25 @@ config CMD_WGET
>  	  wget is a simple command to download kernel, or other files,
>  	  from a http server over TCP.
>  
> +config WGET_HTTPS
> +	bool "wget https"
> +	depends on CMD_WGET
> +	depends on PROT_TCP_LWIP
> +	depends on MBEDTLS_LIB
> +        select SHA256
> +        select RSA
> +        select ASYMMETRIC_KEY_TYPE
> +        select ASYMMETRIC_PUBLIC_KEY_SUBTYPE
> +        select X509_CERTIFICATE_PARSER
> +        select PKCS7_MESSAGE_PARSER
> +	select MBEDTLS_LIB_CRYPTO
> +	select MBEDTLS_LIB_TLS
> +	select RSA_VERIFY_WITH_PKEY
> +	select X509_CERTIFICATE_PARSER
> +	select PKCS7_MESSAGE_PARSER
> +	help
> +	  Enable TLS over http for wget.

s/http/HTTP/
Maybe add "This adds support for https:// URLs"?

> +
>  endif  # if CMD_NET
>  
>  config CMD_PXE
> diff --git a/net/lwip/Kconfig b/net/lwip/Kconfig
> index 8a67de4cf335..a9ae9bf7fa2a 100644
> --- a/net/lwip/Kconfig
> +++ b/net/lwip/Kconfig
> @@ -37,7 +37,7 @@ config PROT_UDP_LWIP
>  
>  config LWIP_TCP_WND
>  	int "Value of TCP_WND"
> -	default 8000 if ARCH_QEMU
> +	default 32768 if ARCH_QEMU
>  	default 3000000
>  	help
>  	  Default value for TCP_WND in the lwIP configuration
> diff --git a/net/lwip/wget.c b/net/lwip/wget.c
> index b495ebd1aa96..1a2ecdcddf34 100644
> --- a/net/lwip/wget.c
> +++ b/net/lwip/wget.c
> @@ -7,13 +7,17 @@
>  #include <efi_loader.h>
>  #include <image.h>
>  #include <lwip/apps/http_client.h>
> +#include "lwip/altcp_tls.h"
>  #include <lwip/timeouts.h>
> +#include <rng.h>
>  #include <mapmem.h>
>  #include <net.h>
>  #include <time.h>
> +#include <dm/uclass.h>
>  
>  #define SERVER_NAME_SIZE 200
>  #define HTTP_PORT_DEFAULT 80
> +#define HTTPS_PORT_DEFAULT 443
>  #define PROGRESS_PRINT_STEP_BYTES (100 * 1024)
>  
>  enum done_state {
> @@ -32,18 +36,53 @@ struct wget_ctx {
>  	enum done_state done;
>  };
>  
> +bool wget_validate_uri(char *uri);
> +
> +int mbedtls_hardware_poll(void *data, unsigned char *output, size_t len,
> +			  size_t *olen)
> +{
> +	struct udevice *dev;
> +	u64 rng = 0;
> +	int ret;
> +
> +	*olen = 0;
> +
> +	ret = uclass_get_device(UCLASS_RNG, 0, &dev);
> +	if (ret) {
> +		log_err("Failed to get an rng: %d\n", ret);
> +		return ret;
> +	}
> +	ret = dm_rng_read(dev, &rng, sizeof(rng));
> +	if (ret)
> +		return ret;
> +
> +	memcpy(output, &rng, len);
> +	*olen = sizeof(rng);
> +
> +	return 0;
> +}
> +
>  static int parse_url(char *url, char *host, u16 *port, char **path)
>  {
>  	char *p, *pp;
>  	long lport;
> +	size_t prefix_len = 0;
> +
> +	if (!wget_validate_uri(url)) {
> +		log_err("Invalid URL. Use http(s)://\n");
> +		return -EINVAL;
> +	}
>  
> +	*port = HTTP_PORT_DEFAULT;
> +	prefix_len = strlen("http://");
>  	p = strstr(url, "http://");
>  	if (!p) {
> -		log_err("only http:// is supported\n");
> -		return -EINVAL;
> +		p = strstr(url, "https://");
> +		prefix_len = strlen("https://");
> +		*port = HTTPS_PORT_DEFAULT;
>  	}
>  
> -	p += strlen("http://");
> +	p += prefix_len;
>  
>  	/* Parse hostname */
>  	pp = strchr(p, ':');
> @@ -67,9 +106,8 @@ static int parse_url(char *url, char *host, u16 *port, char **path)
>  		if (lport > 65535)
>  			return -EINVAL;
>  		*port = (u16)lport;
> -	} else {
> -		*port = HTTP_PORT_DEFAULT;
>  	}
> +
>  	if (*pp != '/')
>  		return -EINVAL;
>  	*path = pp;
> @@ -210,6 +248,9 @@ static void httpc_result_cb(void *arg, httpc_result_t httpc_result,
>  static int wget_loop(struct udevice *udev, ulong dst_addr, char *uri)
>  {
>  	char server_name[SERVER_NAME_SIZE];
> +#if defined CONFIG_WGET_HTTPS
> +	altcp_allocator_t tls_allocator;
> +#endif
>  	httpc_connection_t conn;
>  	httpc_state_t *state;
>  	struct netif *netif;
> @@ -232,6 +273,22 @@ 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 (port == HTTPS_PORT_DEFAULT) {

Does this mean the port has to be 443 for HTTPS to be used? Or that
plain HTTP is not possible over port 443? That would be overly restrictive
IMO.

> +		tls_allocator.alloc = &altcp_tls_alloc;
> +		tls_allocator.arg =
> +			altcp_tls_create_config_client(NULL, 0, server_name);
> +
> +		if (!tls_allocator.arg) {
> +			log_err("error: Cannot create a TLS connection\n");
> +			net_lwip_remove_netif(netif);
> +			return -1;
> +		}
> +
> +		conn.altcp_allocator = &tls_allocator;
> +	}
> +#endif
> +
>  	conn.result_fn = httpc_result_cb;
>  	ctx.path = path;
>  	if (httpc_get_file_dns(server_name, port, path, &conn, httpc_recv_cb,
> @@ -316,6 +373,7 @@ bool wget_validate_uri(char *uri)
>  	char c;
>  	bool ret = true;
>  	char *str_copy, *s, *authority;
> +	size_t prefix_len = 0;
>  
>  	for (c = 0x1; c < 0x21; c++) {
>  		if (strchr(uri, c)) {
> @@ -323,15 +381,21 @@ bool wget_validate_uri(char *uri)
>  			return false;
>  		}
>  	}
> +
>  	if (strchr(uri, 0x7f)) {
>  		log_err("invalid character is used\n");
>  		return false;
>  	}
>  
> -	if (strncmp(uri, "http://", 7)) {
> -		log_err("only http:// is supported\n");
> +	if (!strncmp(uri, "http://", strlen("http://"))) {
> +		prefix_len = strlen("http://");
> +	} else if (!strncmp(uri, "https://", strlen("https://"))) {
> +		prefix_len = strlen("https://");
> +	} else {
> +		log_err("only http(s):// is supported\n");
>  		return false;
>  	}
> +
>  	str_copy = strdup(uri);
>  	if (!str_copy)
>  		return false;

Thanks,
Ilias Apalodimas Nov. 8, 2024, 11:31 a.m. UTC | #4 
Hi Jerome

On Wed, 6 Nov 2024 at 13:50, Jerome Forissier
<jerome.forissier@linaro.org> wrote:
>
> Hi Ilias,
>
> On 10/24/24 12:24, Ilias Apalodimas wrote:
> > With the recent changes of lwip & mbedTLS we can now download from
> > https:// urls instead of just http://.
> > Adjust our wget lwip version parsing to support both URLs.
> > While at it adjust the default TCP window for QEMU since https seems to
> > require at least 16384
> >
> > Signed-off-by: Ilias Apalodimas <ilias.apalodimas@linaro.org>
> > ---
> >  cmd/Kconfig      | 19 ++++++++++++
> >  net/lwip/Kconfig |  2 +-
> >  net/lwip/wget.c  | 78 +++++++++++++++++++++++++++++++++++++++++++-----
> >  3 files changed, 91 insertions(+), 8 deletions(-)
> >
> > diff --git a/cmd/Kconfig b/cmd/Kconfig
> > index 3ee70f31b142..1d90a884e2c1 100644
> > --- a/cmd/Kconfig
> > +++ b/cmd/Kconfig
> > @@ -2126,6 +2126,25 @@ config CMD_WGET
> >         wget is a simple command to download kernel, or other files,
> >         from a http server over TCP.
> >
> > +config WGET_HTTPS
> > +     bool "wget https"
> > +     depends on CMD_WGET
> > +     depends on PROT_TCP_LWIP
> > +     depends on MBEDTLS_LIB
> > +        select SHA256
> > +        select RSA
> > +        select ASYMMETRIC_KEY_TYPE
> > +        select ASYMMETRIC_PUBLIC_KEY_SUBTYPE
> > +        select X509_CERTIFICATE_PARSER
> > +        select PKCS7_MESSAGE_PARSER
> > +     select MBEDTLS_LIB_CRYPTO
> > +     select MBEDTLS_LIB_TLS
> > +     select RSA_VERIFY_WITH_PKEY
> > +     select X509_CERTIFICATE_PARSER
> > +     select PKCS7_MESSAGE_PARSER
> > +     help
> > +       Enable TLS over http for wget.
>
> s/http/HTTP/
> Maybe add "This adds support for https:// URLs"?

Sure

>
> > +
> >  endif  # if CMD_NET
> >
> >  config CMD_PXE
> > diff --git a/net/lwip/Kconfig b/net/lwip/Kconfig
> > index 8a67de4cf335..a9ae9bf7fa2a 100644
> > --- a/net/lwip/Kconfig
> > +++ b/net/lwip/Kconfig
> > @@ -37,7 +37,7 @@ config PROT_UDP_LWIP
> >
> >  config LWIP_TCP_WND
> >       int "Value of TCP_WND"
> > -     default 8000 if ARCH_QEMU
> > +     default 32768 if ARCH_QEMU
> >       default 3000000
> >       help
> >         Default value for TCP_WND in the lwIP configuration
> > diff --git a/net/lwip/wget.c b/net/lwip/wget.c
> > index b495ebd1aa96..1a2ecdcddf34 100644
> > --- a/net/lwip/wget.c
> > +++ b/net/lwip/wget.c
> > @@ -7,13 +7,17 @@
> >  #include <efi_loader.h>
> >  #include <image.h>
> >  #include <lwip/apps/http_client.h>
> > +#include "lwip/altcp_tls.h"
> >  #include <lwip/timeouts.h>
> > +#include <rng.h>
> >  #include <mapmem.h>
> >  #include <net.h>
> >  #include <time.h>
> > +#include <dm/uclass.h>
> >
> >  #define SERVER_NAME_SIZE 200
> >  #define HTTP_PORT_DEFAULT 80
> > +#define HTTPS_PORT_DEFAULT 443
> >  #define PROGRESS_PRINT_STEP_BYTES (100 * 1024)
> >
> >  enum done_state {
> > @@ -32,18 +36,53 @@ struct wget_ctx {
> >       enum done_state done;
> >  };
> >
> > +bool wget_validate_uri(char *uri);
> > +
> > +int mbedtls_hardware_poll(void *data, unsigned char *output, size_t len,
> > +                       size_t *olen)
> > +{
> > +     struct udevice *dev;
> > +     u64 rng = 0;
> > +     int ret;
> > +
> > +     *olen = 0;
> > +
> > +     ret = uclass_get_device(UCLASS_RNG, 0, &dev);
> > +     if (ret) {
> > +             log_err("Failed to get an rng: %d\n", ret);
> > +             return ret;
> > +     }
> > +     ret = dm_rng_read(dev, &rng, sizeof(rng));
> > +     if (ret)
> > +             return ret;
> > +
> > +     memcpy(output, &rng, len);
> > +     *olen = sizeof(rng);
> > +
> > +     return 0;
> > +}
> > +
> >  static int parse_url(char *url, char *host, u16 *port, char **path)
> >  {
> >       char *p, *pp;
> >       long lport;
> > +     size_t prefix_len = 0;
> > +
> > +     if (!wget_validate_uri(url)) {
> > +             log_err("Invalid URL. Use http(s)://\n");
> > +             return -EINVAL;
> > +     }
> >
> > +     *port = HTTP_PORT_DEFAULT;
> > +     prefix_len = strlen("http://");
> >       p = strstr(url, "http://");
> >       if (!p) {
> > -             log_err("only http:// is supported\n");
> > -             return -EINVAL;
> > +             p = strstr(url, "https://");
> > +             prefix_len = strlen("https://");
> > +             *port = HTTPS_PORT_DEFAULT;
> >       }
> >
> > -     p += strlen("http://");
> > +     p += prefix_len;
> >
> >       /* Parse hostname */
> >       pp = strchr(p, ':');
> > @@ -67,9 +106,8 @@ static int parse_url(char *url, char *host, u16 *port, char **path)
> >               if (lport > 65535)
> >                       return -EINVAL;
> >               *port = (u16)lport;
> > -     } else {
> > -             *port = HTTP_PORT_DEFAULT;
> >       }
> > +
> >       if (*pp != '/')
> >               return -EINVAL;
> >       *path = pp;
> > @@ -210,6 +248,9 @@ static void httpc_result_cb(void *arg, httpc_result_t httpc_result,
> >  static int wget_loop(struct udevice *udev, ulong dst_addr, char *uri)
> >  {
> >       char server_name[SERVER_NAME_SIZE];
> > +#if defined CONFIG_WGET_HTTPS
> > +     altcp_allocator_t tls_allocator;
> > +#endif
> >       httpc_connection_t conn;
> >       httpc_state_t *state;
> >       struct netif *netif;
> > @@ -232,6 +273,22 @@ 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 (port == HTTPS_PORT_DEFAULT) {
>
> Does this mean the port has to be 443 for HTTPS to be used? Or that
> plain HTTP is not possible over port 443? That would be overly restrictive
> IMO.

We set that port in parse_url(). So if http:// is parsed we will, by
default, set it to 80 and if https:// is found we set it to 443.
But I think you are right, we already support configurable port
numbers which I missed, so I'll just pass a bool to parse_url and only
enable this for https:// regardless of the port

Cheers
/Ilias
>
> > +             tls_allocator.alloc = &altcp_tls_alloc;
> > +             tls_allocator.arg =
> > +                     altcp_tls_create_config_client(NULL, 0, server_name);
> > +
> > +             if (!tls_allocator.arg) {
> > +                     log_err("error: Cannot create a TLS connection\n");
> > +                     net_lwip_remove_netif(netif);
> > +                     return -1;
> > +             }
> > +
> > +             conn.altcp_allocator = &tls_allocator;
> > +     }
> > +#endif
> > +
> >       conn.result_fn = httpc_result_cb;
> >       ctx.path = path;
> >       if (httpc_get_file_dns(server_name, port, path, &conn, httpc_recv_cb,
> > @@ -316,6 +373,7 @@ bool wget_validate_uri(char *uri)
> >       char c;
> >       bool ret = true;
> >       char *str_copy, *s, *authority;
> > +     size_t prefix_len = 0;
> >
> >       for (c = 0x1; c < 0x21; c++) {
> >               if (strchr(uri, c)) {
> > @@ -323,15 +381,21 @@ bool wget_validate_uri(char *uri)
> >                       return false;
> >               }
> >       }
> > +
> >       if (strchr(uri, 0x7f)) {
> >               log_err("invalid character is used\n");
> >               return false;
> >       }
> >
> > -     if (strncmp(uri, "http://", 7)) {
> > -             log_err("only http:// is supported\n");
> > +     if (!strncmp(uri, "http://", strlen("http://"))) {
> > +             prefix_len = strlen("http://");
> > +     } else if (!strncmp(uri, "https://", strlen("https://"))) {
> > +             prefix_len = strlen("https://");
> > +     } else {
> > +             log_err("only http(s):// is supported\n");
> >               return false;
> >       }
> > +
> >       str_copy = strdup(uri);
> >       if (!str_copy)
> >               return false;
>
> Thanks,
> --
> Jerome
diff mbox series

Patch

diff --git a/cmd/Kconfig b/cmd/Kconfig
index 3ee70f31b142..1d90a884e2c1 100644
--- a/cmd/Kconfig
+++ b/cmd/Kconfig
@@ -2126,6 +2126,25 @@  config CMD_WGET
 	  wget is a simple command to download kernel, or other files,
 	  from a http server over TCP.
 
+config WGET_HTTPS
+	bool "wget https"
+	depends on CMD_WGET
+	depends on PROT_TCP_LWIP
+	depends on MBEDTLS_LIB
+        select SHA256
+        select RSA
+        select ASYMMETRIC_KEY_TYPE
+        select ASYMMETRIC_PUBLIC_KEY_SUBTYPE
+        select X509_CERTIFICATE_PARSER
+        select PKCS7_MESSAGE_PARSER
+	select MBEDTLS_LIB_CRYPTO
+	select MBEDTLS_LIB_TLS
+	select RSA_VERIFY_WITH_PKEY
+	select X509_CERTIFICATE_PARSER
+	select PKCS7_MESSAGE_PARSER
+	help
+	  Enable TLS over http for wget.
+
 endif  # if CMD_NET
 
 config CMD_PXE
diff --git a/net/lwip/Kconfig b/net/lwip/Kconfig
index 8a67de4cf335..a9ae9bf7fa2a 100644
--- a/net/lwip/Kconfig
+++ b/net/lwip/Kconfig
@@ -37,7 +37,7 @@  config PROT_UDP_LWIP
 
 config LWIP_TCP_WND
 	int "Value of TCP_WND"
-	default 8000 if ARCH_QEMU
+	default 32768 if ARCH_QEMU
 	default 3000000
 	help
 	  Default value for TCP_WND in the lwIP configuration
diff --git a/net/lwip/wget.c b/net/lwip/wget.c
index b495ebd1aa96..1a2ecdcddf34 100644
--- a/net/lwip/wget.c
+++ b/net/lwip/wget.c
@@ -7,13 +7,17 @@ 
 #include <efi_loader.h>
 #include <image.h>
 #include <lwip/apps/http_client.h>
+#include "lwip/altcp_tls.h"
 #include <lwip/timeouts.h>
+#include <rng.h>
 #include <mapmem.h>
 #include <net.h>
 #include <time.h>
+#include <dm/uclass.h>
 
 #define SERVER_NAME_SIZE 200
 #define HTTP_PORT_DEFAULT 80
+#define HTTPS_PORT_DEFAULT 443
 #define PROGRESS_PRINT_STEP_BYTES (100 * 1024)
 
 enum done_state {
@@ -32,18 +36,53 @@  struct wget_ctx {
 	enum done_state done;
 };
 
+bool wget_validate_uri(char *uri);
+
+int mbedtls_hardware_poll(void *data, unsigned char *output, size_t len,
+			  size_t *olen)
+{
+	struct udevice *dev;
+	u64 rng = 0;
+	int ret;
+
+	*olen = 0;
+
+	ret = uclass_get_device(UCLASS_RNG, 0, &dev);
+	if (ret) {
+		log_err("Failed to get an rng: %d\n", ret);
+		return ret;
+	}
+	ret = dm_rng_read(dev, &rng, sizeof(rng));
+	if (ret)
+		return ret;
+
+	memcpy(output, &rng, len);
+	*olen = sizeof(rng);
+
+	return 0;
+}
+
 static int parse_url(char *url, char *host, u16 *port, char **path)
 {
 	char *p, *pp;
 	long lport;
+	size_t prefix_len = 0;
+
+	if (!wget_validate_uri(url)) {
+		log_err("Invalid URL. Use http(s)://\n");
+		return -EINVAL;
+	}
 
+	*port = HTTP_PORT_DEFAULT;
+	prefix_len = strlen("http://");
 	p = strstr(url, "http://");
 	if (!p) {
-		log_err("only http:// is supported\n");
-		return -EINVAL;
+		p = strstr(url, "https://");
+		prefix_len = strlen("https://");
+		*port = HTTPS_PORT_DEFAULT;
 	}
 
-	p += strlen("http://");
+	p += prefix_len;
 
 	/* Parse hostname */
 	pp = strchr(p, ':');
@@ -67,9 +106,8 @@  static int parse_url(char *url, char *host, u16 *port, char **path)
 		if (lport > 65535)
 			return -EINVAL;
 		*port = (u16)lport;
-	} else {
-		*port = HTTP_PORT_DEFAULT;
 	}
+
 	if (*pp != '/')
 		return -EINVAL;
 	*path = pp;
@@ -210,6 +248,9 @@  static void httpc_result_cb(void *arg, httpc_result_t httpc_result,
 static int wget_loop(struct udevice *udev, ulong dst_addr, char *uri)
 {
 	char server_name[SERVER_NAME_SIZE];
+#if defined CONFIG_WGET_HTTPS
+	altcp_allocator_t tls_allocator;
+#endif
 	httpc_connection_t conn;
 	httpc_state_t *state;
 	struct netif *netif;
@@ -232,6 +273,22 @@  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 (port == HTTPS_PORT_DEFAULT) {
+		tls_allocator.alloc = &altcp_tls_alloc;
+		tls_allocator.arg =
+			altcp_tls_create_config_client(NULL, 0, server_name);
+
+		if (!tls_allocator.arg) {
+			log_err("error: Cannot create a TLS connection\n");
+			net_lwip_remove_netif(netif);
+			return -1;
+		}
+
+		conn.altcp_allocator = &tls_allocator;
+	}
+#endif
+
 	conn.result_fn = httpc_result_cb;
 	ctx.path = path;
 	if (httpc_get_file_dns(server_name, port, path, &conn, httpc_recv_cb,
@@ -316,6 +373,7 @@  bool wget_validate_uri(char *uri)
 	char c;
 	bool ret = true;
 	char *str_copy, *s, *authority;
+	size_t prefix_len = 0;
 
 	for (c = 0x1; c < 0x21; c++) {
 		if (strchr(uri, c)) {
@@ -323,15 +381,21 @@  bool wget_validate_uri(char *uri)
 			return false;
 		}
 	}
+
 	if (strchr(uri, 0x7f)) {
 		log_err("invalid character is used\n");
 		return false;
 	}
 
-	if (strncmp(uri, "http://", 7)) {
-		log_err("only http:// is supported\n");
+	if (!strncmp(uri, "http://", strlen("http://"))) {
+		prefix_len = strlen("http://");
+	} else if (!strncmp(uri, "https://", strlen("https://"))) {
+		prefix_len = strlen("https://");
+	} else {
+		log_err("only http(s):// is supported\n");
 		return false;
 	}
+
 	str_copy = strdup(uri);
 	if (!str_copy)
 		return false;