Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Tls13 add early data indication #6486
Tls13 add early data indication #6486
Changes from 28 commits
0e97d4d
911c0cc
893ad81
b781a23
338f727
7633281
ecc2948
b0c32d8
01323a4
a341225
f447e8a
a042b84
0977716
50a4794
29ee43c
2d87a9e
ae07cd9
fe3483f
de95604
402bb1e
9a0aafb
72b9b17
2cd5ce0
2dbfeda
f3cefb4
51c5a8b
0cc4320
e7bab00
e9622ac
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
We try usually to force the TLS 1.3 version only on the server side thus like in the following test. That way when both TLS 1.2 and TLS 1.3 are enabled in the build the "hybrid" test is run and when only TLS 1.3 is enabled the test corresponding to TLS 1.3 being forced on the client side is run. Thus, I think we should just remove this one.
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.
I just remember in kex exchange mode tests, we remove the force_version=tls13 in client side. To keep aligned, I prefer to remove this case. How about your opinion? @yuhaoth
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.
For this PR, it is okay to remove it for me.
It should be added in
Write APP data
https://datatracker.ietf.org/doc/html/rfc8446#appendix-A.2 shows early data can be sent after ClientHello. It is Okay for TLS1.3 only. But for hybrid mode, it might cause problem. That's why suggest add hybrid tests.
And I do not think we should postpone
write app data
likeprototype
, it does not match RFC. This point has been raised in GnuTLS https://gitlab.com/gnutls/gnutls/-/issues/1146.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.
So I will remove the test case with -force_version=tls13, and only leave the test case which you said "hibrid case" as basic case.
We will leave the "hibrid case" there. Can you eleborate the problems?
Seems it's related with wrte early data, can we leave the comment to this PR #6542
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.
Okay.
Sure.
From RFC 8446, we should send early data after ClientHello immediately. So we must switch outbound traffic after ClientHello ASAP.
If switch in CLIENT_HELLO and send early data, it match RFC. But if the server select TLS 1.2, I am not sure what will happen.
To avoid the issue, I think we should postpone the traffic switch, just after TLS 1.3 is selected at this moment. And in ClientHello, we should only switch traffic if it is configured as tls1.3 only.
For the test side, we should have
tls1.3 only
and hybrid mode test to cover the cases.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.
Seems it's one issue we should trade off when and in which case to change transform key.
Currently we only support session tikets psk, which means we only send out early data in resumption with tickets, maybe weI can make use of the tls_version in seriealized session to detect whether we only send out tls13 version, and then server will only have one tls13 choice. I am not sure whether the design work or not, maybe we can solve it in some ways.
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.
Yes that's a key point. A TLS 1.3 handshake proposing early data can only be done with a PSK (external or obtained through a ticket) which is TLS 1.3 specific. Thus in that case we are not going to propose TLS 1.2 to the server: "TLS 1.3 only ClientHello".