From 408549a9f65c4f45ff04be7d28a4b89083818917 Mon Sep 17 00:00:00 2001 From: Cameron Bytheway Date: Thu, 23 Jan 2025 17:23:23 -0700 Subject: [PATCH] feat: emit migration deny reason with datagram drop event --- quic/s2n-quic-core/events/common.rs | 2 +- quic/s2n-quic-core/src/event/generated.rs | 8 +- quic/s2n-quic-transport/src/path/manager.rs | 10 ++- .../src/path/manager/fuzz_target.rs | 2 +- .../src/path/manager/tests.rs | 6 +- .../src/tests/connection_migration.rs | 75 ++++++++++++++++++- 6 files changed, 93 insertions(+), 10 deletions(-) diff --git a/quic/s2n-quic-core/events/common.rs b/quic/s2n-quic-core/events/common.rs index 7f1c173091..195cf30920 100644 --- a/quic/s2n-quic-core/events/common.rs +++ b/quic/s2n-quic-core/events/common.rs @@ -784,7 +784,7 @@ enum DatagramDropReason { /// The peer initiated a connection migration before the handshake was confirmed. ConnectionMigrationDuringHandshake, /// The attempted connection migration was rejected. - RejectedConnectionMigration, + RejectedConnectionMigration { reason: MigrationDenyReason }, /// The maximum number of paths per connection was exceeded. PathLimitExceeded, /// The peer initiated a connection migration without supplying enough connection IDs to use. diff --git a/quic/s2n-quic-core/src/event/generated.rs b/quic/s2n-quic-core/src/event/generated.rs index f3e27d4d1d..c00b48ef4b 100644 --- a/quic/s2n-quic-core/src/event/generated.rs +++ b/quic/s2n-quic-core/src/event/generated.rs @@ -878,7 +878,7 @@ pub mod api { ConnectionMigrationDuringHandshake {}, #[non_exhaustive] #[doc = " The attempted connection migration was rejected."] - RejectedConnectionMigration {}, + RejectedConnectionMigration { reason: MigrationDenyReason }, #[non_exhaustive] #[doc = " The maximum number of paths per connection was exceeded."] PathLimitExceeded {}, @@ -4985,7 +4985,7 @@ pub mod builder { #[doc = " The peer initiated a connection migration before the handshake was confirmed."] ConnectionMigrationDuringHandshake, #[doc = " The attempted connection migration was rejected."] - RejectedConnectionMigration, + RejectedConnectionMigration { reason: MigrationDenyReason }, #[doc = " The maximum number of paths per connection was exceeded."] PathLimitExceeded, #[doc = " The peer initiated a connection migration without supplying enough connection IDs to use."] @@ -5010,7 +5010,9 @@ pub mod builder { Self::RejectedConnectionAttempt => RejectedConnectionAttempt {}, Self::UnknownServerAddress => UnknownServerAddress {}, Self::ConnectionMigrationDuringHandshake => ConnectionMigrationDuringHandshake {}, - Self::RejectedConnectionMigration => RejectedConnectionMigration {}, + Self::RejectedConnectionMigration { reason } => RejectedConnectionMigration { + reason: reason.into_event(), + }, Self::PathLimitExceeded => PathLimitExceeded {}, Self::InsufficientConnectionIds => InsufficientConnectionIds {}, } diff --git a/quic/s2n-quic-transport/src/path/manager.rs b/quic/s2n-quic-transport/src/path/manager.rs index e2f901a11e..4400227ca8 100644 --- a/quic/s2n-quic-transport/src/path/manager.rs +++ b/quic/s2n-quic-transport/src/path/manager.rs @@ -338,7 +338,11 @@ impl Manager { if active_migration { ensure!( limits.active_migration_enabled(), - Err(DatagramDropReason::RejectedConnectionMigration) + Err(DatagramDropReason::RejectedConnectionMigration { + reason: migration::DenyReason::ConnectionMigrationDisabled + .into_event() + .reason + }) ) } @@ -367,7 +371,9 @@ impl Manager { } migration::Outcome::Deny(reason) => { publisher.on_connection_migration_denied(reason.into_event()); - return Err(DatagramDropReason::RejectedConnectionMigration); + return Err(DatagramDropReason::RejectedConnectionMigration { + reason: reason.into_event().reason, + }); } _ => { unimplemented!("unimplemented migration outcome"); 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 411d41a438..84f5ff101c 100644 --- a/quic/s2n-quic-transport/src/path/manager/fuzz_target.rs +++ b/quic/s2n-quic-transport/src/path/manager/fuzz_target.rs @@ -196,7 +196,7 @@ impl Model { match datagram_drop_reason { // Ignore errors emitted by the migration::validator and peer_id_registry DatagramDropReason::InsufficientConnectionIds => {} - DatagramDropReason::RejectedConnectionMigration => {} + DatagramDropReason::RejectedConnectionMigration { .. } => {} DatagramDropReason::PathLimitExceeded => {} datagram_drop_reason => panic!("{:?}", datagram_drop_reason), }; diff --git a/quic/s2n-quic-transport/src/path/manager/tests.rs b/quic/s2n-quic-transport/src/path/manager/tests.rs index 217e939902..4f20ce49a0 100644 --- a/quic/s2n-quic-transport/src/path/manager/tests.rs +++ b/quic/s2n-quic-transport/src/path/manager/tests.rs @@ -14,7 +14,7 @@ use crate::{ use core::time::Duration; use s2n_quic_core::{ connection::limits::ANTI_AMPLIFICATION_MULTIPLIER, - event::testing::Publisher, + event::{builder::MigrationDenyReason, testing::Publisher}, inet::{DatagramInfo, ExplicitCongestionNotification, SocketAddress}, path::{migration, RemoteAddress}, random::{self, Generator}, @@ -1043,7 +1043,9 @@ fn active_connection_migration_disabled() { // The active migration is rejected assert!(matches!( res, - Err(DatagramDropReason::RejectedConnectionMigration) + Err(DatagramDropReason::RejectedConnectionMigration { + reason: MigrationDenyReason::ConnectionMigrationDisabled { .. } + }) )); assert_eq!(1, manager.paths.len()); diff --git a/quic/s2n-quic/src/tests/connection_migration.rs b/quic/s2n-quic/src/tests/connection_migration.rs index 09cb75c06a..64c7aad58f 100644 --- a/quic/s2n-quic/src/tests/connection_migration.rs +++ b/quic/s2n-quic/src/tests/connection_migration.rs @@ -4,7 +4,7 @@ use super::*; use s2n_codec::DecoderBufferMut; use s2n_quic_core::{ - event::api::{DatagramDropReason, Subject}, + event::api::{DatagramDropReason, MigrationDenyReason, Subject}, packet::interceptor::{Datagram, Interceptor}, path::{LocalAddress, RemoteAddress}, }; @@ -373,3 +373,76 @@ fn rebind_ipv4_mapped_before_handshake_confirmed() { } } } + +/// Rebinds to a port after a specified number of packets +struct RebindToPort { + port: u16, + after: usize, +} + +impl Interceptor for RebindToPort { + fn intercept_rx_remote_address(&mut self, _subject: &Subject, addr: &mut RemoteAddress) { + if self.after == 0 { + addr.set_port(self.port); + } + } + + fn intercept_rx_datagram<'a>( + &mut self, + _subject: &Subject, + _datagram: &Datagram, + payload: DecoderBufferMut<'a>, + ) -> DecoderBufferMut<'a> { + self.after = self.after.saturating_sub(1); + payload + } +} + +/// Ensures that a blocked port is not migrated to +#[test] +fn rebind_blocked_port() { + let model = Model::default(); + let subscriber = recorder::DatagramDropped::new(); + let datagram_dropped_events = subscriber.events(); + + test(model, move |handle| { + let server = Server::builder() + .with_io(handle.builder().build()?)? + .with_tls(SERVER_CERTS)? + .with_event((tracing_events(), subscriber))? + .with_random(Random::with_seed(456))? + .with_packet_interceptor(RebindToPort { port: 53, after: 2 })? + .start()?; + + let client = Client::builder() + .with_io(handle.builder().build()?)? + .with_tls(certificates::CERT_PEM)? + .with_event(tracing_events())? + .with_random(Random::with_seed(456))? + .start()?; + + let addr = start_server(server)?; + + primary::spawn(async move { + let mut connection = client + .connect(Connect::new(addr).with_server_name("localhost")) + .await + .unwrap(); + let mut stream = connection.open_bidirectional_stream().await.unwrap(); + let _ = stream.send(Bytes::from_static(b"hello")).await; + let _ = stream.finish(); + let _ = stream.receive().await; + }); + + Ok(addr) + }) + .unwrap(); + + let datagram_dropped_events = datagram_dropped_events.lock().unwrap(); + + for event in datagram_dropped_events.iter() { + if let DatagramDropReason::RejectedConnectionMigration { reason, .. } = &event.reason { + assert!(matches!(reason, MigrationDenyReason::BlockedPort { .. })); + } + } +}