pkg/mbedtls: add TLS support for LWIP by mariemC · Pull Request #17519 · RIOT-OS/RIOT

mariemC · 2022-01-14T12:42:24Z

Contribution description

The client node connects to openssl server and sends an encrypted message after a handshake and key exchange.
mbedtls package is added.
lwip package is also modified.

Testing procedure

Board used for the test: same5-xpro.
A wifi shield is used to connect the board to wifi.

Issues/PRs references

-Depends on PR #15671

benpicco · 2022-01-18T07:23:03Z

Please rebase.
While you are at it, please also prefix the commit messages with the relevant subsystem, so pkg/mbedtls: and pkg/lwip:

PeterKietzmann · 2022-01-19T12:36:30Z

It should not be necessary to copy the mbedtls header files into pkg/mbedtls/include/

benpicco

Nice, tested this on native and looks like it works!
There are some whitespace and code style issues, but first round of review:

benpicco · 2022-01-26T16:05:42Z

pkg/mbedtls/patches/0001-mbedtls-patch.patch

+ #if !defined(unix) && !defined(__unix__) && !defined(__unix) && \
+     !defined(__APPLE__) && !defined(_WIN32) && !defined(__QNXNTO__) && \
+-    !defined(__HAIKU__) && !defined(__midipix__)
+    !defined(__HAIKU__) && !defined(__midipix__) && defined(_RIOT_)


Shouldn't this be

Suggested change

+ !defined(__HAIKU__) && !defined(__midipix__) && defined(_RIOT_)

+ !defined(__HAIKU__) && !defined(__midipix__) && !defined(RIOT_VERSION)

benpicco · 2022-01-26T16:11:21Z

tests/mbedtls_test/Makefile

+USEMODULE += shell_commands
+USEMODULE += ps
+# Add atwinc15x0 driver
+USEMODULE += atwinc15x0


No need to force the use of this driver, sam0_eth, netdev_tap or esp_wifi would work as well - just use netdev_default

benpicco · 2022-01-26T16:31:45Z

tests/mbedtls_test/Makefile

+	-DATWINC15X0_PARAM_WAKE_PIN="GPIO_PIN(0, 7)"
+CFLAGS += -DCONFIG_ENTROPY_SOURCE_ADC_HMIN="200"
+CFLAGS += -DISR_STACK_SIZE=4096
+CFLAGS += -DHOST_SERVER="\"10.0.110.117\""


Let's set this using a shell command. Then we also don't need a separate thread - just bump THREAD_STACKSIZE_MAIN if necessary.

static int cmd_tls_connect(int argc, char **argv) { if (argc < 3) { printf("usage: %s <host> <port> [data]\n", argv[0]); return -1; } return _lwip_mbedtls_client(argv[1], argv[2]); } static const shell_command_t shell_commands[] = { { "connect", "Perform a TLS connection", cmd_tls_connect }, { NULL, NULL, NULL } };

benpicco · 2022-01-26T16:32:00Z

tests/mbedtls_test/main.c

+	(void)arg;
+	int status;
+
+	xtimer_sleep(20);


Then you can also drop the delay

benpicco · 2022-01-26T16:32:46Z

tests/mbedtls_test/main.c

+	while(1) {
+		DEBUG_PUTS("_lwip_mbedtls_client_thread: sending encrypted hello world to server is DONE :) ");
+		xtimer_sleep(10);
+	}
+
+error:
+	while(1) {
+		DEBUG_PUTS("FAILED");
+		xtimer_sleep(20);
+	}
+	return NULL;


No need to loop here - just return

benpicco · 2022-01-26T16:34:32Z

tests/mbedtls_test/main.c

+	DEBUG_PUTS("_lwip_mbedtls_client_thread: server connect succeeded");
+
+	//verify certificate belongs to the server
+	status = mbedtls_ssl_set_hostname(&ssl, "mariem.com");


example.com is usually used for this 😉

benpicco · 2022-01-26T16:35:12Z

tests/mbedtls_test/main.c

+mbedtls_net_context server_fd;
+mbedtls_entropy_context entropy;
+mbedtls_ctr_drbg_context ctr_drbg;
+mbedtls_ssl_context ssl;
+mbedtls_ssl_config conf;
+mbedtls_x509_crt x509_certificate;


do those have to be global?

No, they don't

benpicco · 2022-01-26T16:36:44Z

tests/mbedtls_test/main.c

+
+	// send hello world to the server
+	unsigned char write_buf[]="Hello world\n";
+	size_t write_buf_len = sizeof(write_buf) -1;


We could also use data the user supplied as a shell command argument here

tests/mbedtls_test/main.c

yarrick · 2022-01-26T22:17:00Z

pkg/lwip/include/arch/sys_arch.h

 /**
 * @brief   Use `random_uint32()` to generate random numbers, if available
 */
+#if MBEDTLS_ENABLED == 0 


Please fix the trailing whitespaces and the horizontal tabs mentioned by the automatic checks.

yarrick · 2022-01-26T22:17:07Z

pkg/lwip/include/netdb.h

@@ -0,0 +1 @@
+#include "lwip/netdb.h"


Maybe this and the other one can be a symlink instead? It seems they are already used in the tree.

yarrick · 2022-01-26T22:17:38Z

pkg/lwip/patches/0001-lwip-patch.patch

+ /** @ingroup socket */
+ #define close(s)                                  lwip_close(s)
+ /** @ingroup socket */
+#if MBEDTLS_ENABLED == 0


Please edit the patch to explain why this is done.

PeterKietzmann · 2022-02-21T15:30:06Z

pkg/mbedtls/include/riot_mbedtls_config.h

 #endif
 /** @endcond */

+#if MBEDTLS_ENABLED == 1


Please expose (only necessary) symbols via Kconfig, as requested by @miri64 and @aabadie in a conversation starting somewhere here

yarrick · 2022-02-25T14:57:16Z

pkg/lwip/include/arch/sys_arch.h

 /**
 * @brief   Use `random_uint32()` to generate random numbers, if available
 */
+#if MBEDTLS_ENABLED == 0


This seems strange - it (LWIP_RAND) is set to the same value in lwipopts? Why is this needed?

I added (LWIP_RAND) in lwipopts in order to be recognized in build/pkg/lwip/src/core/dns.c:103
But it is already defined in pkg/lwip/include/arch/sys_arch.h:133
=> This will create a redefiniton, therefore I added #if MBEDTLS_ENABLED == 0

But you define it to the same thing there: random_uint32(). Why not just keep this and remove your extra definition? Make sure the random module is enabled and things should be fine.

PeterKietzmann · 2022-04-11T09:41:44Z

pkg/mbedtls/include/riot_mbedtls_config.h

+ *
+ * This module is required for SSL/TLS.
+ */
+#if CONFIG_MBEDTLS_SSL_TLS_C


The CONFIG_* symbols are expected to be generated by Kconfig, but they are not yet included to the Kconfig configuration system. The here and here for reference.

PeterKietzmann · 2022-04-25T13:35:13Z

tests/mbedtls_test/Kconfig

+    bool "Enable debug messages"
+    default y
+
+endif #KCONFIG_USEMODULE_MBEDTLS_TEST


Do you actually need all symbols? Please only add those that are required.

It seem to me that these configs should go into this file which is the Kconfig based point to generate the traditional mbedtls configuration header file.

Yes, all of them are needed, mainly each module requires a few other modules to work correctly therefore it seems quite a lot.

Do you mean all of them should be added in pkg/mbedtls/Kconfig?

Sorry for responding so late, haven't seen this question. Yes, all of the configuration values that are needed. Please check the existing file Kconfig file (+ existing header file) that I pointed to earlier.

PeterKietzmann · 2022-09-27T16:26:50Z

@mariemC thanks for cleaning up the configuration files. The implementation and its representation in menuconfig look good at first sight. I do, unfortunately, not have time for a full review + testing. Anyway, some thoughts on the rest of the PR:

Extend the test README so one knows what to execute (shell commands?), what to expect, and maybe give some context which certificates are included in the folder and how they are incorporated.
The name of the test folder is suboptimal. Usually, we prefix folders that relate to a package with pkg_. Now pkg_mbedtls exists already. Maybe something like pkg_mbedtls_lwip? In general I was wondering if we should execute some more of the built in mbedtls tests in RIOT. Possible to extract some of the tests that you include there?
Most of the Kconfig symbols that you include are enabled by default. I think they shouldn't, but the particular test case should enable the relevant ones.

mariemC requested review from cgundogan, jia200x, leandrolanzieri and miri64 as code owners January 14, 2022 12:42

github-actions bot added Area: doc Area: Documentation Area: Kconfig Area: Kconfig integration Area: network Area: Networking Area: pkg Area: External package ports labels Jan 14, 2022

benpicco requested a review from PeterKietzmann January 14, 2022 12:44

mariemC requested review from MichelRottleuthner, aabadie, fjmolinas and smlng as code owners January 14, 2022 13:29

github-actions bot added the Area: tests Area: tests and testing framework label Jan 14, 2022

benpicco changed the title ~~Pkg/mbedtls~~ pkg/mbedtls: add TLS support for LWIP Jan 14, 2022

benpicco added the Area: security Area: Security-related libraries and subsystems label Jan 14, 2022

mariemC force-pushed the pkg/mbedtls branch 2 times, most recently from 5db9d9f to f31f73e Compare January 25, 2022 22:27

github-actions bot removed the Area: Kconfig Area: Kconfig integration label Jan 25, 2022

benpicco requested a review from yarrick January 26, 2022 16:07

benpicco reviewed Jan 26, 2022

View reviewed changes

tests/mbedtls_test/main.c Outdated Show resolved Hide resolved

yarrick reviewed Jan 26, 2022

View reviewed changes

mariemC force-pushed the pkg/mbedtls branch 2 times, most recently from 67d7b3a to 15a9608 Compare February 21, 2022 08:27

PeterKietzmann reviewed Feb 21, 2022

View reviewed changes

yarrick reviewed Feb 25, 2022

View reviewed changes

mariemC force-pushed the pkg/mbedtls branch from 15a9608 to effcdd8 Compare April 9, 2022 01:17

github-actions bot added the Area: Kconfig Area: Kconfig integration label Apr 9, 2022

mariemC force-pushed the pkg/mbedtls branch 2 times, most recently from 6f6a3b7 to a3fbe69 Compare April 10, 2022 12:12

PeterKietzmann reviewed Apr 11, 2022

View reviewed changes

mariemC force-pushed the pkg/mbedtls branch 3 times, most recently from 1570e1c to a8092c4 Compare April 24, 2022 22:07

PeterKietzmann reviewed Apr 25, 2022

View reviewed changes

mariemC force-pushed the pkg/mbedtls branch from a8092c4 to 4876473 Compare August 22, 2022 10:01

mariemC requested a review from MrKevinWeiss as a code owner August 22, 2022 10:01

mariem added 3 commits September 13, 2022 18:06

tests/mbetls_test: Mbedtls test application

93a7561

pkg/lwip: Adjust LWIP.

17ced9e

pkg/mbedtls: Adjust mbedtls.

6c519d6

mariemC force-pushed the pkg/mbedtls branch from 4876473 to 6c519d6 Compare September 13, 2022 16:07

fabian18 mentioned this pull request Mar 29, 2023

pkg/mbedtls: extend support #19434

Open

	+ !defined(__HAIKU__) && !defined(__midipix__) && defined(_RIOT_)
	+ !defined(__HAIKU__) && !defined(__midipix__) && !defined(RIOT_VERSION)

Conversation

mariemC commented Jan 14, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Contribution description

Testing procedure

Issues/PRs references

Uh oh!

benpicco commented Jan 18, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

PeterKietzmann commented Jan 19, 2022

Uh oh!

benpicco left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

yarrick Feb 25, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

PeterKietzmann commented Sep 27, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

mariemC commented Jan 14, 2022 •

edited

Loading

benpicco commented Jan 18, 2022 •

edited

Loading

benpicco left a comment •

edited

Loading

yarrick Feb 25, 2022 •

edited

Loading