Skip to content

Commit

Permalink
fix: More migration tests and related fixes (mozilla#2170)
Browse files Browse the repository at this point in the history 
* 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 97fafa6.

* Revert "Restore `find_path_with_rebinding`"

This reverts commit 302961f.

* Remove now-unused test names

* fmt

---------

Signed-off-by: Lars Eggert <[email protected]>
  • Loading branch information
@larseggert
larseggert authored Nov 14, 2024
1 parent bcc16fc commit 68e19fc
Show file tree
Hide file tree
Showing 11 changed files with 365 additions and 195 deletions.
30 changes: 2 additions & 28 deletions neqo-bin/src/client/http09.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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<StreamId, Option<BufWriter<File>>>,
Expand All @@ -37,7 +37,6 @@ pub struct Handler<'a> {
token: Option<ResumptionToken>,
needs_key_update: bool,
read_buffer: Vec<u8>,
migration: Option<&'a (u16, SocketAddr)>,
}

impl Handler<'_> {
Expand Down Expand Up @@ -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,
) => {
Expand Down Expand Up @@ -239,11 +218,7 @@ impl super::Client for Connection {
}

impl<'b> Handler<'b> {
pub fn new(
url_queue: VecDeque<Url>,
args: &'b Args,
migration: Option<&'b (u16, SocketAddr)>,
) -> Self {
pub fn new(url_queue: VecDeque<Url>, args: &'b Args) -> Self {
Self {
streams: HashMap::new(),
url_queue,
Expand All @@ -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,
}
}

Expand Down
17 changes: 2 additions & 15 deletions neqo-bin/src/client/mod.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand All @@ -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<ResumptionToken> = None;
let mut first = true;
Expand All @@ -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()
Expand Down
8 changes: 6 additions & 2 deletions neqo-bin/src/lib.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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..]
Expand Down
33 changes: 13 additions & 20 deletions neqo-bin/src/server/mod.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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),
}
}
Expand Down
22 changes: 18 additions & 4 deletions neqo-transport/src/cid.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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};

Expand Down Expand Up @@ -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<()> {
Expand Down Expand Up @@ -405,7 +409,7 @@ impl ConnectionIdStore<[u8; 16]> {
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.is_empty() && e.seqno < retire_prior {
retired.push(e.seqno);
false
} else {
Expand Down Expand Up @@ -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
Expand Down
7 changes: 5 additions & 2 deletions neqo-transport/src/connection/mod.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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<impl AsRef<[u8]>>, 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(),
Expand Down Expand Up @@ -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()
Expand Down Expand Up @@ -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(())
}
Expand Down
16 changes: 3 additions & 13 deletions neqo-transport/src/connection/tests/handshake.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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::{
Expand All @@ -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;
Expand Down Expand Up @@ -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);
}
Expand Down
Loading

0 comments on commit 68e19fc

Please sign in to comment.