Merge pull request #77 from Lochnair/fix/server-open-executable-params

fix(server): require an executable open before acknowledging it

crates/irtt-client/src/client/tests/support.rs

@@ -72,9 +72,10 @@ pub(super) fn open_reply(
     .unwrap()
 }
 
+/// Test params always name a representable length, so the platform
+/// representability check cannot fire here.
 pub(crate) fn echo_packet_len(hmac: bool, params: &Params) -> usize {
-    checked_echo_packet_len(hmac, params)
-        .expect("test params must have a non-negative packet length")
+    checked_echo_packet_len(hmac, params).expect("test params must have a representable length")
 }
 
 pub(super) fn echo_reply_packet(

crates/irtt-client/src/session/machine.rs

@@ -235,7 +235,6 @@ impl SessionMachine {
                 | irtt_proto::ProtoError::VarintOverflow
                 | irtt_proto::ProtoError::InvalidUtf8
                 | irtt_proto::ProtoError::InvalidEnum { .. }
-                | irtt_proto::ProtoError::NegativePacketLength { .. }
                 | irtt_proto::ProtoError::ParameterLengthTooLarge { .. }
                 | irtt_proto::ProtoError::MalformedParams),
             ) => Err(ClientError::Protocol(error)),
@@ -824,6 +823,13 @@ impl SessionMachine {
     }
 }
 
+/// Sizes the receive buffer for the negotiated layout.
+///
+/// Fallible only because a negotiated length wider than `usize` cannot name a
+/// buffer at all. Negotiation already rejects a returned length that is
+/// negative or larger than this client requested, and the requested length is a
+/// `u32`, so the error is unreachable here in practice — it is propagated
+/// rather than asserted away so that no 64-bit assumption is baked in.
 pub(crate) fn recv_buffer_size(
     has_hmac: bool,
     negotiated: Option<&NegotiatedParams>,

crates/irtt-client/tests/support/mod.rs

@@ -534,8 +534,7 @@ fn open_reply(flags: u8, token: u64, params: &Params, hmac_key: Option<&[u8]>) -
 }
 
 pub(super) fn test_echo_packet_len(hmac: bool, params: &Params) -> usize {
-    echo_packet_len(hmac, params)
-        .expect("managed runner test params must have a non-negative packet length")
+    echo_packet_len(hmac, params).expect("test params must have a representable length")
 }
 
 fn echo_reply_packet(

crates/irtt-proto/src/error.rs

@@ -30,8 +30,8 @@ pub enum ProtoError {
     InvalidUtf8,
     #[error("invalid enum value {value} for {name}")]
     InvalidEnum { name: &'static str, value: i64 },
-    #[error("packet length must be non-negative, got {length}")]
-    NegativePacketLength { length: i64 },
+    #[error("packet length {length} cannot be represented as a buffer length")]
+    PacketLengthUnrepresentable { length: i64 },
     #[error("parameter {tag} length is too large: {length} bytes")]
     ParameterLengthTooLarge { tag: u64, length: u64 },
     #[error("open reply has zero token without close flag")]

crates/irtt-proto/src/layout.rs

@@ -111,12 +111,41 @@ pub fn echo_header_len(hmac: bool, params: &Params) -> usize {
     PacketLayout::echo(hmac, params).header_len()
 }
 
+/// The datagram length of an ECHO request or reply under `params`.
+///
+/// The negotiated length is a floor a peer asks for, never a ceiling: a packet
+/// can never be shorter than the field block its negotiated layout requires, so
+/// the result is `max(header, requested)`.
+///
+/// A negative negotiated length is a legitimate input rather than an encoder
+/// error. It is accepted during open and echoed back unchanged, so a session
+/// can genuinely carry one, and there is no datagram shorter than none — it
+/// therefore requests no space beyond the mandatory field block, exactly as
+/// zero does.
+///
+/// # Errors
+///
+/// Returns [`ProtoError::PacketLengthUnrepresentable`] for a positive length
+/// this platform's `usize` cannot hold. That is a representability check, not a
+/// size policy: a wire value that cannot even name a local buffer must not be
+/// converted into one that can, because the result is handed to an allocator.
+/// It is unreachable on a 64-bit target, where every positive `i64` converts,
+/// and is the reason this returns a [`Result`] at all.
+///
+/// A ceiling on what a negotiated length may legitimately *be* — an MTU, a
+/// resource bound, a maximum packet size — is deliberately not here. That is
+/// server and runtime policy, and it belongs where the negotiation happens.
 pub fn echo_packet_len(hmac: bool, params: &Params) -> Result<usize> {
     let header_len = echo_header_len(hmac, params);
-    let requested =
-        usize::try_from(params.length).map_err(|_| ProtoError::NegativePacketLength {
+    let requested = if params.length <= 0 {
+        // No datagram is shorter than none, so a negative length asks for
+        // nothing beyond the mandatory field block, exactly as zero does.
+        0
+    } else {
+        usize::try_from(params.length).map_err(|_| ProtoError::PacketLengthUnrepresentable {
             length: params.length,
-        })?;
+        })?
+    };
     Ok(header_len.max(requested))
 }
 
@@ -278,32 +307,35 @@ mod tests {
     }
 
     #[test]
-    fn negotiated_length_never_smaller_than_header() {
+    fn negotiated_length_is_floored_at_the_required_field_block() {
+        // The 60-byte header of this layout is the floor. A negative negotiated
+        // length is not an error: it asks for nothing beyond the mandatory
+        // fields, exactly as zero does.
         let mut p = params(ReceivedStats::Both, StampAt::Both, Clock::Both);
-        p.length = 20;
-        assert_eq!(echo_packet_len(false, &p).unwrap(), 60);
-        p.length = 92;
-        assert_eq!(echo_packet_len(false, &p).unwrap(), 92);
+        for (length, expected) in [(-4096, 60), (-1, 60), (0, 60), (20, 60), (92, 92)] {
+            p.length = length;
+            assert_eq!(
+                echo_packet_len(false, &p),
+                Ok(expected),
+                "unexpected packet length for negotiated length {length}"
+            );
+        }
     }
 
+    /// A positive length wider than `usize` must stay an error rather than
+    /// becoming a saturated buffer size. Only reachable where `usize` is
+    /// narrower than `i64`; on a 64-bit target every positive `i64` converts.
+    #[cfg(target_pointer_width = "32")]
     #[test]
-    fn checked_packet_len_rejects_negative_requested_length() {
+    fn a_length_wider_than_usize_is_rejected_rather_than_saturated() {
         let mut p = params(ReceivedStats::Both, StampAt::Both, Clock::Both);
-        p.length = -1;
+        p.length = 5_000_000_000;
 
         assert_eq!(
             echo_packet_len(false, &p),
-            Err(ProtoError::NegativePacketLength { length: -1 })
+            Err(ProtoError::PacketLengthUnrepresentable {
+                length: 5_000_000_000
+            })
         );
     }
-
-    #[test]
-    fn checked_packet_len_preserves_non_negative_header_floor() {
-        let mut p = params(ReceivedStats::Both, StampAt::Both, Clock::Both);
-        p.length = 0;
-        assert_eq!(echo_packet_len(false, &p), Ok(60));
-
-        p.length = 92;
-        assert_eq!(echo_packet_len(false, &p), Ok(92));
-    }
 }

crates/irtt-proto/src/request.rs

@@ -84,8 +84,10 @@ pub enum DecodedRequestKind<'a> {
 /// # Errors
 ///
 /// Returns [`ProtoError::PayloadTooLarge`] when an ECHO payload does not fit
-/// the negotiated length, and [`ProtoError::NegativePacketLength`] when the
-/// negotiated length is negative.
+/// the negotiated length, and [`ProtoError::PacketLengthUnrepresentable`] when
+/// the negotiated length is positive and wider than `usize`. A *negative*
+/// negotiated length is not an error: see [`echo_packet_len`], which floors it
+/// at the mandatory field block.
 pub fn encode_request(request: RequestToEncode<'_>, hmac_key: Option<&[u8]>) -> Result<Vec<u8>> {
     match request {
         RequestToEncode::Open { params, no_test } => encode_open(params, no_test, hmac_key),

crates/irtt-proto/tests/directional_codecs.rs

@@ -305,6 +305,32 @@ fn echo_payload_is_copied_zero_filled_and_bounded_in_both_directions() {
     );
 }
 
+#[test]
+fn a_negative_negotiated_length_still_encodes_in_both_directions() {
+    // A server accepts a negative negotiated length during open and returns it
+    // unchanged, so a live session can hold one and must stay executable. There
+    // is no datagram shorter than none, so both directions fall back to the
+    // mandatory field block — header (4) + token (8) + sequence (4) — exactly
+    // as a zero length would.
+    let params = Params {
+        length: -4096,
+        ..Params::default()
+    };
+    assert_eq!(echo_packet_len(false, &params), Ok(16));
+
+    let request = encode_request(echo_request(&params, &[]), None)
+        .expect("a negative negotiated length must still encode a request");
+    assert_eq!(request.len(), 16);
+
+    let reply = encode_echo_reply(&echo_reply(&params, FLAG_REPLY), &params, None)
+        .expect("a negative negotiated length must still encode a reply");
+    assert_eq!(reply.len(), 16);
+    assert_eq!(
+        decode_echo_reply(&reply, &params, None).unwrap().token,
+        TOKEN
+    );
+}
+
 // ---------- Upstream midpoint ECHO-reply compatibility ----------
 //
 // Verified upstream irtt 0.9.1 behavior: for single-clock

crates/irtt-proto/tests/request_codec.rs

@@ -450,21 +450,34 @@ fn encoded_requests_decode_back_to_their_sender_side_identity() {
 }
 
 #[test]
-fn echo_encoding_rejects_a_negative_negotiated_length() {
+fn echo_encoding_floors_a_negative_negotiated_length_at_the_field_block() {
+    // A negative negotiated length is accepted during open and echoed back
+    // unchanged, so a real session can carry one. It must therefore stay
+    // encodable: it asks for no space beyond the mandatory fields.
     let params = Params {
         length: -1,
         ..Params::default()
     };
+    let packet = encode_request(
+        RequestToEncode::Echo {
+            token: TOKEN,
+            sequence: SEQUENCE,
+            params: &params,
+            payload: &[],
+        },
+        None,
+    )
+    .expect("a negative negotiated length must still encode");
+
+    // header (4) + token (8) + sequence (4).
+    assert_eq!(packet.len(), 16);
+    let request = decode_request(&packet).unwrap();
     assert_eq!(
-        encode_request(
-            RequestToEncode::Echo {
-                token: TOKEN,
-                sequence: SEQUENCE,
-                params: &params,
-                payload: &[],
-            },
-            None,
-        ),
-        Err(ProtoError::NegativePacketLength { length: -1 })
+        request.kind,
+        DecodedRequestKind::Echo {
+            token: TOKEN,
+            sequence: SEQUENCE,
+            tail: &[],
+        }
     );
 }

crates/irtt-server/AGENTS.md

@@ -60,6 +60,30 @@ Do not let reply behavior reveal which stage rejected a datagram — in
 particular, an authentication failure must be indistinguishable from an unknown
 token.
 
+## An acknowledged open must be executable
+
+Validate the **final, restricted** parameters before acknowledging an open, not
+just the requested ones. Once an open passes admission and negotiation, its
+effective parameters must be safe for the server to run.
+
+- Effective non-none `StampAt` with `Clock::Unspecified` is silently rejected.
+  Timestamps from no clock lay out no timestamp field, so the session could not
+  be run. Do **not** synthesize a clock and do **not** rewrite `StampAt` to
+  none: either answers with a session the client did not ask for. This is
+  `irtt-rs` policy for nonconforming input, chosen because the clean evidence
+  records the reference server accepting this combination and then failing on
+  the first echo. A conforming client always sends a clock when it selects
+  timestamps, so conforming traffic is unaffected.
+- `Clock::Unspecified` alone is valid — it is just an absent tag. Only the
+  combination is refused, and only an absent tag reaches it, since an explicit
+  zero Clock is already out of range for the decoder.
+- Run the check after negotiation, so that a later policy restricting `StampAt`
+  to none makes an omitted clock safe without this rule changing.
+- Negative `Length` stays an accepted negotiated value, returned unchanged in
+  the open reply and stored verbatim in the session. It needs no clamping to be
+  executable: `irtt-proto` floors actual echo packet sizing at the required
+  protocol field block, so a negative length behaves as zero does.
+
 ## Resource policy
 
 Total session and resource state must stay bounded. A single unauthenticated

crates/irtt-server/src/core.rs

@@ -1,8 +1,8 @@
 use std::{collections::HashMap, net::SocketAddr};
 
 use irtt_proto::{
-    decode_request, encode_open_reply, verify_packet_hmac, DecodedParams, DecodedRequestKind,
-    OpenReply, Params, FLAG_CLOSE, FLAG_OPEN, FLAG_REPLY,
+    decode_request, encode_open_reply, verify_packet_hmac, Clock, DecodedParams,
+    DecodedRequestKind, OpenReply, Params, StampAt, FLAG_CLOSE, FLAG_OPEN, FLAG_REPLY,
 };
 
 use crate::{
@@ -95,8 +95,9 @@ impl ServerCore {
     /// datagram is discarded when it is structurally invalid, carries the reply
     /// flag, disagrees with this server's authentication configuration, fails
     /// authentication, carries a malformed or out-of-range open parameter
-    /// payload, would exceed the session bound, or is a request kind this slice
-    /// does not implement. None of that mutates any live session.
+    /// payload, negotiates to parameters the server could not execute, would
+    /// exceed the session bound, or is a request kind this slice does not
+    /// implement. None of that mutates any live session.
     ///
     /// `Ok(None)` is not only a rejection, though. A close request that does
     /// release its session is answered with nothing as well, because protocol
@@ -161,6 +162,11 @@ impl ServerCore {
             return Ok(None);
         }
         let params = negotiate_params(decoded.params, &self.config);
+        // Deliberately after negotiation, and before either open path: what has
+        // to be executable is the effective session, not the request.
+        if !negotiated_params_are_admissible(&params) {
+            return Ok(None);
+        }
 
         if no_test {
             // No-test validates parameters without running a test: same
@@ -284,6 +290,11 @@ fn same_endpoint(session: SocketAddr, peer: SocketAddr) -> bool {
 
 /// Applies the open parameter rules the decoder cannot express.
 ///
+/// This judges the *request*, before negotiation, which is why it takes the
+/// decoded form with its presence flags. Whether the resulting session is
+/// coherent is a separate question, asked after negotiation by
+/// [`negotiated_params_are_admissible`].
+///
 /// Duration and Interval are accepted when absent, taking the wire default
 /// zero, and rejected when explicitly present and not positive. The presence
 /// distinction is why the decoder reports it: testing the value alone would
@@ -301,3 +312,35 @@ fn open_params_are_admissible(decoded: &DecodedParams) -> bool {
     admissible(decoded.presence.duration_ns, decoded.params.duration_ns)
         && admissible(decoded.presence.interval_ns, decoded.params.interval_ns)
 }
+
+/// Whether the *effective* parameters describe a session this server could
+/// actually run.
+///
+/// This is the last gate before an open is acknowledged, and it asks a
+/// different question from [`open_params_are_admissible`]: not whether the
+/// request was well formed, but whether the parameters negotiation settled on
+/// are coherent. It therefore runs on the negotiated params, after
+/// [`negotiate_params`], and covers the no-test path too — a no-test open
+/// exists to tell a client what the session would be, so it must not report a
+/// session that could not exist.
+///
+/// One rule so far: **selecting timestamps requires a specified clock.** A
+/// non-none `stamp_at` with [`Clock::Unspecified`] asks for timestamp fields
+/// from no clock at all, which lays out no timestamp field and leaves the
+/// session with a request the server cannot honor.
+///
+/// [`Clock::Unspecified`] means the Clock tag was absent — an explicit zero is
+/// already out of range for the decoder — so this cannot be reached by a
+/// conforming client, which sends a clock whenever it selects timestamps. The
+/// combination is silently rejected rather than repaired: synthesizing a clock
+/// would answer with a session the client never asked for, and rewriting
+/// `stamp_at` to none would silently drop the measurement it did ask for.
+/// Neither is the server's to decide.
+///
+/// The check is deliberately *after* negotiation so that a later policy slice
+/// restricting timestamps can restrict `stamp_at` to [`StampAt::None`], at
+/// which point an absent clock is safe and this predicate holds without
+/// changing.
+fn negotiated_params_are_admissible(params: &Params) -> bool {
+    params.stamp_at == StampAt::None || params.clock != Clock::Unspecified
+}

crates/irtt-server/src/negotiate.rs

@@ -18,6 +18,12 @@ use crate::config::ServerConfig;
 /// timestamp allowance, DSCP policy and server-fill allow-list. This function
 /// is where they belong, which is why the configuration is threaded through
 /// even though no knob reads it yet.
+///
+/// Negotiation rewriting nothing but the version does not mean every open is
+/// answered: the core validates what this function returns and silently
+/// discards an open whose effective parameters it could not execute. That is a
+/// separate step on purpose — this function decides what the session *would*
+/// be, and the core decides whether that session can exist.
 pub(crate) fn negotiate_params(mut requested: Params, _config: &ServerConfig) -> Params {
     requested.protocol_version = PROTOCOL_VERSION;
     requested

crates/irtt-server/src/tests/no_test.rs

@@ -1,4 +1,4 @@
-use irtt_proto::{verify_packet_hmac, Params};
+use irtt_proto::{verify_packet_hmac, Clock, Params, StampAt};
 
 use super::support::{
     client_params, core_with_tokens, expect_no_test_reply, no_test_request, open_request,
@@ -74,6 +74,30 @@ fn a_no_test_open_is_rejected_on_the_same_parameters_a_normal_open_is() {
     assert_eq!(core.session_count(), 0);
 }
 
+#[test]
+fn a_no_test_open_selecting_timestamps_without_a_clock_is_silently_refused() {
+    // No-test exists to tell a client what the session would be, so it must not
+    // report a session that could not exist. The effective-parameter check runs
+    // before the no-test path branches off, so both open forms refuse the same
+    // incoherent timestamp request.
+    let tokens = ScriptedTokens::new([TOKEN_A]);
+    let mut core = core_with_tokens(ServerConfig::default(), tokens.clone());
+
+    let requested = Params {
+        clock: Clock::Unspecified,
+        stamp_at: StampAt::Both,
+        ..client_params()
+    };
+
+    assert_eq!(
+        core.handle_datagram(peer(), &no_test_request(&requested, None))
+            .expect("an unusable timestamp request is not a server error"),
+        None
+    );
+    assert_eq!(core.session_count(), 0);
+    assert_eq!(tokens.remaining(), 1, "no token may be drawn");
+}
+
 #[test]
 fn a_no_test_open_is_still_answered_when_no_session_could_be_created() {
     // No-test consumes no capacity, so it must keep working exactly where a

crates/irtt-server/src/tests/open.rs

@@ -104,6 +104,10 @@ fn an_empty_parameter_payload_is_answered_with_only_a_version() {
             ..Params::default()
         }
     );
+    // An unspecified clock is not itself a defect. Only pairing it with a
+    // selected `stamp_at` is, so a session requesting no timestamps at all is
+    // fully valid with no Clock tag.
+    assert_eq!(reply.params.stamp_at, StampAt::None);
     assert_eq!(reply.params.clock, Clock::Unspecified);
     assert_eq!(core.session_count(), 1);
 }
@@ -136,10 +140,61 @@ fn unknown_parameter_tags_are_ignored_and_not_reflected_in_the_reply() {
 }
 
 #[test]
-fn this_slice_restricts_nothing_but_the_protocol_version() {
+fn selecting_timestamps_without_a_clock_is_silently_refused() {
+    // Timestamps from no clock at all describe a session the server could not
+    // run: the negotiated layout carries no timestamp field to fill. The open
+    // is discarded rather than repaired — synthesizing a clock would answer
+    // with a session the client never asked for, and dropping `stamp_at` would
+    // silently discard the measurement it did ask for.
+    //
+    // Only an *absent* Clock tag reaches this: an explicit zero is already out
+    // of range for the decoder, and a conforming client always sends a clock
+    // when it selects timestamps.
+    for stamp_at in [
+        StampAt::Send,
+        StampAt::Receive,
+        StampAt::Both,
+        StampAt::Midpoint,
+    ] {
+        let requested = Params {
+            clock: Clock::Unspecified,
+            stamp_at,
+            ..client_params()
+        };
+        let tokens = ScriptedTokens::new([TOKEN_A]);
+        let mut core = core_with_tokens(ServerConfig::default(), tokens.clone());
+
+        let reply = core
+            .handle_datagram(peer(), &open_request(&requested, None))
+            .expect("an unusable timestamp request is not a server error");
+
+        assert!(
+            reply.is_none(),
+            "{stamp_at:?} without a clock must be silent"
+        );
+        assert_eq!(
+            core.session_count(),
+            0,
+            "{stamp_at:?} without a clock must create no session"
+        );
+        assert_eq!(
+            tokens.remaining(),
+            1,
+            "{stamp_at:?} without a clock must not consume a token"
+        );
+    }
+}
+
+#[test]
+fn negotiation_restricts_nothing_but_the_protocol_version() {
     // These are the values a later restriction-policy slice will deliberately
     // start changing. Pinning them now makes that a visible decision rather
     // than an accident.
+    //
+    // This is about what *negotiation* rewrites, which is a separate question
+    // from whether the core will acknowledge the result: an open whose
+    // effective parameters are incoherent is still discarded, as
+    // `selecting_timestamps_without_a_clock_is_silently_refused` shows.
     let unrestricted = Params {
         protocol_version: 2,
         duration_ns: 3_600_000_000_000,
@@ -175,6 +230,11 @@ fn negative_and_out_of_range_length_and_dscp_survive_negotiation() {
     // Length and DSCP are accepted as decoded in this slice, including values a
     // socket could never carry. Restricting a DSCP the server cannot apply is a
     // later policy decision, not an admission rule.
+    //
+    // A negative length in particular stays negative in both the reply and the
+    // stored session. It does not need clamping here to be executable: echo
+    // packet sizing in `irtt-proto` floors it at the required field block, so
+    // the session this creates can be run as negotiated.
     for (length, dscp) in [(-1, -1), (0, 0), (1472, 184), (-4096, 512)] {
         let requested = Params {
             protocol_version: 1,
@@ -193,5 +253,11 @@ fn negative_and_out_of_range_length_and_dscp_survive_negotiation() {
         assert_eq!(reply.params.length, length);
         assert_eq!(reply.params.dscp, dscp);
         assert_eq!(core.session_count(), 1);
+        let session = core.session(TOKEN_A).expect("the session must be live");
+        assert_eq!(
+            session.params(),
+            &reply.params,
+            "the session must hold the negotiated values verbatim"
+        );
     }
 }