fix: ignore local address when considering path migration (#2458)

Author: camshaft

quic/s2n-quic-core/src/path/mod.rs

@@ -3,7 +3,9 @@
 
 use crate::{
     event,
-    inet::{IpV4Address, IpV6Address, SocketAddress, SocketAddressV4, SocketAddressV6},
+    inet::{
+        IpV4Address, IpV6Address, SocketAddress, SocketAddressV4, SocketAddressV6, Unspecified as _,
+    },
 };
 use core::fmt;
 
@@ -161,6 +163,16 @@ macro_rules! impl_addr {
 
 impl_addr!(LocalAddress);
 
+impl LocalAddress {
+    #[inline]
+    pub fn maybe_update(&mut self, other: &Self) {
+        // only update the local address if it's specified
+        ensure!(!other.is_unspecified());
+
+        *self = *other;
+    }
+}
+
 impl_addr!(RemoteAddress);
 
 impl Handle for RemoteAddress {
@@ -263,14 +275,7 @@ impl Handle for Tuple {
 
     #[inline]
     fn maybe_update(&mut self, other: &Self) {
-        if other.local_address.port() == 0 {
-            return;
-        }
-
-        // once we discover our path, or the port changes, update the address with the new information
-        if self.local_address.port() != other.local_address.port() {
-            self.local_address = other.local_address;
-        }
+        self.local_address.maybe_update(&other.local_address);
     }
 }
 

quic/s2n-quic-platform/src/message/msg/handle.rs

@@ -109,14 +109,7 @@ impl path::Handle for Handle {
 
     #[inline]
     fn maybe_update(&mut self, other: &Self) {
-        if other.local_address.port() == 0 {
-            return;
-        }
-
-        // once we discover our path, or the port changes, update the address with the new information
-        if self.local_address.port() != other.local_address.port() {
-            self.local_address = other.local_address;
-        }
+        self.local_address.maybe_update(&other.local_address);
     }
 }
 

quic/s2n-quic-transport/src/path/manager.rs

@@ -269,8 +269,22 @@ impl<Config: endpoint::Config> Manager<Config> {
                 return Err(DatagramDropReason::InvalidSourceConnectionId);
             }
 
-            // update the address if it was resolved
-            path.handle.maybe_update(path_handle);
+            // Update the address if it was resolved
+            //
+            // NOTE: We don't update the server address since this would cause the client to drop
+            // packets from the server.
+
+            //= https://www.rfc-editor.org/rfc/rfc9000#section-9
+            //# If a client receives packets from an unknown server address, the client MUST discard these packets.
+
+            //= https://www.rfc-editor.org/rfc/rfc9000#section-9
+            //# If the peer sent the disable_active_migration transport parameter, an endpoint also MUST NOT send
+            //# packets (including probing packets; see Section 9.1) from a different local address to the address
+            //# the peer used during the handshake, unless the endpoint has acted on a preferred_address transport
+            //# parameter from the peer.
+            if Config::ENDPOINT_TYPE.is_client() {
+                path.handle.maybe_update(path_handle);
+            }
 
             let amplification_outcome = path.on_bytes_received(datagram.payload_len);
             return Ok((id, amplification_outcome));

quic/s2n-quic-transport/src/path/mod.rs

@@ -540,21 +540,15 @@ impl<Config: endpoint::Config> Path<Config> {
 
     /// Compare a Path based on its PathHandle.
     ///
-    /// In case the local_address on the connection is unknown and set to
-    /// a default un-specified value only the remote_address is used
-    /// to compare Paths.
-    ///
-    /// In the case of the local endpoint being a client, the remote address is only used
-    /// since the client might experience address rebinding.
+    /// QUIC only considers the remote address when identifying paths
+    //= https://www.rfc-editor.org/rfc/rfc9000#section-9.3
+    //# Receiving a packet from a new peer address containing a non-probing frame
+    //# indicates that the peer has migrated to that address.
     #[inline]
     fn eq_by_handle(&self, handle: &Config::PathHandle) -> bool {
-        if Config::ENDPOINT_TYPE.is_client() || self.handle.local_address().port() == 0 {
-            self.handle
-                .remote_address()
-                .unmapped_eq(&handle.remote_address())
-        } else {
-            self.handle.unmapped_eq(handle)
-        }
+        self.handle
+            .remote_address()
+            .unmapped_eq(&handle.remote_address())
     }
 }
 

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

@@ -447,3 +447,66 @@ fn rebind_blocked_port() {
         }
     }
 }
+
+// Changes the local address after N packets
+#[derive(Default)]
+struct RebindAddrAfter {
+    count: usize,
+}
+
+impl Interceptor for RebindAddrAfter {
+    fn intercept_rx_local_address(&mut self, _subject: &Subject, addr: &mut LocalAddress) {
+        if self.count == 0 {
+            addr.0 = rebind_port(rebind_ip(addr.0.into())).into();
+        }
+    }
+
+    fn intercept_rx_datagram<'a>(
+        &mut self,
+        _subject: &Subject,
+        _datagram: &Datagram,
+        payload: DecoderBufferMut<'a>,
+    ) -> DecoderBufferMut<'a> {
+        self.count = self.count.saturating_sub(1);
+        payload
+    }
+}
+
+/// Ensures that a datagram received from a client on a different server IP/port is still
+/// accepted.
+#[test]
+fn rebind_server_addr_before_handshake_confirmed() {
+    let model = Model::default();
+    let subscriber = recorder::DatagramDropped::new();
+    let datagram_dropped_events = subscriber.events();
+
+    test(model, move |handle| {
+        let server = Server::builder()
+            .with_io(handle.builder().build()?)?
+            .with_tls(SERVER_CERTS)?
+            .with_event((tracing_events(), subscriber))?
+            .with_random(Random::with_seed(456))?
+            .with_packet_interceptor(RebindAddrAfter { count: 1 })?
+            .start()?;
+
+        let client = Client::builder()
+            .with_io(handle.builder().build()?)?
+            .with_tls(certificates::CERT_PEM)?
+            .with_event(tracing_events())?
+            .with_random(Random::with_seed(456))?
+            .start()?;
+
+        let addr = start_server(server)?;
+        start_client(client, addr, Data::new(1000))?;
+        Ok(addr)
+    })
+    .unwrap();
+
+    let datagram_dropped_events = datagram_dropped_events.lock().unwrap();
+    let datagram_dropped_events = &datagram_dropped_events[..];
+
+    assert!(
+        datagram_dropped_events.is_empty(),
+        "s2n-quic should not drop packets with different server addrs {datagram_dropped_events:?}"
+    );
+}