Skip to content
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

During a client connect handshake, wait for Frame::HandshakeDone before considering the connection established #1887

Conversation

billylindeman
Copy link

As it stands today, a quinn client will consider itself established/connected prior to receving the HandshakeDone frame.

If a connection fails because the server rejects the client (such as a tls certificate issue, or some other reason), the client will have already resolved the connecting.await? into a valid Connection.

@djc
Copy link
Member

djc commented Jun 6, 2024

Do you have a reference to the RFC section that describes this requirement?

@billylindeman
Copy link
Author

billylindeman commented Jun 6, 2024

@djc The specific section I see is in the QUIC-TLS rfc: https://www.rfc-editor.org/rfc/rfc9001.html#name-handshake-confirmed

At the client, the handshake is considered confirmed when a HANDSHAKE_DONE frame is received.

@djc
Copy link
Member

djc commented Jun 6, 2024

That section also has this:

Additionally, a client MAY consider the handshake to be confirmed when it receives an acknowledgment for a 1-RTT packet. This can be implemented by recording the lowest packet number sent with 1-RTT keys and comparing it to the Largest Acknowledged field in any received 1-RTT ACK frame: once the latter is greater than or equal to the former, the handshake is confirmed.

Maybe that is applicable to our earlier implementation?

@billylindeman
Copy link
Author

@djc, I don't believe quinn implements that section / behavior because the precipitating issue that led to this investigation is in a situation where a client attempts to connect with a server and the server rejects the TLS keys.

In that case, the client considers itself established and then later fails once the client receives the close message - however if quinn was properly waiting for a 1-rtt ACK during the handshake I don't believe it would get to that point?

@billylindeman billylindeman force-pushed the fix-client-connect-await-handshake-done branch from 2c4ba16 to 93841a3 Compare June 6, 2024 17:55
@Ralith
Copy link
Collaborator

Ralith commented Jun 6, 2024

Thanks for the PR!

It is by design that Quinn yields the Connection upon entering the Handshake Complete state. The effect of this change is to wait for Handshake Confirmed instead. This takes an additional round-trip, doubling the expected connection establishment latency. I don't think that's a good default. See also RFC9000's Example Handshake Flows, in particular client transmission of 1-RTT data prior to receiving HANDSHAKE_DONE.

Still, I appreciate that it's awkward for there not to be a well-defined location for clients to detect authentication failures when communicating with servers that require client authentication. Would it be sufficient to expose a new method on Connection that waits for handshake confirmation?

@billylindeman
Copy link
Author

@Ralith Thanks for the explanation! I re-read that part of the spec and that makes sense to me. Having a well-defined place to await handshake confirmation would be great

@djc
Copy link
Member

djc commented Jun 10, 2024

@Ralith Thanks for the explanation! I re-read that part of the spec and that makes sense to me. Having a well-defined place to await handshake confirmation would be great

Do you want to change this PR to address that?

@Ralith
Copy link
Collaborator

Ralith commented Jul 27, 2024

Closing in favor of #1944.

@Ralith Ralith closed this Jul 27, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants