From 20977ee5fe2593f7e019373555fe362f00d4ce33 Mon Sep 17 00:00:00 2001 From: toidiu Date: Thu, 6 Jun 2024 10:34:50 -0700 Subject: [PATCH] feat(s2n-quic): allow application to configure the anti_amplification_multiplier (#2229) * feat(s2n-quic): configurable amplification limited multiplier * test the limits get applied * clippy * comments --- quic/s2n-quic-core/Cargo.toml | 2 + quic/s2n-quic-core/src/connection/limits.rs | 26 +++++++++++ .../src/connection/connection_impl.rs | 1 + quic/s2n-quic-transport/src/path/manager.rs | 1 + .../src/path/manager/tests.rs | 19 ++++++++ quic/s2n-quic-transport/src/path/mod.rs | 46 +++++++++++++++++-- .../src/recovery/manager/tests.rs | 11 ++++- quic/s2n-quic/Cargo.toml | 2 + 8 files changed, 103 insertions(+), 5 deletions(-) diff --git a/quic/s2n-quic-core/Cargo.toml b/quic/s2n-quic-core/Cargo.toml index d844198c64..7c2f1654be 100644 --- a/quic/s2n-quic-core/Cargo.toml +++ b/quic/s2n-quic-core/Cargo.toml @@ -23,6 +23,8 @@ probe-tracing = ["tracing"] state-tracing = ["tracing"] # This feature enables support for third party congestion controller implementations unstable-congestion-controller = [] +# This feature enables the use of unstable connection limits +unstable-limits = [] usdt = ["dep:probe"] [dependencies] diff --git a/quic/s2n-quic-core/src/connection/limits.rs b/quic/s2n-quic-core/src/connection/limits.rs index f7fffe97df..565a59d2ed 100644 --- a/quic/s2n-quic-core/src/connection/limits.rs +++ b/quic/s2n-quic-core/src/connection/limits.rs @@ -29,6 +29,12 @@ const MAX_HANDSHAKE_DURATION_DEFAULT: Duration = Duration::from_secs(10); //# middleboxes from losing state for UDP flows [GATEWAY]. const MAX_KEEP_ALIVE_PERIOD_DEFAULT: Duration = Duration::from_secs(30); +//= https://www.rfc-editor.org/rfc/rfc9000#section-8.1 +//# Prior to validating the client address, servers MUST NOT send more +//# than three times as many bytes as the number of bytes they have +//# received. +pub const ANTI_AMPLIFICATION_MULTIPLIER: u8 = 3; + #[non_exhaustive] #[derive(Debug)] pub struct ConnectionInfo<'a> { @@ -67,6 +73,7 @@ pub struct Limits { pub(crate) max_datagram_frame_size: MaxDatagramFrameSize, pub(crate) initial_round_trip_time: Duration, pub(crate) migration_support: MigrationSupport, + pub(crate) anti_amplification_multiplier: u8, } impl Default for Limits { @@ -112,6 +119,7 @@ impl Limits { max_datagram_frame_size: MaxDatagramFrameSize::DEFAULT, initial_round_trip_time: recovery::DEFAULT_INITIAL_RTT, migration_support: MigrationSupport::RECOMMENDED, + anti_amplification_multiplier: ANTI_AMPLIFICATION_MULTIPLIER, } } @@ -274,6 +282,18 @@ impl Limits { Ok(self) } + #[cfg(feature = "unstable-limits")] + setter!( + /// Limit how many bytes the Server sends prior to address validation (default: 3) + /// + /// Prior to validating the client address, servers will not send more + /// than `anti_amplification_multiplier` times as many bytes as the + /// number of bytes it has received. + with_anti_amplification_multiplier, + anti_amplification_multiplier, + u8 + ); + // internal APIs #[doc(hidden)] @@ -358,6 +378,12 @@ impl Limits { pub fn active_migration_enabled(&self) -> bool { matches!(self.migration_support, MigrationSupport::Enabled) } + + #[doc(hidden)] + #[inline] + pub fn anti_amplification_multiplier(&self) -> u8 { + self.anti_amplification_multiplier + } } /// Creates limits for a given connection diff --git a/quic/s2n-quic-transport/src/connection/connection_impl.rs b/quic/s2n-quic-transport/src/connection/connection_impl.rs index b62ec7cec3..a1f9842ee9 100644 --- a/quic/s2n-quic-transport/src/connection/connection_impl.rs +++ b/quic/s2n-quic-transport/src/connection/connection_impl.rs @@ -597,6 +597,7 @@ impl connection::Trait for ConnectionImpl { parameters.congestion_controller, peer_validated, parameters.mtu_config, + parameters.limits.anti_amplification_multiplier(), ); let path_manager = path::Manager::new(initial_path, parameters.peer_id_registry); diff --git a/quic/s2n-quic-transport/src/path/manager.rs b/quic/s2n-quic-transport/src/path/manager.rs index a0e7989928..b7f70ddf0b 100644 --- a/quic/s2n-quic-transport/src/path/manager.rs +++ b/quic/s2n-quic-transport/src/path/manager.rs @@ -462,6 +462,7 @@ impl Manager { cc, true, mtu_config, + limits.anti_amplification_multiplier(), ); let amplification_outcome = path.on_bytes_received(datagram.payload_len); diff --git a/quic/s2n-quic-transport/src/path/manager/tests.rs b/quic/s2n-quic-transport/src/path/manager/tests.rs index 678a3a3844..fa790fde45 100644 --- a/quic/s2n-quic-transport/src/path/manager/tests.rs +++ b/quic/s2n-quic-transport/src/path/manager/tests.rs @@ -13,6 +13,7 @@ use crate::{ }; use core::time::Duration; use s2n_quic_core::{ + connection::limits::ANTI_AMPLIFICATION_MULTIPLIER, event::testing::Publisher, inet::{DatagramInfo, ExplicitCongestionNotification, SocketAddress}, path::{migration, RemoteAddress}, @@ -60,6 +61,7 @@ fn get_path_by_address_test() { Default::default(), false, mtu::Config::default(), + ANTI_AMPLIFICATION_MULTIPLIER, ); let second_conn_id = connection::PeerId::try_from_bytes(&[5, 4, 3, 2, 1]).unwrap(); @@ -71,6 +73,7 @@ fn get_path_by_address_test() { Default::default(), false, mtu::Config::default(), + ANTI_AMPLIFICATION_MULTIPLIER, ); let mut manager = manager_server(first_path.clone()); @@ -102,6 +105,7 @@ fn test_invalid_path_fallback() { Default::default(), false, mtu::Config::default(), + ANTI_AMPLIFICATION_MULTIPLIER, ); // simulate receiving a handshake packet to force path validation first_path.on_handshake_packet(); @@ -118,6 +122,7 @@ fn test_invalid_path_fallback() { Default::default(), false, mtu::Config::default(), + ANTI_AMPLIFICATION_MULTIPLIER, ); second_path.set_challenge(challenge); @@ -501,6 +506,7 @@ fn silently_return_when_there_is_no_valid_path() { Default::default(), false, mtu::Config::default(), + ANTI_AMPLIFICATION_MULTIPLIER, ); first_path.set_challenge(challenge); let mut manager = manager_server(first_path); @@ -708,6 +714,7 @@ fn test_adding_new_path() { Default::default(), false, mtu::Config::default(), + ANTI_AMPLIFICATION_MULTIPLIER, ); let mut manager = manager_server(first_path); @@ -771,6 +778,7 @@ fn do_not_add_new_path_if_handshake_not_confirmed() { Default::default(), false, mtu::Config::default(), + ANTI_AMPLIFICATION_MULTIPLIER, ); let mut manager = manager_server(first_path); @@ -833,6 +841,7 @@ fn do_not_add_new_path_if_client() { Default::default(), false, mtu::Config::default(), + ANTI_AMPLIFICATION_MULTIPLIER, ); let mut manager = manager_client(first_path); let mut publisher = Publisher::snapshot(); @@ -888,6 +897,7 @@ fn switch_destination_connection_id_after_first_server_response() { Default::default(), false, mtu::Config::default(), + ANTI_AMPLIFICATION_MULTIPLIER, ); let mut manager = manager_client(zero_path); assert_eq!(manager[zero_path_id].peer_connection_id, initial_cid); @@ -926,6 +936,7 @@ fn limit_number_of_connection_migrations() { Default::default(), false, mtu::Config::default(), + ANTI_AMPLIFICATION_MULTIPLIER, ); let mut manager = manager_server(first_path); let mut total_paths = 1; @@ -985,6 +996,7 @@ fn active_connection_migration_disabled() { Default::default(), false, mtu::Config::default(), + ANTI_AMPLIFICATION_MULTIPLIER, ); 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. @@ -1092,6 +1104,7 @@ fn connection_migration_challenge_behavior() { Default::default(), false, mtu::Config::default(), + ANTI_AMPLIFICATION_MULTIPLIER, ); let mut manager = manager_server(first_path); @@ -1187,6 +1200,7 @@ fn connection_migration_use_max_ack_delay_from_active_path() { Default::default(), false, mtu::Config::default(), + ANTI_AMPLIFICATION_MULTIPLIER, ); let mut manager = manager_server(first_path); @@ -1267,6 +1281,7 @@ fn connection_migration_new_path_abandon_timer() { Default::default(), false, mtu::Config::default(), + ANTI_AMPLIFICATION_MULTIPLIER, ); let mut manager = manager_server(first_path); @@ -1389,6 +1404,7 @@ fn stop_using_a_retired_connection_id() { Default::default(), false, mtu::Config::default(), + ANTI_AMPLIFICATION_MULTIPLIER, ); let mut manager = manager_server(first_path); @@ -1485,6 +1501,7 @@ fn pending_paths_should_return_paths_pending_validation() { Default::default(), false, mtu::Config::default(), + ANTI_AMPLIFICATION_MULTIPLIER, ); let expected_response_data = [0; 8]; third_path.on_path_challenge(&expected_response_data); @@ -1550,6 +1567,7 @@ fn temporary_until_authenticated() { Default::default(), false, mtu::Config::default(), + ANTI_AMPLIFICATION_MULTIPLIER, ); let mut manager = manager_server(first_path); @@ -1936,6 +1954,7 @@ pub fn helper_path(peer_id: connection::PeerId) -> ServerPath { Default::default(), false, mtu::Config::default(), + ANTI_AMPLIFICATION_MULTIPLIER, ) } diff --git a/quic/s2n-quic-transport/src/path/mod.rs b/quic/s2n-quic-transport/src/path/mod.rs index d46ab460f4..b02e801d4b 100644 --- a/quic/s2n-quic-transport/src/path/mod.rs +++ b/quic/s2n-quic-transport/src/path/mod.rs @@ -78,6 +78,7 @@ pub struct Path { /// True if the path is currently active is_active: bool, + anti_amplification_multiplier: u8, } impl Clone for Path { @@ -97,6 +98,7 @@ impl Clone for Path { response_data: self.response_data, activated: self.activated, is_active: self.is_active, + anti_amplification_multiplier: self.anti_amplification_multiplier, } } } @@ -113,6 +115,7 @@ impl Path { congestion_controller: ::CongestionController, peer_validated: bool, mtu_config: mtu::Config, + anti_amplification_multiplier: u8, ) -> Path { let state = match Config::ENDPOINT_TYPE { Type::Server => { @@ -144,6 +147,7 @@ impl Path { response_data: None, activated: false, is_active: false, + anti_amplification_multiplier, } } @@ -212,9 +216,9 @@ impl Path { //# Prior to validating the client address, servers MUST NOT send more //# than three times as many bytes as the number of bytes they have //# received. - // if let State::AmplificationLimited { tx_allowance } = &mut self.state { - *tx_allowance += bytes.saturating_mul(3) as u32; + *tx_allowance += + bytes.saturating_mul(self.anti_amplification_multiplier as usize) as u32; } let unblocked = was_at_amplification_limit && !self.at_amplification_limit(); @@ -595,7 +599,10 @@ impl transmission::interest::Provider for Path pub mod testing { use crate::{endpoint, path::Path}; use core::time::Duration; - use s2n_quic_core::{connection, path::mtu, recovery::RttEstimator}; + use s2n_quic_core::{ + connection, connection::limits::ANTI_AMPLIFICATION_MULTIPLIER, path::mtu, + recovery::RttEstimator, + }; pub fn helper_path_server() -> Path { Path::new( @@ -606,6 +613,7 @@ pub mod testing { Default::default(), true, mtu::Config::default(), + ANTI_AMPLIFICATION_MULTIPLIER, ) } @@ -618,6 +626,7 @@ pub mod testing { Default::default(), false, mtu::Config::default(), + ANTI_AMPLIFICATION_MULTIPLIER, ) } } @@ -633,7 +642,9 @@ mod tests { }; use core::time::Duration; use s2n_quic_core::{ - connection, endpoint, + connection, + connection::limits::ANTI_AMPLIFICATION_MULTIPLIER, + endpoint, event::testing::Publisher, path::MINIMUM_MAX_DATAGRAM_SIZE, recovery::{CongestionController, RttEstimator}, @@ -643,6 +654,32 @@ mod tests { type Path = super::Path; + #[test] + fn custom_anti_amplification_multiplier() { + let bytes_received = 100; + + // allowance should increase based on the default anti_amplification_multiplier + let mut default_path = testing::helper_path_server(); + let _ = default_path.on_bytes_received(bytes_received); + let allowance = match default_path.state { + path::State::AmplificationLimited { tx_allowance } => tx_allowance, + _ => unreachable!("path is amplification limited"), + }; + let expected = ANTI_AMPLIFICATION_MULTIPLIER as u32 * bytes_received as u32; + assert_eq!(allowance, Counter::new(expected)); + + // allowance should increase based on the custom anti_amplification_multiplier + let mut custom_path = testing::helper_path_server(); + custom_path.anti_amplification_multiplier = ANTI_AMPLIFICATION_MULTIPLIER + 10; + let _ = custom_path.on_bytes_received(bytes_received); + let allowance = match custom_path.state { + path::State::AmplificationLimited { tx_allowance } => tx_allowance, + _ => unreachable!("path is amplification limited"), + }; + let expected = (ANTI_AMPLIFICATION_MULTIPLIER + 10) as u32 * bytes_received as u32; + assert_eq!(allowance, Counter::new(expected)); + } + #[test] fn response_data_should_only_be_sent_once() { // Setup: @@ -1123,6 +1160,7 @@ mod tests { Default::default(), false, mtu::Config::default(), + ANTI_AMPLIFICATION_MULTIPLIER, ); let now = NoopClock.get_time(); let random = &mut random::testing::Generator::default(); diff --git a/quic/s2n-quic-transport/src/recovery/manager/tests.rs b/quic/s2n-quic-transport/src/recovery/manager/tests.rs index 89ad56d4fb..608a8411d9 100644 --- a/quic/s2n-quic-transport/src/recovery/manager/tests.rs +++ b/quic/s2n-quic-transport/src/recovery/manager/tests.rs @@ -3,7 +3,9 @@ use super::*; use crate::{ - connection::{ConnectionIdMapper, InternalConnectionIdGenerator}, + connection::{ + limits::ANTI_AMPLIFICATION_MULTIPLIER, ConnectionIdMapper, InternalConnectionIdGenerator, + }, endpoint::{ self, testing::{Client as ClientConfig, Server as ServerConfig}, @@ -59,6 +61,7 @@ fn one_second_pto_when_no_previous_rtt_available() { Default::default(), false, mtu::Config::default(), + ANTI_AMPLIFICATION_MULTIPLIER, ); manager @@ -437,6 +440,7 @@ fn on_ack_frame() { MockCongestionController::default(), false, mtu::Config::default(), + ANTI_AMPLIFICATION_MULTIPLIER, ); context.path_manager.activate_path_for_test(path_id); context.path_mut().pto_backoff = 2; @@ -2672,6 +2676,7 @@ fn update_pto_timer() { MockCongestionController::default(), false, mtu::Config::default(), + ANTI_AMPLIFICATION_MULTIPLIER, ); context.path_manager.activate_path_for_test(path_id); // simulate receiving a handshake packet to force path validation @@ -2765,6 +2770,7 @@ fn pto_armed_if_handshake_not_confirmed() { Default::default(), false, mtu::Config::default(), + ANTI_AMPLIFICATION_MULTIPLIER, ); path_manager.activate_path_for_test(path_id); @@ -2793,6 +2799,7 @@ fn pto_must_be_at_least_k_granularity() { Default::default(), false, mtu::Config::default(), + ANTI_AMPLIFICATION_MULTIPLIER, ); // Update RTT with the smallest possible sample @@ -3426,6 +3433,7 @@ fn helper_generate_path_manager_with_first_addr( MockCongestionController::new(first_addr), true, mtu::Config::default(), + ANTI_AMPLIFICATION_MULTIPLIER, ); path::Manager::new(path, registry) @@ -3449,6 +3457,7 @@ fn helper_generate_client_path_manager( MockCongestionController::new(first_addr), false, mtu::Config::default(), + ANTI_AMPLIFICATION_MULTIPLIER, ); path::Manager::new(path, registry) diff --git a/quic/s2n-quic/Cargo.toml b/quic/s2n-quic/Cargo.toml index 810fc1e751..02425a623a 100644 --- a/quic/s2n-quic/Cargo.toml +++ b/quic/s2n-quic/Cargo.toml @@ -57,6 +57,8 @@ unstable-provider-random = [] unstable-provider-dc = [] # This feature enables support for third party congestion controller implementations unstable-congestion-controller = ["s2n-quic-core/unstable-congestion-controller"] +# This feature enables the use of unstable connection limits +unstable-limits = ["s2n-quic-core/unstable-limits"] [dependencies] bytes = { version = "1", default-features = false }