diff --git a/.github/workflows/squid-build-test.yml b/.github/workflows/squid-build-test.yml index c196331..3c9f49d 100644 --- a/.github/workflows/squid-build-test.yml +++ b/.github/workflows/squid-build-test.yml @@ -444,6 +444,140 @@ jobs: pkill microsocks 2>/dev/null || true pkill -f 'python3 -m http.server' 2>/dev/null || true + # ------------------------------------------------------------------ + # 4b. Test: SOCKS5 connection reuse / no cross-target routing + # Regression for codex P1/P2: a SOCKS tunnel bound to target X must + # never be reused for target Y, even with server pconns ENABLED. + # ------------------------------------------------------------------ + test-socks5-reuse: + name: Test SOCKS5 No Cross-Target Reuse + needs: build + runs-on: ubuntu-latest + timeout-minutes: 10 + steps: + - uses: actions/checkout@v4 + + - name: Download image artifact + uses: actions/download-artifact@v4 + with: + name: squid-image + path: /tmp + + - name: Load image + run: docker load -i /tmp/squid-image.tar + + - name: Start two distinct origin servers (A and B) + run: | + mkdir -p /tmp/wwwA /tmp/wwwB + echo 'ORIGIN-AAAA' > /tmp/wwwA/ip + echo 'ORIGIN-BBBB' > /tmp/wwwB/ip + python3 -m http.server 18090 --directory /tmp/wwwA & + python3 -m http.server 18091 --directory /tmp/wwwB & + sleep 1 + curl -sf http://127.0.0.1:18090/ip | grep -q ORIGIN-AAAA + curl -sf http://127.0.0.1:18091/ip | grep -q ORIGIN-BBBB + echo "--- Origins A/B ready ---" + + - name: Build and start SOCKS5 test server (microsocks) + run: | + if [ ! -f /tmp/microsocks/microsocks ]; then + git clone --depth 1 https://github.com/rofl0r/microsocks.git /tmp/microsocks + cd /tmp/microsocks && make -j"$(nproc)" + fi + /tmp/microsocks/microsocks -p 11090 & + sleep 1 + curl -sf --socks5-hostname 127.0.0.1:11090 http://127.0.0.1:18090/ip | grep -q ORIGIN-AAAA + curl -sf --socks5-hostname 127.0.0.1:11090 http://127.0.0.1:18091/ip | grep -q ORIGIN-BBBB + echo "--- SOCKS5 server reaches both origins ---" + + - name: Create Squid config with server pconns ENABLED + run: | + mkdir -p /tmp/squid-conf-reuse + cat > /tmp/squid-conf-reuse/squid.conf <&1 || true + exit 1 + fi + + - name: Wait for Squid to listen + run: | + for i in $(seq 1 30); do + if bash -c 'echo > /dev/tcp/127.0.0.1/3128' 2>/dev/null; then + echo "Squid is listening after ${i}s" + exit 0 + fi + sleep 1 + done + echo "ERROR: Squid never started listening" + docker logs squid-reuse 2>&1 || true + exit 1 + + - name: Alternate A/B requests and assert no cross-target reuse + run: | + fail=0 + for i in 1 2 3 4; do + RA=$(curl -s --max-time 15 -x http://127.0.0.1:3128 http://127.0.0.1:18090/ip || true) + RB=$(curl -s --max-time 15 -x http://127.0.0.1:3128 http://127.0.0.1:18091/ip || true) + echo "iter ${i}: A='${RA}' B='${RB}'" + echo "${RA}" | grep -q 'ORIGIN-AAAA' || { echo "FAIL: target A returned '${RA}'"; fail=1; } + echo "${RB}" | grep -q 'ORIGIN-BBBB' || { echo "FAIL: cross-target reuse! target B returned '${RB}'"; fail=1; } + done + echo "=== Squid logs ===" + docker logs squid-reuse 2>&1 | tail -40 || true + [ "${fail}" -eq 0 ] || { echo "--- Cross-target reuse detected ---"; exit 1; } + echo "--- No cross-target reuse: OK ---" + + - name: Post Squid logs to PR on failure + if: failure() + uses: actions/github-script@v7 + with: + script: | + const { execSync } = require('child_process'); + let logs = ''; + try { logs = execSync('docker logs squid-reuse 2>&1', {encoding: 'utf8', maxBuffer: 50*1024}); } catch(e) { logs = e.stdout || e.message; } + const body = `### Reuse Test Squid Logs\n\`\`\`\n${logs.slice(-3000)}\n\`\`\``; + const issueNumber = context.issue.number; + if (!issueNumber) { + console.log('No PR context; skipping comment.'); + } else { + await github.rest.issues.createComment({ + owner: context.repo.owner, + repo: context.repo.repo, + issue_number: issueNumber, + body: body + }); + } + + - name: Cleanup + if: always() + run: | + docker rm -f squid-reuse 2>/dev/null || true + pkill microsocks 2>/dev/null || true + pkill -f 'python3 -m http.server' 2>/dev/null || true + # ------------------------------------------------------------------ # 5. Test: generate.php produces correct config # ------------------------------------------------------------------ diff --git a/squid_patch/Dockerfile b/squid_patch/Dockerfile index d14cb4e..dfc95e3 100644 --- a/squid_patch/Dockerfile +++ b/squid_patch/Dockerfile @@ -30,13 +30,28 @@ RUN apt-get update && apt-get install -y --no-install-recommends \ WORKDIR /build -# Download and extract Squid source (pinned version + integrity check) +# Download and extract Squid source (pinned version + integrity check). +# Fetch from several mirrors with retries: the primary www.squid-cache.org +# host has intermittent outages. Integrity is guaranteed by the SHA256 check +# regardless of which mirror served the bytes, so mirror order is safe. ARG SQUID_TAR_SHA256=0b07b187e723f04770dd25beb89aec12030a158696aa8892d87c8b26853408a7 -RUN wget -q "https://www.squid-cache.org/Versions/v6/squid-${SQUID_VERSION}.tar.xz" \ - -O squid.tar.xz \ - && echo "${SQUID_TAR_SHA256} squid.tar.xz" | sha256sum -c - \ - && tar xf squid.tar.xz \ - && rm squid.tar.xz +RUN set -eux; \ + SQUID_TAG="SQUID_$(echo "${SQUID_VERSION}" | tr . _)"; \ + for url in \ + "https://github.com/squid-cache/squid/releases/download/${SQUID_TAG}/squid-${SQUID_VERSION}.tar.xz" \ + "https://www.squid-cache.org/Versions/v6/squid-${SQUID_VERSION}.tar.xz" \ + "http://www.squid-cache.org/Versions/v6/squid-${SQUID_VERSION}.tar.xz" \ + ; do \ + echo "Fetching Squid source: ${url}"; \ + if wget -q --tries=3 --timeout=30 -O squid.tar.xz "${url}"; then \ + echo "Downloaded from ${url}"; \ + break; \ + fi; \ + rm -f squid.tar.xz; \ + done; \ + echo "${SQUID_TAR_SHA256} squid.tar.xz" | sha256sum -c -; \ + tar xf squid.tar.xz; \ + rm squid.tar.xz # Copy patch sources and apply COPY src/ /patches/src/ diff --git a/squid_patch/patch_apply.sh b/squid_patch/patch_apply.sh index f2a9501..45c0cff 100755 --- a/squid_patch/patch_apply.sh +++ b/squid_patch/patch_apply.sh @@ -69,6 +69,70 @@ fi echo " CachePeer.cc patched OK" +# --------------------------------------------------------------------------- +# 1c. comm/Connection.h – add socksNegotiated flag (SOCKS anti-reuse guard) +# --------------------------------------------------------------------------- +# 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 otherwise be reused for a +# different target (silent mis-routing) or receive a second SOCKS greeting +# injected into a live stream. This per-connection flag lets the FwdState / +# tunnel hooks detect and refuse such reuse. +CONNECTION_H="${SQUID_SRC}/src/comm/Connection.h" +echo "==> Patching ${CONNECTION_H}" +[ -f "${CONNECTION_H}" ] || die "comm/Connection.h not found" + +if ! grep -q 'socksNegotiated' "${CONNECTION_H}"; then + python3 - "${CONNECTION_H}" << 'PYEOF' +import sys, re + +filepath = sys.argv[1] +with open(filepath, 'r') as f: + content = f.read() + +# Locate the *definition* of class Connection (skip forward declarations like +# "class Connection;"). A definition has an opening "{" before any ";". +match = None +brace_start = -1 +for mm in re.finditer(r'class\s+Connection\b', content): + rest = content[mm.end():] + brace = rest.find('{') + semi = rest.find(';') + if brace != -1 and (semi == -1 or brace < semi): + match = mm + brace_start = mm.end() + brace + break + +if match is None: + print("ERROR: class Connection definition not found in Connection.h", file=sys.stderr) + sys.exit(1) + +# Brace-match to find the closing "}" of the class body. +depth = 1 +pos = brace_start + 1 +while pos < len(content) and depth > 0: + if content[pos] == '{': depth += 1 + elif content[pos] == '}': depth -= 1 + pos += 1 + +# pos is right after the closing "}"; insert the field just before it. +field = ('\npublic:\n' + ' /* SOCKS peer support: true once SocksPeerConnector has\n' + ' * negotiated a tunnel on this fd. Prevents a pooled SOCKS\n' + ' * tunnel (bound to one target) from being reused for another. */\n' + ' bool socksNegotiated = false;\n') +insert_at = pos - 1 +content = content[:insert_at] + field + content[insert_at:] + +with open(filepath, 'w') as f: + f.write(content) +print(" Inserted socksNegotiated flag into class Connection") +PYEOF + grep -q 'socksNegotiated' "${CONNECTION_H}" || die "Failed to add socksNegotiated to Connection.h" +fi + +echo " comm/Connection.h patched OK" + # --------------------------------------------------------------------------- # 2. cache_cf.cc – parse socks4 / socks5 / socks-user= / socks-pass= # --------------------------------------------------------------------------- @@ -210,6 +274,27 @@ socks_hook = r''' /* SOCKS peer negotiation: after TCP connect, before HTTP dispatch */ if (const auto sp = serverConnection()->getPeer()) { if (sp->socks_type) { + /* Anti-reuse guard: a connection that has already been + * SOCKS-negotiated is a pooled tunnel bound to a *previous* + * target. Re-negotiating would inject a second greeting into a + * live stream, and using it as-is would silently mis-route this + * request. Drop it and let FwdState retry on a fresh fd. */ + if (serverConnection()->socksNegotiated) { + /* A reused, already-negotiated pconn is a tunnel bound to a + * previous target. Tear it down before retrying: noteConnection() + * has already set destinationReceipt and syncWithServerConn() has + * installed serverConn/closeHandler, so a bare retryOrBail() would + * re-enter noteConnection() with destinationReceipt still set and + * trip assert(!destinationReceipt). Mirror serverClosed(). */ + debugs(17, 2, "SOCKS: dropping reused negotiated connection to " + << sp->host << "; retrying on a fresh fd"); + closeServerConnection("reused SOCKS tunnel cannot serve a new target"); + serverConn = nullptr; + destinationReceipt = nullptr; + retryOrBail(); + return; + } + /* The SOCKS tunnel is bound to (request->url.host():port). * The pconn pool is keyed by peer address, NOT target, so a * pooled SOCKS-negotiated connection would silently route the @@ -231,9 +316,13 @@ socks_hook = r''' sp->socks_user ? std::string(sp->socks_user) : std::string(), sp->socks_pass ? std::string(sp->socks_pass) : std::string())) { debugs(17, 2, "SOCKS negotiation FAILED for peer " << sp->host); + closeServerConnection("SOCKS negotiation failed"); + serverConn = nullptr; + destinationReceipt = nullptr; retryOrBail(); return; } + serverConnection()->socksNegotiated = true; debugs(17, 3, "SOCKS negotiation OK for peer " << sp->host); } } @@ -299,6 +388,19 @@ socks_tunnel_hook = r''' /* SOCKS peer: negotiate tunnel right after TCP connect */ if (conn->getPeer() && conn->getPeer()->socks_type) { const auto sp = conn->getPeer(); + /* Anti-reuse guard: never re-negotiate (or reuse) a connection that + * already carries a SOCKS tunnel to a previous target. */ + if (conn->socksNegotiated) { + /* Reused tunnel connection already bound to a previous target: + * close the pending conn (server.conn is still nil here) before + * retrying, matching tunnel.cc's other error paths. */ + debugs(26, 2, "SOCKS: dropping reused negotiated tunnel connection to " + << sp->host << "; retrying"); + closePendingConnection(conn, "reused SOCKS tunnel cannot serve a new target"); + saveError(new ErrorState(ERR_CONNECT_FAIL, Http::scBadGateway, request.getRaw(), al)); + retryOrBail("SOCKS tunnel reuse"); + return; + } /* Same rationale as FwdState::dispatch(): the SOCKS tunnel is * bound to one target host, so prevent this connection from being * returned to the pconn pool where another request could pick it @@ -316,10 +418,12 @@ socks_tunnel_hook = r''' sp->socks_user ? std::string(sp->socks_user) : std::string(), sp->socks_pass ? std::string(sp->socks_pass) : std::string())) { debugs(26, 2, "SOCKS tunnel negotiation FAILED for " << sp->host); + closePendingConnection(conn, "SOCKS negotiation failed"); saveError(new ErrorState(ERR_CONNECT_FAIL, Http::scBadGateway, request.getRaw(), al)); retryOrBail("SOCKS negotiation failed"); return; } + conn->socksNegotiated = true; debugs(26, 3, "SOCKS tunnel negotiation OK for " << sp->host); } @@ -357,6 +461,7 @@ echo "" echo "Modified files:" echo " - src/CachePeer.h (added socks_type/user/pass fields)" echo " - src/CachePeer.cc (added socks_user/pass cleanup in destructor)" +echo " - src/comm/Connection.h (added socksNegotiated anti-reuse flag)" echo " - src/cache_cf.cc (added socks4/socks5 option parsing)" echo " - src/FwdState.cc (SOCKS negotiation in dispatch())" echo " - src/tunnel.cc (SOCKS negotiation in connectDone())"