From c4e8f0694795afd6e991878a4a55a871575264db Mon Sep 17 00:00:00 2001 From: Lars Eggert Date: Fri, 18 Oct 2024 11:09:45 +0300 Subject: [PATCH] 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();