Skip to content

Commit

Permalink
feat: emit migration deny reason with datagram drop event
Browse files Browse the repository at this point in the history
  • Loading branch information
camshaft committed Jan 24, 2025
1 parent 36fbf86 commit 408549a
Show file tree
Hide file tree
Showing 6 changed files with 93 additions and 10 deletions.
2 changes: 1 addition & 1 deletion quic/s2n-quic-core/events/common.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down
8 changes: 5 additions & 3 deletions quic/s2n-quic-core/src/event/generated.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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 {},
Expand Down Expand Up @@ -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."]
Expand All @@ -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 {},
}
Expand Down
10 changes: 8 additions & 2 deletions quic/s2n-quic-transport/src/path/manager.rs
Original file line number Diff line number Diff line change
Expand Up @@ -338,7 +338,11 @@ impl<Config: endpoint::Config> Manager<Config> {
if active_migration {
ensure!(
limits.active_migration_enabled(),
Err(DatagramDropReason::RejectedConnectionMigration)
Err(DatagramDropReason::RejectedConnectionMigration {
reason: migration::DenyReason::ConnectionMigrationDisabled
.into_event()
.reason
})
)
}

Expand Down Expand Up @@ -367,7 +371,9 @@ impl<Config: endpoint::Config> Manager<Config> {
}
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");
Expand Down
2 changes: 1 addition & 1 deletion quic/s2n-quic-transport/src/path/manager/fuzz_target.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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),
};
Expand Down
6 changes: 4 additions & 2 deletions quic/s2n-quic-transport/src/path/manager/tests.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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},
Expand Down Expand Up @@ -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());

Expand Down
75 changes: 74 additions & 1 deletion quic/s2n-quic/src/tests/connection_migration.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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},
};
Expand Down Expand Up @@ -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 { .. }));
}
}
}

0 comments on commit 408549a

Please sign in to comment.