fix: Don't encode large RTT guesses in tickets

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

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

@@ -6,15 +6,26 @@
 
 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,
  new_client, resumed_server, send_something, AT_LEAST_PTO,
 };
 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() {

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,
  );
  }

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

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,
  }
  }
 }