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

[Simon Glass]

Hi Ilias,

On Wed, 13 Nov 2024 at 07:46, Ilias Apalodimas
<ilias.apalodimas at linaro.org> wrote:
>
> 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.

I am wondering why we can't just do:

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

*olen = len;

Regards,
Simon