Gate forward SOCKS5 with --allow-socks (wire: add from_socks) (v0.6.1) - #38
Merged
Conversation
Per review: --allow-socks should cover both forward AND reverse SOCKS5, not just reverse. Today forward `socks` decomposes into per-target dynamic Tcp/Udp remotes on the wire (manufactured by the client's SOCKS handler on every CONNECT / UDP ASSOCIATE target), so the server has no way to tell them apart from regular forwards and can't gate them. Fix this by stamping a wire-level `from_socks: bool` on every dynamic remote produced by `RemoteRequest::dynamic_tcp` / `dynamic_udp`. `is_socks()` now returns `true` for either `kind == Socks5` (reverse `R:socks`) or `from_socks == true` (forward `socks`), so the existing control-plane gate fires for both directions. Reverse SOCKS5 (`R:socks`) keeps requiring `--allow-reverse` on top of `--allow-socks`, matching the user-facing semantics. This is a breaking wire change — same precedent as 0.4.0; clients and servers must upgrade together. New regression tests in tests/edge_cases.rs: - test_forward_socks_rejected_when_not_allowed: client SOCKS listener comes up but the per-CONNECT dynamic stream is rejected by the server's --allow-socks gate, so the SOCKS5 reply is non-success. - test_reverse_socks_requires_both_flags: --allow-reverse without --allow-socks still rejects R:socks. Co-authored-by: Cursor <cursoragent@cursor.com>
Co-authored-by: Cursor <cursoragent@cursor.com>
Co-authored-by: Cursor <cursoragent@cursor.com>
Co-authored-by: Cursor <cursoragent@cursor.com>
Co-authored-by: Cursor <cursoragent@cursor.com>
Co-authored-by: Cursor <cursoragent@cursor.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Follow-up to #37: extend the
--allow-socksserver gate to cover forward SOCKS5 in addition to reverse SOCKS5. Previously the flag only blockedR:socksbecauseRemoteKind::Socks5was the only SOCKS-typed thing on the wire — forwardsocksdecomposes into per-target dynamicTcp/Udpremotes that the server couldn't distinguish from regular forwards.Fix this by stamping a wire-level
from_socks: boolon every dynamic remote produced byRemoteRequest::dynamic_tcp/dynamic_udp.is_socks()now returnstruefor eitherkind == Socks5(reverseR:socks) orfrom_socks == true(forwardsocks), so the existing control-plane gate fires for both directions. Reverse SOCKS5 keeps requiring--allow-reverseon top of--allow-socks, soR:socksneeds both flags.Resulting semantics
socks(forward)R:socks(reverse)--allow-socks--allow-reverseBreaking changes
RemoteRequestgainsfrom_socks: bool(#[serde(default)]). Same precedent as 0.4.0 — clients and servers must upgrade together.sockswithout an explicit flag must add--allow-socksto the server invocation when upgrading from 0.6.0.Test plan
test_forward_socks_rejected_when_not_allowedintests/edge_cases.rs: client SOCKS listener still binds (the SOCKS handshake is purely client-side), but the per-CONNECT dynamic stream is rejected by the server's--allow-socksgate, so the SOCKS5 reply is non-success.test_reverse_socks_requires_both_flags:--allow-reversewithout--allow-socksstill rejectsR:socks.allow_socks=trueso SOCKS-using tests don't all need to opt in; newstart_tunnel_with_flagshelper for tests that exercise the deny path).cargo test --allgreen;cargo fmt --checkgreen;cargo clippy --all -- -D warningsgreen.0.6.1; CHANGELOG0.6.1section added (the merged0.6.0entry is preserved verbatim).Notes for downstream embedders
RemoteRequestgainspub from_socks: bool(#[serde(default)]). Hand-builtRemoteRequest { ... }literals must add the field; users ofRemoteRequest::new/RemoteRequest::from_str/dynamic_tcp/dynamic_udpare unaffected.Made with Cursor