Merge pull request #66 from Lochnair/fix/upstream-midpoint-echo-compat

fix(client): accept upstream midpoint echo replies

crates/irtt-proto/src/echo.rs

@@ -2,10 +2,38 @@ use crate::{
     envelope::{self, FlagRule},
     flags::{FLAG_CLOSE, FLAG_OPEN, FLAG_REPLY},
     layout::{echo_packet_len, PacketLayout},
-    params::Params,
+    params::{Clock, Params, StampAt},
     ProtoError, Result, RECV_COUNT_SIZE, RECV_WINDOW_SIZE, SEQ_SIZE, TIMESTAMP_SIZE, TOKEN_SIZE,
 };
 
+/// Upstream irtt 0.9.1 always emits both midpoint timestamp fields (wall,
+/// then monotonic) in an ECHO reply for single-clock `StampAt::Midpoint`
+/// negotiations, even though only one clock was negotiated. That makes the
+/// compatibility header exactly one timestamp longer than the negotiated
+/// header.
+const MIDPOINT_COMPAT_EXTRA: usize = TIMESTAMP_SIZE;
+
+fn is_midpoint_single_clock(params: &Params) -> bool {
+    params.stamp_at == StampAt::Midpoint && !matches!(params.clock, Clock::Both)
+}
+
+/// Total datagram length of the upstream dual-midpoint compatibility form.
+///
+/// Both lengths are `max(negotiated_length, header)`, so this is *not*
+/// `normal_len + TIMESTAMP_SIZE`. The extra midpoint field only lengthens the
+/// datagram while the larger header is still pushing past the negotiated
+/// length; beyond that it displaces payload instead, and the two forms have
+/// identical length. The difference is therefore 8, 7..1, or 0 depending on
+/// the negotiated length.
+///
+/// Deriving this from `normal_len` keeps the identity exact —
+/// `max(negotiated_length, compat_header) == max(compat_header, normal_len)` —
+/// and cannot overflow, because `compat_header` is a small bounded header
+/// size rather than an attacker-influenced packet length.
+fn midpoint_compat_packet_len(layout: PacketLayout, normal_len: usize) -> usize {
+    (layout.header_len() + MIDPOINT_COMPAT_EXTRA).max(normal_len)
+}
+
 #[derive(Debug, Clone, PartialEq, Eq)]
 pub struct EchoRequest {
     pub token: u64,
@@ -83,7 +111,7 @@ pub fn decode_echo_request(
         ],
     )?;
     let layout = PacketLayout::echo(hmac_key.is_some(), params);
-    validate_echo_length(packet, params, layout)?;
+    validate_echo_length(packet, params, layout, false)?;
     envelope::verify(packet, hmac_key)?;
 
     let mut pos = envelope.body_offset;
@@ -128,6 +156,22 @@ pub fn encode_echo_reply(
     envelope::finish(out, hmac_key)
 }
 
+/// Decodes an ECHO reply, accepting the upstream irtt 0.9.1 dual-midpoint
+/// compatibility form when it is identifiable by datagram length.
+///
+/// For single-clock `StampAt::Midpoint` negotiations upstream emits both
+/// midpoint fields (wall, then monotonic). When that larger header makes the
+/// datagram longer than the negotiated length, the compatibility form is
+/// unambiguous and both fields are parsed, exposing only the negotiated
+/// clock's value.
+///
+/// When the negotiated length already covers the larger header the two forms
+/// have identical length and are indistinguishable from the packet alone. The
+/// reply is still accepted, parsed against the negotiated layout. For a
+/// monotonic-only negotiation that means the value exposed as `midpoint_mono`
+/// is whatever occupies the negotiated field position, which for an upstream
+/// peer is its wall timestamp — this cannot be corrected deterministically
+/// without out-of-band knowledge of the peer, so no heuristic is applied.
 pub fn decode_echo_reply(
     packet: &[u8],
     params: &Params,
@@ -139,23 +183,40 @@ pub fn decode_echo_reply(
         &[FlagRule::Reject(FLAG_OPEN), FlagRule::Require(FLAG_REPLY)],
     )?;
     let layout = PacketLayout::echo(hmac_key.is_some(), params);
-    validate_echo_length(packet, params, layout)?;
+    let midpoint_compat = validate_echo_length(packet, params, layout, true)?;
     envelope::verify(packet, hmac_key)?;
 
-    let header_len = layout.header_len();
+    let header_len = layout.header_len()
+        + if midpoint_compat {
+            MIDPOINT_COMPAT_EXTRA
+        } else {
+            0
+        };
     let mut pos = envelope.body_offset;
     let token = read_u64(packet, &mut pos);
     let sequence = read_u32(packet, &mut pos);
     let recv_count = layout.recv_count.then(|| read_u32(packet, &mut pos));
     let recv_window = layout.recv_window.then(|| read_u64(packet, &mut pos));
-    let timestamps = TimestampFields {
+    let recv_wall = layout.recv_wall.then(|| read_i64(packet, &mut pos));
-        recv_wall: layout.recv_wall.then(|| read_i64(packet, &mut pos)),
+    let recv_mono = layout.recv_mono.then(|| read_i64(packet, &mut pos));
-        recv_mono: layout.recv_mono.then(|| read_i64(packet, &mut pos)),
+    let (midpoint_wall, midpoint_mono) = if midpoint_compat {
-        midpoint_wall: layout.midpoint_wall.then(|| read_i64(packet, &mut pos)),
+        let wall = read_i64(packet, &mut pos);
-        midpoint_mono: layout.midpoint_mono.then(|| read_i64(packet, &mut pos)),
+        let mono = read_i64(packet, &mut pos);
-        send_wall: layout.send_wall.then(|| read_i64(packet, &mut pos)),
+        match params.clock {
-        send_mono: layout.send_mono.then(|| read_i64(packet, &mut pos)),
+            Clock::Wall => (Some(wall), None),
+            Clock::Monotonic => (None, Some(mono)),
+            Clock::Both => {
+                unreachable!("midpoint compat only applies to single-clock negotiations")
+            }
+        }
+    } else {
+        (
+            layout.midpoint_wall.then(|| read_i64(packet, &mut pos)),
+            layout.midpoint_mono.then(|| read_i64(packet, &mut pos)),
+        )
     };
+    let send_wall = layout.send_wall.then(|| read_i64(packet, &mut pos));
+    let send_mono = layout.send_mono.then(|| read_i64(packet, &mut pos));
 
     Ok(EchoReply {
         flags: envelope.flags,
@@ -163,12 +224,32 @@ pub fn decode_echo_reply(
         sequence,
         recv_count,
         recv_window,
-        timestamps,
+        timestamps: TimestampFields {
+            recv_wall,
+            recv_mono,
+            midpoint_wall,
+            midpoint_mono,
+            send_wall,
+            send_mono,
+        },
         payload: packet[header_len..].to_vec(),
     })
 }
 
-fn validate_echo_length(packet: &[u8], params: &Params, layout: PacketLayout) -> Result<()> {
+/// Validates the packet length against the negotiated layout. Returns
+/// `Ok(true)` when the packet matched the upstream midpoint compatibility
+/// length rather than the exact negotiated length (only possible when
+/// `allow_midpoint_compat` is set).
+///
+/// The negotiated length is always checked first, so when the compatibility
+/// form is not longer than the negotiated form the two are indistinguishable
+/// by length and normal negotiated parsing wins. See [`decode_echo_reply`].
+fn validate_echo_length(
+    packet: &[u8],
+    params: &Params,
+    layout: PacketLayout,
+    allow_midpoint_compat: bool,
+) -> Result<bool> {
     let header_len = layout.header_len();
     if packet.len() < header_len {
         return Err(ProtoError::PacketTooShort {
@@ -177,13 +258,19 @@ fn validate_echo_length(packet: &[u8], params: &Params, layout: PacketLayout) ->
         });
     }
     let expected_len = echo_packet_len(layout.hmac, params)?;
-    if packet.len() != expected_len {
+    if packet.len() == expected_len {
-        return Err(ProtoError::PacketLengthMismatch {
+        return Ok(false);
-            expected: expected_len,
-            actual: packet.len(),
-        });
     }
-    Ok(())
+    if allow_midpoint_compat && is_midpoint_single_clock(params) {
+        let compat_len = midpoint_compat_packet_len(layout, expected_len);
+        if compat_len > expected_len && packet.len() == compat_len {
+            return Ok(true);
+        }
+    }
+    Err(ProtoError::PacketLengthMismatch {
+        expected: expected_len,
+        actual: packet.len(),
+    })
 }
 
 fn push_zeroed_layout_tail(layout: PacketLayout, out: &mut Vec<u8>) {

crates/irtt-proto/tests/directional_codecs.rs

@@ -1,11 +1,16 @@
 use irtt_proto::{
     decode_close_request, decode_echo_reply, decode_echo_request, decode_open_reply,
-    decode_open_request, encode_close_request, encode_echo_reply, encode_echo_request,
+    decode_open_request, echo_packet_len, encode_close_request, encode_echo_reply,
-    encode_open_reply, encode_open_request, Clock, CloseRequest, EchoReply, EchoRequest, OpenReply,
+    encode_echo_request, encode_open_reply, encode_open_request, hmac::compute_hmac_in_place,
-    OpenRequest, PacketLayout, Params, ProtoError, ReceivedStats, StampAt, TimestampFields,
+    Clock, CloseRequest, EchoReply, EchoRequest, OpenReply, OpenRequest, PacketLayout, Params,
-    FLAG_CLOSE, FLAG_HMAC, FLAG_OPEN, FLAG_REPLY, MAGIC,
+    ProtoError, ReceivedStats, StampAt, TimestampFields, FLAG_CLOSE, FLAG_HMAC, FLAG_OPEN,
+    FLAG_REPLY, MAGIC,
 };
 
+/// Wire offset of the HMAC digest within an ECHO reply: the fixed 4-byte
+/// header (magic + flags) always precedes it.
+const HMAC_OFFSET: usize = 4;
+
 const KEY: &[u8] = b"testkey";
 const TOKEN: u64 = 0x7896_b6ab_8771_5213;
 
@@ -359,3 +364,265 @@ fn echo_payload_is_copied_zero_filled_and_bounded_in_both_directions() {
         })
     );
 }
+
+// ---------- Upstream midpoint ECHO-reply compatibility ----------
+//
+// Verified upstream irtt 0.9.1 behavior: for single-clock
+// `StampAt::Midpoint` negotiations (`Clock::Wall` or `Clock::Monotonic`)
+// upstream always emits BOTH midpoint timestamp fields (wall, then
+// monotonic), so its header is one timestamp longer than the negotiated
+// header.
+//
+// Crucially the datagram length is NOT `normal_len + 8`. Both forms are
+// `max(negotiated_length, header)`, so the extra field lengthens the
+// datagram only while the larger header still exceeds the negotiated
+// length, and displaces payload beyond that:
+//
+//     compat_len - normal_len == 8, then 7..1, then 0
+//
+// `decode_echo_reply` must accept the longer form wherever it is
+// identifiable by length, keep rejecting unrelated lengths, and fall back
+// to plain negotiated parsing once the two forms are the same length.
+//
+// `encode_echo_reply` must never produce the dual-field shape, so these
+// tests hand-build the wire form directly and narrowly.
+
+/// Midpoint params whose negotiated length sits `payload_space` bytes past
+/// the normal (single-midpoint) header.
+fn midpoint_params(clock: Clock, payload_space: usize) -> Params {
+    let base = Params {
+        protocol_version: 1,
+        received_stats: ReceivedStats::Count,
+        stamp_at: StampAt::Midpoint,
+        clock,
+        ..Params::default()
+    };
+    let header_len = PacketLayout::echo(false, &base).header_len();
+    Params {
+        length: (header_len + payload_space) as i64,
+        ..base
+    }
+}
+
+fn midpoint_compat_len(params: &Params, hmac: bool) -> usize {
+    let compat_header = PacketLayout::echo(hmac, params).header_len() + 8;
+    compat_header.max(echo_packet_len(hmac, params).unwrap())
+}
+
+/// Hand-builds upstream's dual-midpoint ECHO reply: both midpoint fields on
+/// the wire, total length `max(negotiated_length, normal_header + 8)`.
+fn build_midpoint_dual_field_reply(
+    params: &Params,
+    flags: u8,
+    seq: u32,
+    midpoint_wire_fields: (i64, i64),
+    hmac_key: Option<&[u8]>,
+) -> Vec<u8> {
+    let (wall, mono) = midpoint_wire_fields;
+    let hmac = hmac_key.is_some();
+    let layout = PacketLayout::echo(hmac, params);
+    let compat_header_len = layout.header_len() + 8;
+    let compat_len = midpoint_compat_len(params, hmac);
+
+    let mut packet = MAGIC.to_vec();
+    packet.push(flags);
+    if hmac {
+        packet.extend_from_slice(&[0u8; 16]);
+    }
+    packet.extend_from_slice(&TOKEN.to_le_bytes());
+    packet.extend_from_slice(&seq.to_le_bytes());
+    if layout.recv_count {
+        packet.extend_from_slice(&7_u32.to_le_bytes());
+    }
+    if layout.recv_window {
+        packet.extend_from_slice(&11_u64.to_le_bytes());
+    }
+    packet.extend_from_slice(&wall.to_le_bytes());
+    packet.extend_from_slice(&mono.to_le_bytes());
+    assert_eq!(packet.len(), compat_header_len);
+
+    packet.resize(compat_len, 0);
+    for (index, byte) in packet[compat_header_len..].iter_mut().enumerate() {
+        *byte = (index as u8).wrapping_add(1);
+    }
+
+    if let Some(key) = hmac_key {
+        compute_hmac_in_place(key, &mut packet, HMAC_OFFSET).unwrap();
+    }
+    packet
+}
+
+#[test]
+fn midpoint_dual_field_reply_is_accepted_wherever_length_identifies_it() {
+    // payload_space -> how much longer the compatibility datagram is.
+    // Covers the full +8 regime and the intermediate +7..+1 regimes.
+    for (payload_space, expected_extra) in [(0, 8), (1, 7), (4, 4), (7, 1)] {
+        for clock in [Clock::Wall, Clock::Monotonic] {
+            let params = midpoint_params(clock, payload_space);
+            let normal_len = echo_packet_len(false, &params).unwrap();
+            let packet = build_midpoint_dual_field_reply(&params, FLAG_REPLY, 3, (111, 222), None);
+            assert_eq!(
+                packet.len(),
+                normal_len + expected_extra,
+                "payload_space {payload_space} clock {clock:?}"
+            );
+
+            let reply = decode_echo_reply(&packet, &params, None).unwrap();
+            let (expected_wall, expected_mono) = match clock {
+                Clock::Wall => (Some(111), None),
+                // The first wire field must not be mistaken for the
+                // negotiated monotonic value.
+                Clock::Monotonic => (None, Some(222)),
+                Clock::Both => unreachable!(),
+            };
+            assert_eq!(
+                reply.timestamps.midpoint_wall, expected_wall,
+                "payload_space {payload_space} clock {clock:?}"
+            );
+            assert_eq!(
+                reply.timestamps.midpoint_mono, expected_mono,
+                "payload_space {payload_space} clock {clock:?}"
+            );
+            // Payload begins after BOTH compatibility timestamp fields.
+            let compat_header_len = PacketLayout::echo(false, &params).header_len() + 8;
+            assert_eq!(reply.payload, packet[compat_header_len..].to_vec());
+        }
+    }
+}
+
+#[test]
+fn midpoint_dual_field_reply_of_equal_length_is_accepted_but_not_correctable() {
+    // Once the negotiated length covers the larger header, the conforming
+    // and upstream forms have identical length. The reply must still be
+    // accepted, parsed against the negotiated layout.
+    for payload_space in [8, 64] {
+        for clock in [Clock::Wall, Clock::Monotonic] {
+            let params = midpoint_params(clock, payload_space);
+            let normal_len = echo_packet_len(false, &params).unwrap();
+            let packet = build_midpoint_dual_field_reply(&params, FLAG_REPLY, 4, (111, 222), None);
+            assert_eq!(
+                packet.len(),
+                normal_len,
+                "compat and negotiated lengths coincide for payload_space {payload_space}"
+            );
+
+            let reply = decode_echo_reply(&packet, &params, None).unwrap();
+            match clock {
+                // Wall-only happens to line up: the negotiated field
+                // position holds upstream's wall timestamp.
+                Clock::Wall => {
+                    assert_eq!(reply.timestamps.midpoint_wall, Some(111));
+                    assert_eq!(reply.timestamps.midpoint_mono, None);
+                }
+                // Monotonic-only cannot be corrected deterministically from
+                // the packet alone: the negotiated field position holds
+                // upstream's WALL timestamp, and nothing in the datagram
+                // distinguishes that from a conforming reply. Documented
+                // limitation - we accept rather than guess.
+                Clock::Monotonic => {
+                    assert_eq!(reply.timestamps.midpoint_wall, None);
+                    assert_eq!(reply.timestamps.midpoint_mono, Some(111));
+                }
+                Clock::Both => unreachable!(),
+            }
+        }
+    }
+}
+
+#[test]
+fn midpoint_dual_field_reply_authenticates_over_full_packet() {
+    let params = midpoint_params(Clock::Wall, 0);
+    let packet =
+        build_midpoint_dual_field_reply(&params, FLAG_REPLY | FLAG_HMAC, 9, (555, 666), Some(KEY));
+    assert_eq!(packet.len(), midpoint_compat_len(&params, true));
+
+    let reply = decode_echo_reply(&packet, &params, Some(KEY)).unwrap();
+    assert_eq!(reply.timestamps.midpoint_wall, Some(555));
+
+    let mut corrupted = packet;
+    corrupted[HMAC_OFFSET] ^= 0xFF;
+    assert_eq!(
+        decode_echo_reply(&corrupted, &params, Some(KEY)),
+        Err(ProtoError::BadHmac)
+    );
+}
+
+#[test]
+fn only_the_exact_compat_length_is_accepted_as_an_extension() {
+    let params = midpoint_params(Clock::Wall, 0);
+    let packet = build_midpoint_dual_field_reply(&params, FLAG_REPLY, 1, (10, 20), None);
+    let expected_len = echo_packet_len(false, &params).unwrap();
+    assert_eq!(packet.len(), expected_len + 8);
+
+    // Every other length around the compatibility size stays rejected,
+    // including the intermediate +1..+7 sizes that are only valid under a
+    // different negotiated length.
+    for delta in [-7_i64, -1, 1, 92] {
+        let target_len = (packet.len() as i64 + delta) as usize;
+        let mut malformed = packet.clone();
+        malformed.resize(target_len, 0);
+        assert_eq!(
+            decode_echo_reply(&malformed, &params, None),
+            Err(ProtoError::PacketLengthMismatch {
+                expected: expected_len,
+                actual: target_len,
+            }),
+            "delta {delta} (target length {target_len}) must still be rejected"
+        );
+    }
+}
+
+#[test]
+fn midpoint_both_clock_reply_has_no_additional_extension() {
+    let params = midpoint_params(Clock::Both, 0);
+    let reply = echo_reply(&params, FLAG_REPLY);
+    let packet = encode_echo_reply(&reply, &params, None).unwrap();
+
+    let mut too_long = packet.clone();
+    too_long.extend_from_slice(&[0; 8]);
+    assert_eq!(
+        decode_echo_reply(&too_long, &params, None),
+        Err(ProtoError::PacketLengthMismatch {
+            expected: packet.len(),
+            actual: too_long.len(),
+        })
+    );
+}
+
+#[test]
+fn non_midpoint_stamp_at_has_no_longer_form_exception() {
+    let params = Params {
+        received_stats: ReceivedStats::None,
+        stamp_at: StampAt::Receive,
+        clock: Clock::Wall,
+        ..Params::default()
+    };
+    let reply = echo_reply(&params, FLAG_REPLY);
+    let packet = encode_echo_reply(&reply, &params, None).unwrap();
+
+    let mut too_long = packet.clone();
+    too_long.extend_from_slice(&[0; 8]);
+    assert_eq!(
+        decode_echo_reply(&too_long, &params, None),
+        Err(ProtoError::PacketLengthMismatch {
+            expected: packet.len(),
+            actual: too_long.len(),
+        })
+    );
+}
+
+#[test]
+fn echo_request_decoding_rejects_the_compat_length() {
+    let params = midpoint_params(Clock::Wall, 0);
+    let packet = encode_echo_request(&echo_request(Vec::new()), &params, None).unwrap();
+
+    let mut too_long = packet.clone();
+    too_long.extend_from_slice(&[0; 8]);
+    assert_eq!(
+        decode_echo_request(&too_long, &params, None),
+        Err(ProtoError::PacketLengthMismatch {
+            expected: packet.len(),
+            actual: too_long.len(),
+        })
+    );
+}