diff --git a/crates/irtt-client/src/client/tests/support.rs b/crates/irtt-client/src/client/tests/support.rs index d612696..0826134 100644 --- a/crates/irtt-client/src/client/tests/support.rs +++ b/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( diff --git a/crates/irtt-client/src/session/machine.rs b/crates/irtt-client/src/session/machine.rs index a7a7775..cbfe6c7 100644 --- a/crates/irtt-client/src/session/machine.rs +++ b/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>, diff --git a/crates/irtt-client/tests/support/mod.rs b/crates/irtt-client/tests/support/mod.rs index 1d1d592..d88e26b 100644 --- a/crates/irtt-client/tests/support/mod.rs +++ b/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( diff --git a/crates/irtt-proto/src/error.rs b/crates/irtt-proto/src/error.rs index 1e69546..8a876c5 100644 --- a/crates/irtt-proto/src/error.rs +++ b/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")] diff --git a/crates/irtt-proto/src/layout.rs b/crates/irtt-proto/src/layout.rs index e712f18..7c486a0 100644 --- a/crates/irtt-proto/src/layout.rs +++ b/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 { 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)); - } } diff --git a/crates/irtt-proto/src/request.rs b/crates/irtt-proto/src/request.rs index 0e2c670..834e37c 100644 --- a/crates/irtt-proto/src/request.rs +++ b/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> { match request { RequestToEncode::Open { params, no_test } => encode_open(params, no_test, hmac_key), diff --git a/crates/irtt-proto/tests/directional_codecs.rs b/crates/irtt-proto/tests/directional_codecs.rs index cfdd467..25b9a13 100644 --- a/crates/irtt-proto/tests/directional_codecs.rs +++ b/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, ¶ms), Ok(16)); + + let request = encode_request(echo_request(¶ms, &[]), None) + .expect("a negative negotiated length must still encode a request"); + assert_eq!(request.len(), 16); + + let reply = encode_echo_reply(&echo_reply(¶ms, FLAG_REPLY), ¶ms, None) + .expect("a negative negotiated length must still encode a reply"); + assert_eq!(reply.len(), 16); + assert_eq!( + decode_echo_reply(&reply, ¶ms, None).unwrap().token, + TOKEN + ); +} + // ---------- Upstream midpoint ECHO-reply compatibility ---------- // // Verified upstream irtt 0.9.1 behavior: for single-clock diff --git a/crates/irtt-proto/tests/request_codec.rs b/crates/irtt-proto/tests/request_codec.rs index daa353d..2652732 100644 --- a/crates/irtt-proto/tests/request_codec.rs +++ b/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: ¶ms, + 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: ¶ms, - payload: &[], - }, - None, - ), - Err(ProtoError::NegativePacketLength { length: -1 }) + request.kind, + DecodedRequestKind::Echo { + token: TOKEN, + sequence: SEQUENCE, + tail: &[], + } ); } diff --git a/crates/irtt-server/AGENTS.md b/crates/irtt-server/AGENTS.md index 3069944..e4d0dc0 100644 --- a/crates/irtt-server/AGENTS.md +++ b/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 diff --git a/crates/irtt-server/src/core.rs b/crates/irtt-server/src/core.rs index 7320c0e..0e28c6b 100644 --- a/crates/irtt-server/src/core.rs +++ b/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(¶ms) { + 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 +} diff --git a/crates/irtt-server/src/negotiate.rs b/crates/irtt-server/src/negotiate.rs index 6485153..486dfe6 100644 --- a/crates/irtt-server/src/negotiate.rs +++ b/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 diff --git a/crates/irtt-server/src/tests/no_test.rs b/crates/irtt-server/src/tests/no_test.rs index ea4602d..ea89ca2 100644 --- a/crates/irtt-server/src/tests/no_test.rs +++ b/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 diff --git a/crates/irtt-server/src/tests/open.rs b/crates/irtt-server/src/tests/open.rs index c2876d7..9a13509 100644 --- a/crates/irtt-server/src/tests/open.rs +++ b/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" + ); } }