diff --git a/Cargo.toml b/Cargo.toml index 973d2e5..cb57f01 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -23,6 +23,10 @@ composed = [] [dependencies] tokio = { version = "1", default-features = false, features = ["rt", "sync", "io-util", "macros", "time"] } +# The fork's declared logging seam (rfc1928.md: upstream `log` calls +# were dropped or re-pointed here); no source file references it yet — +# the first consumer is expected to be the producer layer's +# diagnostics (review 001 N-2). tracing = "0.1" thiserror = "2" diff --git a/docs/architecture/rfc1928.md b/docs/architecture/rfc1928.md index 6bdafef..fdf46ae 100644 --- a/docs/architecture/rfc1928.md +++ b/docs/architecture/rfc1928.md @@ -77,7 +77,7 @@ extraction consumer reached it. None of it is forked: |---|---| | `src/server.rs` | `run_tcp_proxy`, `run_udp_proxy`/`run_udp_proxy_custom`, `udp_bind_random_port`, the legacy `Socks5Server`/`Incoming`/`Socks5Socket` (struct + impls), `DnsResolveHelper` trait + impl, `handle_udp_*`/`transfer_udp`, the net-only test module — plus the legacy `Config` + `Authentication` trait + impls (`SimpleUserPassword`/`DenyAuthentication`/`AcceptAuthentication`, `authenticate_callback`), which POC #4 left ungated but which only this cut region consumes (rows 1–5 are POC #4's gate map; the `Config`/`Authentication` layer and the socks4 row are this recipe's two additions) | | `src/client.rs` | `Socks5Datagram` (struct + impl), the `Socks5Stream` convenience impl (`connect`/`connect_with_password`/`connect_raw`) | -| `src/util/stream.rs` | `tcp_connect*` fns (the `read_exact!`/`ready!` macros + `ConnectError` stay) | +| `src/util/stream.rs` | `tcp_connect*` fns (the `read_exact!`/`ready!` macros + `ConnectError` stay — `ConnectError` kept-with-note, review 001 C-1: constructed nowhere in-tree, kept as the hand-port map's companion for a future kernel-net port) | | `src/util/target_addr.rs` | the `resolve_dns` method (the codec stays) | | `src/lib.rs` | the net-heavy test module (consts/errors/UDP-header codec stay) | | `src/socks4/` | the whole module (upstream's separate default-off `socks4` cargo feature; out of RFC 1928 scope) | @@ -155,7 +155,14 @@ at extraction, before the fork): `connect_timeout` field), and `request_header` writes with `write_all` where upstream wrote with `write` (a partial write of a request header would corrupt the conversation; byte-identical on - a healthy stream — `clippy::unused_io_amount` is deny-by-default). + a healthy stream). `reply_success`/`reply_error` were likewise + converted at the fork review (001 F-1): replies are small, + single-shot frames that must be all-or-nothing — a partial write + desynchronizes the conversation silently. (The + `clippy::unused_io_amount` lint — deny-by-default — does flag bare + `.write()` on `AsyncWriteExt`, but the `err_when`-chaining idiom + type-infers the result through a generic blanket impl and suppresses + it; the linter is not a substitute for the conversion.) Plus the **post-fork convention fixes** (ADR-013 — the lint-allow posture dissolved, project conventions applied): the module-level diff --git a/docs/reviews/001-rfc1928-full-review.md b/docs/reviews/001-rfc1928-full-review.md index 9fa99c9..768a727 100644 --- a/docs/reviews/001-rfc1928-full-review.md +++ b/docs/reviews/001-rfc1928-full-review.md @@ -1,5 +1,5 @@ --- -status: open +status: resolved last_updated: 2026-09-23 reviewed_artifacts: - src/rfc1928/mod.rs @@ -604,20 +604,20 @@ comment. Trivial. Volume is small — one trivial code fix plus doc/test riders — so recommended remediations are included per finding: -| ID | Finding | Recommended fix | Effort | Risk | -|----|---------|----------------|--------|------| -| F-1 | `.write()` reply sites + false doc rationale | `.write_all` at 2 sites + rfc1928.md bullet update | trivial | none | -| S-1 | domain-length allocation invariant undocumented | doc line + pinning test | small | none | -| S-2 | 0xff vocabulary conflation undocumented | doc line on `reject` | trivial | none | -| C-1 | `Bug`/`ConnectError` dead vocabulary | decide keep-with-note vs cut (decide at producer layer) | trivial | none | -| C-2 | `StandardAuthentication` uncalled | note posture now; cut if still uncalled when producer lands | trivial | none | -| L-1 | client v6 request form untested | one differential test | small | none | -| L-2 | `poll_shutdown` passthrough unpinned | one EOF-propagation test | small | none | -| L-3 | `wait_on_tcp` io-error arm uncovered | one test | trivial | none | -| N-1 | rfc1928.md bullet (F-1's face) | rides F-1 | trivial | none | -| N-2 | `tracing` dependency purpose | note or first consumer | trivial | none | -| N-3 | `CheckResult` impls unused | decide at producer layer | none | none | -| N-4 | `Addr::Domain` comment | pin or drop | trivial | none | +| ID | Finding | Recommended fix | Effort | Risk | Status | +|----|---------|----------------|--------|------|--------| +| F-1 | `.write()` reply sites + false doc rationale | `.write_all` at 2 sites + rfc1928.md bullet update | trivial | none | ✅ resolved | +| S-1 | domain-length allocation invariant undocumented | doc line + pinning test | small | none | ✅ resolved | +| S-2 | 0xff vocabulary conflation undocumented | doc line on `reject` | trivial | none | ✅ resolved | +| C-1 | `Bug`/`ConnectError` dead vocabulary | decide keep-with-note vs cut (decide at producer layer) | trivial | none | ✅ resolved (keep-with-note) | +| C-2 | `StandardAuthentication` uncalled | note posture now; cut if still uncalled when producer lands | trivial | none | ✅ resolved (posture noted) | +| L-1 | client v6 request form untested | one differential test | small | none | ✅ resolved | +| L-2 | `poll_shutdown` passthrough unpinned | one EOF-propagation test | small | none | ✅ resolved | +| L-3 | `wait_on_tcp` io-error arm uncovered | one test | trivial | none | ✅ resolved (+ mapping-table + version-mismatch riders) | +| N-1 | rfc1928.md bullet (F-1's face) | rides F-1 | trivial | none | ✅ resolved | +| N-2 | `tracing` dependency purpose | note or first consumer | trivial | none | ✅ resolved (posture noted) | +| N-3 | `CheckResult` impls unused | decide at producer layer | none | none | ✅ resolved (posture noted) | +| N-4 | `Addr::Domain` comment | pin or drop | trivial | none | ✅ resolved (pinned) | Suggested decomposition: (1) the F-1 batch (F-1 + N-1 + S-2 + N-4 — one commit, code + doc + no behavior change beyond the @@ -631,6 +631,144 @@ in the follow-up session before task decomposition (the F-1 mechanism claim in particular rests on an executable probe that should be reproduced fresh). +# Remediation Resolution (2026-09-23) + +All findings resolved in one session: findings re-verified fresh +(source read of every named site), the F-1 mechanism probe +reproduced, then the three batches applied with the review's +decomposition. Batch 1 (code+doc, no behavior change beyond the +partial-write hardening) and batch 2 (tests) and batch 3 (posture +notes) land as one commit (the review's three batches are small and +the verification battery runs once over the union; the batch +boundaries are recorded below). The decide-at-producer-layer items +were decided now — keep-with-note — so the posture is written down +before the producer layer starts consuming this surface. + +## Re-verification + +- **F-1 confirmed** at `server.rs:516`/`529` (fresh read: both + `.write(&...)` sites present, `err_when`-chained). +- **The `unused_io_amount` suppression mechanism reproduced fresh** + (scratch crate `/tmp/opencode/io-probe`, five variants, deleted + after the run): bare `.write()?` on a generic receiver fires the + lint (deny-by-default confirmed); the same `.write()` through + `.err_when(...)?`, `.map(|_| ())`, and a concrete-receiver + `err_when` chain all stay silent; `write_all` never fires. Exactly + the review's probe result. +- **S-1, S-2, C-1, C-2, L-1, L-2, L-3, N-1..N-4 confirmed by + inspection** at the named sites (line numbers verified against the + current tree; the dead-code claims grep-verified — zero + construction sites for `Bug`/`ConnectError`, zero callers of + `StandardAuthentication`/`allow_no_auth`, zero `tracing` + references in `src/`). +- The rfc1928.md bullet (N-1) confirmed as written (the lint + attribution stands; the server-side sites unrecorded). + +## Resolution — batch 1 (F-1 + N-1 + S-2 + N-4) + +- **F-1**: `reply_success`/`reply_error` now `.write_all` the reply + frame (`server.rs`, both sites). Wire-identical on a healthy + stream; the partial-write hazard is closed; the differential + suite's reply-form tests guard the fix byte-for-byte. +- **N-1**: the rfc1928.md adjustment bullet now records both + conversions (client `request_header` + server `reply_success`/ + `reply_error`), states the real rule (replies are small, + single-shot, all-or-nothing), and documents the linter's + `err_when`-chaining blind spot instead of the false + lint-attribution claim. +- **S-2**: `PasswordAuthenticationImpl::reject`'s doc names the + vocabulary choice (0xff is the method-negotiation constant + repurposed; RFC 1929 requires only non-`0x00`; the in-tree + client/server pairing works because both sides write/check the + same arbitrary constant). +- **N-4**: `Addr::Domain(String)`'s upstream deliberating comment + replaced with the pinned decision (validated UTF-8 at the codec + boundary). + +## Resolution — batch 2 (coverage: L-1 + L-2 + L-3 + S-1) + +Four differential tests + one lib test (the review's small batch, +plus one rider): + +- **L-1**: `client_v6_request_encoding_is_22_bytes_with_atyp_4` — + the v6 request arm (`packet[4..20]`/`packet[20..22]` slice + arithmetic) pinned byte-for-byte through the public surface. +- **L-2**: `client_session_shutdown_propagates_to_the_backing_stream` + — `session.shutdown()` reaches the backing stream's `poll_shutdown` + (the mock observes EOF; a post-shutdown write surfaces + `BrokenPipe`), pinning the half-close propagation the consumer + half's two-pump teardown rides. +- **L-3**: `wait_on_tcp_wraps_io_errors_with_context` — the + `Err(err)` arm driven with a failing reader (a dropped-peer duplex + read is a clean EOF, `Ok(0)` — the `EOF` arm — not the `Err` arm, + so the test injects a failing reader); plus + `to_reply_error_mapping_table` (server.rs in-module test) pinning + the `UnknownCommand`/`AddrError` arms and the `GeneralFailure` + fall-through across all non-mapped variants; plus (rider, the + review's "worth one test" note on the version-mismatch arm) + `command_version_mismatch_refuses_before_address_parse` covering + `read_command`'s command-phase version check. +- **S-1**: the `read_address` doc line states the 1-byte-field bound + (widening the field requires a cap before the allocation), and + `decode_side_domain_allocation_is_bounded_by_the_1_byte_wire_field` + pins both faces (255 decodes through the attacker-side path; the + encoder rejects 256, so the decode arm's `len as usize` can never + ask for more). + +Coverage after (llvm-cov): **93.09% regions / 95.65% functions / +96.07% lines** (1528 covered / 60 missed; baseline 92.63/95.08/ +95.40 — 1499/69). The named gaps closed: client v6 request arm +(293–300) covered, `poll_flush`/`poll_shutdown` passthroughs covered, +`wait_on_tcp`'s io-error arm covered, `read_command`'s version- +mismatch arm covered, `to_reply_error`'s fall-through covered. +Remaining misses are exactly the C-2/N-3 accepted-uncovered blocks +(the `StandardAuthentication` macro machinery, `allow_no_auth`, +`CheckResult`'s `Option`/`bool`/`bool` impls, +`skip_auth_this_is_not_rfc_compliant`) plus the client's +`use_password_auth` misuse arm and error-test `Ok(())` tails — +all recorded as accepted-uncovered with their disposition below. + +## Resolution — batch 3 (posture notes: C-1, C-2, N-2, N-3) + +All four decided now, keep-with-note, no code change (the review's +option (b) for C-1; the recommendation for C-2/N-3/N-2): + +- **C-1**: `ConnectError` + `SocksServerError::Bug` kept as the + hand-port map's companions — the module doc on `util/stream.rs` + states it (`to_reply_error` is what a future kernel-net port + reuses; if the producer layer lands without consuming it, cut it + then), and rfc1928.md's cut-table row records the posture. The + deciding consumer remains the dial-callback shape (ADR-004) at + producer-layer time. +- **C-2**: `StandardAuthentication` kept with the posture on its doc + ("currently uncalled in-tree; the producer layer's per-door auth + selection (ADR-005) is its anticipated consumer; cut if still + uncalled when the producer lands"). Its coverage block is + accepted-uncovered. +- **N-2**: `tracing` kept; `Cargo.toml` carries the feature-note- + precedent comment (the fork's declared logging seam; first + consumer expected at the producer layer's diagnostics — the + `try_notify!` secondary-error site). +- **N-3**: `CheckResult`'s doc records the same keep-or-cut decision + (in-tree callers use `bool` only; the producer's username/password + mapping with typed rejections is the anticipated `Result` consumer). + +## Verification (post-remediation) + +- `cargo test`: **39 lib + 37 integration + 2 doc (ignored) — all + pass** (was 38/32/2; +1 lib test, +5 differential tests). +- `cargo clippy --all-targets -- -D warnings`: clean. +- `cargo clippy --target wasm32-unknown-unknown -- -D warnings`: + clean. +- `cargo fmt --check`: clean. +- `cargo doc --no-deps`: 0 warnings. +- `cargo check` + `clippy -D warnings` (wasm32): clean — the + wasm-clean invariant holds. +- `cargo +1.88 check`: clean (MSRV intact). +- `cargo publish --dry-run --allow-dirty`: completes. +- `cargo llvm-cov`: 93.09% regions / 95.65% functions / 96.07% + lines (see the batch-2 section for the per-module delta). + --- # References diff --git a/src/rfc1928/server.rs b/src/rfc1928/server.rs index 628cbc9..3c339be 100644 --- a/src/rfc1928/server.rs +++ b/src/rfc1928/server.rs @@ -141,6 +141,14 @@ impl Socks5ServerProtocol { } } +/// Closure-result adapter for [`Socks5ServerProtocol +/// ::accept_password_auth`]'s credential checker: `bool`, +/// `Option`, and `Result` all work (`is_good`). In-tree +/// callers use `bool` only; the `Option`/`Result` impls anticipate +/// the producer layer's username/password mapping (typed rejections +/// want `Result`-returning checkers — review 001 N-3). Same +/// keep-or-cut decision as `StandardAuthentication` when the producer +/// layer lands. pub trait CheckResult { fn is_good(&self) -> bool; } @@ -349,6 +357,14 @@ impl PasswordAuthenticationImpl Result<(), SocksServerError> { self.inner .write_all(&[1, consts::SOCKS5_AUTH_METHOD_NOT_ACCEPTABLE]) @@ -434,6 +450,13 @@ auth_method_enums! { /// as an enum appropriate for static dispatch. /// /// If you want to add your own custom methods, you can generate a similar enum using the `auth_method_enums` macro. + /// + /// Posture (review 001 C-2): currently uncalled in-tree — the + /// typestate's `negotiate_auth` is driven with the single-method + /// slices everywhere. Kept as the negotiation machinery's + /// combinator: the producer layer's per-door auth selection + /// (ADR-005's auth-split) is its anticipated consumer. If the + /// producer layer lands without consuming it, cut it then. pub enum StandardAuthentication / StandardAuthenticationStarted { NoAuthentication(NoAuthenticationImpl), PasswordAuthentication(PasswordAuthenticationImpl), @@ -513,7 +536,7 @@ impl Socks5ServerProtocol Result { self.inner - .write(&new_reply(&ReplyError::Succeeded, sock_addr)) + .write_all(&new_reply(&ReplyError::Succeeded, sock_addr)) .await .err_when("writing successful reply")?; @@ -526,7 +549,7 @@ impl Socks5ServerProtocol Result<(), SocksServerError> { let reply = new_reply(error, V4_UNSPECIFIED_ZERO); self.inner - .write(&reply) + .write_all(&reply) .await .err_when("writing unsuccessful reply")?; @@ -656,6 +679,51 @@ mod tests { use tokio::io::AsyncReadExt; use tokio::io::AsyncWriteExt; + /// `SocksServerError::to_reply_error`'s full mapping table (review + /// 001 L-3): `UnknownCommand` → `CommandNotSupported`, + /// `AddrError` delegates to its own table, and every remaining + /// variant falls through to `GeneralFailure` — the mapping the + /// producer's error surface relies on when mapping session errors + /// to reply codes (`try_notify!`'s secondary-error write). The + /// fall-through arms (`Io`/`FromUtf8`/`EOF`/auth variants) are + /// exercised here directly, since driving each from the wire is + /// not one-test-cheap. Upstream `src/server.rs:62-70`. + #[test] + fn to_reply_error_mapping_table() { + assert!(matches!( + SocksServerError::UnknownCommand(0x7f).to_reply_error(), + ReplyError::CommandNotSupported + )); + assert!(matches!( + SocksServerError::AddrError(crate::rfc1928::AddrError::IncorrectAddressType) + .to_reply_error(), + ReplyError::AddressTypeNotSupported + )); + for fall_through in [ + SocksServerError::EOF, + SocksServerError::EmptyUsername, + SocksServerError::EmptyPassword, + SocksServerError::AuthenticationRejected, + SocksServerError::UnsupportedSocksVersion(0x04), + SocksServerError::Bug("test"), + SocksServerError::AuthMethodUnacceptable(vec![0x02]), + SocksServerError::UnexpectedUdpControlGarbage(0xde), + SocksServerError::Io { + source: std::io::Error::other("x"), + context: "reading", + }, + SocksServerError::FromUtf8 { + source: String::from_utf8(vec![0xff]).unwrap_err(), + context: "converting", + }, + ] { + assert!(matches!( + fall_through.to_reply_error(), + ReplyError::GeneralFailure + )); + } + } + #[test] fn new_reply_v4_form() { let addr: SocketAddr = "127.0.0.1:1080".parse().unwrap(); diff --git a/src/rfc1928/util/stream.rs b/src/rfc1928/util/stream.rs index 87d4388..dc9b22c 100644 --- a/src/rfc1928/util/stream.rs +++ b/src/rfc1928/util/stream.rs @@ -11,6 +11,15 @@ //! is a plain `macro_rules!` scoped by `pub(crate) use` so it stays //! internal to the RFC 1928 core (implementation detail, not alksocks //! public API). +//! +//! [`ConnectError`] is currently constructed nowhere in-tree (review +//! 001 C-1 — upstream's sole construction site was the cut +//! `tcp_connect` dialer). It is kept as the hand-port map's +//! companion: the dial-error → reply-code mapping +//! (`to_reply_error`) is what a future kernel-net port (e.g. the +//! dial-callback shape, ADR-004) would reuse, and its mapping is +//! pinned by the module's test. If the producer layer lands without +//! consuming it, cut it then. /// Easy to destructure bytes buffers by naming each fields: /// diff --git a/src/rfc1928/util/target_addr.rs b/src/rfc1928/util/target_addr.rs index 90a5eb5..bbd148b 100644 --- a/src/rfc1928/util/target_addr.rs +++ b/src/rfc1928/util/target_addr.rs @@ -204,10 +204,18 @@ impl ToTargetAddr for TargetAddr { pub enum Addr { V4([u8; 4]), V6([u8; 16]), - Domain(String), // Vec<[u8]> or Box<[u8]> or String ? + // `String` — validated UTF-8 at the codec boundary (`read_address` + // converts before constructing). + Domain(String), } -/// This function is used by the client & the server +/// This function is used by the client & the server. +/// +/// The domain arm's allocation is bounded by construction: the wire +/// length field is one byte, so the claimed length is at most 255 — +/// the attacker-controlled byte cannot ask for more. Any change that +/// widens that field (an FRAG-style extension) requires an explicit +/// cap here before the allocation. pub async fn read_address( stream: &mut T, atyp: u8, diff --git a/tests/rfc1928-differential.rs b/tests/rfc1928-differential.rs index 61df6e5..79bd7fe 100644 --- a/tests/rfc1928-differential.rs +++ b/tests/rfc1928-differential.rs @@ -363,6 +363,39 @@ async fn command_domain_255_bytes_is_the_wire_maximum() { assert_eq!(target, TargetAddr::Domain("a".repeat(255), 443)); } +/// The decode-side bound is the wire's own 1-byte length field — +/// stated explicitly (review 001 S-1): `read_address`'s domain arm +/// allocates from an attacker-controlled byte, and the allocation is +/// safe *only* because the length field is one byte (≤ 255). This +/// test pins both faces of that invariant: (a) the 255-byte maximum +/// decodes through `read_address` (the attacker-side path), and (b) +/// the encoder rejects a 256-byte claim — a 256-byte domain cannot +/// enter the wire at all, so the decode arm's `len as usize` can +/// never ask for more than 255. Widening the length field (a +/// hypothetical FRAG-style extension) requires an explicit cap +/// before the allocation (the `read_address` doc note). +#[tokio::test] +async fn decode_side_domain_allocation_is_bounded_by_the_1_byte_wire_field() { + let maxed = { + let (mut client, mut server) = tokio::io::duplex(512); + client.write_all(&[0xff]).await.unwrap(); + client.write_all(&vec![b'a'; 255]).await.unwrap(); + client.write_all(&[0x00, 0x50]).await.unwrap(); + drop(client); + alksocks::rfc1928::util::target_addr::read_address(&mut server, 0x03) + .await + .unwrap() + }; + assert_eq!(maxed, TargetAddr::Domain("a".repeat(255), 80)); + // The 256-byte claim is impossible by construction: the encoder + // refuses it before any byte is emitted. + let over = TargetAddr::Domain("a".repeat(256), 80); + assert!(matches!( + over.to_be_bytes(), + Err(AddrError::DomainLenTooLong(256)) + )); +} + /// Truncated address fields (EOF mid-address) → typed /// `AddrError`-wrapped error, surfaced through `try_notify!`'s /// 0x01-coded secondary reply (AddrError non-ATYP variants map to @@ -417,6 +450,33 @@ async fn command_truncated_domain_content_is_typed_error() { } } +/// Version mismatch at the command phase (post-negotiation, the +/// 4-byte request header read) → typed refusal, no reply. Upstream +/// `read_command` checks `version` before parsing the address +/// (`src/server.rs:1010-1014`) — a distinct check from the greeting's +/// (`negotiate_auth`, `src/server.rs:889-892`), so this exercises the +/// command-phase arm in isolation. +#[tokio::test] +async fn command_version_mismatch_refuses_before_address_parse() { + let (mut client, server) = tokio::io::duplex(64); + let handle: ServerTask<()> = tokio::spawn(async move { + let proto = Socks5ServerProtocol::accept_no_auth(server).await?; + proto.read_command().await?; + Ok(()) + }); + client.write_all(&[0x05, 0x01, 0x00]).await.unwrap(); + let mut method_reply = [0u8; 2]; + client.read_exact(&mut method_reply).await.unwrap(); + assert_eq!(method_reply, [0x05, 0x00]); + client.write_all(&[0x06, 0x01, 0x00, 0x01]).await.unwrap(); + drop(client); + let err = handle.await.unwrap().unwrap_err(); + assert!(matches!( + err, + SocksServerError::UnsupportedSocksVersion(0x06) + )); +} + // --------------------------------------------------------------------------- // Group 3 — reply-form edge cases (the `new_reply` carried change's // regression guard) @@ -675,6 +735,105 @@ async fn client_udp_associate_zero_target_fallback_is_10_zero_bytes() { assert_eq!(expected.len(), 10); } +/// The client's v6 request encoding: ATYP=4, the full 16-byte +/// address, the 2-byte port, 22 bytes total — the `packet[4..20]`/ +/// `packet[20..22]` slice boundaries pinned byte-for-byte. Upstream +/// `request_header`'s v6 arm (`src/client.rs:293-300`); the in-tree +/// arm is byte-identical (`client.rs`, `TargetAddr::Ip +/// (SocketAddr::V6)`). The v6 *reply* form is pinned elsewhere +/// (`reply_v6_form_is_22_bytes`); this pins the request direction. +#[tokio::test] +async fn client_v6_request_encoding_is_22_bytes_with_atyp_4() { + let (mut mock, server) = tokio::io::duplex(64); + let handle: tokio::task::JoinHandle> = + tokio::spawn(async move { + let mut session = Socks5Stream::use_stream(server, None, Config::default()).await?; + session + .request( + Socks5Command::TCPConnect, + TargetAddr::Ip("[2001:db8::1]:443".parse().unwrap()), + ) + .await + }); + let mut greeting = [0u8; 3]; + mock.read_exact(&mut greeting).await.unwrap(); + assert_eq!(greeting, [0x05, 0x01, 0x00]); + mock.write_all(&[0x05, 0x00]).await.unwrap(); + let mut request = [0u8; 22]; + mock.read_exact(&mut request).await.unwrap(); + assert_eq!(&request[..4], &[0x05, 0x01, 0x00, 0x04]); + assert_eq!( + &request[4..20], + &0x2001_0db8_0000_0000_0000_0000_0000_0001u128.to_be_bytes() + ); + assert_eq!(&request[20..22], &443u16.to_be_bytes()); + // The reply form doubles as the request's echo target: v6 replies + // ride the same 22-byte shape the request just used. + mock.write_all(&[0x05, 0x00, 0x00, 0x04]).await.unwrap(); + mock.write_all(&0x2001_0db8_0000_0000_0000_0000_0000_0001u128.to_be_bytes()) + .await + .unwrap(); + mock.write_all(&443u16.to_be_bytes()).await.unwrap(); + let bind = handle.await.unwrap().unwrap(); + assert_eq!(bind, TargetAddr::Ip("[2001:db8::1]:443".parse().unwrap())); +} + +/// `Socks5Stream`'s `AsyncWrite::poll_shutdown` passthrough reaches +/// the backing stream: `shutdown()` closes the write side and the +/// mock observes EOF — the half-close propagation shape the consumer +/// half's two-pump teardown relies on (the EOF-sentinel convention). +/// The passthrough is a 3-line delegate (`client.rs` impl block); +/// without this test a regression in the `poll_shutdown` arm (e.g. +/// silently returning `Ready(Ok(()))` without forwarding) would pass +/// the whole suite. Duplex note: `poll_shutdown` on a `DuplexStream` +/// closes the whole pipe's write side for the pair (tokio +/// `io/util/mem.rs` `SimplexStream::poll_shutdown` → `close_write`), +/// so the mock's read side drains one final EOF after the byte. +#[tokio::test] +async fn client_session_shutdown_propagates_to_the_backing_stream() { + let (mut mock, server) = tokio::io::duplex(64); + let handle: tokio::task::JoinHandle> = tokio::spawn(async move { + let mut session = Socks5Stream::use_stream(server, None, Config::default()).await?; + session + .request( + Socks5Command::TCPConnect, + TargetAddr::Ip("10.0.0.1:80".parse().unwrap()), + ) + .await?; + session.flush().await?; + session.shutdown().await?; + // After the shutdown the write side is closed — a further + // write surfaces `BrokenPipe` (the passthrough's close + // semantics, tokio `DuplexStream`'s closed-writer shape). + match session.write_all(b"late").await { + Ok(()) => panic!("write after shutdown should not succeed"), + Err(err) => { + assert_eq!(err.kind(), std::io::ErrorKind::BrokenPipe) + } + } + Ok(()) + }); + let mut greeting = [0u8; 3]; + mock.read_exact(&mut greeting).await.unwrap(); + assert_eq!(greeting, [0x05, 0x01, 0x00]); + mock.write_all(&[0x05, 0x00]).await.unwrap(); + let mut request = [0u8; 10]; + mock.read_exact(&mut request).await.unwrap(); + assert_eq!(&request[..4], &[0x05, 0x01, 0x00, 0x01]); + mock.write_all(&[0x05, 0x00, 0x00, 0x01, 0, 0, 0, 0, 0, 0]) + .await + .unwrap(); + // The handshake's flush landed before the shutdown; the EOF read + // proves the write side is closed for the peer. + let mut eof = [0u8; 1]; + match mock.read_exact(&mut eof).await { + Ok(_) => panic!("expected EOF after shutdown"), + Err(err) if err.kind() == std::io::ErrorKind::UnexpectedEof => {} + Err(err) => panic!("expected UnexpectedEof, got {err:?}"), + } + handle.await.unwrap().unwrap(); +} + /// `skip_auth` writes no method-negotiation bytes: the first wire /// output is the request itself. Upstream: the `skip_auth` branch /// skips `send_version_and_methods`/`which_method_accepted` @@ -825,3 +984,34 @@ async fn wait_on_tcp_carries_control_garbage_byte() { SocksServerError::UnexpectedUdpControlGarbage(0xde) )); } + +/// `wait_on_tcp`'s io-error arm: the read *fails* (not EOF, not +/// data) → the io::Error is wrapped with the "waiting on UDP control +/// stream" context. Upstream `src/server.rs:1203` +/// (`Err(err) => Err(err).err_when("waiting on UDP control stream")`). +/// Driven by a real error producer: writing to a duplex whose peer is +/// gone surfaces `BrokenPipe` on the *write* side — for the read arm +/// the harness swaps in a failing reader so the `Err(err)` shape is +/// exercised deterministically (a dropped-peer duplex read is a clean +/// EOF, `Ok(0)`, which is `SocksServerError::EOF` — the other arm). +#[tokio::test] +async fn wait_on_tcp_wraps_io_errors_with_context() { + struct FailingReader; + impl tokio::io::AsyncRead for FailingReader { + fn poll_read( + self: std::pin::Pin<&mut Self>, + _cx: &mut std::task::Context<'_>, + _buf: &mut tokio::io::ReadBuf<'_>, + ) -> std::task::Poll> { + std::task::Poll::Ready(Err(std::io::Error::other("read side failure"))) + } + } + let mut stream = FailingReader; + let err = wait_on_tcp(&mut stream).await.unwrap_err(); + match err { + SocksServerError::Io { context, .. } => { + assert_eq!(context, "waiting on UDP control stream") + } + other => panic!("expected Io with context, got {other:?}"), + } +}