fix(server): reject unusable timestamp sessions

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,
+    decode_request, encode_open_reply, verify_packet_hmac, Clock, DecodedParams,
-    OpenReply, Params, FLAG_CLOSE, FLAG_OPEN, FLAG_REPLY,
+    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
+    /// payload, negotiates to parameters the server could not execute, would
-    /// does not implement. None of that mutates any live session.
+    /// 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"
+        );
     }
 }