From 7e1155376138bff6d74a285963b670ac30aedf57 Mon Sep 17 00:00:00 2001 From: Cameron Bytheway Date: Fri, 31 Jan 2025 15:01:57 -0700 Subject: [PATCH] fix: process packets from different sources before handshake confirmed --- quic/s2n-quic-transport/src/path/manager.rs | 62 +++++-------------- .../src/path/manager/tests.rs | 4 +- quic/s2n-quic-transport/src/path/mod.rs | 54 +++++++++++++++- .../src/tests/connection_migration.rs | 16 ++--- quic/s2n-quic/src/tests/recorder.rs | 2 - 5 files changed, 76 insertions(+), 62 deletions(-) diff --git a/quic/s2n-quic-transport/src/path/manager.rs b/quic/s2n-quic-transport/src/path/manager.rs index bac2c328a..d4a548469 100644 --- a/quic/s2n-quic-transport/src/path/manager.rs +++ b/quic/s2n-quic-transport/src/path/manager.rs @@ -249,44 +249,25 @@ impl Manager { ) -> Result<(Id, AmplificationOutcome), DatagramDropReason> { let valid_initial_received = self.valid_initial_received(); - if let Some((id, path)) = self.path_mut(path_handle) { - let source_cid_changed = datagram - .source_connection_id - .is_some_and(|scid| scid != path.peer_connection_id && valid_initial_received); - - if source_cid_changed { - //= https://www.rfc-editor.org/rfc/rfc9000#section-7.2 - //# Once a client has received a valid Initial packet from the server, it MUST - //# discard any subsequent packet it receives on that connection with a - //# different Source Connection ID. - - //= https://www.rfc-editor.org/rfc/rfc9000#section-7.2 - //# Any further changes to the Destination Connection ID are only - //# permitted if the values are taken from NEW_CONNECTION_ID frames; if - //# subsequent Initial packets include a different Source Connection ID, - //# they MUST be discarded. - - return Err(DatagramDropReason::InvalidSourceConnectionId); - } - - // 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. - + let matched_path = if handshake_confirmed { + self.path_mut(path_handle) + } else { //= 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); - } + //# The design of QUIC relies on endpoints retaining a stable address + //# for the duration of the handshake. An endpoint MUST NOT initiate + //# connection migration before the handshake is confirmed, as defined + //# in section 4.1.2 of [QUIC-TLS]. + + // NOTE: while we must not _initiate_ a migration before the handshake is done, + // it doesn't mean we can't handle the packet. So instead we pick the default path. + let path_id = self.active_path_id(); + let path = self.active_path_mut(); + Some((path_id, path)) + }; - let amplification_outcome = path.on_bytes_received(datagram.payload_len); + if let Some((id, path)) = matched_path { + let amplification_outcome = + path.on_datagram_received(path_handle, datagram, valid_initial_received)?; return Ok((id, amplification_outcome)); } @@ -297,15 +278,6 @@ impl Manager { return Err(DatagramDropReason::UnknownServerAddress); } - //= https://www.rfc-editor.org/rfc/rfc9000#section-9 - //# The design of QUIC relies on endpoints retaining a stable address - //# for the duration of the handshake. An endpoint MUST NOT initiate - //# connection migration before the handshake is confirmed, as defined - //# in section 4.1.2 of [QUIC-TLS]. - if !handshake_confirmed { - return Err(DatagramDropReason::ConnectionMigrationDuringHandshake); - } - //= https://www.rfc-editor.org/rfc/rfc9000#section-9 //# If the peer //# violates this requirement, the endpoint MUST either drop the incoming diff --git a/quic/s2n-quic-transport/src/path/manager/tests.rs b/quic/s2n-quic-transport/src/path/manager/tests.rs index 4f20ce49a..2b3420b8d 100644 --- a/quic/s2n-quic-transport/src/path/manager/tests.rs +++ b/quic/s2n-quic-transport/src/path/manager/tests.rs @@ -765,7 +765,7 @@ fn test_adding_new_path() { // - call on_datagram_received with new remote address bit handshake_confirmed false // // Expectation: -// - asset on_datagram_received errors +// - assert on_datagram_received does not error // - assert we have one paths fn do_not_add_new_path_if_handshake_not_confirmed() { // Setup: @@ -811,7 +811,7 @@ fn do_not_add_new_path_if_handshake_not_confirmed() { ); // Expectation: - assert!(on_datagram_result.is_err()); + assert!(on_datagram_result.is_ok()); assert!(manager.path(&new_addr).is_none()); assert_eq!(manager.paths.len(), 1); } diff --git a/quic/s2n-quic-transport/src/path/mod.rs b/quic/s2n-quic-transport/src/path/mod.rs index 7db770dac..07f52661b 100644 --- a/quic/s2n-quic-transport/src/path/mod.rs +++ b/quic/s2n-quic-transport/src/path/mod.rs @@ -13,8 +13,10 @@ use crate::{ }; use s2n_quic_core::{ counter::{Counter, Saturating}, - event::{self, IntoEvent}, - frame, packet, random, + event::{self, builder::DatagramDropReason, IntoEvent}, + frame, + inet::DatagramInfo, + packet, random, time::{timer, Timestamp}, }; @@ -229,6 +231,54 @@ impl Path { } } + #[inline] + pub fn on_datagram_received( + &mut self, + path_handle: &Config::PathHandle, + datagram: &DatagramInfo, + valid_initial_received: bool, + ) -> Result { + let source_cid_changed = datagram + .source_connection_id + .is_some_and(|scid| scid != self.peer_connection_id && valid_initial_received); + + if source_cid_changed { + //= https://www.rfc-editor.org/rfc/rfc9000#section-7.2 + //# Once a client has received a valid Initial packet from the server, it MUST + //# discard any subsequent packet it receives on that connection with a + //# different Source Connection ID. + + //= https://www.rfc-editor.org/rfc/rfc9000#section-7.2 + //# Any further changes to the Destination Connection ID are only + //# permitted if the values are taken from NEW_CONNECTION_ID frames; if + //# subsequent Initial packets include a different Source Connection ID, + //# they MUST be discarded. + + return Err(DatagramDropReason::InvalidSourceConnectionId); + } + + // 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() { + self.handle.maybe_update(path_handle); + } + + let amplification_outcome = self.on_bytes_received(datagram.payload_len); + + Ok(amplification_outcome) + } + #[inline] pub fn on_timeout( &mut self, diff --git a/quic/s2n-quic/src/tests/connection_migration.rs b/quic/s2n-quic/src/tests/connection_migration.rs index d76434c7e..a86dc8979 100644 --- a/quic/s2n-quic/src/tests/connection_migration.rs +++ b/quic/s2n-quic/src/tests/connection_migration.rs @@ -225,15 +225,13 @@ fn ip_and_port_rebind_test() { #[derive(Default)] struct RebindPortBeforeHandshakeConfirmed { datagram_count: usize, - changed_port: bool, } const REBIND_PORT: u16 = 55555; impl Interceptor for RebindPortBeforeHandshakeConfirmed { fn intercept_rx_remote_address(&mut self, _subject: &Subject, addr: &mut RemoteAddress) { - if self.datagram_count == 1 && !self.changed_port { + if (1..5).contains(&self.datagram_count) { addr.set_port(REBIND_PORT); - self.changed_port = true; } } @@ -279,14 +277,10 @@ fn rebind_before_handshake_confirmed() { .unwrap(); let datagram_dropped_events = datagram_dropped_events.lock().unwrap(); - - assert_eq!(1, datagram_dropped_events.len()); - let event = datagram_dropped_events.first().unwrap(); - assert!(matches!( - event.reason, - DatagramDropReason::ConnectionMigrationDuringHandshake { .. }, - )); - assert_eq!(REBIND_PORT, event.remote_addr.port()); + assert!( + datagram_dropped_events.is_empty(), + "the server should allow packets to be processed before the handshake completes" + ); } // Changes the remote address to ipv4-mapped after the first packet diff --git a/quic/s2n-quic/src/tests/recorder.rs b/quic/s2n-quic/src/tests/recorder.rs index 347df076c..2f80fdfdb 100644 --- a/quic/s2n-quic/src/tests/recorder.rs +++ b/quic/s2n-quic/src/tests/recorder.rs @@ -169,14 +169,12 @@ event_recorder!( use s2n_quic_core::event::api::DatagramDropReason; #[derive(Debug)] pub struct DatagramDroppedEvent { - pub remote_addr: SocketAddr, pub reason: DatagramDropReason, } impl<'a> From<&events::DatagramDropped<'a>> for DatagramDroppedEvent { fn from(value: &events::DatagramDropped<'a>) -> Self { DatagramDroppedEvent { - remote_addr: value.remote_addr.to_string().parse().unwrap(), reason: value.reason.clone(), } }