-
Notifications
You must be signed in to change notification settings - Fork 58
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add separate stateless processing for client_hello of epoch 0. #89
Conversation
c884cc6
to
529e58c
Compare
51d7e10
to
f8859c1
Compare
24bdd6e
to
24d02cb
Compare
Together with the other PRs #91, #92, #94, this PR provides now a redesign and cleanup for processing CLIENT_HELLO in epoch 0 and removes some stuff, which seems for me to be work-arounds. I removed the rudiments of the HELLO_REQUEST in epoch 0. |
5edace2
to
ed09aef
Compare
Thanks. May I ask you to change the spelling of DTLS messages in the commit message and in code comments in alignment with RFC 6347 (i.e., camel case: ClientHello, HelloRequest, Finished, etc.)? |
Sure. I'm on it. |
ed09aef
to
b72a04c
Compare
Done. Do you prefer that CaMel also in the c-documentation? |
Yes, please. |
aecd264
to
0185aec
Compare
The comments should be fixed. Let me know, if there is more editorial work to do. I will be back in the later afternoon :-). |
If I try using a bad PSK, the server fails to decrypt the Finished, which is fine. As there is no response to the 'bad' Finish the client retries sending the stuff from epoch 0 and then the Finished again in epoch 1. The previous code reports
and the new code
As the server has done a ChangeCipherSpec and moved into epoch 1, should the server be continuing with the stateless processing for epoch 0? As the Finished cannot be decrypted, there is no way of determining what the actual handshake is, but we know that it is in the new epoch as a handshake with a small record sequence number. Should we just drop this 'bad' record (as does OpenSSL) or send back an alert (as does GnuTLS - Bad Record MAC)? |
I'm not sure, if I understand the scenario. The "stateless" end with a ClientHello with a verified cookie. |
Sure - abstracted an example out of my test suite which gives slightly different results to what I reported above. Server key was 12345678 , client key 87654321 Coap-Server log
Agreed, however the code for |
I'm not sure, if "GnuTLS - Bad Record MAC" is intended. At least for Californium there was a discussion about that PSK-failures and in that context TLS-IETF. There we decide " just drop this 'bad' record (as does OpenSSL)". |
That's not in the tinydtls repo, or? |
No, it is not in the tinydtls repo. I have built up a regression test suite for libcoap over time for testing out different functionality and using different (D)TLS libraries with good/bad keys/certs etc. Diffs are then done for actual against expected. When using valgrind, things grind on for hours..... Unfortunately, it is very dependent on the host OS, versions of underlying (D)TLS libraries that were would be too many false positives if it was to became part of the libcoap repo. |
OK. Good to know, that there are more tests. |
I checked the GnuTLS behavior, it closes on the retransmission of flight 5. So the first flight 5 seems to be ignored. Even in the Californium discussion on 2018, I wasn't totally convinced , what the best behavior will be. Anyway, usually to systems are used with the "right credentials", so I don't care too much about the wrong ;-). |
My feeling is, "alert or not", is not really changed by this PR (hope I'm right.) |
Prior to #91, Decrypt Error is returned when processing the Finished. This PR (which includes #91) changes the message that is reported. RFC5246 says
But agree that RFC6347 effectively states that Alerts should not be sent. I am not sure that |
That protects against "manipulated handshake messages", e.g. on-path-attacker. |
And using RPK changes the possible failure as well. There the encrytion may work more often, but manipulated handshake message may get detected. |
0185aec
to
e77b0f8
Compare
The c-documentation is updated to use CaMeL. I added some more information about the "stateless phase" (which ends with receiving a ClientHello with a valid cookie). I adapted the function names of the affected funtions to a pattern xxx_0_yyy, in order to easier detect them (the other difference is the ephemeral_peer instead of a peer). |
@boaks Thank you, this looks good to me. I am not sure if the two commits should be squashed before merge. Do you have an opinion on that? |
Fine by me. |
OK, I squash and push. |
e77b0f8
to
15a7940
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A few minor nits, otherwise I think it is ready for merge.
Use the record and message sequence numbers of that ClientHellos for either sending a HelloVerifyRequest or starting the handshake on the server side with sending a ServerHello. To reduce the complexity, this stateless processing is separated in a set of special functions, which use the new ephemeral_peer instead of the old peer. The names of those function follow the pattern xxx_0_yyy to indicate their usage in epoch 0. Using a HelloRequest in epoch 0 is removed. If required, a clarification about the processing details is required ahead. Remove checks for peers and code/parameter, which have been used to handle that missing peer. (Fix some minor typos in comments and log messages.) Signed-off-by: Achim Kraus <[email protected]>
15a7940
to
c21c46c
Compare
The PR is on the top of some other pending ones.
If at all merged, it is intended to be merged after these PRs.
Use the record and message sequence numbers of that client_hellos for
either sending a hello verify request or starting the handshake on the
server side with sending a server_hello.
The function of a HELLO_REQUEST is unclear and therefore not considered.
In some scopes a HELLO_REQUEST is only used for associated peers to
trigger a rehandshake (handshake executed within an previous encrypted
association, epoch > 0), and some scopes seems to handle also
situations, where either no previous association is required or the new
handshake is executed in epoch 0. That may be clarified either before or
after this change.
Signed-off-by: Achim Kraus [email protected]