search

cloudflare / quiche

🥧 Savoury implementation of the QUIC transport protocol and HTTP/3
https://docs.quic.tech/quiche/
BSD 2-Clause "Simplified" License
9.4k stars 709 forks source link

Client side `quiche::Connection` `is_established()` returns true before handshake completes. #1489

Open tegefaulkes opened 1 year ago

tegefaulkes commented 1 year ago

For reference https://github.com/MatrixAI/js-quic/issues/9#issuecomment-1519298781.

For context, I have a test in our code that checks if a connection fails if the server fails to authenticate the client. My expectation here is that the server will end up with a TlsFail error and close the connection. The client should see the closing frame with the TLS error BEFORE the handshake has completed and is_established() returns true.

What I am seeing is that the client's is_established() is returning true very early in the handshake procedure.

1   0.000000000 127.0.0.1   127.0.0.1   34238   QUIC    1242    Initial, DCID=494077f00d584ca236388136a42f9a6d, SCID=fa93ec24c633d1f91662cb586d0b57c46cca0dec, PKN: 0, CRYPTO
2   0.002128503 127.0.0.1   127.0.0.1   34148   QUIC    250 Retry, DCID=fa93ec24c633d1f91662cb586d0b57c46cca0dec, SCID=4adf076468fb432462e51fb1018c79749b21e0dc
3   0.003273495 127.0.0.1   127.0.0.1   34238   QUIC    1242    Initial, DCID=4adf076468fb432462e51fb1018c79749b21e0dc, SCID=fa93ec24c633d1f91662cb586d0b57c46cca0dec, PKN: 1, CRYPTO
4   0.006133042 127.0.0.1   127.0.0.1   34148   QUIC    1242    Handshake, DCID=fa93ec24c633d1f91662cb586d0b57c46cca0dec, SCID=4adf076468fb432462e51fb1018c79749b21e0dc, PKN: 0, CRYPTO
// Client's `is_established()` returns true here.
5   0.039734489 127.0.0.1   127.0.0.1   34238   QUIC    1242    Handshake, DCID=4adf076468fb432462e51fb1018c79749b21e0dc, SCID=fa93ec24c633d1f91662cb586d0b57c46cca0dec, PKN: 0, ACK, CRYPTO
6   0.040731996 127.0.0.1   127.0.0.1   34238   QUIC    162 Handshake, DCID=4adf076468fb432462e51fb1018c79749b21e0dc, SCID=fa93ec24c633d1f91662cb586d0b57c46cca0dec, PKN: 1, ACK, CRYPTO
// Server rejects client's TLS cert here
7   0.067375517 127.0.0.1   127.0.0.1   34148   QUIC    113 Handshake, DCID=fa93ec24c633d1f91662cb586d0b57c46cca0dec, SCID=4adf076468fb432462e51fb1018c79749b21e0dc, PKN: 1, CC
8   0.071265881 127.0.0.1   127.0.0.1   34238   QUIC    162 Handshake, DCID=4adf076468fb432462e51fb1018c79749b21e0dc, SCID=fa93ec24c633d1f91662cb586d0b57c46cca0dec, PKN: 2, ACK, CRYPTO
9   0.071536492 127.0.0.1   127.0.0.1   34238   QUIC    115 Handshake, DCID=4adf076468fb432462e51fb1018c79749b21e0dc, SCID=fa93ec24c633d1f91662cb586d0b57c46cca0dec, PKN: 3, ACK, PING

It is my understanding that is_established() should only return true once the handshake has completed. And the handshake only completes once the HANDSHAKE_DONE frame (shown as DONE in the packet logs) has been sent.

For reference, here are the packet logs for a connection that succeeds.

1   0.000000000 127.0.0.1   127.0.0.1   36275   QUIC    1242    Initial, DCID=299582cc5ab22a21e56e76591e28b1bd, SCID=c577439e01a0dcb123b5005c790f1959cdc65459, PKN: 0, CRYPTO
2   0.002151087 127.0.0.1   127.0.0.1   43749   QUIC    250 Retry, DCID=c577439e01a0dcb123b5005c790f1959cdc65459, SCID=b5e03103516e73f4506ee5565710db8455111925
3   0.003300591 127.0.0.1   127.0.0.1   36275   QUIC    1242    Initial, DCID=b5e03103516e73f4506ee5565710db8455111925, SCID=c577439e01a0dcb123b5005c790f1959cdc65459, PKN: 1, CRYPTO
4   0.006159623 127.0.0.1   127.0.0.1   43749   QUIC    1242    Handshake, DCID=c577439e01a0dcb123b5005c790f1959cdc65459, SCID=b5e03103516e73f4506ee5565710db8455111925, PKN: 0, CRYPTO
// Client's `is_established()` returns true here.
5   0.043200211 127.0.0.1   127.0.0.1   36275   QUIC    1242    Handshake, DCID=b5e03103516e73f4506ee5565710db8455111925, SCID=c577439e01a0dcb123b5005c790f1959cdc65459, PKN: 0, ACK, CRYPTO
6   0.044810146 127.0.0.1   127.0.0.1   36275   QUIC    154 Handshake, DCID=b5e03103516e73f4506ee5565710db8455111925, SCID=c577439e01a0dcb123b5005c790f1959cdc65459, PKN: 1, ACK, CRYPTO
// Server's `is_established()` returns true here.
7   0.046969036 127.0.0.1   127.0.0.1   43749   QUIC    529 Protected Payload (KP0), DCID=c577439e01a0dcb123b5005c790f1959cdc65459, PKN: 0, DONE, CRYPTO
// Expected Client side establishment?
8   0.050229052 127.0.0.1   127.0.0.1   36275   QUIC    86  Protected Payload (KP0), DCID=b5e03103516e73f4506ee5565710db8455111925, PKN: 0, ACK
// Expected Server side establishment?

Here we see that the client is established far before the DONE frame is sent in packet 7.

So is this a bug with quiche?

tegefaulkes commented 1 year ago

Digging through the code, is_established gets it's value from self.handshake_completed which is set at https://github.com/cloudflare/quiche/blob/0b37da1cc564e40749ba650febd40586a4355be4/quiche/src/lib.rs#L6431

This is getting the value from https://github.com/cloudflare/quiche/blob/0b37da1cc564e40749ba650febd40586a4355be4/quiche/src/tls.rs#L784

Acording to https://www.openssl.org/docs/man1.1.1/man3/SSL_in_init.html

SSL_in_init() returns 1 if the SSL/TLS state machine is currently processing or awaiting handshake messages, or 0 otherwise.

This seems to be the wrong thing to be using here? If we're still processing the handshake then the handshake isn't done yet. Shouldn't it use SSL_is_init_finished instead?

SSL_is_init_finished() returns 1 if the SSL/TLS connection is in a state where fully protected application data can be transferred or 0 otherwise.
ghedo commented 1 year ago

And the handshake only completes once the HANDSHAKE_DONE frame (shown as DONE in the packet logs) has been sent.

Nope, per RFC9001, Section 4.1.1:

In this document, the TLS handshake is considered complete when the TLS stack has reported that the handshake is complete.

Which is what SSL_in_init() does.

HANDSHAKE_DONE is used for handshake confirmation... they are different things.

Shouldn't it use SSL_is_init_finished instead?

I don't think it would make any difference, since in BoringSSL that just calls SSL_in_init() https://github.com/google/boringssl/blob/master/ssl/ssl_lib.cc#L2787-L2789

tegefaulkes commented 1 year ago

Ah, sorry for my misunderstanding, I'm pretty new to this library. If this is intended behaviour then I had a bad assumption about when a connection is considered established.

Digging deeper into my problem. In my code to avoid an TlsFail error after I consider the connection fully established. I need to know on the client side when the connection has fully completed the TLS handshake or if the HANDSHAKE_DONE frame has been received.

Is there a way to know when this stage in the connection has been reached?