From fcd661e1f9ace153c7bfe5e51ae4416fe45a4fc4 Mon Sep 17 00:00:00 2001 From: Wesley Rosenblum Date: Fri, 12 Apr 2024 01:25:38 -0700 Subject: [PATCH 01/12] feat(s2n-quic): allow disabling active connection migration support --- quic/s2n-quic-core/src/connection/limits.rs | 22 ++- .../src/transport/parameters/mod.rs | 12 +- .../src/connection/connection_impl.rs | 2 +- quic/s2n-quic-transport/src/path/manager.rs | 31 ++-- .../src/path/manager/fuzz_target.rs | 3 +- ..._active_connection_migration_disabled.snap | 7 + ..._migration_disabled_passive_migration.snap | 6 + .../src/path/manager/tests.rs | 146 ++++++++++++++++-- .../src/recovery/manager/tests.rs | 3 +- 9 files changed, 204 insertions(+), 28 deletions(-) create mode 100644 quic/s2n-quic-transport/src/path/manager/snapshots/quic__s2n-quic-transport__src__path__manager__tests__events__active_connection_migration_disabled.snap create mode 100644 quic/s2n-quic-transport/src/path/manager/snapshots/quic__s2n-quic-transport__src__path__manager__tests__events__active_connection_migration_disabled_passive_migration.snap diff --git a/quic/s2n-quic-core/src/connection/limits.rs b/quic/s2n-quic-core/src/connection/limits.rs index 239cef2e4e..64693ef371 100644 --- 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::default(), } } @@ -236,6 +238,18 @@ impl Limits { Duration ); setter!(with_max_keep_alive_period, max_keep_alive_period, Duration); + /// Sets if 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_connection_migration(mut self, enabled: bool) -> Result { + 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 +349,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 index adc2e3ac02..546ac4c7ca 100644 --- a/quic/s2n-quic-core/src/transport/parameters/mod.rs +++ b/quic/s2n-quic-core/src/transport/parameters/mod.rs @@ -853,13 +853,18 @@ impl TransportParameterValidator for MaxAckDelay { //# active connection migration (Section 9) on the address being used //# during the handshake. -#[derive(Clone, Copy, Debug, Default, PartialEq)] +#[derive(Clone, Copy, Debug, PartialEq)] pub enum MigrationSupport { - #[default] Enabled, Disabled, } +impl MigrationSupport { + pub const fn default() -> Self { + MigrationSupport::Enabled + } +} + impl TransportParameter for MigrationSupport { type CodecValue = (); @@ -878,7 +883,7 @@ impl TransportParameter for MigrationSupport { } fn default_value() -> Self { - MigrationSupport::Enabled + Self::default() } } @@ -1462,5 +1467,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-transport/src/connection/connection_impl.rs b/quic/s2n-quic-transport/src/connection/connection_impl.rs index ef9bbf1f8b..12995d88e4 100644 --- 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 connection::Trait for ConnectionImpl { 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 index b17450e3cf..a0e7989928 100644 --- 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 Manager { 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 Manager { congestion_controller_endpoint, migration_validator, mtu_config, - initial_rtt, + limits, publisher, ) } @@ -321,7 +321,7 @@ impl Manager { 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 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(), + Err(DatagramDropReason::RejectedConnectionMigration) + ) + } // TODO set alpn if available let attempt: migration::Attempt = migration::AttemptBuilder { @@ -403,19 +414,21 @@ impl Manager { // 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 index de3df6e7ab..9b7cab7082 100644 --- 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/quic/s2n-quic-transport/src/path/manager/snapshots/quic__s2n-quic-transport__src__path__manager__tests__events__active_connection_migration_disabled.snap b/quic/s2n-quic-transport/src/path/manager/snapshots/quic__s2n-quic-transport__src__path__manager__tests__events__active_connection_migration_disabled.snap new file mode 100644 index 0000000000..d2c94b7370 --- /dev/null +++ b/quic/s2n-quic-transport/src/path/manager/snapshots/quic__s2n-quic-transport__src__path__manager__tests__events__active_connection_migration_disabled.snap @@ -0,0 +1,7 @@ +--- +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 } diff --git a/quic/s2n-quic-transport/src/path/manager/snapshots/quic__s2n-quic-transport__src__path__manager__tests__events__active_connection_migration_disabled_passive_migration.snap b/quic/s2n-quic-transport/src/path/manager/snapshots/quic__s2n-quic-transport__src__path__manager__tests__events__active_connection_migration_disabled_passive_migration.snap new file mode 100644 index 0000000000..b6afe5a302 --- /dev/null +++ b/quic/s2n-quic-transport/src/path/manager/snapshots/quic__s2n-quic-transport__src__path__manager__tests__events__active_connection_migration_disabled_passive_migration.snap @@ -0,0 +1,6 @@ +--- +source: quic/s2n-quic-transport/src/path/manager/tests.rs +expression: "" +--- +PathCreated { active: Path { local_addr: 0.0.0.0:0, local_cid: 0x4c6f63616c4900000000000000004c6f63616c49, remote_addr: 127.0.0.1:1, remote_cid: 0x01, id: 0, is_active: true }, new: Path { local_addr: 0.0.0.0:0, local_cid: 0x4c6f63616c4900000000000000004c6f63616c49, remote_addr: 127.0.0.2:1, remote_cid: 0x01, id: 1, is_active: false } } +MtuUpdated { path_id: 1, mtu: 1200, cause: NewPath } diff --git a/quic/s2n-quic-transport/src/path/manager/tests.rs b/quic/s2n-quic-transport/src/path/manager/tests.rs index 0fc5becff0..b6e64961ab 100644 --- a/quic/s2n-quic-transport/src/path/manager/tests.rs +++ b/quic/s2n-quic-transport/src/path/manager/tests.rs @@ -17,7 +17,7 @@ use s2n_quic_core::{ inet::{DatagramInfo, ExplicitCongestionNotification, SocketAddress}, path::{migration, RemoteAddress}, random::{self, Generator}, - recovery::{RttEstimator, DEFAULT_INITIAL_RTT}, + recovery::RttEstimator, stateless_reset::token::testing::*, time::{Clock, NoopClock}, }; @@ -736,7 +736,7 @@ fn test_adding_new_path() { &mut Default::default(), &mut migration::allow_all::Validator, mtu::Config::default(), - DEFAULT_INITIAL_RTT, + &Limits::default(), &mut publisher, ) .unwrap(); @@ -797,7 +797,7 @@ fn do_not_add_new_path_if_handshake_not_confirmed() { &mut Default::default(), &mut migration::allow_all::Validator, mtu::Config::default(), - DEFAULT_INITIAL_RTT, + &Limits::default(), &mut publisher, ); @@ -859,7 +859,7 @@ fn do_not_add_new_path_if_client() { &mut Default::default(), &mut migration::allow_all::Validator, mtu::Config::default(), - DEFAULT_INITIAL_RTT, + &Limits::default(), &mut publisher, ); @@ -950,7 +950,7 @@ fn limit_number_of_connection_migrations() { &mut Default::default(), &mut migration::allow_all::Validator, mtu::Config::default(), - DEFAULT_INITIAL_RTT, + &Limits::default(), &mut publisher, ); match res { @@ -970,6 +970,130 @@ fn limit_number_of_connection_migrations() { assert_eq!(total_paths, MAX_ALLOWED_PATHS); } +#[test] +fn active_connection_migration_disabled() { + // Setup: + let mut publisher = Publisher::snapshot(); + let new_addr: SocketAddr = "127.0.0.1:1".parse().unwrap(); + let new_addr = SocketAddress::from(new_addr); + let new_addr = RemoteAddress::from(new_addr); + let first_path = ServerPath::new( + new_addr, + connection::PeerId::try_from_bytes(&[1]).unwrap(), + connection::LocalId::TEST_ID, + RttEstimator::default(), + Default::default(), + false, + mtu::Config::default(), + ); + let mut manager = manager_server(first_path); + // Give the path manager soms new CIDs so it's able to use one for an active migration + let id_2 = connection::PeerId::try_from_bytes(b"id02").unwrap(); + assert!(manager + .on_new_connection_id(&id_2, 1, 0, &TEST_TOKEN_1, &mut publisher) + .is_ok()); + let id_3 = connection::PeerId::try_from_bytes(b"id03").unwrap(); + assert!(manager + .on_new_connection_id(&id_3, 2, 0, &TEST_TOKEN_2, &mut publisher) + .is_ok()); + + 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 now = NoopClock {}.get_time(); + let datagram = DatagramInfo { + timestamp: now, + payload_len: 0, + ecn: ExplicitCongestionNotification::default(), + destination_connection_id: new_cid, + destination_connection_id_classification: connection::id::Classification::Local, + source_connection_id: None, + }; + + // Active connection migration is disabled + let limits = Limits::default().with_connection_migration(false).unwrap(); + + let res = manager.handle_connection_migration( + &new_addr, + &datagram, + &mut Default::default(), + &mut migration::allow_all::Validator, + mtu::Config::default(), + &limits, + &mut publisher, + ); + + assert!(matches!( + res, + Err(DatagramDropReason::RejectedConnectionMigration) + )); + assert_eq!(1, manager.paths.len()); + + // Active connection migration is enabled (default) + let res = manager.handle_connection_migration( + &new_addr, + &datagram, + &mut Default::default(), + &mut migration::allow_all::Validator, + mtu::Config::default(), + &Limits::default(), + &mut publisher, + ); + + assert!(res.is_ok()); + assert_eq!(2, manager.paths.len()); +} + +#[test] +fn active_connection_migration_disabled_passive_migration() { + // Setup: + let mut publisher = Publisher::snapshot(); + let new_addr: SocketAddr = "127.0.0.1:1".parse().unwrap(); + let new_addr = SocketAddress::from(new_addr); + let new_addr = RemoteAddress::from(new_addr); + let first_path = ServerPath::new( + new_addr, + connection::PeerId::try_from_bytes(&[1]).unwrap(), + connection::LocalId::TEST_ID, + RttEstimator::default(), + Default::default(), + false, + mtu::Config::default(), + ); + let mut manager = manager_server(first_path); + + 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 now = NoopClock {}.get_time(); + let datagram = DatagramInfo { + timestamp: now, + payload_len: 0, + ecn: ExplicitCongestionNotification::default(), + // the same CID is used, so it's not an active migration + destination_connection_id: connection::LocalId::TEST_ID, + destination_connection_id_classification: connection::id::Classification::Local, + source_connection_id: None, + }; + + // Active connection migration is disabled + let limits = Limits::default().with_connection_migration(false).unwrap(); + + let res = manager.handle_connection_migration( + &new_addr, + &datagram, + &mut Default::default(), + &mut migration::allow_all::Validator, + mtu::Config::default(), + &limits, + &mut publisher, + ); + + assert!(res.is_ok()); + assert_eq!(2, manager.paths.len()); +} + #[test] fn connection_migration_challenge_behavior() { // Setup: @@ -1009,7 +1133,7 @@ fn connection_migration_challenge_behavior() { &mut Default::default(), &mut migration::allow_all::Validator, mtu::Config::default(), - DEFAULT_INITIAL_RTT, + &Limits::default(), &mut publisher, ) .unwrap(); @@ -1105,7 +1229,7 @@ fn connection_migration_use_max_ack_delay_from_active_path() { &mut Default::default(), &mut migration::allow_all::Validator, mtu::Config::default(), - DEFAULT_INITIAL_RTT, + &Limits::default(), &mut publisher, ) .unwrap(); @@ -1184,7 +1308,7 @@ fn connection_migration_new_path_abandon_timer() { &mut Default::default(), &mut migration::allow_all::Validator, mtu::Config::default(), - DEFAULT_INITIAL_RTT, + &Limits::default(), &mut publisher, ) .unwrap(); @@ -1460,7 +1584,7 @@ fn temporary_until_authenticated() { &mut Default::default(), &mut migration::allow_all::Validator, mtu::Config::default(), - DEFAULT_INITIAL_RTT, + &Limits::default(), &mut publisher, ) .unwrap(); @@ -1483,7 +1607,7 @@ fn temporary_until_authenticated() { &mut Default::default(), &mut migration::allow_all::Validator, mtu::Config::default(), - DEFAULT_INITIAL_RTT, + &Limits::default(), &mut publisher, ) .unwrap(); @@ -1521,7 +1645,7 @@ fn temporary_until_authenticated() { &mut Default::default(), &mut migration::allow_all::Validator, mtu::Config::default(), - DEFAULT_INITIAL_RTT, + &Limits::default(), &mut publisher, ) .unwrap(); diff --git a/quic/s2n-quic-transport/src/recovery/manager/tests.rs b/quic/s2n-quic-transport/src/recovery/manager/tests.rs index 5b0377bf31..89ad56d4fb 100644 --- a/quic/s2n-quic-transport/src/recovery/manager/tests.rs +++ b/quic/s2n-quic-transport/src/recovery/manager/tests.rs @@ -14,6 +14,7 @@ use bolero::TypeGenerator; use core::{ops::RangeInclusive, time::Duration}; use s2n_quic_core::{ ack, connection, + connection::Limits, event::testing::Publisher, frame::ack_elicitation::AckElicitation, inet::{DatagramInfo, ExplicitCongestionNotification, SocketAddress}, @@ -3359,7 +3360,7 @@ fn helper_generate_multi_path_manager( &mut Endpoint::default(), &mut migration::allow_all::Validator, mtu::Config::default(), - DEFAULT_INITIAL_RTT, + &Limits::default(), publisher, ) .unwrap(); From 0aaacbe5b586ac2cb5ee37487858d5d19b6c2524 Mon Sep 17 00:00:00 2001 From: Wesley Rosenblum Date: Fri, 12 Apr 2024 11:07:44 -0700 Subject: [PATCH 02/12] change comment --- quic/s2n-quic-core/src/connection/limits.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/quic/s2n-quic-core/src/connection/limits.rs b/quic/s2n-quic-core/src/connection/limits.rs index 64693ef371..1a6459639d 100644 --- a/quic/s2n-quic-core/src/connection/limits.rs +++ b/quic/s2n-quic-core/src/connection/limits.rs @@ -238,7 +238,7 @@ impl Limits { Duration ); setter!(with_max_keep_alive_period, max_keep_alive_period, Duration); - /// Sets if active connection migration is supported for a server endpoint (default: true) + /// 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. From 3acda8e72bacefc8591e60a9ec165a56db195234 Mon Sep 17 00:00:00 2001 From: Wesley Rosenblum Date: Tue, 16 Apr 2024 09:08:15 -0700 Subject: [PATCH 03/12] typo --- quic/s2n-quic-transport/src/path/manager/tests.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/quic/s2n-quic-transport/src/path/manager/tests.rs b/quic/s2n-quic-transport/src/path/manager/tests.rs index b6e64961ab..995e0a8458 100644 --- a/quic/s2n-quic-transport/src/path/manager/tests.rs +++ b/quic/s2n-quic-transport/src/path/manager/tests.rs @@ -987,7 +987,7 @@ fn active_connection_migration_disabled() { mtu::Config::default(), ); let mut manager = manager_server(first_path); - // Give the path manager soms new CIDs so it's able to use one for an active migration + // Give the path manager some new CIDs so it's able to use one for an active migration let id_2 = connection::PeerId::try_from_bytes(b"id02").unwrap(); assert!(manager .on_new_connection_id(&id_2, 1, 0, &TEST_TOKEN_1, &mut publisher) From 9929b590fc06f64ec5941a5cc691d8e5d66c55dd Mon Sep 17 00:00:00 2001 From: Wesley Rosenblum Date: Tue, 16 Apr 2024 09:56:36 -0700 Subject: [PATCH 04/12] combine tests --- ..._active_connection_migration_disabled.snap | 2 + .../src/path/manager/tests.rs | 52 +++++-------------- 2 files changed, 14 insertions(+), 40 deletions(-) diff --git a/quic/s2n-quic-transport/src/path/manager/snapshots/quic__s2n-quic-transport__src__path__manager__tests__events__active_connection_migration_disabled.snap b/quic/s2n-quic-transport/src/path/manager/snapshots/quic__s2n-quic-transport__src__path__manager__tests__events__active_connection_migration_disabled.snap index d2c94b7370..f9a9956f74 100644 --- a/quic/s2n-quic-transport/src/path/manager/snapshots/quic__s2n-quic-transport__src__path__manager__tests__events__active_connection_migration_disabled.snap +++ b/quic/s2n-quic-transport/src/path/manager/snapshots/quic__s2n-quic-transport__src__path__manager__tests__events__active_connection_migration_disabled.snap @@ -5,3 +5,5 @@ 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 } diff --git a/quic/s2n-quic-transport/src/path/manager/tests.rs b/quic/s2n-quic-transport/src/path/manager/tests.rs index 995e0a8458..b0172084cf 100644 --- a/quic/s2n-quic-transport/src/path/manager/tests.rs +++ b/quic/s2n-quic-transport/src/path/manager/tests.rs @@ -1002,7 +1002,7 @@ fn active_connection_migration_disabled() { let new_addr = RemoteAddress::from(new_addr); let new_cid = connection::LocalId::try_from_bytes(b"id02").unwrap(); let now = NoopClock {}.get_time(); - let datagram = DatagramInfo { + let mut datagram = DatagramInfo { timestamp: now, payload_len: 0, ecn: ExplicitCongestionNotification::default(), @@ -1011,16 +1011,14 @@ fn active_connection_migration_disabled() { source_connection_id: None, }; - // Active connection migration is disabled - let limits = Limits::default().with_connection_migration(false).unwrap(); - let res = manager.handle_connection_migration( &new_addr, &datagram, &mut Default::default(), &mut migration::allow_all::Validator, mtu::Config::default(), - &limits, + // Active connection migration is disabled + &Limits::default().with_connection_migration(false).unwrap(), &mut publisher, ); @@ -1043,42 +1041,15 @@ fn active_connection_migration_disabled() { assert!(res.is_ok()); assert_eq!(2, manager.paths.len()); -} - -#[test] -fn active_connection_migration_disabled_passive_migration() { - // Setup: - let mut publisher = Publisher::snapshot(); - let new_addr: SocketAddr = "127.0.0.1:1".parse().unwrap(); - let new_addr = SocketAddress::from(new_addr); - let new_addr = RemoteAddress::from(new_addr); - let first_path = ServerPath::new( - new_addr, - connection::PeerId::try_from_bytes(&[1]).unwrap(), - connection::LocalId::TEST_ID, - RttEstimator::default(), - Default::default(), - false, - mtu::Config::default(), - ); - let mut manager = manager_server(first_path); - let new_addr: SocketAddr = "127.0.0.2:1".parse().unwrap(); + // Now try a non-active (passive) migration + // the same CID is used, so it's not an active migration + datagram.destination_connection_id = connection::LocalId::TEST_ID; + let new_addr: SocketAddr = "127.0.0.3:1".parse().unwrap(); let new_addr = SocketAddress::from(new_addr); let new_addr = RemoteAddress::from(new_addr); - let now = NoopClock {}.get_time(); - let datagram = DatagramInfo { - timestamp: now, - payload_len: 0, - ecn: ExplicitCongestionNotification::default(), - // the same CID is used, so it's not an active migration - destination_connection_id: connection::LocalId::TEST_ID, - destination_connection_id_classification: connection::id::Classification::Local, - source_connection_id: None, - }; - - // Active connection migration is disabled - let limits = Limits::default().with_connection_migration(false).unwrap(); + // Clear the pending packet authentication to allow another migration to proceed + manager.pending_packet_authentication = None; let res = manager.handle_connection_migration( &new_addr, @@ -1086,12 +1057,13 @@ fn active_connection_migration_disabled_passive_migration() { &mut Default::default(), &mut migration::allow_all::Validator, mtu::Config::default(), - &limits, + // Active connection migration is disabled + &Limits::default().with_connection_migration(false).unwrap(), &mut publisher, ); assert!(res.is_ok()); - assert_eq!(2, manager.paths.len()); + assert_eq!(3, manager.paths.len()); } #[test] From 718e9495a84914427d9c3028cf0db68fdd30a017 Mon Sep 17 00:00:00 2001 From: Wesley Rosenblum Date: Tue, 16 Apr 2024 10:22:05 -0700 Subject: [PATCH 05/12] more comments --- quic/s2n-quic-transport/src/path/manager/tests.rs | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/quic/s2n-quic-transport/src/path/manager/tests.rs b/quic/s2n-quic-transport/src/path/manager/tests.rs index b0172084cf..6cc7fe9595 100644 --- a/quic/s2n-quic-transport/src/path/manager/tests.rs +++ b/quic/s2n-quic-transport/src/path/manager/tests.rs @@ -1011,6 +1011,7 @@ fn active_connection_migration_disabled() { source_connection_id: None, }; + // First try an active migration with active migration disabled let res = manager.handle_connection_migration( &new_addr, &datagram, @@ -1022,13 +1023,14 @@ fn active_connection_migration_disabled() { &mut publisher, ); + // The active migration is rejected assert!(matches!( res, Err(DatagramDropReason::RejectedConnectionMigration) )); assert_eq!(1, manager.paths.len()); - // Active connection migration is enabled (default) + // Try an active connection migration with active migration enabled (default) let res = manager.handle_connection_migration( &new_addr, &datagram, @@ -1039,10 +1041,11 @@ fn active_connection_migration_disabled() { &mut publisher, ); + // The migration succeeds assert!(res.is_ok()); assert_eq!(2, manager.paths.len()); - // Now try a non-active (passive) migration + // Now try a non-active (passive) migration, with active migration disabled // the same CID is used, so it's not an active migration datagram.destination_connection_id = connection::LocalId::TEST_ID; let new_addr: SocketAddr = "127.0.0.3:1".parse().unwrap(); @@ -1062,6 +1065,7 @@ fn active_connection_migration_disabled() { &mut publisher, ); + // The passive migration succeeds assert!(res.is_ok()); assert_eq!(3, manager.paths.len()); } From c50665ff85627841c98731572665654bbac24540 Mon Sep 17 00:00:00 2001 From: Wesley Rosenblum Date: Tue, 16 Apr 2024 10:23:26 -0700 Subject: [PATCH 06/12] more comments --- quic/s2n-quic-transport/src/path/manager/tests.rs | 2 ++ 1 file changed, 2 insertions(+) diff --git a/quic/s2n-quic-transport/src/path/manager/tests.rs b/quic/s2n-quic-transport/src/path/manager/tests.rs index 6cc7fe9595..a850779a80 100644 --- a/quic/s2n-quic-transport/src/path/manager/tests.rs +++ b/quic/s2n-quic-transport/src/path/manager/tests.rs @@ -988,6 +988,8 @@ fn active_connection_migration_disabled() { ); let mut manager = manager_server(first_path); // Give the path manager some new CIDs so it's able to use one for an active migration + // id_2 will be moved to `InUse` immediately due to the handshake CID rotation feature, + // so id_3 is added as well to have an unused CID available for connection migration let id_2 = connection::PeerId::try_from_bytes(b"id02").unwrap(); assert!(manager .on_new_connection_id(&id_2, 1, 0, &TEST_TOKEN_1, &mut publisher) From 2ab1ac9557de8cedced2d19c383f18ca40ab7283 Mon Sep 17 00:00:00 2001 From: Wesley Rosenblum Date: Tue, 16 Apr 2024 10:42:46 -0700 Subject: [PATCH 07/12] putting default back --- quic/s2n-quic-core/src/connection/limits.rs | 7 +++++-- quic/s2n-quic-core/src/transport/parameters/mod.rs | 7 +++---- quic/s2n-quic-transport/src/path/manager/tests.rs | 4 ++-- 3 files changed, 10 insertions(+), 8 deletions(-) diff --git a/quic/s2n-quic-core/src/connection/limits.rs b/quic/s2n-quic-core/src/connection/limits.rs index 1a6459639d..f7fffe97df 100644 --- a/quic/s2n-quic-core/src/connection/limits.rs +++ b/quic/s2n-quic-core/src/connection/limits.rs @@ -111,7 +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::default(), + migration_support: MigrationSupport::RECOMMENDED, } } @@ -242,7 +242,10 @@ impl Limits { /// /// 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_connection_migration(mut self, enabled: bool) -> Result { + pub fn with_active_connection_migration( + mut self, + enabled: bool, + ) -> Result { if enabled { self.migration_support = MigrationSupport::Enabled } else { diff --git a/quic/s2n-quic-core/src/transport/parameters/mod.rs b/quic/s2n-quic-core/src/transport/parameters/mod.rs index 546ac4c7ca..b27b7856d8 100644 --- a/quic/s2n-quic-core/src/transport/parameters/mod.rs +++ b/quic/s2n-quic-core/src/transport/parameters/mod.rs @@ -853,16 +853,15 @@ impl TransportParameterValidator for MaxAckDelay { //# active connection migration (Section 9) on the address being used //# during the handshake. -#[derive(Clone, Copy, Debug, PartialEq)] +#[derive(Clone, Copy, Debug, Default, PartialEq)] pub enum MigrationSupport { + #[default] Enabled, Disabled, } impl MigrationSupport { - pub const fn default() -> Self { - MigrationSupport::Enabled - } + pub const RECOMMENDED: Self = Self::Enabled; } impl TransportParameter for MigrationSupport { diff --git a/quic/s2n-quic-transport/src/path/manager/tests.rs b/quic/s2n-quic-transport/src/path/manager/tests.rs index a850779a80..c2127b47d7 100644 --- a/quic/s2n-quic-transport/src/path/manager/tests.rs +++ b/quic/s2n-quic-transport/src/path/manager/tests.rs @@ -1021,7 +1021,7 @@ fn active_connection_migration_disabled() { &mut migration::allow_all::Validator, mtu::Config::default(), // Active connection migration is disabled - &Limits::default().with_connection_migration(false).unwrap(), + &Limits::default().with_active_connection_migration(false).unwrap(), &mut publisher, ); @@ -1063,7 +1063,7 @@ fn active_connection_migration_disabled() { &mut migration::allow_all::Validator, mtu::Config::default(), // Active connection migration is disabled - &Limits::default().with_connection_migration(false).unwrap(), + &Limits::default().with_active_connection_migration(false).unwrap(), &mut publisher, ); From 24e9669538dacecf2b86acf2591206f89c3860bd Mon Sep 17 00:00:00 2001 From: Wesley Rosenblum Date: Tue, 16 Apr 2024 11:05:46 -0700 Subject: [PATCH 08/12] format --- quic/s2n-quic-transport/src/path/manager/tests.rs | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/quic/s2n-quic-transport/src/path/manager/tests.rs b/quic/s2n-quic-transport/src/path/manager/tests.rs index c2127b47d7..286f9f4d49 100644 --- a/quic/s2n-quic-transport/src/path/manager/tests.rs +++ b/quic/s2n-quic-transport/src/path/manager/tests.rs @@ -1021,7 +1021,9 @@ fn active_connection_migration_disabled() { &mut migration::allow_all::Validator, mtu::Config::default(), // Active connection migration is disabled - &Limits::default().with_active_connection_migration(false).unwrap(), + &Limits::default() + .with_active_connection_migration(false) + .unwrap(), &mut publisher, ); @@ -1063,7 +1065,9 @@ fn active_connection_migration_disabled() { &mut migration::allow_all::Validator, mtu::Config::default(), // Active connection migration is disabled - &Limits::default().with_active_connection_migration(false).unwrap(), + &Limits::default() + .with_active_connection_migration(false) + .unwrap(), &mut publisher, ); From ee4aa5243ee2696277b3a062b3301bd88c2d07e7 Mon Sep 17 00:00:00 2001 From: Wesley Rosenblum Date: Tue, 16 Apr 2024 11:10:26 -0700 Subject: [PATCH 09/12] update nightly --- .github/workflows/ci.yml | 2 +- tools/xdp/ebpf/rust-toolchain.toml | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 741188e8e1..cd98d04449 100644 --- 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/tools/xdp/ebpf/rust-toolchain.toml b/tools/xdp/ebpf/rust-toolchain.toml index 44d6820055..f687e4880a 100644 --- a/tools/xdp/ebpf/rust-toolchain.toml +++ b/tools/xdp/ebpf/rust-toolchain.toml @@ -1,6 +1,6 @@ [toolchain] # pin the version to prevent random breakage -channel = "nightly-2024-03-05" +channel = "nightly-2024-04-16" # The source code of rustc, provided by the rust-src component, is needed for # building eBPF programs. components = [ "rustc", "cargo", "rust-src" ] From 4f21ca483af1893c642f71c50e6f5645481f19c4 Mon Sep 17 00:00:00 2001 From: Wesley Rosenblum Date: Tue, 16 Apr 2024 11:20:08 -0700 Subject: [PATCH 10/12] fix test due to new s2n-tls version --- quic/s2n-quic-tls/src/tests.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/quic/s2n-quic-tls/src/tests.rs b/quic/s2n-quic-tls/src/tests.rs index b0f34f4a19..4844903d4a 100644 --- 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] From c8be6480469ff80626d0cbea6a5660a6eec637d8 Mon Sep 17 00:00:00 2001 From: Wesley Rosenblum Date: Tue, 16 Apr 2024 11:53:46 -0700 Subject: [PATCH 11/12] remove unused snapshot --- ...ive_connection_migration_disabled_passive_migration.snap | 6 ------ 1 file changed, 6 deletions(-) delete mode 100644 quic/s2n-quic-transport/src/path/manager/snapshots/quic__s2n-quic-transport__src__path__manager__tests__events__active_connection_migration_disabled_passive_migration.snap diff --git a/quic/s2n-quic-transport/src/path/manager/snapshots/quic__s2n-quic-transport__src__path__manager__tests__events__active_connection_migration_disabled_passive_migration.snap b/quic/s2n-quic-transport/src/path/manager/snapshots/quic__s2n-quic-transport__src__path__manager__tests__events__active_connection_migration_disabled_passive_migration.snap deleted file mode 100644 index b6afe5a302..0000000000 --- a/quic/s2n-quic-transport/src/path/manager/snapshots/quic__s2n-quic-transport__src__path__manager__tests__events__active_connection_migration_disabled_passive_migration.snap +++ /dev/null @@ -1,6 +0,0 @@ ---- -source: quic/s2n-quic-transport/src/path/manager/tests.rs -expression: "" ---- -PathCreated { active: Path { local_addr: 0.0.0.0:0, local_cid: 0x4c6f63616c4900000000000000004c6f63616c49, remote_addr: 127.0.0.1:1, remote_cid: 0x01, id: 0, is_active: true }, new: Path { local_addr: 0.0.0.0:0, local_cid: 0x4c6f63616c4900000000000000004c6f63616c49, remote_addr: 127.0.0.2:1, remote_cid: 0x01, id: 1, is_active: false } } -MtuUpdated { path_id: 1, mtu: 1200, cause: NewPath } From d693c5a3e92ac7a8819117dbabcbc91bdf4b5b3b Mon Sep 17 00:00:00 2001 From: Wesley Rosenblum <55108558+WesleyRosenblum@users.noreply.github.com> Date: Tue, 16 Apr 2024 15:23:41 -0700 Subject: [PATCH 12/12] Update tests.rs Co-authored-by: toidiu --- quic/s2n-quic-transport/src/path/manager/tests.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/quic/s2n-quic-transport/src/path/manager/tests.rs b/quic/s2n-quic-transport/src/path/manager/tests.rs index 286f9f4d49..678a3a3844 100644 --- a/quic/s2n-quic-transport/src/path/manager/tests.rs +++ b/quic/s2n-quic-transport/src/path/manager/tests.rs @@ -987,7 +987,7 @@ fn active_connection_migration_disabled() { mtu::Config::default(), ); let mut manager = manager_server(first_path); - // Give the path manager some new CIDs so it's able to use one for an active migration + // Give the path manager some new CIDs so it's able to use one for an active migration. // id_2 will be moved to `InUse` immediately due to the handshake CID rotation feature, // so id_3 is added as well to have an unused CID available for connection migration let id_2 = connection::PeerId::try_from_bytes(b"id02").unwrap();