test: Make sure our PATH_CHALLENGEs are padded OK (#2168)

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;

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,13 +249,15 @@ 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));
 
         let resp = new_path_modifier(server.process(Some(&probe), now).dgram().unwrap()).unwrap();
         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 }

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,15 +252,17 @@ 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);
         assert_eq!(after.all(), before.all() + 2);
 
         // 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,15 +332,17 @@ 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);
         assert_eq!(after.all(), before.all() + 2);
 
         // 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.

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();