diff --git a/server/src/agents/connection-test.ts b/server/src/agents/connection-test.ts index 5648f7839..fc55e6146 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 133c00759..5d75fdb52 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 b99b87c32..2ceb58359 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 2550e57d6..6b30e6688 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 e33d2951f..762e65126 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 000000000..d2ef8cc56 --- /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); + }); +});