Skip to content

Commit 10b34f0

Browse files
committed
Address review: gate socks auth to socks5, disable pconn for SOCKS peers
- generate.php: only emit socks-user/socks-pass for socks5 entries. The Squid patch rejects these options on socks4, so a socks4 line with credentials would break config parsing. - patch_apply.sh: set request->flags.proxyKeepalive=false on SOCKS peer requests in both FwdState::dispatch() and TunnelStateData:: connectDone(). Without this, the pconn pool (keyed by peer address, not target) could hand a SOCKS-negotiated connection to a request for a different target, silently routing data to the wrong host. https://claude.ai/code/session_01Cz4tkSZKDhTPnBx8YsYoyg
1 parent be2343b commit 10b34f0

2 files changed

Lines changed: 20 additions & 1 deletion

File tree

setup/generate.php

Lines changed: 6 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -81,12 +81,17 @@
8181
elseif(in_array($proxyInfo['scheme'], ['socks4', 'socks5'], true)){
8282
// Native SOCKS support via Squid cache_peer patch (no Gost needed)
8383
$socksOpt = $proxyInfo['scheme']; // "socks4" or "socks5"
84-
if ($proxyInfo['user'] && $proxyInfo['pass']) {
84+
// socks-user/socks-pass are only valid with socks5 (RFC1929).
85+
// The Squid patch rejects them on socks4, so emitting them there
86+
// would break config parsing. Skip and warn for socks4 entries.
87+
if ($proxyInfo['scheme'] === 'socks5' && $proxyInfo['user'] && $proxyInfo['pass']) {
8588
// SOCKS5 RFC1929 uses raw username/password; do not URL-encode.
8689
$socksOpt .= sprintf(' socks-user=%s socks-pass=%s',
8790
$proxyInfo['user'],
8891
$proxyInfo['pass']
8992
);
93+
} elseif ($proxyInfo['scheme'] === 'socks4' && ($proxyInfo['user'] || $proxyInfo['pass'])) {
94+
fwrite(STDERR, "Note: SOCKS4 credentials ignored (use socks5 for auth): " . $proxyInfo['host'] . PHP_EOL);
9095
}
9196
$squid_conf[] = sprintf($squid_socks,
9297
$proxyInfo['host'],

squid_patch/patch_apply.sh

Lines changed: 14 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -210,6 +210,15 @@ socks_hook = r'''
210210
/* SOCKS peer negotiation: after TCP connect, before HTTP dispatch */
211211
if (const auto sp = serverConnection()->getPeer()) {
212212
if (sp->socks_type) {
213+
/* The SOCKS tunnel is bound to (request->url.host():port).
214+
* The pconn pool is keyed by peer address, NOT target, so a
215+
* pooled SOCKS-negotiated connection would silently route the
216+
* next request to the WRONG destination. Force the upstream
217+
* connection to close after this request to keep one tunnel
218+
* per target, and to guarantee the next dispatch() runs on a
219+
* freshly-connected fd that has not been SOCKS-negotiated yet. */
220+
request->flags.proxyKeepalive = false;
221+
213222
const auto targetPort = static_cast<uint16_t>(request->url.port());
214223
debugs(17, 3, "SOCKS" << sp->socks_type
215224
<< " negotiation with peer " << sp->host
@@ -290,6 +299,11 @@ socks_tunnel_hook = r'''
290299
/* SOCKS peer: negotiate tunnel right after TCP connect */
291300
if (conn->getPeer() && conn->getPeer()->socks_type) {
292301
const auto sp = conn->getPeer();
302+
/* Same rationale as FwdState::dispatch(): the SOCKS tunnel is
303+
* bound to one target host, so prevent this connection from being
304+
* returned to the pconn pool where another request could pick it
305+
* up and silently send data into the previous target's tunnel. */
306+
request->flags.proxyKeepalive = false;
293307
const auto targetPort = static_cast<uint16_t>(request->url.port());
294308
debugs(26, 3, "SOCKS" << sp->socks_type
295309
<< " tunnel negotiation with peer " << sp->host

0 commit comments

Comments
 (0)