fix: More migration tests and related fixes

neqo-transport/src/cid.rs

@@ -14,7 +14,7 @@
     rc::Rc,
 };
 
-use neqo_common::{hex, hex_with_len, qinfo, Decoder, Encoder};
+use neqo_common::{hex, hex_with_len, qdebug, qinfo, Decoder, Encoder};
 use neqo_crypto::{random, randomize};
 use smallvec::{smallvec, SmallVec};
 
@@ -405,7 +405,7 @@
     pub fn retire_prior_to(&mut self, retire_prior: u64) -> Vec<u64> {
         let mut retired = Vec::new();
         self.cids.retain(|e| {
-            if e.seqno < retire_prior {
+            if e.seqno != CONNECTION_ID_SEQNO_EMPTY && e.seqno < retire_prior {
                 retired.push(e.seqno);
                 false
             } else {
@@ -510,8 +510,18 @@
     pub fn retire(&mut self, seqno: u64) {
         // TODO(mt) - consider keeping connection IDs around for a short while.
 
-        self.connection_ids.retire(seqno);
-        self.lost_new_connection_id.retain(|cid| cid.seqno != seqno);
+        let zero_len_cid = seqno == CONNECTION_ID_SEQNO_EMPTY
+            || self
+                .connection_ids
+                .cids
+                .iter()
+                .any(|c| c.seqno == seqno && c.cid.len() == 0);
+        if zero_len_cid {
+            qdebug!("Connection ID {seqno} is zero-length, not retiring");
+        } else {
+            self.connection_ids.retire(seqno);
+            self.lost_new_connection_id.retain(|cid| cid.seqno != seqno);
+        }
     }
 
     /// During the handshake, a server needs to regard the client's choice of destination

neqo-transport/src/connection/mod.rs

@@ -1532,7 +1532,7 @@ impl Connection {
     /// This takes two times: when the datagram was received, and the current time.
     fn input(&mut self, d: &Datagram, received: Instant, now: Instant) {
         // First determine the path.
-        let path = self.paths.find_path_with_rebinding(
+        let path = self.paths.find_path(
             d.destination(),
             d.source(),
             self.conn_params.get_cc_algorithm(),

neqo-transport/src/connection/tests/handshake.rs

@@ -25,7 +25,7 @@ use test_fixture::{
 use super::{
     super::{Connection, Output, State},
     assert_error, connect, connect_force_idle, connect_with_rtt, default_client, default_server,
-    get_tokens, handshake, maybe_authenticate, resumed_server, send_something,
+    get_tokens, handshake, maybe_authenticate, resumed_server, send_something, zero_len_cid_client,
     CountingConnectionIdGenerator, AT_LEAST_PTO, DEFAULT_RTT, DEFAULT_STREAM_DATA,
 };
 use crate::{
@@ -38,8 +38,7 @@ use crate::{
     stats::FrameStats,
     tparams::{self, TransportParameter, MIN_ACK_DELAY},
     tracking::DEFAULT_ACK_DELAY,
-    CloseReason, ConnectionParameters, EmptyConnectionIdGenerator, Error, Pmtud, StreamType,
-    Version,
+    CloseReason, ConnectionParameters, Error, Pmtud, StreamType, Version,
 };
 
 const ECH_CONFIG_ID: u8 = 7;
@@ -263,16 +262,7 @@ fn crypto_frame_split() {
 #[test]
 fn chacha20poly1305() {
     let mut server = default_server();
-    let mut client = Connection::new_client(
-        test_fixture::DEFAULT_SERVER_NAME,
-        test_fixture::DEFAULT_ALPN,
-        Rc::new(RefCell::new(EmptyConnectionIdGenerator::default())),
-        DEFAULT_ADDR,
-        DEFAULT_ADDR,
-        ConnectionParameters::default(),
-        now(),
-    )
-    .expect("create a default client");
+    let mut client = zero_len_cid_client(DEFAULT_ADDR, DEFAULT_ADDR);
     client.set_ciphers(&[TLS_CHACHA20_POLY1305_SHA256]).unwrap();
     connect_force_idle(&mut client, &mut server);
 }

neqo-transport/src/connection/tests/migration.rs

@@ -7,12 +7,12 @@
 use std::{
     cell::RefCell,
     mem,
-    net::{IpAddr, Ipv6Addr, SocketAddr},
+    net::{IpAddr, Ipv4Addr, Ipv6Addr, SocketAddr},
     rc::Rc,
     time::Duration,
 };
 
-use neqo_common::{Datagram, Decoder};
+use neqo_common::{qdebug, Datagram, Decoder};
 use test_fixture::{
     assertions::{assert_v4_path, assert_v6_path},
     fixture_init, new_neqo_qlog, now, DEFAULT_ADDR, DEFAULT_ADDR_V4,
@@ -21,7 +21,8 @@ use test_fixture::{
 use super::{
     super::{Connection, Output, State, StreamType},
     connect_fail, connect_force_idle, connect_rtt_idle, default_client, default_server,
-    maybe_authenticate, new_client, new_server, send_something, CountingConnectionIdGenerator,
+    maybe_authenticate, new_client, new_server, send_something, zero_len_cid_client,
+    CountingConnectionIdGenerator,
 };
 use crate::{
     cid::LOCAL_ACTIVE_CID_LIMIT,
@@ -30,9 +31,10 @@ use crate::{
     packet::PacketBuilder,
     path::MAX_PATH_PROBES,
     pmtud::Pmtud,
+    stats::FrameStats,
     tparams::{self, PreferredAddress, TransportParameter},
     CloseReason, ConnectionId, ConnectionIdDecoder, ConnectionIdGenerator, ConnectionIdRef,
-    ConnectionParameters, EmptyConnectionIdGenerator, Error,
+    ConnectionParameters, EmptyConnectionIdGenerator, Error, MIN_INITIAL_PACKET_SIZE,
 };
 
 /// This should be a valid-seeming transport parameter.
@@ -62,27 +64,223 @@ const fn new_port(a: SocketAddr) -> SocketAddr {
     SocketAddr::new(a.ip(), port)
 }
 
+fn new_address_and_port(a: SocketAddr) -> SocketAddr {
+    let ip = match a.ip() {
+        IpAddr::V4(ip) => IpAddr::V4(Ipv4Addr::from(ip.octets().map(|b| b.overflowing_add(11).0))),
+        IpAddr::V6(ip) => IpAddr::V6(Ipv6Addr::from(ip.octets().map(|b| b.overflowing_add(11).0))),
+    };
+    SocketAddr::new(ip, new_port(a).port())
+}
+
 fn change_source_port(d: &Datagram) -> Datagram {
     Datagram::new(new_port(d.source()), d.destination(), d.tos(), &d[..])
 }
 
-#[test]
-fn rebinding_port() {
-    let mut client = default_client();
-    let mut server = default_server();
-    connect_force_idle(&mut client, &mut server);
+fn change_source_ip_and_port(d: &Datagram) -> Datagram {
+    Datagram::new(
+        new_address_and_port(d.source()),
+        d.destination(),
+        d.tos(),
+        &d[..],
+    )
+}
 
-    let dgram = send_something(&mut client, now());
-    let dgram = change_source_port(&dgram);
+fn assert_path_challenge(
+    c: &Connection,
+    d: &Datagram,
+    before: &FrameStats,
+    mod_saddr: fn(SocketAddr) -> SocketAddr,
+    padded: bool,
+) {
+    let after = c.stats().frame_tx;
+    assert_eq!(after.path_challenge, before.path_challenge + 1);
+    assert_eq!(d.source(), DEFAULT_ADDR);
+    assert_eq!(d.destination(), mod_saddr(DEFAULT_ADDR));
+    if padded {
+        assert!(d.len() >= MIN_INITIAL_PACKET_SIZE);
+    } else {
+        assert!(d.len() < MIN_INITIAL_PACKET_SIZE);
+    }
+}
 
-    server.process_input(&dgram, now());
-    // Have the server send something so that it generates a packet.
-    let stream_id = server.stream_create(StreamType::UniDi).unwrap();
-    server.stream_close_send(stream_id).unwrap();
+const fn id(a: SocketAddr) -> SocketAddr {
+    a
+}
+
+fn assert_path_response(c: &Connection, d: &Datagram, before: &FrameStats) {
+    let after = c.stats().frame_tx;
+    assert_eq!(after.path_response, before.path_response + 1);
+    assert_eq!(d.source(), DEFAULT_ADDR);
+    assert_eq!(d.destination(), DEFAULT_ADDR);
+}
+
+fn rebind(
+    client: &mut Connection,
+    to_new_path: fn(&Datagram) -> Datagram,
+    to_new_saddr: fn(SocketAddr) -> SocketAddr,
+) {
+    let mut server = default_server();
+    connect_force_idle(client, &mut server);
+
+    let dgram = send_something(client, now());
+    let orig_saddr = dgram.source();
+    let dgram = to_new_path(&dgram);
+
+    // Server will reply to modified datagram with a PATH_CHALLENGE.
+    // Due to the amplification limit, this will not be padded to MIN_INITIAL_PACKET_SIZE.
+    let before = server.stats().frame_tx;
+    let dgram = server.process(Some(&dgram), now()).dgram().unwrap();
+    assert_path_challenge(&server, &dgram, &before, to_new_saddr, false);
+
+    // Restore the original source address, so to the client it looks like the path has not changed.
+    let dgram = Datagram::new(dgram.source(), orig_saddr, dgram.tos(), &dgram[..]);
+
+    // The client should respond to the PATH_CHALLENGE, without changing paths.
+    let before = client.stats().frame_tx;
+    let dgram = client.process(Some(&dgram), now()).dgram().unwrap();
+    assert_path_response(client, &dgram, &before);
+
+    // The server should now see the response on the new path.
+    // It will send another PATH_CHALLENGE padded to MIN_INITIAL_PACKET_SIZE.
+    let dgram = to_new_path(&dgram);
+    let before = server.stats().frame_tx;
+    let dgram = server.process(Some(&dgram), now()).dgram().unwrap();
+    assert_path_challenge(&server, &dgram, &before, to_new_saddr, true);
+
+    // Restore the original source address, so to the client it looks like the path has not changed.
+    let dgram = Datagram::new(dgram.source(), orig_saddr, dgram.tos(), &dgram[..]);
+
+    // The client should respond to the PATH_CHALLENGE, without changing paths.
+    let before = client.stats().frame_tx;
+    let dgram = client.process(Some(&dgram), now()).dgram().unwrap();
+    assert_path_response(client, &dgram, &before);
+
+    // The server should now see the second response on the new path.
+    // It will then try to probe the old path.
+    let dgram = to_new_path(&dgram);
+    let before = server.stats().frame_tx;
+    let dgram = server.process(Some(&dgram), now()).dgram().unwrap();
+    assert_path_challenge(&server, &dgram, &before, id, true);
+
+    // Do not deliver this probe to the client.
+
+    // Server will now ACK on the new path.
+    let before = server.stats().frame_tx;
     let dgram = server.process_output(now()).dgram();
     let dgram = dgram.unwrap();
+    let after = server.stats().frame_tx;
+    assert_eq!(after.ack, before.ack + 1);
     assert_eq!(dgram.source(), DEFAULT_ADDR);
-    assert_eq!(dgram.destination(), new_port(DEFAULT_ADDR));
+    assert_eq!(dgram.destination(), to_new_saddr(DEFAULT_ADDR));
+
+    // Restore the original source address, so to the client it looks like the path has not changed.
+    let dgram = Datagram::new(dgram.source(), orig_saddr, dgram.tos(), &dgram[..]);
+
+    // The client should process the ACK and go idle.
+    let delay = client.process(Some(&dgram), now()).callback();
+    assert_eq!(delay, ConnectionParameters::default().get_idle_timeout());
+
+    let client_uses_zero_len_cid = client
+        .paths
+        .primary()
+        .unwrap()
+        .borrow()
+        .local_cid()
+        .unwrap()
+        .len()
+        == 0;
+    let mut now = now();
+    let mut total_delay = Duration::new(0, 0);
+    qdebug!("XXXXXXXXX");
+    loop {
+        let before = server.stats().frame_tx;
+        match server.process_output(now) {
+            Output::Callback(t) => {
+                now += t;
+                total_delay += t;
+                if total_delay == ConnectionParameters::default().get_idle_timeout() {
+                    // Server should only hit the idle timeout here when the client uses a zero-len
+                    // CID.
+                    assert!(client_uses_zero_len_cid);
+                    break;
+                }
+            }
+            Output::Datagram(d) => {
+                total_delay = Duration::new(0, 0);
+                match d.destination() {
+                    a if a == DEFAULT_ADDR => {
+                        // Old path gets path challenges.
+                        assert_path_challenge(&server, &d, &before, id, true);
+                        // Don't deliver them.
+                    }
+                    a if a == to_new_saddr(DEFAULT_ADDR) => {
+                        let after = server.stats().frame_tx;
+                        // If the client uses a zero-len CID, the server will only PING.
+                        // Otherwise, it will PING or send a RETIRE_CONNECTION_ID.
+                        if client_uses_zero_len_cid {
+                            assert_eq!(after.ping, before.ping + 1);
+                        } else {
+                            assert!(
+                                after.retire_connection_id == before.retire_connection_id + 1
+                                    || after.ping == before.ping + 1
+                            );
+                        }
+                        let d = Datagram::new(d.source(), orig_saddr, d.tos(), &d[..]);
+                        // Restore the original source address, so to the client it looks like the
+                        // path has not changed.
+                        let d = Datagram::new(d.source(), orig_saddr, d.tos(), &d[..]);
+                        let before = client.stats().frame_tx;
+                        let ack = client.process(Some(&d), now).dgram().unwrap();
+                        let after = client.stats().frame_tx;
+                        assert_eq!(after.ack, before.ack + 1);
+                        // Also deliver the ACK.
+                        let ack = to_new_path(&ack);
+                        server.process_input(&ack, now);
+                        if !client_uses_zero_len_cid
+                            && after.new_connection_id == before.new_connection_id + 1
+                        {
+                            // Declare victory once the client has sent a new connection ID.
+                            break;
+                        }
+                    }
+                    _ => panic!(),
+                }
+            }
+            Output::None => panic!(),
+        }
+    }
+}
+
+#[test]
+fn rebinding_port() {
+    rebind(&mut default_client(), change_source_port, new_port);
+}
+
+#[test]
+fn rebinding_port_zero_len_cid() {
+    rebind(
+        &mut zero_len_cid_client(DEFAULT_ADDR, DEFAULT_ADDR),
+        change_source_port,
+        new_port,
+    );
+}
+
+#[test]
+fn rebinding_address_and_port() {
+    rebind(
+        &mut default_client(),
+        change_source_ip_and_port,
+        new_address_and_port,
+    );
+}
+
+#[test]
+fn rebinding_address_and_port_zero_len_cid() {
+    rebind(
+        &mut zero_len_cid_client(DEFAULT_ADDR, DEFAULT_ADDR),
+        change_source_ip_and_port,
+        new_address_and_port,
+    );
 }
 
 /// This simulates an attack where a valid packet is forwarded on
@@ -448,16 +646,7 @@ fn migration_graceful() {
 #[test]
 fn migration_client_empty_cid() {
     fixture_init();
-    let client = Connection::new_client(
-        test_fixture::DEFAULT_SERVER_NAME,
-        test_fixture::DEFAULT_ALPN,
-        Rc::new(RefCell::new(EmptyConnectionIdGenerator::default())),
-        DEFAULT_ADDR,
-        DEFAULT_ADDR,
-        ConnectionParameters::default(),
-        now(),
-    )
-    .unwrap();
+    let client = zero_len_cid_client(DEFAULT_ADDR, DEFAULT_ADDR);
     migration(client);
 }
 
@@ -506,16 +695,7 @@ fn preferred_address(hs_client: SocketAddr, hs_server: SocketAddr, preferred: So
 
     fixture_init();
     let (log, _contents) = new_neqo_qlog();
-    let mut client = Connection::new_client(
-        test_fixture::DEFAULT_SERVER_NAME,
-        test_fixture::DEFAULT_ALPN,
-        Rc::new(RefCell::new(EmptyConnectionIdGenerator::default())),
-        hs_client,
-        hs_server,
-        ConnectionParameters::default(),
-        now(),
-    )
-    .unwrap();
+    let mut client = zero_len_cid_client(hs_client, hs_server);
     client.set_qlog(log);
     let spa = match preferred {
         SocketAddr::V6(v6) => PreferredAddress::new(None, Some(v6)),