Skip to content
22 changes: 21 additions & 1 deletion quic/s2n-quic-core/src/connection/limits.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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 {
Expand Down Expand Up @@ -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(),
}
}

Expand Down Expand Up @@ -236,6 +238,18 @@ 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_connection_migration(mut self, enabled: bool) -> Result<Self, ValidationError> {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does it make sense to expose a non-exhaustive enum here instead if its functionally equivalent?

Imagining a wild future where this gets expanded to other values (maybe not):

  • allow active mig (covered in this api)
  • deny active mig (covered in this api)
  • deny passive mig
  • allow up to 5 active mig
  • allow active mig from these IPs
  • ..etc

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it would be better to call this with_active_connection_migration, since we're not completely disabling migration, right?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah, I went back and forth on that a bit. I think I ended up on thinking that the application shouldn't be too concerned about "passive" connection migration, so I left the "active" out of it. But thinking about it, its more obviously related to the disable_active_migration transport parameter with active in it, so I can add it

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Regarding the wild future you are talking about @toidiu, I think at that point we would have to stabilize the path migration validator and add more of the configuration in there. This one is really tied to the transport parameter, which doesn't have any of that configurability

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.
Expand Down Expand Up @@ -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
Expand Down
12 changes: 9 additions & 3 deletions quic/s2n-quic-core/src/transport/parameters/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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]
Comment on lines 856 to 858
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why did we need to change this?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Enabled,
Disabled,
}

impl MigrationSupport {
pub const fn default() -> Self {
MigrationSupport::Enabled
}
}

impl TransportParameter for MigrationSupport {
type CodecValue = ();

Expand All @@ -878,7 +883,7 @@ impl TransportParameter for MigrationSupport {
}

fn default_value() -> Self {
MigrationSupport::Enabled
Self::default()
}
}

Expand Down Expand Up @@ -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);
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -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,
)?;

Expand Down
31 changes: 22 additions & 9 deletions quic/s2n-quic-transport/src/path/manager.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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},
Expand Down Expand Up @@ -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();
Expand Down Expand Up @@ -308,7 +308,7 @@ impl<Config: endpoint::Config> Manager<Config> {
congestion_controller_endpoint,
migration_validator,
mtu_config,
initial_rtt,
limits,
publisher,
)
}
Expand All @@ -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
Expand All @@ -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 {
Expand Down Expand Up @@ -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)?
Expand Down
3 changes: 1 addition & 2 deletions quic/s2n-quic-transport/src/path/manager/fuzz_target.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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,
};
Expand Down Expand Up @@ -178,7 +177,7 @@ impl Model {
&mut Default::default(),
&mut migration_validator,
mtu::Config::default(),
DEFAULT_INITIAL_RTT,
&Limits::default(),
&mut publisher,
) {
Ok(_) => {
Expand Down
Original file line number Diff line number Diff line change
@@ -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 }
Original file line number Diff line number Diff line change
@@ -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 }
Loading