From 9b7e52b9dd39bd93cea366fdc0091a6e8b0daaea Mon Sep 17 00:00:00 2001 From: Wesley Rosenblum Date: Mon, 1 Jul 2024 17:02:31 -0700 Subject: [PATCH 1/3] feat(s2n-quic-dc): Handle secret control packets in dc Endpoint --- dc/s2n-quic-dc/src/path/secret/map.rs | 27 ++++++++++++++++++++- quic/s2n-quic-core/src/dc.rs | 19 +++++++++++++++ quic/s2n-quic-core/src/dc/disabled.rs | 10 +++++++- quic/s2n-quic-core/src/dc/testing.rs | 10 +++++++- quic/s2n-quic-core/src/dc/traits.rs | 10 ++++++++ quic/s2n-quic-transport/src/endpoint/mod.rs | 11 +++++++++ 6 files changed, 84 insertions(+), 3 deletions(-) diff --git a/dc/s2n-quic-dc/src/path/secret/map.rs b/dc/s2n-quic-dc/src/path/secret/map.rs index 22cba586f6..5a64cc4e95 100644 --- a/dc/s2n-quic-dc/src/path/secret/map.rs +++ b/dc/s2n-quic-dc/src/path/secret/map.rs @@ -14,7 +14,8 @@ use crate::{ use rand::Rng as _; use s2n_codec::EncoderBuffer; use s2n_quic_core::{ - dc::{self, ApplicationParams}, + dc::{self, ApplicationParams, DatagramInfo}, + ensure, event::api::EndpointType, }; use std::{ @@ -782,6 +783,30 @@ impl dc::Endpoint for Map { fn new_path(&mut self, connection_info: &dc::ConnectionInfo) -> Option { Some(HandshakingPath::new(connection_info, self.clone())) } + + fn on_possible_secret_control_packet( + &mut self, + // TODO: Maybe we should confirm that the sender IP at least matches the IP for the + // corresponding control secret? + _datagram_info: &DatagramInfo, + payload: &mut [u8], + ) -> bool { + let payload = s2n_codec::DecoderBufferMut::new(payload); + // TODO: Is 16 always right? + return match control::Packet::decode(payload, 16) { + Ok((packet, tail)) => { + // Probably a bug somewhere? There shouldn't be anything trailing in the buffer + // after we decode a secret control packet. + ensure!(tail.is_empty(), false); + + // If we successfully decoded a control packet, pass it into our map to handle. + self.handle_control_packet(&packet); + + true + } + Err(_) => false, + }; + } } impl dc::Path for HandshakingPath { diff --git a/quic/s2n-quic-core/src/dc.rs b/quic/s2n-quic-core/src/dc.rs index b1c5e1de50..7164f6ceb0 100644 --- a/quic/s2n-quic-core/src/dc.rs +++ b/quic/s2n-quic-core/src/dc.rs @@ -71,6 +71,25 @@ impl<'a> ConnectionInfo<'a> { } } +/// Information about a received datagram that may be used +/// when parsing it for a secret control packet +#[derive(Clone, Debug)] +#[non_exhaustive] +pub struct DatagramInfo<'a> { + /// The address (IP + Port) of the remote peer + pub remote_address: SocketAddress<'a>, +} + +impl<'a> DatagramInfo<'a> { + #[inline] + #[doc(hidden)] + pub fn new( remote_address: &'a inet::SocketAddress) -> Self { + Self { + remote_address: remote_address.into_event() + } + } +} + /// Various settings relevant to the dc path #[derive(Clone, Copy, Debug)] #[non_exhaustive] diff --git a/quic/s2n-quic-core/src/dc/disabled.rs b/quic/s2n-quic-core/src/dc/disabled.rs index b98af98879..f5a132f777 100644 --- a/quic/s2n-quic-core/src/dc/disabled.rs +++ b/quic/s2n-quic-core/src/dc/disabled.rs @@ -3,7 +3,7 @@ use crate::{ crypto::tls::TlsSession, - dc::{ConnectionInfo, Endpoint, Path}, + dc::{ConnectionInfo, DatagramInfo, Endpoint, Path}, stateless_reset, transport, }; use alloc::vec::Vec; @@ -19,6 +19,14 @@ impl Endpoint for Disabled { fn new_path(&mut self, _connection_info: &ConnectionInfo) -> Option { None } + + fn on_possible_secret_control_packet( + &mut self, + _datagram_info: &DatagramInfo, + _payload: &mut [u8], + ) -> bool { + unreachable!() + } } // The Disabled Endpoint returns `None`, so this is not used diff --git a/quic/s2n-quic-core/src/dc/testing.rs b/quic/s2n-quic-core/src/dc/testing.rs index cc2c581ba6..bbbb8bf0d0 100644 --- a/quic/s2n-quic-core/src/dc/testing.rs +++ b/quic/s2n-quic-core/src/dc/testing.rs @@ -4,7 +4,7 @@ use crate::{ crypto::tls::TlsSession, dc, - dc::{ApplicationParams, ConnectionInfo}, + dc::{ApplicationParams, ConnectionInfo, DatagramInfo}, stateless_reset, transport, varint::VarInt, }; @@ -39,6 +39,14 @@ impl dc::Endpoint for MockDcEndpoint { ..Default::default() }) } + + fn on_possible_secret_control_packet( + &mut self, + _datagram_info: &DatagramInfo, + _payload: &mut [u8], + ) -> bool { + false + } } impl dc::Path for MockDcPath { diff --git a/quic/s2n-quic-core/src/dc/traits.rs b/quic/s2n-quic-core/src/dc/traits.rs index de1010428c..8dab48b969 100644 --- a/quic/s2n-quic-core/src/dc/traits.rs +++ b/quic/s2n-quic-core/src/dc/traits.rs @@ -17,6 +17,16 @@ pub trait Endpoint: 'static + Send { /// /// Return `None` if dc should not be used for this path fn new_path(&mut self, connection_info: &dc::ConnectionInfo) -> Option; + + /// Called when a datagram arrives that cannot be decoded as a non-DC QUIC packet, and + /// thus may contain a secret control packet + /// + /// Return `true` if a secret control packet was decoded from the datagram, `false` otherwise + fn on_possible_secret_control_packet( + &mut self, + datagram_info: &dc::DatagramInfo, + payload: &mut [u8], + ) -> bool; } /// A dc path diff --git a/quic/s2n-quic-transport/src/endpoint/mod.rs b/quic/s2n-quic-transport/src/endpoint/mod.rs index 8388fa87b0..3003476605 100644 --- a/quic/s2n-quic-transport/src/endpoint/mod.rs +++ b/quic/s2n-quic-transport/src/endpoint/mod.rs @@ -459,6 +459,17 @@ impl Endpoint { ) { (packet, remaining) } else { + if Cfg::DcEndpoint::ENABLED + && endpoint_context.dc.on_possible_secret_control_packet( + &dc::DatagramInfo::new(&remote_address), + payload, + ) + { + // This was a DC secret control packet, so we don't need to proceed + // with checking for a stateless reset + return; + } + //= https://www.rfc-editor.org/rfc/rfc9000#section-5.2.2 //# Servers MUST drop incoming packets under all other circumstances. From 368d0581b49ab9cd094b25b206dda7113b879ac1 Mon Sep 17 00:00:00 2001 From: Wesley Rosenblum Date: Mon, 1 Jul 2024 17:07:42 -0700 Subject: [PATCH 2/3] fmt --- quic/s2n-quic-core/src/dc.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/quic/s2n-quic-core/src/dc.rs b/quic/s2n-quic-core/src/dc.rs index 7164f6ceb0..14a88c5884 100644 --- a/quic/s2n-quic-core/src/dc.rs +++ b/quic/s2n-quic-core/src/dc.rs @@ -83,9 +83,9 @@ pub struct DatagramInfo<'a> { impl<'a> DatagramInfo<'a> { #[inline] #[doc(hidden)] - pub fn new( remote_address: &'a inet::SocketAddress) -> Self { + pub fn new(remote_address: &'a inet::SocketAddress) -> Self { Self { - remote_address: remote_address.into_event() + remote_address: remote_address.into_event(), } } } From e0bcda8bb3d0f320a6f543b17dd231a695c45381 Mon Sep 17 00:00:00 2001 From: Wesley Rosenblum Date: Mon, 1 Jul 2024 17:37:11 -0700 Subject: [PATCH 3/3] collapse if else --- quic/s2n-quic-transport/src/endpoint/mod.rs | 21 +++++++++------------ 1 file changed, 9 insertions(+), 12 deletions(-) diff --git a/quic/s2n-quic-transport/src/endpoint/mod.rs b/quic/s2n-quic-transport/src/endpoint/mod.rs index 3003476605..d405ff9f6a 100644 --- a/quic/s2n-quic-transport/src/endpoint/mod.rs +++ b/quic/s2n-quic-transport/src/endpoint/mod.rs @@ -458,18 +458,15 @@ impl Endpoint { endpoint_context.connection_id_format, ) { (packet, remaining) + } else if Cfg::DcEndpoint::ENABLED + && endpoint_context + .dc + .on_possible_secret_control_packet(&dc::DatagramInfo::new(&remote_address), payload) + { + // This was a DC secret control packet, so we don't need to proceed + // with checking for a stateless reset + return; } else { - if Cfg::DcEndpoint::ENABLED - && endpoint_context.dc.on_possible_secret_control_packet( - &dc::DatagramInfo::new(&remote_address), - payload, - ) - { - // This was a DC secret control packet, so we don't need to proceed - // with checking for a stateless reset - return; - } - //= https://www.rfc-editor.org/rfc/rfc9000#section-5.2.2 //# Servers MUST drop incoming packets under all other circumstances. @@ -495,7 +492,7 @@ impl Endpoint { len: payload_len as u16, reason: event::builder::DatagramDropReason::DecodingFailed, }); - } + }; return; };