examples/dtls-echo: handle handshake_failure alert#12478
examples/dtls-echo: handle handshake_failure alert#12478benpicco merged 2 commits intoRIOT-OS:masterfrom
Conversation
examples/dtls-echo/dtls-client.c
Outdated
| &remote); | ||
|
|
||
| if (res <= 0) { | ||
| if (res < 0) { |
There was a problem hiding this comment.
Does it make sense to continue if res == 0, that is
no received data is available, but everything is in order.
sock_udp_recv()
There was a problem hiding this comment.
In this case yes, because DTLS alerts have payload size 0 but are important for the client/server to know if they received any alerts.
There was a problem hiding this comment.
So just to be clear, that is UDP payload size? The signalling happens by sending an empty message to the DTLS port?
There was a problem hiding this comment.
So just to be clear, that is UDP payload size?
Yepp, like with normal udp sockets;
There was a problem hiding this comment.
wait. now that you mention it, I might be wrong here. I'll check again.
There was a problem hiding this comment.
So, I somehow had the wrong idea in mind that a DTLS alert has payload size 0 eventhough I already saw many times in wireshark that it isn't.
In this case yes, because DTLS alerts have payload size 0 but are important for the client/server to know if they received any alerts.
This statement is wrong.
The signalling happens by sending an empty message to the DTLS port?
No, the signalling happens by sending a DTLS message with type alert in a DTLS record header, which means the received payload size cannot be 0.
Receiving a UDP payload size 0 is technically not an error but it doesn't make sense to continue if res == 0 because we are expecting a DTLS packet and not just UDP. So I'll revert this change and check if I made the same mistake at other places.
Sorry for the confusion :)
|
Murdock found some issues. |
|
Travis is all green except for commit message length and squash check. Should I squash? |
|
Yes please squash. |
e9e5863 to
b770e85
Compare
|
Fixed #12478 (comment). Tests all green :) |
|
ping @benpicco It would be nice if you could review and ACK this PR. It contains fixes that is needed so that the tests I did in #11943 (comment) all passes :) |
examples/dtls-echo/dtls-server.c
Outdated
|
|
||
| return; | ||
| memcpy(&session.addr, &remote_peer->remote->addr.ipv6, | ||
| sizeof(session.addr)); |
There was a problem hiding this comment.
minor nit: keep this on one line, the line below is just as long.
benpicco
left a comment
There was a problem hiding this comment.
The code looks good, adds error handling that wasn't there before.
There is a minor style issue, just squash directly.
This pulls in commit 865ca387cd9d05e52943e5641ad0eefafef218a3 which fixes RIOT-OS#12351.
b770e85 to
1900563
Compare
|
Squashed and rebased to latest master. |
Contribution description
This PR adds handling of handshake_failure alert in the dtls-echo example application and bumps tinydtls pkg to include fixes from the tinydtls repo. With this PR, the client should be able to react to handshake_failure alert sent from the server when e.g. no matching cipher suites are supported.
Testing procedure
I wrote a test script using pexpect that starts two native instances and sends a packet from the client to the server. Link to the script. If anybody is using the script, set
BUILD_IN_DOCKER=0because there is problem passing environment variable set to docker.Steps to use this script
RIOT/dist/tools/tapsetup/tapsetup -cdtls-echoexample:cd RIOT/dtls-echoPORT=tap0and get the IP withifconfig:<SERVER_IP>in the script to the one found in step 3../test-send.pyIssues/PRs references
Fixes #12351