search

quinn-rs / quinn

Async-friendly QUIC implementation in Rust
Apache License 2.0
3.57k stars 364 forks source link

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

Open billylindeman opened 3 weeks ago

billylindeman commented 3 weeks ago

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 commented 3 weeks ago

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

billylindeman commented 3 weeks ago

@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 commented 3 weeks ago

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 commented 3 weeks ago

@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?

Ralith commented 3 weeks ago

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 commented 3 weeks ago

@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 commented 2 weeks ago

@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?