review-003: the assembly-layer full review — F-1/F-2 fixed, the residue dispositioned
- Review 003 written (docs/reviews/003-assembly-layer-implementation- review.md): the batch-2 assembly layer post-landing, the review-002 template — correctness/security/smells/coverage over the five batch-2 modules + the batch-2 consumer/producer diff surface. Baseline: 375 default / 620 all-features green, clippy/fmt/doc clean, both wasm checks clean, publish dry-run completes, llvm-cov 93.68% lines / 91.01% functions. - F-1 (major, fixed): the cred-bearing door swallowed every in-band refusal's mapped reply code (the CONNECT/BIND session-open errors and the accept-stage Err were bare closes) — a vanilla client got no reply where ADR-007 pins a reply-code-shaped refusal. Fixed with a write_refusal_reply helper (the door's boundary codec, RFC placeholder BND.ADDR) at all three sites, pinned by cred_door_in_band_refusal_surfaces_the_reply_code. - F-2 (major, fixed): UdpFrontDoor::serve_on returned Ok(()) on a fatal relay-socket I/O failure against its own doc contract — the Io exit now surfaces as Err; the idle/association/session exits stay clean. - Findings recorded, not fixed (the release-polish round's residue): D-1 (associate_with_token 0% covered), D-2 (the composed-egress failure matrix), D-3 (the io-kind refusal-table arms), S-1 (the UDP door's learned client address), N-1..N-8 (doc lines, posture notes, coverage residue). Verification: cargo test (375) + --all-features (620) green, clippy --all-targets --all-features -D warnings clean, fmt --check clean, doc --no-deps clean, both wasm checks clean, publish dry-run completes.
This commit is contained in:
glm-5.3-flash committed
1 parent
04c76e3a41
commit
2db05172ef
3 files changed
+797
-8
No files matched your search
@@ -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<T: Send>` (`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)
|
||||
+42
-8
@@ -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(()),
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
|
||||
@@ -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.
|
||||
|
||||
Reference in new issue
Block a user