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

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