diff --git a/docs/reviews/003-assembly-layer-implementation-review.md b/docs/reviews/003-assembly-layer-implementation-review.md new file mode 100644 index 0000000..4e7f936 --- /dev/null +++ b/docs/reviews/003-assembly-layer-implementation-review.md @@ -0,0 +1,682 @@ +--- +status: open +last_updated: 2026-09-26 (the review written against the post-gate tree + `04c76e3` + the review's inline fixes; F-1/F-2 fixed in this change) +reviewed_artifacts: + - src/door.rs + - src/local/mod.rs + - src/local/producer_door.rs + - src/composed/mod.rs + - src/consumer.rs (the batch-2 additions: TokenClient + the + *_with_token twins) + - src/producer.rs (spot-check: the gate/builder surface) + - src/rfc1928/ (spot-check: the cred-bearing door's forked surface) + - docs/architecture/ (overview, wire, producer, consumer, rfc1928, + README, decisions 003–014) + - tests/ (all batch-2 suites read; coverage attributed) +tool: manual source read + cargo test/clippy/fmt/doc/wasm/publish-dry-run + cargo-llvm-cov + alktunnels 0.2.0 surface cross-checks +reviewer: general implementation review (correctness + security + smells + coverage) +scope_note: the batch-2 assembly layer only — the five new modules plus + the batch-2 additions to consumer.rs/producer.rs; the batch-1 core + was reviewed by review 002 and is not re-litigated here +--- + +# Review 003 — The Assembly-Layer Implementation (Post-Landing Full Review) + +## Purpose + +General review of the batch-2 assembly layer after landing — the +eight-task group (`tasks/implementation/{local-*,door-*,composed-*, +assembly-integration-suite,review-assembly-layer}.md`, commits +`a867f0c..04c76e3`). The narrow review +(`tasks/implementation/review-assembly-layer.md`) checked the eight +gate criteria (never-binds, seam contracts, door fidelity, graduation +shape, conventions, feature hygiene, POC fidelity, docs sync) and +fixed R-1/R-2 inline; this one asks the four questions that review +did not, following review 002's template: + +1. **Correctness** — hazards in the landed door/provider behavior + itself: teardown races, pump-lifetime paths, protocol-state + corruption, error-shape fidelity at the door boundary. +2. **Security** — resource exhaustion on the *kernel-socket* paths + the batch-2 code newly opens (doors, producer backend), gate + fidelity, panic surfaces, log/credential handling. +3. **Code smells** — convention compliance plus the structural smells + an 8-task implementation round leaves behind (dead surface, + duplication, doc drift, test-side posture). +4. **Coverage** (`cargo-llvm-cov`) — important gaps, attributed to + specific behaviors and their risk. + +Method note: line-level over the five batch-2 modules + the batch-2 +diff surface of `consumer.rs`/`producer.rs`, with cross-checks against +alktunnels 0.2.0 (`make_forwarded_op_handler`, `ForwardedHandler`, +`TunnelListener`, `TakenHalves`) and the ADR set 003–014. The RFC +1928 core and the protocol-layer pump are re-read only where batch 2 +calls into them. + +## Verification Baseline + +All commands on the reviewed tree (post-fix; the review change itself): + +- `cargo test`: **375 green** (default; the batch-2 suites are + feature-gated out of the default build). +- `cargo test --all-features`: **620 green** (up 5 from the gate's + 615 — the review's pinning test + the UDP-door exit change). +- `cargo clippy --all-targets --all-features -- -D warnings`: clean. +- `cargo fmt --check`: clean. +- `cargo doc --no-deps`: clean (0 warnings). +- `cargo publish --dry-run --allow-dirty`: completes. +- Wasm checks: default + `--features composed` clean (check + clippy); + `local` on wasm fails with exactly the expected mio unsupported-wasm + error (the alktty posture, observed at the gate and re-verified). +- `cargo llvm-cov --all-features`: **93.68% lines / 91.01% functions / + 92.97% regions** (6485 covered / 410 missed). + +### Coverage breakdown + +| Module | Lines | Functions | Regions | +|---|---:|---:|---:| +| composed/mod.rs | 77.32% | 65.31% | 80.45% | +| consumer.rs | 95.25% | 93.60% | 93.88% | +| door.rs | 81.45% | 81.08% | 79.05% | +| egress.rs | 99.67% | 100.00% | 99.70% | +| error.rs | 100.00% | 100.00% | 97.48% | +| local/mod.rs | 84.91% | 74.29% | 86.94% | +| local/producer_door.rs | 76.98% | 84.21% | 77.46% | +| params.rs | 100.00% | 100.00% | 100.00% | +| producer.rs | 95.70% | 91.83% | 96.74% | +| rfc1928/ (all five) | 96.5–100% | 95.65–100% | 78.6–99.6% | +| wire.rs | 96.58% | 97.06% | 97.19% | +| **TOTAL** | **93.68%** | **91.01%** | **92.97%** | + +Healthy overall; the batch-2 modules carry the lower numbers (D-1, +D-2, N-8 attribute them). The protocol core's residue is unchanged +from review 002's attribution; the assembly layer's gaps are the +findings below. + +## Summary Statistics + +| Severity | Count | IDs | +|----------|------:|-----| +| Critical | 0 | — | +| Major | 2 | F-1, F-2 | +| Minor | 4 | S-1, D-1, D-2, D-3 | +| Observation | 8 | N-1..N-8 | + +No critical findings. The two majors (F-1, F-2) are door-boundary +fidelity/teardown corrections, both fixed in the review change: +F-1 is the cred-bearing door silently swallowing the in-band +refusals' mapped reply codes (a vanilla client got a bare EOF where +ADR-007 pins a reply-code-shaped refusal); F-2 is the UDP door's +fatal-I/O swallow (`serve_on` returned `Ok(())` on a relay-socket +failure the docs call fatal). One coverage finding (D-2) is the +batch-2 analogue of review 002's D-2 (the spec'd surface unexercised); +the rest are small. + +Prefixes follow the review-002 legend: `F` (correctness, fixed +inline — continuing 002's `C` family with the batch's review-fixed +disposition), `S` (security posture), `D` (dead surface/coverage), `N` +(observations/notes/smells). + +--- + +# Part A — Findings + +## F-1 [major, fixed] — The cred-bearing door swallowed every in-band refusal's mapped reply code: a vanilla client got a bare EOF where the RFC (and ADR-007's failure mapping) promises a reply-code-shaped refusal + +**Files**: `src/door.rs` (`cred_bearing`'s CONNECT/BIND session-open +error arms + `bind_quiet_and_pipe`'s accept-stage error arm; +pre-fix state at git `04c76e3`) + +**Problem**: ADR-007 pins the failure mapping as RFC-faithful *end to +end*: factory failure → reply#1's mapped code; accept-stage failure → +reply#2's mapped code. The producer honors this in-band — but the +cred-bearing door sat *between* the in-band conversation and the +vanilla client and dropped the mapped code on the floor at three +sites: + +1. `connect_with_token` refused (dial refused per target policy — + the in-band CONNECT reply carried 0x05/0x02/0x03/…) → the door + returned `DoorExit::Open(err.to_string())` and the vanilla + connection closed with **no reply at all**. +2. `bind_with_token` refused (the in-band BIND command gate's 0x02 — + an identity with `socks5:open` but not `socks5:bind`) → same: bare + EOF where a vanilla client expects reply#1 `ConnectionNotAllowed`. +3. `await_inbound()`'s accept-stage failure (`Err`) — the quiet + wait's accept refused per policy — the door closed the connection + with no reply#2. + +The producer-side in-band paths (the pump handler, `serve_socks5_over`) +write the RFC reply before returning the session error; the door — +the one surface whose whole job is RFC codec translation — discarded +it. The observable: `curl --proxy socks5://user:pass@host` against a +target-policy refusal hangs-then-drops instead of printing +"proxy CONNECT aborted" with a SOCKS error — and, worse, a BIND +command-gate refusal (the ADR-006 security boundary working as +designed) was indistinguishable from a crashed proxy. The door's own +module doc promises "the RFC codec lives only here" and "zero protocol +translation" of *payloads* — refusal re-encoding is the codec's job. + +**Fix (landed in this review change)**: a `write_refusal_reply` helper +(the door's boundary codec, best-effort — a failed write means the +client is gone either way) re-encodes `SocksSessionError::Rfc( +ReplyError)`-shaped refusals to the vanilla client with the RFC's +placeholder BND.ADDR (`0.0.0.0:0`, the same form `reply_error` uses), +at all three sites (CONNECT open, BIND open, the accept-stage `Err`). +Non-reply-code shapes (open/transport/I-O) stay a bare close (nothing +RFC-shaped to encode). Pinned by `cred_door_in_band_refusal_ +surfaces_the_reply_code` (a `socks5:open`-only credential BINDs → +reply#1 carries 0x02 + the placeholder address, within a timeout — +the old shape failed the test by never writing a reply). + +**Lift**: closes the last wire-fidelity gap between the in-band and +vanilla surfaces. Every other door path re-encodes its reply +(`pipe_connect_plane`, `pipe_bind_plane`, the UDP door's codec); the +refusal paths were the asymmetric ones. + +--- + +## F-2 [major, fixed] — `UdpFrontDoor::serve_on` returned `Ok(())` on a fatal relay-socket I/O failure, against its own contract + +**Files**: `src/door.rs` (`UdpFrontDoor::serve_on`'s final `Ok(())`, +pre-fix at `04c76e3`); doc: "Runs until the association ends (the +in-band channel EOF, the client idle bound, **or a fatal I/O +failure**)" + +**Problem**: the two-pump `select!` resolves with a `UdpDoorExit`; an +`Io` exit (a real relay-socket failure — `recv_from` erroring on a +non-idle socket) is documented as one of the run's fatal outcomes, +but the method mapped *every* exit to `Ok(())`. The caller +(assembly wiring, the `serve` wrapper) cannot distinguish "the +association ended cleanly" from "the relay socket died" — the exact +distinction the doc's contract claims to carry. The TCP door has the +same return shape but its per-accept isolation posture makes +per-connection I/O failure genuinely non-fatal (the loop continues); +the UDP door is one association per call — an `Io` exit there *is* +the call's outcome. Latent: nothing in the suite exercised a +relay-socket error, so the swallow passed every test. + +**Fix (landed in this review change)**: the exit match surfaces +`UdpDoorExit::Io` as `Err(err)`; the idle bound, the association end, +and a session error stay clean `Ok(())` (the association's own ends — +each already logs at debug). All UDP-door tests unchanged and green +(the idle-bound and teardown tests assert clean exits, which the +match preserves). + +**Lift**: the caller-visible contract matches the doc; an assembly +monitoring `serve()` can now distinguish fatal from graceful. + +--- + +## S-1 [minor] — The UDP door's reply path trusts the recorded client address for the association's whole lifetime: a spoofed first datagram hijacks every subsequent reply + +**Files**: `src/door.rs` (`UdpFrontDoor::serve_on`'s `client_addr` +capture — the first inbound datagram's source wins permanently) + +**Problem**: the door serves one association per relay socket and +"learns" the client's source address from the first datagram, using it +for every reply. Two residual shapes: + +- An off-path spoofed datagram arriving first (before the real + client's) pins the replies to the spoofer's address. For the + documented posture this is near-moot: the relay socket is bound + loopback by the builder's posture (a wildcard bind is the caller's + explicit variant), and an attacker who can inject datagrams into the + loopback path owns the host anyway. But a deployment that *does* + spell the wildcard bind gets a one-datagram reply-hijack primitive + per association. +- A client that changes source port mid-association (NAT rebinding) + silently stops receiving replies (replies go to the learned + address). + +**Recommendation**: accepted-uncovered posture note rather than a fix +— RFC 1928's own ASSOCIATE has the same per-association peer +expectation (the RFC's relay is expected to drop datagrams from +non-expected peers; learning-on-first is the v1 shape the POC +validated). One doc line on `serve_on` naming the residual ("the +client's source address is learned from the first inbound datagram +and pinned for the association's lifetime — a wildcard-bind deployment +accepts the off-path-first-datagram shape, the RFC's own posture") +so the next reader does not mistake it for a bug. Filing now so the +residual is a named decision, not an accident. + +## D-1 [minor] — `associate_with_token` remains 0%-covered dead surface (the batch-2 completeness twin with no caller) + +**Files**: `src/consumer.rs:450-475` (`associate_with_token` — +uncovered; contrast `connect_with_token`/`bind_with_token`, both +covered via the door tests), the doc claim: "the session family's +completeness … and the wrapper-aware deployment's surface" + +**Problem**: the TCP door refuses ASSOCIATE before any session; the +UDP door rides `AssociateSession::associate` (no token — the UDP door +is NoAuth by construction). No test drives the token twin. This is +review 002 D-1's shape again — spec'd machinery with zero callers — +but here the *doc* already carries the honest posture (the door does +not call it), so this is not doc-vs-code drift. Still: a +constructor-twin whose negotiation body could drift from +`associate`'s (e.g. the DST.ADDR form) would pass the suite. + +**Fix (small)**: one test mirroring `bind_session_with_token_round_ +trip`'s shape (drive `associate_with_token` directly against the +token-provider topology) — or an explicit accepted-uncovered note on +the method. Either closes the review-002-D-1-class trigger before it +fires a third time. + +## D-2 [minor] — `ComposedEgress`'s failure/edge matrix is the batch-2 coverage gap: the LRU's structural `Err` arm, `relay_read`'s sender-closed exit, and the eviction-vs-live-relay accounting are untested + +**Files**: `src/composed/mod.rs` (module total 77.32% lines / 65.31% +functions — the suite's lowest), `tests/composed-egress.rs` (6 tests, +all happy-path or one-shot EOF) + +**Problem**: the composed egress carries several load-bearing failure +shapes that no test drives: + +- `entry_for`'s post-insert invariant `Err` (the structural + diagnostic; unreachable by construction — pinned nowhere, not even + by an `unreachable`-class comment beyond its own doc). +- `relay_read`'s `tx.send(..).is_err()` early return (the shared + queue closed with the egress — the stage exiting mid-relay). The + `Inbound::Ended` path *is* tested (`closed_queue_yields_err_...`), + but the sender-closed path is a different exit. +- The interaction between LRU eviction and `live_relays`: evicting an + entry drops its `TableEntry` (session + write half), which EOFs the + tunnel and ends the relay task → an `Ended` marker → the count + decrements. An association that has evicted *all* of its (capped) + tunnels while parked in `next_reply` would then trip the + `ever_opened && live_relays == 0` association-end — correct, but the + eviction-drives-association-end composition is unpinned (the + existing tests evict and re-dial within a live set). +- `inner_bytes`' oversize guard (`EgressError::General` on a reply + over the joint bound) — the tunnel codec caps at 65535 inner bytes + upstream, so the guard should be unreachable, but nothing pins the + guard fires (or that the tunnel codec's cap makes it unreachable). + +**Recommendation**: one small test round before release polish: (a) +an egress with `max_tunnels: 1` whose single tunnel's far side EOFs → +the association-end `Err` fires through the eviction path; (b) a unit +probe on the invariant-`Err` arm's message shape (or fold it into +(a)). The module's 17 uncovered functions are nearly all these two +regions. + +## D-3 [minor] — The `local` providers' refusal-mapping tables are partially uncovered: `dial_error_of`'s `TimedOut`/`PermissionDenied` arms and `bind_accept`'s `Shortage`-arm splits are compile-checked only + +**Files**: `src/local/mod.rs:76-81` (`dial_error_of`'s +`NetworkUnreachable`/`HostUnreachable`/`TimedOut`/`PermissionDenied` +arms — unreachable from the loopback tests, which can only produce +`ConnectionRefused`), `src/local/mod.rs:253-259` (the bind-map's +`PermissionDenied`/`AddrNotAvailable`/`_` arms — the tests drive +`AddrInUse` only), `src/local/mod.rs:108-110` (the "no addresses +tried" terminal — unreachable: the resolved set is never empty after +the empty-check), `src/local/mod.rs:63-64` (the domain-resolve +"no addresses" arm), `src/local/mod.rs:214` (`try_recv_from`'s +non-`WouldBlock` error arm) + +**Problem**: the task notes record the mapping table as the +deliverable, and `to_reply_error()` is table-tested — but the *io +error kind → SocksDialError* / *io kind → BindAcceptFactoryError* +splits themselves are only half-driven. A refactor that swaps +`TimedOut` into the `_` arm (losing the 0x03 mapping) would pass the +suite. Same class as review 002's N-8 single-arm cosmetics, but +these are security-adjacent (the refusal vocabulary *is* the target +policy's observable surface). + +**Fix (small)**: table-driven unit tests on `dial_error_of` and the +bind-map's closure (they are pure functions of `io::ErrorKind` — +construct errors of each kind with `io::Error::new`); two tests, no +new harness. The two structural arms (`no addresses tried`, the +resolve-empty arm) stay accepted-uncovered with a note each. + +## N-1 [observation] — `ProducerDoorGuard::close`'s `drop(self)`-then-await shape is correct but non-obvious: one doc line would prevent a "simplification" that deadlocks + +**Files**: `src/local/producer_door.rs:194-202` + +**Problem**: `close` drops `self` (releasing the `watch::Receiver`) +*before* awaiting the loop task — load-bearing: the loop's +`done_tx.closed()` only resolves when *all* receivers are dropped, so +holding the receiver across the await would deadlock the graceful +stop. The code is correct; the doc says "ends the accept loop +gracefully" without naming why the drop precedes the await. A future +refactor that "simplifies" the `drop(self)` away (it looks redundant +next to the field-`None`ing above it) reintroduces a hang. + +**Fix (one line)**: extend `close`'s doc: "the receiver is dropped +before the await — the loop's `done_tx.closed()` fires only when +every receiver is gone; holding it here would deadlock." Zero +behavior change. + +## N-2 [observation] — `bindfirst_accept`'s factory errors map to reply#1 codes whose vocabulary is wider than the producer's gate: `NotAllowed`/`Shortage` from a *listen resource* deserve a doc cross-check against the producer-side `bind_policy` gate + +**Files**: `src/composed/mod.rs:718-820` (the factory's +`Refused`/`NotAllowed`/`Shortage` mappings) + +**Problem**: the composed factory returns `BindAcceptFactoryError` +variants that the producer maps to reply#1 codes — including +`NotAllowed` (0x02), the same code the ADR-006 command gate uses for +the ACL refusal. On the channels path the two layers stack: an +identity without `socks5:bind` never reaches the factory (the gate +precedes the seam), so a factory-side `NotAllowed` can only mean the +*tunnel* layer's per-listener ACL — semantically distinct, wire-identical. +That is RFC-faithful (0x02 is the ruleset code) and the composed +variant's per-listener ACL is a real policy source (ADR-007's +per-listener ACL for free), so the behavior is correct — but nothing +in the composed module's doc *names* the stacking ("the factory's +0x02 can also mean the tunnel layer's ACL, distinct from the command +gate's"). An operator debugging a refused BIND sees one code with two +policy sources. + +**Recommendation**: one doc line on `bindfirst_accept` naming the +layering. No code change. + +## N-3 [observation] — `FrontDoor::serve_on`'s per-accept task holds the session-cap permit for the connection's whole life, but the idle-first-byte timeout does not release early: a 256-cap door parks at "cap reached" for connections that are merely slow *readers* + +**Files**: `src/door.rs:338-374` (the accept loop + `Semaphore`), +`src/door.rs:393-401` (the idle-first-byte gate) + +**Problem**: the permit is acquired at accept and dropped at task +end — correct for the cap's purpose (bounding concurrent sessions). +The residual: a connection that passes the idle gate (sent one byte) +but is then slow consuming the producer's conversation holds its +permit for the RFC conversation's full lifetime — the cap counts +*accepted* connections, not *active* sessions. That is the cap +working as designed (ADR-014 knob 2 bounds concurrent *connections*, +which is the slowloris-relevant resource), so this is a naming +residual only: the builder doc says "max-concurrent-sessions" where +the mechanism is "max concurrent *connections*" (including idle-past- +first-byte ones). Same shape on `ProducerDoor`. + +**Recommendation**: one doc word ("connections" not "sessions") on +both builders, or leave as-is — the knob's name is the docs' +vocabulary, not a contract. Zero code. + +## N-4 [observation] — The cred-bearing door's `local_rfc1929_half` check closure writes the mapped identity through a `std::sync::Mutex` witness even when the check rejects: the `None` slot survives a failed check (harmless, but the witness's semantics are "last lookup", not "accepted identity") + +**Files**: `src/door.rs:530-557` + +**Problem**: the checker sets `*slot = mapped` (including `None` on a +miss) before returning `accepted`; the later `.take().ok_or(...)` +correctly treats `None` as rejected. Residual: a checker that ran +twice (upstream's `accept_password_auth` calls it once per +conversation — fine here) would let a later miss overwrite an +earlier hit. Reachable only through API misuse (the closure runs +once per conversation by construction). Harmless as landed; the +"the check IS the table lookup — one table, one truth" doc claim is +accurate for the single-call shape. + +**Recommendation**: none. Recorded to bound re-review (the shape is +the sync-witness pattern's own; a second call site would need +`Option::take`-semantics discipline). + +## N-5 [observation] — The two doors' accept loops duplicate the idle-peek + semaphore-cap + per-accept-isolation shape (`FrontDoor::serve_on` vs `ProducerDoor`'s `serve_on_parts`) — a third door shape should extract it + +**Files**: `src/door.rs:338-374`, `src/local/producer_door.rs:261-294` + +**Problem**: ~35 lines of structural duplication (the +`Semaphore::try_acquire_owned` cap refusal, the `spawn` + +`_permit`-guard shape, the accept-error isolation) — the same N-5 +class as review 002's harness consolidation, across a feature +boundary this time (`door.rs` and `local/producer_door.rs` could +share a helper; both are `local`-gated so no feature-gate obstacle). +Deliberately not fixed inline: the two loops differ in the producer +injection (`serve_socks5_over` vs the variant split) and a third +consumer doesn't exist yet; a premature extraction would trade two +readable loops for one parameterized one. + +**Fix (small, when a third shape lands)**: a +`crate::local::accept_loop`-shaped helper (idle gate + cap + spawn + +isolation) consumed by both. Until then, the duplication is honest. + +## N-6 [observation] — `InbandBridge::recv`'s 16 KiB scratch allocates per call (per datagram when the queue is empty) — bounded, but the retained-capacity note from review 002 S-1 applies one layer up + +**Files**: `src/door.rs` (`InbandBridge::recv`'s `vec![0u8; 16 * 1024]`) + +**Problem**: each decode call allocates a fresh 16 KiB read buffer +(the `AssociateSession::recv_datagram` shape it mirrors does the +same). Bounded per call, dropped per call — fine; the S-1-style +residual (a 65507-byte frame's `BytesMut` retaining ~64 KiB of +capacity in the bridge's reader for the association's lifetime) is +inherited from `DatagramReader`'s design and already recorded in +review 002 S-1. Per-association bounded, unshrunk — same accepted +posture, now with two owners (the session and the door's bridge). + +**Recommendation**: none (the review-002 posture covers it). One line +on `InbandBridge`'s doc pointing at the shared posture, if the next +touch lands. + +## N-7 [observation] — `ProducerDoor::builder()` requires `.producer` but not `.bind` — the bind decision moves to `serve()`, an asymmetry with `FrontDoor`'s builder (where `.bind` is required at build time) + +**Files**: `src/local/producer_door.rs:144-155` + `222-239` vs +`src/door.rs:279-288` + `305-309` + +**Problem**: two builder shapes for sibling types: `FrontDoor`'s +missing-bind is a `build()`-time error (`DoorBuildError:: +MissingBindAddr`); `ProducerDoor`'s is a `serve()`-time error +(`ProducerDoorError::MissingBindAddr`). The producer door's split +exists for a reason (its doc says so — `serve_on` needs no bind), but +`FrontDoor` could have made the same split and didn't. The +asymmetry is documented on both sides; a deployer writing both +shapes hits the difference on the first compile error and reads two +docs. + +**Recommendation**: none (both postures are typed, convention-2 +compliant, and documented). Recorded to bound re-litigation: the +split-vs-eager-bind choice is per-type, not a pattern to propagate. + +## N-8 [observation] — Coverage residue worth one line each (or accepted-uncovered with a note), beyond D-2/D-3 + +The `--all-features --show-missing-lines` sweep attributed; these are +the residuals beyond the findings (each small, named so the next +coverage run needs no re-derivation): + +- `door.rs:160-168` — `CredentialTable::len`/`is_empty`: observability + accessors, no caller (one probe or an accepted-uncovered note). +- `door.rs:194-196` — `DoorExit::LocalRfc`'s Display arm (the + cred-bearing door's local-RFC exit is reached in tests, but that + *variant's* arm render is not asserted): cosmetic. +- `door.rs:305-309` — `FrontDoor::serve` (the bind-then-serve + wrapper; every test drives `serve_on`): one test or a + "the suite drives `serve_on`" note. +- `door.rs:414` — the `(Some(_), None)` unreachable-by-construction + arm: accepted-uncovered by design (convention 2's exhaustive-match + shape), fine. +- `door.rs:483-487` — `raw_pipe`'s open-failure exit (a NoAuth door + whose channel open fails): one probe would pin it. +- `door.rs:595-611` — `pipe_connect_plane`'s Domain-reply form + the + `None`-reply + write-failure arms: the suite drives v4 replies + only; a domain-reply unit probe or accepted-uncovered. +- `door.rs:636-656` — `pipe_bind_plane`'s Domain-form reply wire + (the BIND door's domain-BND re-encode): the suite's BIND tests + drive IP forms; the Domain arm is hand-written wire code with no + test — the batch-2 suite's most substantive single-arm gap. +- `door.rs:722` — `write_refusal_reply`'s `_ => return` arm (a + non-reply-code error stays a bare close): one probe. +- `door.rs:755-758` — `UdpDoorExit`'s Display arms: cosmetic. +- `door.rs:810-814` — `UdpFrontDoor::serve` (same as the TCP door's). +- `door.rs:842-845` — `serve_on`'s spent-`split` arm (`Ok(())` on a + spent association): one probe. +- `door.rs:886` — the `client_addr`-already-set skip; `door.rs:908- + 913` — the reply-before-first-inbound park (`continue` when no + client address is known): both single-arm, timing-shaped. +- `door.rs:941` — the `Io`-exit-as-`Err` arm (F-2's new shape): the + fix landed untested (a relay-socket read error is not producible + over a test duplex without a poisoned socket) — accepted-uncovered + with this note, or a `UdpSocket`-pair probe. +- `door.rs:1005-1048` — the bridge's error arms (a decode garbage in + a *later* batch frame, the `Address` variant): the unit tests pin + the happy shapes; one garbage-later-batch probe. +- `consumer.rs:1145-1147` — `TokenClient`'s Debug: cosmetic. +- `consumer.rs:1185` — the spent-shape assert's `err().map(to_string)` + formatting arm: cosmetic. +- `local/mod.rs:63-64, 108-110, 214, 257-259` — D-3's terminal arms. +- `local/producer_door.rs:71-72` — `BackendExit::Session`'s Display + arm; `:121-124` — the builder's `idle_first_byte`/`max_sessions` + setters (the tests drive both — via `serve()`'s defaults only); + `:200` — `close`'s already-closed arm; `:222-239` — `serve`'s + `MissingBindAddr`/`Bind` error arms; `:269-271` — the accept-error + continue arm; `:314` — the idle-peek `Io` arm. +- `composed/mod.rs` — D-2's matrix (the module's own gap). +- `egress.rs:518` — the sync-`Err` factory test's panic arm (the + match's `Ok` arm — unreachable in the passing test): cosmetic. + +The headline gaps: the BIND door's Domain-form reply wire (D-2's +neighbor), the composed egress's failure matrix, and the +`local/mod.rs` mapping-table arms. + +--- + +# Part B — Non-findings (verified correct, recorded to bound the re-review) + +- **The never-binds posture survived batch 2** (re-swept): the + protocol layer names no kernel-socket type; every bind is a + `local`-gated constructor's explicit decision; the default + + composed wasm checks stay clean; `local`-on-wasm fails with the + expected mio error. The doors' binds are builder-`.bind()`-driven; + `LocalEgress::bind`'s loopback and `bind_accept`'s explicit host + are the doc-pinned explicit shapes. +- **The cred-bearing door's re-origination holds under adversarial + reading**: the door's RFC half consumes greeting + credentials + + one command; the in-band conversation is fresh (the never-pipes- + raw pin); the token is the mapped identity's id riding the open-op + `auth_token` payload (CF-005 precedence (1)); the password never + crosses the channel and never reaches a log line (swept: no + credential is ever in a `tracing` call). +- **The UDP door's InbandBridge carries the ADR-003 reader-state + contract** (the gate review's R-1 fix verified independently): + the pending queue drains in order, partial frames survive call + boundaries, EOF mid-frame fails loud — the three unit pins all + fail against a per-call reader, and the end-to-end doubled-echo + test guards the class through the real stack. +- **The composed accept's teardown-at-birth is structural**: the + `TunnelListener` rides inside the write payload (`ForwardedOwner`); + the disjoint-capture trap is doc-pinned and the whole-struct + rebinding is in place; the client-gone select! drop tears the + listener and the oneshot down together; a correlation miss drops + the forwarded channel at birth (pinned). +- **The forwarded handler's inline pump await (R-02) is real**: the + handler's task lives exactly as long as the data plane (either- + EOF completion), the wrapper's inline await tracks it — verified + against alktunnels' `make_forwarded_op_handler` (the inline + `pump.await`), not just the doc comments. +- **The composed egress's readiness contract is count-based and + sound**: the `ever_opened` guard (the assembly suite's drift fix) + is correct — a never-opened table parks (the stage's channel + branch governs); the ended-relay `Err` fires only after a tunnel + existed and every relay ended; `saturating_sub` bounds the count; + the closed-queue test awaits the `Err` (never a hot `Ok(None)`). +- **The sync-bound wrinkle's Sync proofs are no-unsafe**: both + `SyncHalves` and `OwnedWrite` ride `Mutex` (`Sync` for + `T: Send`, std guarantee); poisoned locks unwrap via `into_inner` + (convention 2); contention is structurally impossible (one pump + task at a time — the doc says so and the pump shapes confirm). +- **No panics in non-test code** (re-swept across all of `src/`, + including the batch-2 modules): every `unwrap`/`expect`/`panic!` + hit is in a `#[cfg(test)]` region or a test file. The batch-2 + `expect` sweep (gate review R-2) held through the review's read. +- **Wire-driven allocation is bounded in the batch-2 code**: the + doors' scratch buffers are fixed (1-byte peek, 64 KiB relay, 16 KiB + bridge chunk); `parse_udp_request`'s domain arm stays 255-bounded; + the UDP door's 65535-byte recv is the kernel's datagram bound; the + cred-bearing door's username/password reads are 1-byte-bounded by + the RFC 1929 length fields (the forked codec's bounded-read + posture). No unbounded wire-driven allocation exists. +- **The doors' failure isolation is real**: per-accept spawn, the + accept loop continues on per-connection failure (both doors + the + producer door, each pinned); a cap-refused accept closes the + socket with no channel opened; the idle gate fires before any + channel opens (witnessed by the channel-count probes). +- **`pump_bidi` is awaited inline everywhere in batch 2** (the + CONNECT/BIND doors' `copy_bidirectional` against taken in-band + streams; the composed accept's middle pump; the producer door + rides `serve_socks5_over`'s own inline await) — no + spawn-and-forget data plane exists. +- **MSRV, wasm, publish**: `cargo publish --dry-run` completes; both + wasm checks clean; `rust-version = "1.88"` unchanged (the batch-2 + additions are plain 2021-edition async/await; no let-else/OnceLock + or other MSRV-relevant surface). +- **License/provenance posture unchanged**: `src/rfc1928/LICENSE` + and the module docs still carry the full fork-point statement; + batch 2 touched no forked file except `server.rs`'s already-pinned + `accept_password_auth` consumption (doc-level only). + +--- + +# Part C — What's Good + +- **The door boundary is where the complexity belongs, and it is + contained.** The entire RFC-codec surface (1929 negotiation, + reply re-encoding, the UDP header codec) lives in `door.rs` + (1135 lines, 70% tests) behind one feature gate; the protocol + layer's growth for all of it was `TokenClient` + three constructor + twins + a `reply_addr` accessor. The F-1 fix was possible *because* + the codec lives at the door — one helper closed all three sites. +- **The provider/producer split is honest.** `ProducerDoor` is pure + accept-loop assembly over the landed `serve_socks5_over` (zero + new protocol-layer code, the module doc's claim verified line by + line); the ADR-014 knobs are two `local` types (`peek` gate, + `Semaphore`) with door-side refusals — the protocol layer never + learned about timers, exactly per ADR-014. +- **The Sync-bound wrinkle's resolution is textbook**: no unsafe, no + new trait, two small adapters with the Sync proof in the type + system, the owner riding inside the payload so the compiler + enforces the teardown — and the whole thing is pinned by a + compile-shape test against the real alktunnels surface. +- **The assembly integration suite is the batch's real deliverable.** + It surfaced the one genuine implementation drift + (`ComposedEgress`'s birth-killed association) that every + per-component suite missed, and it pins the compositions the + component tasks could not (doors + native + composed concurrent, + the idle-bound teardown matrix, the domain-resolution pin). The + mixed-assembly tests are exactly the roles-not-sides posture made + executable. +- **The docs discipline held through an 8-task batch**: the + reconciliations the tasks recorded (the `ever_opened` fix, the + TokenClient door-only contract, the one-command-per-connection + posture, producer.md's local-backend rewrite) were all verified + against the tree; the review found no further spec-vs-code drift + beyond the ones already reconciled. + +--- + +# Suggested Disposition + +The batch-2 gate closed with the crate feature-complete; this review +found the residue a full-surface read always finds. Disposition: + +| Item | Action | Shape | +|---|---|---| +| F-1, F-2 | **Fixed in this review change** | the refusal re-encoding + the UDP-door exit match + the pinning test | +| D-1 | one test or an accepted-uncovered note | the `associate_with_token` probe (or the note) in the release-polish round | +| D-2 | one small test round before release polish | the composed-egress failure matrix (2 tests) | +| D-3 | two table-driven unit tests | the `io::ErrorKind` → vocabulary tables | +| S-1 | accepted-uncovered posture note | one doc line on `UdpFrontDoor::serve_on` | +| N-1 | one doc line | the `close` drop-before-await note | +| N-2 | one doc line | the `bindfirst_accept` 0x02 layering note | +| N-3 | one doc word (or none) | "connections" vs "sessions" on both builders | +| N-4 | none | recorded to bound re-review | +| N-5 | defer | extract when a third accept-loop shape lands | +| N-6 | none | covered by review 002 S-1's posture | +| N-7 | none | recorded to bound re-review | +| N-8 | batch the cosmetics with D-2/D-3's tests | the release-polish round | + +The natural home for the residue is the release-polish task (publish +prep, OQ-SK2-08) the batch-2 decomposition deferred — the review's +test-shaped items ride alongside it; the doc-shaped items are +one-line changes in the same round. + +## References + +- `docs/reviews/002-protocol-core-implementation-review.md` (the + template; the C/D/S/N legend; the N-5/N-8 lineage this review's + D/N items extend) +- `tasks/implementation/review-assembly-layer.md` (the narrow gate — + its R-1 fix is this review's F-class context; its R-2 sweep is the + no-panic posture re-verified in Part B) +- docs/architecture/decisions/003/005/006/007/008/009/014 (the ADRs + the batch-2 surface implements) +- alktunnels 0.2.0 (`producer.rs` `make_forwarded_op_handler` — the + R-02 inline-await cross-check; `ForwardedHandler`'s ownership + shape) +- ADR-014 (the accept-path defense obligations the doors inherit — + the F-2/S-1/N-3 posture context) +- The POC crates (`/workspace/alksocks-{connect,udp,bind}-poc` — the + fidelity sources the gate review diffed; this review's POC-drift + check found no further divergence) \ No newline at end of file diff --git a/src/door.rs b/src/door.rs index 83fa5fb..783b6d1 100644 --- a/src/door.rs +++ b/src/door.rs @@ -480,7 +480,14 @@ async fn cred_bearing( .await { Ok(session) => session, - Err(err) => return DoorExit::Open(err.to_string()), + Err(err) => { + // The in-band refusal's mapped code is re-encoded to + // the vanilla client (the RFC codec at the door + // boundary — ADR-003): a dial refusal is reply#1's + // code, never a bare EOF. + write_refusal_reply(&mut stream, &err).await; + return DoorExit::Open(err.to_string()); + } }; pipe_connect_plane(stream, session).await } @@ -490,7 +497,10 @@ async fn cred_bearing( .await { Ok(session) => session, - Err(err) => return DoorExit::Open(err.to_string()), + Err(err) => { + write_refusal_reply(&mut stream, &err).await; + return DoorExit::Open(err.to_string()); + } }; pipe_bind_plane(stream, session).await } @@ -663,11 +673,13 @@ async fn bind_quiet_and_pipe( let plane = match session.await_inbound().await { Ok(Some(plane)) => plane, Ok(None) => return DoorExit::PlaneEnded, - // An accept-stage refusal (reply#2's mapped code was written - // in-band; the producer's pump ended the session): the door - // closes the vanilla connection — the conversation ends where - // the in-band one did. - Err(_) => { + // An accept-stage refusal (the mapped reply#2 code): the + // vanilla client gets that code re-encoded door-side (the RFC + // codec at the boundary — never a bare EOF), then the + // connection closes — the conversation ends where the in-band + // one did. + Err(err) => { + write_refusal_reply(&mut stream, &err).await; session.close().await; return DoorExit::PlaneEnded; } @@ -699,6 +711,22 @@ async fn write_vanilla_reply(stream: &mut TcpStream, reply: &[u8]) -> std::io::R stream.flush().await } +/// Re-encode an in-band session error's refusal to the vanilla client +/// (the door's RFC codec at the boundary): an error carrying an RFC +/// reply code gets that code as the command's reply (`Succeeded`-form +/// encoding with the error's code + the placeholder BND.ADDR — a +/// refused reply carries a zero address per the RFC); any other error +/// shape (open/transport/I-O) stays a bare close (nothing to encode). +/// Best-effort: a failed write means the client is gone either way. +async fn write_refusal_reply(stream: &mut TcpStream, err: &SocksSessionError) { + let code = match err { + SocksSessionError::Rfc(SocksError::ReplyError(code)) => *code, + _ => return, + }; + let reply = new_reply(&code, SocketAddr::new(IpAddr::V4(Ipv4Addr::UNSPECIFIED), 0)); + let _ = write_vanilla_reply(stream, &reply).await; +} + /// The UDP front door (feature `local`, the POC #2 test-8 shape): the /// ASSOCIATE door — a real UDP relay socket bridged onto one in-band /// association. See the module docs for the shape and the codec @@ -908,7 +936,13 @@ impl UdpFrontDoor { }; tracing::debug!("udp front door: association ended: {exit}"); let _ = association.shutdown().await; - Ok(()) + // A socket I/O failure is the door's fatal outcome (surfaced to + // the caller); the idle bound, the association's end, and a + // session error are the association's own ends — a clean return. + match exit { + UdpDoorExit::Io(err) => Err(err), + _ => Ok(()), + } } } diff --git a/tests/door-tcp.rs b/tests/door-tcp.rs index ac4a39d..a6952b0 100644 --- a/tests/door-tcp.rs +++ b/tests/door-tcp.rs @@ -607,6 +607,79 @@ async fn bind_door_vanilla_conversation_reply_fidelity() { drop(app); } +/// The in-band refusal's reply code is re-encoded to the vanilla +/// client (the RFC codec at the door boundary): an identity carrying +/// `socks5:open` only issues BIND through the cred-bearing door — +/// the in-band command gate refuses 0x02, and the door re-encodes +/// that code as reply#1 (never a bare EOF; the refused reply carries +/// the placeholder zero address). +#[tokio::test] +async fn cred_door_in_band_refusal_surfaces_the_reply_code() { + let dial = alksocks::tcp_dial(); + let table = alksocks::CredentialTable::new(vec![alksocks::DoorCredential { + username: "open-only".to_string(), + password: "pw".to_string(), + identity: Identity { + id: "door-open-only".to_string(), + scopes: vec![alksocks::SOCKS5_OPEN_SCOPE.to_string()], + resources: Default::default(), + }, + }]); + + let topo = harness::wire_consumer_side_with_provider( + dial, + None, + None, + Some(consumer_identity()), + Arc::new(DoorTokenProvider { + bind_scoped: vec![], + }), + ) + .await; + + let listener = TcpListener::bind("127.0.0.1:0").await.expect("bind"); + let addr = listener.local_addr().expect("door addr"); + let door = FrontDoor::builder() + .credentials(table) + .bind(addr) + .build() + .expect("the door builds"); + let _serve = tokio::spawn({ + let client = Arc::clone(&topo.consumer); + async move { + door.serve_on(listener, client) + .await + .expect("the door serves") + } + }); + + let mut vanilla = VanillaClient::dial(addr).await; + let method = timeout(BOUND, vanilla.greet_password()) + .await + .expect("greeting"); + assert_eq!(method, consts::SOCKS5_AUTH_METHOD_PASSWORD); + let status = timeout(BOUND, vanilla.send_credentials("open-only", "pw")) + .await + .expect("credential exchange"); + assert_eq!(status, 0x00, "accepted"); + + // BIND through the open-only identity: the in-band command gate's + // 0x02 rides the door's re-encoded reply (the placeholder BND.ADDR + // — a refused reply carries a zero address). + let (code, advertised) = timeout(BOUND, vanilla.bind_v4([127, 0, 0, 1], 0)) + .await + .expect("the refusal reply arrives (never a bare EOF)"); + assert_eq!( + code, + consts::SOCKS5_REPLY_CONNECTION_NOT_ALLOWED, + "the in-band gate's 0x02 is re-encoded door-side" + ); + assert!( + matches!(advertised, TargetAddr::Ip(sa) if sa.port() == 0 && sa.ip().is_unspecified()), + "a refused reply carries the placeholder BND.ADDR: {advertised:?}" + ); +} + /// The idle-first-byte timeout (ADR-014 knob 1): an accepted /// connection that sends no first byte within the window is closed /// door-side.