From ce1e79f3fa064cf14d4db8dfc6ea4a49fd4e792c Mon Sep 17 00:00:00 2001 From: Lars Eggert Date: Wed, 16 Oct 2024 11:39:05 +0300 Subject: [PATCH 1/4] fix: Build QNS docker image with debug symbols (#2181) So panic stack traces are useful. --- qns/Dockerfile | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/qns/Dockerfile b/qns/Dockerfile index 66ab75d790..60eeaa8e66 100644 --- a/qns/Dockerfile +++ b/qns/Dockerfile @@ -33,7 +33,7 @@ RUN cargo chef cook --release --recipe-path recipe.json ADD . /neqo RUN set -eux; \ cd /neqo; \ - cargo build --release --bin neqo-client --bin neqo-server + CARGO_PROFILE_RELEASE_DEBUG=true cargo build --release --bin neqo-client --bin neqo-server # Copy only binaries to the final image to keep it small. From 62415bfc0cd1065d4e142eb67769120d69f22943 Mon Sep 17 00:00:00 2001 From: Max Inden Date: Thu, 17 Oct 2024 15:54:29 +0200 Subject: [PATCH 2/4] ci: fix cache-hit check (#2186) According to the actions/cache docs an empty string represents a cache miss: > cache-hit - A string value to indicate an exact match was found for the key. > - If there's a cache hit, this will be 'true' or 'false' to indicate if there's an exact match for key. > - If there's a cache miss, this will be an empty string. https://github.com/actions/cache?tab=readme-ov-file#outputs Previously Neqo's CI would check for `"false"`, now it checks for `""`. --- .github/actions/nss/action.yml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.github/actions/nss/action.yml b/.github/actions/nss/action.yml index 24768eac22..1bca72d3d2 100644 --- a/.github/actions/nss/action.yml +++ b/.github/actions/nss/action.yml @@ -120,7 +120,7 @@ runs: if: env.USE_SYSTEM_NSS == '0' shell: bash run: | - if [ "${{ runner.environment }}" != "github-hosted" ] || [ "${{ steps.cache.outputs.cache-hit }}" == "false" ]; then + if [ "${{ runner.environment }}" != "github-hosted" ] || [ "${{ steps.cache.outputs.cache-hit }}" == "" ]; then echo "Building NSS from source" echo "BUILD_NSS=1" >> "$GITHUB_ENV" else From c4e8f0694795afd6e991878a4a55a871575264db Mon Sep 17 00:00:00 2001 From: Lars Eggert Date: Fri, 18 Oct 2024 11:09:45 +0300 Subject: [PATCH 3/4] test: Make sure our PATH_CHALLENGEs are padded OK (#2168) --- .../src/connection/tests/ackrate.rs | 3 +- neqo-transport/src/connection/tests/ecn.rs | 14 ++++++++-- .../src/connection/tests/migration.rs | 28 +++++++++++++++++-- neqo-transport/src/connection/tests/mod.rs | 23 ++++++++++++++- 4 files changed, 60 insertions(+), 8 deletions(-) diff --git a/neqo-transport/src/connection/tests/ackrate.rs b/neqo-transport/src/connection/tests/ackrate.rs index f0a1d17cd9..c39b01895b 100644 --- a/neqo-transport/src/connection/tests/ackrate.rs +++ b/neqo-transport/src/connection/tests/ackrate.rs @@ -13,7 +13,7 @@ use super::{ ack_bytes, connect_rtt_idle, default_client, default_server, fill_cwnd, increase_cwnd, induce_persistent_congestion, new_client, new_server, send_something, DEFAULT_RTT, }; -use crate::stream_id::StreamType; +use crate::{connection::tests::assert_path_challenge_min_len, stream_id::StreamType}; /// With the default RTT here (100ms) and default ratio (4), endpoints won't send /// `ACK_FREQUENCY` as the ACK delay isn't different enough from the default. @@ -169,6 +169,7 @@ fn migrate_ack_delay() { let client1 = send_something(&mut client, now); assertions::assert_v4_path(&client1, true); // Contains PATH_CHALLENGE. + assert_path_challenge_min_len(&client, &client1, now); let client2 = send_something(&mut client, now); assertions::assert_v4_path(&client2, false); // Doesn't. Is dropped. now += DEFAULT_RTT / 2; diff --git a/neqo-transport/src/connection/tests/ecn.rs b/neqo-transport/src/connection/tests/ecn.rs index ca99282cf5..089c7e45fd 100644 --- a/neqo-transport/src/connection/tests/ecn.rs +++ b/neqo-transport/src/connection/tests/ecn.rs @@ -14,9 +14,10 @@ use test_fixture::{ use crate::{ connection::tests::{ - connect_force_idle, connect_force_idle_with_modifier, default_client, default_server, - handshake_with_modifier, migration::get_cid, new_client, new_server, send_and_receive, - send_something, send_something_with_modifier, send_with_modifier_and_receive, DEFAULT_RTT, + assert_path_challenge_min_len, connect_force_idle, connect_force_idle_with_modifier, + default_client, default_server, handshake_with_modifier, migration::get_cid, new_client, + new_server, send_and_receive, send_something, send_something_with_modifier, + send_with_modifier_and_receive, DEFAULT_RTT, }, ecn::ECN_TEST_COUNT, path::MAX_PATH_PROBES, @@ -120,6 +121,7 @@ fn migration_delay_to_ecn_blackhole() { // This should be a PATH_CHALLENGE. probes += 1; assert_eq!(client.stats().frame_tx.path_challenge, probes); + assert_path_challenge_min_len(&client, &d, now); if probes <= MAX_PATH_PROBES { // The first probes should be sent with ECN. assert_ecn_enabled(d.tos()); @@ -247,6 +249,7 @@ pub fn migration_with_modifiers( let probe = new_path_modifier(client.process_output(now).dgram().unwrap()); if let Some(probe) = probe { assert_v4_path(&probe, true); // Contains PATH_CHALLENGE. + assert_path_challenge_min_len(&client, &probe, now); assert_eq!(client.stats().frame_tx.path_challenge, 1); let probe_cid = ConnectionId::from(get_cid(&probe)); @@ -254,6 +257,7 @@ pub fn migration_with_modifiers( assert_v4_path(&resp, true); assert_eq!(server.stats().frame_tx.path_response, 1); assert_eq!(server.stats().frame_tx.path_challenge, 1); + assert_path_challenge_min_len(&server, &resp, now); // Data continues to be exchanged on the old path. let client_data = send_something_with_modifier(&mut client, now, orig_path_modifier); @@ -287,6 +291,10 @@ pub fn migration_with_modifiers( server.stats().frame_tx.path_challenge, if migrated { 2 } else { 0 } ); + if migrated { + assert_path_challenge_min_len(&server, &probe_old_server, now); + } + assert_eq!( server.stats().frame_tx.stream, if migrated { stream_before } else { 1 } diff --git a/neqo-transport/src/connection/tests/migration.rs b/neqo-transport/src/connection/tests/migration.rs index 541ab7dfeb..87683a5c62 100644 --- a/neqo-transport/src/connection/tests/migration.rs +++ b/neqo-transport/src/connection/tests/migration.rs @@ -25,7 +25,7 @@ use super::{ }; use crate::{ cid::LOCAL_ACTIVE_CID_LIMIT, - connection::tests::{send_something_paced, send_with_extra}, + connection::tests::{assert_path_challenge_min_len, send_something_paced, send_with_extra}, frame::FRAME_TYPE_NEW_CONNECTION_ID, packet::PacketBuilder, path::MAX_PATH_PROBES, @@ -102,10 +102,12 @@ fn path_forwarding_attack() { // The server now probes the new (primary) path. let new_probe = server.process_output(now).dgram().unwrap(); assert_eq!(server.stats().frame_tx.path_challenge, 1); + assert_path_challenge_min_len(&server, &new_probe, now); assert_v4_path(&new_probe, false); // Can't be padded. // The server also probes the old path. let old_probe = server.process_output(now).dgram().unwrap(); + assert_path_challenge_min_len(&server, &old_probe, now); assert_eq!(server.stats().frame_tx.path_challenge, 2); assert_v6_path(&old_probe, true); @@ -140,6 +142,7 @@ fn path_forwarding_attack() { let server_data1 = server.process(Some(&new_resp), now).dgram().unwrap(); assert_v4_path(&server_data1, true); assert_eq!(server.stats().frame_tx.path_challenge, 3); + assert_path_challenge_min_len(&server, &server_data1, now); // The client responds to this probe on the new path. client.process_input(&server_data1, now); @@ -179,6 +182,8 @@ fn migrate_immediate() { let client1 = send_something(&mut client, now); assert_v4_path(&client1, true); // Contains PATH_CHALLENGE. + assert_path_challenge_min_len(&client, &client1, now); + let client2 = send_something(&mut client, now); assert_v4_path(&client2, false); // Doesn't. @@ -236,6 +241,7 @@ fn migrate_immediate_fail() { let probe = client.process_output(now).dgram().unwrap(); assert_v4_path(&probe, true); // Contains PATH_CHALLENGE. + assert_path_challenge_min_len(&client, &probe, now); // -1 because first PATH_CHALLENGE already sent above for _ in 0..MAX_PATH_PROBES * 2 - 1 { @@ -246,6 +252,7 @@ fn migrate_immediate_fail() { let before = client.stats().frame_tx; let probe = client.process_output(now).dgram().unwrap(); assert_v4_path(&probe, true); // Contains PATH_CHALLENGE. + assert_path_challenge_min_len(&client, &probe, now); let after = client.stats().frame_tx; assert_eq!(after.path_challenge, before.path_challenge + 1); assert_eq!(after.padding, before.padding + 1); @@ -253,8 +260,9 @@ fn migrate_immediate_fail() { // This might be a PTO, which will result in sending a probe. if let Some(probe) = client.process_output(now).dgram() { - assert_v4_path(&probe, false); // Contains PATH_CHALLENGE. + assert_v4_path(&probe, false); // Contains PING. let after = client.stats().frame_tx; + assert_eq!(after.path_challenge, before.path_challenge + 1); assert_eq!(after.ping, before.ping + 1); assert_eq!(after.all(), before.all() + 3); } @@ -286,6 +294,7 @@ fn migrate_same() { let probe = client.process_output(now).dgram().unwrap(); assert_v6_path(&probe, true); // Contains PATH_CHALLENGE. assert_eq!(client.stats().frame_tx.path_challenge, 1); + assert_path_challenge_min_len(&client, &probe, now); let resp = server.process(Some(&probe), now).dgram().unwrap(); assert_v6_path(&resp, true); @@ -312,6 +321,7 @@ fn migrate_same_fail() { let probe = client.process_output(now).dgram().unwrap(); assert_v6_path(&probe, true); // Contains PATH_CHALLENGE. + assert_path_challenge_min_len(&client, &probe, now); // -1 because first PATH_CHALLENGE already sent above for _ in 0..MAX_PATH_PROBES * 2 - 1 { @@ -322,6 +332,7 @@ fn migrate_same_fail() { let before = client.stats().frame_tx; let probe = client.process_output(now).dgram().unwrap(); assert_v6_path(&probe, true); // Contains PATH_CHALLENGE. + assert_path_challenge_min_len(&client, &probe, now); let after = client.stats().frame_tx; assert_eq!(after.path_challenge, before.path_challenge + 1); assert_eq!(after.padding, before.padding + 1); @@ -329,8 +340,9 @@ fn migrate_same_fail() { // This might be a PTO, which will result in sending a probe. if let Some(probe) = client.process_output(now).dgram() { - assert_v6_path(&probe, false); // Contains PATH_CHALLENGE. + assert_v6_path(&probe, false); // Contains PING. let after = client.stats().frame_tx; + assert_eq!(after.path_challenge, before.path_challenge + 1); assert_eq!(after.ping, before.ping + 1); assert_eq!(after.all(), before.all() + 3); } @@ -368,11 +380,13 @@ fn migration(mut client: Connection) { let probe = client.process_output(now).dgram().unwrap(); assert_v4_path(&probe, true); // Contains PATH_CHALLENGE. + assert_path_challenge_min_len(&client, &probe, now); assert_eq!(client.stats().frame_tx.path_challenge, 1); let probe_cid = ConnectionId::from(get_cid(&probe)); let resp = server.process(Some(&probe), now).dgram().unwrap(); assert_v4_path(&resp, true); + assert_path_challenge_min_len(&server, &resp, now); assert_eq!(server.stats().frame_tx.path_response, 1); assert_eq!(server.stats().frame_tx.path_challenge, 1); @@ -399,6 +413,7 @@ fn migration(mut client: Connection) { let probe_old_server = send_something(&mut server, now); // This is just the double-check probe; no STREAM frames. assert_v6_path(&probe_old_server, true); + assert_path_challenge_min_len(&server, &probe_old_server, now); assert_eq!(server.stats().frame_tx.path_challenge, 2); assert_eq!(server.stats().frame_tx.stream, stream_before); @@ -515,6 +530,7 @@ fn preferred_address(hs_client: SocketAddr, hs_server: SocketAddr, preferred: So let probe = client.process(dgram.as_ref(), now()).dgram().unwrap(); assert_toward_spa(&probe, true); assert_eq!(client.stats().frame_tx.path_challenge, 1); + assert_path_challenge_min_len(&client, &probe, now()); assert_ne!(client.process_output(now()).callback(), Duration::new(0, 0)); // Data continues on the main path for the client. @@ -525,6 +541,7 @@ fn preferred_address(hs_client: SocketAddr, hs_server: SocketAddr, preferred: So let resp = server.process(Some(&probe), now()).dgram().unwrap(); assert_from_spa(&resp, true); assert_eq!(server.stats().frame_tx.path_challenge, 1); + assert_path_challenge_min_len(&server, &resp, now()); assert_eq!(server.stats().frame_tx.path_response, 1); // Data continues on the main path for the server. @@ -544,6 +561,7 @@ fn preferred_address(hs_client: SocketAddr, hs_server: SocketAddr, preferred: So let probe = server.process(Some(&data), now()).dgram().unwrap(); assert_orig_path(&probe, true); assert_eq!(server.stats().frame_tx.path_challenge, 2); + assert_path_challenge_min_len(&server, &probe, now()); // But data now goes on the new path. let data = send_something(&mut server, now()); @@ -854,6 +872,7 @@ fn retire_prior_to_migration_failure() { let probe = client.process_output(now()).dgram().unwrap(); assert_v4_path(&probe, true); assert_eq!(client.stats().frame_tx.path_challenge, 1); + assert_path_challenge_min_len(&client, &probe, now()); let probe_cid = ConnectionId::from(get_cid(&probe)); assert_ne!(original_cid, probe_cid); @@ -865,6 +884,7 @@ fn retire_prior_to_migration_failure() { assert_v4_path(&resp, true); assert_eq!(server.stats().frame_tx.path_response, 1); assert_eq!(server.stats().frame_tx.path_challenge, 1); + assert_path_challenge_min_len(&server, &resp, now()); // Have the client receive the NEW_CONNECTION_ID with Retire Prior To. client.process_input(&retire_all, now()); @@ -907,6 +927,7 @@ fn retire_prior_to_migration_success() { let probe = client.process_output(now()).dgram().unwrap(); assert_v4_path(&probe, true); assert_eq!(client.stats().frame_tx.path_challenge, 1); + assert_path_challenge_min_len(&client, &probe, now()); let probe_cid = ConnectionId::from(get_cid(&probe)); assert_ne!(original_cid, probe_cid); @@ -918,6 +939,7 @@ fn retire_prior_to_migration_success() { assert_v4_path(&resp, true); assert_eq!(server.stats().frame_tx.path_response, 1); assert_eq!(server.stats().frame_tx.path_challenge, 1); + assert_path_challenge_min_len(&server, &resp, now()); // Have the client receive the NEW_CONNECTION_ID with Retire Prior To second. // As this occurs in a very specific order, migration succeeds. diff --git a/neqo-transport/src/connection/tests/mod.rs b/neqo-transport/src/connection/tests/mod.rs index c2cf4db391..7aaced917a 100644 --- a/neqo-transport/src/connection/tests/mod.rs +++ b/neqo-transport/src/connection/tests/mod.rs @@ -30,7 +30,7 @@ use crate::{ stats::{FrameStats, Stats, MAX_PTO_COUNTS}, tparams::{DISABLE_MIGRATION, GREASE_QUIC_BIT}, ConnectionIdDecoder, ConnectionIdGenerator, ConnectionParameters, Error, StreamId, StreamType, - Version, + Version, MIN_INITIAL_PACKET_SIZE, }; // All the tests. @@ -669,6 +669,27 @@ fn assert_default_stats(stats: &Stats) { assert_eq!(stats.frame_tx, dflt_frames); } +fn assert_path_challenge_min_len(c: &Connection, d: &Datagram, now: Instant) { + let path = c.paths.find_path( + d.source(), + d.destination(), + c.conn_params.get_cc_algorithm(), + c.conn_params.pacing_enabled(), + now, + ); + if path.borrow().amplification_limit() < path.borrow().plpmtu() { + // If the amplification limit is less than the PLPMTU, then the path + // challenge will not have been padded. + return; + } + assert!( + d.len() >= MIN_INITIAL_PACKET_SIZE, + "{} < {}", + d.len(), + MIN_INITIAL_PACKET_SIZE + ); +} + #[test] fn create_client() { let client = default_client(); From 5d53446401b08b8392132116d0e496bdc421d331 Mon Sep 17 00:00:00 2001 From: Max Inden Date: Fri, 18 Oct 2024 10:40:37 +0200 Subject: [PATCH 4/4] refactor(udp): pass SocketAddr instead of &SocketAddr (#2169) `SocketAddr` implements `Copy`. https://doc.rust-lang.org/std/net/enum.SocketAddr.html#impl-Copy-for-SocketAddr Thus there is no need to pass by reference instead of by value. --- neqo-bin/src/client/mod.rs | 2 +- neqo-bin/src/server/mod.rs | 2 +- neqo-bin/src/udp.rs | 2 +- neqo-udp/src/lib.rs | 12 ++++++------ 4 files changed, 9 insertions(+), 9 deletions(-) diff --git a/neqo-bin/src/client/mod.rs b/neqo-bin/src/client/mod.rs index 8bfac59703..ad5407e2cc 100644 --- a/neqo-bin/src/client/mod.rs +++ b/neqo-bin/src/client/mod.rs @@ -457,7 +457,7 @@ impl<'a, H: Handler> Runner<'a, H> { async fn process_multiple_input(&mut self) -> Res<()> { loop { - let dgrams = self.socket.recv(&self.local_addr)?; + let dgrams = self.socket.recv(self.local_addr)?; if dgrams.is_empty() { break; } diff --git a/neqo-bin/src/server/mod.rs b/neqo-bin/src/server/mod.rs index abf614f1f8..a6104daa5f 100644 --- a/neqo-bin/src/server/mod.rs +++ b/neqo-bin/src/server/mod.rs @@ -289,7 +289,7 @@ impl ServerRunner { match self.ready().await? { Ready::Socket(inx) => loop { let (host, socket) = self.sockets.get_mut(inx).unwrap(); - let dgrams = socket.recv(host)?; + let dgrams = socket.recv(*host)?; if dgrams.is_empty() { break; } diff --git a/neqo-bin/src/udp.rs b/neqo-bin/src/udp.rs index 148ff43175..bc14aadcf3 100644 --- a/neqo-bin/src/udp.rs +++ b/neqo-bin/src/udp.rs @@ -55,7 +55,7 @@ impl Socket { /// Receive a batch of [`Datagram`]s on the given [`Socket`], each set with /// the provided local address. - pub fn recv(&self, local_address: &SocketAddr) -> Result, io::Error> { + pub fn recv(&self, local_address: SocketAddr) -> Result, io::Error> { self.inner .try_io(tokio::io::Interest::READABLE, || { neqo_udp::recv_inner(local_address, &self.state, &self.inner) diff --git a/neqo-udp/src/lib.rs b/neqo-udp/src/lib.rs index 5f1fb3dbe6..dea2b26721 100644 --- a/neqo-udp/src/lib.rs +++ b/neqo-udp/src/lib.rs @@ -58,7 +58,7 @@ use std::os::fd::AsFd as SocketRef; use std::os::windows::io::AsSocket as SocketRef; pub fn recv_inner( - local_address: &SocketAddr, + local_address: SocketAddr, state: &UdpSocketState, socket: impl SocketRef, ) -> Result, io::Error> { @@ -99,7 +99,7 @@ pub fn recv_inner( ); Datagram::new( meta.addr, - *local_address, + local_address, meta.ecn.map(|n| IpTos::from(n as u8)).unwrap_or_default(), d, ) @@ -138,7 +138,7 @@ impl Socket { /// Receive a batch of [`Datagram`]s on the given [`Socket`], each /// set with the provided local address. - pub fn recv(&self, local_address: &SocketAddr) -> Result, io::Error> { + pub fn recv(&self, local_address: SocketAddr) -> Result, io::Error> { recv_inner(local_address, &self.state, &self.inner) } } @@ -170,7 +170,7 @@ mod tests { ); sender.send(&datagram)?; - let res = receiver.recv(&receiver_addr); + let res = receiver.recv(receiver_addr); assert_eq!(res.unwrap_err().kind(), std::io::ErrorKind::WouldBlock); Ok(()) @@ -192,7 +192,7 @@ mod tests { sender.send(&datagram)?; let received_datagram = receiver - .recv(&receiver_addr) + .recv(receiver_addr) .expect("receive to succeed") .into_iter() .next() @@ -238,7 +238,7 @@ mod tests { let mut num_received = 0; while num_received < max_gso_segments { receiver - .recv(&receiver_addr) + .recv(receiver_addr) .expect("receive to succeed") .into_iter() .for_each(|d| {