From 36500e1ea07a6b0011773b9bf04769f40c1e2774 Mon Sep 17 00:00:00 2001 From: Nils Andreas Svee Date: Fri, 14 Aug 2026 12:32:37 +0700 Subject: [PATCH] feat(server): negotiate server fill --- crates/irtt-server/src/core.rs | 12 +- crates/irtt-server/src/fill.rs | 208 +++++++++++++++++++++++++ crates/irtt-server/src/lib.rs | 1 + crates/irtt-server/src/negotiate.rs | 9 +- crates/irtt-server/src/session.rs | 22 ++- crates/irtt-server/src/tests/fill.rs | 154 ++++++++++++++++++ crates/irtt-server/src/tests/mod.rs | 1 + crates/irtt-server/src/tests/open.rs | 21 +-- crates/irtt-server/src/tests/params.rs | 6 +- 9 files changed, 419 insertions(+), 15 deletions(-) create mode 100644 crates/irtt-server/src/fill.rs create mode 100644 crates/irtt-server/src/tests/fill.rs diff --git a/crates/irtt-server/src/core.rs b/crates/irtt-server/src/core.rs index cbb8c6f..594f64a 100644 --- a/crates/irtt-server/src/core.rs +++ b/crates/irtt-server/src/core.rs @@ -10,6 +10,7 @@ use crate::{ clock::{saturating_ns, ClockSample, ClockSource, SystemClock}, config::ServerConfig, error::ServerError, + fill::negotiate_server_fill, negotiate::negotiate_params, session::{RateDecision, Session}, token::{OsTokenSource, TokenSource, TOKEN_ATTEMPTS}, @@ -421,7 +422,14 @@ impl ServerCore { if !open_params_are_admissible(&decoded) { return Ok(None); } - let params = negotiate_params(decoded.params, &self.config); + let mut params = negotiate_params(decoded.params, &self.config); + // Settled here, once per open, so echo processing never parses a + // descriptor. It returns what the session will actually fill with, + // which an absent or empty descriptor deliberately does not describe, + // and restricts `params` only where an explicit descriptor could not be + // honored. A no-test open runs it for the negotiated value alone and + // drops the mode, having no session to keep it in. + let fill = negotiate_server_fill(&mut params); // 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, &self.config) { @@ -460,7 +468,7 @@ impl ServerCore { // nothing is cloned to keep both. self.sessions.insert( token, - Session::new(peer, reply.params, &self.config, now_ns), + Session::new(peer, reply.params, fill, &self.config, now_ns), ); Ok(Some(OutboundDatagram::unmarked(packet))) } diff --git a/crates/irtt-server/src/fill.rs b/crates/irtt-server/src/fill.rs new file mode 100644 index 0000000..0a8f5f5 --- /dev/null +++ b/crates/irtt-server/src/fill.rs @@ -0,0 +1,208 @@ +//! Server fill policy: what the ServerFill descriptor means, and what bytes it +//! puts in an echo reply's payload region. +//! +//! Descriptor *strings* are wire data and `irtt-proto` carries them verbatim; +//! deciding what one means is server policy and lives here. The result is a +//! [`FillMode`], which is deliberately crate-private: it is neither wire format +//! nor public configuration. + +use irtt_proto::{Params, ServerFill}; + +/// The bytes of this server's default fill, repeated: `69 72 74 74`, or `irtt`. +/// +/// The value matches the descriptor the clean evidence records the reference +/// server defaulting to, so a client that recognizes it sees what it expects. +/// Its use here is nevertheless `irtt-rs` policy: payload bytes carry no +/// protocol meaning, and nothing about interoperability requires this pattern +/// over any other. +const DEFAULT_FILL_PATTERN: &[u8] = b"irtt"; + +/// The descriptor naming [`DEFAULT_FILL_PATTERN`], as returned in an open reply +/// whose explicit request this server could not honor. +/// +/// Held as a constant beside the bytes rather than derived from them, so the +/// descriptor a client is told and the bytes it is sent cannot drift apart +/// through a formatting or parsing step neither of them needs. +pub(crate) const DEFAULT_FILL_DESCRIPTOR: &str = "pattern:69727474"; + +/// The mode prefix of a repeating hexadecimal pattern descriptor. +const PATTERN_PREFIX: &str = "pattern:"; + +/// How this server fills the payload region of one session's echo replies. +/// +/// Crate-private on purpose. It is an interpretation of a wire string, not the +/// string itself and not a configuration surface, so nothing outside the crate +/// has any use for it. +#[derive(Debug, Clone, PartialEq, Eq)] +pub(crate) enum FillMode { + /// Write no fill of the server's own. The payload region is zeroes — see + /// [`FillMode::payload`]. + None, + /// Fill the payload region with bytes from the operating system's random + /// source. + Random, + /// Fill the payload region with these bytes, repeated. Never empty. + Pattern(Vec), +} + +impl FillMode { + /// This server's default fill, used by a session whose client expressed no + /// preference and by one whose explicit descriptor could not be honored. + pub(crate) fn default_fill() -> Self { + Self::Pattern(DEFAULT_FILL_PATTERN.to_vec()) + } + + /// The mode a descriptor asks for, or `None` if it names none. + /// + /// The three mode names are matched exactly and case-sensitively; only a + /// pattern's hexadecimal body is case-insensitive. An unrecognized name, a + /// differently-cased one, and a `pattern:` whose body is empty, odd-length + /// or not hexadecimal all return `None`, and negotiation replaces them with + /// the default. Nothing here rejects an open, panics, or half-parses a + /// descriptor into a partial pattern. + fn parse(descriptor: &str) -> Option { + match descriptor { + "none" => Some(Self::None), + "rand" => Some(Self::Random), + _ => descriptor + .strip_prefix(PATTERN_PREFIX) + .and_then(parse_hex_pattern) + .map(Self::Pattern), + } + } +} + +/// The pattern a `pattern:` descriptor's body names, or `None` if the body is +/// not a whole number of hexadecimal bytes. +/// +/// A descriptor is at most [`MAX_SERVER_FILL_BYTES`] on the wire, so this +/// decodes at most twelve bytes and complexity is beside the point. Both digit +/// cases are accepted; nothing else is, including a non-ASCII character, whose +/// bytes are all rejected as digits. +/// +/// [`MAX_SERVER_FILL_BYTES`]: irtt_proto::MAX_SERVER_FILL_BYTES +fn parse_hex_pattern(body: &str) -> Option> { + let digits = body.as_bytes(); + if digits.is_empty() || !digits.len().is_multiple_of(2) { + return None; + } + digits + .chunks_exact(2) + .map(|pair| Some(hex_digit(pair[0])? << 4 | hex_digit(pair[1])?)) + .collect() +} + +/// The value of one ASCII hexadecimal digit. +fn hex_digit(digit: u8) -> Option { + match digit { + b'0'..=b'9' => Some(digit - b'0'), + b'a'..=b'f' => Some(digit - b'a' + 10), + b'A'..=b'F' => Some(digit - b'A' + 10), + _ => None, + } +} + +/// Settles what fill a session will actually use, restricting `params` only +/// where this server could not honor what was asked for. +/// +/// Four cases, and the distinction between the first two and the last is the +/// point: +/// +/// - **Absent or empty descriptor.** The client expressed no preference, so +/// `params` is left exactly as it arrived — absent stays absent, an explicit +/// empty value stays empty — and the session uses this server's default fill. +/// Writing the default descriptor into the reply would manufacture a +/// restriction out of a request that made none, and a strict client rejects a +/// descriptor it never asked for. +/// - **A recognized, well-formed descriptor.** Honored exactly, and returned +/// byte-for-byte, including a pattern body whose hexadecimal case differs from +/// the decoded bytes. `irtt-rs` accepts every valid `none`, `rand` and +/// `pattern:` descriptor: this is deliberately more permissive than the +/// reference server's default glob allow-list, which is upstream policy rather +/// than an interoperability requirement, and refusing valid modes would create +/// strict-negotiation failures while preventing nothing — the payload carries +/// no protocol meaning, the descriptor is bounded to 32 wire bytes, a pattern +/// is tiny, `none` is safely zero-filled here, and a random fill is bounded by +/// the negotiated packet length like every other reply. +/// - **An unrecognized or malformed explicit descriptor.** The server really did +/// change what was asked for, so it says so: `params.server_fill` becomes +/// [`DEFAULT_FILL_DESCRIPTOR`] and the session uses the default fill. A strict +/// client is then free to reject the session — the restriction is honest, and +/// hiding it would be worse. +/// +/// The descriptor is parsed exactly once, here, so echo processing never looks +/// at a string. +pub(crate) fn negotiate_server_fill(params: &mut Params) -> FillMode { + let Some(requested) = params + .server_fill + .as_ref() + .filter(|fill| !fill.value.is_empty()) + else { + return FillMode::default_fill(); + }; + match FillMode::parse(&requested.value) { + Some(mode) => mode, + None => { + params.server_fill = Some(ServerFill { + value: DEFAULT_FILL_DESCRIPTOR.to_owned(), + }); + FillMode::default_fill() + } + } +} + +#[cfg(test)] +mod tests { + use super::*; + + #[test] + fn recognized_descriptors_parse_to_their_mode() { + for (descriptor, expected) in [ + ("none", FillMode::None), + ("rand", FillMode::Random), + ("pattern:00", FillMode::Pattern(vec![0x00])), + ("pattern:ff00", FillMode::Pattern(vec![0xff, 0x00])), + // Only the hexadecimal body is case-insensitive. + ("pattern:AaBb", FillMode::Pattern(vec![0xaa, 0xbb])), + ( + "pattern:69727474", + FillMode::Pattern(DEFAULT_FILL_PATTERN.to_vec()), + ), + ] { + assert_eq!(FillMode::parse(descriptor), Some(expected), "{descriptor}"); + } + } + + #[test] + fn unrecognized_and_malformed_descriptors_parse_to_nothing() { + for descriptor in [ + "", + "bogus", + // The mode names themselves are case-sensitive. + "RAND", + "None", + "Pattern:aabb", + "rand ", + "pattern:", + "pattern:f", + "pattern:abc", + "pattern:zz", + "pattern:0g", + // A multi-byte character is not two hexadecimal digits, whatever + // its byte length. + "pattern:é", + ] { + assert_eq!(FillMode::parse(descriptor), None, "{descriptor:?}"); + } + } + + #[test] + fn the_default_descriptor_and_the_default_pattern_agree() { + // The two constants are held separately so nothing has to parse a + // string per open; this is what keeps them from drifting. + assert_eq!( + FillMode::parse(DEFAULT_FILL_DESCRIPTOR), + Some(FillMode::default_fill()) + ); + } +} diff --git a/crates/irtt-server/src/lib.rs b/crates/irtt-server/src/lib.rs index 7b46657..bfd2eb7 100644 --- a/crates/irtt-server/src/lib.rs +++ b/crates/irtt-server/src/lib.rs @@ -68,6 +68,7 @@ mod clock; mod config; mod core; mod error; +mod fill; mod negotiate; mod runtime; mod session; diff --git a/crates/irtt-server/src/negotiate.rs b/crates/irtt-server/src/negotiate.rs index 9c5b565..78b22b1 100644 --- a/crates/irtt-server/src/negotiate.rs +++ b/crates/irtt-server/src/negotiate.rs @@ -42,8 +42,13 @@ use crate::{clock::saturating_ns, config::ServerConfig}; /// interval, so this server never enforces a cadence slower than the one it just /// handed the client. /// -/// Later policy slices will add the timestamp allowance, DSCP policy and the -/// server-fill allow-list. +/// **ServerFill is settled separately**, by +/// [`negotiate_server_fill`](crate::fill::negotiate_server_fill), because it +/// produces a second result this function has nowhere to put: the effective +/// fill the session will use, which is not always what the descriptor in +/// `Params` says. The two run back to back on the same open. +/// +/// Later policy slices may add the timestamp allowance and DSCP policy. /// /// Negotiation is not by itself an acknowledgement: the core validates what this /// function returns and silently discards an open whose effective parameters it diff --git a/crates/irtt-server/src/session.rs b/crates/irtt-server/src/session.rs index c96c992..29338bc 100644 --- a/crates/irtt-server/src/session.rs +++ b/crates/irtt-server/src/session.rs @@ -2,7 +2,7 @@ use std::{net::SocketAddr, time::Duration}; use irtt_proto::Params; -use crate::{clock::saturating_ns, config::ServerConfig}; +use crate::{clock::saturating_ns, config::ServerConfig, fill::FillMode}; /// How long past its negotiated maximum test duration a session is served /// before the next echo it answers carries the Close flag. @@ -34,6 +34,7 @@ const MAX_DURATION_CLOSE_GRACE: Duration = Duration::from_secs(2); pub(crate) struct Session { peer: SocketAddr, params: Params, + fill: FillMode, receive: ReceiveState, rate: RateState, lifetime: LifetimeState, @@ -48,9 +49,16 @@ impl Session { /// [`RateState::new`] for why the two together decide it. The idle deadline /// starts running immediately: a session that never carries an echo request /// ages out like any other. + /// + /// `fill` is the *effective* fill negotiation settled on, which the session + /// has to be told rather than left to infer from `params` later: an absent + /// or empty descriptor keeps its representation on the wire while the + /// session uses this server's default fill, and that distinction only + /// exists at the moment of negotiation. pub(crate) fn new( peer: SocketAddr, params: Params, + fill: FillMode, config: &ServerConfig, now_ns: i64, ) -> Self { @@ -59,6 +67,7 @@ impl Session { lifetime: LifetimeState::opened_at(now_ns), peer, params, + fill, receive: ReceiveState::default(), } } @@ -89,6 +98,17 @@ impl Session { &self.params } + /// How this session's echo replies fill their payload region. + /// + /// This is the *effective* fill, which is not always what + /// [`params`](Self::params) reports: a client that expressed no preference + /// keeps its absent or empty descriptor in the negotiated parameters and is + /// served this server's default fill. Only a descriptor this server could + /// not honor is rewritten in `params`. + pub(crate) fn fill(&self) -> &FillMode { + &self.fill + } + /// What this session's accepted echo requests have added up to so far. pub(crate) fn receive_state(&self) -> ReceiveState { self.receive diff --git a/crates/irtt-server/src/tests/fill.rs b/crates/irtt-server/src/tests/fill.rs new file mode 100644 index 0000000..2504b73 --- /dev/null +++ b/crates/irtt-server/src/tests/fill.rs @@ -0,0 +1,154 @@ +//! Server fill negotiation: what an open reply reports, and what the session +//! will actually fill with. +//! +//! The two are deliberately different questions. A client that expressed no +//! preference keeps its absent or empty descriptor in the negotiated parameters +//! while the session uses this server's default fill, and only a descriptor the +//! server could not honor is rewritten. Asserting the reply alone would not +//! separate those, so these tests read the session's effective mode too. + +use irtt_proto::{Params, ServerFill}; + +use super::support::{ + client_params, core_with_tokens, expect_no_test_reply, expect_normal_open_reply, + no_test_request, open_request, peer, unthrottled, ScriptedTokens, +}; +use crate::{core::ServerCore, fill::FillMode}; + +const TOKEN_A: u64 = 0x0102_0304_0506_0708; + +/// Params requesting `descriptor` as the server fill. +fn requesting(descriptor: Option<&str>) -> Params { + Params { + server_fill: descriptor.map(|value| ServerFill { + value: value.to_owned(), + }), + ..client_params() + } +} + +/// Opens a session requesting `descriptor` and returns the descriptor the reply +/// carried together with the fill the session actually holds. +fn negotiated(descriptor: Option<&str>) -> (Option, FillMode) { + let mut core = core_with_tokens(unthrottled(), ScriptedTokens::new([TOKEN_A])); + let packet = core + .handle_datagram(peer(), &open_request(&requesting(descriptor), None)) + .unwrap() + .expect("the open must be answered"); + + let reply = expect_normal_open_reply(&packet, None); + let fill = effective_fill(&core, reply.token); + (reply.params.server_fill.map(|fill| fill.value), fill) +} + +/// The effective fill of the session `token` names. +fn effective_fill(core: &ServerCore, token: u64) -> FillMode { + core.session(token) + .expect("the open must have created a session") + .fill() + .clone() +} + +/// The descriptor a no-test open reports for `descriptor`, which creates no +/// session and so retains no fill. +fn no_test_negotiated(descriptor: Option<&str>) -> Option { + let tokens = ScriptedTokens::new([TOKEN_A]); + let mut core = core_with_tokens(unthrottled(), tokens.clone()); + let packet = core + .handle_datagram(peer(), &no_test_request(&requesting(descriptor), None)) + .unwrap() + .expect("the no-test open must be answered"); + + let reply = expect_no_test_reply(&packet, None); + assert_eq!(core.session_count(), 0, "no-test creates no session"); + assert_eq!(tokens.remaining(), 1, "no-test draws no session token"); + reply.params.server_fill.map(|fill| fill.value) +} + +#[test] +fn no_preference_keeps_an_absent_descriptor_and_uses_the_default_fill() { + // The important half is the first: a client that asked for nothing must be + // answered with nothing, or a strict client rejects a restriction the + // server never actually imposed. The server's own default is an internal + // choice and stays internal. + let (returned, fill) = negotiated(None); + assert_eq!(returned, None, "an absent request stays absent"); + assert_eq!(fill, FillMode::default_fill()); +} + +#[test] +fn an_explicitly_empty_descriptor_is_preserved_and_uses_the_default_fill() { + // A low-level peer can send this even though `ClientConfig` rejects an + // empty fill before it reaches the wire. The clean evidence groups empty + // with absent as "no preference", so it is neither refused nor rewritten: + // the requested wire value comes back exactly, and the server picks its own + // behavior behind it. + let (returned, fill) = negotiated(Some("")); + assert_eq!( + returned.as_deref(), + Some(""), + "an empty request stays empty" + ); + assert_eq!(fill, FillMode::default_fill()); +} + +#[test] +fn a_valid_descriptor_is_returned_exactly_as_requested() { + for (descriptor, expected) in [ + ("none", FillMode::None), + ("rand", FillMode::Random), + ("pattern:00", FillMode::Pattern(vec![0x00])), + ("pattern:ff00", FillMode::Pattern(vec![0xff, 0x00])), + // Returned with its own hexadecimal case, not normalized to the bytes + // it decodes to: rewriting it would report a restriction that did not + // happen, and a strict client would reject the session for it. + ("pattern:AaBb", FillMode::Pattern(vec![0xaa, 0xbb])), + ] { + let (returned, fill) = negotiated(Some(descriptor)); + assert_eq!(returned.as_deref(), Some(descriptor), "{descriptor}"); + assert_eq!(fill, expected, "{descriptor}"); + } +} + +#[test] +fn an_unknown_or_malformed_descriptor_is_replaced_by_the_default() { + // Here the server really did change what was asked for, so the reply says + // so and a strict client may refuse the session. The requested string is + // not retained anywhere. + for descriptor in [ + "bogus", + "RAND", + "None", + "Pattern:aabb", + "pattern:", + "pattern:f", + "pattern:abc", + "pattern:zz", + "pattern:0g", + "0123456789abcdef0123456789abcdef", + ] { + let (returned, fill) = negotiated(Some(descriptor)); + assert_eq!( + returned.as_deref(), + Some(crate::fill::DEFAULT_FILL_DESCRIPTOR), + "{descriptor}" + ); + assert_eq!(fill, FillMode::default_fill(), "{descriptor}"); + } +} + +#[test] +fn a_no_test_open_negotiates_fill_exactly_as_a_normal_open_does() { + // Same negotiation, no session, and therefore no effective mode to retain. + assert_eq!(no_test_negotiated(None), None); + assert_eq!(no_test_negotiated(Some("")).as_deref(), Some("")); + assert_eq!( + no_test_negotiated(Some("pattern:AaBb")).as_deref(), + Some("pattern:AaBb") + ); + assert_eq!(no_test_negotiated(Some("none")).as_deref(), Some("none")); + assert_eq!( + no_test_negotiated(Some("pattern:zz")).as_deref(), + Some(crate::fill::DEFAULT_FILL_DESCRIPTOR) + ); +} diff --git a/crates/irtt-server/src/tests/mod.rs b/crates/irtt-server/src/tests/mod.rs index 513687e..28067b6 100644 --- a/crates/irtt-server/src/tests/mod.rs +++ b/crates/irtt-server/src/tests/mod.rs @@ -14,6 +14,7 @@ mod admission; mod close; mod echo; +mod fill; mod hmac; mod lifecycle; mod negotiation; diff --git a/crates/irtt-server/src/tests/open.rs b/crates/irtt-server/src/tests/open.rs index 93d5913..a9ff45f 100644 --- a/crates/irtt-server/src/tests/open.rs +++ b/crates/irtt-server/src/tests/open.rs @@ -191,14 +191,17 @@ fn selecting_timestamps_without_a_clock_is_silently_refused() { #[test] fn current_non_length_parameters_survive_negotiation_unchanged() { - // Statistics, timestamps, DSCP and server fill are the values a later - // restriction-policy slice will deliberately start changing. Pinning them - // now makes that a visible decision rather than an accident. Version, an - // oversized packet length, the duration maximum and the interval floor and - // cap are already restricted and have their own tests; the length here is - // deliberately under the default maximum, and the configuration below - // leaves duration and interval unrestricted, so this test says nothing - // about either policy. + // Statistics, timestamps and DSCP are the values a later restriction-policy + // slice will deliberately start changing. Pinning them now makes that a + // visible decision rather than an accident. Version, an oversized packet + // length, the duration maximum and the interval floor and cap are already + // restricted and have their own tests; the length here is deliberately + // under the default maximum, and the configuration below leaves duration + // and interval unrestricted, so this test says nothing about either policy. + // + // The server fill below is a *valid* descriptor, which is what survives + // unchanged. Fill negotiation has its own restriction rule and its own + // tests in `fill`. // // This is about what *negotiation* rewrites, which is a separate question // from whether the core will acknowledge the result: an open whose @@ -214,7 +217,7 @@ fn current_non_length_parameters_survive_negotiation_unchanged() { clock: Clock::Monotonic, dscp: 184, server_fill: Some(ServerFill { - value: "pattern:abc".to_owned(), + value: "pattern:abcd".to_owned(), }), }; diff --git a/crates/irtt-server/src/tests/params.rs b/crates/irtt-server/src/tests/params.rs index f665c11..a524847 100644 --- a/crates/irtt-server/src/tests/params.rs +++ b/crates/irtt-server/src/tests/params.rs @@ -134,10 +134,14 @@ fn valid_optional_parameter_values_survive_unchanged() { }, ), ( + // Exactly `MAX_SERVER_FILL_BYTES`, and a valid descriptor: the + // longest one a client can send is also one this server honors, so + // the row stays about the length bound rather than about fill + // policy, which `fill` tests on its own. "maximum length server fill", Params { server_fill: Some(ServerFill { - value: "0123456789abcdef0123456789abcdef".to_owned(), + value: "pattern:0123456789abcdef01234567".to_owned(), }), ..Params::default() },