Skip to content

pkg/wolfssl: implement sock_dtls#15698

Closed
pokgak wants to merge 3 commits intoRIOT-OS:masterfrom
pokgak:wolfssl_sock_dtls_v2
Closed

pkg/wolfssl: implement sock_dtls#15698
pokgak wants to merge 3 commits intoRIOT-OS:masterfrom
pokgak:wolfssl_sock_dtls_v2

Conversation

@pokgak
Copy link
Copy Markdown
Contributor

@pokgak pokgak commented Dec 27, 2020 •
edited
Loading

Contribution description

This PR implements the synchronous part DTLS sock API for wolfSSL. An async implementation will be done as a follow-up.

This PR also moves the existing sock_tls implementation provided by the wolfSSL team to its own directory under the contrib directory. sock_tls is a wrapper around wolfSSL functions to setup and destroys sessions when using wolfSSL directly. Although the similarity the name is similar, it is not related to the sock_dtls API.

Testing procedure

Use the dtls-sock example to test. Make sure that in the Makefile, USEPKG += wolfssl and one of USEMODULE += wolfssl_psk or USEMODULE += wolfcrypt_ecc are uncommented. Try sending messages between client and server. The behavior should be the same as when using tinydtls apart from the issues mentioned below.

Known Issues

I tried to make sure this implementation behaves as close as possible to the existing tinydtls implementation but there are some parts that are harder to do so:

  • wolfSSL uses a different timeout system during a handshake compared to RIOT
    • wolfSSL tracks timeout using 3 struct member dtls_timeout_init, dtls_timeout, and dtls_timeout_max
    • During a handshake process, dtls_timeout is set to dtls_timeout_init at the start and multiplied by 2 each time the _recv callback returns until it reaches dtls_timeout_max where it will return
    • this means if the server is given a timeout value of 1 second to listen to new connections, it might actually wait for more than that
    • this PR works around this by setting all three timeout value that wolfSSL uses to the same value so that it will return directly after one call to the _recv callback
  • there are no SOCK_NO_TIMEOUT equivalent in wolfSSL which means the server might return an error when waiting for a long time even though timeout is set to SOCK_NO_TIMEOUT
  • wolfSSL will not send data if len is 0. The handshake will succeed and it will not report any error but Wireshark does not show any data transmitted
  • wolfSSL will automatically call the _recv callback to listen to handshake packets after a new ClientHello is sent which is not the expected behavior of the sock_dtls_session_init function
    • This PR currently uses non-blocking options of wolfSSL to make it return directly after sending the first ClientHello but sometimes it might already receives other handshake packets before returning.

Useful Links

Documentation of the wolfSSL API

@pokgak pokgak added Area: network Area: Networking Area: pkg Area: External package ports Area: security Area: Security-related libraries and subsystems labels Dec 27, 2020
@pokgak
Copy link
Copy Markdown
Contributor Author

pokgak commented Dec 27, 2020 •
edited
Loading

I also plan to implement Kconfig options for wolfssl and refactor common config options (e.g. DEBUG, PSK, ECC) under the sock DTLS Kconfig module but dependencies seem a bit complicated to define in Kconfig.

Other than that, I think we need a general recommendation of which information should go in sock_dtls_t and sock_dtls_session_t. In this PR, I ended up including a pointer to sock_dtls_t in sock_dtls_session_t (see sock_dtls_types.h) because wolfSSL functions only allows session as parameter for sending/receiving but we still need the UDP sock from sock_dtls_t to send it using RIOTs UDP sock API in the _recv and _send callback.

@pokgak pokgak requested a review from benpicco December 27, 2020 18:36
@miri64
Copy link
Copy Markdown
Member

miri64 commented Jan 8, 2021 •
edited
Loading

Great! Now that there are more than one implementations and the API has proven to be flexible enough for other stacks than tinydtls (does it?), it would also be great to have some kind of API tests, like we have in tests/{gnrc,lwip,openwsn}_sock_{udp,ip,tcp}, so we can ensure, that the sock_dtls API behaves the same in all implementation. We can do this however as a follow-up.

@pokgak
Copy link
Copy Markdown
Contributor Author

pokgak commented Jan 8, 2021

From the API perspective, I think sock_dtls_session_init might need to be reconsidered. The current described behavior of only sending the ClientHello is influenced too much by how tinydtls dtls_connect works. A more fully featured DTLS library like wolfssl usually tries to do as much as possible when calling their equivalent functions.

From the OP:

wolfSSL will automatically call the _recv callback to listen to handshake packets after a new ClientHello is sent which is not the expected behavior of the sock_dtls_session_init function

Other than that, I would like to wait until the async part is implemented before I'm comfortable to say that the API is flexible enough.

@pokgak
Copy link
Copy Markdown
Contributor Author

pokgak commented Jan 8, 2021

@leandrolanzieri @akshaim currently wolfssl defines pseudomodules to be used in its pkg/wolfssl/includes/user_settings.h to configure the compilation of wolfssl (ciphers, features, etc). Is it okay if we replace it or provide an alternative using Kconfig? If we have Kconfig for wolfssl pkg, then later we can make it so that general DTLS options can be configured through sock_dtls Kconfig and the user does not have to touch the config of the underlying DTLS library for the most cases. Is this doable?

@leandrolanzieri
Copy link
Copy Markdown
Contributor

leandrolanzieri commented Jan 11, 2021

@leandrolanzieri @akshaim currently wolfssl defines pseudomodules to be used in its pkg/wolfssl/includes/user_settings.h to configure the compilation of wolfssl (ciphers, features, etc). Is it okay if we replace it or provide an alternative using Kconfig?

If we are currently using pseudomodules to enable/disable features, then this falls into the 'Module dependency modelling' part of Kconfig. This means that it would not be visible unless TEST_KCONFIG=1 is used. Adding these symbols to Kconfig would still be really valuable.

If we have Kconfig for wolfssl pkg, then later we can make it so that general DTLS options can be configured through sock_dtls Kconfig and the user does not have to touch the config of the underlying DTLS library for the most cases. Is this doable?

Which configurations would be the common ones? I agree that the more we can factor out to a common DTLS config the better.

@pokgak
Copy link
Copy Markdown
Contributor Author

pokgak commented Jan 12, 2021

Which configurations would be the common ones?

What I have in mind right are enabling debug options, which cipher to enable, hanshake timeout.

@janosbrodbeck
Copy link
Copy Markdown
Member

janosbrodbeck commented Jan 13, 2021 •
edited
Loading

Is there something comparable to DTLS_PEER_MAX from tinydtls to configure how many sessions can be hold by wolfssl? This should, in my opinion, also be a parameter which should be easily configurable.

@janosbrodbeck janosbrodbeck mentioned this pull request Apr 30, 2021
Merged
@MrKevinWeiss MrKevinWeiss added this to the Release 2021.07 milestone Jun 21, 2021
@MrKevinWeiss MrKevinWeiss removed this from the Release 2021.07 milestone Jul 15, 2021
@pokgak
Copy link
Copy Markdown
Contributor Author

pokgak commented Oct 6, 2021

Closing since I don't have much time to look at this nowadays. Feel free to takeover this PR :)

@pokgak pokgak closed this Oct 6, 2021
@benpicco benpicco added the Community: help wanted The contributors require help from other members of the community label Oct 6, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@leandrolanzieri leandrolanzieri Awaiting requested review from leandrolanzieri leandrolanzieri is a code owner

@miri64 miri64 Awaiting requested review from miri64

@janosbrodbeck janosbrodbeck Awaiting requested review from janosbrodbeck

@benpicco benpicco Awaiting requested review from benpicco

Assignees

No one assigned

Labels

Area: network Area: Networking Area: pkg Area: External package ports Area: security Area: Security-related libraries and subsystems Community: help wanted The contributors require help from other members of the community

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

@pokgak @miri64 @leandrolanzieri @janosbrodbeck @benpicco @MrKevinWeiss