socks5: add an idle timeout to UDP associations - #1678
Open
eidos-case wants to merge 1 commit into
Open
Conversation
Every SOCKS5 UDP association binds an ephemeral port for its relay socket and holds it until the control TCP connection closes. UDP has no FIN or RST, so a client that opens many associations and never closes their control connections pins one port per association indefinitely, until the whole ephemeral range is gone and every process on the host fails to open a socket. Server gains UDPTimeout, zero meaning 60s. The read deadline slides forward on activity in both directions, and the resulting timeout is treated as an ordinary end of life rather than an error, so the event log is not filled with routine expiries. This mirrors UDPTProxy.updateConnDeadline rather than introducing a different mechanism: tproxy, tun and forwarding all already have this timeout, and socks5 was the only entry point without one. Exposed as socks5.timeout, spelled the way udpTProxyConfig, udpRedirectConfig and tunConfig already spell the same knob. Three neighbouring reads are bounded too, each able to pin a goroutine and a descriptor for the lifetime of the process when a client connects and then stays silent: the proxymux protocol sniff, the SOCKS5 handshake, and the HTTP request header read. Each deadline is cleared the moment the header is in, so none of them ever reaches a request body or proxied traffic. In http the first header read and the wait between keep-alive requests get different bounds, 10s and 60s. A connection that opens and never speaks is not a client waiting to be served; a keep-alive connection that goes quiet is ordinary, and browsers hold those for tens of seconds. Tests: TestUDPAssociationsDoNotAccumulate and TestUDPAssociationIdleTimeout both fail on unpatched code with the deadline refresh removed as the only variable. TestUDPAssociationSurvivesTraffic passes without the fix, and must: it is the control against this being a hard lifetime cap. client_test.yaml and client_test.go gain the new config field, without which the existing config-coverage check fails; mux_test.go gains a SetReadDeadline expectation, without which its strict mock rejects the new call.
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.
Fixes #1677.
socks5:ServergainsUDPTimeout time.Duration(zero → 60s, matching theother entry points). The read deadline slides forward on activity in both
directions, and the resulting timeout is treated as an ordinary end of life
rather than an error, so the event log is not filled with routine expiries.
Exposed in the client config as
socks5.timeout, spelled the wayudpTProxyConfig,udpRedirectConfigandtunConfigalready spell the sameknob —
tunConfigincluded, which likewise carries both TCP and UDP and stillcalls it
timeout. Internally the field isUDPTimeout, matching whattun/server.goalready passes down. This mirrorsUDPTProxy.updateConnDeadlinerather than introducing a different mechanism.Three neighbouring reads are bounded, each of which can pin a goroutine and a
file descriptor for the lifetime of the process when a client connects and
then stays silent:
app/internal/proxymux/mux.go,dispatch()— the one-byte protocol sniff;app/internal/socks5/server.go,dispatch()— the SOCKS5 handshake;app/internal/http/server.go,dispatch()— the request header read.Each deadline is cleared once the header is in, so none of them ever applies
to a request body or to proxied traffic, which is legitimately long-lived and
idle. That distinction is the same one fix: plain HTTP requests through the HTTP proxy cut off after 10 seconds #1676 has just drawn in this very file,
replacing a whole-request
TimeoutwithResponseHeaderTimeout; this changeis deliberately on that side of it.
In
httpthe first header read and the wait between keep-alive requests aregiven different bounds — 10s and 60s. A connection that opens and never speaks
is not a client waiting to be served; a keep-alive connection that goes quiet
is ordinary, and browsers hold those for tens of seconds, so a single 10s
bound would trade one leak for a lot of needless reconnects.
Tests:
TestUDPAssociationsDoNotAccumulate,TestUDPAssociationIdleTimeout,TestUDPAssociationSurvivesTraffic.The first two were verified to fail on unpatched code — with the deadline refresh
removed as the only variable, they report
all idle associations must release their portsandidle association must release its ephemeral port. So theymeasure the change rather than merely accompanying it.
The third passes without the fix, and must: it asserts that an association
carrying traffic is not torn down. It is a control against the fix being a hard
lifetime cap, so a version of it that failed on unpatched code would be testing
the wrong thing.
app/internal/proxymux/mux_test.gogains aSetReadDeadlineexpectation on itsmock
net.Conn; without it the strict mock rejects the new call. On this machinethe full
app/...suite has the same set of failures before and after the change(
http.TestServer,proxymux× 3,socks5.TestServer,utils. TestCertificateLoaderFullChain), all of which are the macOS inability to bindaddresses other than
127.0.0.1.