From 68e19fc0ae9c906cf98c2a501b95f38e7fc4a7b9 Mon Sep 17 00:00:00 2001 From: Lars Eggert Date: Thu, 14 Nov 2024 16:29:05 +0200 Subject: [PATCH] fix: More migration tests and related fixes (#2170) * test: More migration testing This expands the migration test quite a bit. It now tests alll the way until retirement of the original path, for both port-only (`rebinding_port`) and address-and-port (`rebinding_address_and_port`) changes. `rebinding_address_and_port` is succeeding, but `rebinding_port` is currently failing. That's because we treat it differently for some reason. If we replace `Paths::find_path_with_rebinding` with `Paths::find_path`, i.e., do proper path validation when only the port changes, the test succeeds. Leaving this out in case I'm missing something about the intent of the difference. * fix: Replace `find_path_with_rebinding` with `find_path` We need to do a path challenge even if only the remote port changes. * Remove unused arg from `received_on` * Fixes * identity * More review comments * Simplify * Fix merge * Address review comments * Migrate to preferred address * Fix URL splitting * Restore `find_path_with_rebinding` * Fix test for not doine path challenge if only port rebinds * Revert "Fix test for not doine path challenge if only port rebinds" This reverts commit 97fafa6d2e7f04e1011af0eb308f55f4dbf97a4c. * Revert "Restore `find_path_with_rebinding`" This reverts commit 302961f3615a991953e9cbeda6f04a266ae8c910. * Remove now-unused test names * fmt --------- Signed-off-by: Lars Eggert --- neqo-bin/src/client/http09.rs | 30 +- neqo-bin/src/client/mod.rs | 17 +- neqo-bin/src/lib.rs | 8 +- neqo-bin/src/server/mod.rs | 33 +- neqo-transport/src/cid.rs | 22 +- neqo-transport/src/connection/mod.rs | 7 +- .../src/connection/tests/handshake.rs | 16 +- .../src/connection/tests/migration.rs | 314 +++++++++++++++--- neqo-transport/src/connection/tests/mod.rs | 18 +- neqo-transport/src/path.rs | 80 +---- qns/interop.sh | 15 +- 11 files changed, 365 insertions(+), 195 deletions(-) diff --git a/neqo-bin/src/client/http09.rs b/neqo-bin/src/client/http09.rs index e7135fc0e7..f7f62bf57a 100644 --- a/neqo-bin/src/client/http09.rs +++ b/neqo-bin/src/client/http09.rs @@ -26,7 +26,7 @@ use neqo_transport::{ use url::Url; use super::{get_output_file, qlog_new, Args, CloseState, Res}; -use crate::{client::local_addr_for, STREAM_IO_BUFFER_SIZE}; +use crate::STREAM_IO_BUFFER_SIZE; pub struct Handler<'a> { streams: HashMap>>, @@ -37,7 +37,6 @@ pub struct Handler<'a> { token: Option, needs_key_update: bool, read_buffer: Vec, - migration: Option<&'a (u16, SocketAddr)>, } impl Handler<'_> { @@ -86,26 +85,6 @@ impl super::Handler for Handler<'_> { self.download_urls(client); } } - ConnectionEvent::StateChange(State::Confirmed) => { - if let Some((local_port, migration_addr)) = self.migration.take() { - let local_addr = local_addr_for(migration_addr, *local_port); - qdebug!("Migrating path to {:?} -> {:?}", local_addr, migration_addr); - client - .migrate( - Some(local_addr), - Some(*migration_addr), - false, - Instant::now(), - ) - .map(|()| { - qinfo!( - "Connection migrated to {:?} -> {:?}", - local_addr, - migration_addr - ); - })?; - } - } ConnectionEvent::StateChange( State::WaitInitial | State::Handshaking | State::Connected, ) => { @@ -239,11 +218,7 @@ impl super::Client for Connection { } impl<'b> Handler<'b> { - pub fn new( - url_queue: VecDeque, - args: &'b Args, - migration: Option<&'b (u16, SocketAddr)>, - ) -> Self { + pub fn new(url_queue: VecDeque, args: &'b Args) -> Self { Self { streams: HashMap::new(), url_queue, @@ -253,7 +228,6 @@ impl<'b> Handler<'b> { token: None, needs_key_update: args.key_update, read_buffer: vec![0; STREAM_IO_BUFFER_SIZE], - migration, } } diff --git a/neqo-bin/src/client/mod.rs b/neqo-bin/src/client/mod.rs index 78cd2c649f..35463cad31 100644 --- a/neqo-bin/src/client/mod.rs +++ b/neqo-bin/src/client/mod.rs @@ -557,13 +557,12 @@ pub async fn client(mut args: Args) -> Res<()> { exit(127); } - let mut remote_addrs = format!("{host}:{port}").to_socket_addrs()?.filter(|addr| { + let remote_addr = format!("{host}:{port}").to_socket_addrs()?.find(|addr| { !matches!( (addr, args.ipv4_only, args.ipv6_only), (SocketAddr::V4(..), false, true) | (SocketAddr::V6(..), true, false) ) }); - let remote_addr = remote_addrs.next(); let Some(remote_addr) = remote_addr else { qerror!("No compatible address found for: {host}"); exit(1); @@ -577,18 +576,6 @@ pub async fn client(mut args: Args) -> Res<()> { remote_addr, ); - let migration = if args.shared.qns_test.as_deref() == Some("connectionmigration") { - #[allow(clippy::option_if_let_else)] - if let Some(addr) = remote_addrs.next() { - Some((real_local.port(), addr)) - } else { - qerror!("Cannot migrate from {host} when there is no address that follows"); - exit(127); - } - } else { - None - }; - let hostname = format!("{host}"); let mut token: Option = None; let mut first = true; @@ -606,7 +593,7 @@ pub async fn client(mut args: Args) -> Res<()> { http09::create_client(&args, real_local, remote_addr, &hostname, token) .expect("failed to create client"); - let handler = http09::Handler::new(to_request, &args, migration.as_ref()); + let handler = http09::Handler::new(to_request, &args); Runner::new(real_local, &mut socket, client, handler, &args) .run() diff --git a/neqo-bin/src/lib.rs b/neqo-bin/src/lib.rs index f151f2642e..8ab352287a 100644 --- a/neqo-bin/src/lib.rs +++ b/neqo-bin/src/lib.rs @@ -218,14 +218,18 @@ impl QuicParameters { #[must_use] pub fn get(&self, alpn: &str) -> ConnectionParameters { - let params = ConnectionParameters::default() + let mut params = ConnectionParameters::default() .max_streams(StreamType::BiDi, self.max_streams_bidi) .max_streams(StreamType::UniDi, self.max_streams_uni) .idle_timeout(Duration::from_secs(self.idle_timeout)) .cc_algorithm(self.congestion_control) .pacing(!self.no_pacing) .pmtud(!self.no_pmtud); - + params = if let Some(pa) = self.preferred_address() { + params.preferred_address(pa) + } else { + params + }; if let Some(&first) = self.quic_version.first() { let all = if self.quic_version[1..].contains(&first) { &self.quic_version[1..] diff --git a/neqo-bin/src/server/mod.rs b/neqo-bin/src/server/mod.rs index fc04fa5ad4..a7bc1dfcfe 100644 --- a/neqo-bin/src/server/mod.rs +++ b/neqo-bin/src/server/mod.rs @@ -370,37 +370,30 @@ pub async fn server(mut args: Args) -> Res<()> { qwarn!("Both -V and --qns-test were set. Ignoring testcase specific versions."); } - // This is the default for all tests except http3. + // These are the default for all tests except http3. args.shared.use_old_http = true; + args.shared.alpn = String::from(HQ_INTEROP); // TODO: More options to deduplicate with client? match testcase.as_str() { - "http3" => args.shared.use_old_http = false, - "zerortt" => { - args.shared.alpn = String::from(HQ_INTEROP); - args.shared.quic_parameters.max_streams_bidi = 100; + "http3" => { + args.shared.use_old_http = false; + args.shared.alpn = "h3".into(); } - "handshake" - | "transfer" - | "resumption" - | "multiconnect" - | "v2" - | "ecn" - | "rebind-port" - | "rebind-addr" - | "connectionmigration" => { - args.shared.alpn = String::from(HQ_INTEROP); + "zerortt" => args.shared.quic_parameters.max_streams_bidi = 100, + "handshake" | "transfer" | "resumption" | "multiconnect" | "v2" | "ecn" => {} + "connectionmigration" => { + if args.shared.quic_parameters.preferred_address().is_none() { + qerror!("No preferred addresses set for connectionmigration test"); + exit(127); + } } "chacha20" => { - args.shared.alpn = String::from(HQ_INTEROP); args.shared.ciphers.clear(); args.shared .ciphers .extend_from_slice(&[String::from("TLS_CHACHA20_POLY1305_SHA256")]); } - "retry" => { - args.shared.alpn = String::from(HQ_INTEROP); - args.retry = true; - } + "retry" => args.retry = true, _ => exit(127), } } diff --git a/neqo-transport/src/cid.rs b/neqo-transport/src/cid.rs index ce6a060063..0e3144a707 100644 --- a/neqo-transport/src/cid.rs +++ b/neqo-transport/src/cid.rs @@ -14,7 +14,7 @@ use std::{ 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}; @@ -314,6 +314,10 @@ impl ConnectionIdEntry<[u8; 16]> { stats.new_connection_id += 1; true } + + pub fn is_empty(&self) -> bool { + self.seqno == CONNECTION_ID_SEQNO_EMPTY || self.cid.is_empty() + } } impl ConnectionIdEntry<()> { @@ -405,7 +409,7 @@ impl ConnectionIdStore<[u8; 16]> { pub fn retire_prior_to(&mut self, retire_prior: u64) -> Vec { let mut retired = Vec::new(); self.cids.retain(|e| { - if e.seqno < retire_prior { + if !e.is_empty() && e.seqno < retire_prior { retired.push(e.seqno); false } else { @@ -510,8 +514,18 @@ impl ConnectionIdManager { 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 empty_cid = seqno == CONNECTION_ID_SEQNO_EMPTY + || self + .connection_ids + .cids + .iter() + .any(|c| c.seqno == seqno && c.cid.is_empty()); + if empty_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 diff --git a/neqo-transport/src/connection/mod.rs b/neqo-transport/src/connection/mod.rs index 94986b9b68..25c650c75a 100644 --- a/neqo-transport/src/connection/mod.rs +++ b/neqo-transport/src/connection/mod.rs @@ -1547,7 +1547,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(), @@ -1934,10 +1934,11 @@ impl Connection { } fn migrate_to_preferred_address(&mut self, now: Instant) -> Res<()> { - let spa = if matches!( + let spa: Option<(tparams::PreferredAddress, ConnectionIdEntry<[u8; 16]>)> = if matches!( self.conn_params.get_preferred_address(), PreferredAddressConfig::Disabled ) { + qdebug!([self], "Preferred address is disabled"); None } else { self.tps.borrow_mut().remote().get_preferred_address() @@ -1975,6 +1976,8 @@ impl Connection { } else { qwarn!([self], "Unable to migrate to a different address family"); } + } else { + qdebug!([self], "No preferred address to migrate to"); } Ok(()) } diff --git a/neqo-transport/src/connection/tests/handshake.rs b/neqo-transport/src/connection/tests/handshake.rs index cac2b205a4..79a19b445e 100644 --- a/neqo-transport/src/connection/tests/handshake.rs +++ b/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); } diff --git a/neqo-transport/src/connection/tests/migration.rs b/neqo-transport/src/connection/tests/migration.rs index 6531a9bea4..06ef5b131b 100644 --- a/neqo-transport/src/connection/tests/migration.rs +++ b/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, + time::{Duration, Instant}, }; -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. @@ -49,8 +51,8 @@ const SAMPLE_PREFERRED_ADDRESS: &[u8] = &[ // This simplifies validation as the same assertions can be used for client and server. // The risk is that there is a place where source/destination local/remote is inverted. -fn loopback() -> SocketAddr { - SocketAddr::new(IpAddr::V6(Ipv6Addr::from(1)), 443) +const fn loopback() -> SocketAddr { + SocketAddr::new(IpAddr::V6(Ipv6Addr::LOCALHOST), 443) } fn change_path(d: &Datagram, a: SocketAddr) -> Datagram { @@ -62,27 +64,273 @@ const fn new_port(a: SocketAddr) -> SocketAddr { SocketAddr::new(a.ip(), port) } -fn change_source_port(d: &Datagram) -> Datagram { - Datagram::new(new_port(d.source()), d.destination(), d.tos(), &d[..]) +fn assert_path_challenge( + c: &Connection, + d: &Datagram, + before: &FrameStats, + dst: 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(), dst); + if padded { + assert!(d.len() >= MIN_INITIAL_PACKET_SIZE); + } else { + assert!(d.len() < MIN_INITIAL_PACKET_SIZE); + } +} + +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 local_address(c: &Connection) -> SocketAddr { + c.paths.primary().unwrap().borrow().local_address() +} + +fn rebind( + client: &mut Connection, + server: &mut Connection, + cur_path: fn(&Datagram) -> Datagram, + new_path: fn(&Datagram) -> Datagram, + mut now: Instant, +) -> Instant { + qdebug!("Rebinding"); + let c1 = send_something(client, now); + let c1_new = new_path(&c1); + qdebug!("Rebinding to {}", c1_new.source()); + + // 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 s1 = server.process(Some(c1_new.clone()), now).dgram().unwrap(); + assert_path_challenge(server, &s1, &before, c1_new.source(), false); + + // Restore the original source address, so to the client it looks like the path has not changed. + let s1_reb = Datagram::new(s1.source(), local_address(client), s1.tos(), &s1[..]); + + // The client should respond to the PATH_CHALLENGE, without changing paths. + let before = client.stats().frame_tx; + let c2 = client.process(Some(s1_reb), now).dgram().unwrap(); + assert_path_response(client, &c2, &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 c2_new = new_path(&c2); + let before = server.stats().frame_tx; + let s2 = server.process(Some(c2_new.clone()), now).dgram().unwrap(); + assert_path_challenge(server, &s2, &before, c2_new.source(), true); + + // Restore the original source address, so to the client it looks like the path has not changed. + let s2_reb = Datagram::new(s2.source(), local_address(client), s2.tos(), &s2[..]); + + // The client should respond to the PATH_CHALLENGE, without changing paths. + let before = client.stats().frame_tx; + let c3 = client.process(Some(s2_reb.clone()), now).dgram().unwrap(); + assert_path_response(client, &s2_reb, &before); + + // The server should now see the second response on the new path. + // It will then try to probe the old path. + let c3_new = new_path(&c3); + let c3_cur = cur_path(&c3); + let before = server.stats().frame_tx; + let s3 = server.process(Some(c3_new.clone()), now).dgram().unwrap(); + assert_path_challenge(server, &s3, &before, c3_cur.source(), true); + + // Do not deliver this probe to the client. + + // Server will now ACK on the new path. + let before = server.stats().frame_tx; + let s4 = server.process_output(now).dgram().unwrap(); + let after = server.stats().frame_tx; + assert_eq!(after.ack, before.ack + 1); + assert_eq!(s4.source(), c3_new.destination()); + assert_eq!(s4.destination(), c3_new.source()); + + // Restore the original source address, so to the client it looks like the path has not changed. + let s4_reb = Datagram::new(s4.source(), local_address(client), s4.tos(), &s4[..]); + + // The client should process the ACK and go idle. + let delay = client.process(Some(s4_reb), 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 total_delay = Duration::new(0, 0); + loop { + let before = server.stats().frame_tx; + match server.process_output(now) { + Output::Callback(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; + } + now += t; + } + Output::Datagram(sx) => { + total_delay = Duration::new(0, 0); + if sx.destination() == c3_cur.source() { + // Old path gets path challenges. + assert_path_challenge(server, &sx, &before, c3_cur.source(), true); + // Don't deliver them. + } else { + 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 + ); + } + // Restore the original source address, so to the client it looks like + // the path has not changed. + let sx_r = Datagram::new(sx.source(), local_address(client), sx.tos(), &sx[..]); + let before = client.stats().frame_tx; + let cx = client.process(Some(sx_r), now).dgram().unwrap(); + let after = client.stats().frame_tx; + assert_eq!(after.ack, before.ack + 1); + // Also deliver the ACK. + let cx_n = new_path(&cx); + server.process_input(cx_n, 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; + } + } + } + Output::None => panic!(), + } + } + + if !client_uses_zero_len_cid { + // Eat up any delays before returning. + now += client.process_output(now).callback(); + now += server.process_output(now).callback(); + } + + qdebug!("Rebinding done"); + now +} + +fn inc_port(port: u16, i: usize) -> u16 { + port.overflowing_add(i.overflowing_mul(11).0.try_into().unwrap()) + .0 +} + +fn inc_addr(ip: IpAddr, i: usize) -> IpAddr { + let inc: u8 = i.overflowing_mul(11).0.try_into().unwrap(); + match ip { + IpAddr::V4(ip) => IpAddr::V4(Ipv4Addr::from( + ip.octets().map(|b| b.overflowing_add(inc).0), + )), + IpAddr::V6(ip) => IpAddr::V6(Ipv6Addr::from( + ip.octets().map(|b| b.overflowing_add(inc).0), + )), + } +} + +fn change_source_port(d: &Datagram, i: usize) -> Datagram { + Datagram::new( + SocketAddr::new(d.source().ip(), inc_port(d.source().port(), i)), + d.destination(), + d.tos(), + &d[..], + ) +} + +fn change_source_address_and_port(d: &Datagram, i: usize) -> Datagram { + Datagram::new( + SocketAddr::new(inc_addr(d.source().ip(), i), inc_port(d.source().port(), i)), + d.destination(), + d.tos(), + &d[..], + ) +} + +fn rebind_port_with_client(client: &mut Connection) { + let mut server = default_server(); + connect_force_idle(client, &mut server); + let mut now = now(); + + now = rebind( + client, + &mut server, + |d| change_source_port(d, 0), + |d| change_source_port(d, 1), + now, + ); + _ = rebind( + client, + &mut server, + |d| change_source_port(d, 1), + |d| change_source_port(d, 2), + now, + ); +} + +fn rebind_address_and_port_with_client(client: &mut Connection) { + let mut server = default_server(); + connect_force_idle(client, &mut server); + let mut now = now(); + + now = rebind( + client, + &mut server, + |d| change_source_address_and_port(d, 0), + |d| change_source_address_and_port(d, 1), + now, + ); + _ = rebind( + client, + &mut server, + |d| change_source_address_and_port(d, 1), + |d| change_source_address_and_port(d, 2), + now, + ); } #[test] -fn rebinding_port() { +fn rebind_port() { let mut client = default_client(); - let mut server = default_server(); - connect_force_idle(&mut client, &mut server); + rebind_port_with_client(&mut client); +} - let dgram = send_something(&mut client, now()); - let dgram = change_source_port(&dgram); +#[test] +fn rebind_port_zero_len_cid() { + let mut client = zero_len_cid_client(DEFAULT_ADDR, DEFAULT_ADDR); + rebind_port_with_client(&mut client); +} - 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(); - let dgram = server.process_output(now()).dgram(); - let dgram = dgram.unwrap(); - assert_eq!(dgram.source(), DEFAULT_ADDR); - assert_eq!(dgram.destination(), new_port(DEFAULT_ADDR)); +#[test] +fn rebind_address_and_port() { + let mut client = default_client(); + rebind_address_and_port_with_client(&mut client); +} + +#[test] +fn rebind_address_and_port_zero_len_cid() { + let mut client = zero_len_cid_client(DEFAULT_ADDR, DEFAULT_ADDR); + rebind_address_and_port_with_client(&mut client); } /// This simulates an attack where a valid packet is forwarded on @@ -448,16 +696,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 +745,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)), diff --git a/neqo-transport/src/connection/tests/mod.rs b/neqo-transport/src/connection/tests/mod.rs index 3528f7d488..e897b7082b 100644 --- a/neqo-transport/src/connection/tests/mod.rs +++ b/neqo-transport/src/connection/tests/mod.rs @@ -8,6 +8,7 @@ use std::{ cell::RefCell, cmp::min, mem, + net::SocketAddr, rc::Rc, time::{Duration, Instant}, }; @@ -29,8 +30,8 @@ use crate::{ recovery::ACK_ONLY_SIZE_LIMIT, stats::{FrameStats, Stats, MAX_PTO_COUNTS}, tparams::{DISABLE_MIGRATION, GREASE_QUIC_BIT}, - ConnectionIdDecoder, ConnectionIdGenerator, ConnectionParameters, Error, StreamId, StreamType, - Version, MIN_INITIAL_PACKET_SIZE, + ConnectionIdDecoder, ConnectionIdGenerator, ConnectionParameters, EmptyConnectionIdGenerator, + Error, StreamId, StreamType, Version, MIN_INITIAL_PACKET_SIZE, }; // All the tests. @@ -120,6 +121,19 @@ pub fn default_client() -> Connection { new_client(ConnectionParameters::default()) } +fn zero_len_cid_client(local_addr: SocketAddr, remote_addr: SocketAddr) -> Connection { + Connection::new_client( + test_fixture::DEFAULT_SERVER_NAME, + test_fixture::DEFAULT_ALPN, + Rc::new(RefCell::new(EmptyConnectionIdGenerator::default())), + local_addr, + remote_addr, + ConnectionParameters::default(), + now(), + ) + .unwrap() +} + pub fn new_server(params: ConnectionParameters) -> Connection { fixture_init(); let (log, _contents) = new_neqo_qlog(); diff --git a/neqo-transport/src/path.rs b/neqo-transport/src/path.rs index 50740444d9..69f75dd8c5 100644 --- a/neqo-transport/src/path.rs +++ b/neqo-transport/src/path.rs @@ -83,7 +83,7 @@ impl Paths { self.paths .iter() .find_map(|p| { - if p.borrow().received_on(local, remote, false) { + if p.borrow().received_on(local, remote) { Some(Rc::clone(p)) } else { None @@ -98,48 +98,6 @@ impl Paths { }) } - /// Find the path, but allow for rebinding. That matches the pair of addresses - /// to paths that match the remote address only based on IP addres, not port. - /// We use this when the other side migrates to skip address validation and - /// creating a new path. - pub fn find_path_with_rebinding( - &self, - local: SocketAddr, - remote: SocketAddr, - cc: CongestionControlAlgorithm, - pacing: bool, - now: Instant, - ) -> PathRef { - self.paths - .iter() - .find_map(|p| { - if p.borrow().received_on(local, remote, false) { - Some(Rc::clone(p)) - } else { - None - } - }) - .or_else(|| { - self.paths.iter().find_map(|p| { - if p.borrow().received_on(local, remote, true) { - Some(Rc::clone(p)) - } else { - None - } - }) - }) - .unwrap_or_else(|| { - Rc::new(RefCell::new(Path::temporary( - local, - remote, - cc, - pacing, - self.qlog.clone(), - now, - ))) - }) - } - /// Get a reference to the primary path, if one exists. pub fn primary(&self) -> Option { self.primary.clone() @@ -152,13 +110,14 @@ impl Paths { } fn retire(to_retire: &mut Vec, retired: &PathRef) { - let seqno = retired - .borrow() - .remote_cid - .as_ref() - .unwrap() - .sequence_number(); - to_retire.push(seqno); + if let Some(cid) = &retired.borrow().remote_cid { + let seqno = cid.sequence_number(); + if cid.connection_id().is_empty() { + qdebug!("Connection ID {seqno} is zero-length, not retiring"); + } else { + to_retire.push(seqno); + } + } } /// Adopt a temporary path as permanent. @@ -389,22 +348,23 @@ impl Paths { to_retire.append(&mut retired); self.paths.retain(|p| { - let current = p.borrow().remote_cid.as_ref().unwrap().sequence_number(); - if current < retire_prior { - to_retire.push(current); + let mut path = p.borrow_mut(); + let current = path.remote_cid.as_ref().unwrap(); + if current.sequence_number() < retire_prior && !current.connection_id().is_empty() { + to_retire.push(current.sequence_number()); let new_cid = store.next(); let has_replacement = new_cid.is_some(); // There must be a connection ID available for the primary path as we // keep that path at the first index. - debug_assert!(!p.borrow().is_primary() || has_replacement); - p.borrow_mut().remote_cid = new_cid; + debug_assert!(!path.is_primary() || has_replacement); + path.remote_cid = new_cid; if !has_replacement && migration_target .as_ref() .map_or(false, |target| Rc::ptr_eq(target, p)) { qinfo!( - [p.borrow()], + [path], "NEW_CONNECTION_ID with Retire Prior To forced migration to fail" ); *migration_target = None; @@ -623,12 +583,8 @@ impl Path { } /// Determine if this path was the one that the provided datagram was received on. - /// This uses the full local socket address, but ignores the port number on the peer - /// if `flexible` is true, allowing for NAT rebinding that retains the same IP. - fn received_on(&self, local: SocketAddr, remote: SocketAddr, flexible: bool) -> bool { - self.local == local - && self.remote.ip() == remote.ip() - && (flexible || self.remote.port() == remote.port()) + fn received_on(&self, local: SocketAddr, remote: SocketAddr) -> bool { + self.local == local && self.remote == remote } /// Update the remote port number. Any flexibility we allow in `received_on` diff --git a/qns/interop.sh b/qns/interop.sh index c83dd6552d..04ee0c7ffb 100755 --- a/qns/interop.sh +++ b/qns/interop.sh @@ -12,9 +12,12 @@ export PATH="${PATH}:/neqo/bin" case "$ROLE" in client) /wait-for-it.sh sim:57832 -s -t 30 - # shellcheck disable=SC2086 - RUST_LOG=debug RUST_BACKTRACE=1 neqo-client --cc cubic --qns-test "$TESTCASE" \ - --qlog-dir "$QLOGDIR" --output-dir /downloads $REQUESTS 2> >(tee -i -a "/logs/$ROLE.log" >&2) + OPTIONS=(--cc cubic --qns-test "$TESTCASE" --qlog-dir "$QLOGDIR" --output-dir /downloads) + if [ "$REQUESTS" ]; then + mapfile -d ' ' -t URLS <<<"$REQUESTS" + OPTIONS+=("${URLS[@]}") + fi + RUST_LOG=debug RUST_BACKTRACE=1 neqo-client "${OPTIONS[@]}" 2> >(tee -i -a "/logs/$ROLE.log" >&2) ;; server) @@ -27,8 +30,10 @@ server) -name "$CERT" -passout pass: -out "$P12CERT" pk12util -d "sql:$DB" -i "$P12CERT" -W '' certutil -L -d "sql:$DB" -n "$CERT" - RUST_LOG=debug RUST_BACKTRACE=1 neqo-server --cc cubic --qns-test "$TESTCASE" \ - --qlog-dir "$QLOGDIR" -d "$DB" -k "$CERT" '[::]:443' 2> >(tee -i -a "/logs/$ROLE.log" >&2) + OPTIONS=(--cc cubic --qns-test "$TESTCASE" --qlog-dir "$QLOGDIR" -d "$DB" -k "$CERT") + [ "$TESTCASE" = "connectionmigration" ] && + OPTIONS+=(--preferred-address-v4 server4:4443 --preferred-address-v6 server6:4443) + RUST_LOG=debug RUST_BACKTRACE=1 neqo-server "${OPTIONS[@]}" '[::]:443' 2> >(tee -i -a "/logs/$ROLE.log" >&2) ;; *)