diff --git a/.env.example b/.env.example index 65362425..bfd61b36 100644 --- a/.env.example +++ b/.env.example @@ -164,6 +164,17 @@ COMPUTER_TOKEN= # The server refuses to start with it set under NODE_ENV=production. # AGENT_COMPUTER_ALLOW_PRIVATE_HOSTS=true # +# Private addresses an agent may be registered at, named one at a time, comma separated. This is what +# a deployment uses instead of the switch above: bring your own agent, running on your own network, +# without lifting the floor for browsing or for anything else. +# +# A host, optionally with a port. `agents.internal` covers any port on that host; `10.0.0.42:9000` +# pins that one. Matching is exact — no wildcards, no suffixes — and a URL or a `*` is refused at +# startup with the entry named, rather than silently never matching. The never-allowed addresses, +# cloud metadata among them, cannot be named back in. Unset means none, which is the default posture. +# +# AGENT_ENDPOINT_ALLOWED_HOSTS=agents.internal,10.0.0.42:9000 +# # What a Bot may do on its computer, as one JSON object. Absent uses the built-in default, which # permits the acting tools and forbids nothing, and records every action either way. # diff --git a/CHANGELOG.md b/CHANGELOG.md index 2d824c03..1df02a3e 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -8,6 +8,30 @@ Newest first. `Unreleased` is what is on `main` and not yet tagged. ## Unreleased +### Name the private addresses an agent may live at + +Refusing `AGENT_COMPUTER_ALLOW_PRIVATE_HOSTS` in production closed a hole and took something with +it: bring your own agent is a headline capability, a company's own agent legitimately lives at an +internal address, and the only way to reach one was to lift the floor for everything. Telling people +to set that flag is exactly the advice that made it dangerous. + +`AGENT_ENDPOINT_ALLOWED_HOSTS` names addresses instead. A comma-separated list of hosts, each +optionally with a port: `agents.internal` covers any port on that host, `10.0.0.42:9000` pins that +one. A deployment sets this and leaves the floor where it is. + +It is narrow on purpose: + +- **Agent endpoints only.** Browsing is not widened. A page can steer a Bot somewhere; an operator + naming an address they run is a different act from a Bot following a link to it. +- **Exact matching.** No wildcards and no suffixes. A list written with a `*`, or written as URLs, is + refused at startup with the entry named, rather than quietly never matching. +- **The never-allowed addresses stay never-allowed.** Cloud metadata is refused before the private + rule is reached, so naming it changes nothing. +- **Every hop, not just the first.** A named address is reachable wherever it appears and an unnamed + one is refused wherever it appears, so a redirect is not a way around registration. + +Unset means none, which is what every deployment has today. + ### Upgrading **A deployment that sets `AGENT_COMPUTER_ALLOW_PRIVATE_HOSTS=true` with `NODE_ENV=production` no diff --git a/docs/configuration.md b/docs/configuration.md index 4a54392d..be7ff85b 100644 --- a/docs/configuration.md +++ b/docs/configuration.md @@ -179,6 +179,7 @@ where `` is `google`, `microsoft` or `okta`. | `COMPUTER_SUPERVISOR_URL` | Supervisor URL for per-Bot computers. If absent, Bots share `AGENT_COMPUTER_URL`. | | `SUPERVISOR_TOKEN` | Bearer token required by the supervisor. | | `AGENT_COMPUTER_ALLOW_PRIVATE_HOSTS` | Local-only private-host browsing when `true`. A deployment running with `NODE_ENV=production` refuses to start while it is set. Cloud metadata addresses are refused either way. | +| `AGENT_ENDPOINT_ALLOWED_HOSTS` | unset | Private addresses an agent may be registered at, comma separated. Host, optionally with a port. Exact match; no wildcards. Never-allowed addresses cannot be named. | | `AGENT_COMPUTER_POLICY` | JSON action policy: `{"mode":"enforce","deny":[...],"allow":[...]}`. | | `COMPUTER_RUNTIME` | Set to `runsc` to run supervised computers under gVisor. | | `COMPUTER_SANDBOX` | Set to `on` to enable Chromium's own sandbox where the host permits user namespaces. Which way it went is printed at start-up. | diff --git a/server/src/agents/connection-test.ts b/server/src/agents/connection-test.ts index c8810f43..1606e214 100644 --- a/server/src/agents/connection-test.ts +++ b/server/src/agents/connection-test.ts @@ -90,6 +90,7 @@ export async function testAgentConnection( options: { headers?: Record; allowPrivateHosts?: boolean; + allowedHosts?: ReadonlySet; fetchImpl?: typeof fetch; timeoutMs?: number; } = {}, @@ -98,12 +99,14 @@ export async function testAgentConnection( // addresses that registration would refuse. const verdict = checkAgentEndpoint(rawEndpoint, { allowPrivateHosts: options.allowPrivateHosts, + ...(options.allowedHosts ? { allowedHosts: options.allowedHosts } : {}), }); if (!verdict.allowed) return { ok: false, reason: verdict.reason }; // Wrapped rather than called directly, so the address the request finally lands on is checked too. // Checking only what the person typed leaves the redirect as the way around it. const doFetch = createAgentFetch({ + ...(options.allowedHosts ? { allowedHosts: options.allowedHosts } : {}), ...(options.allowPrivateHosts !== undefined ? { allowPrivateHosts: options.allowPrivateHosts } : {}), diff --git a/server/src/agents/endpoint.ts b/server/src/agents/endpoint.ts index 646039e3..1a7656b3 100644 --- a/server/src/agents/endpoint.ts +++ b/server/src/agents/endpoint.ts @@ -22,6 +22,37 @@ import { checkNavigationTarget } from "../computer/target"; * refused. */ +/** + * Is this address one the deployment named, and refused only for being private? + * + * The second half is the load-bearing half. Re-running the check with the floor down separates + * "refused because it is inside the network", which naming may overrule, from "refused because it is + * the metadata address or is not a web address at all", which nothing may. Deciding that by reading + * the refusal text would break the first time somebody rephrased it. + * + * Matching is exact, on the host as written, and a name with a port pins that port. No suffixes and + * no patterns: a pattern that widened by accident is how host checks usually fail, and an operator + * naming three addresses can name three addresses. + */ +function namedAsAllowed( + raw: string, + allowedHosts: ReadonlySet | undefined, +): boolean { + if (!allowedHosts || allowedHosts.size === 0) return false; + if (!checkNavigationTarget(raw, { allowPrivateHosts: true }).allowed) { + return false; + } + let url: URL; + try { + url = new URL(raw); + } catch { + return false; + } + const hostname = url.hostname.toLowerCase().replace(/^\[|\]$/g, ""); + const host = url.host.toLowerCase().replace(/^\[/, "").replace(/\]/, ""); + return allowedHosts.has(host) || allowedHosts.has(hostname); +} + export type EndpointVerdict = | { allowed: true; url: string } | { allowed: false; reason: string }; @@ -34,13 +65,36 @@ export type EndpointVerdict = */ export function checkAgentEndpoint( raw: unknown, - options: { allowPrivateHosts?: boolean } = {}, + options: { + allowPrivateHosts?: boolean; + allowedHosts?: ReadonlySet; + } = {}, ): EndpointVerdict { if (typeof raw !== "string" || !raw.trim()) { return { allowed: false, reason: "An agent needs a web address." }; } const verdict = checkNavigationTarget(raw.trim(), options); + if (!verdict.allowed && namedAsAllowed(raw.trim(), options.allowedHosts)) { + /* + * Named, one host at a time, by whoever runs this deployment. + * + * The private-host opt-in is a floor: it permits this deployment's whole network, to browsing + * and to agent endpoints alike, which is why it is refused in production. But a company's own + * agent legitimately lives at an internal address, and telling them to drop the floor to reach + * it is the advice that made the opt-in dangerous in the first place. So an address may be named + * instead, and nothing else is opened. + * + * Only ever reached for an address the strict check refused *for being private*. Anything on the + * never-allowed list, and anything that is not http or https, is refused before this and cannot + * be named back in — see `namedAsAllowed`, which re-runs the check with the floor down to find + * out which kind of refusal it was rather than pattern-matching the message. + * + * Agent endpoints only. Browsing is not widened by this: a page can steer a Bot somewhere, and + * an operator naming an address they run is a different act from a Bot following a link to it. + */ + return { allowed: true, url: new URL(raw.trim()).toString() }; + } if (!verdict.allowed) { // The navigation wording talks about "the assistant opening" a page, which is not what is // happening here, so the reason is restated for the form surface. @@ -177,6 +231,12 @@ function strippedBody(body: BodyInit | null | undefined): { body?: BodyInit } { export function createAgentFetch( options: { allowPrivateHosts?: boolean; + /** + * Carried to every hop, not only the first. An address the deployment named is reachable + * wherever it appears, and one it did not name is refused wherever it appears — a redirect must + * not be a way to arrive somewhere registration would have declined. + */ + allowedHosts?: ReadonlySet; fetchImpl?: typeof fetch; /** * Told about every address this refused to dial, and why. @@ -211,6 +271,7 @@ export function createAgentFetch( ...(options.allowPrivateHosts !== undefined ? { allowPrivateHosts: options.allowPrivateHosts } : {}), + ...(options.allowedHosts ? { allowedHosts: options.allowedHosts } : {}), }); return async function guardedFetch(url: string, init?: RequestInit) { diff --git a/server/src/agents/routes.ts b/server/src/agents/routes.ts index 144a8c9b..50cb872c 100644 --- a/server/src/agents/routes.ts +++ b/server/src/agents/routes.ts @@ -42,6 +42,8 @@ type AgentInputObject = { export function parseAgentInput( input: unknown, allowPrivateHosts = false, + /** Private addresses this deployment named as acceptable. Empty is the default posture. */ + allowedHosts: ReadonlySet = new Set(), ): AgentInputParseResult { if (!isAgentInputObject(input)) { return { ok: false, error: "Agent input must be a JSON object." }; @@ -81,7 +83,10 @@ export function parseAgentInput( // goes through the same target check as navigation before it is allowed anywhere near the database. let endpoint: string | undefined; if (input.endpoint !== undefined && input.endpoint !== "") { - const verdict = checkAgentEndpoint(input.endpoint, { allowPrivateHosts }); + const verdict = checkAgentEndpoint(input.endpoint, { + allowPrivateHosts, + allowedHosts, + }); if (!verdict.allowed) return { ok: false, error: verdict.reason }; endpoint = verdict.url; } @@ -130,6 +135,13 @@ export function createAgentRoutes( allowPrivateHosts = false, /** Where a Bot's own refusal is recorded. Absent in tests that do not care about the trail. */ auditStore?: AuditStore, + /** + * Private addresses this deployment named as acceptable for an agent to live at. + * + * Separate from `allowPrivateHosts` on purpose: that one opens the network, this one opens an + * address. A hosted deployment sets this and leaves the other off. + */ + allowedHosts: ReadonlySet = new Set(), ) { const routes = new Hono<{ Variables: AppVariables }>(); @@ -228,6 +240,7 @@ export function createAgentRoutes( const result = await testAgentConnection(body?.endpoint, { headers, allowPrivateHosts, + allowedHosts, }); // 200 either way: the request succeeded, and the verdict is the payload. A failed connection test // is an answer, not an error, and a 4xx here would have the surface render it as a broken button. @@ -278,6 +291,7 @@ export function createAgentRoutes( const parsed = parseAgentInput( await context.req.json().catch(() => null), allowPrivateHosts, + allowedHosts, ); if (!parsed.ok) return context.json({ error: parsed.error }, 400); @@ -303,6 +317,7 @@ export function createAgentRoutes( const parsed = parseAgentInput( await context.req.json().catch(() => null), allowPrivateHosts, + allowedHosts, ); if (!parsed.ok) return context.json({ error: parsed.error }, 400); diff --git a/server/src/app.ts b/server/src/app.ts index 23595c94..fcf65c87 100644 --- a/server/src/app.ts +++ b/server/src/app.ts @@ -646,6 +646,9 @@ export function createApp( config.computer?.allowPrivateHosts ?? false, // A Bot's own refusal goes in the same trail as everything else it does. auditStore, + // Addresses this deployment named, which is how a hosted one reaches an agent on its own + // network without dropping the floor for everything else. + config.agentEndpointAllowedHosts, ), ); // Choosing a coworker for an untagged message needs the same permission-filtered roster the diff --git a/server/src/config.ts b/server/src/config.ts index ef04eafe..c4a71671 100644 --- a/server/src/config.ts +++ b/server/src/config.ts @@ -103,6 +103,20 @@ export type DeploymentConfig = { * when a remote Bot is actually running. */ managedAgent?: ManagedAgentConfig; + /** + * Private addresses an agent may be registered at, named one at a time. + * + * WHY THIS EXISTS. `AGENT_COMPUTER_ALLOW_PRIVATE_HOSTS` is a floor, not a permission: it opens this + * deployment's whole network, to browsing and to agent endpoints alike, which is why a production + * deployment refuses to start with it on. That left bring-your-own-agent — a headline capability — + * unusable in the image people are told to deploy, because a company's own agent legitimately lives + * at an internal address and the only way to reach it was to drop the floor. + * + * So the address is named instead. Nothing else is opened, browsing is not widened, and the + * never-allowed list is still checked first, so the metadata address cannot be named back in. + * Empty by default, which is the same posture as before for anybody who does not set it. + */ + agentEndpointAllowedHosts: ReadonlySet; /** * What this deployment calls itself, when more than one shares an Intelligence project. * @@ -493,6 +507,40 @@ function runtimeCapabilities(environment: Environment): RuntimeCapabilities { * cloud metadata addresses are refused underneath this either way — see `computer/target.ts` — but * that floor is the last one, not the only one worth keeping. */ +/** + * The private addresses this deployment will let an agent be registered at. + * + * A comma-separated list of hosts, each optionally with a port: `agents.internal`, + * `10.0.0.42:9000`. Matching is exact, so a name with a port pins that port and a name without one + * covers any port on that host. No suffixes and no wildcards, because a pattern that widens by + * accident is the usual way a host check fails, and naming three addresses is not onerous. + * + * A scheme or a path is a mistake worth catching here rather than at the first registration that + * silently never matches, so both are refused with the offending entry named. + */ +function agentEndpointAllowedHosts( + environment: NodeJS.ProcessEnv, +): ReadonlySet { + const named = commaSeparated(environment, "AGENT_ENDPOINT_ALLOWED_HOSTS"); + const hosts = new Set(); + for (const entry of named) { + const host = entry.trim().toLowerCase(); + if (!host) continue; + if (host.includes("/") || host.includes("://")) { + throw new Error( + `AGENT_ENDPOINT_ALLOWED_HOSTS entry "${entry}" must be a host, optionally with a port, and not a URL.`, + ); + } + if (host.includes("*")) { + throw new Error( + `AGENT_ENDPOINT_ALLOWED_HOSTS entry "${entry}" must name one host. Patterns are not accepted: list each address instead.`, + ); + } + hosts.add(host.replace(/^\[/, "").replace(/\]$/, "")); + } + return hosts; +} + function privateHostsAllowed(environment: Environment): boolean { if (optional(environment, "AGENT_COMPUTER_ALLOW_PRIVATE_HOSTS") !== "true") { return false; @@ -645,6 +693,7 @@ export function loadConfig( databaseUrl: required(environment, "DATABASE_URL"), keyEncryptionKey: keyEncryptionKey(environment), ...(managedAgent ? { managedAgent } : {}), + agentEndpointAllowedHosts: agentEndpointAllowedHosts(environment), deploymentId: optional(environment, "DEPLOYMENT_ID"), publicUrl: ( optional(environment, "OPENBOT_PUBLIC_URL") ?? auth?.baseUrl diff --git a/server/src/index.ts b/server/src/index.ts index ae13e393..4b7f2bf6 100644 --- a/server/src/index.ts +++ b/server/src/index.ts @@ -496,6 +496,8 @@ const app = createApp( // reading and the same one `createApp` takes. createAgentFetch({ allowPrivateHosts: config.computer?.allowPrivateHosts === true, + // Named addresses are reachable on every hop, not only the one that was registered. + allowedHosts: config.agentEndpointAllowedHosts, // The refusal is what the run already knows; this is what the deployment knows. Written here // rather than in `endpoint.ts` so that file keeps deciding and nothing else, the way the // target check it reuses does. diff --git a/server/tests/agent-endpoint.test.ts b/server/tests/agent-endpoint.test.ts index b74c26b1..92911df4 100644 --- a/server/tests/agent-endpoint.test.ts +++ b/server/tests/agent-endpoint.test.ts @@ -590,3 +590,135 @@ describe("dialling a stored agent endpoint", () => { expect(third.get("authorization")).toBeNull(); }); }); + +/** + * Naming a private address instead of opening the network. + * + * `AGENT_COMPUTER_ALLOW_PRIVATE_HOSTS` is a floor: it permits this deployment's whole network, to + * browsing and to agent endpoints alike, and a production deployment refuses to start with it on. + * That left bring-your-own-agent unusable in the image people are told to deploy, because a company's + * own agent lives at an internal address. Naming the address is the narrow answer, and these are the + * cases that decide whether it stays narrow. + */ +describe("private addresses named one at a time", () => { + const named = (...hosts: string[]) => new Set(hosts); + + test("an unnamed private address is still refused", () => { + const verdict = checkAgentEndpoint("http://10.0.0.42:9000/ag-ui", { + allowedHosts: named("agents.internal"), + }); + expect(verdict.allowed).toBeFalse(); + }); + + test("a named private address is allowed", () => { + const verdict = checkAgentEndpoint("http://10.0.0.42:9000/ag-ui", { + allowedHosts: named("10.0.0.42:9000"), + }); + expect(verdict.allowed).toBeTrue(); + }); + + test("naming a host without a port covers its ports", () => { + expect( + checkAgentEndpoint("http://10.0.0.42:9000/ag-ui", { + allowedHosts: named("10.0.0.42"), + }).allowed, + ).toBeTrue(); + }); + + test("naming a host with a port pins that port", () => { + // The narrow reading is the point: an operator who wrote a port meant that port. + expect( + checkAgentEndpoint("http://10.0.0.42:9999/ag-ui", { + allowedHosts: named("10.0.0.42:9000"), + }).allowed, + ).toBeFalse(); + }); + + test("the metadata address cannot be named back in", () => { + /* + * The property that makes this safe to ship. The never-allowed list is checked before the + * private rule, so an address on it is not "refused for being private" and naming it changes + * nothing. If this ever passes, the allowlist has become a way to hand a deployment's own cloud + * credentials to anybody who can register an agent. + */ + for (const address of [ + "http://169.254.169.254/latest/meta-data/", + "http://metadata.google.internal/computeMetadata/v1/", + ]) { + const verdict = checkAgentEndpoint(address, { + allowedHosts: named(new URL(address).host, new URL(address).hostname), + }); + expect(verdict.allowed).toBeFalse(); + } + }); + + test("a non-web address cannot be named back in either", () => { + expect( + checkAgentEndpoint("file:///etc/passwd", { + allowedHosts: named("", "localhost"), + }).allowed, + ).toBeFalse(); + }); + + test("naming nothing is the same as before", () => { + expect( + checkAgentEndpoint("http://10.0.0.42:9000/ag-ui", { + allowedHosts: named(), + }).allowed, + ).toBeFalse(); + expect( + checkAgentEndpoint("http://10.0.0.42:9000/ag-ui").allowed, + ).toBeFalse(); + }); + + test("a public address is unaffected by the list", () => { + expect( + checkAgentEndpoint("https://agent.example.com/ag-ui", { + allowedHosts: named("10.0.0.42"), + }).allowed, + ).toBeTrue(); + }); +}); + +/** + * The named address, across a redirect. + * + * Registration and the hop go through the same check, so a name has to mean the same thing in both + * places. If a hop ignored the list, a legitimately named agent could not redirect at all; if a hop + * were more generous than registration, the redirect would be the way around it. + */ +describe("named addresses on a redirect hop", () => { + const redirectingTo = (location: string): typeof fetch => + (async (input: string | URL | Request) => { + const target = typeof input === "string" ? input : input.toString(); + return target.includes("/start") + ? new Response(null, { status: 307, headers: { location } }) + : new Response("landed", { status: 200 }); + }) as unknown as typeof fetch; + + test("a hop to a named private address is followed", async () => { + const dial = createAgentFetch({ + allowedHosts: new Set(["10.0.0.42:9000"]), + fetchImpl: redirectingTo("http://10.0.0.42:9000/ag-ui"), + }); + const response = await dial("https://agent.example.com/start"); + expect(response.status).toBe(200); + expect(await response.text()).toBe("landed"); + }); + + test("a hop to an unnamed private address is refused", async () => { + const dial = createAgentFetch({ + allowedHosts: new Set(["10.0.0.42:9000"]), + fetchImpl: redirectingTo("http://10.0.0.99:9000/ag-ui"), + }); + await expect(dial("https://agent.example.com/start")).rejects.toThrow(); + }); + + test("a hop to the metadata address is refused however the list is written", async () => { + const dial = createAgentFetch({ + allowedHosts: new Set(["169.254.169.254"]), + fetchImpl: redirectingTo("http://169.254.169.254/latest/meta-data/"), + }); + await expect(dial("https://agent.example.com/start")).rejects.toThrow(); + }); +}); diff --git a/server/tests/config.test.ts b/server/tests/config.test.ts index d41e45dc..687b894a 100644 --- a/server/tests/config.test.ts +++ b/server/tests/config.test.ts @@ -598,3 +598,45 @@ describe("accessibility", () => { }, ); }); + +/** + * Naming the private addresses an agent may live at. + * + * The refusal cases matter as much as the parse: a list written as URLs or with a wildcard is a + * list somebody believed was working, and finding out at the first registration that silently never + * matches is worse than being told at boot. + */ +describe("AGENT_ENDPOINT_ALLOWED_HOSTS", () => { + // The suite's own base, so this describes only its subject rather than re-deriving a whole + // deployment and failing on whichever requirement it forgot. + const base = () => ({ ...baseEnvironment }); + + test("unset means none, which is the posture that shipped", () => { + expect(loadConfig(base()).agentEndpointAllowedHosts.size).toBe(0); + }); + + test("a comma-separated list is parsed, lower-cased and trimmed", () => { + const hosts = loadConfig({ + ...base(), + AGENT_ENDPOINT_ALLOWED_HOSTS: " Agents.Internal , 10.0.0.42:9000 ", + }).agentEndpointAllowedHosts; + expect([...hosts].sort()).toEqual(["10.0.0.42:9000", "agents.internal"]); + }); + + test("a URL is refused, naming the entry", () => { + expect(() => + loadConfig({ + ...base(), + AGENT_ENDPOINT_ALLOWED_HOSTS: "http://agents.internal/ag-ui", + }), + ).toThrow(/must be a host/); + }); + + test("a wildcard is refused, naming the entry", () => { + // A pattern that widens by accident is the usual way a host check fails, so there are no + // patterns to get wrong. + expect(() => + loadConfig({ ...base(), AGENT_ENDPOINT_ALLOWED_HOSTS: "*.internal" }), + ).toThrow(/Patterns are not accepted/); + }); +});