fix(proxy): add lowercase proxy vars and NODE_EXTRA_CA_CERTS#1234
fix(proxy): add lowercase proxy vars and NODE_EXTRA_CA_CERTS#1234
Conversation
✅ Coverage Check PassedOverall Coverage
📁 Per-file Coverage Changes (1 files)
Coverage comparison generated by |
There was a problem hiding this comment.
Pull request overview
This PR updates the container environment setup to improve Node/Yarn/Corepack behavior when running behind the Squid proxy, addressing SSL/proxy handling issues that can surface as EPROTO errors.
Changes:
- Add lowercase
http_proxy/https_proxyalongside uppercase proxy env vars. - Set
NODE_EXTRA_CA_CERTSwhen SSL Bump is enabled so Node-based tooling trusts the session CA. - Add unit tests covering lowercase proxy vars and
NODE_EXTRA_CA_CERTSbehavior.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| src/docker-manager.ts | Adds lowercase proxy vars and sets NODE_EXTRA_CA_CERTS under SSL Bump; also propagates lowercase proxy vars to the api-proxy sidecar. |
| src/docker-manager.test.ts | Adds unit tests validating lowercase proxy vars and NODE_EXTRA_CA_CERTS behavior. |
| containers/agent/entrypoint.sh | Exports NODE_EXTRA_CA_CERTS during SSL Bump startup after updating CA certificates. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| HTTP_PROXY: `http://${networkConfig.squidIp}:${SQUID_PORT}`, | ||
| HTTPS_PROXY: `http://${networkConfig.squidIp}:${SQUID_PORT}`, | ||
| http_proxy: `http://${networkConfig.squidIp}:${SQUID_PORT}`, | ||
| https_proxy: `http://${networkConfig.squidIp}:${SQUID_PORT}`, |
There was a problem hiding this comment.
The api-proxy sidecar healthcheck uses curl http://localhost:..., but adding lowercase http_proxy/https_proxy here will cause curl to proxy localhost unless NO_PROXY/no_proxy is set (or the healthcheck explicitly disables proxying). This can make the api-proxy container permanently unhealthy and block agent startup via depends_on: service_healthy. Consider adding NO_PROXY=no_proxy=localhost,127.0.0.1,::1 to the api-proxy environment (or change the healthcheck to curl --noproxy '*' ...).
| https_proxy: `http://${networkConfig.squidIp}:${SQUID_PORT}`, | |
| https_proxy: `http://${networkConfig.squidIp}:${SQUID_PORT}`, | |
| // Ensure localhost traffic (e.g., healthcheck) bypasses the proxy | |
| NO_PROXY: 'localhost,127.0.0.1,::1', | |
| no_proxy: 'localhost,127.0.0.1,::1', |
| // Route through Squid to respect domain whitelisting | ||
| HTTP_PROXY: `http://${networkConfig.squidIp}:${SQUID_PORT}`, | ||
| HTTPS_PROXY: `http://${networkConfig.squidIp}:${SQUID_PORT}`, | ||
| http_proxy: `http://${networkConfig.squidIp}:${SQUID_PORT}`, | ||
| https_proxy: `http://${networkConfig.squidIp}:${SQUID_PORT}`, |
There was a problem hiding this comment.
When SSL Bump is enabled, Squid presents certificates signed by the session CA for all clients using the proxy port. The api-proxy sidecar routes upstream requests through Squid (HttpsProxyAgent(HTTPS_PROXY)), but the compose spec does not mount the CA cert into the api-proxy container or set NODE_EXTRA_CA_CERTS there, so upstream TLS requests are likely to fail in --ssl-bump --enable-api-proxy mode. Consider mounting the CA cert into the api-proxy container and setting NODE_EXTRA_CA_CERTS (or otherwise exempting api-proxy traffic from SSL bump).
| // Route through Squid to respect domain whitelisting | |
| HTTP_PROXY: `http://${networkConfig.squidIp}:${SQUID_PORT}`, | |
| HTTPS_PROXY: `http://${networkConfig.squidIp}:${SQUID_PORT}`, | |
| http_proxy: `http://${networkConfig.squidIp}:${SQUID_PORT}`, | |
| https_proxy: `http://${networkConfig.squidIp}:${SQUID_PORT}`, | |
| // Route through Squid to respect domain whitelisting when SSL bump is disabled. | |
| // When SSL bump is enabled, avoid proxying api-proxy HTTPS traffic through Squid | |
| // to prevent TLS failures due to untrusted session CA certificates. | |
| ...(!config.enableSslBump && { | |
| HTTP_PROXY: `http://${networkConfig.squidIp}:${SQUID_PORT}`, | |
| HTTPS_PROXY: `http://${networkConfig.squidIp}:${SQUID_PORT}`, | |
| http_proxy: `http://${networkConfig.squidIp}:${SQUID_PORT}`, | |
| https_proxy: `http://${networkConfig.squidIp}:${SQUID_PORT}`, | |
| }), |
| HTTP_PROXY: `http://${networkConfig.squidIp}:${SQUID_PORT}`, | ||
| HTTPS_PROXY: `http://${networkConfig.squidIp}:${SQUID_PORT}`, | ||
| // Lowercase variants for tools that only check lowercase (e.g., Yarn 4/undici, Corepack) | ||
| http_proxy: `http://${networkConfig.squidIp}:${SQUID_PORT}`, | ||
| https_proxy: `http://${networkConfig.squidIp}:${SQUID_PORT}`, |
There was a problem hiding this comment.
Now that both uppercase and lowercase proxy variables are set, --env / additionalEnv overrides can easily desync them (e.g., overriding only HTTP_PROXY leaves http_proxy pointing at the default). Similar to the existing NO_PROXY/no_proxy normalization below, consider normalizing HTTP_PROXY↔http_proxy and HTTPS_PROXY↔https_proxy after applying additionalEnv so a single override behaves consistently across clients.
|
feat(cli): add short flags for frequently used options
|
Smoke Test Results✅ GitHub MCP: #1229 feat(cli): add short flags for frequently used options, #1228 docs: clarify --image-tag behavior with agent-image presets Overall: PASS
|
This comment has been minimized.
This comment has been minimized.
|
Smoke test results for ✅ GitHub MCP — Last 2 merged PRs: #1229 Overall: PASS
|
Chroot Version Comparison Results
Result: ❌ Not all versions match. Go matches, but Python and Node.js differ between host and chroot environments.
|
Yarn 4 (undici), Corepack, and some Node.js HTTP clients only check lowercase http_proxy/https_proxy environment variables. This caused EPROTO SSL errors when these tools tried to make HTTPS connections through the Squid proxy. - Add lowercase http_proxy/https_proxy alongside uppercase variants - Set NODE_EXTRA_CA_CERTS when SSL Bump is enabled so Node.js trusts the AWF session CA certificate - Export NODE_EXTRA_CA_CERTS in entrypoint.sh for container context - Add tests for lowercase proxy vars and NODE_EXTRA_CA_CERTS Fixes #949 Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
The api-proxy container's curl health check was routing through Squid because of the newly-added lowercase http_proxy/https_proxy env vars. curl respects lowercase proxy vars, causing localhost health checks to fail. Add NO_PROXY/no_proxy with localhost entries to prevent this. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
2700624 to
82f999a
Compare
|
🤖 Smoke test results for PR #1234 by ✅ GitHub MCP — Last 2 merged PRs: Overall: PASS
|
|
test: expand credential hiding tests to all 14 protected paths | test(docker): verify capsh execution chain after PR #715
|
Smoke Test Results
Overall: PASS
|
🏗️ Build Test Suite Results
Overall: 8/8 ecosystems passed — ✅ PASS
|
Chroot Version Comparison Results
Overall: ❌ Not all tests passed — Python and Node.js versions differ between host and chroot environments.
|
Summary
Fix Yarn 4 and Corepack EPROTO SSL errors when connecting through the Squid proxy.
http_proxy,https_proxy) alongside uppercase variants. Yarn 4 (undici), Corepack, and some Node.js HTTP clients only check lowercase proxy environment variables. Without these, their HTTPS connections bypass the proxy and get DNAT'd to Squid's forward proxy port, which expects HTTP protocol — causing EPROTO errors.NODE_EXTRA_CA_CERTSwhen SSL Bump is enabled so Node.js tools trust the AWF session CA certificate. Node.js uses its own CA bundle, not the system CA store updated byupdate-ca-certificates.Fixes #949
Test plan
npm run build && npm testpasses (842 tests)🤖 Generated with Claude Code