-
Notifications
You must be signed in to change notification settings - Fork 60
mitm: support plain-HTTP forward-proxy requests on the MITM ingress #150
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -42,14 +42,16 @@ Environment variables set on the child: | |
| AGENT_VAULT_ADDR — base URL of the Agent Vault HTTP control server | ||
| AGENT_VAULT_VAULT — vault the session is scoped to | ||
|
|
||
| The child also inherits HTTPS_PROXY / NO_PROXY / NODE_USE_ENV_PROXY plus | ||
| the root CA trust variables (SSL_CERT_FILE, NODE_EXTRA_CA_CERTS, | ||
| REQUESTS_CA_BUNDLE, CURL_CA_BUNDLE, GIT_SSL_CAINFO, DENO_CERT) so standard | ||
| HTTPS clients transparently route through the broker. NODE_USE_ENV_PROXY=1 | ||
| enables Node.js built-in proxy support (v22.21.0+) so fetch() and | ||
| https.get() honor HTTPS_PROXY natively. HTTP_PROXY is intentionally not | ||
| set — the MITM proxy only handles HTTPS (CONNECT) and would 405 any plain | ||
| http:// request. The root CA PEM is written to ~/.agent-vault/mitm-ca.pem. | ||
| The child also inherits HTTPS_PROXY / HTTP_PROXY / NO_PROXY / | ||
| NODE_USE_ENV_PROXY plus the root CA trust variables (SSL_CERT_FILE, | ||
| NODE_EXTRA_CA_CERTS, REQUESTS_CA_BUNDLE, CURL_CA_BUNDLE, GIT_SSL_CAINFO, | ||
| DENO_CERT) so both HTTPS and plain-HTTP clients transparently route | ||
| through the broker. NODE_USE_ENV_PROXY=1 enables Node.js built-in proxy | ||
| support (v22.21.0+) so fetch() and http.get()/https.get() honor the | ||
| proxy env natively. HTTPS_PROXY and HTTP_PROXY both point at the same | ||
| TLS-wrapped proxy URL — the listener accepts CONNECT for https:// | ||
| upstreams and absolute-form forward-proxy requests for http:// on the | ||
| same port. The root CA PEM is written to ~/.agent-vault/mitm-ca.pem. | ||
|
|
||
| Example: | ||
| ` + examplePrefix + ` -- claude | ||
|
Comment on lines
42
to
57
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🟡 The runtime startup banner at cmd/run.go:149 still prints Extended reasoning...What the bug is. The PR rewrote every text surface that mentioned HTTPS-only routing — the cobra Long help (lines 42–57 of the diff), the docstring on |
||
|
|
@@ -381,15 +383,16 @@ func requireMITMEnv(env []string, addr, token, vault, caPath string) ([]string, | |
| return newEnv, port, nil | ||
| } | ||
|
|
||
| // augmentEnvWithMITM extends env so the child transparently routes HTTPS | ||
| // through the broker. Returns (env, 0, false, nil) when the server has | ||
| // MITM disabled. The second return value is the port the server reported; | ||
| // callers log it so operators see the actual listen port (not a constant). | ||
| // caPath is a test seam — pass "" for the default location. | ||
| // augmentEnvWithMITM extends env so the child transparently routes HTTP | ||
| // and HTTPS through the broker. Returns (env, 0, false, nil) when the | ||
| // server has MITM disabled. The second return value is the port the | ||
| // server reported; callers log it so operators see the actual listen | ||
| // port (not a constant). caPath is a test seam — pass "" for the | ||
| // default location. | ||
| // | ||
| // Only HTTPS_PROXY is injected — not HTTP_PROXY. The MITM proxy handles | ||
| // HTTP CONNECT only and returns 405 for every other method, so setting | ||
| // HTTP_PROXY would route plain http:// requests into a dead end. | ||
| // Both HTTPS_PROXY and HTTP_PROXY are injected, pointing at the same | ||
| // TLS-wrapped proxy URL. The listener handles CONNECT for https:// | ||
| // upstreams and absolute-form forward-proxy requests for http://. | ||
| func augmentEnvWithMITM(env []string, addr, token, vault, caPath string) ([]string, int, bool, error) { | ||
| pem, port, enabled, mitmTLS, err := fetchMITMCA(addr) | ||
| if err != nil { | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🟡 Doc cross-sync miss: this PR adds
HTTP_PROXYalongsideHTTPS_PROXYinvault runand updates ~8 pages to match, but ~14 other docs still describe HTTPS_PROXY-only behavior — quickstarts (claude-code,codex,cursor,hermes-agent,opencode,nanoclaw,openclaw,custom-agent),docs/index.mdx:24,docs/agents/overview.mdx,docs/learn/security.mdx:118(heading),docs/learn/services.mdx:187,docs/guides/connect-custom-agent.mdx, anddocs/self-hosting/{local,docker,fly-io}.mdx. Theagents/protocol.mdxbody table on line 90 was correctly updated, but the section heading "Route requests through HTTPS_PROXY" (and the frontmatter description) wasn't. Nothing functionally breaks — this is purely the cross-doc-sync rule the PR's ownCLAUDE.mdline ~32/50 codifies. Mechanical fix: the same wording flip the PR already applied in the eight updated pages, repeated across the listed paths.Extended reasoning...
What is wrong. This PR's own
CLAUDE.mdchange (the file's own conventions section, line ~32/50) explicitly says: "Whenever a new feature lands, scan docs/ for pages that need a matching update — quickstart, guides, self-hosting, learn, and reference all mirror runtime behavior and drift fast." The PR does this for eight pages (CLAUDE.md,README.md,docs/agents/protocol.mdxbody table,docs/guides/{connect-coding-agent,container-isolation}.mdx,docs/installation.mdx,docs/reference/cli.mdx,docs/self-hosting/environment-variables.mdx) and the two embedded skill files. It misses ~14 others that still describe HTTPS_PROXY-only behavior, contradicting the new runtime behavior the PR ships.Verified by grep against the working tree. Each of the following lines was confirmed:
docs/quickstart/claude-code.mdx:13,cursor.mdx:13,codex.mdx:13,hermes-agent.mdx:13,opencode.mdx:13: "pre-configuresHTTPS_PROXYand CA trust so outbound HTTPS routes through Agent Vault transparently". Same five files at line ~39: "HTTPS_PROXYroutes the call through Agent Vault".docs/quickstart/custom-agent.mdx:3,13,27,62,89— frontmatter description and body all HTTPS_PROXY-only.docs/quickstart/nanoclaw.mdx:43,openclaw.mdx:43: "Invited agents like NanoClaw build their ownHTTPS_PROXY…".docs/index.mdx:24: "You point the agent at Agent Vault by settingHTTPS_PROXYand trusting the Agent Vault CA certificate.".docs/agents/overview.mdx:12,22,162— three HTTPS_PROXY-only mentions on the canonical agents overview page.docs/learn/security.mdx:118— section heading "Transparent proxy (HTTPS_PROXY) transport" plus body.docs/learn/services.mdx:187— Twilio walkthrough still says "viaHTTPS_PROXY".docs/guides/connect-custom-agent.mdx:3,27,41,62,69,91,118,127— frontmatter description, body, env-var table row, and the heading "SetHTTPS_PROXYmanually" all HTTPS_PROXY-only.docs/self-hosting/local.mdx:81: "transparentHTTPS_PROXYlistener on port14322".docs/self-hosting/docker.mdx:24: "transparent HTTPS proxy (14322) so agents'HTTPS_PROXYcan reach the broker".docs/self-hosting/fly-io.mdx:76: "AgentHTTPS_PROXY".docs/agents/protocol.mdx: the body env-var table on line ~88-90 correctly added anHTTP_PROXYrow, but the section heading "Route requests through HTTPS_PROXY" and the frontmatter description still describe HTTPS_PROXY-only behavior.Step-by-step proof of impact.
docs/installation.mdx(updated in this PR) and stands up a server. They read line 121 — "the transparent HTTP/HTTPS proxy that agents'HTTPS_PROXYandHTTP_PROXYpoint at". Good.docs/quickstart/claude-code.mdx. Line 13 says: "pre-configuresHTTPS_PROXYand CA trust so outbound HTTPS routes through Agent Vault transparently". They reasonably conclude that plain-http://upstreams are NOT brokered.http://call to an internal service. Under the new code path it IS brokered (audited, credential-injected if matched, denied underunmatched_host_policy=deny). The operator is surprised — the docs told them otherwise.unmatched_host_policy=denyand the operator hadn't added the internal service. Under the old behavior, a plain-http://call would never have hit the broker. Under the new behavior it returns 403 from Agent Vault. The operator goes todocs/learn/security.mdxto debug, sees the heading "Transparent proxy (HTTPS_PROXY) transport" and the HTTPS-only body below it, and now genuinely cannot tell whether plain-HTTP traffic is supposed to go through the broker or not.That's the operator-facing harm: not a runtime bug, but documentation that actively misleads someone trying to reason about traffic flow with the new code in place.
Why existing code does not prevent this. This is documentation, not code — there's no test that fails on doc drift, and the PR's own self-review checklist (run /simplify, run /security-review,
make test) does not cover scanning unchanged docs for stale references. The only safeguard is the CLAUDE.md convention that this PR violates.Impact. Severity nit. No functional regression — every line cited is factually correct for HTTPS, just incomplete with respect to HTTP in the new world. Operators reading these pages will conclude HTTP_PROXY is best-effort or only-some-pages, which is the opposite of true. Cosmetic, but pervasive (~14 pages on a docs surface this PR explicitly touched).
How to fix. Mechanical sed pass over the listed paths replacing the same wording the PR already established in the updated pages:
HTTPS_PROXY→HTTPS_PROXY/HTTP_PROXY,HTTPS traffic→HTTP and HTTPS traffic, the section headings updated to drop the HTTPS_PROXY-specific framing, and theagents/protocol.mdxfrontmatter description + section heading updated to match the body table that was already corrected.