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

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

Open
tegefaulkes opened this issue Apr 24, 2023 · 3 comments

Comments

@tegefaulkes
Copy link

For reference MatrixAI/js-quic#9 (comment).

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
Copy link
Author

tegefaulkes commented Apr 24, 2023

Digging through the code, is_established gets it's value from self.handshake_completed which is set at

self.handshake_completed = self.handshake.is_completed();

This is getting the value from

unsafe { SSL_in_init(self.as_ptr()) == 0 }

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
Copy link
Member

ghedo commented Apr 24, 2023

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
Copy link
Author

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?

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

No branches or pull requests

2 participants