Improving TinyDTLS package and dtls-echo example #7615

rfuentess · 2017-09-18T15:23:56Z

This PR is the first of a series of PRs for addressing the work with tinyDTLS (See RFC).

This PR upgrade the following:

pkg/tinydtls: Upgrades to the current version of tinyDTLS (0.8.6) and simplifies the user requirements for the application's makefile.
example/dtls-echo: Switches to the use of socks, increases the stability and also add a feature for testing CoAP Secure servers (or any other service running over DLTS).

Work in progress

[December: Not anymore WIP]
~~The following issues are the reason this is marked as WIP.~~

Dynamic memory

As stated previously in the RFC, one of the major issues with tinyDTLS and RIOT is the handling of the memory required for the data structures of the former. Right now, I'm using the original code which uses malloc/free functions. Yet, this is far from optimal for RIOT.

The testings with the iotlab-m3 and iotlab-a8-m3 is OK, but with the price of inflating the stack size of the main thread. In a similar fashion, the testings for samr21-xpro can handle the client side of DTLS, yet is suffering from overflow from the server side.

I'm thinking of using macro functions with static arrays for replacing the calls of malloc/free. Yet, this is a very general problem for many applications, so, I would like the opinion of others before to proceed with this.

Official repository

This is a minor issue, my current branch of tinyDTLS is ready to merge with the official branch. Yet, we are not sure of how integrating it without making more complex it for other platforms.

~~If not viable solutions are achieved here, I'm going to change the approach used for pkg/tinydtls by using the official repository and a series of patches for when the package is called.~~

Issues/PRs references

This PR depends on PR ~~#7651~~

miri64 · 2017-09-19T07:44:54Z

@tobhe could you maybe have a look at this to maybe?

tobhe · 2017-09-19T08:09:22Z

Sure, I'm gonna have a look at it

tobhe · 2017-09-20T13:04:34Z

@rfuentess I'm on your side with the macro functions for dynamic allocations. As far as I can see the allocations we are talking about are the hmac_context_storage, peer_storage, handshake_storage, security_storage and netq_storage. In other words: every place Contiki uses memb functions, which do exactly what you propose.

For the upstream repository I guess the least complex solution would be to provide one or more platform.h files containing not only constants but also platform specific function definitions, similar to micro-ecc's platform-specific.inc, as our function signatures will not differ from contiki or *nix. However, I guess it all just depends how the tinyDTLS maintainers want to solve this.

About the PR itself: which gcc version have you been testing with? I'm getting a lot of implicit fallthrough warnings (which are enabled by default in GCC 7) building tinydtls:

/examples/dtls-echo/bin/pkg/native/tinydtls/uthash.h:449:21: error: this statement may fall through [-Werror=implicit-fallthrough=]
      case 3:  _hj_i += ( (unsigned)_hj_key[2] << 16 );                           \
               ~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

I propose adding a -Wno-implicit-fallthrough to the CFLAGS or, which would be even better, fixing it upstream using fallthrough comments.
While testing on native the dtls-echo server was working well, the client had to be restarted to get a second reply, but i've not yet looked into this further.

rfuentess · 2017-09-20T19:33:30Z

Thanks by the feedback @tobhe !

In other words: every place Contiki uses memb functions, which do exactly what you propose.

Yes, this brings me a next question that it has been on my mind for a while, this adaptation should be used only for tinyDTLS or should be a PR by itself? Maybe another application can benefit from it.

About the PR itself: which gcc version have you been testing with?

uh, it's seems that I'm falling behind as I'm using GCC 4.9.2 for Debian. For now, I'm going to add the -Wno-implicit-fallthrough flag..

tobhe · 2017-09-20T20:12:48Z

uh, it's seems that I'm falling behind as I'm using GCC 4.9.2 for Debian. For now, I'm going to add the -Wno-implicit-fallthrough flag..

It should be possbile to disable it in the uthash.h file only, using #pragma GCC diagnostic ignored "-Wimplicit-fallthrough"

Yes, this brings me a next question that it has been on my mind for a while, this adaptation should be used only for tinyDTLS or should be a PR by itself? Maybe another application can benefit from it.

I think this should be it's own PR and it's own module, as it is surely at least as useful as utlist.h which also got it's own module.

rfuentess · 2017-09-27T10:44:21Z

During the RIOT summit, there was a nice reminder of security issues with tinyDTLS (0.8.2 and 0.8.6), that were previously mentioned in different conversations but were left out of this PR.

Probably, the most relevant one is the current use of PRNG as the one used by tinyDTLS is just an (non-secure) example. Therefore, I need to integrate the one from RIOT to tinyDTLS.

tobhe · 2017-09-27T11:40:48Z

Probably, the most relevant one is the current use of PRNG as the one used by tinyDTLS is just an (non-secure) example. Therefore, I need to integrate the one from RIOT to tinyDTLS.

As the current random api only allows to generate 32 bit random blocks, #7390 might be interesting for this

rfuentess · 2017-09-27T12:55:35Z

Thanks @tobhe !

I also notice this PR. I'll see if it's possible to integrate it to tinyDTLS and if not, like a patch for RIOT :)

tobhe · 2017-09-27T17:47:50Z

@rfuentess I added a PR (#7651) for a static memory pool allocater which could be used to replace malloc

rfuentess · 2017-11-22T15:33:38Z

I just made a rebase in the PR for updating to the last release of RIOT.

Also, now tinydtls is making use of @tobhe's memarray module (PR #7651). In native this is working wonderfully. However, there is an issue for communicating with nodes outside the 6LoWPAN. This is happening in the lasts steps of negotiation as the external nodes are sending keys and masters bigger than the one expected by the sensors. ~~Therefore, I believe, is merely an issue of calibrating the static size used by tinydtls/crypto.*~~. By the way, this was also an issue in my old testings with Contiki and tinyDTLS.

But for now, I'm putting my efforts in seeking to remove the DTHREAD_STACKSIZE_MAIN line. Yet, I'm facing a hard fault at the reception of UDP packets. ~~I have a theory of why is happening, but I want to debug it more to be sure.~~

There is also a memory leak happening with dtls_free_context() that requires to be fixed for the official repository.

So, in resume, those are the next steps:

Located the hardfault when the main thread is not artificially increased.
Fix the issue for scenarios with external nodes
Fix the memory leak of dtls_free_context()

rfuentess · 2017-11-30T16:37:22Z

I'm still facing some issues with memory leaks (which may or not may be related to the one I already know).

Located the hardfault when the main thread is not artificially increased.

Good news: I located the issues (there were 2). One is fixed.
Bad news: The remaining issue requires extra efforts in redesigning tinyDTLS internal functions (the packet buffer is copied and duplicated up to four times).

Therefore, the Makefile line increasing the size of DTHREAD_STACKSIZE_MAIN must remain there for now. Also, this is hitting some boards harder than others.

Fix the issue for scenarios with external nodes

Identified, and fixed. This also is reported to the main tinyDTLS repository as it was affecting Contiki implementations.

rfuentess · 2017-12-01T17:42:48Z

While testing on native the dtls-echo server was working well, the client had to be restarted to get a second reply, but i've not yet looked into this further.

This is an issue more hard to resolve than I thought originally. And, probably is the reason behind the pseudo-process used by Contiki (which in turn never erase the dtls context).

One or more resources are not being released correctly by dtls_context_free(). As a result, the next time a new context is generated, the state machine believes it's in another step or that has incorrect data. Of course, a partial solution is to never release our context once that is generated. In a similar approach that the one used on the server side.

rfuentess · 2017-12-14T17:15:43Z

I'm going to rebase this tomorrow. For now, I'm able to create and close as much DTLS channels as I want with PSK or RPK (tested with iotlab-m3 and samr21-xpro).

Fix the memory leak of dtls_free_context()

The memory leak remains for now. It's not that easy to remove it and the team of tinyDTLS want to analyze it better before applying an official patch.

rfuentess · 2017-12-15T16:42:05Z

And finally, the WIP status is not anymore required!. I edited the OP.

I'm leaving the commits affecting pkg/tinydtls/Makefile separated if by any reason, users are required to make git bisect.

@MichelRottleuthner can you give it a look? :) Also, Is it possible to add the flag Waiting For Other PR ?

smlng · 2017-12-21T12:25:06Z

please rebase

add the flag Waiting For Other PR ?

sure, but on which PR does this one wait?

tobhe · 2017-12-21T13:03:53Z

@smlng it uses the memory block allocator in #7651, i suppose that's what @rfuentess meant

rfuentess · 2017-12-22T21:11:34Z

As briefly discussed offline with @smlng, I'll wait for #7651 to be merged into master before making a rebase.

@smlng it uses the memory block allocator in #7651, i suppose that's what @rfuentess meant

Yes, exactly! I should had been more clearly, sorry.

rfuentess · 2018-02-16T13:14:43Z

At this moment, there is a testing branch (for RIOT) on the official tinydtls repository (Thanks @obgm !) :)

git clone -b riot-support git://git.eclipse.org/gitroot/tinydtls/org.eclipse.tinydtls.git

It's already tested with this PR for native, iotlab-m3, iotlab-a8-m3, samr21-xpro and partially with openmote-cc2538.

So, this is the current plan:

Wait until Generic memory block allocator #7651 is merged, for having the final API for memarray.
Wait until tinydtls' master branch merges the support for RIOT.
Rebase this PR (and update the client due it will be affected by issue 457 (comment).
Update pkg/tinydtls/Makefile for using the official repository!

rfuentess · 2018-06-19T08:35:08Z

Starting the next month is going to be hard for me to test this PR on real hardware. Therefore, it is possible to merge it for the next release?

rfuentess · 2018-06-26T06:42:02Z

Ping @MichelRottleuthner, @aabadie.

MichelRottleuthner · 2018-06-26T14:06:52Z

I had no time to do a proper review and just did a quick test. On native everything works as expected. On real hardware I ran into problems related to the delay parameter. If it's too small the example won't work with DTLS_ECC enabled.
Does it actually make sense to have this delay (exposed to the user)? Like it is implemented now the client needs to know how fast the server can crunch the numbers to choose the delay correctly.
Also it seems a bit confusing to me how the mapping of unencrypted udp socket to tinydtls is done: why is the data from sock_udp_recv passed to dtls_handle_read "manually" while the send is completely encapsulated in try_send? Looking at a tinyDTLS test this is done completely inside of dtls_handle_read. Most of the handler functions also look like they can be deduplicated from server and client.

rfuentess · 2018-06-26T15:56:07Z

real hardware I ran into problems related to the delay parameter. If it's too small the example won't work with DTLS_ECC enabled
Does it actually make sense to have this delay (exposed to the user)? Like it is implemented now the client needs to know how fast the server can crunch the numbers to choose the delay correctly.

Actually, I left the timer exposed intentionally. I think this would be particularly useful to users who have different types of hardware for making performance tests. Particularly with ECC which requires around 10 seconds if all the encryption/decryption is made by software.

Also it seems a bit confusing to me how the mapping of unencrypted udp socket to tinydtls is done: why is the data from sock_udp_recv passed to dtls_handle_read "manually" while the send is completely encapsulated in try_send? Looking at a tinyDTLS test this is done completely inside of dtls_handle_read.

For 'try_send' both my RIOT client example and the default Linux client have a similar behavior. They send the upper layer data by means of dtls_write(). However, I just noticed that the description of my 'try_send' is wrong. I'll fix this.

In the case of dtls_handle_read and sock_udp_recv. I think that my reasoning at that time was to try to separate the errors related to the reception of messages of those with DTLS. However, you are right, this is more confusing. I'll make the changes on both sides.

Most of the handler functions also look like they can be deduplicated from server and client.

Yes, this is true. But, because I was writing an example, I wanted to make easy the comparison between client and server instead of having a common source file for both of them.

MichelRottleuthner · 2018-06-26T16:28:15Z

I think this would be particularly useful to users who have different types of hardware for making performance >tests.

I don't get how the timer makes it easier to make performance tests? If I measure performance I want to measure time spent doing calcuation, sending data and son on - not the time spent (unconditionally)waiting.
That handshaking etc. takes up to 10 seconds may be fine, but I dont' understand why the static delay is needed for it to work correctly. If the remote needs time for processing then why not just wait for it to respond to the current step of handshakig?

rfuentess · 2018-06-27T08:14:18Z

I don't get how the timer makes it easier to make performance tests? ...

Probably it was only useful for me due that I was testing the minimum delay that I could implement. For other scenario probably is just redundant and adding complexity. Fair enough, I'll make use of #ifdef for the delay :)

That handshaking etc. takes up to 10 seconds may be fine, but I dont' understand why the static delay is needed for it to work correctly. If the remote needs time for processing then why not just wait for it to respond to the current step of handshakig?

It is not ideal to just wait for an answer in the handshake process because it's not uncommon that one or more messages got lost. Also, the tinyDTLS retransmission of lost DTLS records is kind of broken on 802.15.4 and I had to put it at a minimum. This is on the TODO list for tinyDTLS but far below of the memory improvement, and both of them are for future PRs.

If I don't use the timeout and the handshake fails after the second flight I'm forced to restart the other side as well. With this timeout, the node eventually sends a DTLS Alert record to the other peer, saving me some seconds between tests.

rfuentess · 2018-06-27T08:19:36Z

Though, alternatively, I could add the delay directly into sock_udp_recv() and not with a xtimer at the end of the loop.

rfuentess · 2018-06-27T13:21:43Z

@MichelRottleuthner, Comments addressed :)

smlng · 2018-06-29T08:05:46Z

examples/dtls-echo/Makefile

-USEMODULE += gnrc_udp
-# Add a routing protocol
+USEMODULE += gnrc_sock_udp
+# Add a routing protocol (optional)


can you please remove any module not needed for the dtls example? IHMO, gnrc_rpl and icmpv6_echo are not required, even ps os not mandatory. Also I would simply use gnrc_ipv6_default instead of _router_default.

Keep it simple, if somebody wants to use multi-hop networking and run dtls over it, fine but I guess then they can also extend and add modules as needed.

smlng · 2018-06-29T08:08:27Z

pkg/tinydtls/Makefile.dep

@@ -1,9 +1,41 @@
 ifneq (,$(filter tinydtls,$(USEPKG)))
  USEMODULE += tinydtls
+
+  # Mandatory for tinyDTLS
+  CFLAGS += -DDTLSv12 -DWITH_SHA256


IMHO all of these CFLAGS modification should go into pkg/tinydtls/Makefile.include or even in the pkg/tinydtls/Makefile, the .dep should only describe module dependencies.

rfuentess · 2018-06-29T08:46:05Z

@smlng , Comments addressed

kYc0o · 2018-06-29T16:15:54Z

Are all comments addressed here?

rfuentess · 2018-06-29T16:35:03Z

From my side, I believe that yes. Le ven. 29 juin 2018 à 18:16, Francisco Acosta <[email protected]> a écrit :

…

Are all comments addressed here? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#7615 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AKv_kSR9S9TqXcdtByq6soRob6cphpcQks5uBlLAgaJpZM4PbFS9> .

MichelRottleuthner · 2018-07-04T06:49:22Z

@aabadie @smlng changes ok? @rfuentess there are still some murdock errors

smlng · 2018-07-04T09:25:13Z

@MichelRottleuthner can you please re-test and approve when good?

Though I see lots of room for improving the code, eg. deduplication and structure, I think this PR already goes into the right direction. Let's get this in and work on it in followup PRs.

@rfuentess after @MichelRottleuthner ACK please squash as needed and make Murdock happy 😄

rfuentess · 2018-07-04T09:32:02Z

@MichelRottleuthner is OK if I a do squash now?

MichelRottleuthner · 2018-07-04T09:32:52Z

sure, go ahead

The integration of TinyDTLS for RIOT has been merged into the development branch of the official repository. It will be officially available on the master branch in the next release of tinyDTLS. Other minor updates: - Support for the RIOT's memarray - tinydtls/sha2 is removed from the compilation for giving priority to RIOT's sha2 functions.

Due to all the changes, this is basically a new version for this example. The main benefit is the use of sock_udp but also the client side is now more robust and reliable. The parameters required for the PSK and ECC (e.g. keys) modes are moved to an unique header.

MichelRottleuthner

tested with PSK and ECC config on native<->native and pba-d-01-kw2x<->samr21-xpro works! -> ACK

MichelRottleuthner · 2018-07-04T09:56:08Z

ping @aabadie

aabadie

I don't want to block this and my comments were already addressed.

ACK

smlng · 2018-07-04T09:59:25Z

🎉 congrats @rfuentess - thx @MichelRottleuthner for testing!

miri64 assigned MichelRottleuthner Sep 19, 2017

miri64 requested a review from MichelRottleuthner September 19, 2017 07:44

miri64 added Area: network Area: Networking Area: pkg Area: External package ports labels Sep 19, 2017

rfuentess mentioned this pull request Sep 27, 2017

RFC: (TLSMAN) API for handling all the (D)TLS stacks for the user #7649

Closed

tobhe mentioned this pull request Sep 27, 2017

Generic memory block allocator #7651

Merged

rfuentess mentioned this pull request Nov 14, 2017

pkg/tinydtls: update version to fix compiler errors #8031

Merged

rfuentess force-pushed the dtls_upgrade branch from 63edee3 to 7fe7ffa Compare November 22, 2017 15:26

rfuentess force-pushed the dtls_upgrade branch 2 times, most recently from 451239c to 8696c5e Compare December 15, 2017 16:30

rfuentess changed the title ~~[WIP] Improving TinyDTLS package and dtls-echo example~~ Improving TinyDTLS package and dtls-echo example Dec 15, 2017

smlng added the State: waiting for other PR State: The PR requires another PR to be merged first label Dec 21, 2017

This was referenced Jun 28, 2018

[WIP] TLSMAN: Support for sock_async #9446

Closed

[WIP] sock_secure module #9450

Closed

smlng suggested changes Jun 29, 2018

View reviewed changes

kYc0o added the CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR label Jul 2, 2018

smlng approved these changes Jul 4, 2018

View reviewed changes

rfuentess added 2 commits July 4, 2018 11:33

rfuentess force-pushed the dtls_upgrade branch from 6ea7de7 to 4092eda Compare July 4, 2018 09:39

MichelRottleuthner approved these changes Jul 4, 2018

View reviewed changes

aabadie approved these changes Jul 4, 2018

View reviewed changes

aabadie merged commit a3fc323 into RIOT-OS:master Jul 4, 2018

rfuentess deleted the dtls_upgrade branch July 4, 2018 10:04

cladmi added this to the Release 2018.07 milestone Jul 10, 2018

Improving TinyDTLS package and dtls-echo example #7615

Improving TinyDTLS package and dtls-echo example #7615

Conversation

rfuentess commented Sep 18, 2017 • edited by aabadie Loading

Work in progress

Dynamic memory

Official repository

Issues/PRs references

miri64 commented Sep 19, 2017

tobhe commented Sep 19, 2017

tobhe commented Sep 20, 2017

rfuentess commented Sep 20, 2017

tobhe commented Sep 20, 2017

rfuentess commented Sep 27, 2017

tobhe commented Sep 27, 2017

rfuentess commented Sep 27, 2017 • edited Loading

tobhe commented Sep 27, 2017

rfuentess commented Nov 22, 2017 • edited Loading

rfuentess commented Nov 30, 2017

Located the hardfault when the main thread is not artificially increased.

Fix the issue for scenarios with external nodes

rfuentess commented Dec 1, 2017

rfuentess commented Dec 14, 2017

Fix the memory leak of dtls_free_context()

rfuentess commented Dec 15, 2017 • edited Loading

smlng commented Dec 21, 2017

tobhe commented Dec 21, 2017

rfuentess commented Dec 22, 2017

rfuentess commented Feb 16, 2018

rfuentess commented Jun 19, 2018

rfuentess commented Jun 26, 2018

MichelRottleuthner commented Jun 26, 2018

rfuentess commented Jun 26, 2018

MichelRottleuthner commented Jun 26, 2018

rfuentess commented Jun 27, 2018

rfuentess commented Jun 27, 2018

rfuentess commented Jun 27, 2018

smlng Jun 29, 2018

Choose a reason for hiding this comment

smlng Jun 29, 2018

Choose a reason for hiding this comment

rfuentess commented Jun 29, 2018

kYc0o commented Jun 29, 2018

rfuentess commented Jun 29, 2018 via email

MichelRottleuthner commented Jul 4, 2018

smlng commented Jul 4, 2018

rfuentess commented Jul 4, 2018

MichelRottleuthner commented Jul 4, 2018

MichelRottleuthner left a comment

Choose a reason for hiding this comment

MichelRottleuthner commented Jul 4, 2018

aabadie left a comment

Choose a reason for hiding this comment

smlng commented Jul 4, 2018

rfuentess commented Sep 18, 2017 •

edited by aabadie

Loading

rfuentess commented Sep 27, 2017 •

edited

Loading

rfuentess commented Nov 22, 2017 •

edited

Loading

rfuentess commented Dec 15, 2017 •

edited

Loading