From b72b3bae2983b19d19c7b483f6d08db45ef4a1ed Mon Sep 17 00:00:00 2001 From: Lars Eggert Date: Tue, 17 Sep 2024 06:03:18 -0700 Subject: [PATCH] fix: Don't encode large RTT guesses in tickets (#2114) * fix: Don't encode large RTT guesses in tickets Because under lossy conditions (e.g., QNS `handshakeloss` test), the guess can be multiple times the actual RTT, which when encoded in the resumption ticket will cause an extremely slow second handshake, often causing the test to time out. Broken out of #1998 Fixes #2088 * Fixes & tests * Suggestion from @mxinden * Fix --- neqo-transport/src/connection/mod.rs | 22 +++- .../src/connection/tests/resumption.rs | 124 +++++++++++++++++- neqo-transport/src/path.rs | 4 +- neqo-transport/src/recovery/mod.rs | 10 +- neqo-transport/src/rtt.rs | 25 +++- 5 files changed, 173 insertions(+), 12 deletions(-) diff --git a/neqo-transport/src/connection/mod.rs b/neqo-transport/src/connection/mod.rs index 29bc85ff73..577a93277c 100644 --- a/neqo-transport/src/connection/mod.rs +++ b/neqo-transport/src/connection/mod.rs @@ -48,7 +48,7 @@ use crate::{ quic_datagrams::{DatagramTracking, QuicDatagrams}, recovery::{LossRecovery, RecoveryToken, SendProfile, SentPacket}, recv_stream::RecvStreamStats, - rtt::{RttEstimate, GRANULARITY}, + rtt::{RttEstimate, GRANULARITY, INITIAL_RTT}, send_stream::SendStream, stats::{Stats, StatsCell}, stream_id::StreamType, @@ -594,9 +594,25 @@ impl Connection { fn make_resumption_token(&mut self) -> ResumptionToken { debug_assert_eq!(self.role, Role::Client); debug_assert!(self.crypto.has_resumption_token()); + // Values less than GRANULARITY are ignored when using the token, so use 0 where needed. let rtt = self.paths.primary().map_or_else( - || RttEstimate::default().estimate(), - |p| p.borrow().rtt().estimate(), + // If we don't have a path, we don't have an RTT. + || Duration::from_millis(0), + |p| { + let rtt = p.borrow().rtt().estimate(); + if p.borrow().rtt().is_guesstimate() { + // When we have no actual RTT sample, do not encode a guestimated RTT larger + // than the default initial RTT. (The guess can be very large under lossy + // conditions.) + if rtt < INITIAL_RTT { + rtt + } else { + Duration::from_millis(0) + } + } else { + rtt + } + }, ); self.crypto diff --git a/neqo-transport/src/connection/tests/resumption.rs b/neqo-transport/src/connection/tests/resumption.rs index 7410e76ef8..01e977e506 100644 --- a/neqo-transport/src/connection/tests/resumption.rs +++ b/neqo-transport/src/connection/tests/resumption.rs @@ -6,7 +6,16 @@ use std::{cell::RefCell, mem, rc::Rc, time::Duration}; -use test_fixture::{assertions, now}; +use neqo_common::{Datagram, Decoder, Role}; +use neqo_crypto::AuthenticationStatus; +use test_fixture::{ + assertions, + header_protection::{ + apply_header_protection, decode_initial_header, initial_aead_and_hp, + remove_header_protection, + }, + now, split_datagram, +}; use super::{ connect, connect_with_rtt, default_client, default_server, exchange_ticket, get_tokens, @@ -14,7 +23,9 @@ use super::{ }; use crate::{ addr_valid::{AddressValidation, ValidateAddress}, - ConnectionParameters, Error, Version, + frame::FRAME_TYPE_PADDING, + rtt::INITIAL_RTT, + ConnectionParameters, Error, State, Version, MIN_INITIAL_PACKET_SIZE, }; #[test] @@ -75,6 +86,115 @@ fn remember_smoothed_rtt() { ); } +fn ticket_rtt(rtt: Duration) -> Duration { + // A simple ACK_ECN frame for a single packet with packet number 0 with a single ECT(0) mark. + const ACK_FRAME_1: &[u8] = &[0x03, 0x00, 0x00, 0x00, 0x00, 0x01, 0x00, 0x00]; + + let mut client = new_client( + ConnectionParameters::default().versions(Version::Version1, vec![Version::Version1]), + ); + let mut server = default_server(); + let mut now = now(); + + let client_initial = client.process_output(now); + let (_, client_dcid, _, _) = + decode_initial_header(client_initial.as_dgram_ref().unwrap(), Role::Client).unwrap(); + let client_dcid = client_dcid.to_owned(); + + now += rtt / 2; + let server_packet = server.process(client_initial.as_dgram_ref(), now).dgram(); + let (server_initial, server_hs) = split_datagram(server_packet.as_ref().unwrap()); + let (protected_header, _, _, payload) = + decode_initial_header(&server_initial, Role::Server).unwrap(); + + // Now decrypt the packet. + let (aead, hp) = initial_aead_and_hp(&client_dcid, Role::Server); + let (header, pn) = remove_header_protection(&hp, protected_header, payload); + assert_eq!(pn, 0); + let pn_len = header.len() - protected_header.len(); + let mut buf = vec![0; payload.len()]; + let mut plaintext = aead + .decrypt(pn, &header, &payload[pn_len..], &mut buf) + .unwrap() + .to_owned(); + + // Now we need to find the frames. Make some really strong assumptions. + let mut dec = Decoder::new(&plaintext[..]); + assert_eq!(dec.decode(ACK_FRAME_1.len()), Some(ACK_FRAME_1)); + assert_eq!(dec.decode_varint(), Some(0x06)); // CRYPTO + assert_eq!(dec.decode_varint(), Some(0x00)); // offset + dec.skip_vvec(); // Skip over the payload. + + // Replace the ACK frame with PADDING. + plaintext[..ACK_FRAME_1.len()].fill(FRAME_TYPE_PADDING.try_into().unwrap()); + + // And rebuild a packet. + let mut packet = header.clone(); + packet.resize(MIN_INITIAL_PACKET_SIZE, 0); + aead.encrypt(pn, &header, &plaintext, &mut packet[header.len()..]) + .unwrap(); + apply_header_protection(&hp, &mut packet, protected_header.len()..header.len()); + let si = Datagram::new( + server_initial.source(), + server_initial.destination(), + server_initial.tos(), + packet, + ); + + // Now a connection can be made successfully. + now += rtt / 2; + client.process_input(&si, now); + client.process_input(&server_hs.unwrap(), now); + client.authenticated(AuthenticationStatus::Ok, now); + let finished = client.process_output(now); + + assert_eq!(*client.state(), State::Connected); + + now += rtt / 2; + _ = server.process(finished.as_dgram_ref(), now); + assert_eq!(*server.state(), State::Confirmed); + + // Don't deliver the server's handshake finished, it has ACKs. + // Now get a ticket. + let validation = AddressValidation::new(now, ValidateAddress::NoToken).unwrap(); + let validation = Rc::new(RefCell::new(validation)); + server.set_validation(&validation); + send_something(&mut server, now); + server.send_ticket(now, &[]).expect("can send ticket"); + let ticket = server.process_output(now).dgram(); + assert!(ticket.is_some()); + now += rtt / 2; + client.process_input(&ticket.unwrap(), now); + let token = get_tokens(&mut client).pop().unwrap(); + + // And connect again. + let mut client = default_client(); + client.enable_resumption(now, token).unwrap(); + let ticket_rtt = client.paths.rtt(); + let mut server = resumed_server(&client); + + connect_with_rtt(&mut client, &mut server, now, Duration::from_millis(50)); + assert_eq!( + client.paths.rtt(), + Duration::from_millis(50), + "previous RTT should be completely erased" + ); + ticket_rtt +} + +#[test] +fn ticket_rtt_less_than_default() { + assert_eq!( + ticket_rtt(Duration::from_millis(10)), + Duration::from_millis(10) + ); +} + +#[test] +fn ticket_rtt_larger_than_default() { + assert_eq!(ticket_rtt(Duration::from_millis(500)), INITIAL_RTT); +} + /// Check that a resumed connection uses a token on Initial packets. #[test] fn address_validation_token_resume() { diff --git a/neqo-transport/src/path.rs b/neqo-transport/src/path.rs index 35d29f0253..49c289f60b 100644 --- a/neqo-transport/src/path.rs +++ b/neqo-transport/src/path.rs @@ -27,7 +27,7 @@ use crate::{ packet::PacketBuilder, pmtud::Pmtud, recovery::{RecoveryToken, SentPacket}, - rtt::RttEstimate, + rtt::{RttEstimate, RttSource}, sender::PacketSender, stats::FrameStats, tracking::PacketNumberSpace, @@ -984,7 +984,7 @@ impl Path { &self.qlog, now - sent.time_sent(), Duration::new(0, 0), - false, + RttSource::Guesstimate, now, ); } diff --git a/neqo-transport/src/recovery/mod.rs b/neqo-transport/src/recovery/mod.rs index 5820dc07a0..f2c3e8e298 100644 --- a/neqo-transport/src/recovery/mod.rs +++ b/neqo-transport/src/recovery/mod.rs @@ -27,7 +27,7 @@ use crate::{ packet::PacketNumber, path::{Path, PathRef}, qlog::{self, QlogMetric}, - rtt::RttEstimate, + rtt::{RttEstimate, RttSource}, stats::{Stats, StatsCell}, tracking::{PacketNumberSpace, PacketNumberSpaceSet}, }; @@ -558,9 +558,13 @@ impl LossRecovery { now: Instant, ack_delay: Duration, ) { - let confirmed = self.confirmed_time.map_or(false, |t| t < send_time); + let source = if self.confirmed_time.map_or(false, |t| t < send_time) { + RttSource::AckConfirmed + } else { + RttSource::Ack + }; if let Some(sample) = now.checked_duration_since(send_time) { - rtt.update(&self.qlog, sample, ack_delay, confirmed, now); + rtt.update(&self.qlog, sample, ack_delay, source, now); } } diff --git a/neqo-transport/src/rtt.rs b/neqo-transport/src/rtt.rs index 7bfbbe4aaa..027b574aad 100644 --- a/neqo-transport/src/rtt.rs +++ b/neqo-transport/src/rtt.rs @@ -28,6 +28,17 @@ pub const GRANULARITY: Duration = Duration::from_millis(1); // Defined in -recovery 6.2 as 333ms but using lower value. pub const INITIAL_RTT: Duration = Duration::from_millis(100); +/// The source of the RTT measurement. +#[derive(Debug, PartialEq, Eq, PartialOrd, Ord, Clone, Copy)] +pub enum RttSource { + /// RTT guess from a retry or dropping a packet number space. + Guesstimate, + /// Ack on an unconfirmed connection. + Ack, + /// Ack on a confirmed connection. + AckConfirmed, +} + #[derive(Debug)] #[allow(clippy::module_name_repetitions)] pub struct RttEstimate { @@ -37,6 +48,7 @@ pub struct RttEstimate { rttvar: Duration, min_rtt: Duration, ack_delay: PeerAckDelay, + best_source: RttSource, } impl RttEstimate { @@ -58,6 +70,7 @@ impl RttEstimate { rttvar: Duration::from_millis(0), min_rtt: rtt, ack_delay: PeerAckDelay::Fixed(Duration::from_millis(25)), + best_source: RttSource::Ack, } } @@ -83,17 +96,24 @@ impl RttEstimate { self.ack_delay.update(cwnd, mtu, self.smoothed_rtt); } + pub fn is_guesstimate(&self) -> bool { + self.best_source == RttSource::Guesstimate + } + pub fn update( &mut self, qlog: &NeqoQlog, mut rtt_sample: Duration, ack_delay: Duration, - confirmed: bool, + source: RttSource, now: Instant, ) { + debug_assert!(source >= self.best_source); + self.best_source = max(self.best_source, source); + // Limit ack delay by max_ack_delay if confirmed. let mad = self.ack_delay.max(); - let ack_delay = if confirmed && ack_delay > mad { + let ack_delay = if self.best_source == RttSource::AckConfirmed && ack_delay > mad { mad } else { ack_delay @@ -205,6 +225,7 @@ impl Default for RttEstimate { rttvar: INITIAL_RTT / 2, min_rtt: INITIAL_RTT, ack_delay: PeerAckDelay::default(), + best_source: RttSource::Guesstimate, } } }