-
-
Notifications
You must be signed in to change notification settings - Fork 10.1k
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
DTLS 1.3: Place start of ClientHello correctly when calculating binder #24426
Conversation
@mattcaswell @t8m Please have a look. |
CI failure looks relevant |
Agreed. It was there with the first commit. I'll give it a look now. |
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.
Looks good. One minor comment and the CI failure to resolve.
Fixed the minor. There's a failure on the latest commit which I don't believe is relevant. Out of curiosity: How is the SCTP auth export supposed to work? I see that the As far as I understand this check should only pass after the FINISH message, so how is that supposed to work when it gets called from the |
The current code only gets called from
It seems to me that SCTP is not yet well defined in the DTLSv1.3 case. There is a draft: https://www.ietf.org/archive/id/draft-tuexen-tsvwg-rfc6083-bis-04.html @tuexen might be able to comment further on its status. I would suggest that we don't attempt to enable SCTP for DTLSv1.3 until an actual RFC exists for it. |
Ah fair. I was looking at this from rfc6063:
And hence didn't consider any updated RFC |
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.
One very minor nit, otherwise I'm ready to approve
I've noticed recently that sometimes ci checks are cancelled somewhat randomly. What's the reason for this? Is it new behaviour? |
Actually I'm not sure why this happens. |
…inder for DTLS 1.3
…ating binder for DTLS 1.3
… calculating binder for DTLS 1.3
…ly when calculating binder for DTLS 1.3
…correctly when calculating binder for DTLS 1.3
I've rebased and fixed two conflicts. Please reconfirm @mattcaswell. |
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.
Reconfirmed
IMO it is (was) some Github flakiness. It looks like it has been fixed. |
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.
Good work!
This pull request is ready to merge |
Merged to the feature branch. Thank you for your contribution. |
… 1.3 Reviewed-by: Matt Caswell <matt@openssl.org> Reviewed-by: Tomas Mraz <tomas@openssl.org> (Merged from #24426)
Reviewed-by: Matt Caswell <matt@openssl.org> Reviewed-by: Tomas Mraz <tomas@openssl.org> (Merged from #24426)
… 1.3 Reviewed-by: Matt Caswell <matt@openssl.org> Reviewed-by: Tomas Mraz <tomas@openssl.org> (Merged from #24426)
Reviewed-by: Matt Caswell <matt@openssl.org> Reviewed-by: Tomas Mraz <tomas@openssl.org> (Merged from #24426)
I found out that the msgstart and the binderoffset are calculated incorrectly for DTLS 1.3. I'm not sure exactly why, but it seems like it is off with the size of the handshake message header. Maybe you have a suggestion to a better fix than my original commit? It also appears that the maxfragmentlen is parsed wrongly.