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

benpicco notifications at github.com
Thu May 13 18:24:45 CEST 2021


@benpicco commented on this pull request.

Sorry for not taking a look at this earlier.
There are two conceptual things that irk me, but that might also be due to me not being familiar with the CoAP/DTLS/async socket implementation.

 - Why is DTLS sock not just an extension to UDP sock? The DTLS implementation would then be specific to the network stack (e.g. GNRC), but that would still be preferable to pushing it to the application layer IMHO
 - Why is there a need for `dtls/dsm` - can't we attach the session directly to the socket?



> +    for (uint8_t i=0; i < DTLS_PEER_MAX; i++) {
+        _sessions[i].state = SESSION_STATE_NONE;
+        memset(&_sessions[i], 0, sizeof(dsm_session_t));
+    }

That's redundant as `_sessions` is already `static` and thus initialized with 0.

> +dsm_state_t dsm_store(sock_dtls_t *sock, sock_dtls_session_t *session,
+                      dsm_state_t new_state, bool restore);

Why not extend `sock_dtls` instead? Is this so there can be multiple sessions per socket?

```patch
--- a/pkg/tinydtls/include/sock_dtls_types.h
+++ b/pkg/tinydtls/include/sock_dtls_types.h
@@ -81,6 +81,10 @@ struct sock_dtls {
     dtls_peer_type role;                    /**< DTLS role of the socket */
     sock_dtls_client_psk_cb_t client_psk_cb;/**< Callback to determine PSK credential for session */
     sock_dtls_rpk_cb_t rpk_cb;              /**< Callback to determine RPK credential for session */
+
+    sock_dtls_session_t session;
+    dsm_state_t state;
+    uint64_t last_used;
 };
 
 /**
```

> @@ -110,6 +126,17 @@ static event_queue_t _queue;
 static uint8_t _listen_buf[CONFIG_GCOAP_PDU_BUF_SIZE];
 static sock_udp_t _sock_udp;
 
+#if IS_ACTIVE(CONFIG_GCOAP_ENABLE_DTLS)
+/* DTLS variables and definitions */
+#define SOCK_DTLS_CLIENT_TAG (2)
+
+static sock_dtls_t _sock_dtls;

What if instead we did

```patch
--- a/pkg/tinydtls/include/sock_dtls_types.h
+++ b/pkg/tinydtls/include/sock_dtls_types.h
@@ -39,8 +39,8 @@ extern "C" {
  * @brief Information about DTLS sock
  */
 struct sock_dtls {
+    sock_udp_t udp_sock;                    /**< Underlying UDP sock to use */
     dtls_context_t *dtls_ctx;               /**< TinyDTLS context for sock */
-    sock_udp_t *udp_sock;                   /**< Underlying UDP sock to use */
 #if defined(SOCK_HAS_ASYNC) || defined(DOXYGEN)
     /**
      * @brief   Network stack internal buffer context.
```

And use the `.flags` field to indicate there is a DTLS context attached to the socket? 

Could that save the application from dealing with DTLS entirely (save for calling a DTLS setup function on the DTLS socket) and leave the rest to GNRC?

-- 
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-656119150
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.riot-os.org/pipermail/notifications/attachments/20210513/28554473/attachment.htm>


More information about the notifications mailing list