From 2fcb37ae8c04807eb8e3dddff7b8a468871c7c44 Mon Sep 17 00:00:00 2001 From: Lars Eggert Date: Thu, 23 Jan 2025 18:03:22 +0200 Subject: [PATCH] feat: Option to turn off SNI slicing Wireshark can't reassemble sliced CRYPTO frames, which causes QNS tests to fail bcause it then can't parse all packets. This PR adds an option to disable SNI slicing, and we do so by default when running in QNS. --- neqo-bin/src/client/mod.rs | 3 +++ neqo-bin/src/lib.rs | 8 +++++++- neqo-transport/src/connection/mod.rs | 9 ++++++++- neqo-transport/src/connection/params.rs | 14 ++++++++++++++ neqo-transport/src/connection/tests/idle.rs | 2 ++ neqo-transport/src/crypto.rs | 7 +++++-- 6 files changed, 39 insertions(+), 4 deletions(-) diff --git a/neqo-bin/src/client/mod.rs b/neqo-bin/src/client/mod.rs index d196169f3d..18e0865a63 100644 --- a/neqo-bin/src/client/mod.rs +++ b/neqo-bin/src/client/mod.rs @@ -240,6 +240,9 @@ impl Args { self.shared.quic_parameters.quic_version = vec![Version::Version1]; // This is the default for all tests except http3. self.shared.alpn = String::from("hq-interop"); + // Wireshark can't reassemble sliced CRYPTO frames, which causes tests to fail. + // So let's turn that off. + self.shared.quic_parameters.sni_slicing = false; match testcase.as_str() { "http3" => { self.shared.alpn = String::from("h3"); diff --git a/neqo-bin/src/lib.rs b/neqo-bin/src/lib.rs index 9b1a0630dc..ed5fa3e4c2 100644 --- a/neqo-bin/src/lib.rs +++ b/neqo-bin/src/lib.rs @@ -131,6 +131,10 @@ pub struct QuicParameters { #[arg(name = "preferred-address-v6", long)] /// An IPv6 address for the server preferred address. pub preferred_address_v6: Option, + + #[arg(long, default_value = "true")] + /// Whether to slice the SNI. + pub sni_slicing: bool, } #[cfg(any(test, feature = "bench"))] @@ -146,6 +150,7 @@ impl Default for QuicParameters { no_pmtud: false, preferred_address_v4: None, preferred_address_v6: None, + sni_slicing: true, } } } @@ -218,7 +223,8 @@ impl QuicParameters { .idle_timeout(Duration::from_secs(self.idle_timeout)) .cc_algorithm(self.congestion_control) .pacing(!self.no_pacing) - .pmtud(!self.no_pmtud); + .pmtud(!self.no_pmtud) + .sni_slicing(self.sni_slicing); params = if let Some(pa) = self.preferred_address() { params.preferred_address(pa) } else { diff --git a/neqo-transport/src/connection/mod.rs b/neqo-transport/src/connection/mod.rs index 023545354b..dcdbb7bebe 100644 --- a/neqo-transport/src/connection/mod.rs +++ b/neqo-transport/src/connection/mod.rs @@ -2160,6 +2160,7 @@ impl Connection { let frame_stats = &mut stats.frame_tx; self.crypto.write_frame( PacketNumberSpace::ApplicationData, + self.conn_params.sni_slicing_enabled(), builder, tokens, frame_stats, @@ -2294,7 +2295,13 @@ impl Connection { self.write_appdata_frames(builder, &mut tokens); } else { let stats = &mut self.stats.borrow_mut().frame_tx; - self.crypto.write_frame(space, builder, &mut tokens, stats); + self.crypto.write_frame( + space, + self.conn_params.sni_slicing_enabled(), + builder, + &mut tokens, + stats, + ); } } diff --git a/neqo-transport/src/connection/params.rs b/neqo-transport/src/connection/params.rs index ce27440543..5260415b2d 100644 --- a/neqo-transport/src/connection/params.rs +++ b/neqo-transport/src/connection/params.rs @@ -83,6 +83,8 @@ pub struct ConnectionParameters { pacing: bool, /// Whether the connection performs PLPMTUD. pmtud: bool, + /// Whether the connection should use SNI slicing. + sni_slicing: bool, } impl Default for ConnectionParameters { @@ -107,6 +109,7 @@ impl Default for ConnectionParameters { disable_migration: false, pacing: true, pmtud: false, + sni_slicing: true, } } } @@ -367,6 +370,17 @@ impl ConnectionParameters { self } + #[must_use] + pub const fn sni_slicing_enabled(&self) -> bool { + self.sni_slicing + } + + #[must_use] + pub const fn sni_slicing(mut self, sni_slicing: bool) -> Self { + self.sni_slicing = sni_slicing; + self + } + /// # Errors /// When a connection ID cannot be obtained. /// # Panics diff --git a/neqo-transport/src/connection/tests/idle.rs b/neqo-transport/src/connection/tests/idle.rs index b71a9d0ee0..38f657b89b 100644 --- a/neqo-transport/src/connection/tests/idle.rs +++ b/neqo-transport/src/connection/tests/idle.rs @@ -307,6 +307,7 @@ fn idle_caching() { let mut tokens = Vec::new(); server.crypto.streams.write_frame( PacketNumberSpace::Initial, + server.conn_params.sni_slicing_enabled(), &mut builder, &mut tokens, &mut FrameStats::default(), @@ -315,6 +316,7 @@ fn idle_caching() { tokens.clear(); server.crypto.streams.write_frame( PacketNumberSpace::Initial, + server.conn_params.sni_slicing_enabled(), &mut builder, &mut tokens, &mut FrameStats::default(), diff --git a/neqo-transport/src/crypto.rs b/neqo-transport/src/crypto.rs index 64d0e09a3e..ee0547a549 100644 --- a/neqo-transport/src/crypto.rs +++ b/neqo-transport/src/crypto.rs @@ -327,11 +327,13 @@ impl Crypto { pub fn write_frame( &mut self, space: PacketNumberSpace, + sni_slicing: bool, builder: &mut PacketBuilder, tokens: &mut Vec, stats: &mut FrameStats, ) { - self.streams.write_frame(space, builder, tokens, stats); + self.streams + .write_frame(space, sni_slicing, builder, tokens, stats); } pub fn acked(&mut self, token: &CryptoRecoveryToken) { @@ -1475,6 +1477,7 @@ impl CryptoStreams { pub fn write_frame( &mut self, space: PacketNumberSpace, + sni_slicing: bool, builder: &mut PacketBuilder, tokens: &mut Vec, stats: &mut FrameStats, @@ -1506,7 +1509,7 @@ impl CryptoStreams { let cs = self.get_mut(space).unwrap(); if let Some((offset, data)) = cs.tx.next_bytes() { - let written = if offset == 0 { + let written = if sni_slicing && offset == 0 { if let Some(sni) = find_sni(data) { // Cut the crypto data in two at the midpoint of the SNI and swap the chunks. let mid = sni.start + (sni.end - sni.start) / 2;