net/sock/dtls: allow using multiple credentials by leandrolanzieri · Pull Request #16179 · RIOT-OS/RIOT

leandrolanzieri · 2021-03-10T12:48:17Z

Contribution description

Currently only one credential can be associated to each DTLS sock. This limits certain use cases that need to establish multiple connections on a single sock using potentially multiple credentials.

This PR proposes an extension of the sock DTLS API to allow registering multiple credentials to be used on a single sock. An application may register callbacks to specify a certain credential from the list based on the session information (and, when using PSK, based on the hint that may have been sent).

Additionally, now a server can send a PSK Identity hint to a client during the handshake, to help deciding on the PSK Identity that should be used.

Finally, the example application has been extended to show these functionalities.

Testing procedure

The example application should be working both when using PSK and ECC (for more information enable the debug messages in the sock_dtls.c file).
Check that the correct hint is being sent in PSK
Check that the correct RPK is being used in ECC
Read the documentation
A single credential use case should still work seamlessly

Issues/PRs references

None so far

pkg/tinydtls/contrib/sock_dtls.c

leandrolanzieri · 2021-03-15T12:15:11Z

@janosbrodbeck the new definitions are not split into their own header, this should avoid the issues when using Sock Async.

pokgak

Looks good overall.

pkg/tinydtls/contrib/sock_dtls.c

janosbrodbeck

It works now with async sockets!
tests/pkg_tinydtls_sock_async/dtls-server.c needs one change, because the PSK matching changed. The server in the test does not provide an identity for the credential. Don't know why the ID is missing but now it leads with the current changes to not being able to find a matching credential, since it compares with an empty identity.

static const uint8_t psk_id_0[] = PSK_DEFAULT_IDENTITY; needs to be added to pkg_tinydtls_sock_async/dtls-server.c. And the ID then needs to be added to the credential.

janosbrodbeck · 2021-03-16T07:28:53Z

Looks good to me. Existing code is still working (except the test, but I'll leave that out) and the API is easy to use. Already have a WIP build for my gcoap-dtls running which supports the new functionality.

However, I am for a small extension in sock_dtls_create() as I found a small issue:

When creating a new DTLS socket it is mandatory to provide a credman tag, as it includes sock_dtls_add_credential(). That was okay when there could only be one credential per socket. For gcoap I defined a pre-defined tag and the user could add their credential and tag it with the gcoap tag. But now there is no need for that anymore, I would prefer to have the possibility to create a socket with an empty tag list. The user has to create the credential anyway, and I see no reason to make the usage of a pre-defined tag mandatory. I would like to leave the tag creation up to the user.

But the current API does not provide that functionality. It should be fairly easy to achieve, since CREDMAN_TAG_EMPTY already exists. We should parse this option in sock_dtls_create() to allow the creation of a socket with an empty credential list. And should also add this possibility to the documentation.

leandrolanzieri · 2021-03-17T13:10:28Z

static const uint8_t psk_id_0[] = PSK_DEFAULT_IDENTITY; needs to be added to pkg_tinydtls_sock_async/dtls-server.c. And the ID then needs to be added to the credential.

Added in 26cf8cf.

But the current API does not provide that functionality. It should be fairly easy to achieve, since CREDMAN_TAG_EMPTY already exists. We should parse this option in sock_dtls_create() to allow the creation of a socket with an empty credential list. And should also add this possibility to the documentation.

Added in b051dae.

pkg/tinydtls/contrib/sock_dtls.c

pokgak

Almost done. Just need to update the docs.

sys/include/net/sock/dtls.h

pokgak · 2021-03-18T22:57:41Z

sys/include/net/sock/dtls.h

+ * registering a callback with @ref sock_dtls_set_client_psk_cb. If no callback is registered the
+ * credential is chosen as follows: if a hint is sent by the server, all credentials registered to
+ * the sock are checked for a matching @ref psk_params_t::hint "hint". A credential is select on
+ * matching hint. If no credential matches the hint or no hint is provided, the first PSK credential


Not sure what exactly needs to change here. I made some adaptions to the doc in 74ea508. Could you take a look?

janosbrodbeck

Besides the documentation updates that pokgak suggested, I am very happy.

Tested it again with examples/dtls-sock, tests/pkg_tinydtls_sock_async and my gcoap-dtls PR without adaption changes and with some first integration changes. The right credentials are used, and existing single credential use cases are also working without changes.

leandrolanzieri · 2021-03-19T08:12:29Z

I added another function to get a read-only array of the registered credentials on a sock.

pokgak

You can squash now.

leandrolanzieri · 2021-03-22T08:16:42Z

Any maintainer that would like to take a look? @miri64? @cgundogan?

PeterKietzmann · 2021-03-30T15:43:05Z

@leandrolanzieri thanks, looks better! Maybe introduce PSK and RPK as abbreviations. Mention that the hint (string) could also be changed, you could also explain that it is printed on the client (for verification by the user). I leave this up to you... Please squash directly

leandrolanzieri · 2021-03-31T09:31:34Z

@leandrolanzieri thanks, looks better! Maybe introduce PSK and RPK as abbreviations. Mention that the hint (string) could also be changed, you could also explain that it is printed on the client (for verification by the user). I leave this up to you... Please squash directly

Added the suggestions and squashed directly!

sys/include/net/sock/dtls/creds.h

PeterKietzmann

ACK from my side. Anyone else who wants to give this PR a look before merge?

PeterKietzmann · 2021-04-06T08:04:41Z

Well then, go!

leandrolanzieri added Area: network Area: Networking Type: new feature The issue requests / The PR implemements a new feature for RIOT Area: security Area: Security-related libraries and subsystems labels Mar 10, 2021

leandrolanzieri requested review from cgundogan, janosbrodbeck, miri64 and pokgak March 10, 2021 12:48

leandrolanzieri requested review from jia200x and maribu as code owners March 10, 2021 12:48

leandrolanzieri force-pushed the dev/net/sock/dtls_multi_cred branch 2 times, most recently from d47f275 to 198c519 Compare March 10, 2021 13:08

leandrolanzieri commented Mar 11, 2021

View reviewed changes

pkg/tinydtls/contrib/sock_dtls.c Show resolved Hide resolved

leandrolanzieri force-pushed the dev/net/sock/dtls_multi_cred branch from 198c519 to aaecc9c Compare March 15, 2021 12:14

pokgak suggested changes Mar 16, 2021

View reviewed changes

pkg/tinydtls/contrib/sock_dtls.c Show resolved Hide resolved

pkg/tinydtls/contrib/sock_dtls.c Outdated Show resolved Hide resolved

pkg/tinydtls/contrib/sock_dtls.c Outdated Show resolved Hide resolved

janosbrodbeck suggested changes Mar 16, 2021

View reviewed changes

leandrolanzieri requested review from MichelRottleuthner, aabadie, fjmolinas and smlng as code owners March 17, 2021 13:08

janosbrodbeck suggested changes Mar 18, 2021

View reviewed changes

pkg/tinydtls/contrib/sock_dtls.c Show resolved Hide resolved

pokgak suggested changes Mar 18, 2021

View reviewed changes

janosbrodbeck approved these changes Mar 19, 2021

View reviewed changes

pokgak approved these changes Mar 19, 2021

View reviewed changes

leandrolanzieri force-pushed the dev/net/sock/dtls_multi_cred branch 2 times, most recently from 6187e42 to 1f6e68c Compare March 19, 2021 10:06

leandrolanzieri force-pushed the dev/net/sock/dtls_multi_cred branch from 4a153ea to f2dda22 Compare March 31, 2021 09:31

leandrolanzieri added the CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR label Mar 31, 2021

leandrolanzieri force-pushed the dev/net/sock/dtls_multi_cred branch from f2dda22 to a5ccb9a Compare March 31, 2021 11:25

PeterKietzmann reviewed Apr 1, 2021

View reviewed changes

sys/include/net/sock/dtls/creds.h Outdated Show resolved Hide resolved

PeterKietzmann approved these changes Apr 1, 2021

View reviewed changes

leandrolanzieri added 10 commits April 1, 2021 09:47

net/sock/dtls: allow to set PSK Identity hint

d7440ce

net/sock/dtls: allow to register multiple credentials into a sock

62fb9ad

net/sock/dtls: add client PSK callback for credential selection

81892ee

net/sock/dtls: add RPK callback for credential selection

8b57b87

net/sock/dtls: add documentation on multi-credential

95fd4f9

pkg/tinydtls/sock: allow multiple PSK credentials

1c1c5f1

pkg/tinydtls/sock: allow multiple RPK credentials

bccfb6e

examples/dtls-sock: add multiple credentials

cff9a54

net/sock/dtls: allow CREDMAN_TAG_EMPTY on sock creation

6428505

tests/pkg_tinydtls_sock_async: add missing ID to server PSK

835589a

leandrolanzieri force-pushed the dev/net/sock/dtls_multi_cred branch from a5ccb9a to 835589a Compare April 1, 2021 07:48

fjmolinas added CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR and removed CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR labels Apr 1, 2021

PeterKietzmann merged commit f348207 into RIOT-OS:master Apr 6, 2021

leandrolanzieri deleted the dev/net/sock/dtls_multi_cred branch April 6, 2021 08:07

kaspar030 added this to the Release 2021.04 milestone Apr 23, 2021

leandrolanzieri added the Release notes: added Set on PRs that have been processed into the release notes for the current release. label Apr 28, 2021

Conversation

leandrolanzieri commented Mar 10, 2021

Contribution description

Testing procedure

Issues/PRs references

Uh oh!

Uh oh!

leandrolanzieri commented Mar 15, 2021

Uh oh!

pokgak left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

janosbrodbeck 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!

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

Uh oh!

Uh oh!

leandrolanzieri commented Mar 17, 2021

Uh oh!

Uh oh!

pokgak left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

pokgak Mar 18, 2021

Choose a reason for hiding this comment

Uh oh!

leandrolanzieri Mar 19, 2021

Choose a reason for hiding this comment

Uh oh!

janosbrodbeck 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!

leandrolanzieri commented Mar 19, 2021

Uh oh!

pokgak left a comment

Choose a reason for hiding this comment

Uh oh!

leandrolanzieri commented Mar 22, 2021

Uh oh!

PeterKietzmann commented Mar 30, 2021

Uh oh!

leandrolanzieri commented Mar 31, 2021

Uh oh!

Uh oh!

PeterKietzmann left a comment

Choose a reason for hiding this comment

Uh oh!

PeterKietzmann commented Apr 6, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

janosbrodbeck left a comment •

edited

Loading

janosbrodbeck commented Mar 16, 2021 •

edited

Loading

janosbrodbeck left a comment •

edited

Loading