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/quic/s2n-quic-core/src/connection/limits.rs b/quic/s2n-quic-core/src/connection/limits.rs index 239cef2e4e..f7fffe97df 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::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 { + 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 index adc2e3ac02..b27b7856d8 100644 --- 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 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] 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..f9a9956f74 --- /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,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 } diff --git a/quic/s2n-quic-transport/src/path/manager/tests.rs b/quic/s2n-quic-transport/src/path/manager/tests.rs index 0fc5becff0..678a3a3844 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,112 @@ 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 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) + .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 mut 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, + }; + + // First try an active migration with active migration disabled + let res = manager.handle_connection_migration( + &new_addr, + &datagram, + &mut Default::default(), + &mut migration::allow_all::Validator, + mtu::Config::default(), + // Active connection migration is disabled + &Limits::default() + .with_active_connection_migration(false) + .unwrap(), + &mut publisher, + ); + + // The active migration is rejected + assert!(matches!( + res, + Err(DatagramDropReason::RejectedConnectionMigration) + )); + assert_eq!(1, manager.paths.len()); + + // Try an active connection migration with active migration 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, + ); + + // The migration succeeds + assert!(res.is_ok()); + assert_eq!(2, manager.paths.len()); + + // 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(); + let new_addr = SocketAddress::from(new_addr); + let new_addr = RemoteAddress::from(new_addr); + // Clear the pending packet authentication to allow another migration to proceed + manager.pending_packet_authentication = None; + + let res = manager.handle_connection_migration( + &new_addr, + &datagram, + &mut Default::default(), + &mut migration::allow_all::Validator, + mtu::Config::default(), + // Active connection migration is disabled + &Limits::default() + .with_active_connection_migration(false) + .unwrap(), + &mut publisher, + ); + + // The passive migration succeeds + assert!(res.is_ok()); + assert_eq!(3, manager.paths.len()); +} + #[test] fn connection_migration_challenge_behavior() { // Setup: @@ -1009,7 +1115,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 +1211,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 +1290,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 +1566,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 +1589,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 +1627,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(); 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" ]