From f6b95a11511a386dc95d4856e8d072db7700bd01 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Mat=C3=BA=C5=A1=20Nosko?= Date: Fri, 30 Oct 2026 00:22:12 +0100 Subject: [PATCH] Resolve an agent's host before dialling it The agent endpoint check reads the address as written and does no DNS resolution, so two kinds of address passed it and were then dialled by the server with headers the registering person chose: - a single-label service name on the deployment's own network, such as a Compose or Kubernetes service - a public-looking name whose DNS answers with a private, loopback or metadata address The guarded agent fetch now resolves the host of the stored address and of every redirect hop before dialling, and refuses a single-label name or any resolved address the textual check would refuse. Hosts named in AGENT_ENDPOINT_ALLOWED_HOSTS, and hosts the deployment configures itself (the managed Bot, a picked harness, tenant package coworkers), are exempt, as is everything when the private-host floor is down. The connection test goes through the same fetch. The textual check also now treats 0.0.0.0/8 and 100.64.0.0/10 as internal. DNS rebinding between the lookup and the dial is not closed by this; it needs a pinned dialler or a network policy around the server. --- server/src/agents/connection-test.ts | 6 + server/src/agents/endpoint.ts | 150 +++++++++++++++- server/src/agents/routes.ts | 7 +- server/src/computer/target.ts | 41 +++++ server/src/index.ts | 14 +- server/tests/agent-endpoint-resolved.test.ts | 170 +++++++++++++++++++ 6 files changed, 385 insertions(+), 3 deletions(-) create mode 100644 server/tests/agent-endpoint-resolved.test.ts diff --git a/server/src/agents/connection-test.ts b/server/src/agents/connection-test.ts index 5648f78398..fc55e61468 100644 --- a/server/src/agents/connection-test.ts +++ b/server/src/agents/connection-test.ts @@ -2,6 +2,7 @@ import { checkAgentEndpoint, createAgentFetch, EndpointNotAllowedError, + type ResolveHost, } from "./endpoint"; /** @@ -140,6 +141,9 @@ export async function testAgentConnection( headers?: Record; allowPrivateHosts?: boolean; allowedHosts?: ReadonlySet; + /** Hosts this deployment's own configuration names. See `checkResolvedAgentHost`. */ + trustedHosts?: ReadonlySet; + resolveHost?: ResolveHost; fetchImpl?: typeof fetch; timeoutMs?: number; } = {}, @@ -159,6 +163,8 @@ export async function testAgentConnection( ...(options.allowPrivateHosts !== undefined ? { allowPrivateHosts: options.allowPrivateHosts } : {}), + ...(options.trustedHosts ? { trustedHosts: options.trustedHosts } : {}), + ...(options.resolveHost ? { resolveHost: options.resolveHost } : {}), ...(options.fetchImpl ? { fetchImpl: options.fetchImpl } : {}), }); let response: Response; diff --git a/server/src/agents/endpoint.ts b/server/src/agents/endpoint.ts index 133c00759e..5d75fdb523 100644 --- a/server/src/agents/endpoint.ts +++ b/server/src/agents/endpoint.ts @@ -1,4 +1,6 @@ -import { checkNavigationTarget } from "../computer/target"; +import { lookup } from "node:dns/promises"; +import { isIP } from "node:net"; +import { checkNavigationTarget, isInternalAddress } from "../computer/target"; /** * Where an external agent lives, and whether we are willing to talk to it. @@ -113,6 +115,110 @@ export function checkAgentEndpoint( return { allowed: true, url: verdict.url }; } +/** Every address a name resolves to, which is what a dial will actually reach. */ +export type ResolveHost = (hostname: string) => Promise; + +const resolveWithDns: ResolveHost = async (hostname) => + (await lookup(hostname, { all: true, verbatim: true })).map( + (entry) => entry.address, + ); + +/** The host of an address in the spelling `allowedHosts` uses, with and without its port. */ +function hostKeys(url: URL): string[] { + const hostname = url.hostname.toLowerCase(); + return url.port ? [`${hostname}:${url.port}`, hostname] : [hostname]; +} + +/** + * The hosts of addresses this deployment's own configuration names, which the resolved check below + * does not apply to. An operator pointing the managed Bot or a tenant package's coworker at a + * service name on its own network has decided that, and is not somebody registering an address. + */ +export function trustedHostsOf( + addresses: readonly (string | URL | undefined)[], +): ReadonlySet { + const hosts = new Set(); + for (const address of addresses) { + if (!address) continue; + try { + for (const key of hostKeys(new URL(address))) hosts.add(key); + } catch { + // Not a URL: nothing to trust. + } + } + return hosts; +} + +/** + * The half of the endpoint check that text cannot do: where the name actually points. + * + * `checkAgentEndpoint` reads the address as written, so `https://agent.example.com` passes however + * its DNS answers, including with 10.0.0.5 or the metadata address, and a bare service name such as + * `http://supervisor:4300` passes because it is neither an IP nor `localhost`. This server then + * POSTs to it with headers the person chose. So before a dial the name is resolved and every address + * it gives must be one the textual check would have allowed, and a single-label name is refused + * outright: nothing public is reached that way, and inside a deployment almost everything is. + * + * Skipped where the floor is down (`allowPrivateHosts`), for a host the deployment named + * (`allowedHosts`) or configured itself (`trustedHosts`), and for an IP literal, which the textual + * check has already judged. Resolution failing is a refusal: a name that does not resolve is not + * somewhere a run can go, and answering otherwise would turn a resolver hiccup into a dial. + * + * What this does not close is a name that answers differently between this lookup and the dial + * (DNS rebinding). Pinning the connection to the checked address needs a dialler of its own; a + * network policy around the server is the boundary that holds regardless. + */ +export async function checkResolvedAgentHost( + raw: string, + options: { + allowPrivateHosts?: boolean; + allowedHosts?: ReadonlySet; + trustedHosts?: ReadonlySet; + resolveHost?: ResolveHost; + } = {}, +): Promise { + let url: URL; + try { + url = new URL(raw); + } catch { + return { allowed: false, reason: "An agent needs a web address." }; + } + if (options.allowPrivateHosts) return { allowed: true, url: url.toString() }; + const keys = hostKeys(url); + if ( + keys.some( + (key) => options.allowedHosts?.has(key) || options.trustedHosts?.has(key), + ) + ) { + return { allowed: true, url: url.toString() }; + } + const hostname = url.hostname.replace(/^\[|\]$/g, "").replace(/\.+$/, ""); + if (isIP(hostname)) return { allowed: true, url: url.toString() }; + if (!hostname.includes(".")) { + return { + allowed: false, + reason: + "That is a name inside OpenBot's own network, so an agent may not live there.", + }; + } + let addresses: string[]; + try { + addresses = await (options.resolveHost ?? resolveWithDns)(hostname); + } catch { + return { + allowed: false, + reason: `${hostname} could not be resolved, so an agent cannot live there.`, + }; + } + if (addresses.length === 0 || addresses.some(isInternalAddress)) { + return { + allowed: false, + reason: `${hostname} points inside OpenBot's own network, so an agent may not live there.`, + }; + } + return { allowed: true, url: url.toString() }; +} + /** * How many redirects an agent is allowedbefore we stop believing it has somewhere to be. * @@ -258,6 +364,16 @@ export function createAgentFetch( * not be a way to arrive somewhere registration would have declined. */ allowedHosts?: ReadonlySet; + /** + * Hosts of addresses this deployment's own configuration names (the managed Bot, a tenant + * package's coworkers), exempt from the resolved check. See {@link checkResolvedAgentHost}. + */ + trustedHosts?: ReadonlySet; + /** + * How a name is resolved before it is dialled. DNS by default. A caller that substitutes + * `fetchImpl` substitutes the network, so with no resolver of its own nothing is resolved. + */ + resolveHost?: ResolveHost; fetchImpl?: typeof fetch; /** * Told about every address this refused to dial, and why. @@ -295,6 +411,24 @@ export function createAgentFetch( ...(options.allowedHosts ? { allowedHosts: options.allowedHosts } : {}), }); + const resolveHost = + options.resolveHost ?? (options.fetchImpl ? undefined : resolveWithDns); + const resolved = (address: string): Promise => + resolveHost + ? checkResolvedAgentHost(address, { + ...(options.allowPrivateHosts !== undefined + ? { allowPrivateHosts: options.allowPrivateHosts } + : {}), + ...(options.allowedHosts + ? { allowedHosts: options.allowedHosts } + : {}), + ...(options.trustedHosts + ? { trustedHosts: options.trustedHosts } + : {}), + resolveHost, + }) + : Promise.resolve({ allowed: true, url: address }); + return async function guardedFetch(url: string, init?: RequestInit) { const stored = check(url); if (!stored.allowed) { @@ -303,6 +437,13 @@ export function createAgentFetch( `${NOT_DIALLED}${url}: ${stored.reason.charAt(0).toLowerCase()}${stored.reason.slice(1)}`, ); } + const storedResolved = await resolved(stored.url); + if (!storedResolved.allowed) { + throw refuse( + url, + `${NOT_DIALLED}${url}: ${storedResolved.reason.charAt(0).toLowerCase()}${storedResolved.reason.slice(1)}`, + ); + } const origin = stored.url; let target = stored.url; @@ -330,6 +471,13 @@ export function createAgentFetch( `That address redirected to ${next}, and ${verdict.reason.charAt(0).toLowerCase()}${verdict.reason.slice(1)}`, ); } + const hopResolved = await resolved(verdict.url); + if (!hopResolved.allowed) { + throw refuse( + next, + `That address redirected to ${next}, and ${hopResolved.reason.charAt(0).toLowerCase()}${hopResolved.reason.slice(1)}`, + ); + } if (!sameCredentialScope(origin, verdict.url)) { // A body this cannot strip refuses the hop rather than forwarding it, and that refusal is // worth the same row as any other: it means a run was carrying something unreadable to a diff --git a/server/src/agents/routes.ts b/server/src/agents/routes.ts index b99b87c32d..2ceb583590 100644 --- a/server/src/agents/routes.ts +++ b/server/src/agents/routes.ts @@ -6,7 +6,7 @@ import type { AppVariables } from "../auth/guards"; import { afterExposureChange } from "../plugins/exposure-change"; import type { SharedUseStore } from "../plugins/shared-use-store"; import { testAgentConnection } from "./connection-test"; -import { checkAgentEndpoint } from "./endpoint"; +import { checkAgentEndpoint, trustedHostsOf } from "./endpoint"; import { isolationState, mayIsolate } from "./isolation"; import { isManagedEndpoint } from "./managed-endpoint"; import { canManageAgent } from "./profile-policy"; @@ -524,6 +524,11 @@ export function createAgentRoutes( ...(Object.keys(headers).length > 0 ? { headers } : {}), allowPrivateHosts, allowedHosts, + // The deployment's own Bot and harness live at names on its own network, and are not an + // address somebody is registering. + ...(managedRun + ? { trustedHosts: trustedHostsOf(managedRun.endpoints) } + : {}), }); // 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. diff --git a/server/src/computer/target.ts b/server/src/computer/target.ts index 2550e57d6b..6b30e66888 100644 --- a/server/src/computer/target.ts +++ b/server/src/computer/target.ts @@ -72,6 +72,41 @@ function isLocalhostName(hostname: string): boolean { return hostname === "localhost" || hostname.endsWith(".localhost"); } +/** + * A name with no dot in it: `postgres`, `supervisor`, `agent-computer`. + * + * Nothing on the public internet is reached by a single label. Inside a deployment almost everything + * is: Docker Compose and Kubernetes name each service this way, so `http://supervisor:4300` is this + * deployment's own Docker-socket holder however ordinary it looks as text. IP literals are excluded + * (an IPv4 address has dots, an IPv6 one colons) because they have their own checks above. + */ +function isSingleLabelHostname(hostname: string): boolean { + return hostname !== "" && !hostname.includes(".") && !hostname.includes(":"); +} + +/** + * Whether an address a name resolved to is one a public check would refuse. + * + * For callers that resolve before they dial (agents/endpoint.ts): the textual check above cannot see + * that `agent.example.com` points at 10.0.0.5, and the resolved address is the one that is dialled. + */ +export function isInternalAddress(address: string): boolean { + // An IPv4-mapped address with a dotted tail is the IPv4 address it carries. + const mapped = /^::ffff:(\d+\.\d+\.\d+\.\d+)$/i.exec(address); + if (mapped?.[1]) return isInternalAddress(mapped[1]); + const host = canonicalHostname( + address.includes(":") && !address.startsWith("[") + ? `[${address.toLowerCase()}]` + : address.toLowerCase(), + ); + return ( + isNeverAllowedHostname(host) || + INTERNAL_HOSTNAMES.has(host) || + isPrivateIpv4(host) || + isPrivateIpv6(host) + ); +} + export type TargetVerdict = | { allowed: true; url: string } | { allowed: false; reason: string }; @@ -186,7 +221,12 @@ function isPrivateIpv4(hostname: string): boolean { return false; } const [a, b] = octets as [number, number, number, number]; + // "This network": 0.0.0.0 reaches the local host on Linux, and the rest of 0/8 is never a server. + if (a === 0) return true; if (a === 10) return true; + // Shared address space (RFC 6598). Carrier-grade NAT, and the range cloud providers use for + // internal services, so it is no more a public address than 10/8 is. + if (a === 100 && b >= 64 && b <= 127) return true; if (a === 127) return true; if (a === 169 && b === 254) return true; // link-local, includes metadata if (a === 172 && b >= 16 && b <= 31) return true; @@ -284,6 +324,7 @@ export function checkNavigationTarget( if ( INTERNAL_HOSTNAMES.has(hostname) || isLocalhostName(hostname) || + isSingleLabelHostname(hostname) || isPrivateIpv4(hostname) || isPrivateIpv6(hostname) ) { diff --git a/server/src/index.ts b/server/src/index.ts index e33d2951f2..762e651261 100644 --- a/server/src/index.ts +++ b/server/src/index.ts @@ -31,7 +31,7 @@ import { } from "./agents/callback-token"; import { canUseComputer } from "./agents/computer-access"; import { wireCoordination } from "./agents/coordination-wiring"; -import { createAgentFetch } from "./agents/endpoint"; +import { createAgentFetch, trustedHostsOf } from "./agents/endpoint"; import { createHandoffDesk } from "./agents/handoff"; import { createHandoffRunner } from "./agents/handoff-runner"; import { startHandoffDelivery } from "./agents/handoff-wiring"; @@ -1678,6 +1678,18 @@ const agentFetch = createAgentFetch({ allowPrivateHosts: config.computer?.allowPrivateHosts === true, // Named addresses are reachable on every hop, not only the one that was registered. allowedHosts: config.agentEndpointAllowedHosts, + // Addresses this deployment's own configuration points at: the managed Bot, a harness picked during + // setup, and a tenant package's remote coworkers. Exempt from resolving where a name points, + // because they are the operator's decision rather than an address somebody registered. + trustedHosts: trustedHostsOf([ + config.managedAgent?.endpoint, + config.managedAgent?.alsoRun, + ...tenantPackage.agents.map((agent) => + typeof agent.configuration.endpoint === "string" + ? agent.configuration.endpoint + : undefined, + ), + ]), // 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-resolved.test.ts b/server/tests/agent-endpoint-resolved.test.ts new file mode 100644 index 0000000000..d2ef8cc56b --- /dev/null +++ b/server/tests/agent-endpoint-resolved.test.ts @@ -0,0 +1,170 @@ +import { describe, expect, test } from "bun:test"; +import { + checkResolvedAgentHost, + createAgentFetch, + EndpointNotAllowedError, + trustedHostsOf, +} from "../src/agents/endpoint"; +import { + checkNavigationTarget, + isInternalAddress, +} from "../src/computer/target"; + +/** + * Where an agent's name actually points, checked before this server dials it. + * + * The textual check reads the address as written. These are the addresses it cannot see through: + * a bare service name on the deployment's own network, and a public-looking name whose DNS answers + * with a private or metadata address. + */ + +const resolvesTo = + (addresses: string[]) => + async (_hostname: string): Promise => + addresses; + +describe("a name is judged by where it resolves", () => { + test("a public name that resolves to a public address is allowed", async () => { + const verdict = await checkResolvedAgentHost( + "https://agent.example.com/ag-ui", + { + resolveHost: resolvesTo(["93.184.216.34"]), + }, + ); + expect(verdict.allowed).toBe(true); + }); + + test.each([ + ["10.0.0.5"], + ["169.254.169.254"], + ["127.0.0.1"], + ["100.64.1.2"], + ["::1"], + ["::ffff:10.0.0.5"], + ["fd00::1"], + ])("a public name that resolves to %s is refused", async (address) => { + const verdict = await checkResolvedAgentHost( + "https://agent.example.com/ag-ui", + { + resolveHost: resolvesTo(["93.184.216.34", address]), + }, + ); + expect(verdict.allowed).toBe(false); + }); + + test("a single-label service name is refused without resolving it", async () => { + let asked = false; + const verdict = await checkResolvedAgentHost( + "http://supervisor:4300/computers", + { + resolveHost: async () => { + asked = true; + return ["172.18.0.4"]; + }, + }, + ); + expect(verdict.allowed).toBe(false); + expect(asked).toBe(false); + }); + + test("a name that does not resolve is refused", async () => { + const verdict = await checkResolvedAgentHost( + "https://nowhere.example.com/", + { + resolveHost: async () => { + throw new Error("ENOTFOUND"); + }, + }, + ); + expect(verdict.allowed).toBe(false); + }); + + test("a host the deployment named or configured is not resolved", async () => { + const never = async (): Promise => { + throw new Error("must not resolve"); + }; + expect( + ( + await checkResolvedAgentHost("http://agent-bot:4200/ag-ui", { + trustedHosts: trustedHostsOf(["http://agent-bot:4200/ag-ui"]), + resolveHost: never, + }) + ).allowed, + ).toBe(true); + expect( + ( + await checkResolvedAgentHost("http://agents.internal:8443/", { + allowedHosts: new Set(["agents.internal:8443"]), + resolveHost: never, + }) + ).allowed, + ).toBe(true); + }); + + test("the private-host floor being down skips the check, as it does for text", async () => { + const verdict = await checkResolvedAgentHost("http://supervisor:4300/", { + allowPrivateHosts: true, + }); + expect(verdict.allowed).toBe(true); + }); +}); + +describe("the guarded fetch resolves before every dial", () => { + test("a stored address whose name points inside is never dialled", async () => { + let dialled = false; + const guarded = createAgentFetch({ + resolveHost: resolvesTo(["10.0.0.5"]), + fetchImpl: (async () => { + dialled = true; + return new Response(""); + }) as unknown as typeof fetch, + }); + await expect( + guarded("https://agent.example.com/ag-ui", { method: "POST" }), + ).rejects.toBeInstanceOf(EndpointNotAllowedError); + expect(dialled).toBe(false); + }); + + test("a redirect to a name that points inside is refused", async () => { + const dialled: string[] = []; + const guarded = createAgentFetch({ + resolveHost: async (hostname) => + hostname === "agent.example.com" + ? ["93.184.216.34"] + : ["169.254.169.254"], + fetchImpl: (async (url: string) => { + dialled.push(url); + return new Response(null, { + status: 307, + headers: { location: "https://rebound.example.net/latest" }, + }); + }) as unknown as typeof fetch, + }); + await expect( + guarded("https://agent.example.com/ag-ui", { method: "POST" }), + ).rejects.toBeInstanceOf(EndpointNotAllowedError); + expect(dialled).toEqual(["https://agent.example.com/ag-ui"]); + }); +}); + +describe("address ranges the textual check now treats as internal", () => { + test.each([ + ["http://0.0.0.1/"], + ["http://100.64.0.1/"], + ["http://100.127.255.255/"], + ])("%s is refused", (address) => { + expect(checkNavigationTarget(address).allowed).toBe(false); + }); + + test("100.63.x and 100.128.x are still public", () => { + expect(checkNavigationTarget("http://100.63.0.1/").allowed).toBe(true); + expect(checkNavigationTarget("http://100.128.0.1/").allowed).toBe(true); + }); + + test("isInternalAddress reads resolver output", () => { + expect(isInternalAddress("10.1.2.3")).toBe(true); + expect(isInternalAddress("fe80::1")).toBe(true); + expect(isInternalAddress("8.8.8.8")).toBe(false); + expect(isInternalAddress("2606:4700::1111")).toBe(false); + }); +});