diff --git a/src/error.rs b/src/error.rs index 9a6851b..f21d202 100644 --- a/src/error.rs +++ b/src/error.rs @@ -140,6 +140,14 @@ pub enum AdapterError { /// half-configured adapter — a construction where the fabric could /// mint listens the door cannot serve is worse than no adapter. /// The string carries the registration failure detail. + /// + /// This stays the construction-time class: the wire-time sibling + /// (`w4-router-collision`, the `ListenRouter::register` + /// token-collision refusal) is a request-time **decline + /// disposition** (`RequestDenied`, reason `token-collision`) — + /// a global-request hook's `Err` (this taxonomy's only route into + /// the request surface) would kill the whole russh session, and + /// wire decline reasons are never this taxonomy. #[error("ssh server construction refused (listen registration): {0}")] ListenRegistration(String), diff --git a/src/handler.rs b/src/handler.rs index f0013c2..66ddb55 100644 --- a/src/handler.rs +++ b/src/handler.rs @@ -1293,8 +1293,13 @@ impl russh::server::Handler for SessionHandler { /// bounded by `LISTEN_DIAL_TIMEOUT`, so the stall is bounded). The /// reply's `bound` port writes the out-param (`port = 0` → the /// actually-bound number, sshd parity); the route registers into the -/// shared router (session-scope: the owning session's handle + its -/// channel-cap `Arc`) and the pending listen into the session's +/// shared router — the register is the collision pre-check +/// (`w4-router-collision`): a mint on an already-held token refuses +/// fail-closed (`token-collision`, the minted listener torn down, the +/// cap released — a collided mint can never steal a live route; the +/// decline rides `Ok(false)`, never the error taxonomy, since a +/// global-request hook's `Err` would kill the whole session) — and +/// the pending listen into the session's /// packet-loop-only map. `Ok(true)`'s answer is the only success /// shape; every decline path answers the vanilla client's /// `RequestDenied` via `Ok(false)`. @@ -1359,20 +1364,25 @@ async fn listen_request_dispatch( return Ok(false); } }; + let token = minted.token; + let route = tunnel_map::ListenRoute { + echo, + substrate, + handle: Arc::new(session.handle()), + cap: Arc::clone(&handler.channel_cap), + cap_value: handler.serve.limits.max_concurrent_channels, + }; + if let Err(_collision) = router.register(token, route) { + handler.listener_cap.release(); + observe::decline_global_request(row, "token-collision"); + serve::spawn_serving_path(async move { + minted.listener.close().await; + }); + return Ok(false); + } if let (Some(port), Some(bound_port)) = (port, tunnel_map::bound_port(&bound)) { *port = bound_port; } - let token = minted.token; - router.register( - token, - tunnel_map::ListenRoute { - echo, - substrate, - handle: Arc::new(session.handle()), - cap: Arc::clone(&handler.channel_cap), - cap_value: handler.serve.limits.max_concurrent_channels, - }, - ); handler.listeners.insert( token, tunnel_map::PendingListen { diff --git a/src/tunnel_map.rs b/src/tunnel_map.rs index 4573227..ab6e65d 100644 --- a/src/tunnel_map.rs +++ b/src/tunnel_map.rs @@ -337,7 +337,9 @@ pub(crate) enum RouteEcho { /// bounded bookkeeping's spine: entries are minted exactly at accepted /// listens (the per-session listener cap gated), taken at the cancel /// forms, and removed at the owning session's teardown (the handler's -/// `Drop`). Arrivals that find no route are the correlation-miss +/// `Drop`). A mint on an already-held token is refused fail-closed +/// ([`ListenRouter::register`], `w4-router-collision`). Arrivals +/// that find no route are the correlation-miss /// class — teardown-at-birth, never a half-open SSH channel. /// /// The map is behind a `parking_lot::Mutex` because arrivals arrive on @@ -348,9 +350,40 @@ pub(crate) struct ListenRouter { } impl ListenRouter { - /// Minted-entry registration (the listen-acceptance step). - pub(crate) fn register(&self, token: u32, route: ListenRoute) { - self.routes.lock().insert(token, route); + /// Minted-entry registration (the listen-acceptance step), + /// fail-closed on token collision (the wave-3 gate's booked item + /// 4, `w4-router-collision` — the same posture the same gate + /// landed inline for a duplicate `tunnel/forwarded` + /// registration): safety rests on the fabric's ADR-008 mint + /// discipline upstream, and this defends in depth against a + /// collided mint — a `register` on an already-held token returns + /// `Err` and **never replaces** the held route, so a colliding + /// mint can never steal a live adapter's arrival routes. The + /// caller declines the listen (the fabric listener is then torn + /// down — a refused registration never left a half state). + /// + /// Error surface (decided, `w4-router-collision`): the refusal + /// rides [`String`]-carried detail surfaced as the request-time + /// **decline disposition** (`RequestDenied`, reason + /// `token-collision`), not a distinct [`AdapterError`](crate::AdapterError) + /// variant — a global-request hook's `Err` would kill the whole + /// russh session (russh 0.65 answers the decline from the hook's + /// `Ok(false)`, and `Err` propagates out of the packet loop), and + /// the taxonomy's rule holds that wire decline reasons are never + /// the error taxonomy. `AdapterError::ListenRegistration` honestly + /// carries the construction-time class it always did; the shared + /// fail-closed registration posture is the reuse, no variant + /// needed. + pub(crate) fn register(&self, token: u32, route: ListenRoute) -> Result<(), String> { + let mut routes = self.routes.lock(); + if routes.contains_key(&token) { + return Err(format!( + "listen token {token} is already routed — a collided \ + mint may not replace a live adapter's route" + )); + } + routes.insert(token, route); + Ok(()) } /// The per-arrival lookup — cloned out, never taken: one route diff --git a/tasks/implementation/w4-router-collision.md b/tasks/implementation/w4-router-collision.md index 4cd2878..93417c6 100644 --- a/tasks/implementation/w4-router-collision.md +++ b/tasks/implementation/w4-router-collision.md @@ -1,7 +1,7 @@ --- id: w4-router-collision name: Wave 4 — ListenRouter token-collision pre-check (wave-3 gate booked item 4) -status: pending +status: completed depends_on: [w3-tunnel-listen] scope: single risk: low @@ -37,11 +37,11 @@ the silent-replace). ## Acceptance Criteria -- [ ] `ListenRouter::register` refuses on token collision (fail-closed, +- [x] `ListenRouter::register` refuses on token collision (fail-closed, no silent replace); the error surface decided + recorded -- [ ] Wire row(s): the second register refuses, the first route still +- [x] Wire row(s): the second register refuses, the first route still serves; existing rows green -- [ ] `cargo test` (+ per-feature + `--all-features`), clippy ×2 +- [x] `cargo test` (+ per-feature + `--all-features`), clippy ×2 (`-D warnings`), fmt, `cargo doc --no-deps` pass ## References @@ -56,8 +56,63 @@ the silent-replace). ## Notes -> To be filled by implementation agent +### Implementation (2026-10-29) + +- **`ListenRouter::register` (src/tunnel_map.rs)** now returns + `Result<(), String>`: a register on an already-held token returns + `Err` and **never replaces** the held route — a collided mint can + never steal a live adapter's arrival routes. The refusal detail + carries the token + the steal posture. +- **Error surface (decided + recorded):** the refusal surfaces as the + request-time **decline disposition** (`RequestDenied`, reason + `token-collision`), not a distinct `AdapterError` variant. The + deciding fact: a global-request hook's `Err` is the taxonomy's only + route into the request surface, and russh 0.65 propagates it with + `?` out of the packet loop — killing the whole session — while the + vanilla contract for a refused listen is `RequestDenied`. + `AdapterError::ListenRegistration` honestly carries the + construction-time class it always did (the doc in src/error.rs + records the wire-time sibling); the reuse is the shared fail-closed + registration posture, no new variant needed. +- **`listen_request_dispatch` (src/handler.rs)**: the register moved + **before** the `&mut u32` out-param write (a refused listen never + writes the bound port); on refusal the path releases the listener + cap, emits `decline_global_request(row, "token-collision")` (the + uniform vocabulary, closed — no dynamic fields), and tears the + minted fabric listener down immediately in a spawned task + (`TunnelListener::close` — the cancel path's mechanism; a refused + registration never left a half state). +- **Wire row** (`tests/tunnel_mapping.rs` + `a_collided_mint_is_refused_and_never_steals_the_live_route`): the + fabric double gained a mint-strategy construct + (`wire_fabric_with_listen_mint` + `MintStrategy::WithCollisions` — + the first N mints return a fixed token, then the ADR-008 counter + continues; the test forces exactly what the fabric's discipline + prevents). The row pins, against a 2-count listener ceiling (the + ceiling that admits the collided mint past the cap — the cap row's + shape would decline at the cap before the register — and makes the + release observable): the first listen registers normally, the + colliding mint's listen answers `RequestDenied`, its bound endpoint + never serves again (teardown observable — the cancel row's + bounded-retry shape), the listener count was released (a fresh + `port = 0` mint is admitted and registers — and its success reply + proves the shape), and the first route **still serves** end to end + (an arrival opens the echoed `forwarded-tcpip` and the bytes ride) + — the steal is impossible, row-pinned. ## Summary -> To be filled on completion \ No newline at end of file +**Completed.** The `ListenRouter` token-collision pre-check landed +fail-closed (register refuses, never replaces) with the error-surface +decision recorded: the wire-time collision is the decline disposition +(`token-collision` / `RequestDenied`), not the taxonomy — an `Err` +would kill the whole russh session — while `AdapterError:: +ListenRegistration` keeps its construction-time class. The refusal +path is the full fail-closed shape (cap release, uniform decline +event, immediate fabric teardown — no half state). Row-pinned through +a mint-strategy construct on the fabric double: second register +refuses, first route still serves end to end, refused endpoint never +serves, count released. Full verification set green: cargo test +(default + git/session/tunnel/sftp + `--all-features`), clippy ×2 +`-D warnings`, fmt, `cargo doc --no-deps`; existing arrival/cancel/ +teardown rows unchanged and green. \ No newline at end of file diff --git a/tests/tunnel_mapping.rs b/tests/tunnel_mapping.rs index 57d5bd5..3390537 100644 --- a/tests/tunnel_mapping.rs +++ b/tests/tunnel_mapping.rs @@ -235,6 +235,19 @@ mod serving { } } + /// The double's establisher mint discipline: `Counter` is the + /// ADR-008 shape the real fabric carries (a non-zero-seeded + /// counter, never 0, never re-minted); `WithCollisions` is the + /// row-pinned collision construct — the first `collisions` mints + /// return a fixed `token` (the test forces what the fabric's + /// discipline prevents), then the counter continues from where it + /// left. + #[derive(Clone, Copy)] + pub(super) enum MintStrategy { + Counter, + WithCollisions { token: u32, collisions: usize }, + } + /// The dynamic-bind producer's establisher (the double's shape of /// the `bindfirst_establisher` recipe): parse /// `TunnelParams {resource, substrate}`, bind via the injected @@ -245,19 +258,31 @@ mod serving { fn dynamic_bindfirst_establisher( bind: BindFn, minted: Arc>, + mint: MintStrategy, ) -> alkcall::channels::operations::OpenEstablisher { let counter = Arc::new(AtomicU32::new(1)); + let (collided_token, remaining_collisions) = match mint { + MintStrategy::Counter => (0, 0usize), + MintStrategy::WithCollisions { token, collisions } => (token, collisions), + }; + let collisions = Arc::new(AtomicUsize::new(remaining_collisions)); Arc::new(move |input: serde_json::Value, _auth| { let bind = Arc::clone(&bind); let minted = Arc::clone(&minted); let counter = Arc::clone(&counter); + let collisions = Arc::clone(&collisions); Box::pin(async move { let params: alktunnels::params::TunnelParams = serde_json::from_value(input.clone()).map_err(|e| { TunnelEstablishError::HandlerError(format!("invalid params: {e}")) })?; - let token = counter.fetch_add(1, Ordering::Relaxed); - let token = if token == 0 { 1 } else { token }; + let token = if collided_token != 0 && collisions.load(Ordering::Relaxed) > 0 { + collisions.fetch_sub(1, Ordering::Relaxed); + collided_token + } else { + let next = counter.fetch_add(1, Ordering::Relaxed); + if next == 0 { 1 } else { next } + }; let _ = minted.send(token); let BoundListener { listener, bound } = bind(params.substrate, ¶ms.resource).await?; @@ -375,6 +400,16 @@ mod serving { }) } + /// Wires the leg-2 fabric double with the ADR-008 mint discipline + /// (the rows' default mint). + pub(super) async fn wire_fabric_with_listen( + door_scoped_caller: bool, + arrival_scoped: bool, + ) -> FabricDouble { + wire_fabric_with_listen_mint(door_scoped_caller, arrival_scoped, MintStrategy::Counter) + .await + } + /// Wires the leg-2 fabric double (leg 1 unchanged underneath — the /// dial seam + direct openable stay exactly `wire_fabric`'s /// composition, against a leg-1 fail dial the -R rows never @@ -382,10 +417,12 @@ mod serving { /// serving identity (what gates the door's listen op's ACL there); /// `arrival_scoped` composes the door-side `ServingConfig.identity` /// with or without the `tunnel:forwarded` scope (the composed-gate - /// row's negative leg). - pub(super) async fn wire_fabric_with_listen( + /// row's negative leg). `mint` is the establisher's mint + /// discipline ([`MintStrategy`]) — the collision rows' vehicle. + pub(super) async fn wire_fabric_with_listen_mint( door_scoped_caller: bool, arrival_scoped: bool, + mint: MintStrategy, ) -> FabricDouble { // The two sides' registries are independent (the fabric's own // dispatch registry; and the DOOR-side shared one the row @@ -495,6 +532,7 @@ mod serving { Some(dynamic_bindfirst_establisher( real_bind(), Arc::clone(&minted_tx), + mint, )), make_bindfirst_accept_handler(real_accept_loop(), open_forwarded), ®istry, @@ -1330,6 +1368,124 @@ mod serving { session.unmount().await; } + /// - *Contract stamp:* the wave-3 gate's booked finding item 4 + /// (`w4-router-collision`): the `ListenRouter` token-collision + /// pre-check. The fabric's ADR-008 mint discipline is the + /// upstream guarantee the door defends in depth against: the + /// test forces what the discipline prevents (the double + /// re-mints a live token), and the second listen **refuses + /// fail-closed** — `RequestDenied`, its minted fabric listener + /// torn down immediately (the refused registration never left a + /// half state), the listener cap released (the next mint is + /// admitted against the same 1-count ceiling) — and the **first + /// route still serves**: the collided mint can never steal a + /// live adapter's arrival routes. + #[tokio::test] + async fn a_collided_mint_is_refused_and_never_steals_the_live_route() { + let double = wire_fabric_with_listen_mint( + true, + true, + MintStrategy::WithCollisions { + token: 7, + collisions: 2, + }, + ) + .await; + // The ceiling of 2 both admits the collided mint past the + // listener cap (so the refusal is the route register, not the + // cap — the cap row's shape) and makes the refusal's cap + // release observable: without the release, the third mint + // would be cap-exceeded. + let limits = Limits { + max_concurrent_channels: 2, + ..Limits::default() + }; + let (session, mut opens) = listen_session(&double, limits).await; + + let port_a = pick_port().await; + tcpip_forward(&session.client, "127.0.0.1", u32::from(port_a)) + .await + .expect("the first listen registers its route normally"); + + // The second mint deliberately re-mints the live token: the + // route registration refuses, the listen declines fail-closed. + let port_b = pick_port().await; + let refused = tcpip_forward(&session.client, "127.0.0.1", u32::from(port_b)) + .await + .expect_err("the collided mint's listen refuses"); + assert!( + matches!(refused, Error::RequestDenied), + "expected RequestDenied, got {refused:?}" + ); + + // The collided listen's minted listener was torn down + // immediately: its bound endpoint never serves (bounded retry, + // the cancel row's teardown-cascade shape). + tokio::time::timeout(Duration::from_secs(10), async { + loop { + match tokio::net::TcpStream::connect(("127.0.0.1", port_b)).await { + Err(_) => return, + Ok(stream) => drop(stream), + } + tokio::time::sleep(Duration::from_millis(100)).await; + } + }) + .await + .expect("the refused listen's fabric listener was torn down"); + + // The listener count was released at the refusal: against the + // same 2-count ceiling, the next mint (a fresh token — the + // collision was two mints on one listen, not a broken fabric) + // is admitted and registers normally (a `0` mint's success + // reply carries the actually-bound number — the sshd parity). + let third = tcpip_forward(&session.client, "127.0.0.1", 0) + .await + .expect("the collision released the listener count"); + assert_ne!(third, 0); + + // The first route still serves — the steal is impossible: an + // arrival onto the live route's endpoint opens the + // forwarded-tcpip channel and the bytes ride the pump. + let mut arrival = tokio::net::TcpStream::connect(("127.0.0.1", port_a)) + .await + .expect("the live route's listener still accepts arrivals"); + let request = b"still-served"; + arrival + .write_all(request) + .await + .expect("the arrival's bytes ride toward the door"); + let open = tokio::time::timeout(Duration::from_secs(15), opens.recv()) + .await + .expect("the live route's arrival open arrives in time") + .expect("observed"); + let ForwardedOpen::Tcpip { mut channel, .. } = open else { + panic!("the live route's arrival opened something else"); + }; + let mut seen: Vec = Vec::new(); + tokio::time::timeout(Duration::from_secs(15), async { + while seen.len() < request.len() { + match channel.wait().await { + Some(ChannelMsg::Data { data }) => seen.extend_from_slice(&data), + Some(_) => continue, + None => panic!("the live route's channel ended before the request rode"), + } + } + }) + .await + .expect("the live route's pump rides in time"); + assert_eq!(seen, request, "the collided mint stole nothing"); + + // Nothing rode the acceptor beyond the live route's served + // arrival (the refused mint's listener was torn down at birth + // — its arrivals could not exist). + tokio::time::timeout(Duration::from_millis(500), opens.recv()) + .await + .expect_err("no arrival may follow the refused mint"); + assert!(opens.is_empty()); + + session.unmount().await; + } + /// - *Contract stamp:* the arrival pump's correlation-miss class /// (`w3-tunnel-listen` — teardown-at-birth, never a half-open SSH /// channel): an arrival whose `listen` token has no route