Hi Simon,
On Sat, 19 Oct 2024 at 14:50, Simon Glass <s...@chromium.org> wrote:
>
> Hi Ilias,
>
> On Fri, 18 Oct 2024 at 08:23, Ilias Apalodimas
> <ilias.apalodi...@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
> > requite at least 16384
> >
> > Signed-off-by: Ilias Apalodimas <ilias.apalodi...@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 8c677b1e4864..e58566a9ba34 100644
> > --- a/cmd/Kconfig
> > +++ b/cmd/Kconfig
> > @@ -2118,6 +2118,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
>
> Is it possible to do wget programmatically, i.e. without CMDLINE?
Yes. But that would require more untangling of wget. I'll look into
that once we are done with features.
>
> > + 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..b4f039d38962 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 err;
>
> ret
>
> > +
> > + *olen = 0;
> > +
> > + err = uclass_get_device(UCLASS_RNG, 0, &dev);
>
> Using uclass_get_device_by_seq() would allow aliases to select which
> device is used.
Ok, but I don't think we need it here. We just need a random number
from any device that can send us one no?
>
> > + if (err)
> > + return err;
> > + err = dm_rng_read(dev, &rng, sizeof(rng));
> > + if (err) {
> > + log_err("Failed to get an rng: %d\n", err);
>
> If you are showing an error, I think it is more likely to happen when
> trying to find the device, so perhaps do this on the
> uclass_get_device() call?
Sure
>
> > + return err;
> > + }
> > +
> > + memcpy(output, &rng, len);
> > + *olen = sizeof(rng);
>
> But then why not dm_rng_read() into output and avoid the u64?
> Actually, dm_rng_ops-read() should return the number of bytes,
> perhaps?
The caller defines the length. Copying blindly a u64 might overflow.
But the current code should be
*olen = len
>
> > +
> > + 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 (IS_ENABLED(CONFIG_WGET_HTTPS))
>
Unfortunately, I don't think we can use that here. If
CONFIG_WGET_HTTPS is not enabled LWIP_ALTCP will not be defined, and
the altcp_allocator_t field will be missing from the struct leading to
a compilation error
Thanks
/Ilias
> > + 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
> >
>
> Regards,
> Simon
