Skip to content

Conversation

@WesleyRosenblum
Copy link
Contributor

@WesleyRosenblum WesleyRosenblum commented Sep 27, 2023

Description of changes:

QUIC§4.9.2 specifies:

An endpoint MUST discard its handshake keys when the TLS handshake is confirmed (Section 4.1.2).

Currently we discard the handshake packet number space on the first transmitted handshake packet after the handshake is confirmed. Since the handshake is confirmed upon receipt of a packet (on the server, the CRYPTO[FIN], and on the client the HANDSHAKE_DONE), this leaves a brief moment between when the handshake is confirmed and when the handshake keys are discarded in which additional handshake packets may be received. These packets will not be subject to certain restrictions that exist prior to the handshake being confirmed, such as allowing for connection migration.

This change will discard the handshake packet space after a packet is received that confirms the handshake, reducing the amount of time the handshake space exists with the handshake confirmed.

Call-outs:

Since the handshake space is now discarded prior to transmission, this means the final handshake packet sent from the server (typically Handshake[1]: ACK[0]) will not be transmitted. This ack is not necessary for progressing the connection though, so it should be OK.

The xquic anti-amplification test started failing with this change. This is because xquic has a bug where it cannot parse the NEW_CONNECTION_ID frames that s2n-quic sends as soon as the handshake is confirmed. With this PR we send the NEW_CONNECTION_ID frames earlier than before, due to this check on the existence of the Handshake packet space. Because of this, xquic fails the connection prior to completing the request in the test. I've remove xquic anti-amplification test from the required interop tests.

Testing:

Added a test that switches the remote port immediately after the handshake is confirmed so verify that the connection succeeds.

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

camshaft
camshaft previously approved these changes Sep 29, 2023
@WesleyRosenblum WesleyRosenblum merged commit e3eb0d1 into main Sep 29, 2023
@WesleyRosenblum WesleyRosenblum deleted the WesleyRosenblum/discardhandshake branch September 29, 2023 23:34
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.

2 participants