diff --git a/reference-implementations/agt/README.md b/reference-implementations/agt/README.md index 91b91ad..426d082 100644 --- a/reference-implementations/agt/README.md +++ b/reference-implementations/agt/README.md @@ -156,6 +156,8 @@ This registers `hosts/claude-code/acs-hook.ts` on two Claude Code hook events. ` claude ``` +Start it from a shell where `127.0.0.1` and `localhost` are in both `NO_PROXY` and `no_proxy`, as step 1 of [Produce a trace](#produce-a-trace) sets them. Otherwise an `HTTP_PROXY` in that shell sends every hook payload, commands and tool output included, through the proxy on its way to the Guardian. + 3. Ask Claude Code to run a destructive command, for example `echo rm -rf /`. The hook sends the call to the Guardian. AGT's stock pattern rule denies it. Claude Code shows the policy reason in the transcript. Ask for `ls -la` in the same session and it runs. Use `echo rm -rf /` rather than `rm -rf /` when you script this. The pattern matches at offset 5 and the command is inert if the hook ever fails. @@ -208,10 +210,11 @@ git checkout # skip this line to use the latest integration cd reference-implementations/agt bun install --frozen-lockfile mkdir -p .acs && chmod 700 .acs # owner-only, even if .acs already existed -export NO_PROXY=127.0.0.1,localhost no_proxy=127.0.0.1,localhost # keep local traffic off any HTTP_PROXY +export NO_PROXY="${NO_PROXY:+$NO_PROXY,}${no_proxy:+$no_proxy,}127.0.0.1,localhost" +export no_proxy="$NO_PROXY" ``` -Run every later step in a terminal where you've run the `export` line. Without it, an `HTTP_PROXY` in your shell sends the shim's requests and the Guardian's spans to the proxy. +Run every later step in a terminal where you've run the two `export` lines. Without them, an `HTTP_PROXY` in your shell sends the shim's requests and the Guardian's spans to the proxy. The lines keep the exemptions you already had and add this machine to both variables, because Bun reads `no_proxy` in preference to `NO_PROXY` when both are set. They need bash or zsh. **2. Start the Guardian with trace export on.** Leave it running in its own terminal. @@ -268,7 +271,7 @@ NO_PROXY=127.0.0.1,localhost no_proxy=127.0.0.1,localhost \ To run the collector in Docker instead, from `reference-implementations/agt`: ```bash -docker run --rm --user "$(id -u):$(id -g)" -p 127.0.0.1:4318:4318 \ +docker run --rm --name acs-otelcol --user "$(id -u):$(id -g)" -p 127.0.0.1:4318:4318 \ -v "$PWD/otel-collector.yaml":/etc/otelcol-contrib/config.yaml:ro \ -v "$PWD/.acs":/work/.acs -w /work \ otel/opentelemetry-collector-contrib:0.162.0@sha256:39923a8e431bd1f57be82411999d389fcfe40857492e4365456d97a4c1f74be6 \ @@ -282,17 +285,17 @@ Start the collector before the Guardian. The Guardian stops sending to an endpoi Repeat step 3. The collector writes what it received to `.acs/otel-collector-traces.jsonl`. That file can still be empty right after you repeat step 3, because spans go out in batches about every five seconds. -Stop the collector with Ctrl-C or `docker stop`, never `kill -9` or `docker kill`. A hard kill can leave half a line, and the next batch the restarted collector writes is joined onto it. The step 5 reader skips that combined line and tells you, but the spans in it are lost. +Stop the collector with Ctrl-C or `docker stop acs-otelcol`, never `kill -9` or `docker kill`. A hard kill can leave half a line, and the next batch the restarted collector writes is joined onto it. The step 5 reader skips that combined line and tells you, but the spans in it are lost. The collector appends across restarts. Natively under `umask 077` the file is `0600`. In a container it's `0644`, and the `0700` `.acs` directory is what keeps other users out ([issue #71](https://github.com/GenAI-Security-Project/agent-control-standard/issues/71) tracks file modes). -The `export NO_PROXY` line from step 1 keeps the spans off any `HTTP_PROXY`. The Guardian also adds a loopback endpoint to `NO_PROXY` itself. `OTEL_EXPORTER_OTLP_*HEADERS` variables still add headers to every send, so unset them if you don't mean them. +The `export` lines from step 1 keep the spans off any `HTTP_PROXY`, and so does the inline prefix on the Guardian's line above, because some Bun versions read these variables only when a process starts. The Guardian also adds a loopback endpoint to whichever of `NO_PROXY` and `no_proxy` you set, which takes effect on Bun 1.3.14 and later. When a proxy is set and loopback isn't exempt, it prints a warning on stderr when it starts. `OTEL_EXPORTER_OTLP_*HEADERS` variables still add headers to every send, so unset them if you don't mean them. If `.acs/otel-collector-traces.jsonl` doesn't appear within about ten seconds of step 3, the collector isn't running this config. Verified on macOS (arm64): natively with otelcol-contrib 0.162.0, and with the Docker command above on Docker Desktop for Mac. Rootless Docker, Podman, SELinux hosts and musl-based Linux haven't been verified. -**Run it again.** Every output here except the converter's appends, so a second run on the same `.acs` doubles every count, and the converter refuses to run at all. Delete it first: `rm -rf .acs && mkdir -p .acs && chmod 700 .acs`. +**Run it again.** Every output here except the converter's appends, so a second run on the same `.acs` doubles every count, and the converter refuses to run at all. Stop the Guardian and the collector first, with Ctrl-C or `docker stop acs-otelcol`. A process that is still running keeps writing into the deleted file, and its spans are lost without an error. Then delete it: `rm -rf .acs && mkdir -p .acs && chmod 700 .acs`. **What this is, and isn't.** The exporter covers the two hook methods this Guardian evaluates, `steps/toolCallRequest` and `steps/toolCallResult`. It isn't an ACS-Trace conformance claim. Traces driven through the shim also lack `acs.capability` and `acs.request_id_ref`, because the shim doesn't send them yet ([issue #274](https://github.com/GenAI-Security-Project/agent-control-standard/issues/274)). The committed sample drives the Guardian directly, so it has both. Spans also lack `acs.evaluator` until the Guardian sends `metadata.evaluator` (PR #229). diff --git a/reference-implementations/agt/otel-collector.yaml b/reference-implementations/agt/otel-collector.yaml index 6542e3f..29a3279 100644 --- a/reference-implementations/agt/otel-collector.yaml +++ b/reference-implementations/agt/otel-collector.yaml @@ -9,7 +9,7 @@ # # Docker (run from reference-implementations/agt, after # mkdir -p .acs && chmod 700 .acs): -# docker run --rm --user "$(id -u):$(id -g)" -p 127.0.0.1:4318:4318 \ +# docker run --rm --name acs-otelcol --user "$(id -u):$(id -g)" -p 127.0.0.1:4318:4318 \ # -v "$PWD/otel-collector.yaml":/etc/otelcol-contrib/config.yaml:ro \ # -v "$PWD/.acs":/work/.acs -w /work \ # otel/opentelemetry-collector-contrib:0.162.0@sha256:39923a8e431bd1f57be82411999d389fcfe40857492e4365456d97a4c1f74be6 \ diff --git a/reference-implementations/agt/packages/guardian/src/otel-trace-exporter.ts b/reference-implementations/agt/packages/guardian/src/otel-trace-exporter.ts index 9969b93..e315db7 100644 --- a/reference-implementations/agt/packages/guardian/src/otel-trace-exporter.ts +++ b/reference-implementations/agt/packages/guardian/src/otel-trace-exporter.ts @@ -575,6 +575,23 @@ export function createFileSpanExporter(path: string): SpanExporter { }; } +/** The NO_PROXY entries that exempt a loopback endpoint's host, or none + * when the endpoint is not loopback. IPv6 gets both forms: Bun matches only + * `[::1]`, and other clients expect the bare `::1`. */ +function loopbackProxyEntries(endpoint: string): string[] { + const host = new URL(endpoint).hostname.replace(/^\[|\]$/g, ""); + if (host === "127.0.0.1" || host === "localhost") { + return [host]; + } + return host === "::1" ? ["::1", "[::1]"] : []; +} + +const splitProxyList = (value: string): string[] => + value + .split(",") + .map((entry) => entry.trim()) + .filter((entry) => entry !== ""); + /** * Adds a loopback endpoint's host to NO_PROXY and no_proxy, so an * HTTP_PROXY in the Guardian's environment cannot route spans meant for this @@ -584,6 +601,18 @@ export function createFileSpanExporter(path: string): SpanExporter { * opt out, so the environment is the only lever. A non-loopback endpoint is * left to the operator's proxy settings. * + * Only the variables the user set are touched. Bun 1.3.14 reads a non-empty + * no_proxy in preference to NO_PROXY, so creating the one the user did not + * set would switch off every exemption in the one they did. "Set" follows + * Bun's own rule: any value but the empty string counts, and an empty one + * makes Bun fall back to the other. A set variable that holds no entries + * (a stray comma, say) is what Bun reads, so it is filled with the host. + * When neither is set, both get the host, and nothing is lost. + * + * Bun 1.3.11 reads these variables only when the process starts, so on that + * version this edit has no effect, and loopbackProxyWarning below is what + * tells the operator. + * * The change is process-wide and permanent: it is written to process.env and * never undone, so anything else in this process (a host adapter embedded * with the Guardian) and any child process it spawns inherits it. That is @@ -591,18 +620,47 @@ export function createFileSpanExporter(path: string): SpanExporter { * machine should not use a proxy for any of them. */ export function keepLoopbackOffProxy(endpoint: string): void { - const host = new URL(endpoint).hostname.replace(/^\[|\]$/g, ""); - if (host !== "127.0.0.1" && host !== "localhost" && host !== "::1") { + const additions = loopbackProxyEntries(endpoint); + if (additions.length === 0) { return; } - for (const key of ["NO_PROXY", "no_proxy"]) { - const entries = (process.env[key] ?? "").split(",").map((entry) => entry.trim()).filter((entry) => entry !== ""); - if (!entries.includes(host)) { - process.env[key] = [...entries, host].join(","); + const keys = ["NO_PROXY", "no_proxy"] as const; + const setKeys = keys.filter((key) => (process.env[key] ?? "") !== ""); + for (const key of setKeys.length === 0 ? keys : setKeys) { + const entries = splitProxyList(process.env[key] ?? ""); + const missing = additions.filter((entry) => !entries.includes(entry)); + if (missing.length > 0) { + process.env[key] = [...entries, ...missing].join(","); } } } +/** + * A warning when a loopback endpoint would reach an HTTP proxy, judged by + * the variables as they stand: a proxy is set, and the list Bun reads (a + * non-empty no_proxy, otherwise NO_PROXY) does not exempt the host. Called + * before keepLoopbackOffProxy, so on Bun 1.3.11, which ignores the runtime + * edit, the operator still learns that spans may go through the proxy and + * what to set before starting the Guardian. + */ +export function loopbackProxyWarning(endpoint: string, env: Record = process.env): string | undefined { + const additions = loopbackProxyEntries(endpoint); + const proxy = env.http_proxy || env.HTTP_PROXY; + if (additions.length === 0 || !proxy) { + return undefined; + } + const list = splitProxyList(env.no_proxy ? env.no_proxy : (env.NO_PROXY ?? "")); + // The bracketed form is the one Bun matches for IPv6. + const needed = additions[additions.length - 1]!; + if (list.includes("*") || list.includes(needed)) { + return undefined; + } + return ( + `otel trace export to ${endpoint}: an HTTP proxy is set and NO_PROXY/no_proxy did not exempt ${needed}, ` + + `so on some Bun versions these spans go through the proxy. Start the Guardian with ${needed} in both NO_PROXY and no_proxy.` + ); +} + /** One place spans go, with the name its failure is reported under: the * file's path, the endpoint's URL, or "span exporter" for an injected one. */ type Output = { label: string; exporter: SpanExporter }; @@ -735,6 +793,10 @@ export function createOtelTraceExporter({ // the SDK under any set here and cannot be refused, so the README lists // them as honored. construct(endpoint, () => { + const warning = loopbackProxyWarning(endpoint); + if (warning !== undefined) { + console.error(warning); + } keepLoopbackOffProxy(endpoint); return new OTLPTraceExporter({ url: endpoint, timeoutMillis: ENDPOINT_TIMEOUT_MS, compression: "none" as NonNullable[0]>["compression"] }); }); @@ -957,3 +1019,4 @@ export function createOtelTraceExporter({ }, }; } + diff --git a/reference-implementations/agt/packages/guardian/test/otel-trace-exporter.test.ts b/reference-implementations/agt/packages/guardian/test/otel-trace-exporter.test.ts index 581bf4c..2c5f05b 100644 --- a/reference-implementations/agt/packages/guardian/test/otel-trace-exporter.test.ts +++ b/reference-implementations/agt/packages/guardian/test/otel-trace-exporter.test.ts @@ -15,6 +15,7 @@ import { createOtelTraceExporter, DECISION_ATTRIBUTE_SOURCES, loadOtelMapping, + loopbackProxyWarning, METHODS_WITHOUT_SPAN, NULL_OTEL_TRACE_EXPORTER, OTEL_SERVICE_NAME, @@ -1425,10 +1426,62 @@ describe("keepLoopbackOffProxy", () => { expect([process.env.NO_PROXY, process.env.no_proxy]).toEqual(["localhost", "localhost"]); }); - it("adds ::1 without brackets for an IPv6 loopback endpoint", () => { + it("adds ::1 both bare and bracketed for an IPv6 loopback endpoint, because Bun matches only [::1]", () => { reset(undefined); keepLoopbackOffProxy("http://[::1]:4318/v1/traces"); - expect([process.env.NO_PROXY, process.env.no_proxy]).toEqual(["::1", "::1"]); + expect([process.env.NO_PROXY, process.env.no_proxy]).toEqual(["::1,[::1]", "::1,[::1]"]); + }); + + // Bar's finding on issue #274: Bun 1.3.14 reads a non-empty no_proxy in + // preference to NO_PROXY, so creating the one the user did not set switches + // off every exemption in the one they did. + it("appends to NO_PROXY alone when only NO_PROXY is set, and does not create no_proxy", () => { + reset(undefined); + process.env.NO_PROXY = "corp.internal"; + keepLoopbackOffProxy("http://127.0.0.1:4318/v1/traces"); + expect(process.env.NO_PROXY).toBe("corp.internal,127.0.0.1"); + expect("no_proxy" in process.env).toBe(false); + }); + + it("appends to no_proxy alone when only no_proxy is set, and does not create NO_PROXY", () => { + reset(undefined); + process.env.no_proxy = "corp.internal"; + keepLoopbackOffProxy("http://127.0.0.1:4318/v1/traces"); + expect(process.env.no_proxy).toBe("corp.internal,127.0.0.1"); + expect("NO_PROXY" in process.env).toBe(false); + }); + + it("treats an empty no_proxy as unset, since Bun then falls back to NO_PROXY", () => { + reset(undefined); + process.env.NO_PROXY = "corp.internal"; + process.env.no_proxy = ""; + keepLoopbackOffProxy("http://127.0.0.1:4318/v1/traces"); + expect([process.env.NO_PROXY, process.env.no_proxy]).toEqual(["corp.internal,127.0.0.1", ""]); + }); + + it("fills a no_proxy that holds no entries, because Bun reads any non-empty value as the list", () => { + for (const blank of [",", " ", " , "]) { + reset(undefined); + process.env.NO_PROXY = "corp.internal"; + process.env.no_proxy = blank; + keepLoopbackOffProxy("http://127.0.0.1:4318/v1/traces"); + expect({ blank, NO_PROXY: process.env.NO_PROXY, no_proxy: process.env.no_proxy }).toEqual({ + blank, + NO_PROXY: "corp.internal,127.0.0.1", + no_proxy: "127.0.0.1", + }); + } + }); + + it("sets both when neither is set, and appends once to each when both are set", () => { + reset(undefined); + keepLoopbackOffProxy("http://127.0.0.1:4318/v1/traces"); + expect([process.env.NO_PROXY, process.env.no_proxy]).toEqual(["127.0.0.1", "127.0.0.1"]); + process.env.NO_PROXY = "a.internal"; + process.env.no_proxy = "b.internal"; + keepLoopbackOffProxy("http://127.0.0.1:4318/v1/traces"); + keepLoopbackOffProxy("http://127.0.0.1:4318/v1/traces"); + expect([process.env.NO_PROXY, process.env.no_proxy]).toEqual(["a.internal,127.0.0.1", "b.internal,127.0.0.1"]); }); it("does not duplicate the entry when called twice", () => { @@ -1454,3 +1507,90 @@ describe("keepLoopbackOffProxy", () => { expect([process.env.NO_PROXY, process.env.no_proxy]).toEqual(["example.internal", "example.internal"]); }); }); + +// Bun 1.3.11 reads the proxy variables only when the process starts, so the +// runtime edit above cannot help there. The warning is what every version +// gets: it judges the variables as they stand when the exporter is built. +describe("loopbackProxyWarning", () => { + const endpoint = "http://127.0.0.1:4318/v1/traces"; + + it("warns when a proxy is set and the list Bun reads does not exempt the host", () => { + const warning = loopbackProxyWarning(endpoint, { HTTP_PROXY: "http://proxy:3128", NO_PROXY: "corp.internal" }); + expect(warning).toContain("127.0.0.1"); + expect(warning).toContain("NO_PROXY"); + }); + + it("judges by no_proxy when it is non-empty, the list Bun reads first", () => { + expect(loopbackProxyWarning(endpoint, { http_proxy: "http://proxy:3128", NO_PROXY: "127.0.0.1", no_proxy: "corp.internal" })).toBeDefined(); + expect(loopbackProxyWarning(endpoint, { http_proxy: "http://proxy:3128", NO_PROXY: "corp.internal", no_proxy: "127.0.0.1" })).toBeUndefined(); + expect(loopbackProxyWarning(endpoint, { http_proxy: "http://proxy:3128", NO_PROXY: "127.0.0.1", no_proxy: "" })).toBeUndefined(); + }); + + it("stays quiet with no proxy, with a wildcard exemption, or for a non-loopback endpoint", () => { + expect(loopbackProxyWarning(endpoint, {})).toBeUndefined(); + expect(loopbackProxyWarning(endpoint, { HTTP_PROXY: "http://proxy:3128", NO_PROXY: "*" })).toBeUndefined(); + expect(loopbackProxyWarning("http://collector.example.com:4318/v1/traces", { HTTP_PROXY: "http://proxy:3128" })).toBeUndefined(); + }); + + it("wants the bracketed form for IPv6, the only one Bun matches", () => { + const v6 = "http://[::1]:4318/v1/traces"; + expect(loopbackProxyWarning(v6, { HTTP_PROXY: "http://proxy:3128", NO_PROXY: "::1" })).toBeDefined(); + expect(loopbackProxyWarning(v6, { HTTP_PROXY: "http://proxy:3128", NO_PROXY: "[::1]" })).toBeUndefined(); + }); +}); + +// Routing, not strings. It runs in a fresh child with its environment set at +// spawn, because Bun latches proxy variables across edits within a process, +// which makes any in-process routing check pass for the wrong reason. Bun +// 1.3.11 ignores runtime edits altogether, so this is gated to 1.3.14 and +// later, and CI on 1.3.11 skips it. +describe("keepLoopbackOffProxy, routed through a real proxy in a child process", () => { + const runtimeEditsHonored = Bun.semver.satisfies(Bun.version, ">=1.3.14"); + + it.skipIf(!runtimeEditsHonored)("keeps loopback off the proxy without switching off the user's existing NO_PROXY entries", async () => { + let proxyConnections = 0; + const proxy = createServer((socket) => { + proxyConnections += 1; + socket.destroy(); + }); + await new Promise((resolve) => proxy.listen(0, "127.0.0.1", resolve)); + const proxyUrl = `http://127.0.0.1:${(proxy.address() as { port: number }).port}`; + const source = new URL("../src/otel-trace-exporter.ts", import.meta.url).pathname; + const script = ` + const { keepLoopbackOffProxy } = await import(${JSON.stringify(source)}); + const target = Bun.serve({ port: 0, hostname: "127.0.0.1", fetch: () => new Response("ok") }); + const go = async (host) => { + try { + const response = await fetch("http://" + host + ":" + target.port + "/", { signal: AbortSignal.timeout(1500) }); + return (await response.text()) === "ok" ? "direct" : "other"; + } catch { + return "proxied"; + } + }; + const guard = await go("127.0.0.1"); + keepLoopbackOffProxy("http://127.0.0.1:" + target.port + "/v1/traces"); + const loopback = await go("127.0.0.1"); + const existing = await go("localhost"); + console.log(JSON.stringify({ guard, loopback, existing })); + target.stop(true); + process.exit(0); + `; + try { + // localhost stands in for a corporate host the user already exempted. + // Async spawn, not spawnSync: the proxy listener runs on this process's + // event loop and could not count the child's connection while blocked. + const child = Bun.spawn([process.execPath, "-e", script], { + env: { PATH: process.env.PATH ?? "", HOME: process.env.HOME ?? "", HTTP_PROXY: proxyUrl, http_proxy: proxyUrl, NO_PROXY: "localhost" }, + stdout: "pipe", + }); + const output = await new Response(child.stdout).text(); + await child.exited; + const result = JSON.parse(output.trim().split("\n").pop()!); + // The guard proves the proxy was in the path before the call. + expect(result).toEqual({ guard: "proxied", loopback: "direct", existing: "direct" }); + expect(proxyConnections).toBe(1); + } finally { + proxy.close(); + } + }, 15_000); +}); diff --git a/reference-implementations/agt/test/collector-config.test.ts b/reference-implementations/agt/test/collector-config.test.ts index 85b57b1..4778f3f 100644 --- a/reference-implementations/agt/test/collector-config.test.ts +++ b/reference-implementations/agt/test/collector-config.test.ts @@ -57,3 +57,25 @@ describe("the walkthrough's collector step", () => { expect(readme).toContain("with the Docker command above on Docker Desktop for Mac"); }); }); + +// Bar's two pre-handoff findings on issue #274. +describe("the walkthrough's proxy and rerun instructions", () => { + const STOP_FIRST = + "Stop the Guardian and the collector first, with Ctrl-C or `docker stop acs-otelcol`. A process that is still running keeps writing into the deleted file, and its spans are lost without an error."; + + it("tells the reader to stop the Guardian and the collector before deleting .acs", () => { + const paragraph = readme.slice(readme.indexOf("**Run it again.**")); + expect(paragraph.indexOf(STOP_FIRST)).toBeGreaterThan(-1); + expect(paragraph.indexOf(STOP_FIRST)).toBeLessThan(paragraph.indexOf("rm -rf .acs")); + }); + + it("names the container, so docker stop has something to stop, in both the README and the config", () => { + expect(readme).toContain("docker run --rm --name acs-otelcol"); + expect(text).toContain("docker run --rm --name acs-otelcol"); + }); + + it("keeps the reader's own exemptions in both proxy variables in step 1", () => { + expect(readme).toContain('export NO_PROXY="${NO_PROXY:+$NO_PROXY,}${no_proxy:+$no_proxy,}127.0.0.1,localhost"\nexport no_proxy="$NO_PROXY"'); + expect(readme).not.toContain("export NO_PROXY=127.0.0.1,localhost no_proxy=127.0.0.1,localhost"); + }); +});