fix(s2n-quic-transport): discard handshake keys on packet receive (#1976

)

* fix(s2n-quic-transport): discard handshake keys on packet receive

* don't require xquic antiamplification test to pass

.github/interop/required.json

@@ -415,9 +415,7 @@
       "client",
       "server"
     ],
-    "xquic": [
-      "server"
-    ]
+    "xquic": []
   },
   "handshakeloss": {
     "aioquic": [],

quic/s2n-quic-transport/src/connection/connection_impl.rs

@@ -381,6 +381,18 @@ impl<Config: endpoint::Config> ConnectionImpl<Config> {
             })
         }
 
+        if self.space_manager.handshake().is_some() && self.space_manager.is_handshake_confirmed() {
+            //= https://www.rfc-editor.org/rfc/rfc9001#section-4.9.2
+            //# An endpoint MUST discard its handshake keys when the TLS handshake is
+            //# confirmed (Section 4.1.2).
+            let path_id = self.path_manager.active_path_id();
+            self.space_manager.discard_handshake(
+                self.path_manager.active_path_mut(),
+                path_id,
+                &mut publisher,
+            );
+        }
+
         // check to see if we're flushing and should now close the connection
         if self.poll_flush().is_ready() {
             self.error?;

quic/s2n-quic-transport/src/connection/transmission.rs

@@ -304,13 +304,7 @@ impl<'a, 'sub, Config: endpoint::Config> tx::Message for ConnectionTransmission<
                 //= https://www.rfc-editor.org/rfc/rfc9001#section-4.9.2
                 //# An endpoint MUST discard its handshake keys when the TLS handshake is
                 //# confirmed (Section 4.1.2).
-                if space_manager.is_handshake_confirmed() {
-                    space_manager.discard_handshake(
-                        &mut self.context.path_manager[self.context.path_id],
-                        self.context.path_id,
-                        self.context.publisher,
-                    );
-                }
+                debug_assert!(!space_manager.is_handshake_confirmed());
 
                 encoder
             } else {

quic/s2n-quic/src/tests/connection_migration.rs

@@ -2,6 +2,11 @@
 // SPDX-License-Identifier: Apache-2.0
 
 use super::*;
+use s2n_codec::DecoderBufferMut;
+use s2n_quic_core::{
+    event::api::Subject,
+    packet::interceptor::{Datagram, Interceptor},
+};
 
 fn run_test<F>(mut on_rebind: F)
 where
@@ -90,6 +95,86 @@ where
     );
 }
 
+/// Ensures that a client that changes its port immediately after
+/// sending a handshake packet that completes the handshake succeeds.
+#[test]
+fn rebind_after_handshake_confirmed() {
+    let model = Model::default();
+
+    test(model, move |handle| {
+        let server = Server::builder()
+            .with_io(handle.builder().build()?)?
+            .with_tls(SERVER_CERTS)?
+            .with_event(events())?
+            .with_packet_interceptor(RebindPortBeforeLastHandshakePacket::default())?
+            .start()?;
+
+        let client = Client::builder()
+            .with_io(handle.builder().build()?)?
+            .with_tls(certificates::CERT_PEM)?
+            .with_event(events())?
+            .start()?;
+
+        let addr = start_server(server)?;
+        start_client(client, addr, Data::new(1000))?;
+        Ok(addr)
+    })
+    .unwrap();
+}
+
+// Changes the port of the second handshake packet received
+#[derive(Default)]
+struct RebindPortBeforeLastHandshakePacket {
+    datagram_count: usize,
+    handshake_packet_count: usize,
+    changed_port: bool,
+}
+
+impl Interceptor for RebindPortBeforeLastHandshakePacket {
+    // Change the port after the first Handshake packet is received
+    fn intercept_rx_remote_port(&mut self, _subject: &Subject, port: &mut u16) {
+        if self.handshake_packet_count == 1 && !self.changed_port {
+            *port += 1;
+            self.changed_port = true;
+        }
+    }
+
+    // Drop the first handshake packet from the client (contained within the second
+    // datagram the client sends) so that the client sends two Handshake PTO packets
+    fn intercept_rx_datagram<'a>(
+        &mut self,
+        _subject: &Subject,
+        _datagram: &Datagram,
+        payload: DecoderBufferMut<'a>,
+    ) -> DecoderBufferMut<'a> {
+        self.datagram_count += 1;
+        if self.datagram_count == 2 {
+            return DecoderBufferMut::new(&mut payload.into_less_safe_slice()[..0]);
+        }
+        payload
+    }
+
+    // Remove the `ACK` frame from the first two Handshake packets received from the
+    // peer, so the first Handshake packet the server transmitted remains in the server's
+    // sent packets
+    fn intercept_rx_payload<'a>(
+        &mut self,
+        _subject: &Subject,
+        packet: &s2n_quic_core::packet::interceptor::Packet,
+        payload: DecoderBufferMut<'a>,
+    ) -> DecoderBufferMut<'a> {
+        if packet.number.space().is_handshake() {
+            self.handshake_packet_count += 1;
+
+            if self.handshake_packet_count <= 2 {
+                return DecoderBufferMut::new(&mut payload.into_less_safe_slice()[8..]);
+            }
+        }
+
+        payload
+    }
+}
+
 /// Rebinds the IP of an address
 fn rebind_ip(mut addr: SocketAddr) -> SocketAddr {
     let ip = match addr.ip() {