Add SOCKS5 anti-reuse guard to prevent cross-target routing#34
Conversation
A SOCKS-negotiated cache_peer connection is a raw tunnel bound to one target (request->url.host():port). The persistent-connection pool is keyed by peer, not target, so a pooled SOCKS connection could be reused for a different target (silent mis-routing, codex P1) or receive a second SOCKS greeting injected into a live stream (codex P2). - Add a per-connection socksNegotiated flag to Comm::Connection via a new patch_apply.sh section (brace-matched insertion that skips the forward declaration and lands the field inside the class body). - FwdState::dispatch() and TunnelStateData::connectDone() hooks now refuse to reuse or re-negotiate an already-negotiated connection: they drop it and retry on a fresh fd, so a tunnel to target X can never serve target Y. proxyKeepalive=false and server_persistent_connections off are kept as defense-in-depth. - Add CI regression test (test-socks5-reuse) that runs with server_persistent_connections ON and alternates requests to two distinct origins through one SOCKS peer, asserting no cross-target routing. https://claude.ai/code/session_01Dzbzq3FqJHPNEsdNYjGQWn
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
CI build was failing at the tarball download step: www.squid-cache.org is currently returning HTTP 503 (wget exit 5/8). This is unrelated to the SOCKS patch and blocks every build (no test can run). Fetch from multiple mirrors with retries -- GitHub release first (which serves a byte-identical tarball; its SHA256 matches the existing pin), then www.squid-cache.org (https, then http) as fallback. The SHA256 integrity check is unchanged, so any mirror serving different bytes still fails the build; mirror order is therefore safe. https://claude.ai/code/session_01Dzbzq3FqJHPNEsdNYjGQWn
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 7abc3ce1ca
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if (serverConnection()->socksNegotiated) { | ||
| debugs(17, 2, "SOCKS: peer connection to " << sp->host | ||
| << " already negotiated; retrying on a fresh connection"); | ||
| retryOrBail(); |
There was a problem hiding this comment.
When this guard fires on a reused SOCKS pconn, FwdState::noteConnection() has already set destinationReceipt and syncWithServerConn() has installed serverConn/closeHandler before dispatch() runs. Calling retryOrBail() here without first closing/unregistering that fd leaves the negotiated tunnel attached, so the retry re-enters connection opening with destinationReceipt still set and the next noteConnection() can hit its assert(!destinationReceipt) instead of forcing the fresh fd promised by the comment. Please clear/drop the current server connection before retrying.
Useful? React with 👍 / 👎.
Codex flagged that the anti-reuse guard called retryOrBail() without dropping the current server connection. Verified against the Squid 6.10 source: at FwdState::dispatch() entry, noteConnection() has already set destinationReceipt and syncWithServerConn() installed serverConn / closeHandler, so a bare retryOrBail() re-enters noteConnection() with destinationReceipt still set and trips assert(!destinationReceipt). - FwdState reuse-guard and negotiation-failure branches now mirror serverClosed(): closeServerConnection() + serverConn=nullptr + destinationReceipt=nullptr before retryOrBail(). - tunnel.cc connectDone() reuse-guard and negotiation-failure branches now call closePendingConnection(conn, ...) before saveError()/retryOrBail(), matching every other tunnel.cc error path (retryOrBail asserts !server.conn, which holds at connectDone() entry). Verified by applying patch_apply.sh to the real squid-6.10 tree: patches apply cleanly, every referenced symbol (closeServerConnection, destinationReceipt, serverConn, closePendingConnection, saveError, retryOrBail) resolves to a real declaration, and brace balance is preserved. https://claude.ai/code/session_01Dzbzq3FqJHPNEsdNYjGQWn
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
39ff
merged commit dc95120
into
claude/squid-socks-proxy-support-CqoL5
2 participants
Summary
This PR adds a critical safety mechanism to prevent SOCKS peer connections from being reused across different target hosts. A SOCKS-negotiated tunnel is bound to a specific target (host:port), but Squid's persistent connection pool is keyed by peer address only. Without this guard, a pooled SOCKS connection could be silently reused for a different target, causing mis-routing or protocol violations.
Key Changes
comm/Connection.h: Added
socksNegotiatedboolean flag to track whether a connection has already completed SOCKS negotiation. This per-connection marker prevents reuse of tunnels bound to previous targets.FwdState.cc (dispatch hook): Added anti-reuse check that detects if a pooled connection was already SOCKS-negotiated. If so, the connection is dropped and FwdState retries on a fresh fd. Also sets the flag after successful SOCKS negotiation.
tunnel.cc (connectDone hook): Added similar anti-reuse guard for tunnel mode. Detects and rejects attempts to re-negotiate or reuse a connection that already carries a SOCKS tunnel. Sets the flag after successful tunnel negotiation.
squid-build-test.yml: Added comprehensive regression test (
test-socks5-reuse) that:server_persistent_connections onto enable connection poolingImplementation Details
The fix works by:
socksNegotiated = trueimmediately after successful SOCKS tunnel setupThis is a regression fix for P1/P2 codex issues where SOCKS connection reuse could cause silent mis-routing even with persistent connections enabled.
https://claude.ai/code/session_01Dzbzq3FqJHPNEsdNYjGQWn