fix(proxy): SOCKS5 upstream auth reliability#455
Open
liasica wants to merge 1 commit into
Open
Conversation
…iable Two independent root causes were producing auth failures on the upstream SOCKS5 dial path: 1. `url::Url::username()` / `Url::password()` return percent-encoded strings per the WHATWG URL spec, but the producer side already percent-encodes the credentials when assembling the upstream URL — so the upstream was receiving `%40` instead of `@` and authentication silently failed for any credential containing `@ : / + = % ! space` or non-ASCII characters. Centralize the decode in a new `upstream_userpass` helper and route all four upstream-dial sites through it (HTTP CONNECT → SOCKS5, HTTP CONNECT → HTTP Basic-Auth, local SOCKS5 → HTTP Basic-Auth, local SOCKS5 → SOCKS5). The Shadowsocks path already decoded manually and is unchanged. 2. async_socks5 0.6 issues a `write_u8` for every single-byte field of the SOCKS5 method-selection and RFC1929 sub-negotiation. On a raw `TcpStream` each call becomes its own TCP segment, and some upstream SOCKS5 implementations treat this fragmented submission as a misbehaving client and silently FIN instead of returning a status — curl with the same credentials succeeds because it buffers each sub-message into a single send(). Wrap the upstream socket in `tokio::io::BufStream` (the usage pattern the async_socks5 README shows) and enable TCP_NODELAY so flushes leave unsegmented. Includes unit tests covering percent-decode for ASCII / special-char / non-ASCII / no-credentials / username-only inputs, plus a trace-level SOCKS5 handshake byte logger that can be enabled with RUST_LOG=donutbrowser_lib::proxy_server=trace for future debugging.
Owner
|
Hi there and thanks for the PR!
|
Author
@zhom Hi zhom, thanks for reviewing this PR.
I used Claude and ChatGPT — mainly to help me read through
Reproducing on
Both failure modes are gone on this branch. Tests I ran:
|
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
Two independent root causes were producing intermittent / per-credential SOCKS5 upstream authentication failures on the worker dial path. Both addressed in this PR.
url::Url::username()/Url::password()return percent-encoded strings per WHATWG, butproxy_manager::build_proxy_urlandbin/proxy_server::build_proxy_urlalready percent-encode the user/pass when assembling the upstream URL. The upstream therefore received%40instead of@for any credential containing@ : / + = % ! spaceor non-ASCII — RFC1929 (and HTTP Basic-Auth) silently failed. Centralized the decode in a newupstream_userpasshelper and routed all four upstream-dial sites through it. Shadowsocks already decoded manually and is unchanged.async_socks5handshake triggered silent FIN on some upstreams.async_socks50.6 callswrite_u8for every single-byte field of method-selection and RFC1929. On a rawTcpStreameach call becomes its own TCP segment, and some upstream SOCKS5 implementations treat that as a misbehaving client and silently FIN instead of returning a status — curl with the same credentials works because it buffers each sub-message into a singlesend(). Wrapped the upstream socket intokio::io::BufStream(the pattern the async_socks5 README shows) and enabledTCP_NODELAYso flushes leave unsegmented.Also adds a trace-level
SocksHandshakeLogger(off by default, enable withRUST_LOG=donutbrowser_lib::proxy_server=trace) that dumps every read/write byte of the SOCKS5 handshake — used to root-cause #2 and kept around for future debugging.Test plan
pnpm format && pnpm lint && pnpm test— all green (lib 373 + 14 + 15 + 15)upstream_userpass: plain ASCII /@ : / + = % ! space/ non-ASCII / no-credentials / username-only — all passERR_SOCKS_CONNECTION_FAILED(CONNECT tunnel ended with error: unexpected end of file) now succeeds through Wayfern; verified browser fetchesapi.ipify.orgover the upstreamRUST_LOG=donutbrowser_lib::proxy_server=traceconfirms the handshake now sends method-selection and RFC1929 each in a single coalesced syscall, and that the server returns[01, 00]before the CONNECT command is issued