Repository navigation
socks: track SOCKS5 authentication per socket, not per connection - #23014
MegaManSec wants to merge 2 commits into
Conversation
The bit that CURLAUTH_ONLY uses to refuse reuse of an unauthenticated connection was set by any SOCKS5 filter on the connection. The FTP data connection negotiates its own tunnel, so a proxy answering "no auth" for the control connection and an authenticated method for the data one left the bit set on a control connection that never authenticated. Only the primary socket decides, and the bit is cleared when it starts a new negotiation. Add test 1745, and a socksd "method2" keyword to answer the second connection differently.
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
The process-global connection counter prevents the regression test from reliably exercising its intended authentication sequence.
Get a fresh assessment by requesting another Copilot review.
Review effort: Balanced
Findings: 1
What changed in this PR
Tracks SOCKS5 authentication only for the primary reusable socket and adds an FTP regression test.
Changes:
- Gates authentication state updates by socket index.
- Clears stale authentication state when renegotiating.
- Adds
method2support and test 1745.
| File | Description |
|---|---|
lib/socks.c |
Scopes authentication tracking to FIRSTSOCKET. |
lib/urldata.h |
Clarifies authentication flag semantics. |
tests/server/socksd.c |
Adds second-connection method configuration. |
tests/libtest/lib1745.c |
Implements the FTP regression scenario. |
tests/libtest/Makefile.inc |
Registers the libtest source. |
tests/data/test1745 |
Defines expected regression behavior. |
tests/data/Makefile.am |
Registers test 1745. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
|
Analysis of PR #23014 at 30ef17c0: Test ../../tests/http/test_07_upload.py::TestUpload::test_07_13_upload_seq_large[http/1.1] failed, which has NOT been flaky recently, so there could be a real issue in this PR. Note that this test has failed in 2 different CI jobs (the link just goes to one of them). Generated by Testclutch |
30ef17c to
a1925f5
Compare
Follow-up to #23003.
conn->bits.socks5_authenticatedis written by any SOCKS5 filter on the connection, but only the primary socket's tunnel is what gets reused. FTP negotiates a separate tunnel for the data connection, so a proxy that answers method 0 for the control connection and method 2 for the data connection leaves the bit set on a control connection that never authenticated. A laterCURLAUTH_ONLYtransfer then passes the reuse check inurl_match_proxy_use()and keeps using that tunnel. The bit was also never cleared, so it survived a re-negotiation that selected no authentication.Gate both writes on
cf->sockindex == FIRSTSOCKETand clear the bit when the primary socket starts a negotiation, so it always reflects the tunnel that reuse is deciding about.Test 1745 does two FTP transfers over one handle: the proxy leaves the control connection unauthenticated and authenticates the data connection, then the second transfer sets
CURLAUTH_ONLY. Without the fix the reused control connection carries the second transfer's commands. It needs a new socksdmethod2keyword, since the contamination requires the proxy to answer the two tunnels differently.Note this is an incomplete guarantee in the new option rather than a regression: before #23003
CURLAUTH_ONLYwas rejected by setopt for this option, and the guard only ever refuses reuse, so no configuration reuses a connection it would not have reused before.