diff --git a/.changeset/authorize-container-http.md b/.changeset/authorize-container-http.md new file mode 100644 index 00000000..8899fceb --- /dev/null +++ b/.changeset/authorize-container-http.md @@ -0,0 +1,5 @@ +--- +"@cloudflare/computer": minor +--- + +The container's HTTP surface now requires a bearer token. The host generates a secret, passes it to the container as RPC_CLIENT_SECRET at launch, and sends it on /connect. Readiness at /health stays open, and leaving the variable unset disables the checks. Before opening a session the host checks that the container refuses an unauthenticated request and fails the connect if it does not, so a container or image predating this has to be recycled. The container's dial-back to the host carries the same secret, and the host refuses an upgrade that does not present it. diff --git a/.changeset/container-launch-spec.md b/.changeset/container-launch-spec.md new file mode 100644 index 00000000..7cc4b90c --- /dev/null +++ b/.changeset/container-launch-spec.md @@ -0,0 +1,5 @@ +--- +"@cloudflare/computer": minor +--- + +`IWorkspaceContainerAPI.start()` and `restart()` now take a single `ContainerLaunchSpec` of `{ env, enableInternet }` instead of two arguments, and return which of `launched`, `adopted` or `relaunched` happened. Each launch records its spec, and a container found already running is relaunched unless it matches, because neither the environment nor the internet flag can be changed on a live container. A container started outside this API has no record and is relaunched rather than trusted. `setInactivityTimeout()` joins the interface so a caller that pre-starts containers, such as a warm pool, does not need to reach past it. diff --git a/.changeset/exec-env-allowlist.md b/.changeset/exec-env-allowlist.md new file mode 100644 index 00000000..a9dc991a --- /dev/null +++ b/.changeset/exec-env-allowlist.md @@ -0,0 +1,5 @@ +--- +"@cloudflare/computer": minor +--- + +A command run through the shell no longer inherits the container's whole environment. It receives PATH, HOME, TMPDIR, TZ, LANG, TERM, the LC_ family, and any variable prefixed COMPUTER_VAR_, which arrives with the prefix stripped so COMPUTER_VAR_NODE_ENV becomes NODE_ENV. A workspace that relied on some other inherited variable needs the prefix. diff --git a/docs/07_injected_service.md b/docs/07_injected_service.md index e3b8b58f..a980eec1 100644 --- a/docs/07_injected_service.md +++ b/docs/07_injected_service.md @@ -49,7 +49,7 @@ backend pins it to `8080`) and serves: | Route | Method | Purpose | | --- | --- | --- | -| `/health` | `GET`, `HEAD` | Liveness probe; `200 ok\n` as soon as the HTTP server binds. | +| `/health` | `GET`, `HEAD` | Liveness probe; `200 ok\n` as soon as the HTTP server binds. The only route reachable without the shared secret. | | `/__computerd/info` | `GET` | Runtime info: FUSE backend, mount point, port. | | `/api` | `GET` (upgrade) | WebSocket capnweb transport — the bootstrap stub is `WorkspaceRPC`. Only the exact path upgrades. A request without an `Upgrade` header gets `400`; an unsupported `Sec-WebSocket-Version` gets `426` and the versions the server speaks. | | `/api/watermarks` | `GET`, `HEAD` | Sync revisions: `currentRev`, `pushRev`, `fetchCursor`. The same values `sync.watermarks()` returns, for callers that want a few numbers without holding a session. | @@ -121,13 +121,20 @@ Provider-agnostic shape — three steps, in order: `CloudflareContainerBackend` (`packages/computer/src/backends/container/cloudflare-container.ts`) wires it like this: -1. **Start.** `container.start({ enableInternet, env })` on the - Cloudflare Containers API — not the `@cloudflare/sandbox` SDK. - Idempotence comes from `container.running` plus a cached `#handle`; - there is no process-name registry, no `startProcess`/`getProcess`, - and no `node /app/...` command (the container's `ENTRYPOINT` runs - `computerd` directly). `containerEnv` pins `PORT=8080` and lets the - image's own `FUSE_MOUNT` value (typically `auto`) win. +1. **Start.** `WorkspaceContainerAPI.start({ env, enableInternet })`, + which reaches the Cloudflare Containers API — not the + `@cloudflare/sandbox` SDK. There is no process-name registry, no + `startProcess`/`getProcess`, and no `node /app/...` command (the + container's `ENTRYPOINT` runs `computerd` directly). `containerEnv` + pins `PORT=8080` and lets the image's own `FUSE_MOUNT` value + (typically `auto`) win, and the API adds `RPC_CLIENT_SECRET`. + + Neither the environment nor the internet flag can be changed on a + running container, so the launch records both and a container found + already running is only adopted when it matches. Otherwise it is + relaunched, which is what keeps a warm pool from handing a workspace + a container configured for something else. A container started + outside this API has no record and is relaunched too. 2. **Wire egress.** `container.interceptOutboundHttp(egressHost, egress)` routes outbound HTTP from the container at `egressHost` back to a Worker `Fetcher` the DO controls. @@ -163,6 +170,8 @@ These are the variables `computerd` actually consumes (see | `MOUNT_POINT` | `/workspace` | Absolute path inside the container to mount the FUSE filesystem at. Ignored when `FUSE_MOUNT=none`. | | `FUSE_MOUNT` | `auto` | Backend selector: `auto` probes `/dev/fuse` (linux) or macFUSE (darwin) and falls back to the userspace shim; `fuse` / `macfuse` require the corresponding real backend; `shim` forces the userspace shim; `none` skips the mount entirely. | | `EXEC_LOG_MAX_BYTES` | runner default | Caps the per-exec stdout/stderr log retained in-memory. | +| `RPC_CLIENT_SECRET` | unset | When set, every route except `/health` requires it as `Authorization: Bearer `, including the `/api` upgrade. Unset leaves the surface open. The Cloudflare backend generates one per workspace and sets it at launch. | +| `COMPUTER_VAR_*` | unset | Forwarded into every `shell.exec` command with the prefix stripped, so `COMPUTER_VAR_NODE_ENV` arrives as `NODE_ENV`. | | `LOG_FILE` | unset | If set, every `console.log` / `console.error` line and any `uncaughtException` / `unhandledRejection` is also appended to this file. Stdout/stderr behaviour is unchanged. | When `LOG_FILE` is set, `computerd` mirrors console output into the file in diff --git a/examples/think-compare-runtimes/worker/computer-container-pool.test.ts b/examples/think-compare-runtimes/worker/computer-container-pool.test.ts index 75ab3c6c..9249ac34 100644 --- a/examples/think-compare-runtimes/worker/computer-container-pool.test.ts +++ b/examples/think-compare-runtimes/worker/computer-container-pool.test.ts @@ -1,3 +1,4 @@ +import type { ContainerLaunchSpec } from "@cloudflare/computer/backends/container"; import { describe, expect, test, vi } from "vitest"; import type { WorkspaceContainerHost } from "./computer-container-pool"; @@ -11,8 +12,11 @@ describe("createWorkspaceWarmPoolRuntime", () => { const { createWorkspaceWarmPoolRuntime } = await import("./computer-container-pool"); const calls: string[] = []; const host = { - async startWarmContainer(env: Record, inactivityTimeoutMs: number) { - calls.push(`start ${env.PORT} ${env.MOUNT_POINT} ${env.FUSE_MOUNT} ${inactivityTimeoutMs}`); + async startWarmContainer(spec: ContainerLaunchSpec, inactivityTimeoutMs: number) { + const { env, enableInternet } = spec; + calls.push( + `start ${env.PORT} ${env.MOUNT_POINT} ${env.FUSE_MOUNT} internet=${enableInternet} ${inactivityTimeoutMs}`, + ); }, async destroyWarmContainer() { calls.push("destroy"); @@ -36,22 +40,25 @@ describe("createWorkspaceWarmPoolRuntime", () => { await runtime.startContainer("warm-a"); await expect(runtime.isContainerRunning("warm-a")).resolves.toBe(true); - expect(calls).toEqual(["start 8080 /workspace shim 120000", "healthy"]); + expect(calls).toEqual(["start 8080 /workspace shim internet=true 120000", "healthy"]); }); test("retries Workspace container placement while waiting for health", async () => { const { startWorkspaceContainerAndWait } = await import("./computer-container-pool"); const calls: string[] = []; let healthAttempts = 0; - const container = { - running: false, + // Stands in for the workspace container API rather than + // ctx.container: the warm start goes through the API so the launch + // carries the shared secret and is recorded for adoption. + const api = { async setInactivityTimeout(durationMs: number) { calls.push(`timeout ${durationMs}`); }, - start() { + async start() { calls.push("start"); + return { runtimeId: "runtime", clientSecret: "secret", outcome: "launched" as const }; }, - getTcpPort() { + port() { return { async fetch() { healthAttempts += 1; @@ -61,17 +68,18 @@ describe("createWorkspaceWarmPoolRuntime", () => { "There is no container instance that can be provided to this Durable Object, try again later", ); } - container.running = true; return new Response(null, { status: 200 }); }, } as unknown as Fetcher; }, }; - await startWorkspaceContainerAndWait(container, { PORT: "8080" }, 120_000, { - attempts: 3, - wait: async () => {}, - }); + await startWorkspaceContainerAndWait( + api, + { env: { PORT: "8080" }, enableInternet: true }, + 120_000, + { attempts: 3, wait: async () => {} }, + ); expect(calls).toEqual([ "timeout 120000", diff --git a/examples/think-compare-runtimes/worker/computer-container-pool.ts b/examples/think-compare-runtimes/worker/computer-container-pool.ts index dbee7364..5ba3111d 100644 --- a/examples/think-compare-runtimes/worker/computer-container-pool.ts +++ b/examples/think-compare-runtimes/worker/computer-container-pool.ts @@ -1,5 +1,6 @@ import { DurableObject } from "cloudflare:workers"; import { + type ContainerLaunchSpec, type IWorkspaceContainerAPI, withWorkspaceContainer, } from "@cloudflare/computer/backends/container"; @@ -17,7 +18,7 @@ export interface WorkspacePoolEnv extends ContainerPoolConfigEnv { export interface WorkspaceContainerHostHandle { getWorkspaceContainer(): IWorkspaceContainerAPI | Promise; - startWarmContainer(env: Record, inactivityTimeoutMs: number): Promise; + startWarmContainer(spec: ContainerLaunchSpec, inactivityTimeoutMs: number): Promise; destroyWarmContainer(): Promise; isWarmContainerHealthy(): Promise; } @@ -35,11 +36,12 @@ class WorkspaceContainerHostBase extends withWorkspaceContainer( ) {} export class WorkspaceContainerHost extends WorkspaceContainerHostBase { - async startWarmContainer( - env: Record, - inactivityTimeoutMs: number, - ): Promise { - await startWorkspaceContainerAndWait(this.getContainer(), env, inactivityTimeoutMs); + async startWarmContainer(spec: ContainerLaunchSpec, inactivityTimeoutMs: number): Promise { + // Through the workspace API rather than ctx.container, so the launch + // carries whatever the API adds — today the shared secret the + // daemon's HTTP surface requires — and is recorded, so the workspace + // that adopts this container can tell it matches. + await startWorkspaceContainerAndWait(this.getWorkspaceContainer(), spec, inactivityTimeoutMs); } async destroyWarmContainer(): Promise { @@ -69,7 +71,7 @@ export function createWorkspaceWarmPoolRuntime(env: WorkspacePoolEnv): WarmPoolR async startContainer(containerId) { const host = getWorkspaceContainerHost(env, containerId); try { - await host.startWarmContainer(workspaceContainerEnv(env), containerSleepAfterMs(env)); + await host.startWarmContainer(workspaceLaunchSpec(env), containerSleepAfterMs(env)); } catch (error) { console.warn({ message: "Workspace warm container failed to start", @@ -103,19 +105,28 @@ function getWorkspaceContainerHost( ) as unknown as WorkspaceContainerHostHandle; } -function workspaceContainerEnv(env: WorkspacePoolEnv): Record { +function workspaceLaunchSpec(env: WorkspacePoolEnv): ContainerLaunchSpec { return { - PORT: String(WORKSPACE_PORT), - MOUNT_POINT: "/workspace", - ...(env.FUSE_MOUNT ? { FUSE_MOUNT: env.FUSE_MOUNT } : {}), + env: { + PORT: String(WORKSPACE_PORT), + MOUNT_POINT: "/workspace", + ...(env.FUSE_MOUNT ? { FUSE_MOUNT: env.FUSE_MOUNT } : {}), + }, + // A pool cannot know the egress policy of the workspace that will + // adopt a container, so this has to agree with it by configuration. + // Disagreeing costs a relaunch on adoption, not the policy: the + // adopting workspace compares this spec against its own and + // replaces the container rather than inheriting the wrong one. + enableInternet: true, }; } -interface WorkspaceContainerControl { - readonly running: boolean; +// The subset of the workspace container API a warm start needs. +// Structurally satisfied by IWorkspaceContainerAPI. +interface WorkspaceWarmStartAPI { setInactivityTimeout(durationMs: number): Promise; - start(options: { enableInternet: boolean; env: Record }): void; - getTcpPort(port: number): Fetcher; + start(spec: ContainerLaunchSpec): Promise; + port(port: number): Fetcher; } interface WorkspaceStartWaitOptions { @@ -124,22 +135,23 @@ interface WorkspaceStartWaitOptions { } export async function startWorkspaceContainerAndWait( - container: WorkspaceContainerControl, - env: Record, + api: WorkspaceWarmStartAPI, + spec: ContainerLaunchSpec, inactivityTimeoutMs: number, options: WorkspaceStartWaitOptions = {}, ): Promise { const attempts = options.attempts ?? 120; const wait = options.wait ?? ((durationMs) => scheduler.wait(durationMs)); - await container.setInactivityTimeout(inactivityTimeoutMs); + await api.setInactivityTimeout(inactivityTimeoutMs); let lastError: unknown; for (let attempt = 0; attempt < attempts; attempt += 1) { - if (!container.running) { - container.start({ enableInternet: true, env }); - } + // Called unconditionally: start() adopts a container already running + // with this spec, so there is no need to check `running` first the + // way a raw ctx.container.start() did. + await api.start(spec); try { - await waitForWorkspaceHealth(() => container.getTcpPort(WORKSPACE_PORT), 1); + await waitForWorkspaceHealth(() => api.port(WORKSPACE_PORT), 1); return; } catch (error) { lastError = error; diff --git a/packages/computer/src/backends/container/cloudflare-container.test.ts b/packages/computer/src/backends/container/cloudflare-container.test.ts index 51eebe92..f34a0135 100644 --- a/packages/computer/src/backends/container/cloudflare-container.test.ts +++ b/packages/computer/src/backends/container/cloudflare-container.test.ts @@ -16,6 +16,11 @@ import { CloudflareContainerBackend } from "./cloudflare-container.js"; import type { IWorkspaceContainerAPI, WorkspaceRef } from "./container-host.js"; interface FakeHostOptions { + // Status the container returns for an unauthenticated request to a + // gated route. Defaults to 401, meaning the secret is enforced. + authProbeStatus?: number; + // Accept the probe and never answer it, to prove the request is bounded. + authProbeHang?: boolean; healthy?: boolean; // Health probe sequence: each connect() reads from the head of // this array. true = answer 200, false = throw "connection @@ -35,6 +40,8 @@ interface FakeHost { host: IWorkspaceContainerAPI; calls: { name: string; args: unknown[] }[]; connectBody?: Record; + connectAuthorization?: string | null; + clientSecret?: string; startEnv?: Record; enableInternet?: boolean; interceptedHost?: string; @@ -71,17 +78,23 @@ function makeFakeHost(opts: FakeHostOptions = {}): FakeHost { } state.host = { - async start(env, enableInternet) { + async start(spec) { + const { env, enableInternet } = spec; calls.push({ name: "start", args: [env, enableInternet] }); state.startEnv = env; state.enableInternet = enableInternet; await opts.start?.(); if (!state.running) state.runtimeId = crypto.randomUUID(); + state.clientSecret ??= "00112233445566778899aabbccddeeff"; state.running = true; // A successful start clears any prior exit, matching // WorkspaceContainerAPI.start. state.exit = null; - return { runtimeId: state.runtimeId ?? "missing-runtime" }; + return { + runtimeId: state.runtimeId ?? "missing-runtime", + clientSecret: state.clientSecret ?? "unset", + outcome: "launched", + }; }, async interceptOutboundHttp(host, ref) { calls.push({ name: "interceptOutboundHttp", args: [host, ref] }); @@ -102,11 +115,24 @@ function makeFakeHost(opts: FakeHostOptions = {}): FakeHost { if (!nextHealthy()) throw new Error("connection refused"); return new Response(null, { status: 200 }); } + if (url.pathname === "/api") { + // An enforcing container refuses an unauthenticated request here. + // authProbeStatus models one that does not; authProbeHang models + // one that accepts the connection and never answers. + if (opts.authProbeHang) { + await new Promise((resolve) => { + init?.signal?.addEventListener("abort", resolve, { once: true }); + }); + throw new Error("aborted"); + } + return new Response(null, { status: opts.authProbeStatus ?? 401 }); + } if (url.pathname === "/connect") { - state.connectBody = await request + state.connectBody = (await request .clone() .json() - .catch(() => undefined); + .catch(() => undefined)) as Record | undefined; + state.connectAuthorization = request.headers.get("authorization"); if (connectStatus !== 200) { return new Response(`/connect ${connectStatus}`, { status: connectStatus }); } @@ -117,7 +143,8 @@ function makeFakeHost(opts: FakeHostOptions = {}): FakeHost { port() { throw new Error("cross-boundary Fetchers should not be used by CloudflareContainerBackend"); }, - async restart(env, enableInternet) { + async restart(spec) { + const { env, enableInternet } = spec; calls.push({ name: "restart", args: [env, enableInternet] }); if (opts.restart) { await opts.restart(); @@ -125,7 +152,15 @@ function makeFakeHost(opts: FakeHostOptions = {}): FakeHost { state.running = true; state.exit = null; state.runtimeId = crypto.randomUUID(); - return { runtimeId: state.runtimeId }; + state.clientSecret ??= "00112233445566778899aabbccddeeff"; + return { + runtimeId: state.runtimeId, + clientSecret: state.clientSecret, + outcome: "launched", + }; + }, + async setInactivityTimeout(durationMs: number) { + calls.push({ name: "setInactivityTimeout", args: [durationMs] }); }, async status() { calls.push({ name: "status", args: [] }); @@ -404,6 +439,63 @@ describe("CloudflareContainerBackend", () => { api: "/api", }); expect(typeof fake.connectBody?.healthTimeoutMs).toBe("number"); + // The daemon refuses an unauthorized request once it has a secret, + // so the bearer token travels with the dial-back instruction. + expect(fake.connectAuthorization).toBe(`Bearer ${fake.clientSecret}`); + }); + + test("connect() fails when the container accepts an unauthenticated request", async () => { + // An image predating RPC_CLIENT_SECRET ignores the variable and serves + // everything. Connecting anyway would hand a session to a container + // that is not authorizing anyone, so this refuses instead. + const fake = makeFakeHost({ authProbeStatus: 200 }); + const backend = new CloudflareContainerBackend({ + container: () => ({ getWorkspaceContainer: () => fake.host }), + workspace: fakeWorkspace, + connectTimeoutMs: 600, + }); + + const error = await backend.connect().catch((caught: unknown) => caught); + + expect(error).toBeInstanceOf(WorkspaceTransportError); + expect(String(error)).toMatch(/stage=auth/); + expect(String(error)).toMatch(/without authorization/); + expect(String(error)).toMatch(/recycled/); + }); + + test("connect() gets past the check when the container refuses an unauthenticated request", async () => { + const fake = makeFakeHost(); + const backend = new CloudflareContainerBackend({ + container: () => ({ getWorkspaceContainer: () => fake.host }), + workspace: fakeWorkspace, + connectTimeoutMs: 600, + }); + + const error = await backend.connect().catch((caught: unknown) => caught); + + // It still fails, but on the upgrade rather than the auth check: there + // is no WebSocketPair in this environment. + expect(String(error)).not.toMatch(/stage=auth/); + expect(fake.calls.map((c) => c.name)).toContain("fetchPort"); + }); + + test("connect() does not hang when the container never answers the check", async () => { + // The probe is on the critical path, so an unbounded request would + // wedge the connect past its own timeout. + const fake = makeFakeHost({ authProbeHang: true }); + const backend = new CloudflareContainerBackend({ + container: () => ({ getWorkspaceContainer: () => fake.host }), + workspace: fakeWorkspace, + connectTimeoutMs: 600, + healthProbeTimeoutMs: 100, + }); + + const started = Date.now(); + await backend.connect().catch(() => {}); + + // A probe that cannot answer is inconclusive, so the connect carries + // on and fails later on the upgrade, well inside its own budget. + expect(Date.now() - started).toBeLessThan(5_000); }); test("connect() throws a transport error when the /api upgrade never arrives", async () => { @@ -439,6 +531,73 @@ describe("CloudflareContainerBackend", () => { expect(res.status).toBe(400); }); + test("handleFetch refuses a dial-back that does not present the secret", async () => { + // The armed slot hands its session to whoever arrives first, and this + // endpoint is reachable from inside the container. Without a token, + // any command the workspace runs could take the daemon's place. + const fake = makeFakeHost(); + const backend = new CloudflareContainerBackend({ + container: () => ({ getWorkspaceContainer: () => fake.host }), + workspace: fakeWorkspace, + connectTimeoutMs: 600, + }); + // connect() records the secret and arms the slot; it then fails on the + // upgrade, which is what leaves the slot open for these requests. + await backend.connect().catch(() => {}); + const upgrade = { upgrade: "websocket" }; + + const none = await backend.handleFetch( + new Request("http://computer.internal/api", { headers: upgrade }), + ); + expect(none.status).toBe(401); + expect(none.headers.get("www-authenticate")).toBe("Bearer"); + + const wrong = await backend.handleFetch( + new Request("http://computer.internal/api", { + headers: { ...upgrade, authorization: `Bearer ${"f".repeat(32)}` }, + }), + ); + expect(wrong.status).toBe(401); + + const shortToken = await backend.handleFetch( + new Request("http://computer.internal/api", { + headers: { ...upgrade, authorization: "Bearer short" }, + }), + ); + expect(shortToken.status).toBe(401); + + const otherScheme = await backend.handleFetch( + new Request("http://computer.internal/api", { + headers: { ...upgrade, authorization: `Basic ${fake.clientSecret}` }, + }), + ); + expect(otherScheme.status).toBe(401); + }); + + test("handleFetch accepts a dial-back presenting the secret, in any scheme case", async () => { + const fake = makeFakeHost(); + const backend = new CloudflareContainerBackend({ + container: () => ({ getWorkspaceContainer: () => fake.host }), + workspace: fakeWorkspace, + connectTimeoutMs: 600, + }); + await backend.connect().catch(() => {}); + + // WebSocketPair does not exist outside workerd, so getting past the + // check is what is observable here: it fails constructing the pair + // rather than answering 401. + for (const scheme of ["Bearer", "bearer", "BEARER"]) { + const res = await backend + .handleFetch( + new Request("http://computer.internal/api", { + headers: { upgrade: "websocket", authorization: `${scheme} ${fake.clientSecret}` }, + }), + ) + .catch(() => null); + expect(res?.status, scheme).not.toBe(401); + } + }); + test("connect() consults host.exitInfo() before host.start()", async () => { const fake = makeFakeHost(); const backend = new CloudflareContainerBackend({ diff --git a/packages/computer/src/backends/container/cloudflare-container.ts b/packages/computer/src/backends/container/cloudflare-container.ts index 155a7cb1..fc7445b6 100644 --- a/packages/computer/src/backends/container/cloudflare-container.ts +++ b/packages/computer/src/backends/container/cloudflare-container.ts @@ -151,6 +151,33 @@ const DEFAULT_HEALTH_PROBE_TIMEOUT_MS = 2_000; const DEFAULT_HEALTH_RETRY_INITIAL_DELAY_MS = 250; const DEFAULT_HEALTH_RETRY_MAX_DELAY_MS = 2_000; +// Bearer check for the dial-back. The scheme token is case-insensitive +// and may be followed by more than one space, matching what the daemon +// accepts on its own surface. +// +// The comparison walks every byte rather than stopping at the first +// difference, so it does not leak how much of the secret was correct. +// crypto.subtle.timingSafeEqual would be the primitive to reach for, but +// the SubtleCrypto this package compiles against does not declare it. +// +// An absent expected secret refuses everything rather than allowing it. +// This only runs once connect() has recorded the secret it launched the +// container with, so an absent one means the upgrade arrived without that +// having happened. +function bearerMatches(header: string | null, expected: string | undefined): boolean { + if (expected === undefined || header === null) return false; + const separator = header.indexOf(" "); + if (separator === -1) return false; + if (header.slice(0, separator).toLowerCase() !== "bearer") return false; + const presented = header.slice(separator + 1).trim(); + if (presented.length !== expected.length) return false; + let differences = 0; + for (let i = 0; i < expected.length; i += 1) { + differences |= presented.charCodeAt(i) ^ expected.charCodeAt(i); + } + return differences === 0; +} + export class CloudflareContainerBackend implements WorkspaceBackend { readonly type = "cloudflare-container"; readonly id: string; @@ -164,6 +191,9 @@ export class CloudflareContainerBackend implements WorkspaceBackend { Pick; readonly #egress: WorkspaceEgressPolicy; readonly #egressToken: string | undefined; + // Set once start() reports it, before the upgrade slot is armed, so + // handleFetch can check the dial-back against it. + #clientSecret: string | undefined; // State for the in-flight /api upgrade. handleFetch() resolves // #pendingUpgrade; connect() awaits it. @@ -216,8 +246,15 @@ export class CloudflareContainerBackend implements WorkspaceBackend { ...this.#options.containerEnv, }; let runtimeId: string; + // The container requires this on its HTTP surface. It is durable, so + // the value is the same across incarnations and across a replaced + // container. + let clientSecret: string; try { - ({ runtimeId } = await host.start(env, this.#egress.mode === "direct")); + ({ runtimeId, clientSecret } = await host.start({ + env, + enableInternet: this.#egress.mode === "direct", + })); } catch (error) { throw new WorkspaceTransportError( this.#formatStageError("start", { @@ -248,13 +285,16 @@ export class CloudflareContainerBackend implements WorkspaceBackend { ); } + this.#clientSecret = clientSecret; + // Arm the upgrade promise before posting /connect — computerd // dials back as soon as /health on the egress answers, so // the upgrade can arrive before the POST resolves. this.#armUpgrade(); runtimeId = await this.#readyWithRestarts(host, env, deadline, priorExit, runtimeId); - await this.#postConnect(host, deadline); + await this.#requireAuthEnforced(host, deadline); + await this.#postConnect(host, deadline, clientSecret); const ws = await this.#waitForUpgrade(deadline); const stub = newWebSocketRpcSession( @@ -368,6 +408,18 @@ export class CloudflareContainerBackend implements WorkspaceBackend { if (req.headers.get("upgrade") !== "websocket") { return new Response(`${EGRESS_API_PATH} requires a websocket upgrade`, { status: 400 }); } + // The slot armed by connect() hands its session to whoever arrives + // first, and this endpoint is reachable from inside the container + // through the egress interceptor. Without a token, any command the + // workspace runs could take the daemon's place and become the peer + // this durable object pushes its files to and takes its exec output + // from. The daemon presents the secret it was launched with. + if (!bearerMatches(req.headers.get("authorization"), this.#clientSecret)) { + return new Response("unauthorized", { + status: 401, + headers: { "www-authenticate": "Bearer" }, + }); + } const pair = new WebSocketPair(); const [client, server] = [pair[0], pair[1]]; @@ -442,7 +494,10 @@ export class CloudflareContainerBackend implements WorkspaceBackend { if (attempt < maxAttempts) { try { - ({ runtimeId } = await host.restart(env, this.#egress.mode === "direct")); + ({ runtimeId } = await host.restart({ + env, + enableInternet: this.#egress.mode === "direct", + })); restarts++; } catch (error) { this.#rejectUpgrade?.(error); @@ -519,13 +574,68 @@ export class CloudflareContainerBackend implements WorkspaceBackend { ); } - async #postConnect(host: IWorkspaceContainerAPI, deadline: number): Promise { + // Confirm the container refuses an unauthorized request before handing + // it a session. Launching with the secret is arranged elsewhere, but an + // image built before the daemon understood RPC_CLIENT_SECRET ignores it + // and serves everything, and that is invisible from the host: the + // bearer token goes out and the container is content either way. + // + // /api is the probe target because it exists on every daemon that has a + // capnweb endpoint at all, so this check does not depend on which + // diagnostic routes a given build happens to expose. An enforcing + // daemon answers 401 before it looks at the route; one that is not + // enforcing answers whatever the route says for an unauthenticated + // GET. + // + // A definite answer other than 401 fails the connect. Recycling a + // container that predates the secret is the cost of the upgrade, and it + // is preferable to a workspace that believes it is authorized and is + // not. A probe that cannot complete is not evidence either way and is + // allowed through: the readiness loop above is what decides whether the + // container is alive. + async #requireAuthEnforced(host: IWorkspaceContainerAPI, deadline: number): Promise { + let status: number; + try { + const res = await host.fetchPort(this.#options.containerPort, "http://container/api", { + // Bounded like every other request this file makes to the + // container. Unbounded, a container that accepts the connection + // and then stops serving would hang the connect, and a slow one + // would eat the budget #waitForUpgrade needs, surfacing as an + // upgrade that never arrived. + signal: AbortSignal.timeout( + Math.min(this.#options.healthProbeTimeoutMs, Math.max(50, deadline - Date.now())), + ), + }); + status = res.status; + // Release the body; nothing here reads it. + await res.text().catch(() => ""); + } catch { + return; + } + if (status === 401) return; + this.#rejectUpgrade?.(new Error("container is not enforcing RPC_CLIENT_SECRET")); + this.#clearUpgrade(); + throw new WorkspaceTransportError( + `CloudflareContainerBackend(${this.id}) [stage=auth]: container served an unauthenticated ` + + `request to /api with ${status}, so this workspace would run without authorization. ` + + `A container or image predating RPC_CLIENT_SECRET has to be recycled.`, + ); + } + + async #postConnect( + host: IWorkspaceContainerAPI, + deadline: number, + clientSecret: string, + ): Promise { const remaining = Math.max(0, deadline - Date.now()); let res: Response; try { res = await host.fetchPort(this.#options.containerPort, "http://container/connect", { method: "POST", - headers: { "content-type": "application/json" }, + headers: { + "content-type": "application/json", + authorization: `Bearer ${clientSecret}`, + }, body: JSON.stringify({ base: `http://${this.#options.egressHost}`, health: EGRESS_HEALTH_PATH, diff --git a/packages/computer/src/backends/container/container-client-secret.test.ts b/packages/computer/src/backends/container/container-client-secret.test.ts new file mode 100644 index 00000000..3368f8d0 --- /dev/null +++ b/packages/computer/src/backends/container/container-client-secret.test.ts @@ -0,0 +1,67 @@ +import { describe, expect, test } from "vitest"; + +import { ContainerClientSecret } from "./container-client-secret.js"; + +function fakeStorage(initial: Record = {}) { + const values = new Map(Object.entries(initial)); + let writes = 0; + return { + writes: () => writes, + values, + async get(key: string): Promise { + return values.get(key) as T | undefined; + }, + async put(key: string, value: unknown): Promise { + writes++; + values.set(key, value); + }, + }; +} + +describe("ContainerClientSecret", () => { + test("generates 32 hex characters on first use", async () => { + const storage = fakeStorage(); + const secret = await new ContainerClientSecret(storage).ensure(); + + expect(secret).toMatch(/^[0-9a-f]{32}$/); + expect(storage.writes()).toBe(1); + }); + + test("returns the stored value on later calls without rewriting it", async () => { + const storage = fakeStorage(); + const store = new ContainerClientSecret(storage); + + const first = await store.ensure(); + const second = await store.ensure(); + + expect(second).toBe(first); + expect(storage.writes()).toBe(1); + }); + + test("a reconstructed instance reuses the persisted value", async () => { + // This is the case that matters: a durable object rebuilt against a + // container that is still running has to present the secret that + // container was launched with. + const storage = fakeStorage(); + const before = await new ContainerClientSecret(storage).ensure(); + + const after = await new ContainerClientSecret(storage).ensure(); + + expect(after).toBe(before); + }); + + test("two workspaces do not share a secret", async () => { + const one = await new ContainerClientSecret(fakeStorage()).ensure(); + const two = await new ContainerClientSecret(fakeStorage()).ensure(); + + expect(one).not.toBe(two); + }); + + test("replaces a stored value that is empty", async () => { + const storage = fakeStorage({ "computer:container-client-secret": "" }); + + const secret = await new ContainerClientSecret(storage).ensure(); + + expect(secret).toMatch(/^[0-9a-f]{32}$/); + }); +}); diff --git a/packages/computer/src/backends/container/container-client-secret.ts b/packages/computer/src/backends/container/container-client-secret.ts new file mode 100644 index 00000000..b7bb2ea7 --- /dev/null +++ b/packages/computer/src/backends/container/container-client-secret.ts @@ -0,0 +1,42 @@ +// Shared secret authorizing the host's requests to the container's HTTP +// surface. +// +// It has to be durable rather than generated per call. A durable object +// can be reconstructed while its container keeps running, and in that +// case start() does not relaunch, so the environment the container holds +// is the one from the original launch. A fresh secret each incarnation +// would not match it and every request would be refused, breaking the +// reconnect that POST /connect exists to serve. +// +// Persisting it also covers the other direction: when a replacement +// container is launched, it receives the value already stored, so both +// ends stay in step without the host having to read anything back. + +interface ClientSecretStorage { + get(key: string): Promise; + put(key: string, value: unknown): Promise; +} + +const STORAGE_KEY = "computer:container-client-secret"; + +// 16 random bytes, hex encoded: 32 characters carrying 128 bits. +const SECRET_BYTES = 16; + +function generate(): string { + const bytes = crypto.getRandomValues(new Uint8Array(SECRET_BYTES)); + return Array.from(bytes, (byte) => byte.toString(16).padStart(2, "0")).join(""); +} + +export class ContainerClientSecret { + constructor(private readonly storage: ClientSecretStorage) {} + + // Returns the stored secret, generating and persisting one the first + // time. Callers may invoke this on every start; only the first writes. + async ensure(): Promise { + const existing = await this.storage.get(STORAGE_KEY); + if (typeof existing === "string" && existing.length > 0) return existing; + const secret = generate(); + await this.storage.put(STORAGE_KEY, secret); + return secret; + } +} diff --git a/packages/computer/src/backends/container/container-host-adoption.test.ts b/packages/computer/src/backends/container/container-host-adoption.test.ts new file mode 100644 index 00000000..aa28d558 --- /dev/null +++ b/packages/computer/src/backends/container/container-host-adoption.test.ts @@ -0,0 +1,162 @@ +// What start() does when it finds a container already running. +// +// The environment and the outbound-internet flag can only be set at +// launch, so adopting a container launched with different ones would +// silently discard what the caller asked for. These cover the three +// outcomes and, in particular, the case that motivated them: a warm +// pool that started the container itself and left no record. +import { describe, expect, test, vi } from "vitest"; + +import { WorkspaceContainerAPI } from "./container-host.js"; +import type { ContainerLaunchSpec } from "./container-launch-record.js"; + +function fakeCtx(options: { running?: boolean } = {}) { + const values = new Map(); + const starts: { enableInternet: boolean; env: Record }[] = []; + let destroys = 0; + // monitor() has to settle when the container goes away: the destroy + // path waits on the current generation's monitor before the caller + // installs the next one. + let exited: (() => void) | undefined; + const container = { + running: options.running ?? false, + start(spec: { enableInternet: boolean; env: Record }) { + starts.push(spec); + container.running = true; + }, + async destroy() { + destroys += 1; + container.running = false; + exited?.(); + exited = undefined; + }, + async setInactivityTimeout() {}, + monitor: () => + new Promise((resolve) => { + exited = resolve; + }), + getTcpPort: () => ({}) as Fetcher, + }; + const ctx = { + container, + storage: { + async get(key: string): Promise { + return values.get(key) as T | undefined; + }, + async put(key: string, value: unknown): Promise { + values.set(key, value); + }, + async delete(key: string): Promise { + return values.delete(key); + }, + }, + blockConcurrencyWhile: async (fn: () => Promise) => fn(), + } as unknown as DurableObjectState; + return { ctx, container, starts, destroyCount: () => destroys, values }; +} + +const spec: ContainerLaunchSpec = { + env: { PORT: "8080", MOUNT_POINT: "/workspace" }, + enableInternet: false, +}; + +describe("WorkspaceContainerAPI.start", () => { + test("launches when nothing is running", async () => { + const { ctx, starts } = fakeCtx(); + + const info = await new WorkspaceContainerAPI(ctx).start(spec); + + expect(info.outcome).toBe("launched"); + expect(starts).toHaveLength(1); + expect(starts[0]?.enableInternet).toBe(false); + expect(starts[0]?.env.RPC_CLIENT_SECRET).toMatch(/^[0-9a-f]{32}$/); + }); + + test("adopts a container it launched with the same spec", async () => { + const { ctx, starts, destroyCount } = fakeCtx(); + const api = new WorkspaceContainerAPI(ctx); + const first = await api.start(spec); + + const second = await api.start(spec); + + expect(second.outcome).toBe("adopted"); + expect(second.runtimeId).toBe(first.runtimeId); + expect(second.clientSecret).toBe(first.clientSecret); + expect(starts).toHaveLength(1); + expect(destroyCount()).toBe(0); + }); + + test("relaunches a container started outside this API", async () => { + // The warm-pool case. A container running with no launch record was + // started by something that never injected the secret, so adopting + // it would leave the daemon unauthenticated and any requested + // environment unapplied. + const { ctx, container, starts, destroyCount } = fakeCtx(); + container.running = true; + const warn = vi.spyOn(console, "warn").mockImplementation(() => {}); + try { + const info = await new WorkspaceContainerAPI(ctx).start(spec); + + expect(info.outcome).toBe("relaunched"); + expect(destroyCount()).toBe(1); + expect(starts).toHaveLength(1); + expect(starts[0]?.env.RPC_CLIENT_SECRET).toMatch(/^[0-9a-f]{32}$/); + expect(warn).toHaveBeenCalled(); + } finally { + warn.mockRestore(); + } + }); + + test("relaunches when the internet flag differs", async () => { + // The egress case: a container launched with the internet enabled + // must not serve a workspace that asked for it off. + const { ctx, starts } = fakeCtx(); + const api = new WorkspaceContainerAPI(ctx); + await api.start({ ...spec, enableInternet: true }); + const warn = vi.spyOn(console, "warn").mockImplementation(() => {}); + try { + const info = await api.start({ ...spec, enableInternet: false }); + + expect(info.outcome).toBe("relaunched"); + expect(starts).toHaveLength(2); + expect(starts[1]?.enableInternet).toBe(false); + } finally { + warn.mockRestore(); + } + }); + + test("relaunches when the environment differs", async () => { + const { ctx, starts } = fakeCtx(); + const api = new WorkspaceContainerAPI(ctx); + await api.start(spec); + const warn = vi.spyOn(console, "warn").mockImplementation(() => {}); + try { + const info = await api.start({ + ...spec, + env: { ...spec.env, FUSE_MOUNT: "none" }, + }); + + expect(info.outcome).toBe("relaunched"); + expect(starts).toHaveLength(2); + expect(starts[1]?.env.FUSE_MOUNT).toBe("none"); + } finally { + warn.mockRestore(); + } + }); + + test("keeps the same secret across a relaunch", async () => { + // The secret is durable, so a replacement container is launched with + // the value the host already holds. + const { ctx } = fakeCtx(); + const api = new WorkspaceContainerAPI(ctx); + const first = await api.start(spec); + const warn = vi.spyOn(console, "warn").mockImplementation(() => {}); + try { + const second = await api.start({ ...spec, enableInternet: true }); + + expect(second.clientSecret).toBe(first.clientSecret); + } finally { + warn.mockRestore(); + } + }); +}); diff --git a/packages/computer/src/backends/container/container-host.ts b/packages/computer/src/backends/container/container-host.ts index eba11ba9..848a1b36 100644 --- a/packages/computer/src/backends/container/container-host.ts +++ b/packages/computer/src/backends/container/container-host.ts @@ -20,16 +20,21 @@ import { RpcTarget } from "cloudflare:workers"; import { WorkspaceTransportError } from "../../transport-failure.js"; +import { ContainerClientSecret } from "./container-client-secret.js"; +import { + type ContainerLaunchRecord, + type ContainerLaunchSpec, + CurrentContainerLaunchRecord, + launchRecordFor, + sameLaunch, +} from "./container-launch-record.js"; import { type ContainerExitInfo, containerExitInfo, destroyContainerExpectingExit, installContainerMonitor, } from "./container-lifecycle.js"; -import { - type ContainerRuntimeIdentity, - CurrentContainerRuntimeIdentity, -} from "./container-runtime-identity.js"; +import { CurrentContainerRuntimeIdentity } from "./container-runtime-identity.js"; export type { ContainerExitInfo } from "./container-lifecycle.js"; @@ -48,13 +53,24 @@ export interface WorkspaceRef { // the `ws` accessor that withWorkspaceContainer installs. export interface ContainerRuntimeInfo { runtimeId: string; + // Shared secret the container requires on its HTTP surface. Durable, + // so a reconstructed durable object presents the value the running + // container was launched with. + clientSecret: string; + // What the call actually did. `adopted` means a container was already + // running with the requested launch spec and was reused. `relaunched` + // means one was running with a different spec, or with none recorded, + // and had to be replaced: the environment and the internet flag can + // only be set at launch, so adopting it would have quietly ignored + // what the caller asked for. + outcome: "launched" | "adopted" | "relaunched"; } export interface IWorkspaceContainerAPI { // Idempotent start. Returns the durable identity of the running // container process once the runtime has accepted the start command; // readiness is verified by the backend through probeComputerdHealth. - start(env: Record, enableInternet: boolean): Promise; + start(spec: ContainerLaunchSpec): Promise; // Wire `host` → workspace inside the container's egress table. // Called once per backend connect(). The implementation @@ -77,7 +93,12 @@ export interface IWorkspaceContainerAPI { // current generation dead. Implementation: destroy() the // container, then start({ env }). Callers bound the number of // restart attempts — this method does no looping of its own. - restart(env: Record, enableInternet: boolean): Promise; + restart(spec: ContainerLaunchSpec): Promise; + + // Set the platform's idle timeout for the attached container. Exposed + // so a caller that pre-starts containers, such as a warm pool, never + // needs to reach past this API to ctx.container. + setInactivityTimeout(durationMs: number): Promise; // Coarse diagnostic state. The `running` flag reports whether // the platform still has a container instance attached; it does @@ -104,6 +125,8 @@ export class WorkspaceContainerAPI extends RpcTarget implements IWorkspaceContai readonly #container: NonNullable; readonly #ctx: DurableObjectState; readonly #runtimeIdentity: CurrentContainerRuntimeIdentity; + readonly #clientSecret: ContainerClientSecret; + readonly #launchRecord: CurrentContainerLaunchRecord; constructor(ctx: DurableObjectState) { super(); @@ -113,9 +136,11 @@ export class WorkspaceContainerAPI extends RpcTarget implements IWorkspaceContai this.#container = ctx.container; this.#ctx = ctx; this.#runtimeIdentity = new CurrentContainerRuntimeIdentity(ctx.storage); + this.#clientSecret = new ContainerClientSecret(ctx.storage); + this.#launchRecord = new CurrentContainerLaunchRecord(ctx.storage); } - async start(env: Record, enableInternet: boolean) { + async start(spec: ContainerLaunchSpec): Promise { // If a prior generation has died, commit to a fresh one: the // destroy clears any platform-side carcass, and the start that // follows is unconditional. We cannot rely on @@ -123,7 +148,12 @@ export class WorkspaceContainerAPI extends RpcTarget implements IWorkspaceContai // destroy resolves, so guarding the start against it would let // a stale-running flag skip the re-launch entirely. const priorExit = containerExitInfo(this.#ctx); - let runtime: ContainerRuntimeIdentity; + // Resolved before any launch so the value written into the + // container's environment is the same one later incarnations read + // back and present on their requests. + const clientSecret = await this.#clientSecret.ensure(); + const requested = await launchRecordFor(spec); + if (priorExit !== null) { try { await destroyContainerExpectingExit(this.#ctx, this.#container); @@ -131,20 +161,47 @@ export class WorkspaceContainerAPI extends RpcTarget implements IWorkspaceContai // best-effort — the next start() will surface any real // platform-side failure. } - runtime = await this.#launch(env, enableInternet); - } else if (!this.#container.running) { - runtime = await this.#launch(env, enableInternet); - } else { - // A Durable Object incarnation can be reconstructed while its - // container stays alive. Reuse the durable runtime id rather - // than treating the new WebSocket as a new process. - runtime = (await this.#runtimeIdentity.get()) ?? (await this.#runtimeIdentity.markStarted()); + return this.#launchAs(spec, clientSecret, requested, "launched"); + } + if (!this.#container.running) { + return this.#launchAs(spec, clientSecret, requested, "launched"); + } + + // A container is already running. Adopting it is only correct if it + // was launched with the spec being asked for now: the environment + // and the internet flag cannot be changed on a live container, so + // adopting a mismatched one would silently drop what this caller + // wants. An absent record means something started the container + // without going through here, which is the same problem. + const actual = await this.#launchRecord.get(); + if (actual === null || !sameLaunch(actual, requested)) { + console.warn({ + message: + actual === null + ? "container was started outside WorkspaceContainerAPI; relaunching so the requested environment applies" + : "running container was launched with a different spec; relaunching", + component: "workspace-container", + requested, + actual, + }); + try { + await destroyContainerExpectingExit(this.#ctx, this.#container); + } catch { + // best-effort, as above. + } + return this.#launchAs(spec, clientSecret, requested, "relaunched"); } + + // A Durable Object incarnation can be reconstructed while its + // container stays alive. Reuse the durable runtime id rather + // than treating the new WebSocket as a new process. + const runtime = + (await this.#runtimeIdentity.get()) ?? (await this.#runtimeIdentity.markStarted()); installContainerMonitor(this.#ctx, this.#container, () => this.#runtimeIdentity.clear(runtime)); - return { runtimeId: runtime.id }; + return { runtimeId: runtime.id, clientSecret, outcome: "adopted" }; } - async restart(env: Record, enableInternet: boolean) { + async restart(spec: ContainerLaunchSpec): Promise { // destroy() resolves once the platform has torn down the // attached container. A subsequent start() launches a fresh // generation — ports re-bind, the computerd daemon comes up clean. @@ -157,20 +214,35 @@ export class WorkspaceContainerAPI extends RpcTarget implements IWorkspaceContai // succeed against a fresh generation or surface its own // failure. } - const runtime = await this.#launch(env, enableInternet); - installContainerMonitor(this.#ctx, this.#container, () => this.#runtimeIdentity.clear(runtime)); - return { runtimeId: runtime.id }; + const clientSecret = await this.#clientSecret.ensure(); + return this.#launchAs(spec, clientSecret, await launchRecordFor(spec), "launched"); + } + + async setInactivityTimeout(durationMs: number): Promise { + await this.#container.setInactivityTimeout(durationMs); } - async #launch(env: Record, enableInternet: boolean) { + async #launchAs( + spec: ContainerLaunchSpec, + clientSecret: string, + record: ContainerLaunchRecord, + outcome: "launched" | "relaunched", + ): Promise { const runtime = await this.#runtimeIdentity.markStarted(); try { - this.#container.start({ enableInternet, env }); - return runtime; + this.#container.start({ + enableInternet: spec.enableInternet, + env: { ...spec.env, RPC_CLIENT_SECRET: clientSecret }, + }); } catch (error) { await this.#runtimeIdentity.clear(runtime); throw error; } + // Written after the start is accepted, so a failed launch does not + // leave a record claiming the container holds this spec. + await this.#launchRecord.set(record); + installContainerMonitor(this.#ctx, this.#container, () => this.#runtimeIdentity.clear(runtime)); + return { runtimeId: runtime.id, clientSecret, outcome }; } async status() { diff --git a/packages/computer/src/backends/container/container-launch-record.test.ts b/packages/computer/src/backends/container/container-launch-record.test.ts new file mode 100644 index 00000000..f0c96133 --- /dev/null +++ b/packages/computer/src/backends/container/container-launch-record.test.ts @@ -0,0 +1,96 @@ +import { describe, expect, test } from "vitest"; + +import { + type ContainerLaunchSpec, + CurrentContainerLaunchRecord, + launchRecordFor, + sameLaunch, +} from "./container-launch-record.js"; + +function spec(overrides: Partial = {}): ContainerLaunchSpec { + return { + env: { PORT: "8080", MOUNT_POINT: "/workspace" }, + enableInternet: false, + ...overrides, + }; +} + +function fakeStorage(initial: Record = {}) { + const values = new Map(Object.entries(initial)); + return { + values, + async get(key: string): Promise { + return values.get(key) as T | undefined; + }, + async put(key: string, value: unknown): Promise { + values.set(key, value); + }, + }; +} + +describe("launchRecordFor", () => { + test("is stable regardless of the order keys were written in", async () => { + const one = await launchRecordFor(spec({ env: { A: "1", B: "2" } })); + const two = await launchRecordFor(spec({ env: { B: "2", A: "1" } })); + + expect(one.envDigest).toBe(two.envDigest); + expect(sameLaunch(one, two)).toBe(true); + }); + + test("changes when a value changes", async () => { + const before = await launchRecordFor(spec({ env: { FUSE_MOUNT: "auto" } })); + const after = await launchRecordFor(spec({ env: { FUSE_MOUNT: "none" } })); + + expect(sameLaunch(before, after)).toBe(false); + }); + + test("changes when a variable is added", async () => { + const before = await launchRecordFor(spec({ env: { PORT: "8080" } })); + const after = await launchRecordFor(spec({ env: { PORT: "8080", EXTRA: "" } })); + + expect(sameLaunch(before, after)).toBe(false); + }); + + test("distinguishes the internet flag", async () => { + // This is the egress case: a pool that launches with the internet + // enabled must not be adopted by a workspace that asked for it off. + const off = await launchRecordFor(spec({ enableInternet: false })); + const on = await launchRecordFor(spec({ enableInternet: true })); + + expect(sameLaunch(off, on)).toBe(false); + }); + + test("does not carry the environment in the clear", async () => { + // containerEnv is consumer-supplied and may hold their own secrets, + // so only a digest is persisted. + const record = await launchRecordFor(spec({ env: { API_TOKEN: "hunter2" } })); + + expect(JSON.stringify(record)).not.toContain("hunter2"); + expect(record.envDigest).toMatch(/^[0-9a-f]{64}$/); + }); +}); + +describe("CurrentContainerLaunchRecord", () => { + test("round-trips a record", async () => { + const storage = fakeStorage(); + const store = new CurrentContainerLaunchRecord(storage); + const record = await launchRecordFor(spec()); + + await store.set(record); + + expect(await store.get()).toEqual(record); + }); + + test("reads null before anything has been launched", async () => { + expect(await new CurrentContainerLaunchRecord(fakeStorage()).get()).toBeNull(); + }); + + test("reads null when a caller launched the container behind our back", async () => { + // A container started outside this API leaves no record. Returning + // null is what makes the adoption check relaunch it rather than + // trust it. + const storage = fakeStorage({ "computer:container-runtime-identity": { id: "abc" } }); + + expect(await new CurrentContainerLaunchRecord(storage).get()).toBeNull(); + }); +}); diff --git a/packages/computer/src/backends/container/container-launch-record.ts b/packages/computer/src/backends/container/container-launch-record.ts new file mode 100644 index 00000000..8af2b281 --- /dev/null +++ b/packages/computer/src/backends/container/container-launch-record.ts @@ -0,0 +1,70 @@ +// What the running container process was launched with. +// +// The environment and the outbound-internet flag can only be set when +// the process starts. A durable object that finds a container already +// running therefore cannot apply either one, and until it can tell what +// the container was launched with it has no way to know whether adopting +// it is safe. A warm pool that pre-starts containers is the case in +// point: the workspace that later adopts one may want a different +// environment, or no internet access at all. +// +// So each launch records what it used, and adoption compares. A +// container launched outside this API leaves no record at all, which +// reads as a mismatch and gets it relaunched rather than trusted. + +export interface ContainerLaunchSpec { + // Environment for the container image. The launch adds + // RPC_CLIENT_SECRET on top, so no caller needs to know it exists and + // it stays out of the digest below. + env: Record; + // Platform switch for outbound internet. Cannot be changed on a live + // container, which is why a mismatch has to relaunch. + enableInternet: boolean; +} + +export interface ContainerLaunchRecord { + enableInternet: boolean; + // A digest rather than the environment itself: containerEnv is + // consumer-supplied and may carry their own secrets, and this record + // only ever needs to answer "the same or not". + envDigest: string; +} + +interface LaunchRecordStorage { + get(key: string): Promise; + put(key: string, value: unknown): Promise; +} + +const STORAGE_KEY = "computer:container-launch-record"; + +export async function launchRecordFor(spec: ContainerLaunchSpec): Promise { + return { enableInternet: spec.enableInternet, envDigest: await digestEnv(spec.env) }; +} + +export function sameLaunch(a: ContainerLaunchRecord, b: ContainerLaunchRecord): boolean { + return a.enableInternet === b.enableInternet && a.envDigest === b.envDigest; +} + +// Sorted so two callers building the same environment in a different +// order agree, and length-prefixed so no combination of names and +// values can be rearranged into the same input. +async function digestEnv(env: Record): Promise { + const canonical = Object.keys(env) + .sort() + .map((name) => `${name.length}:${name}=${env[name]?.length ?? 0}:${env[name] ?? ""}`) + .join(";"); + const digest = await crypto.subtle.digest("SHA-256", new TextEncoder().encode(canonical)); + return Array.from(new Uint8Array(digest), (byte) => byte.toString(16).padStart(2, "0")).join(""); +} + +export class CurrentContainerLaunchRecord { + constructor(private readonly storage: LaunchRecordStorage) {} + + async get(): Promise { + return (await this.storage.get(STORAGE_KEY)) ?? null; + } + + async set(record: ContainerLaunchRecord): Promise { + await this.storage.put(STORAGE_KEY, record); + } +} diff --git a/packages/computer/src/backends/container/index.ts b/packages/computer/src/backends/container/index.ts index 2aa20d7a..5ad9453f 100644 --- a/packages/computer/src/backends/container/index.ts +++ b/packages/computer/src/backends/container/index.ts @@ -18,8 +18,10 @@ export { type CloudflareContainerBackendOptions, } from "./cloudflare-container.js"; export { + type ContainerRuntimeInfo, type IWorkspaceContainerAPI, WorkspaceContainerAPI, type WorkspaceRef, withWorkspaceContainer, } from "./container-host.js"; +export type { ContainerLaunchSpec } from "./container-launch-record.js"; diff --git a/packages/computerd/README.md b/packages/computerd/README.md index 7ba1088e..64345557 100644 --- a/packages/computerd/README.md +++ b/packages/computerd/README.md @@ -33,6 +33,11 @@ Current endpoints: All other paths and methods return `404`/`405` with a `text/plain` body. +When `RPC_CLIENT_SECRET` is set, every route above except `/health` requires +it as `Authorization: Bearer `, and so does the `/api` upgrade. +Requests without it get `401`. Leaving the variable unset disables the +check, which is what the container harnesses rely on. + Current filesystem support: - `@platformatic/vfs` in-memory filesystem provided by `@cloudflare/dofs`'s node provider. @@ -107,7 +112,9 @@ FUSE_MOUNT=none # skip the mount entirely; HTTP and /api still come up Additional environment variables: ```sh -EXEC_LOG_MAX_BYTES=1048576 # cap the in-memory exec log buffer (bytes) +EXEC_LOG_MAX_BYTES=1048576 # cap the in-memory exec log buffer (bytes) +RPC_CLIENT_SECRET= # require Authorization: Bearer on every route but /health +COMPUTER_VAR_NODE_ENV=production # forwarded into exec as NODE_ENV ``` `FUSE_MOUNT=auto` is the friendly default: if `/dev/fuse` (or macFUSE) is available `computerd` mounts a real FUSE filesystem, otherwise it transparently falls back to the userspace shim. Pin the value (`fuse` / `macfuse` / `shim` / `none`) when a test needs to assert a specific code path. diff --git a/packages/computerd/src/cli/computerd.test.ts b/packages/computerd/src/cli/computerd.test.ts index d84665d9..cb5d2ce1 100644 --- a/packages/computerd/src/cli/computerd.test.ts +++ b/packages/computerd/src/cli/computerd.test.ts @@ -389,6 +389,7 @@ test("/connect re-dial tears down the prior WebSocket session", async (_ctx) => const { WebSocketServer } = await import("ws"); const peerPort = await getAvailablePort(); const opened = []; + const upgradeAuthorizations = []; const peerSockets = new Set(); // Deliberately non-default paths: the daemon must use what the // request names, not paths of its own. @@ -411,6 +412,9 @@ test("/connect re-dial tears down the prior WebSocket session", async (_ctx) => return; } wss.handleUpgrade(req, socket, head, (ws) => { + // The host arms a slot for this upgrade and hands the first arrival + // its session, so the dial-back has to prove it is this daemon. + upgradeAuthorizations.push(req.headers.authorization ?? null); const entry = { closed: false, closeCode: null }; ws.on("close", (code) => { entry.closed = true; @@ -457,6 +461,72 @@ test("/connect re-dial tears down the prior WebSocket session", async (_ctx) => await waitFor(() => opened[0].closed); expect(opened[0].closed).toBe(true, "first peer WS should be closed after re-POST /connect"); expect(opened[1].closed).toBe(false, "second peer WS should still be open"); + + // No secret is configured here, so there is nothing to present. + expect(upgradeAuthorizations).toEqual([null, null]); +}); + +test("/connect presents the shared secret on the dial-back", async (_ctx) => { + // The host arms a slot for this upgrade and gives the first arrival its + // session. Anything that can reach the host's endpoint — which includes + // every command this daemon runs — could take the daemon's place, so + // the dial has to carry the secret the daemon was launched with. + const { WebSocketServer } = await import("ws"); + const secret = "0123456789abcdef0123456789abcdef"; + const peerPort = await getAvailablePort(); + const seen = []; + const peerSockets = new Set(); + const peerServer = http.createServer((req, res) => { + if (req.url === "/probe") { + res.writeHead(200, { "content-type": "text/plain" }); + res.end("ok\n"); + return; + } + res.writeHead(404).end(); + }); + peerServer.on("connection", (sock) => { + peerSockets.add(sock); + sock.on("close", () => peerSockets.delete(sock)); + }); + const wss = new WebSocketServer({ noServer: true }); + peerServer.on("upgrade", (req, socket, head) => { + seen.push(req.headers.authorization ?? null); + wss.handleUpgrade(req, socket, head, () => {}); + }); + await new Promise((resolve) => peerServer.listen(peerPort, "127.0.0.1", resolve)); + onTestFinished( + () => + new Promise((resolve) => { + for (const sock of peerSockets) sock.destroy(); + wss.close(); + peerServer.close(() => resolve()); + }), + ); + + const port = await getAvailablePort(); + const mountPoint = await fs.mkdtemp(path.join(os.tmpdir(), "computerd-dialback-")); + await startComputerd({ + port, + mountPoint, + env: { FUSE_MOUNT: "none", RPC_CLIENT_SECRET: secret }, + }); + + const res = await postJson(`http://127.0.0.1:${port}/connect`, { + base: `http://127.0.0.1:${peerPort}`, + health: "/probe", + api: "/socket", + }); + expect(res.statusCode, "the /connect request itself needs the token too").toBe(401); + + const authorized = await postJson( + `http://127.0.0.1:${port}/connect`, + { base: `http://127.0.0.1:${peerPort}`, health: "/probe", api: "/socket" }, + { authorization: `Bearer ${secret}` }, + ); + expect(authorized.statusCode).toBe(200); + + await waitFor(() => seen.length === 1); + expect(seen[0]).toBe(`Bearer ${secret}`); }); test("/connect rejects a body that does not name every part", async (_ctx) => { @@ -595,9 +665,37 @@ async function waitForHTTPOK(url, child, output, timeoutMs = 5_000) { throw new Error(`timed out waiting for ${url}\n${output()}`); } -function request(url) { +// Write a request onto a raw socket and return the response head. +// http.request() will not send a malformed websocket handshake, and +// fetch() will not send one at all, so the handshake cases need this. +function rawRequest(port, lines) { return new Promise((resolve, reject) => { - const request = http.get(url, (response) => { + const socket = net.connect(port, "127.0.0.1", () => { + socket.write(`${lines.join("\r\n")}\r\n\r\n`); + }); + let buf = ""; + socket.setEncoding("utf8"); + socket.on("data", (chunk) => { + buf += chunk; + }); + socket.once("error", reject); + // The server may hold the socket open after refusing the + // handshake, so settle on the end of the response head. + const settle = () => { + socket.destroy(); + resolve(buf); + }; + socket.on("data", () => { + if (buf.includes("\r\n\r\n")) settle(); + }); + socket.once("close", () => resolve(buf)); + setTimeout(settle, 2_000); + }); +} + +function request(url, options = {}) { + return new Promise((resolve, reject) => { + const request = http.get(url, options, (response) => { response.setEncoding("utf8"); let body = ""; response.on("data", (chunk) => { @@ -632,35 +730,7 @@ async function waitFor(predicate, { timeoutMs = 2_000, intervalMs = 10 } = {}) { throw new Error("waitFor: predicate did not become true within the timeout"); } -// Write a request onto a raw socket and return the response head. -// http.request() will not send a malformed websocket handshake, and -// fetch() will not send one at all, so the handshake cases need this. -function rawRequest(port, lines) { - return new Promise((resolve, reject) => { - const socket = net.connect(port, "127.0.0.1", () => { - socket.write(`${lines.join("\r\n")}\r\n\r\n`); - }); - let buf = ""; - socket.setEncoding("utf8"); - socket.on("data", (chunk) => { - buf += chunk; - }); - socket.once("error", reject); - // The server may hold the socket open after refusing the - // handshake, so settle on the end of the response head. - const settle = () => { - socket.destroy(); - resolve(buf); - }; - socket.on("data", () => { - if (buf.includes("\r\n\r\n")) settle(); - }); - socket.once("close", () => resolve(buf)); - setTimeout(settle, 2_000); - }); -} - -function postJson(url, body) { +function postJson(url, body, headers = {}) { const payload = JSON.stringify(body); return new Promise((resolve, reject) => { const req = http.request( @@ -670,6 +740,7 @@ function postJson(url, body) { headers: { "content-type": "application/json", "content-length": Buffer.byteLength(payload), + ...headers, }, }, (response) => { @@ -709,3 +780,85 @@ function stopProcess(child) { child.kill("SIGTERM"); }); } + +test("RPC_CLIENT_SECRET gates the HTTP surface but not /health", async (_ctx) => { + const secret = "0123456789abcdef0123456789abcdef"; + const port = await getAvailablePort(); + const mountPoint = await fs.mkdtemp(path.join(os.tmpdir(), "computerd-auth-")); + await startComputerd({ + port, + mountPoint, + env: { FUSE_MOUNT: "none", RPC_CLIENT_SECRET: secret }, + }); + const base = `http://127.0.0.1:${port}`; + const bearer = { authorization: `Bearer ${secret}` }; + + // Readiness has to stay reachable: the host polls it before it has + // any session, and a gated probe turns a bad token into what looks + // like a container that never came up. + expect((await request(`${base}/health`)).statusCode).toBe(200); + + for (const route of ["/", "/__computerd/info", "/api/watermarks"]) { + const anonymous = await request(`${base}${route}`); + expect(anonymous.statusCode, `${route} without a token`).toBe(401); + const authorized = await request(`${base}${route}`, { headers: bearer }); + expect(authorized.statusCode, `${route} with the token`).toBe(200); + } + + // Wrong token, right shape. + const wrong = await request(`${base}/__computerd/info`, { + headers: { authorization: `Bearer ${"f".repeat(secret.length)}` }, + }); + expect(wrong.statusCode).toBe(401); + + // A token of a different length must not be treated as a match. + const short = await request(`${base}/__computerd/info`, { + headers: { authorization: "Bearer short" }, + }); + expect(short.statusCode).toBe(401); + + // The scheme token is case-insensitive per the HTTP grammar, and the + // credentials may be preceded by more than one space. + for (const header of [ + `Bearer ${secret}`, + `bearer ${secret}`, + `BEARER ${secret}`, + `BeArEr ${secret}`, + `Bearer ${secret}`, + ]) { + const res = await request(`${base}/__computerd/info`, { headers: { authorization: header } }); + expect(res.statusCode, JSON.stringify(header)).toBe(200); + } + + // Another scheme is not a bearer token. + const basic = await request(`${base}/__computerd/info`, { + headers: { authorization: `Basic ${secret}` }, + }); + expect(basic.statusCode).toBe(401); + + // /connect is gated too, and refused before its body is considered. + const connect = await postJson(`${base}/connect`, {}); + expect(connect.statusCode).toBe(401); + + // The upgrade is gated as well. + const upgrade = await rawRequest(port, [ + "GET /api HTTP/1.1", + `Host: 127.0.0.1:${port}`, + "Upgrade: websocket", + "Connection: Upgrade", + "Sec-WebSocket-Key: dGhlIHNhbXBsZSBub25jZQ==", + "Sec-WebSocket-Version: 13", + ]); + expect(upgrade).toMatch(/^HTTP\/1\.1 401 /); +}); + +test("without RPC_CLIENT_SECRET every route stays open", async (_ctx) => { + const port = await getAvailablePort(); + const mountPoint = await fs.mkdtemp(path.join(os.tmpdir(), "computerd-noauth-")); + await startComputerd({ port, mountPoint, env: { FUSE_MOUNT: "none" } }); + const base = `http://127.0.0.1:${port}`; + + for (const route of ["/health", "/", "/__computerd/info", "/api/watermarks"]) { + expect((await request(`${base}${route}`)).statusCode, route).toBe(200); + } +}); diff --git a/packages/computerd/src/cli/computerd.ts b/packages/computerd/src/cli/computerd.ts index 0de544b0..5036db4b 100644 --- a/packages/computerd/src/cli/computerd.ts +++ b/packages/computerd/src/cli/computerd.ts @@ -1,5 +1,6 @@ #!/usr/bin/env node +import { timingSafeEqual } from "node:crypto"; import { mkdir } from "node:fs/promises"; import { createServer, type IncomingMessage, type Server, type ServerResponse } from "node:http"; import type { Socket } from "node:net"; @@ -99,6 +100,34 @@ function requestPath(request: IncomingMessage): string { return url.pathname; } +// Routes reachable without the shared secret. Readiness has to stay +// open: the host polls it before it holds any session, and a gated +// probe turns a bad token into what looks like a container that never +// started. +const UNAUTHENTICATED_PATHS = new Set(["/health"]); + +// When RPC_CLIENT_SECRET is set, every other route requires it as a +// bearer token. When it is unset there is nothing to check and the +// surface is open, which is how the harnesses and local runs work. +function isAuthorized(request: IncomingMessage, secret: string | undefined): boolean { + if (secret === undefined) return true; + const header = request.headers.authorization; + if (typeof header !== "string") return false; + // The scheme token is case-insensitive and may be followed by more + // than one space, so "bearer " from a hand-written client is + // as valid as "Bearer ". Only the credentials that follow are + // compared byte for byte. + const separator = header.indexOf(" "); + if (separator === -1) return false; + if (header.slice(0, separator).toLowerCase() !== "bearer") return false; + const presented = Buffer.from(header.slice(separator + 1).trim()); + const expected = Buffer.from(secret); + // timingSafeEqual throws on a length mismatch, and the length of the + // secret is not worth leaking through the comparison either. + if (presented.length !== expected.length) return false; + return timingSafeEqual(presented, expected); +} + // Strip heartbeat events from a Runner stream before it reaches the // RPC layer. Heartbeats are a local observability signal only; the // computer-rpc wire contract carries only stdout, stderr, and exit. @@ -162,6 +191,7 @@ interface HTTPHandle { function createHTTPServer( info: ComputerdInfo, rpc: ReturnType, + secret: string | undefined, getStats?: () => Record, ): HTTPHandle { // Holds the current outbound capnweb session opened via /connect. @@ -172,6 +202,16 @@ function createHTTPServer( const server = createServer((request, response) => { const path = requestPath(request); + // Checked before routing so an unauthorized caller cannot learn + // which routes exist. + if (!UNAUTHENTICATED_PATHS.has(path) && !isAuthorized(request, secret)) { + send(response, 401, "unauthorized\n", { + "www-authenticate": "Bearer", + "content-type": "text/plain; charset=utf-8", + }); + return; + } + // /api — the capnweb endpoint. It carries one transport, a // websocket, so a request that reaches the ordinary handler here // has no Upgrade header and cannot be served. Say so plainly: @@ -197,7 +237,7 @@ function createHTTPServer( }); return; } - void handleConnect(request, response, rpc, upstreamSlot); + void handleConnect(request, response, rpc, upstreamSlot, secret); return; } @@ -307,6 +347,13 @@ function createHTTPServer( acceptWebSocketSession(ws, rpc); }); server.on("upgrade", (request, socket, head) => { + if (!isAuthorized(request, secret)) { + socket.write( + "HTTP/1.1 401 Unauthorized\r\nWWW-Authenticate: Bearer\r\nConnection: close\r\n\r\n", + ); + socket.destroy(); + return; + } if (requestPath(request) !== "/api") { socket.write("HTTP/1.1 404 Not Found\r\n\r\n"); socket.destroy(); @@ -398,6 +445,7 @@ async function handleConnect( response: ServerResponse, rpc: ReturnType, upstreamSlot: { ws: WebSocket | undefined }, + secret: string | undefined, ): Promise { let body: ConnectBody; try { @@ -461,7 +509,14 @@ async function handleConnect( } const wsUrl = `${toWebSocketUrl(baseUrl)}${body.api}`; - const ws = new WebSocket(wsUrl); + // Present the shared secret on the way out as well. The host arms a + // slot for this upgrade and hands the first arrival its session, so + // without a token anything that can reach the host's endpoint — which + // includes every command this daemon runs — could take the workspace's + // place. The host holds the same secret, having set it at launch. + const ws = new WebSocket(wsUrl, { + ...(secret !== undefined ? { headers: { authorization: `Bearer ${secret}` } } : {}), + }); upstreamSlot.ws = ws; ws.once("open", () => { console.log(`/connect: attached RPC session to ${wsUrl}`); @@ -522,6 +577,13 @@ async function main(): Promise { rejectLegacyFuseEnv(process.env); + // Read the shared secret once and drop it from the environment. The + // exec allowlist already keeps it out of spawned commands; removing + // it here means a future code path that spreads process.env cannot + // reintroduce the leak. + const clientSecret = process.env.RPC_CLIENT_SECRET?.trim() || undefined; + delete process.env.RPC_CLIENT_SECRET; + const port = parsePort(process.env.PORT); const mountPoint = parseMountPoint(process.env.MOUNT_POINT); // FUSE_MOUNT picks the backend. auto (default) probes /dev/fuse @@ -615,7 +677,7 @@ async function main(): Promise { } : {}), }); - const http = createHTTPServer(info, rpc, () => ({ + const http = createHTTPServer(info, rpc, clientSecret, () => ({ ...collectDbStats(db), ...(fuse?.getBufferStats?.() ?? {}), })); diff --git a/packages/computerd/src/exec/env.test.ts b/packages/computerd/src/exec/env.test.ts new file mode 100644 index 00000000..9779a74e --- /dev/null +++ b/packages/computerd/src/exec/env.test.ts @@ -0,0 +1,71 @@ +import { expect, test } from "vitest"; + +import { inheritedEnv } from "./env.js"; + +test("forwards the standard variables a shell environment expects", () => { + const env = inheritedEnv({ + PATH: "/opt/toolchain/bin:/usr/bin", + HOME: "/root", + TMPDIR: "/tmp", + TZ: "UTC", + LANG: "C.UTF-8", + LC_ALL: "C.UTF-8", + TERM: "xterm", + }); + + expect(env).toEqual({ + PATH: "/opt/toolchain/bin:/usr/bin", + HOME: "/root", + TMPDIR: "/tmp", + TZ: "UTC", + LANG: "C.UTF-8", + LC_ALL: "C.UTF-8", + TERM: "xterm", + }); +}); + +test("drops the daemon's own configuration and anything else unrecognized", () => { + // The secret is the reason this list is an allowlist rather than a + // denylist: a new one must not reach a spawned process by default. + const env = inheritedEnv({ + PATH: "/usr/bin", + RPC_CLIENT_SECRET: "sssh", + PORT: "8080", + MOUNT_POINT: "/workspace", + FUSE_MOUNT: "auto", + LOG_FILE: "/tmp/computerd.log", + EXEC_LOG_MAX_BYTES: "1024", + AWS_SECRET_ACCESS_KEY: "also-sssh", + }); + + expect(env).toEqual({ PATH: "/usr/bin" }); +}); + +test("forwards COMPUTER_VAR_ values with the prefix stripped", () => { + const env = inheritedEnv({ + COMPUTER_VAR_NODE_ENV: "production", + COMPUTER_VAR_EMPTY: "", + }); + + expect(env).toEqual({ NODE_ENV: "production", EMPTY: "" }); +}); + +test("lets a prefixed value replace a standard variable", () => { + const env = inheritedEnv({ + PATH: "/usr/bin", + COMPUTER_VAR_PATH: "/opt/only/bin", + }); + + expect(env.PATH).toBe("/opt/only/bin"); +}); + +test("ignores a bare prefix with no name after it", () => { + expect(inheritedEnv({ COMPUTER_VAR_: "nameless" })).toEqual({}); +}); + +test("omits variables that are absent rather than defining them empty", () => { + const env = inheritedEnv({ PATH: "/usr/bin", HOME: undefined }); + + expect(env).toEqual({ PATH: "/usr/bin" }); + expect("HOME" in env).toBe(false); +}); diff --git a/packages/computerd/src/exec/env.ts b/packages/computerd/src/exec/env.ts new file mode 100644 index 00000000..c904466c --- /dev/null +++ b/packages/computerd/src/exec/env.ts @@ -0,0 +1,49 @@ +// Builds the environment a spawned command inherits from the daemon. +// +// This is an allowlist, not a denylist. The daemon's own environment +// carries its configuration and, when the host sets one, the shared +// secret that authorizes requests to its HTTP surface. Spreading all of +// it into every `shell.exec` child would hand that secret to any command +// the workspace runs, and a denylist would leak the next secret somebody +// adds. So nothing crosses unless it is named here. + +// Variables a shell environment is expected to carry. PATH is the +// load-bearing one: /bin/sh falls back to a compiled-in default that +// covers /usr/bin, so dropping it leaves standard tools working while +// silently hiding anything an image installed elsewhere, which is a +// common pattern for language runtimes and vendored toolchains. +const FORWARDED = ["PATH", "HOME", "TMPDIR", "TZ", "LANG", "TERM"] as const; + +// Locale variables are a family rather than a fixed name. +const FORWARDED_PREFIX = "LC_"; + +// The operator's channel for reaching a command's environment. The +// prefix is stripped on the way through, so COMPUTER_VAR_NODE_ENV +// arrives as NODE_ENV. +export const ENV_VAR_PREFIX = "COMPUTER_VAR_"; + +export function inheritedEnv(source: NodeJS.ProcessEnv): Record { + const env: Record = {}; + + for (const name of FORWARDED) { + const value = source[name]; + if (value !== undefined) env[name] = value; + } + + for (const [name, value] of Object.entries(source)) { + if (value === undefined) continue; + if (name.startsWith(FORWARDED_PREFIX)) env[name] = value; + } + + // Applied last so an operator can deliberately replace one of the + // standard variables, e.g. COMPUTER_VAR_PATH to pin a toolchain. + for (const [name, value] of Object.entries(source)) { + if (value === undefined) continue; + if (!name.startsWith(ENV_VAR_PREFIX)) continue; + const stripped = name.slice(ENV_VAR_PREFIX.length); + if (stripped.length === 0) continue; + env[stripped] = value; + } + + return env; +} diff --git a/packages/computerd/src/exec/runner.test.ts b/packages/computerd/src/exec/runner.test.ts index 4258017b..09f417da 100644 --- a/packages/computerd/src/exec/runner.test.ts +++ b/packages/computerd/src/exec/runner.test.ts @@ -491,3 +491,29 @@ test("heartbeat seq is monotonically increasing with other events", async () => dispose(); } }); + +test("a spawned command sees the allowlisted environment, not the daemon's", async () => { + // The daemon's environment holds its own configuration and, when the + // host sets one, the secret authorizing requests to its HTTP surface. + // Neither may reach a command the workspace runs. + process.env.RPC_CLIENT_SECRET = "must-not-escape"; + process.env.COMPUTER_VAR_GREETING = "hello"; + const { runner, dispose } = fixture(); + try { + // Written without brace expansion so the assertion reads as shell + // rather than as a mistyped template literal. + const handle = runner.exec( + 'printf \'[%s][%s]\' "$RPC_CLIENT_SECRET" "$GREETING"; test -n "$PATH" && printf \'[path]\'', + ); + const events = await drain(handle.events); + const stdout = events + .filter((event) => event.name === "stdout") + .map((event) => decode(event.value as Uint8Array)) + .join(""); + expect(stdout).toBe("[][hello][path]"); + } finally { + delete process.env.RPC_CLIENT_SECRET; + delete process.env.COMPUTER_VAR_GREETING; + dispose(); + } +}); diff --git a/packages/computerd/src/exec/runner.ts b/packages/computerd/src/exec/runner.ts index 347fd227..21333eaa 100644 --- a/packages/computerd/src/exec/runner.ts +++ b/packages/computerd/src/exec/runner.ts @@ -20,6 +20,7 @@ import { randomUUID } from "node:crypto"; import { type Database, stat } from "@cloudflare/dofs"; +import { inheritedEnv } from "./env.js"; import { createLog, type EventLog, openLog } from "./log.js"; import { clearExecState, initializeExecSchema } from "./schema.js"; import { ExecError, type ExecEvent, type ExecOptions, type RunnerOptions } from "./types.js"; @@ -111,7 +112,7 @@ export class Runner { if (existing !== undefined) this.disposeRecord(existing); const cwd = options.cwd ?? this.opts.cwd; - const env = { ...process.env, ...this.opts.env, ...options.env }; + const env = { ...inheritedEnv(process.env), ...this.opts.env, ...options.env }; // Pre-flight the cwd via dofs's stat which walks vfs_nodes / // vfs_dirents in SQLite directly — no node:fs.statSync, no // FUSE callback. Preserves the historical ENOENT-cwd error