feat(s2n-quic): allow disabling active connection migration support (#…

…2182) * feat(s2n-quic): allow disabling active connection migration support * change comment * typo * combine tests * more comments * more comments * putting default back * format * update nightly * fix test due to new s2n-tls version * remove unused snapshot * Update tests.rs Co-authored-by: toidiu <[email protected]> --------- Co-authored-by: toidiu <[email protected]>
aws · Apr 17, 2024 · 23b07e4 · 23b07e4
1 parent 5f88e54
commit 23b07e4
Show file tree

Hide file tree

Showing 11 changed files with 185 additions and 29 deletions.
diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml
@@ -15,7 +15,7 @@ env:
   RUST_BACKTRACE: 1
   # Pin the nightly toolchain to prevent breakage.
   # This should be occasionally updated.
-  RUST_NIGHTLY_TOOLCHAIN: nightly-2024-03-21
+  RUST_NIGHTLY_TOOLCHAIN: nightly-2024-04-16
   CDN: https://dnglbrstg7yg.cloudfront.net
   # enable unstable features for testing
   S2N_UNSTABLE_CRYPTO_OPT_TX: 100

diff --git a/quic/s2n-quic-core/src/connection/limits.rs b/quic/s2n-quic-core/src/connection/limits.rs
@@ -9,7 +9,7 @@ use crate::{
         AckDelayExponent, ActiveConnectionIdLimit, InitialFlowControlLimits, InitialMaxData,
         InitialMaxStreamDataBidiLocal, InitialMaxStreamDataBidiRemote, InitialMaxStreamDataUni,
         InitialMaxStreamsBidi, InitialMaxStreamsUni, InitialStreamLimits, MaxAckDelay,
-        MaxDatagramFrameSize, MaxIdleTimeout, TransportParameters,
+        MaxDatagramFrameSize, MaxIdleTimeout, MigrationSupport, TransportParameters,
     },
 };
 use core::time::Duration;
@@ -66,6 +66,7 @@ pub struct Limits {
     pub(crate) max_keep_alive_period: Duration,
     pub(crate) max_datagram_frame_size: MaxDatagramFrameSize,
     pub(crate) initial_round_trip_time: Duration,
+    pub(crate) migration_support: MigrationSupport,
 }
 
 impl Default for Limits {
@@ -110,6 +111,7 @@ impl Limits {
             max_keep_alive_period: MAX_KEEP_ALIVE_PERIOD_DEFAULT,
             max_datagram_frame_size: MaxDatagramFrameSize::DEFAULT,
             initial_round_trip_time: recovery::DEFAULT_INITIAL_RTT,
+            migration_support: MigrationSupport::RECOMMENDED,
         }
     }
 
@@ -236,6 +238,21 @@ impl Limits {
         Duration
     );
     setter!(with_max_keep_alive_period, max_keep_alive_period, Duration);
+    /// Sets whether active connection migration is supported for a server endpoint (default: true)
+    ///
+    /// If set to false, the `disable_active_migration` transport parameter will be sent to the
+    /// peer, and any attempt by the peer to perform an active connection migration will be ignored.
+    pub fn with_active_connection_migration(
+        mut self,
+        enabled: bool,
+    ) -> Result<Self, ValidationError> {
+        if enabled {
+            self.migration_support = MigrationSupport::Enabled
+        } else {
+            self.migration_support = MigrationSupport::Disabled
+        }
+        Ok(self)
+    }
 
     /// Sets the initial round trip time (RTT) for use in recovery mechanisms prior to
     /// measuring an actual RTT sample.
@@ -335,6 +352,12 @@ impl Limits {
     pub fn initial_round_trip_time(&self) -> Duration {
         self.initial_round_trip_time
     }
+
+    #[doc(hidden)]
+    #[inline]
+    pub fn active_migration_enabled(&self) -> bool {
+        matches!(self.migration_support, MigrationSupport::Enabled)
+    }
 }
 
 /// Creates limits for a given connection

diff --git a/quic/s2n-quic-core/src/transport/parameters/mod.rs b/quic/s2n-quic-core/src/transport/parameters/mod.rs
@@ -860,6 +860,10 @@ pub enum MigrationSupport {
     Disabled,
 }
 
+impl MigrationSupport {
+    pub const RECOMMENDED: Self = Self::Enabled;
+}
+
 impl TransportParameter for MigrationSupport {
     type CodecValue = ();
 
@@ -878,7 +882,7 @@ impl TransportParameter for MigrationSupport {
     }
 
     fn default_value() -> Self {
-        MigrationSupport::Enabled
+        Self::default()
     }
 }
 
@@ -1462,5 +1466,6 @@ impl<
         load!(ack_delay_exponent, ack_delay_exponent);
         load!(max_active_connection_ids, active_connection_id_limit);
         load!(max_datagram_frame_size, max_datagram_frame_size);
+        load!(migration_support, migration_support);
     }
 }
diff --git a/quic/s2n-quic-tls/src/tests.rs b/quic/s2n-quic-tls/src/tests.rs
@@ -403,7 +403,7 @@ fn s2n_client_no_client_auth_s2n_server_requires_client_auth_test() {
     // but the client does not support it.
     assert!(test_result.is_err());
     let e = test_result.unwrap_err();
-    assert_eq!(e.description().unwrap(), "UNEXPECTED_MESSAGE");
+    assert_eq!(e.description().unwrap(), "CERTIFICATE_REQUIRED");
 }
 
 #[test]

diff --git a/quic/s2n-quic-transport/src/connection/connection_impl.rs b/quic/s2n-quic-transport/src/connection/connection_impl.rs
@@ -1154,7 +1154,7 @@ impl<Config: endpoint::Config> connection::Trait for ConnectionImpl<Config> {
             congestion_controller_endpoint,
             path_migration,
             mtu_config,
-            self.limits.initial_round_trip_time(),
+            &self.limits,
             &mut publisher,
         )?;
 

diff --git a/quic/s2n-quic-transport/src/path/manager.rs b/quic/s2n-quic-transport/src/path/manager.rs
@@ -9,10 +9,10 @@ use crate::{
     path::{challenge, Path},
     transmission,
 };
-use core::time::Duration;
 use s2n_quic_core::{
     ack,
-    connection::{self, PeerId},
+    connection::{self, Limits, PeerId},
+    ensure,
     event::{
         self,
         builder::{DatagramDropReason, MtuUpdatedCause},
@@ -245,7 +245,7 @@ impl<Config: endpoint::Config> Manager<Config> {
         congestion_controller_endpoint: &mut Config::CongestionControllerEndpoint,
         migration_validator: &mut Config::PathMigrationValidator,
         mtu_config: mtu::Config,
-        initial_rtt: Duration,
+        limits: &Limits,
         publisher: &mut Pub,
     ) -> Result<(Id, AmplificationOutcome), DatagramDropReason> {
         let valid_initial_received = self.valid_initial_received();
@@ -308,7 +308,7 @@ impl<Config: endpoint::Config> Manager<Config> {
             congestion_controller_endpoint,
             migration_validator,
             mtu_config,
-            initial_rtt,
+            limits,
             publisher,
         )
     }
@@ -321,7 +321,7 @@ impl<Config: endpoint::Config> Manager<Config> {
         congestion_controller_endpoint: &mut Config::CongestionControllerEndpoint,
         migration_validator: &mut Config::PathMigrationValidator,
         mtu_config: mtu::Config,
-        initial_rtt: Duration,
+        limits: &Limits,
         publisher: &mut Pub,
     ) -> Result<(Id, AmplificationOutcome), DatagramDropReason> {
         //= https://www.rfc-editor.org/rfc/rfc9000#section-9
@@ -332,6 +332,17 @@ 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(),
+                Err(DatagramDropReason::RejectedConnectionMigration)
+            )
+        }
 
         // TODO set alpn if available
         let attempt: migration::Attempt = migration::AttemptBuilder {
@@ -403,19 +414,21 @@ impl<Config: endpoint::Config> Manager<Config> {
         // estimator for the new path, and they are initialized with initial values,
         // we do not need to reset congestion controller and round-trip time estimator
         // again on confirming the peer's ownership of its new address.
-        let rtt = self.active_path().rtt_estimator.for_new_path(initial_rtt);
+        let rtt = self
+            .active_path()
+            .rtt_estimator
+            .for_new_path(limits.initial_round_trip_time());
         let path_info =
             congestion_controller::PathInfo::new(mtu_config.initial_mtu, &remote_address);
         let cc = congestion_controller_endpoint.new_congestion_controller(path_info);
 
         let peer_connection_id = {
-            if self.active_path().local_connection_id != datagram.destination_connection_id {
+            if active_migration {
                 //= 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.
 
-                // Peer has intentionally tried to migrate to this new path because they changed
-                // their destination_connection_id, so we will change our destination_connection_id as well.
+                // Active connection migrations must use a new connection ID
                 self.peer_id_registry
                     .consume_new_id_for_new_path()
                     .ok_or(DatagramDropReason::InsufficientConnectionIds)?

diff --git a/quic/s2n-quic-transport/src/path/manager/fuzz_target.rs b/quic/s2n-quic-transport/src/path/manager/fuzz_target.rs
@@ -15,7 +15,6 @@ use s2n_quic_core::{
     inet::{DatagramInfo, ExplicitCongestionNotification},
     random,
     random::testing::Generator,
-    recovery::DEFAULT_INITIAL_RTT,
     time::{testing::Clock, Clock as _},
     transport,
 };
@@ -178,7 +177,7 @@ impl Model {
             &mut Default::default(),
             &mut migration_validator,
             mtu::Config::default(),
-            DEFAULT_INITIAL_RTT,
+            &Limits::default(),
             &mut publisher,
         ) {
             Ok(_) => {

diff --git a/...c-transport__src__path__manager__tests__events__active_connection_migration_disabled.snap b/...c-transport__src__path__manager__tests__events__active_connection_migration_disabled.snap
@@ -0,0 +1,9 @@
+---
+source: quic/s2n-quic-transport/src/path/manager/tests.rs
+expression: ""
+---
+ConnectionIdUpdated { path_id: 0, cid_consumer: Local, previous: 0x01, current: 0x69643032 }
+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 }
+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 } }
+MtuUpdated { path_id: 2, mtu: 1200, cause: NewPath }