Skip to content

Comments

feat: unify API proxy sidecar into Squid proxy container#1026

Closed
Mossaka wants to merge 1 commit intomainfrom
worktree-unified-proxy
Closed

feat: unify API proxy sidecar into Squid proxy container#1026
Mossaka wants to merge 1 commit intomainfrom
worktree-unified-proxy

Conversation

@Mossaka
Copy link
Collaborator

@Mossaka Mossaka commented Feb 25, 2026

Summary

  • Merge the standalone API proxy sidecar (containers/api-proxy/) into the Squid proxy container, reducing container count from 3→2 when --enable-api-proxy is used
  • Node.js auth proxy now runs alongside Squid in a single container, eliminating the extra network hop and simplifying architecture
  • All security properties maintained: credential isolation, domain filtering, header stripping, container hardening

Architecture Change

Before (3 containers):

Agent → API Proxy (172.30.0.30) → Squid (172.30.0.10) → Internet

After (2 containers):

Agent → Unified Proxy (172.30.0.10, Squid + Node.js auth) → Internet

Security Review

Security review performed with CONDITIONAL PASS — all 3 required mitigations implemented:

Check Result
iptables & Network Filtering ✅ PASS
Squid Proxy Configuration ✅ PASS
Container Security ✅ PASS (mitigations applied)
Domain Pattern Security ✅ PASS
General Security ✅ PASS
Credential Isolation ✅ CONDITIONAL PASS

Mitigations implemented:

  1. Node.js auth proxy runs as non-root proxy user (not root)
  2. Squid container hardened with no-new-privileges, memory/PID limits
  3. Proper signal handling for both processes (no exec, proper wait)

Files Changed

  • containers/squid/ — Added Node.js, server.js, package.json, updated Dockerfile & entrypoint
  • containers/api-proxy/Deleted (merged into squid)
  • src/docker-manager.ts — Removed api-proxy service, unified into squid service
  • src/host-iptables.ts — Updated apiProxy handling (boolean flag, not separate IP)
  • src/cli-workflow.ts — Updated apiProxy configuration
  • containers/agent/setup-iptables.sh — Removed AWF_API_PROXY_IP handling
  • scripts/ci/cleanup.sh — Removed api-proxy container references
  • docs/api-proxy-sidecar.md — Updated architecture documentation
  • Test files updated to reflect unified architecture

gh-aw Integration

No changes required in gh-aw CLI. The --enable-api-proxy flag and all related CLI options work unchanged. Agent environment variables (*_BASE_URL) now point to squid IP (172.30.0.10) instead of separate proxy IP (172.30.0.30), which is transparent to agent code.

Test plan

  • Unit tests pass (npm test)
  • TypeScript builds (npm run build)
  • Lint passes (npm run lint)
  • Integration tests with --enable-api-proxy flag
  • Smoke test with Claude Code (Anthropic key)
  • Smoke test with Codex (OpenAI key)
  • Verify credential isolation (agent can't read API keys)

🤖 Generated with Claude Code

Merge the standalone API proxy sidecar (containers/api-proxy/) into the
Squid proxy container, reducing the container count from 3 to 2 when
--enable-api-proxy is used.

The Node.js auth proxy now runs inside the Squid container alongside
Squid itself. This eliminates the extra container, network hop, and
simplifies the architecture while maintaining all security properties:
- Credential isolation (API keys never reach agent container)
- Domain filtering (all LLM traffic still passes through Squid ACLs)
- Header stripping (client auth headers removed before injection)
- Container hardening (no-new-privileges, resource limits, non-root)

Key changes:
- Move server.js, package.json into containers/squid/
- Update Dockerfile to install Node.js and auth proxy deps
- Update entrypoint.sh to start both Squid and Node.js (non-root)
- Remove api-proxy Docker Compose service from docker-manager.ts
- Add security hardening to Squid service (no-new-privileges, limits)
- Update iptables rules (no separate proxy IP needed)
- Update agent BASE_URL env vars to point to squid IP (172.30.0.10)
- Delete containers/api-proxy/ directory

Security review: CONDITIONAL PASS (all mitigations implemented)
- Node.js runs as non-root 'proxy' user
- Squid container gets no-new-privileges and resource limits
- Proper signal handling for both processes

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Copilot AI review requested due to automatic review settings February 25, 2026 00:29
@github-actions
Copy link
Contributor

✅ Coverage Check Passed

Overall Coverage

Metric Base PR Delta
Lines 82.30% 82.42% 📈 +0.12%
Statements 82.23% 82.35% 📈 +0.12%
Functions 82.74% 82.74% ➡️ +0.00%
Branches 74.46% 74.55% 📈 +0.09%
📁 Per-file Coverage Changes (1 files)
File Lines (Before → After) Statements (Before → After)
src/docker-manager.ts 83.2% → 83.7% (+0.46%) 82.5% → 82.9% (+0.45%)

Coverage comparison generated by scripts/ci/compare-coverage.ts

@github-actions
Copy link
Contributor

Go Build Test Results

Project Download Tests Status
color 1/1 PASS
env 1/1 PASS
uuid 1/1 PASS

Overall: PASS

Generated by Build Test Go for issue #1026

@github-actions
Copy link
Contributor

🤖 Smoke test results for run 22376346760@Mossaka

✅ GitHub MCP: Last 2 merged PRs — #1025 "docs: add sandbox design rationale (Docker vs microVMs)", #992 "docs: update runner and architecture compatibility"
✅ Playwright: github.com title contains "GitHub"
✅ File write: /tmp/gh-aw/agent/smoke-test-copilot-22376346760.txt created and verified
✅ Bash: cat confirmed file contents

Overall: PASS

📰 BREAKING: Report filed by Smoke Copilot for issue #1026

@github-actions
Copy link
Contributor

C++ Build Test Results

Project CMake Build Status
fmt PASS
json PASS

Overall: PASS

Generated by Build Test C++ for issue #1026

@github-actions
Copy link
Contributor

Build Test: Node.js Results

Project Install Tests Status
clsx PASS PASS
execa PASS PASS
p-limit PASS PASS

Overall: ✅ PASS

Generated by Build Test Node.js for issue #1026

@github-actions
Copy link
Contributor

.NET Build Test Results

Project Restore Build Run Status
hello-world PASS
json-parse PASS

Overall: PASS

Run outputs

hello-world: Hello, World!

json-parse:

{
  "Name": "AWF Test",
  "Version": 1,
  "Success": true
}
Name: AWF Test, Success: True

Generated by Build Test .NET for issue #1026

@github-actions
Copy link
Contributor

🦀 Build Test: Rust

Project Build Tests Status
fd 1/1 PASS
zoxide 1/1 PASS

Overall: ✅ PASS

Generated by Build Test Rust for issue #1026

@github-actions
Copy link
Contributor

Smoke Test Results — PASS

✅ GitHub MCP: #1025 docs: add sandbox design rationale (Docker vs microVMs) | #991 feat(docker): pre-seed Maven/Gradle/sbt proxy config in agent container
✅ Playwright: github.com title contains "GitHub"
✅ File write: /tmp/gh-aw/agent/smoke-test-claude-22376346766.txt created
✅ Bash verify: file content confirmed

💥 [THE END] — Illustrated by Smoke Claude for issue #1026

@github-actions
Copy link
Contributor

Build Test: Bun ✅

Project Install Tests Status
elysia 1/1 PASS
hono 1/1 PASS

Overall: PASS

Bun version: 1.3.9

Generated by Build Test Bun for issue #1026

@github-actions
Copy link
Contributor

Build Test: Deno Results

Project Tests Status
oak 1/1 ✅ PASS
std 1/1 ✅ PASS

Overall: ✅ PASS

Generated by Build Test Deno for issue #1026

@github-actions
Copy link
Contributor

Java Build Test Results

Project Compile Tests Status
gson 1/1 PASS
caffeine 1/1 PASS

Overall: PASS

All Maven projects compiled and all tests passed successfully.

Generated by Build Test Java for issue #1026

@github-actions
Copy link
Contributor

GitHub MCP merged PRs: docs: add sandbox design rationale (Docker vs microVMs); feat(docker): pre-seed Maven/Gradle/sbt proxy config in agent container
safeinputs-gh PRs: feat: unify API proxy sidecar into Squid proxy container; [Test Coverage] test: expand host-iptables branch coverage
GitHub MCP merged PRs ✅
safeinputs-gh list ✅
Playwright title ✅
Tavily search ❌
File write ✅ | File read ✅
Discussion comment ✅
Build ✅
Overall: FAIL

🔮 The oracle has spoken through Smoke Codex for issue #1026

@github-actions
Copy link
Contributor

Chroot Version Comparison Results

Runtime Host Version Chroot Version Match?
Python Python 3.12.12 Python 3.12.3 ❌ No
Node.js v24.13.0 v20.20.0 ❌ No
Go go1.22.12 go1.22.12 ✅ Yes

Result: Some versions differ between host and chroot. Python and Node.js versions do not match (chroot uses older versions). Go matches. smoke-chroot label not applied.

Tested by Smoke Chroot for issue #1026

Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR consolidates the Node.js “API auth proxy” into the existing Squid proxy container, removing the standalone api-proxy sidecar so --enable-api-proxy runs with 2 containers instead of 3 while keeping credential isolation and domain filtering via Squid.

Changes:

  • Embed the Node.js auth proxy into containers/squid (Dockerfile, entrypoint, Node app + deps).
  • Update runtime orchestration and firewall rules to target Squid IP/ports directly (no separate proxy IP/service).
  • Refresh tests + docs to reflect the unified architecture and new routing.

Reviewed changes

Copilot reviewed 13 out of 16 changed files in this pull request and generated 5 comments.

Show a summary per file
File Description
tests/integration/api-proxy.test.ts Updates integration tests to target the unified proxy at 172.30.0.10 instead of the sidecar IP.
src/host-iptables.ts Removes proxyIp handling and adds optional allow-rule for auth-proxy ports on Squid IP.
src/host-iptables.test.ts Updates expectations to match the new fixed network config (no proxyIp).
src/docker-manager.ts Removes api-proxy service, injects keys into Squid service, mounts auth-proxy logs dir, and updates healthcheck + env wiring.
src/docker-manager.test.ts Updates compose-generation tests for the new “embedded proxy” behavior and env vars.
src/cli-workflow.ts Switches workflow dependency types and passes a boolean “apiProxyEnabled” instead of an IP.
scripts/ci/cleanup.sh Ensures cleanup removes legacy awf-api-proxy container name (backward compatibility).
docs/api-proxy-sidecar.md Re-documents the feature as unified proxy architecture and updates diagrams/env var expectations.
containers/squid/server.js Adds the Node.js auth proxy implementation (OpenAI/Anthropic/Copilot) into the squid image.
containers/squid/package.json Adds npm metadata + dependency for the embedded Node proxy.
containers/squid/package-lock.json Locks https-proxy-agent dependency tree for reproducible image builds.
containers/squid/entrypoint.sh Starts Node proxy alongside Squid and adds multi-process signal handling.
containers/squid/Dockerfile Installs Node.js/npm and copies/builds the embedded auth proxy into the Squid image.
containers/agent/setup-iptables.sh Removes sidecar-specific bypass rules (agent now reaches auth proxy on Squid IP).
containers/api-proxy/README.md Deletes obsolete sidecar container docs after merge into squid.
containers/api-proxy/Dockerfile Deletes obsolete sidecar Dockerfile after merge into squid.
Comments suppressed due to low confidence (1)

src/docker-manager.ts:1087

  • The code now creates/mounts /var/log/api-proxy (and mentions the auth proxy "still writes logs separately"), but containers/squid/server.js only logs to stdout/stderr and nothing redirects output into that directory. Either wire the Node process output to a file in /var/log/api-proxy (or add file logging), or drop the extra host directory creation + volume mount to avoid unused writable paths.
  // Create api-proxy logs directory for persistence
  // The auth proxy now runs inside the Squid container but still writes logs separately
  // If proxyLogsDir is specified, write to sibling directory (timeout-safe)
  // Otherwise, write to workDir/api-proxy-logs (will be moved to /tmp after cleanup)
  const apiProxyLogsDir = config.proxyLogsDir
    ? path.join(path.dirname(config.proxyLogsDir), 'api-proxy-logs')
    : path.join(config.workDir, 'api-proxy-logs');
  if (!fs.existsSync(apiProxyLogsDir)) {
    fs.mkdirSync(apiProxyLogsDir, { recursive: true, mode: 0o777 });
    // Explicitly set permissions to 0o777 (not affected by umask)
    fs.chmodSync(apiProxyLogsDir, 0o777);
  }
  logger.debug(`API proxy logs directory created at: ${apiProxyLogsDir}`);

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment on lines +301 to +322
// When API proxy is enabled, add API keys and auth proxy config to the Squid container
// The auth proxy Node.js server runs alongside Squid in the same container
if (config.enableApiProxy) {
// Pass API keys as environment variables to the unified Squid container
squidService.environment = {
...(config.openaiApiKey && { OPENAI_API_KEY: config.openaiApiKey }),
...(config.anthropicApiKey && { ANTHROPIC_API_KEY: config.anthropicApiKey }),
...(config.copilotGithubToken && { COPILOT_GITHUB_TOKEN: config.copilotGithubToken }),
};

// Mount API proxy log directory
squidVolumes.push(`${apiProxyLogsPath}:/var/log/api-proxy:rw`);

// Update healthcheck to verify both Squid AND the auth proxy are running
squidService.healthcheck = {
test: ['CMD-SHELL', `nc -z localhost 3128 && curl -sf http://localhost:${API_PROXY_HEALTH_PORT}/health`],
interval: '5s',
timeout: '3s',
retries: 5,
start_period: '10s',
};
}
Copy link

Copilot AI Feb 25, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When --enable-api-proxy is set but no provider keys are present, generateDockerCompose still switches Squid to the combined healthcheck (expects :10000/health), but containers/squid/entrypoint.sh only starts the Node auth proxy when keys exist. This makes squid-proxy permanently unhealthy and blocks the agent via depends_on.

Adjust the condition so the combined healthcheck (and/or auth-proxy startup) is enabled only when at least one key is configured, or pass an explicit flag env var to the container and always start a minimal health-only proxy when enabled.

Copilot uses AI. Check for mistakes.
Comment on lines +407 to 417
// When API proxy is enabled, bypass HTTP_PROXY for the Squid IP
// so the agent can reach the auth proxy ports (10000-10002) directly
// The auth proxy is now embedded in the Squid container at the same IP
if (config.enableApiProxy) {
if (environment.NO_PROXY) {
environment.NO_PROXY += `,${networkConfig.proxyIp}`;
environment.NO_PROXY += `,${networkConfig.squidIp}`;
} else {
environment.NO_PROXY = `localhost,127.0.0.1,${networkConfig.proxyIp}`;
environment.NO_PROXY = `localhost,127.0.0.1,${networkConfig.squidIp}`;
}
environment.no_proxy = environment.NO_PROXY;
}
Copy link

Copilot AI Feb 25, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This NO_PROXY update can introduce duplicates (e.g., --enable-host-access already adds networkConfig.squidIp, then --enable-api-proxy appends it again). Consider normalizing/deduplicating the comma-separated list before assigning NO_PROXY/no_proxy to keep env output stable and avoid surprising diffs.

See below for a potential fix:

    const baseNoProxy = environment.NO_PROXY || 'localhost,127.0.0.1';
    const entries = baseNoProxy
      .split(',')
      .map((entry) => entry.trim())
      .filter((entry) => entry.length > 0);
    if (!entries.includes(networkConfig.squidIp)) {
      entries.push(networkConfig.squidIp);
    }
    environment.NO_PROXY = entries.join(',');

Copilot uses AI. Check for mistakes.
Comment on lines +27 to +30
# Fix permissions on api-proxy log directory
chown -R proxy:proxy /var/log/api-proxy
chmod -R 755 /var/log/api-proxy

Copy link

Copilot AI Feb 25, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

chmod -R 755 /var/log/api-proxy makes the mounted log directory world-readable. Since these logs may contain sensitive request metadata, it would be safer to restrict permissions (e.g., 750/700) and rely on proxy:proxy ownership for access.

Copilot uses AI. Check for mistakes.
Comment on lines +36 to +40
su -s /bin/sh proxy -c "HTTP_PROXY='$HTTP_PROXY' HTTPS_PROXY='$HTTPS_PROXY' \
OPENAI_API_KEY='${OPENAI_API_KEY:-}' \
ANTHROPIC_API_KEY='${ANTHROPIC_API_KEY:-}' \
COPILOT_GITHUB_TOKEN='${COPILOT_GITHUB_TOKEN:-}' \
node /app/api-proxy/server.js" &
Copy link

Copilot AI Feb 25, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The su ... -c "... OPENAI_API_KEY='${OPENAI_API_KEY:-}' ..." pattern embeds credentials into the shell command line. That can leak secrets via process listings and is also fragile if values ever contain quotes/shell metacharacters.

Prefer exporting the env vars and using su with environment preservation (or a tool like gosu/setpriv) so secrets stay in the process environment rather than argv/command text.

Suggested change
su -s /bin/sh proxy -c "HTTP_PROXY='$HTTP_PROXY' HTTPS_PROXY='$HTTPS_PROXY' \
OPENAI_API_KEY='${OPENAI_API_KEY:-}' \
ANTHROPIC_API_KEY='${ANTHROPIC_API_KEY:-}' \
COPILOT_GITHUB_TOKEN='${COPILOT_GITHUB_TOKEN:-}' \
node /app/api-proxy/server.js" &
# Preserve environment so proxy and API key variables are available to Node.js
su -m -s /bin/sh proxy -c "node /app/api-proxy/server.js" &

Copilot uses AI. Check for mistakes.
Comment on lines +443 to 455
// 5b. Allow traffic to API auth proxy ports (when enabled)
// The auth proxy is embedded in the Squid container, so allow ports 10000-10002 to Squid IP.
// The auth proxy routes through Squid internally, so domain whitelisting is still enforced.
if (apiProxyEnabled) {
const minPort = Math.min(API_PROXY_PORTS.OPENAI, API_PROXY_PORTS.ANTHROPIC, API_PROXY_PORTS.COPILOT);
const maxPort = Math.max(API_PROXY_PORTS.OPENAI, API_PROXY_PORTS.ANTHROPIC, API_PROXY_PORTS.COPILOT);
logger.debug(`Allowing traffic to API proxy sidecar at ${apiProxyIp}:${minPort}-${maxPort}`);
logger.debug(`Allowing traffic to API auth proxy at ${squidIp}:${minPort}-${maxPort}`);
await execa('iptables', [
'-t', 'filter', '-A', CHAIN_NAME,
'-p', 'tcp', '-d', apiProxyIp, '--dport', `${minPort}:${maxPort}`,
'-p', 'tcp', '-d', squidIp, '--dport', `${minPort}:${maxPort}`,
'-j', 'ACCEPT',
]);
}
Copy link

Copilot AI Feb 25, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

setupHostIptables now has a new code path to allow agent traffic to the auth-proxy ports (10000-10002) when apiProxyEnabled is true, but there’s no unit test asserting the expected iptables call(s) are added for that flag.

Add a focused test that calls setupHostIptables(..., true) and verifies the ACCEPT rule for the ${minPort}:${maxPort} range to squidIp.

Copilot uses AI. Check for mistakes.
@Mossaka Mossaka closed this Feb 25, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant