sys: new sock submodule for DTLS#11909
Conversation
|
#ifdef DOXYGEN
#define SOCK_DTLS_PSK (0) /**< PSK mode */
#define SOCK_DTLS_ECC (0) /**< ECC mode */
#endifor something similar to |
Not sure this helps with the problem at hand, but I wanted to point out that there is an implementation guideline for representation of elliptic curves by the light-weigh implementation guidance working group of the IETF: https://tools.ietf.org/html/draft-ietf-lwig-curve-representations-08 |
I think it is better to expose it in tinydtls pkg instead of DTLS sock as Or would documenting it all in a new doxygen group (e.g. net_encryption) be better? The group could also describe caveats and configuration options of each DTLS implementation and explain how to use credman with DTLS sock. In general, information on how to get started with using (D)TLS in RIOT.
No, it is not mutually exclusive. A DTLS sock should be able to use multiple cipher suites. |
This should not be part of this PR. But how about making these (pseudo) submodules of
I don't see why a new group would be necessary. If those variables are |
|
I'm not sure about having tinydtls_psk and tinydtls_ecc as submodules but I'll try it first and open a PR for that first along with the configuration variable documentation and we can continue to discuss it there.
Yeah this makes more sense. I added the documentation on how to use DTLS sock in net_sock_dtls (33793d7) and general info about DTLS support in RIOT in net_dtls (e5be939). |
miri64
left a comment
There was a problem hiding this comment.
Could you maybe split this PR into at least two:
- Introduction of the
sock_dtlsAPI - Implementation of
sock_dtlsintinydtls
This would help greatly with the review.
bcbb4c1 to
9a47cd0
Compare
|
Addressed the comments and split the PR into two. This PR now introduces the DTLS sock API and a new Doxygen group |
|
Thanks. I'll have a look later this week. |
miri64
left a comment
There was a problem hiding this comment.
Only reviewed the API and its documentation so far. Not the more general documentation on top.
sys/include/net/sock/dtls.h
Outdated
| * res = sock_dtls_establish_session(&dtls_sock, &remote, &session); | ||
| * if (res < 0) { | ||
| * printf("Error establishing session: %d\n", (int)res); | ||
| * goto end; |
There was a problem hiding this comment.
Please don't advertise gotos in the high-level doc.
There was a problem hiding this comment.
I'll rewrite this without using gotos.
|
If I understand your question correctly, for now it could be done using sock_dtls_create();
#ifdef MODULE_WOLFSSL_SOCK_DTLS
sock_dtls_config_t config = {
.cipherlist = "...";
.version = DTLSv1_2,
};
sock_dtls_set_config(&config);
#endif
sock_dtls_establish_session(); |
edf3e65 to
59a6ec3
Compare
|
On second thought, doing it like the example from my last comment doesn't actually solve the problem. It is more straightforward to just use
But this is still a valid problem, right? |
Yeah, I am thinking for a while about something similar to |
|
Added some commits. I'll update the higher level documentation if these changes are okay. (8eb26a9) - The current API does not specify whether the UDP sock used is also closed when (8d40d5f) - Added |
miri64
left a comment
There was a problem hiding this comment.
(8eb26a9) - The current API does not specify whether the UDP sock used is also closed when
sock_dtls_close()is called. To avoid callingsock_udp_close()twice, I think it should not close the UDP sock. I'm not sure howsock_udp_close()behaves if called twice with the same UDP sock but with this, we do not have to worry about this problem.
✔️
(8d40d5f) - Added
sock_dtls_method_tto specify version number and role of the endpoint. Values for DTLS 1.0 and 1.2 are taken from RFC4347 and RFC6347 respectively.ProtocolVersionare as define in RFC5246. Some DTLS implementation also definesProtocolVersionin their library. I'm not sure if it will cause a problem when it is defined twice.
There will be a conflict. See below.
sys/include/net/sock/dtls.h
Outdated
| * @anchor sock_dtls_prot_version | ||
| * @{ | ||
| */ | ||
| #define DTLSv1_0 (1) /**< DTLS version 1.0 */ |
There was a problem hiding this comment.
SOCK_DTLS_1_0 to stick to the naming scheme?
|
Great! I think we are getting to a point were we can merge this. I'll have another look soon (hopefully before my vacation next week). |
pokgak
left a comment
There was a problem hiding this comment.
Addressed comments.
|
Sorry, Github UI is confusing. I thought I'm replying directly to the comments. Edit: nvm. it seems okay. |
|
Needs another rebase. Other than that, I believe we can finally merge this. You may squash during that rebase. |
b84b4d3 to
ede7edd
Compare
|
Squashed and rebased. |
|
Thanks @miri64 for the review! |
Contribution description
This PR is part of the PR series to introduce a new DTLS sock module in RIOT.
The module introduces a new sock type
sock_dtls. Similar to the TCP/UDP sock, DTLS sock needs to be implemented separately for each underlying DTLS implementation.This PR also introduces a new
net_dtlsgroup in Doxygen to group and document all related DTLS components together for easier discovery.The example implementation of this PR is provided by #11943.
Testing procedure
make static-testandmake docproduces no errors or warnings.Issues/PRs references
Depends on #11564
Previous discussion #10897