From d269f77c87b6e23eaee3ef399e62d2b4e8cd4a47 Mon Sep 17 00:00:00 2001 From: Nils Andreas Svee Date: Wed, 12 Aug 2026 10:30:50 +0700 Subject: [PATCH] feat(server): handle client close MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Release the session a client close names. A close is bound to a session only when the token matches and the datagram arrived from that session's exact endpoint, so holding a token is not by itself authority to tear the session down: a close from a foreign source port, address, family or IPv6 scope leaves the session live, and one close cannot take down a sibling session the same peer opened separately. A token that is unknown, already closed or zero matches nothing and leaves the table untouched; zero is reserved for no-test replies and never issued. Nothing is sent either way. Protocol version 1 defines no acknowledgement for a client close, so a successful close returns the same `Ok(None)` as a rejected one and a delivered close is indistinguishable to the client from a lost one. A standalone `Reply|Close` datagram is tolerated by clients but has no demonstrated use, so none is encoded. Authentication still runs before the session is looked up. That keeps a bad MAC on a live token indistinguishable from an unknown token, and means a close that fails authentication — missing, wrongly keyed, corrupted, or unexpected on a server with no key — cannot remove a session. Trailing bytes after the token stay acceptable: the structural decoder already tolerates them and the server does not re-parse the request. The session table remains the only record of how full the server is, so a close frees capacity immediately and a refused open can succeed once one is released. Close draws no token and touches no other state; the endpoint lookup borrows the table only until the comparison is made, so the removal needs no clone. Echo remains ignored. The test that pinned close as a no-op is replaced by `tests/close.rs`, which covers removal, silence, repeated close, echo after close, endpoint binding, unknown/zero/stale tokens, authentication failure, trailing bytes, sibling sessions and capacity reuse; `unsupported.rs` keeps echo and structurally invalid traffic. --- AGENTS.md | 2 +- crates/irtt-server/src/core.rs | 54 +++- crates/irtt-server/src/lib.rs | 8 +- crates/irtt-server/src/session.rs | 12 +- crates/irtt-server/src/tests/close.rs | 274 ++++++++++++++++++++ crates/irtt-server/src/tests/mod.rs | 1 + crates/irtt-server/src/tests/support.rs | 48 ++++ crates/irtt-server/src/tests/unsupported.rs | 35 +-- 8 files changed, 380 insertions(+), 54 deletions(-) create mode 100644 crates/irtt-server/src/tests/close.rs diff --git a/AGENTS.md b/AGENTS.md index 8a15cda..016a5c2 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -57,7 +57,7 @@ Tokio must remain optional for `irtt-client`; default builds must remain runtime ### Server architecture (`irtt-server`) -`irtt-server` is a first-class reusable server crate. It currently contains the deterministic OPEN/session core — packet admission, authentication policy, open negotiation, the bounded session table and open reply construction. ECHO processing, client/server close and session lifetime remain unimplemented, as does the Tokio runtime; see `crates/irtt-server/AGENTS.md`. The rest of this section describes the boundary the crate is built to, including the parts not yet written. +`irtt-server` is a first-class reusable server crate. It currently contains the deterministic OPEN/session core — packet admission, authentication policy, open negotiation, the bounded session table, open reply construction and client-initiated close. ECHO processing, server-initiated close and session lifetime remain unimplemented, as does the Tokio runtime; see `crates/irtt-server/AGENTS.md`. The rest of this section describes the boundary the crate is built to, including the parts not yet written. - It shares wire semantics with the client through `irtt-proto` and follows the same clean-room boundary. - It must **not** depend on `irtt-client` merely to reuse protocol behavior. diff --git a/crates/irtt-server/src/core.rs b/crates/irtt-server/src/core.rs index 2265937..fbcc117 100644 --- a/crates/irtt-server/src/core.rs +++ b/crates/irtt-server/src/core.rs @@ -35,9 +35,9 @@ use crate::{ /// /// # Scope /// -/// This slice implements open handling and session creation. Echo and close -/// requests are structurally recognized and then deliberately ignored — no -/// reply, no session mutation — until their own slices. +/// This slice implements open handling, session creation and client-initiated +/// close. Echo requests are structurally recognized and then deliberately +/// ignored — no reply, no session mutation — until their own slice. #[derive(Debug)] pub struct ServerCore { config: ServerConfig, @@ -81,11 +81,8 @@ impl ServerCore { /// /// This is a lookup by token alone and is therefore *not* an admission /// check: a request is bound to a session only when the token matches and - /// it arrived from the session's exact endpoint. - // - // Called only by tests until echo and close processing arrive, which is - // what a token lookup is for. - #[allow(dead_code)] + /// it arrived from the session's exact endpoint. Callers pair it with an + /// endpoint comparison, as close handling does. pub(crate) fn session(&self, token: u64) -> Option<&Session> { self.sessions.get(&token) } @@ -101,6 +98,10 @@ impl ServerCore { /// payload, 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 + /// version 1 defines no acknowledgement for one. + /// /// Authentication is checked before open parameters are decoded. That /// ordering is deliberate: a packet that fails authentication is discarded /// without parsing untrusted parameter data, and no reply behavior may @@ -133,11 +134,14 @@ impl ServerCore { match request.kind { DecodedRequestKind::Open { no_test, params } => self.handle_open(peer, no_test, params), - // Not implemented in this slice. A close must not remove a session - // and an echo must not advance receive state until the slices that - // implement those behaviors; emitting a placeholder reply now would - // be worse than silence. - DecodedRequestKind::Close { .. } | DecodedRequestKind::Echo { .. } => Ok(None), + DecodedRequestKind::Close { token } => { + self.handle_close(peer, token); + Ok(None) + } + // Not implemented in this slice. An echo must not advance receive + // state until the slice that implements it; emitting a placeholder + // reply now would be worse than silence. + DecodedRequestKind::Echo { .. } => Ok(None), } } @@ -191,6 +195,30 @@ impl ServerCore { Ok(Some(packet)) } + /// Releases the session a close request names, if that request owns one. + /// + /// A close is bound to a session only when the token matches *and* the + /// datagram arrived from that session's exact endpoint, so holding a token + /// is not by itself authority to tear the session down: a close from a + /// foreign endpoint leaves it live, and one close cannot take down a + /// sibling session that the same peer opened separately. A token that is + /// unknown, already closed, or zero — zero is reserved for no-test replies + /// and never issued — matches nothing and leaves the table untouched. + /// + /// Nothing is sent either way, which is why this returns no reply to + /// encode. Protocol version 1 defines no acknowledgement for a client + /// close, so a delivered close is indistinguishable to the client from a + /// lost one, and the freed capacity is immediately reusable because the + /// session table is the only record of it. + fn handle_close(&mut self, peer: SocketAddr, token: u64) { + let owns_session = self + .session(token) + .is_some_and(|session| session.peer() == peer); + if owns_session { + self.sessions.remove(&token); + } + } + /// Encodes a reply this server constructed. Any failure here is internal. fn encode_reply(&self, reply: &OpenReply) -> Result, ServerError> { encode_open_reply(reply, self.config.hmac_key()) diff --git a/crates/irtt-server/src/lib.rs b/crates/irtt-server/src/lib.rs index 5f4fd6d..c5b2e00 100644 --- a/crates/irtt-server/src/lib.rs +++ b/crates/irtt-server/src/lib.rs @@ -31,10 +31,10 @@ //! //! # Current scope //! -//! Open handling and session creation only. Echo processing, client-initiated -//! close, session lifetime and expiry, and the Tokio runtime are separate -//! slices; structurally valid echo and close requests are currently accepted by -//! the admission boundary and then ignored. +//! Open handling, session creation and client-initiated close. Echo +//! processing, server-initiated close, session lifetime and expiry, and the +//! Tokio runtime are separate slices; a structurally valid echo request is +//! currently accepted by the admission boundary and then ignored. #![forbid(unsafe_code)] mod config; diff --git a/crates/irtt-server/src/session.rs b/crates/irtt-server/src/session.rs index b1e40de..328e60c 100644 --- a/crates/irtt-server/src/session.rs +++ b/crates/irtt-server/src/session.rs @@ -29,15 +29,9 @@ impl Session { /// /// A session is bound to this endpoint, not merely to its address: address /// family, address, UDP source port and — for IPv6 — the scope identifier - /// all form part of its identity. Echo and close processing, once - /// implemented, must require both a token match and an exact endpoint match - /// before touching a session. - // - // Read only by tests until then. The endpoint has to be captured at open - // time — it is not recoverable later — so this slice stores state whose - // production consumer is the next one, and the tests prove it is stored - // exactly. - #[allow(dead_code)] + /// all form part of its identity. Close processing requires both a token + /// match and an exact endpoint match before removing a session, and echo + /// processing will require the same. pub(crate) fn peer(&self) -> SocketAddr { self.peer } diff --git a/crates/irtt-server/src/tests/close.rs b/crates/irtt-server/src/tests/close.rs new file mode 100644 index 0000000..e8abab7 --- /dev/null +++ b/crates/irtt-server/src/tests/close.rs @@ -0,0 +1,274 @@ +//! Client-initiated close. +//! +//! A close request that matches a live token *and* arrived from that session's +//! exact endpoint releases the session and is answered with nothing. Every +//! other close — unknown, stale or zero token, foreign endpoint, failed +//! authentication — is the same silence with the session left live, so these +//! tests assert the table as well as the reply. + +use irtt_proto::{encode_request, RequestToEncode}; + +use super::support::{ + client_params, close_request, close_request_with_trailing, core_for, core_with_tokens, + open_request, open_session, other_peer, peer, ScriptedTokens, KEY, OTHER_KEY, +}; +use crate::ServerConfig; + +const TOKEN_A: u64 = 0x0102_0304_0506_0708; +const TOKEN_B: u64 = 0x1112_1314_1516_1718; +const UNKNOWN_TOKEN: u64 = 0x2122_2324_2526_2728; + +#[test] +fn a_valid_close_removes_the_session_and_answers_nothing() { + for hmac_key in [None, Some(KEY)] { + let tokens = ScriptedTokens::new([TOKEN_A]); + let mut core = core_for(hmac_key, tokens.clone()); + let token = open_session(&mut core, peer(), hmac_key); + assert_eq!(core.session_count(), 1); + + // Exactly `None`, not an empty or acknowledging packet: version 1 has + // no close reply, so the client learns nothing about the outcome. + assert_eq!( + core.handle_datagram(peer(), &close_request(token, hmac_key)) + .unwrap(), + None + ); + + assert_eq!(core.session_count(), 0); + assert_eq!(core.session(token), None); + assert_eq!( + tokens.remaining(), + 0, + "a close must not draw from the token source" + ); + } +} + +#[test] +fn a_repeated_close_is_a_stale_token_and_changes_nothing() { + for hmac_key in [None, Some(KEY)] { + let mut core = core_for(hmac_key, ScriptedTokens::new([TOKEN_A])); + let token = open_session(&mut core, peer(), hmac_key); + let close = close_request(token, hmac_key); + + assert_eq!(core.handle_datagram(peer(), &close).unwrap(), None); + assert_eq!(core.handle_datagram(peer(), &close).unwrap(), None); + + assert_eq!(core.session_count(), 0, "nothing may be resurrected"); + assert_eq!(core.session(token), None); + } +} + +#[test] +fn an_echo_after_a_close_is_a_stale_token() { + // Echo processing does not exist yet; the point here is that the token is + // gone, so the request has no session to reach even once it does. + for hmac_key in [None, Some(KEY)] { + let mut core = core_for(hmac_key, ScriptedTokens::new([TOKEN_A])); + let token = open_session(&mut core, peer(), hmac_key); + let params = core.session(token).expect("session must exist").params(); + let echo = encode_request( + RequestToEncode::Echo { + token, + sequence: 0, + params, + payload: &[], + }, + hmac_key, + ) + .unwrap(); + + core.handle_datagram(peer(), &close_request(token, hmac_key)) + .unwrap(); + + assert_eq!(core.handle_datagram(peer(), &echo).unwrap(), None); + assert_eq!(core.session_count(), 0); + assert_eq!(core.session(token), None); + } +} + +#[test] +fn a_close_from_any_other_endpoint_leaves_the_session_live() { + // Endpoint identity is exact: family, address, UDP source port and IPv6 + // scope all count. Possessing the token is not authority to close. + for (bound, foreign) in [ + ("198.51.100.7:41234", "198.51.100.7:41235"), + ("198.51.100.7:41234", "198.51.100.9:41234"), + ("198.51.100.7:41234", "[::ffff:198.51.100.7]:41234"), + ("[2001:db8::1]:2112", "[2001:db8::2]:2112"), + ("[fe80::1%3]:2112", "[fe80::1%4]:2112"), + ] { + let bound = bound.parse().unwrap(); + let foreign = foreign.parse().unwrap(); + let mut core = core_with_tokens(ServerConfig::default(), ScriptedTokens::new([TOKEN_A])); + let token = open_session(&mut core, bound, None); + let session = core.session(token).cloned().expect("session must exist"); + let close = close_request(token, None); + + assert_eq!( + core.handle_datagram(foreign, &close).unwrap(), + None, + "a close from {foreign} must be silent" + ); + assert_eq!(core.session_count(), 1); + assert_eq!( + core.session(token), + Some(&session), + "a close from {foreign} must not remove the session bound to {bound}" + ); + + // The same request from the session's own endpoint does close it, so + // the request itself was never the reason for the silence. + assert_eq!(core.handle_datagram(bound, &close).unwrap(), None); + assert_eq!(core.session_count(), 0); + } +} + +#[test] +fn an_unknown_or_zero_token_closes_nothing() { + // Zero is reserved for no-test replies and is never issued to a session, so + // it can only ever miss the table. + for (name, token) in [("an unknown token", UNKNOWN_TOKEN), ("a zero token", 0)] { + let mut core = core_with_tokens(ServerConfig::default(), ScriptedTokens::new([TOKEN_A])); + let live = open_session(&mut core, peer(), None); + let session = core.session(live).cloned().expect("session must exist"); + + assert_eq!( + core.handle_datagram(peer(), &close_request(token, None)) + .unwrap(), + None, + "{name} must be answered with silence" + ); + assert_eq!(core.session_count(), 1, "{name} must remove nothing"); + assert_eq!(core.session(live), Some(&session)); + } +} + +#[test] +fn a_close_that_fails_authentication_leaves_the_session_live() { + // Authentication runs before the session is looked up, so a close that + // fails it is indistinguishable from one bearing an unknown token — and it + // must not be able to tear down an authenticated session. + let mut core = core_for(Some(KEY), ScriptedTokens::new([TOKEN_A])); + let token = open_session(&mut core, peer(), Some(KEY)); + let session = core.session(token).cloned().expect("session must exist"); + + let mut corrupted = close_request(token, Some(KEY)); + corrupted[5] ^= 0x80; + + for (name, packet) in [ + ( + "a close with no MAC field at all", + close_request(token, None), + ), + ( + "a close signed with a different key", + close_request(token, Some(OTHER_KEY)), + ), + ("a close with one bit flipped in the MAC", corrupted), + ] { + assert_eq!( + core.handle_datagram(peer(), &packet).unwrap(), + None, + "{name} must be answered with silence" + ); + assert_eq!(core.session_count(), 1, "{name} must not remove a session"); + assert_eq!(core.session(token), Some(&session)); + } +} + +#[test] +fn a_non_authenticating_server_ignores_an_authenticated_close() { + let mut core = core_with_tokens(ServerConfig::default(), ScriptedTokens::new([TOKEN_A])); + let token = open_session(&mut core, peer(), None); + let session = core.session(token).cloned().expect("session must exist"); + + assert_eq!( + core.handle_datagram(peer(), &close_request(token, Some(KEY))) + .unwrap(), + None + ); + + assert_eq!(core.session_count(), 1); + assert_eq!(core.session(token), Some(&session)); +} + +#[test] +fn trailing_bytes_after_the_token_do_not_prevent_the_close() { + for hmac_key in [None, Some(KEY)] { + let mut core = core_for(hmac_key, ScriptedTokens::new([TOKEN_A])); + let token = open_session(&mut core, peer(), hmac_key); + + let close = close_request_with_trailing(token, &[0xde, 0xad, 0xbe, 0xef], hmac_key); + + assert_eq!(core.handle_datagram(peer(), &close).unwrap(), None); + assert_eq!(core.session_count(), 0); + } +} + +#[test] +fn a_close_removes_only_the_session_it_names() { + // Repeated opens from one endpoint are independent sessions, so a close + // must be resolved by token, never by endpoint. + let mut core = core_with_tokens( + ServerConfig::default(), + ScriptedTokens::new([TOKEN_A, TOKEN_B]), + ); + let first = open_session(&mut core, peer(), None); + let second = open_session(&mut core, peer(), None); + let survivor = core.session(second).cloned().expect("session must exist"); + assert_eq!(core.session_count(), 2); + + assert_eq!( + core.handle_datagram(peer(), &close_request(first, None)) + .unwrap(), + None + ); + assert_eq!(core.session_count(), 1, "only one session may be released"); + assert_eq!(core.session(first), None); + assert_eq!(core.session(second), Some(&survivor)); + + assert_eq!( + core.handle_datagram(peer(), &close_request(second, None)) + .unwrap(), + None + ); + assert_eq!(core.session_count(), 0); +} + +#[test] +fn a_close_frees_session_capacity_immediately() { + // The session table is the only record of how full the server is, so a + // removal is reclaimed at once with no separate counter to drift. + let tokens = ScriptedTokens::new([TOKEN_A, TOKEN_B]); + let mut core = core_with_tokens(ServerConfig::default().with_max_sessions(1), tokens.clone()); + let request = open_request(&client_params(), None); + + let first = open_session(&mut core, peer(), None); + assert_eq!( + core.handle_datagram(other_peer(), &request).unwrap(), + None, + "the bound is reached, so the second open is refused" + ); + assert_eq!(core.session_count(), 1); + assert_eq!( + tokens.remaining(), + 1, + "a refused open must not commit a token" + ); + + core.handle_datagram(peer(), &close_request(first, None)) + .unwrap(); + assert_eq!(core.session_count(), 0); + assert_eq!( + tokens.remaining(), + 1, + "a close must not draw from the token source" + ); + + let second = open_session(&mut core, other_peer(), None); + assert_eq!(second, TOKEN_B); + assert_ne!(second, 0); + assert_eq!(core.session_count(), 1); + assert_eq!(core.session(second).unwrap().peer(), other_peer()); +} diff --git a/crates/irtt-server/src/tests/mod.rs b/crates/irtt-server/src/tests/mod.rs index d820706..96687ed 100644 --- a/crates/irtt-server/src/tests/mod.rs +++ b/crates/irtt-server/src/tests/mod.rs @@ -8,6 +8,7 @@ //! Tokens come from a scripted source so session identity, collisions and //! allocation failure are all assertable. +mod close; mod hmac; mod no_test; mod open; diff --git a/crates/irtt-server/src/tests/support.rs b/crates/irtt-server/src/tests/support.rs index bf35c19..73c8d66 100644 --- a/crates/irtt-server/src/tests/support.rs +++ b/crates/irtt-server/src/tests/support.rs @@ -72,6 +72,31 @@ pub(crate) fn core_with_tokens(config: ServerConfig, tokens: ScriptedTokens) -> ServerCore::with_token_source(config, Box::new(tokens)) } +/// An otherwise default core that authenticates iff `hmac_key` is set. +/// +/// Most behavior has to hold identically with and without authentication, so +/// tests loop over both and build the core from the key. +pub(crate) fn core_for(hmac_key: Option<&[u8]>, tokens: ScriptedTokens) -> ServerCore { + let mut config = ServerConfig::default(); + if let Some(key) = hmac_key { + config = config.with_hmac_key(key); + } + core_with_tokens(config, tokens) +} + +/// Opens one ordinary session from `endpoint` and returns its token. +pub(crate) fn open_session( + core: &mut ServerCore, + endpoint: SocketAddr, + hmac_key: Option<&[u8]>, +) -> u64 { + let packet = core + .handle_datagram(endpoint, &open_request(&client_params(), hmac_key)) + .unwrap() + .expect("the session-creating open must be answered"); + expect_normal_open_reply(&packet, hmac_key).token +} + /// Encodes a normal open request carrying `params`. pub(crate) fn open_request(params: &Params, hmac_key: Option<&[u8]>) -> Vec { encode_request( @@ -96,6 +121,29 @@ pub(crate) fn no_test_request(params: &Params, hmac_key: Option<&[u8]>) -> Vec) -> Vec { + encode_request(RequestToEncode::Close { token }, hmac_key).unwrap() +} + +/// Encodes a close request carrying arbitrary bytes after the token. +/// +/// The decoder tolerates them, and this is the shape that proves it. The MAC +/// covers the whole datagram, so it is recomputed over the final form rather +/// than left behind by the encoder's shorter packet. +pub(crate) fn close_request_with_trailing( + token: u64, + trailing: &[u8], + hmac_key: Option<&[u8]>, +) -> Vec { + let mut packet = close_request(token, hmac_key); + packet.extend_from_slice(trailing); + if let Some(key) = hmac_key { + irtt_proto::compute_hmac_in_place(key, &mut packet, hmac_field_offset()).unwrap(); + } + packet +} + /// Builds an open request around a raw parameter payload. /// /// Used for payloads a compliant encoder cannot produce, such as a truncated diff --git a/crates/irtt-server/src/tests/unsupported.rs b/crates/irtt-server/src/tests/unsupported.rs index 4036ca2..4b69d46 100644 --- a/crates/irtt-server/src/tests/unsupported.rs +++ b/crates/irtt-server/src/tests/unsupported.rs @@ -1,25 +1,20 @@ //! Requests this slice deliberately does not implement. //! -//! Echo and close are recognized by the admission boundary and then ignored. -//! These tests exist to keep the scope boundary honest: a half-implemented -//! lifecycle must not appear by accident before the slices that own it. +//! Echo is recognized by the admission boundary and then ignored. These tests +//! exist to keep the scope boundary honest: receive state must not appear by +//! accident before the slice that owns it. use irtt_proto::{encode_request, RequestToEncode}; -use super::support::{client_params, core_with_tokens, open_request, peer, ScriptedTokens, KEY}; -use crate::ServerConfig; +use super::support::{ + client_params, core_for, open_request, open_session, peer, ScriptedTokens, KEY, +}; const TOKEN_A: u64 = 0x0102_0304_0506_0708; fn open_one_session(hmac_key: Option<&[u8]>) -> crate::ServerCore { - let mut config = ServerConfig::default(); - if let Some(key) = hmac_key { - config = config.with_hmac_key(key); - } - let mut core = core_with_tokens(config, ScriptedTokens::new([TOKEN_A])); - core.handle_datagram(peer(), &open_request(&client_params(), hmac_key)) - .unwrap() - .expect("the session-creating open must succeed"); + let mut core = core_for(hmac_key, ScriptedTokens::new([TOKEN_A])); + open_session(&mut core, peer(), hmac_key); core } @@ -46,20 +41,6 @@ fn a_valid_echo_is_neither_answered_nor_counted_yet() { } } -#[test] -fn a_valid_close_does_not_remove_the_session_yet() { - for hmac_key in [None, Some(KEY)] { - let mut core = open_one_session(hmac_key); - let session = core.session(TOKEN_A).cloned().expect("session must exist"); - - let close = encode_request(RequestToEncode::Close { token: TOKEN_A }, hmac_key).unwrap(); - - assert_eq!(core.handle_datagram(peer(), &close).unwrap(), None); - assert_eq!(core.session_count(), 1); - assert_eq!(core.session(TOKEN_A), Some(&session)); - } -} - #[test] fn structurally_invalid_datagrams_are_discarded() { let mut core = open_one_session(None);