Redesign cli-proxy: connect to external DIFC proxy started by compiler#1809
Redesign cli-proxy: connect to external DIFC proxy started by compiler#1809
Conversation
Replace internal mcpg container management with external DIFC proxy connection. The gh-aw compiler now starts mcpg on the host; AWF only launches the cli-proxy container and connects via TCP tunnel. New CLI flags: - --difc-proxy-host <host:port>: connect to external DIFC proxy - --difc-proxy-ca-cert <path>: TLS CA cert from DIFC proxy Removed CLI flags: - --enable-cli-proxy - --cli-proxy-writable - --cli-proxy-policy - --cli-proxy-mcpg-image Key changes: - Remove mcpg service from Docker Compose generation - cli-proxy gets its own IP (172.30.0.50) with extra_hosts - TCP tunnel forwards localhost to host DIFC proxy for TLS - Write control delegated to DIFC guard policy - Remove cli-proxy-tls named volume (CA cert bind-mounted) - Update all tests (1374 pass) Agent-Logs-Url: https://github.com/github/gh-aw-firewall/sessions/a38ec447-a23c-4104-be85-bd7328c48559
- Add port validation and error logging to tcp-tunnel.js - Make NODE_EXTRA_CA_CERTS conditional on cert existence - Fix CLI help text for --difc-proxy-ca-cert Agent-Logs-Url: https://github.com/github/gh-aw-firewall/sessions/a38ec447-a23c-4104-be85-bd7328c48559
There was a problem hiding this comment.
Pull request overview
Redesigns AWF’s cli-proxy sidecar to connect to an externally started DIFC proxy (mcpg) rather than managing an internal cli-proxy-mcpg service, with a TCP tunnel to preserve TLS hostname validation against localhost.
Changes:
- Replaces internal mcpg sidecar orchestration with
--difc-proxy-host/--difc-proxy-ca-certand updates config/types accordingly. - Adds a
tcp-tunnel.jsforwarder and updatescli-proxyentrypoint/server behavior for the external-proxy model. - Updates unit/integration tests and docs to reflect the new architecture; removes mcpg image from predownload logic.
Show a summary per file
| File | Description |
|---|---|
| tests/integration/cli-proxy.test.ts | Updates integration tests to assume an external DIFC proxy and validates compose/container setup. |
| tests/fixtures/awf-runner.ts | Replaces --enable-cli-proxy plumbing with DIFC proxy flags in the test runner. |
| src/types.ts | Replaces cli-proxy config fields with difcProxyHost / difcProxyCaCert and updates docs. |
| src/docker-manager.ts | Removes internal mcpg service generation; adds external DIFC proxy wiring and CA cert bind-mount. |
| src/docker-manager.test.ts | Updates compose generation tests for external DIFC proxy mode. |
| src/commands/predownload.ts | Stops predownloading the mcpg image (now external). |
| src/commands/predownload.test.ts | Updates predownload tests to match the new image set. |
| src/cli.ts | Replaces runtime CLI flags and updates status logging; keeps predownload flags. |
| src/cli.test.ts | Updates status log tests for the new flags. |
| docs/gh-cli-proxy-design.md | Documents the new external DIFC proxy architecture and TLS tunneling approach. |
| containers/cli-proxy/tcp-tunnel.js | Adds new TCP tunnel used for localhost TLS SAN matching. |
| containers/cli-proxy/server.test.js | Updates server tests to reflect removal of read-only enforcement and focus on meta-command denial. |
| containers/cli-proxy/server.js | Removes read-only/writable allowlists and relies on DIFC guard policy + meta-command denial. |
| containers/cli-proxy/entrypoint.sh | Starts the TCP tunnel and configures GH_HOST / CA cert behavior for the external proxy. |
| containers/cli-proxy/Dockerfile | Includes the new tunnel script and updates comments for external DIFC proxy mode. |
Copilot's findings
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 15/15 changed files
- Comments generated: 6
src/docker-manager.ts
Outdated
| // Parse host:port from difcProxyHost | ||
| const hostParts = config.difcProxyHost.split(':'); | ||
| const difcProxyHost = hostParts[0] || 'host.docker.internal'; | ||
| const difcProxyPort = hostParts[1] || '18443'; | ||
|
|
||
| // Build the guard policy for the mcpg proxy | ||
| let guardPolicy = config.cliProxyPolicy || ''; | ||
| if (!guardPolicy) { | ||
| const repo = process.env.GITHUB_REPOSITORY; | ||
| guardPolicy = repo | ||
| ? `{"repos":["${repo}"],"min-integrity":"public"}` | ||
| : '{"min-integrity":"public"}'; | ||
| } | ||
|
|
||
| // --- mcpg DIFC proxy service (runs the official gh-aw-mcpg image directly) --- | ||
| const mcpgService: any = { | ||
| container_name: CLI_PROXY_MCPG_CONTAINER_NAME, | ||
| image: mcpgImage, | ||
| // Override entrypoint+command to run in proxy mode (matches gh-aw start_difc_proxy.sh) | ||
| entrypoint: ['/app/run.sh'], | ||
| command: [ | ||
| 'proxy', | ||
| '--policy', guardPolicy, | ||
| // Bind to localhost only — cli-proxy shares this container's network | ||
| // namespace (network_mode: service:cli-proxy-mcpg), so it can reach | ||
| // mcpg via 127.0.0.1. No other container on awf-net can connect. | ||
| '--listen', `127.0.0.1:${mcpgPort}`, | ||
| '--tls', | ||
| '--tls-dir', '/tmp/proxy-tls', | ||
| '--guards-mode', 'filter', | ||
| '--trusted-bots', 'github-actions[bot],github-actions,dependabot[bot],copilot', | ||
| '--log-dir', '/var/log/cli-proxy/mcpg', | ||
| ], | ||
| // --- CLI proxy HTTP server (Node.js + gh CLI) --- | ||
| // Connects to external DIFC proxy via TCP tunnel for TLS hostname matching. | ||
| // The TCP tunnel forwards localhost:${difcProxyPort} → ${difcProxyHost}:${difcProxyPort} | ||
| // so that gh CLI's GH_HOST=localhost:${difcProxyPort} matches the cert's SAN. |
There was a problem hiding this comment.
When difcProxyHost is enabled, cli-proxy needs to open a TCP connection to host.docker.internal:<port>, but the host firewall rules only allow host-gateway traffic when host access is enabled (and non-80/443 ports require allowHostServicePorts). With the current compose generation, --difc-proxy-host will likely be unreachable unless users also pass --enable-host-access + --allow-host-service-ports <difcPort>. Consider auto-enabling host access and adding the parsed DIFC port to the host-gateway allowlist when difcProxyHost is set, or fail fast with a clear error if these flags aren’t provided.
src/docker-manager.ts
Outdated
| // Parse host:port from difcProxyHost | ||
| const hostParts = config.difcProxyHost.split(':'); | ||
| const difcProxyHost = hostParts[0] || 'host.docker.internal'; | ||
| const difcProxyPort = hostParts[1] || '18443'; |
There was a problem hiding this comment.
difcProxyHost parsing via split(':') will break for IPv6 (colons), bracketed IPv6 ([::1]:18443), or inputs that include a scheme. It would be safer to use a proper host:port parser (e.g., URL parsing with defaults, supporting [host]:port), and validate that the port is numeric before injecting it into container env.
| // Parse host:port from difcProxyHost | |
| const hostParts = config.difcProxyHost.split(':'); | |
| const difcProxyHost = hostParts[0] || 'host.docker.internal'; | |
| const difcProxyPort = hostParts[1] || '18443'; | |
| const parseDifcProxyHost = (value: string): { host: string; port: string } => { | |
| const trimmedValue = value.trim(); | |
| if (!trimmedValue) { | |
| return { | |
| host: 'host.docker.internal', | |
| port: '18443', | |
| }; | |
| } | |
| const hasScheme = /^[a-zA-Z][a-zA-Z\d+\-.]*:\/\//.test(trimmedValue); | |
| const candidate = hasScheme ? trimmedValue : `tcp://${trimmedValue}`; | |
| let parsed: URL; | |
| try { | |
| parsed = new URL(candidate); | |
| } catch { | |
| throw new Error(`Invalid difcProxyHost: ${value}`); | |
| } | |
| const host = parsed.hostname || 'host.docker.internal'; | |
| const port = parsed.port || '18443'; | |
| if (!/^\d+$/.test(port)) { | |
| throw new Error(`Invalid difcProxyHost port: ${port}`); | |
| } | |
| const portNumber = Number(port); | |
| if (!Number.isInteger(portNumber) || portNumber < 1 || portNumber > 65535) { | |
| throw new Error(`Invalid difcProxyHost port: ${port}`); | |
| } | |
| return { | |
| host, | |
| port: String(portNumber), | |
| }; | |
| }; | |
| // Parse host:port from difcProxyHost | |
| const { host: difcProxyHost, port: difcProxyPort } = parseDifcProxyHost(config.difcProxyHost); |
| @@ -87,13 +35,14 @@ const ALWAYS_DENIED_SUBCOMMANDS = new Set([ | |||
| ]); | |||
|
|
|||
There was a problem hiding this comment.
The current meta-command denylist is very small. In particular, gh alias should be denied as well: gh aliases can be defined with shell execution (e.g., !cmd), which allows arbitrary command execution inside the cli-proxy container (bypassing the intent of routing through the DIFC proxy). Add alias to ALWAYS_DENIED_SUBCOMMANDS (and consider other gh subcommands that can execute local programs).
containers/cli-proxy/entrypoint.sh
Outdated
| @@ -31,26 +37,33 @@ while [ $i -lt 30 ]; do | |||
| done | |||
|
|
|||
| if [ ! -f /tmp/proxy-tls/ca.crt ]; then | |||
| echo "[cli-proxy] ERROR: mcpg TLS certificate not found within 30s" | |||
| exit 1 | |||
| echo "[cli-proxy] WARNING: DIFC proxy TLS certificate not found within 30s, continuing without it" | |||
| fi | |||
|
|
|||
| # Configure gh CLI to route through the mcpg proxy (TLS, self-signed CA) | |||
| # Uses localhost because cli-proxy shares mcpg's network namespace — the | |||
| # self-signed cert's SAN covers localhost, so TLS hostname verification passes. | |||
| export GH_HOST="localhost:${MCPG_PORT}" | |||
| export NODE_EXTRA_CA_CERTS="/tmp/proxy-tls/ca.crt" | |||
| # Configure gh CLI to route through the DIFC proxy via the TCP tunnel | |||
| # Uses localhost because the tunnel makes the DIFC proxy appear on localhost, | |||
| # matching the self-signed cert's SAN. | |||
| export GH_HOST="localhost:${DIFC_PORT}" | |||
There was a problem hiding this comment.
If /tmp/proxy-tls/ca.crt is missing, the container currently logs a warning and continues. Since the DIFC proxy is described as using a self-signed cert, subsequent gh calls will likely fail TLS verification and the sidecar will appear healthy while being non-functional. Consider failing fast when --difc-proxy-host is set but the CA cert isn’t mounted, or add an explicit opt-out flag/env to allow running without TLS verification.
containers/cli-proxy/tcp-tunnel.js
Outdated
| net.createServer(client => { | ||
| const upstream = net.connect(remotePort, remoteHost); | ||
| client.pipe(upstream); | ||
| upstream.pipe(client); | ||
| client.on('error', (err) => { console.error('[tcp-tunnel] Client error:', err.message); upstream.destroy(); }); | ||
| upstream.on('error', (err) => { console.error('[tcp-tunnel] Upstream error:', err.message); client.destroy(); }); | ||
| }).listen(localPort, '127.0.0.1', () => { |
There was a problem hiding this comment.
tcp-tunnel.js doesn’t attach an error handler to the listening server. If the local port can’t be bound (already in use, permission issues), Node will emit an error event and terminate with a stack trace. Add server.on('error', …) (and ideally log + exit 1) so failures are reported cleanly by the entrypoint.
| net.createServer(client => { | |
| const upstream = net.connect(remotePort, remoteHost); | |
| client.pipe(upstream); | |
| upstream.pipe(client); | |
| client.on('error', (err) => { console.error('[tcp-tunnel] Client error:', err.message); upstream.destroy(); }); | |
| upstream.on('error', (err) => { console.error('[tcp-tunnel] Upstream error:', err.message); client.destroy(); }); | |
| }).listen(localPort, '127.0.0.1', () => { | |
| const server = net.createServer(client => { | |
| const upstream = net.connect(remotePort, remoteHost); | |
| client.pipe(upstream); | |
| upstream.pipe(client); | |
| client.on('error', (err) => { console.error('[tcp-tunnel] Client error:', err.message); upstream.destroy(); }); | |
| upstream.on('error', (err) => { console.error('[tcp-tunnel] Upstream error:', err.message); client.destroy(); }); | |
| }); | |
| server.on('error', (err) => { | |
| console.error('[tcp-tunnel] Server error:', err.message); | |
| process.exit(1); | |
| }); | |
| server.listen(localPort, '127.0.0.1', () => { |
| ) | ||
| .option('--enable-api-proxy', 'Also download the API proxy image', false) | ||
| .option('--enable-cli-proxy', 'Also download the CLI proxy image', false) | ||
| .option('--cli-proxy-mcpg-image <image>', 'Docker image for the mcpg DIFC proxy container', 'ghcr.io/github/gh-aw-mcpg:v0.2.15') | ||
| .action(handlePredownloadAction); |
There was a problem hiding this comment.
PR description says --enable-cli-proxy is replaced by --difc-proxy-host, but the predownload subcommand still exposes --enable-cli-proxy. If this is intentional (predownload-only), consider renaming it or clarifying the help text to avoid implying --enable-cli-proxy still enables the runtime CLI proxy feature.
|
@copilot apply changes based on the comments in this thread |
- Use URL-based parser for difcProxyHost (IPv6 support)
- Add host iptables rule for cli-proxy → DIFC proxy port
- Add 'alias' to ALWAYS_DENIED_SUBCOMMANDS (shell exec)
- Fail fast when CA cert not found in entrypoint.sh
- Add server.on('error') handler in tcp-tunnel.js
- Rename --enable-cli-proxy to --difc-proxy in predownload
Agent-Logs-Url: https://github.com/github/gh-aw-firewall/sessions/11e4f4ef-6a07-4318-9e6f-aba436bd9590
Applied all 6 review comments in commits
|
The CA cert is now a bind mount (not generated at runtime by mcpg), so it's immediately available at container start. Replace the 30s polling loop with a single existence check for faster startup. Also update stale comments referencing --enable-cli-proxy to --difc-proxy-host in setup-iptables.sh and gh-cli-proxy-wrapper.sh. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
lpcox
left a comment
lpcox
left a comment
There was a problem hiding this comment.
Review: PR #1809 — Redesign cli-proxy to connect to external DIFC proxy
Overall this is a clean, well-structured refactor. The old mcpg-internal architecture (~170 lines of compose service, guard policy generation, GH_TOKEN validation, healthcheck, named TLS volume) is cleanly replaced with a much simpler external DIFC proxy model. Tests are comprehensive. A few items I fixed in commit b387976:
Fixes committed
-
Removed unnecessary 30s CA cert polling loop (
entrypoint.sh): The old architecture needed a wait loop because mcpg generated the cert at runtime in a shared Docker volume. With the new architecture, the cert is bind-mounted from the host (already created by the compiler), so it's immediately available at container start. Replaced with a single existence check for faster startup and clearer failure messages. -
Updated stale
--enable-cli-proxycomments insetup-iptables.sh(line 177) andgh-cli-proxy-wrapper.sh(line 5) — these still referenced the old flag name.
What looks good
parseDifcProxyHost()properly extracted to module-level exported function. Handles IPv6 ([::1]:18443), missing port (defaults 18443), scheme stripping, and validation. Good test coverage.tcp-tunnel.jsis minimal and correct — validates ports, proper bidirectional piping, clean error handling withclient.destroy()/upstream.destroy().- Token exclusion (
GITHUB_TOKEN/GH_TOKENremoved from agent env) correctly gates onconfig.difcProxyHostinstead ofconfig.enableCliProxy. - Host iptables rule (5b2) correctly positioned before DROP rules, scoped to cli-proxy IP only, and handles both gateway IPs.
server.jssimplification — removed all write-mode logic (WRITABLE_MODE,ALLOWED_SUBCOMMANDS_READONLY,BLOCKED_ACTIONS_READONLY), leaving only meta-command denial. Write control delegated to DIFC guard policy as designed.- 17 cli-proxy tests in
docker-manager.test.tscover enablement, IP assignment, CA cert mounting, env vars, IPv6 parsing, error cases, healthcheck, cap_drop, and depends_on chain.
Notes (not blocking)
containers/cli-proxy/server.test.jsis outside Jest's configured roots (src/,scripts/), so it never runs duringnpm test. This is pre-existing (same on main), not a regression from this PR. Worth fixing separately.docs/gh-cli-proxy-design.mdstill has old architecture references in the<details>block — this is intentional (historical context), just noting for clarity.- The 3 test failures in
docker-manager.test.ts(getRealUserHome,writeConfigs chroot subdirs) are pre-existing on main, not caused by this PR.
|
| Metric | Base | PR | Delta |
|---|---|---|---|
| Lines | 86.22% | 85.95% | 📉 -0.27% |
| Statements | 86.08% | 85.82% | 📉 -0.26% |
| Functions | 87.50% | 87.54% | 📈 +0.04% |
| Branches | 78.81% | 78.50% | 📉 -0.31% |
📁 Per-file Coverage Changes (4 files)
| File | Lines (Before → After) | Statements (Before → After) |
|---|---|---|
src/cli-workflow.ts |
100.0% → 92.0% (-8.00%) | 100.0% → 92.0% (-8.00%) |
src/host-iptables.ts |
91.9% → 88.7% (-3.19%) | 91.6% → 88.5% (-3.14%) |
src/docker-manager.ts |
86.6% → 86.6% (+0.05%) | 86.0% → 86.1% (+0.05%) |
src/cli.ts |
62.1% → 62.2% (+0.17%) | 62.6% → 62.8% (+0.17%) |
Coverage comparison generated by scripts/ci/compare-coverage.ts
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
Co-authored-by: Copilot Autofix powered by AI <62310815+github-advanced-security[bot]@users.noreply.github.com>
|
| Metric | Base | PR | Delta |
|---|---|---|---|
| Lines | 86.22% | 85.95% | 📉 -0.27% |
| Statements | 86.08% | 85.82% | 📉 -0.26% |
| Functions | 87.50% | 87.54% | 📈 +0.04% |
| Branches | 78.81% | 78.50% | 📉 -0.31% |
📁 Per-file Coverage Changes (4 files)
| File | Lines (Before → After) | Statements (Before → After) |
|---|---|---|
src/cli-workflow.ts |
100.0% → 92.0% (-8.00%) | 100.0% → 92.0% (-8.00%) |
src/host-iptables.ts |
91.9% → 88.7% (-3.19%) | 91.6% → 88.5% (-3.14%) |
src/docker-manager.ts |
86.6% → 86.6% (+0.05%) | 86.0% → 86.1% (+0.05%) |
src/cli.ts |
62.1% → 62.2% (+0.17%) | 62.6% → 62.8% (+0.17%) |
Coverage comparison generated by scripts/ci/compare-coverage.ts
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
Smoke Test: GitHub Actions Services Connectivity ✅All checks passed:
|
Chroot Version Comparison Results
Result: ❌ Not all tests passed — Python and Node.js versions differ between host and chroot environments.
|
🏗️ Build Test Suite Results
Overall: 8/8 ecosystems passed — ✅ PASS
|
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
|
🔮 The ancient spirits stir in the firewall halls.
|
🔥 Smoke Test Results
Overall: PASS —
|
|
Smoke test results (run 24166690630)
Overall: PASS
|
AWF's internal mcpg container consistently crashes (exit code 1) due to
cap_drop: ALL,pids_limit: 50, containerized entrypoint redirection, and Squid proxy interference. The gh-aw compiler already runs mcpg successfully with--network hostand no restrictions. Split responsibilities: compiler starts mcpg on the host, AWF just connects to it.New CLI flags
--enable-cli-proxy--difc-proxy-host <host:port>--cli-proxy-writable--cli-proxy-policy <json>--cli-proxy-mcpg-image <img>--difc-proxy-ca-cert <path>--difc-proxy(predownload subcommand)Architecture
The DIFC proxy's TLS cert only has SANs for
localhost/127.0.0.1. A newtcp-tunnel.jsforwardslocalhost:18443inside cli-proxy tohost.docker.internal:18443, soGH_HOST=localhost:18443matches the cert.Key changes
src/docker-manager.ts— Remove entire mcpg service block (~170 lines),cli-proxy-tlsnamed volume, guard policy generation, GH_TOKEN validation. cli-proxy gets its own IP on awf-net withextra_hosts: ['host.docker.internal:host-gateway']and bind-mounted CA cert. Export sharedparseDifcProxyHost()utility with URL-based parsing (supports IPv6 bracketed notation, scheme prefixes, port validation).src/host-iptables.ts— AddCliProxyHostConfiginterface and iptables rules to allow the cli-proxy container (172.30.0.50) to reach the host gateway on the DIFC proxy port. Without this, the default deny rule would block cli-proxy →host.docker.internal:<difcPort>traffic.src/cli-workflow.ts— PassCliProxyHostConfigtosetupHostIptableswhendifcProxyHostis set, using sharedparseDifcProxyHost()to extract the port.src/types.ts— ReplaceenableCliProxy,cliProxyWritable,cliProxyPolicy,cliProxyMcpgImagewithdifcProxyHost,difcProxyCaCert.src/cli.ts— Replace 4 CLI options with 2, update config passthrough andemitCliProxyStatusLogs.containers/cli-proxy/tcp-tunnel.js— New ~50-line Node.js TCP forwarder with port validation, error logging, andserver.on('error')handler for clean failure on port bind issues.containers/cli-proxy/entrypoint.sh— Start TCP tunnel, fail fast (exit 1) when CA cert is not found, setGH_HOSTandNODE_EXTRA_CA_CERTSvia tunnel.containers/cli-proxy/server.js— RemoveWRITABLE_MODE,ALLOWED_SUBCOMMANDS_READONLY,BLOCKED_ACTIONS_READONLY. Meta-commands (alias,auth,config,extension) are denied —aliasis included becausegh alias set 'x' '!cmd'allows arbitrary shell execution. Write control is the DIFC guard policy's job.src/commands/predownload.ts— Remove mcpg image from predownload (compiler manages it). Rename--enable-cli-proxyto--difc-proxyfor consistent naming.