[riot-notifications] [RIOT-OS/RIOT] net/gcoap: support DTLS (#15549)

benpicco notifications at github.com
Tue May 25 12:37:08 CEST 2021


@benpicco commented on this pull request.



> @@ -30,6 +30,27 @@
 #define ENABLE_DEBUG 0
 #include "debug.h"
 
+#if IS_ACTIVE(CONFIG_GCOAP_ENABLE_DTLS)
+#include "net/credman.h"
+#include "net/dsm.h"
+#include "tinydtls_keys.h"
+
+#define GCOAP_DTLS_CREDENTIAL_TAG 10

What significance does the number 10 bear? 

> + * @}
+ */
+
+#include "net/dsm.h"
+#include "mutex.h"
+#include "net/sock/util.h"
+#include "xtimer.h"
+
+#define ENABLE_DEBUG 0
+#include "debug.h"
+
+typedef struct {
+    sock_dtls_t *sock;
+    sock_dtls_session_t session;
+    dsm_state_t state;
+    uint64_t last_used;

what unit is that in?
do we really need µs accuracy / 64 bit here?

> +        sock_udp_ep_t ep;
+        sock_dtls_session_get_udp_ep(&socket.ctx_dtls_session, &ep);
+        _process_coap_pdu(&socket, &ep,  _listen_buf, res);
+    }
+}
+#endif
+
+#if IS_ACTIVE(CONFIG_GCOAP_ENABLE_DTLS)
+/* Timeout function to free up a session when too many session slots are occupied */
+static void _dtls_free_up_session(void *arg) {
+    (void)arg;
+    sock_dtls_session_t session;
+
+    uint8_t minimum_free = CONFIG_GCOAP_DTLS_MINIMUM_AVAILABLE_SESSIONS;
+    if (dsm_get_num_available_slots() < minimum_free) {
+        if(dsm_get_oldest_used_session(&_sock_dtls, &session) != -1) {

since this is the only use of this function, why not make it `dsm_drop_oldest_used_session()` and save yourself the need for iterating the sessions array twice? 

> +uint8_t dsm_get_num_available_slots(void)
+{
+    return _available_slots;
+}
+
+uint8_t dsm_get_num_maximum_slots(void)
+{
+    return DTLS_PEER_MAX;
+}
+
+ssize_t dsm_get_oldest_used_session(sock_dtls_t *sock, sock_dtls_session_t *session)
+{
+    int res = -1;
+    dsm_session_t *session_slot = NULL;
+
+    if (dsm_get_num_available_slots() != DTLS_PEER_MAX) {

```suggestion
    if (dsm_get_num_available_slots() == DTLS_PEER_MAX) {
        return -1;
    }
```

You can drop a level of nesting 

> +    } else {
+        (void)session;
+    }

This can be dropped. 

> -#ifndef CONFIG_GCOAP_PORT
 #define CONFIG_GCOAP_PORT              (5683)

leave the `#ifndef` in place, add a new one for `CONFIG_GCOAPS_PORT`

> @@ -35,6 +35,23 @@ DEVELHELP ?= 1
 # Change this to 0 show compiler invocation lines by default:
 QUIET ?= 1
 
+# Enables DTLS-secured CoAP messaging
+GCOAP_ENABLE_DTLS ?= 1
+ifeq (1,$(GCOAP_ENABLE_DTLS))
+	# Required by DTLS. Currently, only tinyDTLS is supported by sock_dtls.
+	USEPKG += tinydtls
+	USEMODULE += tinydtls_sock_dtls
+	USEMODULE += gcoap_dtls
+	# tinydtls needs crypto secure PRNG
+	USEMODULE += prng_sha1prng
+
+	CFLAGS += -DCONFIG_GCOAP_ENABLE_DTLS=1
+	# Maximum number of DTLS sessions
+	CFLAGS += -DDTLS_PEER_MAX=1
+	# increase main stack
+	CFLAGS += -DTHREAD_STACKSIZE_MAIN=2*THREAD_STACKSIZE_DEFAULT

is it not enough to bump `GCOAP_STACK_SIZE`?

>  
+    if (IS_ACTIVE(CONFIG_GCOAP_ENABLE_DTLS)) {
+#if IS_ACTIVE(CONFIG_GCOAP_ENABLE_DTLS)
+        sock_dtls_init();
+        if (sock_dtls_create(&_sock_dtls, &_sock_udp,
+                            CREDMAN_TAG_EMPTY,
+                            SOCK_DTLS_1_2, SOCK_DTLS_SERVER) < 0) {
+            puts("gcoap: error creating DTLS sock");

```suggestion
            DEBUG("gcoap: error creating DTLS sock");
```

>  
+    if (IS_ACTIVE(CONFIG_GCOAP_ENABLE_DTLS)) {
+#if IS_ACTIVE(CONFIG_GCOAP_ENABLE_DTLS)
+        sock_dtls_init();

that's already called by `sys/auto_init/auto_init.c`

> +}
+#endif
+
+#if IS_ACTIVE(CONFIG_GCOAP_ENABLE_DTLS)

```suggestion
}

```

> +#if IS_ACTIVE(CONFIG_GCOAP_ENABLE_DTLS)
+/* Timeout function to free up a session when too many session slots are occupied */
+static void _dtls_free_up_session(void *arg) {
+    (void)arg;
+    sock_dtls_session_t session;
+
+    uint8_t minimum_free = CONFIG_GCOAP_DTLS_MINIMUM_AVAILABLE_SESSIONS;
+    if (dsm_get_num_available_slots() < minimum_free) {
+        if(dsm_get_oldest_used_session(&_sock_dtls, &session) != -1) {
+            /* free up session */
+            dsm_remove(&_sock_dtls, &session);
+            sock_dtls_session_destroy(&_sock_dtls, &session);
+        }
+    }
+}
+#endif

```suggestion
#endif /* CONFIG_GCOAP_ENABLE_DTLS */
```

> +        /* If not enough session slots left: set timeout to free up session */
+        uint8_t minimum_free = CONFIG_GCOAP_DTLS_MINIMUM_AVAILABLE_SESSIONS;
+        if (dsm_get_num_available_slots() < minimum_free)
+        {
+            uint32_t timeout = CONFIG_GCOAP_DTLS_MINIMUM_AVAILABLE_SESSIONS_TIMEOUT_USEC;
+            event_callback_init(&_dtls_session_free_up_tmout_cb,
+                                _dtls_free_up_session, NULL);
+            event_timeout_init(&_dtls_session_free_up_tmout, &_queue,
+                               &_dtls_session_free_up_tmout_cb.super);
+            event_timeout_set(&_dtls_session_free_up_tmout, timeout);
+        }
+    }
+
+    if (type & SOCK_ASYNC_CONN_FIN) {
+        if (sock_dtls_get_event_session(sock, &socket.ctx_dtls_session)) {
+            dsm_remove(sock, &socket.ctx_dtls_session);

No `sock_dtls_session_destroy()` here? 

> +            return;
+        }
+        dsm_state_t prev_state = dsm_store(sock, &socket.ctx_dtls_session,
+                                           SESSION_STATE_ESTABLISHED, false);
+
+        /* If session is already stored and the state was SESSION_STATE_HANDSHAKE
+        before, the handshake has been initiated internally by a gcoap client request
+        and another thread is waiting for the handshake. Send message to the
+        waiting thread to inform about established session */
+        if (prev_state == SESSION_STATE_HANDSHAKE) {
+            msg_t msg = { .type = DTLS_EVENT_CONNECTED };
+            msg_send(&msg, _auth_waiting_thread);
+        } else if (prev_state == NO_SPACE) {
+            /* No space in session management. Should not happen. If it occurs,
+            we lost track of sessions stored in tinydtls internally */
+            sock_dtls_session_destroy(sock, &socket.ctx_dtls_session);

if it shouldn't happen, maybe add some debug output / `assert(false)`

> +        /* Remove all memos of the concerned session. TODO: oberservable memos! */
+        for (int i = 0; i < CONFIG_GCOAP_REQ_WAITING_MAX; i++) {
+            if (_coap_state.open_reqs[i].state == GCOAP_MEMO_UNUSED) {

Do we need to lock `_coap_state.lock` here? 

> @@ -299,7 +446,9 @@ static void _on_resp_timeout(void *arg) {
             return;
         }
 
-        ssize_t bytes = sock_udp_send(&_sock_udp, memo->msg.data.pdu_buf,
+        coap_socket_t socket;
+        _tl_get_socket(&socket);
+        ssize_t bytes = _tl_send(&socket, memo->msg.data.pdu_buf,
                                       memo->msg.data.pdu_len, &memo->remote_ep);

please align ;) 

> @@ -732,6 +881,95 @@ static void _find_obs_memo_resource(gcoap_observe_memo_t **memo,
     }
 }
 
+/*
+ * Transport layer functions
+ */
+
+static void _tl_get_socket(coap_socket_t *sock)

```suggestion
static void _tl_init_socket(coap_socket_t *sock)
```

> +static void _tl_get_socket(coap_socket_t *sock)
+{
+#if IS_ACTIVE(CONFIG_GCOAP_ENABLE_DTLS)
+    sock->type = COAP_SOCKET_TYPE_DTLS;
+    sock->socket.dtls = &_sock_dtls;
+#else
+    sock->type = COAP_SOCKET_TYPE_UDP;
+    sock->socket.udp = &_sock_udp;
+#endif
+}
+
+static ssize_t _tl_send(coap_socket_t *sock, const void *data, size_t len,
+                      const sock_udp_ep_t *remote)
+{
+    int res;
+#if IS_ACTIVE(CONFIG_GCOAP_ENABLE_DTLS)

We should probably also check `sock->type` here so a DTLS enabled client can still send unencrypted requests 

> +#if IS_ACTIVE(CONFIG_GCOAP_ENABLE_DTLS)
+static ssize_t _tl_authenticate(coap_socket_t *sock, const sock_udp_ep_t *remote,
+                                uint32_t timeout)
+{

```suggestion
static ssize_t _tl_authenticate(coap_socket_t *sock, const sock_udp_ep_t *remote,
                                uint32_t timeout)
{
#if !IS_ACTIVE(CONFIG_GCOAP_ENABLE_DTLS)
    return 0;
#else
```

> +        if (timeout != SOCK_NO_TIMEOUT) {
+            uint32_t diff = (xtimer_now_usec() - start);
+            timeout = (diff > timeout) ? 0: timeout - diff;
+            is_timed_out = (res < 0) || (timeout == 0);
+        }

What is going on here?

> @@ -887,8 +1125,16 @@ size_t gcoap_req_send(const uint8_t *buf, size_t len,
         }
     }
 
+    ssize_t res = 0;
+    coap_socket_t socket = { 0 };
+
+    _tl_get_socket(&socket);
+#if IS_ACTIVE(CONFIG_GCOAP_ENABLE_DTLS)

You can drop the `#ifdef` with the change suggested above 

-- 
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/RIOT-OS/RIOT/pull/15549#pullrequestreview-667626720
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.riot-os.org/pipermail/notifications/attachments/20210525/69aaebc9/attachment.htm>


More information about the notifications mailing list