[fix] samples/net/sockets/http_server fails to build for several nrf5340/cpuapp/ns targets

[valeriosetti]

Change the sample configuration in order to replace legacy Mbed TLS crypto support with PSA one. The test case is also then split in 2 to differentiate boards that have TF-M from those that do not. This is done to avoid duplication of the PSA Crypto core.

Resolves #85155

This PR started from a suggestion from @mrodgers-witekio

[valeriosetti]

@mrodgers-witekio I tested this PR locally with:
twister -c -T samples/net/sockets/http_server --all
and all tests passed. However they were only built and not tested. Can you give it a try and let me know if everything is fine?

[jukkar]

Should we leave these and have a separate overlay config file for PSA options so that the legacy compilation works as before?
And then have separate section that tests the non-PSA and PSA compilations in sample.yaml file?
So have something like

sample.net.sockets.http.server: {}
sample.net.sockets.http.server.no_psa:
    filter: not CONFIG_BUILD_WITH_TFM
    extra_configs:
      - CONFIG_MBEDTLS_PSA_CRYPTO_C=y
      here setting the psa overlay
  sample.net.sockets.http.server.psa:
   filter: CONFIG_BUILD_WITH_TFM
      here setting the psa overlay

WDYT?

[mrodgers-witekio]

While this currently works in twister, the sample doesn't build on a platform without TF-M when built the usual way using west, since in this case it doesn't enable PSA crypto in mbedtls.

The CONFIG_MBEDTLS_PSA_CRYPTO_C=y in the snippet I tried yesterday worked for this, but like you say is probably not the best way to do this. What I really want to express in the config file is "Use PSA crypto API, but I don't care whether this is via mbedtls or TF-M". I'm not sure if there is a way to do this at the moment?

To see the build failure you can try:

west build -b nucleo_h753zi samples/net/sockets/http_server/ -- -DCONFIG_NET_SAMPLE_HTTPS_SERVICE=y

[valeriosetti]

While this currently works in twister, the sample doesn't build on a platform without TF-M when built the usual way using west, since in this case it doesn't enable PSA crypto in mbedtls.

Yes, because to build the sample you have to add the CONFIG_MBEDTLS_PSA_CRYPTO_C=y as it's done in the sample.yaml file. So for non TF-M platforms the build command will be:

west build -p always -b nucleo_h753zi samples/net/sockets/http_server/ -- -DCONFIG_MBEDTLS_PSA_CRYPTO_C=y

Or simply targetting the test case you want to reproduce:
west build -p always -b nucleo_h753zi --test samples/net/sockets/http_server/sample.net.sockets.http.server

What I really want to express in the config file is "Use PSA crypto API, but I don't care whether this is via mbedtls or TF-M". I'm not sure if there is a way to do this at the moment?

Perhaps I can do this in the Kconfig file of the sample. Let me try this

[rlubos]

Yes, because to build the sample you have to add the CONFIG_MBEDTLS_PSA_CRYPTO_C=y as it's done in the sample.yaml file. So for non TF-M platforms the build command will be:
west build -p always -b nucleo_h753zi samples/net/sockets/http_server/ -- -DCONFIG_MBEDTLS_PSA_CRYPTO_C=y

Please let's not do that, we need to be able to build for platforms like native_sim out of the box.

Why don't we just enable CONFIG_MBEDTLS_PSA_CRYPTO_C=y in the config file? There is a depends on !BUILD_WITH_TFM dependency, so for TFM-enabled builds it should end up disabled anyway?

[valeriosetti]

Why don't we just enable CONFIG_MBEDTLS_PSA_CRYPTO_C=y in the config file? There is a depends on !BUILD_WITH_TFM dependency, so for TFM-enabled builds it should end up disabled anyway?

Wouldn't that result in a warning at build time? The build will proceed of course, but I don't know if it's really elegant to have an expected warning at build time.

[mrodgers-witekio]

This builds successfully now, but I am seeing TLS handshake errors when running it:

*** Booting Zephyr OS build v4.0.0-4567-ga4b27bf7ad3a ***
[00:00:00.000,000] <inf> net_config: Initializing network
[00:00:00.000,000] <inf> net_config: IPv4 address: 192.0.2.1
[00:00:00.110,000] <inf> net_config: IPv6 address: 2001:db8::1
[00:00:00.110,000] <inf> net_config: IPv6 address: 2001:db8::1
[00:00:10.920,000] <err> net_sock_tls: TLS handshake error: -0x7080
[00:00:11.160,000] <err> net_sock_tls: TLS handshake error: -0x7080
[00:00:12.420,000] <err> net_sock_tls: TLS handshake error: -0x7080
[00:00:12.660,000] <err> net_sock_tls: TLS handshake error: -0x7080

This is reproducible on the native_sim platform. In one terminal window (path starting from west workspace root):

./tools/net-tools/net-setup.sh

In another terminal:

west build -b native_sim samples/net/sockets/http_server/ -t run -- -DCONFIG_NET_SAMPLE_HTTPS_SERVICE=y

And then navigate to https://192.0.2.1 in a web browser.

I guess there's probably a PSA_WANT config missing, but not sure exactly which one

[rlubos]

Why don't we just enable CONFIG_MBEDTLS_PSA_CRYPTO_C=y in the config file? There is a depends on !BUILD_WITH_TFM dependency, so for TFM-enabled builds it should end up disabled anyway?

Wouldn't that result in a warning at build time? The build will proceed of course, but I don't know if it's really elegant to have an expected warning at build time.

It would, still better than not building at all for non-tfm platforms IMHO. I'm fine with the current proposal though. However - maybe we need to rethink on how PSA APIs are enabled in general, it's a bit awkward that you need to define a custom Kconfig entry in the sample because the configuration differs depending on platform type.

[valeriosetti]

@mrodgers-witekio thanks for the test. I was indeed trying to test it locally but I'm a bit stuck on this. I would like to use the nrf52480dk board through the USB-Ethernet bridge which I saw should be possible from the README file of that sample. So I built it with:
west build -p always -b nrf52840dk/nrf52840 --extra-conf=overlay-netusb.conf samples/net/sockets/http_server/

Then on my PC I run ip addr add 192.168.x.y/24 dev <dev-name> to assign an IP. Now, if I try to ping the IP that I just assigned it works perfectly, but if I try with curl I get Connection refused
As a consequence I'm not able to see the TLS handshake failures you shown above.
Any idea of what I'm missing?

[mrodgers-witekio]

However - maybe we need to rethink on how PSA APIs are enabled in general, it's a bit awkward that you need to define a custom Kconfig entry in the sample because the configuration differs depending on platform type.

I was about to say, it might be useful to have a Kconfig at a system level rather than a sample app level that is something like CONFIG_PSA_WANT_CLIENT, and defaults to using TF-M if it's available or falls back to enabling mbedtls PSA core if not. I'm sure it's probably more complicated than this, and I'm not suggesting it should be done as part of this PR, but might be worth considering...

[mrodgers-witekio]

Any idea of what I'm missing?

I've never used the netusb config myself so not sure about that one specifically. The net samples generally have an IP address of 192.0.2.1, not in the usual 192.168.x.x range, although I don't know if the netusb is doing anything different here.

Using "real" target hardware connected directly to my PC via Ethernet, I generally have to do something like:

Assign my PC an IP address on the network interface the device is plugged into:

sudo ip addr add 192.0.2.2 dev enp0s31f6

Add an entry in routing table for the device:

sudo ip route add 192.0.2.1 via 192.0.2.2 dev enp0s31f6

And after this it should work: I can ping 192.0.2.1, and use the HTTP server via a web browser

[rlubos]

I got it to work with native sim at least. The handshake failed with:

[00:00:14.520,000] mbedtls: ssl_tls12_server.c:2993: ECDHE curve: secp256r1
[00:00:14.520,000] mbedtls: ssl_tls12_server.c:3007: Perform PSA-based ECDH computation.
[00:00:14.520,000] mbedtls: ssl_tls12_server.c:3043: psa_generate_key() returned -28800 (-0x7080)
[00:00:14.520,000] mbedtls: ssl_tls.c:4617: <= handshake
[00:00:14.520,000] net_sock_tls: TLS handshake error: -0x7080

Adding the following lines to the config file allowed me to establish TLS session with the sample:

CONFIG_PSA_WANT_KEY_TYPE_ECC_KEY_PAIR_GENERATE=y
CONFIG_PSA_WANT_ALG_TLS12_PRF=y

No idea why was the second config needed though, just trial & error based on https://github.com/zephyrproject-rtos/zephyr/tree/main/tests/net/socket/tls_configurations configs.

[valeriosetti]

No idea why was the second config needed though

Thanks for the test and update. Well the additional config makes sense to me:

• CONFIG_PSA_WANT_KEY_TYPE_ECC_KEY_PAIR_GENERATE is because if the device is doing ECDH then it needs to generate an EC key pair
• CONFIG_PSA_WANT_ALG_TLS12_PRF is of the key derivation function used in TLS 1.2 and I guess it's used to generate some other key after the agreed EC key.

[mrodgers-witekio]

Tested on native_sim and it''s working now.

Flash size needed for the sample is bumped up a bit (approx 325K --> 350K on nucleo_h753zi board, I wonder if there's any crypto features enabled by default that could be disabled to reduce this? I'm thinking of RSA in particular, as it's not needed by this sample.

[valeriosetti]

I've just updated the PR with the changes required to make the sample working on the nrf52840dk. Since it took me some time to do it, I also updated the documentation.

I'm thinking of RSA in particular, as it's not needed by this sample.

Correct, indeed that part was removed. I kept it at the beginning because an RSA key exchange was previously enabled, but I realized that actually an ECDHE-ECDSA cipher suite is used when testing the connection on my PC.

On this topic, I've seen that samples/net/sockets/http_server/src/certs/ca.der contains an RSA key which, if I'm not mistaken, seems not to be used anywhere. Can I just delete that file?

[mrodgers-witekio]

On this topic, I've seen that samples/net/sockets/http_server/src/certs/ca.der contains an RSA key which, if I'm not mistaken, seems not to be used anywhere. Can I just delete that file?

Yes, I think that file is just leftover from before the certificates were updated to use ECDSA so can be deleted (the ca_cert.der is still needed though despite not being included in this sample specifically - see 8f07784)

[jukkar]

If this is only used for this sample, then please namespace the option name by NET_SAMPLE to make it clear that this is not a global option.

[valeriosetti]

Actually this is the very same Kconfig defined in zephyr/modules/mbedtls/Kconfig.tls-generic. I added it here as trick to enable it for platforms that do not have TF-M support.

As stated previously we might improve how PSA Crypto support is enabled, but that will require additional analysis and work and it's out of scope for this PR.

The other option was to set CONFIG_MBEDTLS_PSA_CRYPTO_C=y directly in the prj.conf for all the samples, but that would result in warnings at compile time for boards that support TF-M, so I preferred not to go that way.

[jukkar]

This looks very hackish now, basically you are stealing Kconfig option from mbedtls which I would say is a big no-no.

[valeriosetti]

I see. In my initial implementation I split the sample testing between TF-M and non-TF-M platforms (see f3b7c2a), but I was told this was not the proper way to go.
If you also agree that what I implemented back then was not OK, then I don't see many other options than just adding CONFIG_MBEDTLS_PSA_CRYPTO_C=y for all the boards and then live with the warning for those that support TF-M.

[rlubos]

Why was this removed, the sample will still not fit to that platform?

[valeriosetti]

Yes, correct. I initially thought that moving from legacy Mbed TLS support to TF-M's PSA Crypto on that platform could make the sample build successfully, but I just tried and it's not the case. So I need to revert this leftover. Thanks for noticing.

[jukkar]

I am giving this -1 atm before we figure out what to do with CONFIG_MBEDTLS_PSA_CRYPTO_C option.
This looks very confusing to the end user that we are introducing a new option in the sample and basically steal the option from mbedtls. Is this really a common way things are done in zephyr?

[mrodgers-witekio]

I am giving this -1 atm before we figure out what to do with CONFIG_MBEDTLS_PSA_CRYPTO_C option. This looks very confusing to the end user that we are introducing a new option in the sample and basically steal the option from mbedtls. Is this really a common way things are done in zephyr?

It's quite commonly done in Kconfig.defconfig files at a board level (e.g. see https://github.com/zephyrproject-rtos/zephyr/blob/main/boards/st/nucleo_f429zi/Kconfig.defconfig#L10C1-L11C11), but I don't think I've seen it in a "normal" Kconfig file before, so not sure if this is OK...

As an alternative that might be a bit cleaner, how about this:

config NET_SAMPLE_HTTPS_SERVICE
	bool "Enable https service"
	depends on NET_SOCKETS_SOCKOPT_TLS || TLS_CREDENTIALS
	imply MBEDTLS_PSA_CRYPTO_C if !BUILD_WITH_TFM

I tried a build and it seems to work for me.

This would need the TLS-related options to be split out into an overlay file (otherwise the build without HTTPS selected will still be broken), but this is probably a good idea anyway and aligns better with the other networking samples

[jukkar]

It's quite commonly done in Kconfig.defconfig files at a board level (e.g. see https://github.com/zephyrproject-rtos/zephyr/blob/main/boards/st/nucleo_f429zi/Kconfig.defconfig#L10C1-L11C11),

The Kconfig.defconfig options I knew of, and setting options there is fine.

This would need the TLS-related options to be split out into an overlay file (otherwise the build without HTTPS selected will still be broken), but this is probably a good idea anyway and aligns better with the other networking samples

I like this much better, it is way clearer to the end user what we are trying to do here.
@valeriosetti could you convert the PR to use the options like that?

[valeriosetti]

@valeriosetti could you convert the PR to use the options like that?

Sure! I will rework the PR ASAP

[mrodgers-witekio]

Should CONFIG_NET_SAMPLE_HTTPS_SERVICE=y go directly in the overlay file, so that the user can just add the overlay and not have to specify any additional Kconfig options?

Apart from that all looks good to me now