[WIP] Module GDTLS #7177

rfuentess · 2017-06-12T15:53:09Z

This PR introduce the use of the package tinyDTLS as an module for RIOT O.S together to upgrade to the current package and examples.

The idea behind this is to make the use of DTLS the most transparent possible for the user, by just requiring to add USEMODULE += gnrc_dtls to the application's Makefile.

For this first PR the support is only for the module GCOAP.
This is tested with the iotlab-m3 boards.

Work in progress

gdtls

I think is necessary to remark that this module is using the keys used by tinyDTLS by default and therefore should not be used directly for any real implementation.

Also, the BOARD_BLACKLIST of the application running this module should be update to include those of the example/dtls-echo/Makefile. Or maybe we can update the list at fly ?

TODO

Extend the documentation.
Add support for DTLS renegotiation.
Handing the BOARD_BLACKLIST.

pkg/tinydtls

The package now has default settings for avoiding extra lines in the application's Makefile. Also, it was upgraded to 0.8.6.

There is still problems with memory usage. This is the reason behind issues such as #7020. This is where almost all my efforts will be focused in the following days.

TODO

Fix memory issues.

gcoap

The simplicity of this module permits to use a single sock for listening to the client and server messages. However, for DTLS is necessary to separate both entities. Mostly of the changes introduced here were for creating a third thread to handle the DTLS session for the DTLS client; meanwhile the original server thread handles now the DTLS server side and listen for incoming messages for both of them sides.

TODO

Full compatibility with the RFC 7252 (e.g. support for DTLS renegotiation).
Permit that CoAP URIs handles the status of the DTLS channel (E.g. /dtls/stop or /dtls/renegotiatie )

example/dtls-echo

I'm not sure what to do with the current example. It's not using this new module, so it can be seen as redundant. However, is a good example of how to handle asymmetric keys separately, for server/client sides, and how to modify GDTLS for custom applications. Also, it has been useful for testing the client and server side of Gcoap and for other implementations that make use of DTLS.

example/gcoap

An additional command is introduced for stopping the DTLS client side of gcoap.

rfuentess · 2017-06-12T15:56:49Z

Also, the PR #6430 is now deprecated.

aabadie · 2017-06-12T16:00:08Z

Very nice, an important piece of software that was missing !

rfuentess · 2017-06-12T16:09:37Z

sys/net/application_layer/coap/gcoap.c

        _listen(&_sock);
+#else
+
+    if ( (flag == DTLS_FLAGS_SERVER_LISTEN) && !dtls_context_clnt ) {


Once a CoAP command is given, the new thread will generate a new DTLS session or use a previous one for the client side, but also disable the DTLS server side of the sensor. And I want some opinions for this behavior.

The logical would be that behavior was of a polling, by first listening the server sock and then the client sock. However, I fear that this could exhaust the sensor resources as handling resources for both sides could be too much (and currently the risk is very high).

Another idea I was thinking is to leave the polling (if - if instead of if - else if) with ANDs operators, but is still the same behavior.

aabadie

I overlooked the changes here but it will take some time for in-depth review.
To simply review, can you try to follow the coding conventions from the beginning ? Otherwise, it disturbs the reviewer concentration.

I already put a few comments and questions in-line. I'll go back to this later.

aabadie · 2017-06-12T16:02:11Z

examples/dtls-echo/Makefile

-                             nucleo-f070 nucleo-f072 nucleo-f103 nucleo-f302 nucleo-f334 \
-                             nucleo-l053 nucleo-l073 opencm904 pca10000 pca10005 \
-                             spark-core stm32f0discovery weio yunjia-nrf51822
+BOARD_INSUFFICIENT_MEMORY := airfy-beacon nrf51dongle nrf6310 nucleo-f103 \


Can you keep the alphabetical order ? even better, set one board on one line.
by the way, you removed calliope-mini (and maybe others), why ?

This was an omission from my part that was already identified at PR #6430. It's now addressed for this PR.

Also, the Issue #6022 is not anymore a factor for tinydtls. Therefore, I removed the nrf52dk board from the BOARD_BLACKLIST.

Yet, the new boards mips-malta, pic32-clicker and pic32-wifire are failing the buildtests, even with docker. Maybe because of the Issue #6588 ?

aabadie · 2017-06-12T16:03:09Z

examples/dtls-echo/Makefile


-# NOTE: This adds support for TLS_ECDHE_ECDSA_WITH_AES_128_CCM_8
-CFLAGS += -DDTLS_ECC
+# If enabled TinyDTLS' log are disabled (if memory is a issue).


is an issue

aabadie · 2017-06-12T16:03:45Z

examples/dtls-echo/Makefile


-# NOTE: This adds support for TLS_ECDHE_ECDSA_WITH_AES_128_CCM_8
-CFLAGS += -DDTLS_ECC
+# If enabled TinyDTLS' log are disabled (if memory is a issue).


aabadie · 2017-06-12T16:04:06Z

examples/dtls-echo/Makefile

-# NOTE: This adds support for TLS_ECDHE_ECDSA_WITH_AES_128_CCM_8
-CFLAGS += -DDTLS_ECC
+# If enabled TinyDTLS' log are disabled (if memory is a issue).
+CFLAGS += -DNDEBUG


do we want this by default ?

aabadie · 2017-06-12T16:05:05Z

examples/dtls-echo/dtls-client.c


-#define ENABLE_DEBUG (0)
+#define ENABLE_DEBUG  (1)


aabadie · 2017-06-12T16:17:27Z

sys/net/application_layer/coap/gcoap.c

+                                            coap_pkt_t *pdu);
+#ifdef MODULE_GNRC_DTLS
+static int _dtlsc_data_try_send(dtls_data_app_t * data,
+                              dtls_context_t * dtls_context,  session_t *dst);


alignment issue

aabadie · 2017-06-12T16:18:42Z

sys/net/application_layer/coap/gcoap.c

+
+
+  mutex_lock(&gcoap_mutex); /* Racing condition? (With udp_sock_rcvd ) */
+  while(app_data_buf>0){


spaces missing here : after while, around operator, before brace

aabadie · 2017-06-12T16:19:13Z

sys/net/application_layer/coap/gcoap.c

+int _read_from_peer(struct dtls_context_t *ctx,
+                          session_t *session, uint8 *data, size_t len)
+{
+  (void) ctx;


we use 4 spaces indent, there are some missing

aabadie · 2017-06-12T16:19:19Z

sys/net/application_layer/coap/gcoap.c

+ * @brief Reception of a DTLS Applicaiton data record for GCOAP module.
+ */
+int _read_from_peer(struct dtls_context_t *ctx,
+                          session_t *session, uint8 *data, size_t len)


aabadie · 2017-06-12T16:19:57Z

sys/net/application_layer/coap/gcoap.c

+                                      session_t *dst ){
+
+
+  msg_t mbox_msg;


indent is off in this function

kYc0o · 2017-06-13T13:28:44Z

Adding the CI to check for early errors.

rfuentess · 2017-06-13T13:38:11Z

@aabadie ~~ACK~~ and squashed the changes.

kYc0o · 2017-06-13T13:41:28Z

Don't amend and push commits for each time you fix something. The commit history in the PR lets the reviewer know how did you address the comments. Besides, on each push you do the CI is triggered, which is not so performant while the CI is busy with some other PRs in the queue.

By the way, you cannot ACK your own PRs. One or more reviewers will do that through the Github interface when all comments are addressed and the CI agrees.

kaspar030

some formal remarks

kaspar030 · 2017-06-13T13:40:55Z

examples/dtls-echo/Makefile


-# NOTE: Add the package for TinyDTLS
-USEPKG += tinydtls
+# If enabled, DTLS client will spoofed a CoAP message (GET /riot/value)


don't understand the sentence

Just to be sure the following change will be enough clear:
DTLS-client will generate a "CoAP get /riot/value" message instead of sending the user input

kaspar030 · 2017-06-13T13:41:50Z

examples/dtls-echo/main.c

@@ -32,7 +32,7 @@
 #error TinyDTLS is configured for working with Sockets. Yet, this is non-socket
 #endif

-#define MAIN_QUEUE_SIZE     (8)
+#define MAIN_QUEUE_SIZE     (16)


This mostly because of the behavior of DTLS while the handshake process is taking place. In some part of it, one peer will transmit five DTLS records in a very short time. Now, because of 6LoWPAN fragmentation, two of them could be fragmented. So, I fear that for the moment the sensors reach the sock_udp_rcv () we could have a full buffer.

This is why I moved the default value of 8 to the next one.

kaspar030 · 2017-06-13T13:42:47Z

examples/gcoap/gcoap_cli.c

+#ifdef MODULE_GNRC_DTLS
+int dtlsc_cli_cmd(int argc, char **argv)
+{
+    (void) argc;


kaspar030 · 2017-06-13T13:43:24Z

pkg/tinydtls/Makefile

+#Last tested commit:
+PKG_VERSION=5b5ba4b3c7a79dda168103be4d119ca489f89c3c
+# (Official) Branch with tested(?) updates before merging to remote master
+#PKG_VERSION=RIOT-Tested-Upgrades


please only exact commits

A question, I was leaving that line for having an easier time for testing the future upgrades to tinyDTLS. Also, for those users that requires to have access to recent patches that would still be in the process of being tested. Is not recommended having it?

kaspar030 · 2017-06-13T13:44:36Z

sys/include/net/dtls/dtls_keys.h

+
+#ifndef NET_DTLS_KEYS_PARAMS_H
+#define NET_DTLS_KEYS_PARAMS_H
+


extra newline

kaspar030 · 2017-06-13T13:45:59Z

IMO dtls should be integrated above sock_udp...

rfuentess · 2017-06-14T09:52:07Z

In relation tothe idea of add DTLS directly over sock for an asynchronous connection. I think the steps used for making the DTLS client side for GCOAP are adequate for it and could be replicated for the server side. However, it will take me some time to have a prototype.

I'm thinking of the following API for the final user:

secure_sock_init ( )    /* Prepare DTLS and two threads (client and server DTLS)*/

secure_sock_create( )  /* Universal (Client/Server), it also requires the keys
                           for the DTLS client or server,  and a pointer to the
                           function to call when data is received  */

secure_sock_listening( ) /* Only for the server side. It listen to any request
                             to make a safe channel */

secure_sock_connect( ) /* Only for the client side. It starts the secure channel */

secure_sock_send() /* Universal. (Try) send data. Maybe non-blocking? */

secure_sock_renew( ) /* Universal. Probably will be linked to the same
                         function than secure_sock_create.*/

secure_sock_stop() /* Universal (and probably can be called internally by
                      the other side once the DTLS alert is received) */

The problem here is that will not be that transparent for the current modules and examples. Also, I'm not sure how reliable could be.

I'll take the original GCOAP module as an example:

We need to prepare the DTLS, the socks, the end points local ~~and remote and~~ maybe the DTLS context. This will be done in gcoap_init() by calling secure_sock_init().

When the GCOAP client wants to send a message, it will replace sock_udp_create() by secure_sock_create(). However, the scenario here is that an already established secure channel maybe already there. I think the endpoints local and remote should be key here, and maybe timers.

Depending on the success of secure_sock_create(), the GCOAP client could call secure_sock_connect() or maybe secure_sock_renew().

Once the GCOAP client reaches the line where a CoAP message is ready to be sent. The sock_udp_send() is changed by secure_sock_send(). Take note that for timeouts, the first time it could take much more time than secondary times and this could be repeated once that renegotiation is taking place.

The part where GCOAP is listening for CoAP requests, including answers to the GCOAP client side. It will be modified to only have the secure_sock_create() for the server and the secure_sock_listening(). That is, is not anymore linked to the GCOAP client side.

But, this is the easy part. Now come the hard part: When, and by whom, the DTLS renegotiation will take place? When are we to close open DTLS channels in the client side? If we closed them dynamically, by means of timers? What we do while waiting to try to send data?

kb2ma · 2017-06-14T13:33:23Z

Just noticed this PR. Thanks for this work! I'll provide detailed reactions soon on integration with gcoap. FWIW, I'm just about to post my plan for implementation of confirmable messaging in gcoap. I'll think about this PR from that perspective, too.

miri64 · 2017-06-14T13:38:05Z

Hi, checking in here, too. I'm not sure providing a gdtls API would be the right way to go (what's the g standing for btw??!?). Wouldn't a sock-based sock_dtls API be much more in line with our transport layer philosophy?

rfuentess · 2017-06-14T14:06:02Z

Hi, checking in here, too. I'm not sure providing a gdtls API would be the right way to go (what's the g standing for btw??!?)

The idea behind making this module was from a discussion with @aabadie. But, we only explored a rapid and easy way to combine DTLS and CoAP and still being transparent for the user. It was not until speaking with @kaspar030 that the discussion of running DTLS over sock was put on the table. If this approach is considered a better option for everyone, it can be done.

About the "g", I wanted to use the g as an abbreviation for GNRC. In part because GDTLS is using socks. But also, for having some concordance with the name for gcoap (that I'm assuming is the same meaning :) )

miri64 · 2017-06-14T14:13:52Z

About the "g", I wanted to use the g as an abbreviation for GNRC. In part because GDTLS is using socks.

Sock does not imply GNRC, so this statement doesn't make much sense.

It was not until speaking with @kaspar030 that the discussion of running DTLS over sock was put on the table. If this approach is considered a better option for everyone, it can be done.

I did not propose to run DTLS over sock (though this is a desirable outcome, too). I proposed to provide a completely new component to sock (sock_dtls) that can be a generic front-end to any DTLS implementation (like tinyDTLS).

The idea behind making this module was from a discussion with @aabadie. But, we only explored a rapid and easy way to combine DTLS and CoAP and still being transparent for the user. It was not until speaking with @kaspar030 that the discussion of running DTLS over sock was put on the table.

[...] But also, for having some concordance with the name for gcoap (that I'm assuming is the same meaning :) )

If the goal of this module is to provide CoAP on top of DTLS, I think gcoaps would be a better fitting name. gDTLS implies a new DTLS implementation (or DTLS API?).

rfuentess · 2017-06-14T14:24:12Z

Question about the current murdock's error messages,

Seems that for the mips-malta (and its extended family) murdock is inspecting the pkg/tinydtls files, which I believe it should not happen.

Additionally, when I was testing make buildtest, the same family given me problems, even when DOCKER was used (but here I believe that issue #6588 is related). So, I'm not sure how to proceed for those boards.

rfuentess · 2017-06-14T14:30:37Z

If the goal of this module is to provide CoAP on top of DTLS, I think gcoaps would be a better fitting name. gDTLS implies a new DTLS implementation (or DTLS API?).

Yes, I considered a DTLS API because it was considered to use it for other protocols, such as TFTP.

kb2ma · 2017-06-14T18:45:57Z

I know little about DTLS, so I'm sort of waving my hands, but here's my 2 cents. With an additional event loop and generic functions like _dtlsc_create_channel() in gcoap.c, I see enough functionality for a separate module to house them. This module then is an adapter between tinyDTLS and application-level modules like gcoap. Maybe there are callbacks and the application passes some DTLS context object. At the same time, abstraction/adaptation also can be used to integrate with the sock-udp API.

I would be happy to help make gcoap the first module to use such adaptations. :-)

rfuentess · 2018-06-21T17:41:55Z

Closing in favor of the work done on #7397.

rfuentess commented Jun 12, 2017

View reviewed changes

kYc0o mentioned this pull request Jun 12, 2017

pkg: tinydtls: Upgrades (02c) #6430

Closed

aabadie requested changes Jun 12, 2017

View reviewed changes

kYc0o self-assigned this Jun 13, 2017

rfuentess force-pushed the Module_DTLS branch from 8114d06 to db086dd Compare June 13, 2017 13:29

rfuentess added 5 commits June 13, 2017 15:37

Example/dtls-echo: Upgrade for using socks

51f5bda

Pkg/tinydtls: Upgrade to TinyDTLS 0.8.6

30538f9

GDTLS: Adding module for DTLS oveR GNRC

fec7dfb

gcoap: Added support for the module GDTLS

6e62b78

examples/gcoap: Added special lines for GDTLS

9b0b18f

rfuentess force-pushed the Module_DTLS branch from db086dd to 9b0b18f Compare June 13, 2017 13:37

kaspar030 requested changes Jun 13, 2017

View reviewed changes

rfuentess added 8 commits June 15, 2017 12:37

gdtls: Fixed header guards issue

0007fc1

gcoap: Removed unnecessary variable for GDTLS

b48eee7

gdtls/dtls_keys.h: extra line removed

4bc93ce

Removed static property of variable (coccinelle)

dbd2575

pkg/tinydtls: Makefiles updated

c381464

dtls-echo/Makefile: Boards mips-malta and pci32-* added to black list

bdc38ea

Dtls-echo/Makefile: documentation for COAP_MSFG_SPOOF clarified

eec8859

gcoap/gcoap_cli.c: Removed unnecessary (void) lines

b288653

rfuentess force-pushed the Module_DTLS branch from 0cf7823 to b288653 Compare June 15, 2017 10:38

aabadie added this to the Release 2017.10 milestone Jun 23, 2017

rfuentess mentioned this pull request Jul 21, 2017

RFC: secure_sock - Integration of (D)TLS stacks for RIOT O.S. #7397

Closed

smlng modified the milestones: Release 2017.10, Release 2018.01 Nov 16, 2017

smlng removed this from the Release 2018.01 milestone Jan 18, 2018

rfuentess closed this Jun 21, 2018

rfuentess mentioned this pull request Jun 28, 2018

[WIP] sock_secure module #9450

Closed

5 tasks



		mutex_lock(&gcoap_mutex); /* Racing condition? (With udp_sock_rcvd ) */
		while(app_data_buf>0){


		#ifndef NET_DTLS_KEYS_PARAMS_H
		#define NET_DTLS_KEYS_PARAMS_H

[WIP] Module GDTLS #7177

[WIP] Module GDTLS #7177

Conversation

rfuentess commented Jun 12, 2017

Work in progress

gdtls

TODO

pkg/tinydtls

TODO

gcoap

TODO

example/dtls-echo

example/gcoap

rfuentess commented Jun 12, 2017

aabadie commented Jun 12, 2017

Choose a reason for hiding this comment

aabadie left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kYc0o commented Jun 13, 2017

rfuentess commented Jun 13, 2017 • edited Loading

kYc0o commented Jun 13, 2017

kaspar030 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rfuentess Jun 13, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kaspar030 commented Jun 13, 2017

rfuentess commented Jun 14, 2017

kb2ma commented Jun 14, 2017

miri64 commented Jun 14, 2017

rfuentess commented Jun 14, 2017

miri64 commented Jun 14, 2017

rfuentess commented Jun 14, 2017

rfuentess commented Jun 14, 2017

kb2ma commented Jun 14, 2017 • edited Loading

rfuentess commented Jun 21, 2018

rfuentess commented Jun 13, 2017 •

edited

Loading

rfuentess Jun 13, 2017 •

edited

Loading

kb2ma commented Jun 14, 2017 •

edited

Loading