From adcda428287d692bb0dae531bcc8e40608ad0854 Mon Sep 17 00:00:00 2001 From: Cameron Bytheway Date: Fri, 24 Jan 2025 14:25:11 -0700 Subject: [PATCH] fix: ignore local address when considering path migration --- quic/s2n-quic-core/src/path/mod.rs | 23 ++++--- .../src/message/msg/handle.rs | 9 +-- quic/s2n-quic-transport/src/path/manager.rs | 18 +++++- quic/s2n-quic-transport/src/path/mod.rs | 20 +++--- .../src/tests/connection_migration.rs | 63 +++++++++++++++++++ 5 files changed, 101 insertions(+), 32 deletions(-) diff --git a/quic/s2n-quic-core/src/path/mod.rs b/quic/s2n-quic-core/src/path/mod.rs index 458f9dce6..34b7dca3d 100644 --- a/quic/s2n-quic-core/src/path/mod.rs +++ b/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); } } diff --git a/quic/s2n-quic-platform/src/message/msg/handle.rs b/quic/s2n-quic-platform/src/message/msg/handle.rs index bc89f73b4..ead072325 100644 --- a/quic/s2n-quic-platform/src/message/msg/handle.rs +++ b/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); } } diff --git a/quic/s2n-quic-transport/src/path/manager.rs b/quic/s2n-quic-transport/src/path/manager.rs index 87a46c72d..bac2c328a 100644 --- a/quic/s2n-quic-transport/src/path/manager.rs +++ b/quic/s2n-quic-transport/src/path/manager.rs @@ -269,8 +269,22 @@ impl Manager { 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)); diff --git a/quic/s2n-quic-transport/src/path/mod.rs b/quic/s2n-quic-transport/src/path/mod.rs index a31091df1..7db770dac 100644 --- a/quic/s2n-quic-transport/src/path/mod.rs +++ b/quic/s2n-quic-transport/src/path/mod.rs @@ -540,21 +540,15 @@ impl Path { /// 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()) } } diff --git a/quic/s2n-quic/src/tests/connection_migration.rs b/quic/s2n-quic/src/tests/connection_migration.rs index 1ab3d4dba..d76434c7e 100644 --- a/quic/s2n-quic/src/tests/connection_migration.rs +++ b/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:?}" + ); +}