fix: fail reads clearly when the peer sends a TLS 1.3 KeyUpdate - #75
Conversation
|
@djc can you also take a look |
|
Tell me if anything needs to happen on my end here. I'd like to do a followup PR in terms of implementing KeyUpdates, and this is a prerequisite. |
cpu
left a comment
cpu
left a comment
There was a problem hiding this comment.
Tell me if anything needs to happen on my end here.
Please be patient before following up again for reviews. Four days isn't very long, especially for this repo that gets less maintainer attention given its niche status.
3428ff4 to
db35997
Compare
Understood. Thank you for the review and I'm sorry. |
|
Can you squash your commits into one? I will merge afterwards and if djc has any additional feedback we can iterate as follow-up. |
The kernel cannot switch traffic keys, so after a peer rekeys every read fails to decrypt with an opaque `EBADMSG`. Detect the KeyUpdate handshake message and fail the read with an explicit error instead. NewSessionTicket is still ignored. This matches Facebook's kTLS implementation: https://github.com/facebookincubator/fizz/blob/7fb7075a65ec9e669a6851669502d549e12f9e0f/fizz/experimental/ktls/AsyncKTLSSocket.cpp#L257-L280 We should implement `KeyUpdate` in the `ktls` crate at some point. There is already ongoing work (rustls#62). Until that is complete, it's best to return an explicit error.
131dd43 to
54d5e07
Compare
|
I squashed the commits |
|
I'll let these changes run in production for a while. If everything looks good, I think it could be a good time to make a |
3 participants
We don't support switching traffic keys, so after a peer rekeys every read fails to decrypt with an opaque
EBADMSG. Detect the KeyUpdate handshake message and fail the read with an explicit error instead. NewSessionTicket is still ignored.This matches Facebook's kTLS implementation's handling of both
KeyUpdateandNewSessionTicket: https://github.com/facebookincubator/fizz/blob/7fb7075a65ec9e669a6851669502d549e12f9e0f/fizz/experimental/ktls/AsyncKTLSSocket.cpp#L257-L280We should implement
KeyUpdatein thektlscrate at some point. There is already ongoing work (#62). Until that is complete, it's best to return an explicit error.