[PATCH v3 4/6] net: lwip: Enable https:// support for wget

[Ilias Apalodimas]

On Wed, 13 Nov 2024 at 15:39, Simon Glass <sjg at chromium.org> wrote:
>
> HI Ilias,
>
> On Mon, 11 Nov 2024 at 07:20, Ilias Apalodimas
> <ilias.apalodimas at linaro.org> wrote:
> >
> > Thanks Simon,
> >
> > On Mon, 11 Nov 2024 at 15:03, Simon Glass <sjg at chromium.org> wrote:
> > >
> > > On Sun, 10 Nov 2024 at 01:32, Ilias Apalodimas
> > > <ilias.apalodimas at 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 at linaro.org>
> > > > ---
> > > >  cmd/Kconfig      | 19 +++++++++++
> > > >  net/lwip/Kconfig |  2 +-
> > > >  net/lwip/wget.c  | 86 +++++++++++++++++++++++++++++++++++++++++++-----
> > > >  3 files changed, 97 insertions(+), 10 deletions(-)
> > >
> > > Reviewed-by: Simon Glass <sjg at chromium.org>
> > >
> > > Some nits / questions
> > >
> > > I am not sure what mbedtls_hardware_poll() is,
> >
> > It's an entropy collector used by mbedTLS to ensure the platform has
> > enough entropy.
> > It's required if your platform doesn't support standards like the
> > /dev/urandom or Windows CryptoAPI.
> >
> > > but if @len is too
> > > short, would it be acceptable to return an error? How many bytes is it
> > > requesting in the https case?
> >
> > If you don't return enough entropy https:// will fail and mbedTLS &
> > lwIP will print an error. I think we currently use 128 and the default
> > for mbedTLS is 32.
>
> OK, then the code is quite strange to me. It seems like it should
> check that 'len' is large enough.
>
> But what does 'len' actually mean? Its arguments are not described in
> mbedtls. Shouldn't you just pass it on to the dm_rng_read() function?

The entry point is mbedtls_entropy_func(). mbedtls then calls
entropy_gather_internal() which asks for some bytes of entropy and is
controlled by a config option (and defaults to 128 for the config we
use in U-Boot -- MBEDTLS_ENTROPY_MAX_GATHER). len is actually
MBEDTLS_ENTROPY_MAX_GATHER.

> Why call it with only 8 bytes? It might be more bytes than is
> requested (and larger than the buffer), or fewer.

It doesn't matter because we copy back the correct amount of
bytes(what the caller requested). If it's less mbedTLS calls that
function until it gathers all requested entropy.

> It seems that your
> function is written with knowledge of the internals of mbedtls.

Ofc it is. It's a function needed by mbedTLS -- not U-Boot, it
requires internal knowledge of it (and is probably part of the ABI,
but I'll have to check that).

What happens in the TLS case is that 64b are required. We either make
8 calls of 8b or make a single call with MBEDTLS_ENTROPY_MAX_GATHER.

 I could rewrite it as
        uclass_first_device(UCLASS_RNG, &dev);
        if (!dev) {
                log_err("Failed to get an rng device\n");
                return ret;
        }

        rng = malloc(len);
        if (!rng)
                return -ENOMEM;

        ret = dm_rng_read(dev, rng, len);
        if (ret) {
                free(rng);
                return ret;
        }

        memcpy(output, rng, len);
        *olen = len;
        free(rng);

It does the same thing but gets 128b in one go. I don't have a strong
opinion on that. Let me know what you prefer
The tradeoff seems pretty straightforward. you either gather
potentially more entropy than required and call that function once, or
you gather exactly what's required on 8b steps -- but call that
function multiple times.

>
> BTW mbedtls_hardware_poll() is a strange name as it actually reads
> random data, so I think it should be renamed.

I don't think that can't change as you would break all projects using
it, but you can try sending a patch.

Thanks
/Ilias
>
> >
> > >
> > > uclass_first_device() if you want the first device, not uclass_get_device(...0)
> >
> > Sure
> >
>
> Regards,
> SImon