net/sock_dtls: add public key verification by leandrolanzieri · Pull Request #20048 · RIOT-OS/RIOT

leandrolanzieri · 2023-11-04T16:45:35Z

Contribution description

This adds a pseudomodule to enable public key verification of peers. When sock_dtls_verify_public_key is enabled the public key sent by the peer is checked against the registered keys on the corresponding sock. The implementation for tinydtls is added, as well as the corresponding changes to use it on the dtls-sock example.

Testing procedure

Run examples/dtls-sock with ECC enabled, the public key should be verified. You can try removing the server's public key from the client, the connection should be aborted.

Issues/PRs references

Loosely related to #19838

leandrolanzieri · 2023-11-06T09:17:47Z

I'm wondering whether this feature should actually be opt-out instead.

riot-ci · 2024-01-31T23:10:38Z

Murdock results

✔️ PASSED

6d9a9a3 examples/dtls-sock: cleanup credentials

Success	Failures	Total	Runtime
10009	0	10009	07m:27s

Artifacts

Teufelchen1

Some nits. Manually and successfully tested on native.

I found the code in the example to be confusing due to naming of the key variables and lack of comments. If you want to go the extra mile and and a bit of ✨ that would be neat but totally optional for this PR.

pkg/tinydtls/contrib/sock_dtls.c

leandrolanzieri · 2024-02-01T16:16:10Z

I found the code in the example to be confusing due to naming of the key variables and lack of comments. If you want to go the extra mile and and a bit of ✨ that would be neat but totally optional for this PR.

I gave it a try. It's still not perfect, but I think much clearer. I tried clearly splitting both sets of credentials. A bonus: now the client can actively use the PSK hint to decide on the key to use.

benpicco · 2024-02-01T16:39:04Z

examples/dtls-sock/dtls-client.c

-    }
+    if (IS_ACTIVE(CONFIG_DTLS_PSK)) {
+        /* make the new credential available to the sock */
+        if (sock_dtls_add_credential(&dtls_sock, SOCK_DTLS_CLIENT_TAG_1) < 0) {


I was always very confused about what's the idea behind credman.
Shouldn't a credential be associated with a host? So what does it mean to add multiple credentials to a socket?

At first I thought it would do the host <-> credential mapping, but that's not the case.
So what's the advantage of the credential IDs ('tags') as opposed to just passing pointers?

I was always very confused about what's the idea behind credman.

Yeah, I know what you mean...

Shouldn't a credential be associated with a host? So what does it mean to add multiple credentials to a socket?

Not necessarily. I mean, you can already see the example of the server with more than one pair of keys. IMO one of the greatest problems of this module is that a credential is a single "private key" and 0 or more "client keys". Like I commented in the example, the way it currently works looks more like a keyring of client keys and your own.

So what's the advantage of the credential IDs ('tags') as opposed to just passing pointers?

In key management using tags is not uncommon, this is crucial for instance when integrating secure elements or other backends where the key is directly not reachable, but a tag is passed to the crypto backend. I'm not sure why this was the case for credman, as it clearly doesn't support any other backend than a buffer in memory.

IMO we are in need of a rethinking of the credentials management in RIOT. We would really benefit from:

not only DTLS credentials (of course)

possibility of multiple backends including secure elements

more types of credentials

ability to indicate scope and capabilities of the credentials

Hey @Einhornhool! Take a look at this comment please 🐅
(Also, maybe this comment should be an issue instead?)

Hmm most of the points in Leandros credentials requirement list could be handled by the PSA Crypto key management. It might be worth looking into patching tinydtls to use PSA for crypto operations and credential storage, instead of implementing the same features twice.

benpicco · 2024-02-01T19:20:46Z

This needs a rebase

leandrolanzieri · 2024-02-01T22:56:13Z

Rebased. I also modified a bit the example: the server only stores one client public key, but has two key pairs. It allows the user to select which one to use on the handshake. As the client knows both public keys form the server, any will work. I think this showcases a bit better the usage of the callbacks and multiple credentials. I also updated the docs including this and the new command.

Teufelchen1 · 2024-02-09T10:43:05Z

This is great! Way easier to understand!
Could you fix the format issues in the *credentials.h files?
Don't forget to squash!

leandrolanzieri · 2024-02-09T10:48:14Z

Fixed and squashed!

benpicco · 2024-02-09T10:49:56Z

examples/dtls-sock/dtls-client.c

-#endif

-    res = sock_dtls_establish_session(&udp_sock, &dtls_sock, &session, tag,
-                                      &local, &remote, buf, sizeof(buf));


Why move away from the sock_dtls_establish_session() helper and open-code the handshake again?

Because it doesn't allow me to register the extra credentials before initiating the handshake.

Same with the callbacks

tbh I didn't quite understand the requirement when adding that helper, so if you think the API can be improved that would be great - leaving the handshake to the user seemed like a bad example.

Hm I don't feel like this example is bad. It's not overly complicated or lengthy, the reader should get along well.
While a single helper function is nice and clean, it is clear the existing one is insufficient for this particular task. Adjusting that helper would mess with an API that is out of scope for this PR.

Can we resolve this comment on these terms?

Do you think the API needs a rework? If so, we should open an issue for that.

Makes me wonder, should we integrate credman closer with dtls?

Sure, this shouldn't block this PR, I just took the opportunity to try to get some open questions about credman / DTLS answered 😅

I'd very much welcome an overhaul of credman as it seems still a bit unclear what it actually is and what's it's purpose.

Maybe it would also fit into PSA crypto?

sys/include/net/sock/dtls.h

benpicco · 2024-02-12T10:49:29Z

examples/dtls-sock/dtls_client_credentials.h

+/**
+ * @brief The identity hint of the PSK 0
+ */
+#define CLIENT_PSK_IDENTITY_0_HINT "Prefer_Id_0"


Would the hint be something like a hostname / node ID or serial number?

This is actually application-specific (https://datatracker.ietf.org/doc/html/rfc4279#section-5.2), so the content is not really restricted as long as it can be understood by the peer.

Sure, but since this is an example I think some comment of what this would be in a real-world use case would be valuable.

something like a hostname / node ID or serial number

This would somehow imply that the key is only to be used with a single client, this is not necessarily the case. Searching for examples I found this summary: https://stackoverflow.com/a/8753393/5633371 seems that many implementations send encoded information on how to derive the keys.

I think this hint is fine for the didactic purpose of the example: It's easily readable in code and when sniffing the handshake, and it describes its intention.

leandrolanzieri · 2024-03-15T10:39:16Z

Ping?

benpicco

Thank you for the ping.

@Teufelchen1 I see you approved this, any reason not to press the merge button?

examples/dtls-sock/Makefile

leandrolanzieri · 2024-03-19T14:59:43Z

Thanks for the review!

leandrolanzieri added the Type: new feature The issue requests / The PR implemements a new feature for RIOT label Nov 4, 2023

leandrolanzieri requested a review from benpicco November 4, 2023 16:45

leandrolanzieri requested a review from maribu as a code owner November 4, 2023 16:45

github-actions bot added Area: network Area: Networking Area: build system Area: Build system Area: pkg Area: External package ports Area: sys Area: System Area: examples Area: Example Applications labels Nov 4, 2023

leandrolanzieri mentioned this pull request Nov 4, 2023

sock_dtls, dsm: Add possibility to determine the credentials that were used for a DTLS connection #19838

Open

benpicco requested a review from Teufelchen1 January 31, 2024 21:27

benpicco added the CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR label Jan 31, 2024

Teufelchen1 requested changes Feb 1, 2024

View reviewed changes

pkg/tinydtls/contrib/sock_dtls.c Outdated 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

benpicco reviewed Feb 1, 2024

View reviewed changes

leandrolanzieri force-pushed the pr/tinydtls/check_public_key branch from 30a9e0e to 01b6e77 Compare February 1, 2024 22:53

leandrolanzieri requested a review from jia200x as a code owner February 1, 2024 22:53

github-actions bot added the Area: doc Area: Documentation label Feb 1, 2024

leandrolanzieri force-pushed the pr/tinydtls/check_public_key branch from 01b6e77 to a57bdb7 Compare February 9, 2024 10:47

benpicco reviewed Feb 9, 2024

View reviewed changes

leandrolanzieri force-pushed the pr/tinydtls/check_public_key branch from a57bdb7 to 4321cf1 Compare February 9, 2024 13:49

Teufelchen1 approved these changes Feb 12, 2024

View reviewed changes

benpicco reviewed Feb 12, 2024

View reviewed changes

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

benpicco reviewed Feb 12, 2024

View reviewed changes

leandrolanzieri force-pushed the pr/tinydtls/check_public_key branch from 4321cf1 to c424a76 Compare February 26, 2024 16:06

benpicco reviewed Mar 16, 2024

View reviewed changes

examples/dtls-sock/Makefile Show resolved Hide resolved

Teufelchen1 enabled auto-merge March 18, 2024 12:32

Teufelchen1 added this pull request to the merge queue Mar 18, 2024

Teufelchen1 reviewed Mar 18, 2024

View reviewed changes

examples/dtls-sock/Makefile Show resolved Hide resolved

Teufelchen1 removed this pull request from the merge queue due to a manual request Mar 18, 2024

sys/net/sock_dtls: add public key verification

868d839

leandrolanzieri force-pushed the pr/tinydtls/check_public_key branch from 082638b to 9b18399 Compare March 19, 2024 06:45

leandrolanzieri added 2 commits March 19, 2024 07:49

examples/dtls-sock: add public key verification

f2b6d28

examples/dtls-sock: cleanup credentials

6d9a9a3

leandrolanzieri force-pushed the pr/tinydtls/check_public_key branch from 9b18399 to 6d9a9a3 Compare March 19, 2024 06:50

Teufelchen1 added this pull request to the merge queue Mar 19, 2024

Merged via the queue into RIOT-OS:master with commit 741d6b3 Mar 19, 2024

leandrolanzieri deleted the pr/tinydtls/check_public_key branch March 19, 2024 14:59

MrKevinWeiss added this to the Release 2024.04 milestone Apr 30, 2024

Conversation

leandrolanzieri commented Nov 4, 2023

Contribution description

Testing procedure

Issues/PRs references

Uh oh!

leandrolanzieri commented Nov 6, 2023

Uh oh!

riot-ci commented Jan 31, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Murdock results

Artifacts

Uh oh!

Teufelchen1 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

leandrolanzieri commented Feb 1, 2024

Uh oh!

benpicco Feb 1, 2024 • 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

benpicco commented Feb 1, 2024

Uh oh!

leandrolanzieri commented Feb 1, 2024

Uh oh!

Teufelchen1 commented Feb 9, 2024

Uh oh!

leandrolanzieri commented Feb 9, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

leandrolanzieri commented Mar 15, 2024

Uh oh!

benpicco left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

leandrolanzieri commented Mar 19, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

riot-ci commented Jan 31, 2024 •

edited

Loading

benpicco Feb 1, 2024 •

edited

Loading