net/gcoap: support DTLS #15549

janosbrodbeck · 2020-12-03T11:17:47Z

This PR is still WIP. But the core mechanics work. Feedback in every way is very, very welcome!

Contribution description

This PR adds DTLS support in gcoap. It adds the following:

example/gcoap (gcoap-cli) supports DTLS
DTLS session management to support more than one session, hold sessions as long as possible and to ensure that no session gets lost
Unified _sock_send function to abstract udp/dtls away from gcoap
So far no API changes to gcoap. Coap part itself is hardly touched.

But it's still not complete:

Testing of observable resources
Fulfill all CoAP DTLS RFC requirements (e.g. CoAP requests needs to be handled in the same epoch)
Possible additions to the DTLS-API / add some utility functionality to handle a few things easier (e.g., sock_udp_ep_t <-> sock_dtls_session_t, function to get epoch from session)
Adjust documentation
maybe more

And known bug(s) remain:

How is it possible to increase DTLS_PEER_MAX for tinyDTLS? I've seen no file where it is set except the Kconfig file. It appears only in the build-files. Whatever I set, it is always 1 for tinyDTLS.

At the end are some more in depth comments regarding session management and some unchanged parameters.

Thanks to @pokgak for his gcoap/dtls PR, that helped me.

Testing procedure

It's currently only tested between native RIOT nodes and between RIOT and coap-shell.

For testing between two RIOT nodes: flash example/gcoap and send a CoAP get request from the client to the server:
coap get <ip> 5684 /riot/board

For testing between RIOT and coap-shell: flash example/gcoap on one device/native and launch the coap-shell. For native testing:

connect --uri coaps://[<ip>%<interface>] --identity Client_identity --secret secretPSK
get --path /riot/board

Issues/PRs references

Issue #15517 needs to be resolved. For now I've included a workaround commit till it's resolved. Workaround is just for testing purposes.

Relates to #10897

Comments

Session management: Gcoap only stores memos for client requests on client-side and registered observers on server-side. This causes clients to block internal session slots in tinyDTLS if clients crash or sessions are simply not resolved by the clients - the server gets completely blocked if all session slots are occupied. And these sessions/udp endpoints are not stored in any way. Gcoap has to make sure to never lose sessions and to free up slots on its own if neccessary. This sadly costs memory - but I have not found a way around it.

Gcoap now reuses a session if it's already established. If the last session slot is occupied a configurable timeout will start and closes a session after expiration. Then the oldest, in terms of last usage, session will be closed.

Not changing sock_udp in parameters: In the long term it would be nice to support DTLS and UDP connections simultaneously if desired. Therefore I did not want to change any socket parameters. For supporting multiple transport layer we should maybe think about a more unified socket/tl type in gcoap which supports more than DTLS/UDP (if needed) and is also supported by the gcoap API. But that is out of scope of this PR I guess.

miri64

I know this is still WIP, but I think you can get rid of most #ifdef / #if foobar, when using IS_ACTIVE() and relying on the optimizer.

examples/gcoap/gcoap_cli.c

miri64 · 2020-12-03T15:55:01Z

sys/net/application_layer/gcoap/gcoap.c

+#ifdef CONFIG_GCOAP_ENABLE_DTLS
+    #define SOCK_DTLS_CLIENT_TAG (2)
+    static sock_dtls_t _sock_dtls;
+    static kernel_pid_t _main_thread;
+
+    static event_timeout_t _dtls_session_free_up_tmout;
+    static event_callback_t _dtls_session_free_up_tmout_cb;
+#endif


I think you can even drop the preprocessor #if around here and use if (IS_ACTIVE(CONFIG_GCOAP_ENABLE_DTLS)) through-out the code. This way we get rid of a lot of pre-processor magic, which allows you to not hide your code from the compiler in any case.

sys/net/application_layer/gcoap/gcoap.c

examples/gcoap/gcoap_cli.c

examples/gcoap/Makefile

sys/include/net/gcoap/dtls_session_management.h

sys/net/application_layer/gcoap/gcoap.c

pokgak

I don't see gcoap specific code in the dtls_session_management module other than reading the max peer config option. Maybe we could move it out of gcoap and make it an optional module for sock dtls instead?

sys/net/application_layer/gcoap/dtls_session_management.c

pokgak · 2020-12-05T05:22:03Z

How is it possible to increase DTLS_PEER_MAX for tinyDTLS? I've seen no file where it is set except the Kconfig file. It appears only in the build-files. Whatever I set, it is always 1 for tinyDTLS.

Might be a bug when translating the Kconfig option. This is done here. Have you tried setting it manually when building e.g.:

CFLAGS="-DDTLS_PEER_MAX=2" make -C examples/dtls-sock

benpicco

just some high-level comments

sys/Makefile.dep

sys/net/application_layer/gcoap/gcoap.c

sys/net/application_layer/gcoap/dtls_session_management.c

sys/net/application_layer/gcoap/gcoap.c

janosbrodbeck · 2021-02-28T16:50:24Z

After a long time finally a new push. This is my current WIP state.

Introduced _tl_send(), _tl_authenticate(), _tl_get_socket() functions as wrapper for UDP/DTLS
Could not get rid of #ifdef/#if since the compiler did not optimize the DTLS functions out when not used with IS_ACTIVE() in case of not using DTLS. And without providing USEPKG += tinydtls the compiler just complains, that it cannot resolve the DTLS objects.
Kconfig is addressed
Session management (well, at the moment it is still more like session storage) as independent module for sock_dtls

The current state is still experimental. But most issues stem from the sock_dtls implementation of tinydtls. Because of that I've opened an issue to track/mention those. It's a little difficult because this implementation is actually the only real user for sock_dtls/tinydtls and all the problems show up here, but it doesn't really have anything to do with gcoap. Because of that, there are currently two sock_dtls/tinydtls hotfix commits pushed.

Known issues:

On boards I have to send two messages, since the first authentication process always fails. Not reproducible on native. But could only test it on particle-xenon boards and over BLE. Would be nice, if someone else could test it on other boards.

Still missing in gcoap/session management:

Session expiration
DTLS epoch check in the coap process
How to determine which credential to use in client mode for remote server, when using gcoap as coap server and client.

janosbrodbeck · 2021-05-03T11:53:03Z

I've updated and rebased once again (mostly documentation). I remove the WIP state now. The issues with DTLS are not related to gcoap, nonetheless I keep the hotfix pkg/tinydtls commit for easier testing for now. Testing procedure from start post remains. I will also update the start post to the current state soon.

DTLS epoch checking and session expiration is not yet addressed, however, I would consider both to address in a follow-up.

Feedback and review is more than welcome.

benpicco

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?

benpicco · 2021-05-10T21:59:17Z

sys/net/dsm/dsm.c

+    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.

sys/include/net/dsm.h

sys/net/application_layer/gcoap/gcoap.c

janosbrodbeck · 2021-05-13T19:00:11Z

* Why is there a need for `dtls/dsm` - can't we attach the session directly to the socket?

Like mentioned in one conversation comment, the distribution how the maximum number of sessions DTLS_PEER_MAX is distributed to individual sockets is unknown. In some place, currently existing sessions must be held. Keeping actual sessions could of course be done in the implementation of the respective DTLS library. My decision here was to provide a simple generic way to hold sessions without each application/library implementation having to find its own solution.

Additionally, I have a scenario in mind: cross-application session management. Possibly a bit too far thought, but I can definitely see problems if an application can theoretically "occupy" all sessions indefinitely or for a very long time. By this I mean that e.g. DTLS_PEER_MAX sessions are occupied by gcoap. As a result, all other applications using DTLS have no possibility to establish connections or to receive incoming connections on their sockets.

Right now dsm does not help here, but is very easy to extend to get access to sessions stored by other applications and close them to free peer resources. Provided dsm is used for storage by the respective applications.

Although I have built in the mechanism in gcoap for this very reason, so that sessions are automatically freed, but I consider the basic scenario with multiple DTLS applications far from impossible.

sys/include/net/gcoap.h

benpicco

CI still has some style nits so I added my own

sys/net/dsm/dsm.c

benpicco · 2021-07-01T14:19:00Z

sys/net/application_layer/gcoap/gcoap.c

+        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);
+        }


Can you add a comment about this?
Future readers won't be hunting down the PR comment 😉

janosbrodbeck · 2021-07-05T11:29:23Z

I've also renamed dsm_get_oldest_used_session() to dsm_get_least_recently_used_session() to get rid of this awful naming. Thought about using LRU to shorten it, but nah, I think it's more clear so even if it's longer.

benpicco · 2021-07-06T11:04:18Z

Please squash (& rebase)

janosbrodbeck · 2021-07-06T17:50:20Z

Please squash (& rebase)

Squashed and rebased. Also removed the tinyDTLS workaround commit, since it is now merged into master.
There is one change compared to before. I've added the following marked lines:
janosbrodbeck@31518c7#diff-210ffdc67fd874379abf19f95a95fe1f6d7c465aa69e20b6e4fcfd625fbcae18R94-R100

Without this, it could happen that two session slots were added as available for one closed session.

sys/net/dsm/dsm.c

benpicco

I think there is still something wrong here:
When playing around with two native RIOT nodes querying a third native RIOT node with

coap get fe80::7837:fcff:fe7d:1aaf 5684 /.well-known/core

after a while one of the nodes would no longer receive a response. (This is after the dsm: removed session message), the server shows

CoAP server is listening on port 5684
Connection secured with DTLS
Free DTLS session slots: 1/1
 CLI requests sent: 0
CoAP open requests: 0
Configured Proxy: None

The clients meanwhile display

CoAP server is listening on port 5684
Connection secured with DTLS
Free DTLS session slots: 13/1
 CLI requests sent: 17
CoAP open requests: 0
Configured Proxy: None

The number of 'free sessions' increases with each request that does not receive a response.

sys/include/net/gcoap.h

sys/net/application_layer/gcoap/gcoap.c

janosbrodbeck · 2021-07-07T12:36:18Z

I think there is still something wrong here:
The number of 'free sessions' increases with each request that does not receive a response.

Fixed. It was a problem with finding sessions which have been removed but stated them as "already existing", since session state was not taken into account.

benpicco · 2021-07-07T12:44:02Z

Thank you, works now!
Please squash.

janosbrodbeck · 2021-07-07T12:47:04Z

Squashed.

benpicco · 2021-07-07T13:35:01Z

Thank you!

janosbrodbeck · 2021-07-07T13:40:31Z

Thanks for the help and review, everyone!

fjmolinas · 2021-12-09T07:30:31Z

I think there is still something wrong here: When playing around with two native RIOT nodes querying a third native RIOT node with
coap get fe80::7837:fcff:fe7d:1aaf 5684 /.well-known/core
after a while one of the nodes would no longer receive a response. (This is after the dsm: removed session message), the server shows
CoAP server is listening on port 5684
Connection secured with DTLS
Free DTLS session slots: 1/1
 CLI requests sent: 0
CoAP open requests: 0
Configured Proxy: None
The clients meanwhile display
CoAP server is listening on port 5684
Connection secured with DTLS
Free DTLS session slots: 13/1
 CLI requests sent: 17
CoAP open requests: 0
Configured Proxy: None
The number of 'free sessions' increases with each request that does not receive a response.

I was trying to test #17141, and was for this using the examples/gcoap_dtls application, for some reason I could not get it to work between real BOARDS and with native I could, I went back to this PR to double check and I experience the same issue, might I be missing something? I'm flashing the cpplication to 2 BOARDS and the performing a get on /.well-known/core

janosbrodbeck requested review from cgundogan, haukepetersen, leandrolanzieri, miri64 and PeterKietzmann as code owners December 3, 2020 11:17

janosbrodbeck added Area: CoAP Area: Constrained Application Protocol implementations State: WIP State: The PR is still work-in-progress and its code is not in its final presentable form yet labels Dec 3, 2020

benpicco requested a review from chrysn December 3, 2020 11:31

miri64 requested a review from pokgak December 3, 2020 12:05

miri64 previously requested changes Dec 3, 2020

View reviewed changes

miri64 reviewed Dec 3, 2020

View reviewed changes

examples/gcoap/gcoap_cli.c Outdated Show resolved Hide resolved

miri64 reviewed Dec 3, 2020

View reviewed changes

examples/gcoap/Makefile Outdated Show resolved Hide resolved

miri64 reviewed Dec 3, 2020

View reviewed changes

examples/gcoap/Makefile Outdated Show resolved Hide resolved

akshaim reviewed Dec 4, 2020

View reviewed changes

examples/gcoap/Makefile Outdated Show resolved Hide resolved

pokgak reviewed Dec 5, 2020

View reviewed changes

sys/include/net/gcoap/dtls_session_management.h Outdated Show resolved Hide resolved

sys/net/application_layer/gcoap/gcoap.c Outdated Show resolved Hide resolved

pokgak reviewed Dec 5, 2020

View reviewed changes

sys/net/application_layer/gcoap/dtls_session_management.c Outdated Show resolved Hide resolved

sys/net/application_layer/gcoap/dtls_session_management.c Outdated Show resolved Hide resolved

sys/net/application_layer/gcoap/dtls_session_management.c Outdated Show resolved Hide resolved

benpicco reviewed Dec 6, 2020

View reviewed changes

janosbrodbeck force-pushed the gcoap/pr/dtls branch from f482ca2 to e46166c Compare February 28, 2021 16:48

janosbrodbeck requested a review from jia200x as a code owner February 28, 2021 16:48

janosbrodbeck mentioned this pull request Feb 28, 2021

pkg/tinydtls: Multiple issues #16108

Open

5 tasks

janosbrodbeck force-pushed the gcoap/pr/dtls branch from 9dff2d3 to 45a064a Compare May 3, 2021 11:48

janosbrodbeck removed the State: WIP State: The PR is still work-in-progress and its code is not in its final presentable form yet label May 3, 2021

benpicco added Type: new feature The issue requests / The PR implemements a new feature for RIOT Area: security Area: Security-related libraries and subsystems CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR labels May 3, 2021

benpicco reviewed May 13, 2021

View reviewed changes

benpicco reviewed Jun 21, 2021

View reviewed changes

sys/include/net/gcoap.h Outdated Show resolved Hide resolved

benpicco reviewed Jun 21, 2021

View reviewed changes

sys/include/net/gcoap.h Show resolved Hide resolved

benpicco reviewed Jun 21, 2021

View reviewed changes

sys/include/net/gcoap.h Outdated Show resolved Hide resolved

benpicco reviewed Jun 21, 2021

View reviewed changes

sys/include/net/gcoap.h Outdated Show resolved Hide resolved

MrKevinWeiss added this to the Release 2021.07 milestone Jun 22, 2021

benpicco reviewed Jul 1, 2021

View reviewed changes

janosbrodbeck force-pushed the gcoap/pr/dtls branch from 08ea058 to c38bec4 Compare July 6, 2021 17:44

github-actions bot removed the Area: pkg Area: External package ports label Jul 6, 2021

benpicco reviewed Jul 7, 2021

View reviewed changes

sys/net/dsm/dsm.c Outdated Show resolved Hide resolved

benpicco reviewed Jul 7, 2021

View reviewed changes

sys/include/net/gcoap.h Outdated Show resolved Hide resolved

sys/net/application_layer/gcoap/gcoap.c Outdated Show resolved Hide resolved

benpicco approved these changes Jul 7, 2021

View reviewed changes

janosbrodbeck added 3 commits July 7, 2021 14:45

net/dtls/dsm: add DTLS session management module

5567f13

net/gcoap: support DTLS

23a8659

example/gcoap: add DTLS support

0a8eaea

janosbrodbeck force-pushed the gcoap/pr/dtls branch from 7f2ce6f to 0a8eaea Compare July 7, 2021 12:45

benpicco merged commit eacbaf5 into RIOT-OS:master Jul 7, 2021

janosbrodbeck deleted the gcoap/pr/dtls branch July 8, 2021 12:13

MrKevinWeiss removed this from the Release 2021.07 milestone Jul 15, 2021

miri64 mentioned this pull request Jul 27, 2021

gcoap: multi-transport support #16688

Merged

2 tasks

benpicco added this to the Release 2021.10 milestone Oct 20, 2021

kfessel mentioned this pull request Feb 20, 2023

gcoap: add DTLS integration #12104

Closed

net/gcoap: support DTLS #15549

net/gcoap: support DTLS #15549

Uh oh!

Conversation

janosbrodbeck commented Dec 3, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Contribution description

Testing procedure

Issues/PRs references

Comments

Uh oh!

miri64 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

miri64 Dec 3, 2020

Choose a reason for hiding this comment

Uh oh!

miri64 Dec 3, 2020

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

pokgak left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

pokgak commented Dec 5, 2020

Uh oh!

benpicco left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

janosbrodbeck commented Feb 28, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

janosbrodbeck commented May 3, 2021

Uh oh!

benpicco left a comment

Choose a reason for hiding this comment

Uh oh!

benpicco May 10, 2021

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

janosbrodbeck commented May 13, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

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!

Uh oh!

Uh oh!

benpicco Jul 1, 2021

Choose a reason for hiding this comment

Uh oh!

janosbrodbeck commented Jul 5, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

benpicco commented Jul 6, 2021

Uh oh!

janosbrodbeck commented Jul 6, 2021

Uh oh!

janosbrodbeck commented Dec 3, 2020 •

edited

Loading

janosbrodbeck commented Feb 28, 2021 •

edited

Loading

janosbrodbeck commented May 13, 2021 •

edited

Loading

benpicco left a comment •

edited

Loading

janosbrodbeck commented Jul 5, 2021 •

edited

Loading

benpicco left a comment •

edited

Loading