From c82daf73b0216ba078b3620eb0cb4530da60bb3b Mon Sep 17 00:00:00 2001 From: Wesley Rosenblum <55108558+WesleyRosenblum@users.noreply.github.com> Date: Thu, 6 Mar 2025 12:10:18 -0800 Subject: [PATCH] fix(s2n-quic-transport): allow migrations even when disable_active_migration is sent (#2516) * fix: allow migrations even when disable_active_migrations is sent * update events --- quic/s2n-quic-core/events/common.rs | 4 ++ quic/s2n-quic-core/src/event/generated.rs | 8 ++++ quic/s2n-quic-transport/src/path/manager.rs | 24 +++------- ...connection_migration_disabled__events.snap | 5 ++- .../src/path/manager/tests.rs | 45 +++++++++++++------ 5 files changed, 53 insertions(+), 33 deletions(-) diff --git a/quic/s2n-quic-core/events/common.rs b/quic/s2n-quic-core/events/common.rs index 195cf3092..b13423749 100644 --- a/quic/s2n-quic-core/events/common.rs +++ b/quic/s2n-quic-core/events/common.rs @@ -782,12 +782,16 @@ enum DatagramDropReason { /// A datagram was received from an unknown server address. UnknownServerAddress, /// The peer initiated a connection migration before the handshake was confirmed. + /// + /// Note: This drop reason is no longer emitted ConnectionMigrationDuringHandshake, /// The attempted connection migration was rejected. RejectedConnectionMigration { reason: MigrationDenyReason }, /// The maximum number of paths per connection was exceeded. PathLimitExceeded, /// The peer initiated a connection migration without supplying enough connection IDs to use. + /// + /// Note: This drop reason is no longer emitted InsufficientConnectionIds, } diff --git a/quic/s2n-quic-core/src/event/generated.rs b/quic/s2n-quic-core/src/event/generated.rs index d8725ac9f..3da5ba809 100644 --- a/quic/s2n-quic-core/src/event/generated.rs +++ b/quic/s2n-quic-core/src/event/generated.rs @@ -875,6 +875,8 @@ pub mod api { UnknownServerAddress {}, #[non_exhaustive] #[doc = " The peer initiated a connection migration before the handshake was confirmed."] + #[doc = ""] + #[doc = " Note: This drop reason is no longer emitted"] ConnectionMigrationDuringHandshake {}, #[non_exhaustive] #[doc = " The attempted connection migration was rejected."] @@ -884,6 +886,8 @@ pub mod api { PathLimitExceeded {}, #[non_exhaustive] #[doc = " The peer initiated a connection migration without supplying enough connection IDs to use."] + #[doc = ""] + #[doc = " Note: This drop reason is no longer emitted"] InsufficientConnectionIds {}, } impl aggregate::AsVariant for DatagramDropReason { @@ -5051,12 +5055,16 @@ pub mod builder { #[doc = " A datagram was received from an unknown server address."] UnknownServerAddress, #[doc = " The peer initiated a connection migration before the handshake was confirmed."] + #[doc = ""] + #[doc = " Note: This drop reason is no longer emitted"] ConnectionMigrationDuringHandshake, #[doc = " The attempted connection migration was rejected."] RejectedConnectionMigration { reason: MigrationDenyReason }, #[doc = " The maximum number of paths per connection was exceeded."] PathLimitExceeded, #[doc = " The peer initiated a connection migration without supplying enough connection IDs to use."] + #[doc = ""] + #[doc = " Note: This drop reason is no longer emitted"] InsufficientConnectionIds, } impl IntoEvent for DatagramDropReason { diff --git a/quic/s2n-quic-transport/src/path/manager.rs b/quic/s2n-quic-transport/src/path/manager.rs index f031f2293..e0c8b3189 100644 --- a/quic/s2n-quic-transport/src/path/manager.rs +++ b/quic/s2n-quic-transport/src/path/manager.rs @@ -12,7 +12,6 @@ use crate::{ use s2n_quic_core::{ ack, connection::{self, Limits, PeerId}, - ensure, event::{ self, builder::{DatagramDropReason, MtuUpdatedCause}, @@ -339,20 +338,6 @@ impl Manager { let local_address = path_handle.local_address(); let active_local_addr = self.active_path().local_address(); let active_remote_addr = self.active_path().remote_address(); - // The peer has intentionally tried to migrate to a new path because they changed - // their destination_connection_id. This is considered an "active" migration. - let active_migration = - self.active_path().local_connection_id != datagram.destination_connection_id; - - if active_migration { - ensure!(limits.active_migration_enabled(), { - let reason = migration::DenyReason::ConnectionMigrationDisabled; - publisher.on_connection_migration_denied(reason.into_event()); - Err(DatagramDropReason::RejectedConnectionMigration { - reason: reason.into_event().reason, - }) - }) - } // TODO set alpn if available let attempt: migration::Attempt = migration::AttemptBuilder { @@ -441,15 +426,18 @@ impl Manager { let cc = congestion_controller_endpoint.new_congestion_controller(path_info); let peer_connection_id = { - if active_migration { + if self.active_path().local_connection_id != datagram.destination_connection_id { //= https://www.rfc-editor.org/rfc/rfc9000#section-9.5 //# Similarly, an endpoint MUST NOT reuse a connection ID when sending to //# more than one destination address. - // Active connection migrations must use a new connection ID + // The peer changed destination CIDs, so we will attempt to switch to a new + // destination CID as well. This could still just be a NAT rebind though, so + // we continue with the existing destination CID if there isn't a new one + // available. self.peer_id_registry .consume_new_id_for_new_path() - .ok_or(DatagramDropReason::InsufficientConnectionIds)? + .unwrap_or(self.active_path().peer_connection_id) } else { //= https://www.rfc-editor.org/rfc/rfc9000#section-9.5 //# Due to network changes outside diff --git a/quic/s2n-quic-transport/src/path/manager/snapshots/path__manager__tests__active_connection_migration_disabled__events.snap b/quic/s2n-quic-transport/src/path/manager/snapshots/path__manager__tests__active_connection_migration_disabled__events.snap index 84dd28067..b62d38b62 100644 --- a/quic/s2n-quic-transport/src/path/manager/snapshots/path__manager__tests__active_connection_migration_disabled__events.snap +++ b/quic/s2n-quic-transport/src/path/manager/snapshots/path__manager__tests__active_connection_migration_disabled__events.snap @@ -3,8 +3,9 @@ source: quic/s2n-quic-core/src/event/snapshot.rs input_file: quic/s2n-quic-transport/src/path/manager/tests.rs --- ConnectionIdUpdated { path_id: 0, cid_consumer: Local, previous: 0x01, current: 0x69643032 } -ConnectionMigrationDenied { reason: ConnectionMigrationDisabled } PathCreated { active: Path { local_addr: 0.0.0.0:0, local_cid: 0x4c6f63616c4900000000000000004c6f63616c49, remote_addr: 127.0.0.1:1, remote_cid: 0x69643032, id: 0, is_active: true }, new: Path { local_addr: 0.0.0.0:0, local_cid: 0x69643032, remote_addr: 127.0.0.2:1, remote_cid: 0x69643033, id: 1, is_active: false } } MtuUpdated { path_id: 1, mtu: 1200, cause: NewPath, search_complete: false } -PathCreated { active: Path { local_addr: 0.0.0.0:0, local_cid: 0x4c6f63616c4900000000000000004c6f63616c49, remote_addr: 127.0.0.1:1, remote_cid: 0x69643032, id: 0, is_active: true }, new: Path { local_addr: 0.0.0.0:0, local_cid: 0x4c6f63616c4900000000000000004c6f63616c49, remote_addr: 127.0.0.3:1, remote_cid: 0x69643032, id: 2, is_active: false } } +PathCreated { active: Path { local_addr: 0.0.0.0:0, local_cid: 0x4c6f63616c4900000000000000004c6f63616c49, remote_addr: 127.0.0.1:1, remote_cid: 0x69643032, id: 0, is_active: true }, new: Path { local_addr: 0.0.0.0:0, local_cid: 0x69643033, remote_addr: 127.0.0.2:1, remote_cid: 0x69643032, id: 2, is_active: false } } MtuUpdated { path_id: 2, mtu: 1200, cause: NewPath, search_complete: false } +PathCreated { active: Path { local_addr: 0.0.0.0:0, local_cid: 0x4c6f63616c4900000000000000004c6f63616c49, remote_addr: 127.0.0.1:1, remote_cid: 0x69643032, id: 0, is_active: true }, new: Path { local_addr: 0.0.0.0:0, local_cid: 0x4c6f63616c4900000000000000004c6f63616c49, remote_addr: 127.0.0.3:1, remote_cid: 0x69643032, id: 3, is_active: false } } +MtuUpdated { path_id: 3, mtu: 1200, cause: NewPath, search_complete: false } diff --git a/quic/s2n-quic-transport/src/path/manager/tests.rs b/quic/s2n-quic-transport/src/path/manager/tests.rs index 2b3420b8d..fe674a6b0 100644 --- a/quic/s2n-quic-transport/src/path/manager/tests.rs +++ b/quic/s2n-quic-transport/src/path/manager/tests.rs @@ -14,7 +14,7 @@ use crate::{ use core::time::Duration; use s2n_quic_core::{ connection::limits::ANTI_AMPLIFICATION_MULTIPLIER, - event::{builder::MigrationDenyReason, testing::Publisher}, + event::testing::Publisher, inet::{DatagramInfo, ExplicitCongestionNotification, SocketAddress}, path::{migration, RemoteAddress}, random::{self, Generator}, @@ -982,6 +982,9 @@ fn limit_number_of_connection_migrations() { assert_eq!(total_paths, MAX_ALLOWED_PATHS); } +// Connection migration is still allowed to proceed even if the `disable_active_migration` +// transport parameter is sent, as there is no way to definitely distinguish an active +// migration from a NAT rebind. #[test] fn active_connection_migration_disabled() { // Setup: @@ -1015,13 +1018,14 @@ fn active_connection_migration_disabled() { let new_addr: SocketAddr = "127.0.0.2:1".parse().unwrap(); let new_addr = SocketAddress::from(new_addr); let new_addr = RemoteAddress::from(new_addr); - let new_cid = connection::LocalId::try_from_bytes(b"id02").unwrap(); + let new_cid_1 = connection::LocalId::try_from_bytes(b"id02").unwrap(); + let new_cid_2 = connection::LocalId::try_from_bytes(b"id03").unwrap(); let now = NoopClock {}.get_time(); let mut datagram = DatagramInfo { timestamp: now, payload_len: 0, ecn: ExplicitCongestionNotification::default(), - destination_connection_id: new_cid, + destination_connection_id: new_cid_1, destination_connection_id_classification: connection::id::Classification::Local, source_connection_id: None, }; @@ -1040,16 +1044,21 @@ fn active_connection_migration_disabled() { &mut publisher, ); - // The active migration is rejected - assert!(matches!( - res, - Err(DatagramDropReason::RejectedConnectionMigration { - reason: MigrationDenyReason::ConnectionMigrationDisabled { .. } - }) - )); - assert_eq!(1, manager.paths.len()); + // The migration succeeds + assert!(res.is_ok()); + assert_eq!(2, manager.paths.len()); + // The new path uses a new CID since there were enough supplied + assert_eq!( + manager.paths[res.unwrap().0.as_u8() as usize].peer_connection_id, + id_3 + ); + + // Clear the pending packet authentication to allow another migration to proceed + manager.pending_packet_authentication = None; // Try an active connection migration with active migration enabled (default) + datagram.destination_connection_id = new_cid_2; + let res = manager.handle_connection_migration( &new_addr, &datagram, @@ -1062,7 +1071,12 @@ fn active_connection_migration_disabled() { // The migration succeeds assert!(res.is_ok()); - assert_eq!(2, manager.paths.len()); + assert_eq!(3, manager.paths.len()); + // The new path uses the existing id since there wasn't a new one available + assert_eq!( + manager.paths[res.unwrap().0.as_u8() as usize].peer_connection_id, + id_2 + ); // Now try a non-active (passive) migration, with active migration disabled // the same CID is used, so it's not an active migration @@ -1088,7 +1102,12 @@ fn active_connection_migration_disabled() { // The passive migration succeeds assert!(res.is_ok()); - assert_eq!(3, manager.paths.len()); + assert_eq!(4, manager.paths.len()); + // The new path uses the existing id since the peer did not change their destination CID + assert_eq!( + manager.paths[res.unwrap().0.as_u8() as usize].peer_connection_id, + id_2 + ); } #[test]