pkg/tinydtls: add DTLS sock API implementation by pokgak · Pull Request #11943 · RIOT-OS/RIOT

pokgak · 2019-07-31T09:56:25Z

Contribution description

This PR implements the DTLS sock API introduced in #11909 using tinyDTLS. It adds an example application dtls-sock that follows the same behavior as the existing dtls-echo example.

Testing procedure

Run the example application using two nodes.

Start a DTLS server on one node and get the IP:

> dtlss start
> ifconfig
[...]

Send a message from the other node to the server:

> dtlsc <IP server> <message>

PSK credentials are used by default. To test with ECC credentials, uncomment following line in Makefile:

# CFLAGS += -DDTLS_ECC

Using ECC credentials is a bit iffy currently. It works but takes too long (>10s) to send and receive the data. Probably the same problem as #11859.

Issues/PRs references

Depends on PR #11909.
Earlier discussion in #10897

rfuentess · 2019-08-05T10:57:07Z

I can start to review the PR until this weekend. Is it OK ?

rfuentess · 2019-08-05T10:58:43Z

By the way, @pokgak can you put the link to the previous discussion ?

miri64 · 2019-08-05T11:00:18Z

I can start to review the PR until this weekend. Is it OK ?

#11909 needs to be reviewed and ACK'd first, so it still has some time (sorry, forgot to set the "State: waiting for other PR" label)

pokgak · 2019-08-06T12:28:28Z

@rfuentess, it'd be great if you could review it, there are some places where I'm not really sure about the implementation but as @miri64 said we still have time. In the meantime, I've updated #10897 with a list to track the current status.

pokgak · 2019-08-13T13:35:21Z

Added commits to update the implementation to use the latest API. There are not much changes to the implementation of the API in sock_dtls.c aside from function renaming and removal of sock_dtls_init_server().

I think there will no changes anymore to the API proposed in #11909, only refining the example left. So it should be okay to start reviewing this PR @rfuentess .

rfuentess · 2019-08-15T16:12:29Z

I finally got some free time.

The interface seems to be elegant and clean. At simple glance I don't see anything that requires special attention.

I'll try to get some physical boards on the following weeks to play with it. Otherwise, I'll recover my IoT-lab account for it.

miri64 · 2019-08-28T03:40:02Z

I think with #11909 merged this can be rebased.

miri64 · 2019-08-28T09:10:41Z

I guess it also needs some adaptation to the changes we discussed there.

examples/dtls-sock/dtls-client.c

examples/dtls-sock/server_keys.h

examples/dtls-sock/dtls-client.c

miri64 · 2019-09-24T18:38:45Z

Please rebase, #12299 was merged.

miri64 · 2019-09-25T09:06:52Z

As 894ffe5 still does exist, I believe you did not rebase to current master ;-)

pokgak · 2019-09-25T09:08:21Z

@rfuentess update on the review? I'm looking into constant-time when copying the credentials in credential callbacks (get_psk(), get_ecc()). So that part may still change.

pokgak · 2019-09-25T09:23:31Z

As 894ffe5 still does exist, I believe you did not rebase to current master ;-)

No, that is not a workaround for the error stated in comment. After #12299 merged, compiling the example gives following error:

error: assignment discards 'const' qualifier from pointer target type

894ffe5 is needed to fix it. And I checked git log just in case and #12299 is in the history.

pokgak · 2019-11-05T15:23:39Z

cppcheck error fixed and squashed commits. I need to readd b7d0355 (originally included in #11909) because it somehow got removed from master.

tcschmidt · 2019-11-27T14:02:52Z

@smlng are your requests competed? Then, please remove the blocking.

needs testing

smlng · 2019-11-27T15:29:16Z

please squash

pokgak · 2019-11-28T12:55:50Z

Squashed.

smlng

tested and works in general, however I observed 2 things:

cannot start, then stop and start server again, fails with:

dtlss stop
 dtlss stop
Stopping server...
Terminating
Success: DTLS server stopped
> dtlss start
dtlss start
Error cannot add credential to system: -1

further it would be nice to print the message send by dtlsc on server side and/or on client side (after echo). Currently both only report that a message was received but the content is not shown.

pokgak · 2019-11-29T15:10:10Z

* cannot start, then stop and start server again, fails with:

Fixed.

* further it would be nice to print the message send by `dtlsc` on server side and/or on client side (after echo). Currently both only report that a message was received but the content is not shown.

I had this discussion with @miri64 before (#11909 (comment)) and there decided against printing the message.

smlng · 2019-12-02T20:06:45Z

please squash, otherwise ACK.

pokgak · 2019-12-05T13:19:10Z

@smlng sorry for the delay.

I tested part of the tests from #11943 (comment) again on native and all OK:

-------- -------------- -------- -------------- ---------------------
client   cipher suite   server   cipher suite   result
                                                ---------------------
                                                native 
-------- -------------- -------- -------------- ---------------------
sock     ecc            sock     ecc            OK     
sock     psk            sock     psk            OK    
sock     ecc            sock     psk            OK 1* 
sock     psk            sock     ecc            OK 1* 

1*: client got handshake failure as expected

smlng · 2019-12-05T14:23:09Z

okay ... cool, lets see what murdock thinks

benpicco · 2019-12-05T14:42:05Z

Murdock is happy and so am I 😃

smlng · 2019-12-05T14:47:45Z

well then let's do this!

rfuentess · 2019-12-05T14:56:05Z

Congrats @pokgak !

pokgak · 2019-12-05T16:13:58Z

Thank you all for helping with the review :)

benpicco · 2021-05-13T15:38:09Z

pkg/tinydtls/include/sock_dtls_types.h

+struct sock_dtls {
+    dtls_context_t *dtls_ctx;               /**< TinyDTLS context for sock */
+    sock_udp_t *udp_sock;                   /**< Underlying UDP sock to use */


What was the reason for not doing

Suggested change

struct sock_dtls {

dtls_context_t *dtls_ctx; /**< TinyDTLS context for sock */

sock_udp_t *udp_sock; /**< Underlying UDP sock to use */

sock_udp_t udp_sock; /**< Underlying UDP sock to use */

dtls_context_t *dtls_ctx; /**< TinyDTLS context for sock */

instead?

That way application code using UDP sockets could transparently handle DTLS sockets (after a custom setup procedure).
sock_udp_t would only need to be extended by a flag to indicate there is a DTLS context attached to it.

We might want to support other transports in the future. With that, it would be easier and better understandable to create the transport sock in the application first and then the DTLS sock. As we do not want to copy that transport sock, a pointer is the best way to go here.

pokgak mentioned this pull request Jul 31, 2019

sys: new sock submodule for DTLS #11909

Merged

miri64 requested review from rfuentess and smlng August 5, 2019 09:34

miri64 self-assigned this Aug 5, 2019

miri64 added Area: pkg Area: External package ports Area: security Area: Security-related libraries and subsystems Type: enhancement The issue suggests enhanceable parts / The PR enhances parts of the codebase / documentation labels Aug 5, 2019

miri64 added this to the Release 2019.10 milestone Aug 5, 2019

miri64 added the State: waiting for other PR State: The PR requires another PR to be merged first label Aug 5, 2019

pokgak mentioned this pull request Aug 6, 2019

sock_dtls: a DTLS sock API #10897

Open

8 tasks

pokgak force-pushed the sock_dtls_impl branch from ad3728c to c777bc3 Compare August 28, 2019 09:58

pokgak mentioned this pull request Aug 28, 2019

gcoap: add DTLS integration #12104

Closed

miri64 reviewed Aug 29, 2019

View reviewed changes

examples/dtls-sock/dtls-client.c Outdated Show resolved Hide resolved

miri64 reviewed Aug 29, 2019

View reviewed changes

examples/dtls-sock/server_keys.h Outdated Show resolved Hide resolved

miri64 reviewed Aug 30, 2019

View reviewed changes

examples/dtls-sock/dtls-client.c Outdated Show resolved Hide resolved

examples/dtls-sock/dtls-client.c Outdated Show resolved Hide resolved

examples/dtls-sock/dtls-client.c Outdated Show resolved Hide resolved

miri64 removed the State: waiting for other PR State: The PR requires another PR to be merged first label Aug 30, 2019

pokgak mentioned this pull request Sep 24, 2019

credman: change credman_buffer_t::s type to 'const void *' #12299

Merged

pokgak force-pushed the sock_dtls_impl branch from 653e89a to 894ffe5 Compare September 24, 2019 18:57

pokgak force-pushed the sock_dtls_impl branch from 692429c to c6517a2 Compare November 27, 2019 12:30

pokgak added 2 commits November 28, 2019 13:54

pseudomodules: add sock_dtls

0eaf0b2

pkg/tinydtls: implement DTLS sock API

4ed4ab4

pokgak force-pushed the sock_dtls_impl branch from c6517a2 to 87ce70c Compare November 28, 2019 12:54

smlng suggested changes Nov 28, 2019

View reviewed changes

smlng approved these changes Dec 2, 2019

View reviewed changes

smlng added the Reviewed: 3-testing The PR was tested according to the maintainer guidelines label Dec 2, 2019

examples: add DTLS sock example application

57533a7

pokgak force-pushed the sock_dtls_impl branch from fb46966 to 57533a7 Compare December 5, 2019 13:15

smlng merged commit 6e53e28 into RIOT-OS:master Dec 5, 2019

kb2ma mentioned this pull request Jan 11, 2020

Potential buffer overflow in dtls-sock #12905

Closed

pokgak deleted the sock_dtls_impl branch February 27, 2020 10:34

benpicco reviewed May 13, 2021

View reviewed changes

Conversation

pokgak commented Jul 31, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Contribution description

Testing procedure

Issues/PRs references

Uh oh!

rfuentess commented Aug 5, 2019

Uh oh!

rfuentess commented Aug 5, 2019

Uh oh!

miri64 commented Aug 5, 2019

Uh oh!

pokgak commented Aug 6, 2019

Uh oh!

pokgak commented Aug 13, 2019

Uh oh!

rfuentess commented Aug 15, 2019

Uh oh!

miri64 commented Aug 28, 2019

Uh oh!

miri64 commented Aug 28, 2019

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

miri64 commented Sep 24, 2019

Uh oh!

miri64 commented Sep 25, 2019

Uh oh!

pokgak commented Sep 25, 2019

Uh oh!

pokgak commented Sep 25, 2019

Uh oh!

pokgak commented Nov 5, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

tcschmidt commented Nov 27, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

smlng commented Nov 27, 2019

Uh oh!

pokgak commented Nov 28, 2019

Uh oh!

smlng left a comment

Choose a reason for hiding this comment

Uh oh!

pokgak commented Nov 29, 2019

Uh oh!

smlng commented Dec 2, 2019

Uh oh!

pokgak commented Dec 5, 2019

Uh oh!

smlng commented Dec 5, 2019

Uh oh!

benpicco commented Dec 5, 2019

Uh oh!

smlng commented Dec 5, 2019

Uh oh!

rfuentess commented Dec 5, 2019

Uh oh!

pokgak commented Dec 5, 2019

Uh oh!

benpicco May 13, 2021

Choose a reason for hiding this comment

Uh oh!

miri64 May 17, 2021

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

pokgak commented Jul 31, 2019 •

edited

Loading

pokgak commented Nov 5, 2019 •

edited

Loading

tcschmidt commented Nov 27, 2019 •

edited

Loading