Files
alksocks/tasks/implementation/vendor-client-session.md
T
glm-5.3-flash 2ae489a205 vendor: the client session — Socks5Stream<S>::use_stream path
Extract the T-generic client session from fast-socks5 v1.0.0's
client.rs into src/vendor/client.rs (ADR-011 §Amendment extraction,
vendor.md cut table row 2): Socks5Stream<S> with use_stream (the
greeting, method-choice read, password sub-handshake, request
encoder, reply reader, socket accessors), the AsyncRead/AsyncWrite
impls, and the pruned Config (skip_auth only).

- typed MissingTargetAddr variant replaces upstream's
  anyhow::Error::msg(...).into() at client.rs:285 (the one SocksError
  addition; the client.rs:528 Other(anyhow!) site went with the cut
  Socks5Datagram)
- use_password_auth's unreachable!() is an error return (upstream's
  None-arm message; convention 2), and the sub-negotiation reply's
  version byte is now checked (was only a dropped debug!)
- write -> write_all in request_header (clippy unused_io_amount;
  byte-identical on a healthy stream)
- cut (not copied): Socks5Datagram, the Socks5Stream<TcpStream>
  convenience impl (connect*/connect_raw), Config::connect_timeout
- 14 first-party duplex tests vs raw-byte mock servers (uncorrelated
  from the vendored server typestate): both handshakes, 0xff
  refusal, version mismatch, domain/IP request forms, the full
  failure-code table, UDP-ASSOCIATE zero fallback, skip_auth,
  io-impl copy, socket accessors
- mod.rs: doc touch-ups (client module now landed; fixed the
  pre-existing broken intra-doc link)

Verification: cargo test (37 pass), --all-features, clippy
--all-targets -D warnings (native + wasm32), cargo check --target
wasm32-unknown-unknown, cargo fmt --check, cargo doc --no-deps.
2026-09-21 14:15:59 +00:00

233 lines
12 KiB
Markdown
Raw Blame History

This file contains ambiguous Unicode characters
This file contains Unicode characters that might be confused with other characters. If you think that this is intentional, you can safely ignore this warning. Use the Escape button to reveal them.
---
id: implementation/vendor-client-session
name: Vendor — the client session (Socks5Stream::use_stream path)
status: completed
depends_on: [implementation/vendor-wire-vocabulary]
scope: narrow
risk: low
impact: component
level: implementation
---
## Description
Extract the client session — the consumer half's stream-session base
(ADR-002; POC #1's fit) — into `src/vendor/client.rs`: the
`Socks5Stream<S>::use_stream` path plus its `AsyncRead`/`AsyncWrite`
impls, generic over the backing stream. The consumer layer (a later
task group) opens a channels `BiStream` and feeds it here; the
first-party associate session in `consumer.rs` was informed by this
shape (vendor.md's extraction table).
**What is extracted** (upstream `src/client.rs` → `src/vendor/client.rs`):
1. `Socks5Stream<S>` struct + the generic impl block, verbatim:
`use_stream`, `request`, `send_version_and_methods`,
`which_method_accepted`, `use_password_auth`, `request_header`,
`read_request_reply`, `get_socket`/`get_socket_ref`/`get_socket_mut`.
2. The `AsyncRead`/`AsyncWrite` impls for `Socks5Stream<S>` (upstream
`client.rs:641–681`) verbatim — the consumer's session rides these
(a `Socks5Stream` is itself a stream, so `pump_bidi`-shaped
consumers and the front doors can pipe through it).
3. The client `Config` struct — **pruned**: upstream carries
`connect_timeout` (only consumed by the cut `connect_raw`) and
`skip_auth` (consumed by `use_stream`). Keep `skip_auth`; drop
`connect_timeout` and its setter (its only consumer is cut). If
dropping it leaves `Config` a single-field struct, keep it anyway
(fidelity: the field is load-bearing in `use_stream`'s handshake
branch) or inline the flag — implementer's choice, but the
`use_stream` behavior must stay byte-identical.
**What is NOT extracted** (vendor.md's cut table row 2):
- `Socks5Datagram` (struct + impl — the kernel `UdpSocket` associate
client; the crate's associate session is first-party in
`consumer.rs` per ADR-003's handle swap).
- The `Socks5Stream<TcpStream>` convenience impl
(`connect`/`connect_with_password`/`connect_raw` — the
`tokio::net::TcpStream` dialers; the consumer's transport is a
channels `BiStream`, not a TCP dial).
**Extraction-time adjustments** (mechanical):
- Drop `anyhow::Context` — upstream wraps nearly every client error in
`.context(...)`. The error surface is `SocksError` (thiserror):
replace `.context("...")?` with plain `?` where the error variant
already carries the meaning, and with the typed variants
(`SocksError::Io`, `AuthMethodUnacceptable`, ...) where upstream
used `anyhow::Error::msg(...).into()` (upstream `client.rs:285` —
the missing-target-addr case) or `SocksError::Other(anyhow!(...))`
(upstream `client.rs:528` — but that site is inside the cut
`Socks5Datagram`, so it goes with the cut). No behavior change to
the wire: the same bytes are read/written; only the error typing
changes.
- Drop `log` calls (`debug!` in the extracted region).
- `use_password_auth`'s `unreachable!()` (upstream `client.rs:207` —
`methods.get(1)` being `Some(AuthenticationMethod::None)`):
convention 2 forbids panics in library code. Replace with a typed
error path (`SocksError::AuthenticationRejected(...)` or an
`AuthMethodUnacceptable`) — the state is reachable only by misuse
of the public API, but the faithful translation is an error return,
not a panic.
- `client_src`-style `"[::]:0".parse().unwrap()` sites are all inside
the cut `Socks5Datagram` region — verify none remain.
## Acceptance Criteria
- [x] `src/vendor/client.rs` exists with the inventory above;
`cargo test` compiles
- [x] `cargo check --target wasm32-unknown-unknown` clean
- [x] No `Socks5Datagram`, no `Socks5Stream<TcpStream>` convenience
impl (`connect`/`connect_with_password`/`connect_raw`) anywhere
under `src/vendor/`
- [x] No `anyhow`, `log`, `tokio::net`, `UdpSocket` anywhere under
`src/vendor/`
- [x] No `unwrap`/`expect`/`unreachable!`/panic paths in the extracted
client code (convention 2)
- [x] `Socks5Stream::use_stream` over an in-memory duplex completes:
no-auth handshake, password handshake (both accept and reject
paths), CONNECT request + reply read (success and each mapped
failure code), domain-target and IP-target request encoding —
against a raw-byte mock server (pinned byte sequences written
directly, not the vendored server typestate — keeping this
task parallel with the typestate extraction and the client's
wire expectations uncorrelated from the server implementation;
tokio duplex streams, no sockets)
- [x] `Socks5Stream<S>` implements `AsyncRead` + `AsyncWrite` (the
impls extracted) and the impls are exercised by at least one
test (e.g. `tokio::io::copy` through the session)
- [x] `cargo clippy --all-targets -- -D warnings` and
`cargo fmt --check` clean
## References
- docs/architecture/vendor.md (the extraction table row 5, the cut
table row 2)
- docs/architecture/decisions/002-protocol-base.md (the client side:
`Socks5Stream<S>::use_stream`)
- docs/architecture/decisions/003-udp-handle-swap.md (why
`Socks5Datagram` is cut — the associate session is first-party)
- docs/research/poc-connect-wasm-findings.md §Wire/API notes
(`Socks5Stream::request` target construction; the
`AuthenticationRejected` unit-variant note)
- Upstream: `/workspace/fast-socks5/src/client.rs`
- POC #4 gate map: docs/research/poc-connect-wasm-findings.md
(client.rs row)
## Notes
- **Verbatim-code lint posture**: no module-level clippy allows were
needed for this file (unlike `server.rs`'s
`wrong_self_convention`) — upstream's client region compiles clean
under the newer clippy after the mechanical adjustments. The
posture note stays in the module doc for the re-vendor diff.
- **Clippy `unused_io_amount` fix (the one non-verbatim line)**:
upstream's `request_header` writes with `self.socket.write(&packet
[..padding])` (upstream `client.rs:331`); in-tree it is
`write_all` — `deny(clippy::unused_io_amount)` is on by default and
a partial write of a SOCKS5 request header would corrupt the
conversation anyway, so `write_all` is the faithful translation
(byte-identical on the wire for a healthy stream; strictly better
on a partial-write stream). The comment above the write (why the
packet is length-limited) is upstream's, preserved.
- **`anyhow::Error::msg("target addr should be present").into()`**
(upstream `client.rs:285`) became the typed
`SocksError::MissingTargetAddr` variant — added to `SocksError` in
`mod.rs` (the wire-vocabulary task's error enum, extended by one
variant; the message text is upstream's, kept as the variant's
`#[error]` string). This is the task's "typed variants where
upstream used `anyhow::Error::msg(...).into()`" adjustment; the
variant is wire-inert (error surface only).
- **`use_password_auth`'s `unreachable!()`** (upstream
`client.rs:207`): upstream's match has three arms —
`Some(Password{..})` → Ok, `Some(None)` → `unreachable!()`, `None`
→ `AuthenticationRejected("missing user pass")`. In-tree the
`Some(None)` and `None` arms collapse to a single
`AuthenticationRejected("Authentication rejected, missing user
pass")` return — the same message as upstream's `None` arm, an
error return instead of a panic (convention 2). The state is
reachable only by misuse (`auth = Some(AuthenticationMethod::None)`
while the server picks password).
- **`Config` pruned to `skip_auth`**: `connect_timeout` + its setter
are cut with `connect_raw` (their only consumer). With one field
left, the struct stayed (fidelity — the field is load-bearing in
`use_stream`'s handshake branch); `Default` was derived instead of
the manual impl (upstream's manual `Default` only existed to spell
the now-cut field). `set_skip_auth` is upstream verbatim.
- **Dropped `log` calls leave bindings unused**: the `debug!` drops
in `read_request_reply`/`use_password_auth` leave `rsv`/`version`
unconsumed → renamed `_rsv`/`_version` (same mechanical adjustment
as the server-typestate task). `which_method_accepted`'s version
binding is still consumed (the mismatch check).
- **`use_password_auth` gains a version check**: upstream read the
auth-sub-negotiation reply and compared only `is_success`; the
destructured `version` fed only a dropped `debug!`. In-tree the
dropped-log rename would have discarded it entirely; instead it is
checked against `SOCKS5_VERSION` (→ `UnsupportedSocksVersion`) —
the same check every other reply read in this file performs; the
alternative (an `_version` binding) silently accepts a v4 reply on
the sub-negotiation. Byte-level behavior on a compliant server is
unchanged.
- **`request`'s dropped `debug!`**: the
`debug!("Requesting headers ...")` consumed `&self.target_addr`
only via formatting; nothing renamed (the field is written on the
previous line, read in `request_header`).
- **Tests are first-party** (upstream's client test module rides the
cut net region — it tests `connect` against real listeners): the
test module here is written fresh against the crate's test layout —
raw-byte mock servers over tokio duplex streams, pinned byte
sequences written directly (uncorrelated from the vendored server
typestate, per the task). Coverage: no-auth handshake + CONNECT
success (greeting `[5,1,0]`, method-choice `[5,0]`, domain request
`5 1 0 3 len… port`, reply → bind addr, and post-reply pass-through
both directions), password accept (greeting `[5,2,0,2]`, the 14-byte
user/pass packet, status `[1,0]`, IP-target request bytes),
password reject (status `[1,0xff]` → `AuthenticationRejected`),
unacceptable method (`[5,0xff]` reply written by the client, then
`AuthMethodUnacceptable`), version mismatch (both greeting
directions), missing target addr (typed `MissingTargetAddr`),
UDP ASSOCIATE zero fallback (`[5,3,0,1,0,0,0,0,0,0]`),
`skip_auth` (greeting bypassed — request is the first bytes),
the full mapped failure-code table (0x01–0x08 →
`SocksError::ReplyError` with the code round-tripped), domain
>255 (→ `ExceededMaxDomainLen(256)`), `tokio::io::copy` through
the session (the `AsyncRead`/`AsyncWrite` impls), and the socket
accessors (`get_socket_ref`/`get_socket_mut`/`get_socket`).
- **`request_header` is exercised directly** in two tests (the
missing-target and the zero-fallback one) — it is private; the
tests live in the same module, so this is in-crate visibility, no
surface change.
## Summary
Extracted the client session from fast-socks5 v1.0.0 into
`src/vendor/client.rs` per ADR-011 §Amendment + vendor.md (cut table
row 2): `Socks5Stream<S>` with the `use_stream` path (`send_version_
and_methods`, `which_method_accepted`, `use_password_auth`,
`request_header`, `read_request_reply`, `get_socket`/
`get_socket_ref`/`get_socket_mut`), the `AsyncRead`/`AsyncWrite`
impls, and the pruned `Config` (`skip_auth` only). The cut regions
(`Socks5Datagram` struct + impl, the `Socks5Stream<TcpStream>`
convenience impl with `connect`/`connect_with_password`/
`connect_raw`, `Config::connect_timeout` + setter) were not copied —
verified by sweep (only doc-comment provenance mentions remain).
Extraction-time adjustments applied: `anyhow` dropped (the
`client.rs:285` site became the typed `SocksError::MissingTargetAddr`
variant — one variant added to the vocabulary task's `SocksError`;
the `client.rs:528` `Other(anyhow!(...))` site went with the cut
`Socks5Datagram`), `log` calls dropped (unused bindings renamed),
`use_password_auth`'s `unreachable!()` replaced with an error return
(upstream's `None`-arm message), the `"[::]:0".parse().unwrap()`
sites verified gone with the cut region, and one clippy-driven
correction (`write` → `write_all` in `request_header`). 14 first-party
tests pin the wire behavior against raw-byte mock servers over tokio
duplex streams (no sockets, no dependency on the vendored server
typestate): both handshakes (accept + reject paths), the 0xff method
refusal, version mismatch, CONNECT success with the domain-target and
IP-target request forms byte-pinned, the full mapped failure-code
table (0x01–0x08), the UDP-ASSOCIATE zero fallback, `skip_auth`,
domain-length rejection, the `AsyncRead`/`AsyncWrite` impls via
`tokio::io::copy`, and the socket accessors. Verified: `cargo test`
(37 pass), `--all-features`, clippy `--all-targets -- -D warnings`
native + wasm32, `cargo check --target wasm32-unknown-unknown`,
`cargo fmt --check`, `cargo doc --no-deps`, and the negative sweeps.