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
Clarify server CONNECTION_CLOSE with Handshake #2688
Clarify server CONNECTION_CLOSE with Handshake #2688
Changes from 1 commit
9582787
9bcc7fa
ead6736
96c8913
e70eff7
cfa8de6
ab282cd
1bb9ef0
4079e66
9dfb614
4029df2
68d1c8b
b669d0f
ce74eec
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.
This needs to be only a statement of fact that describes the exception to the preceding SHOULD. "If the endpoint does not have Handshake keys, it sends CONNECTION_CLOSE frames in an Initial packet"
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'm confused why we have a MUST and a SHOULD for the other two cases, but you're advising no normative text for the Initial case? Am I missing something?
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.
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'm fairly sure server to client is the only case two connection closes may need to be sent, which is why it's written with server an client.
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.
That assumes a lot about the shape of the handshake though. If you are going to make that call, cite QUIC-TLS.
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 have in mind 17.2.2.1 in Transport: https://tools.ietf.org/html/draft-ietf-quic-transport-20#section-17.2.2.1
"A client stops both sending and processing Initial packets when it sends its first Handshake packet."
This clearly indicates that a client will almost never be processing Initial and Handshake packets at once, so if you only send one connection close, some clients will drop it. In all other cases with #2673 landed, the client and server should both still have Handshake keys even if they also have 1-RTT keys?
Is there something about TLS I'm implicitly assuming as well? I can imagine I am, but I don't know what it is.
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.
Ahh, the two docs blur together in my mind. This is OK. I would suggest that you need to address both transitions here though. The transition to Handshake and the transition to 1-RTT have the same sort of uncertainty.
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.
The handshake to 1-RTT transition has an overlap where the peer has both sets of keys, unlike Initial to Handshake, so I think using handshake confirmed as the signal here provides what we want?
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.
The server can't guarantee this. (see proposed text below)
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 server can't guarantee the packet arrives, but I think sending both guarantees that if it does arrive at the client, the client can be decrypted. Am I missing something?
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.
Suggested rewording of text after the first sentence:
"Under these circumstances, an endpoint SHOULD send a CONNECTION_CLOSE frame in both Handshake and Initial packets to increase the likelihood of a CONNECTION_CLOSE frame being processed by the peer. These packets can be coalesced into a single UDP datagram (see {{packet-coalesce}})."
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 bigger point here is that the server also isn't sure when the client gets 1-RTT keys. We should say the same about that transition. I believe that we can rely on handshake confirmation for this transition. Endpoints might choose to send CONNECTION_CLOSE in Handshake packets prior to the handshake being confirmed.
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.
Agreed, another spot we're relying on #2673?
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 updated the text about sending 1-RTT connection closes to use Handshake confirmed, which lines up with #2673. I think that's a good fix?
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.
Don't sound so uncertain :) I think that the fix is good.
And we'd (collectively) be crazy if we didn't take #2673.
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'd also individually go crazy if we don't take it. 😉
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.
"Under these circumstances, a server SHOULD send a CONNECTION_CLOSE frame in both Handshake and Initial packets to ensure that at least one of them is processable by the client. These packets can be coalesced into a single UDP datagram (see {{packet-coalesce}})."