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

Author: WesleyRosenblum

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,
 }
 

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<api::DatagramDropReason> for DatagramDropReason {

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<Config: endpoint::Config> Manager<Config> {
         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<Config: endpoint::Config> Manager<Config> {
         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

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 }

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]