diff --git a/.github/workflows/check.yaml b/.github/workflows/check.yaml index a9d6f6c..ea0c6ff 100644 --- a/.github/workflows/check.yaml +++ b/.github/workflows/check.yaml @@ -53,6 +53,13 @@ jobs: echo "No LOCALMOST_HEARTBEAT variable found" fi - # Default: use fallback runner + # Default: use fallback runner. Announced here rather than by a + # caller, because this is the job that knows: a caller would need + # `needs: check` purely to repeat it, which couples a job to one it + # does not otherwise depend on and takes it down whenever this one + # fails. A job pinned to self-hosted cannot fall back and will queue, + # and `timeout-minutes` does not bound time spent waiting for a + # runner, so this line is the only warning it gets. + echo "::warning title=localmost runner offline::No localmost heartbeat, so this run falls back to $FALLBACK. Any job pinned to the self-hosted runner stays queued until one comes online." echo "No localmost runner available, using $FALLBACK" echo "runner=$FALLBACK" >> $GITHUB_OUTPUT diff --git a/.github/workflows/ci.yaml b/.github/workflows/ci.yaml index 602b654..73d88cd 100644 --- a/.github/workflows/ci.yaml +++ b/.github/workflows/ci.yaml @@ -12,7 +12,10 @@ permissions: jobs: check: - # Use the check workflow from the same ref (branch/SHA) as this workflow + # Use the check workflow from the same ref (branch/SHA) as this workflow. + # Named to distinguish it from the other workflows' check jobs; the job id + # stays `check`, which is what `needs:` below refers to. + name: ci runner uses: ./.github/workflows/check.yaml with: fallback: ubuntu-latest # This repo builds on Linux; typical users would use macos-latest diff --git a/.github/workflows/docker.yaml b/.github/workflows/docker.yaml index 675bfa5..5edf02c 100644 --- a/.github/workflows/docker.yaml +++ b/.github/workflows/docker.yaml @@ -25,11 +25,17 @@ permissions: jobs: check: + # Named so it is tellable apart from the other workflows' check jobs in the + # PR checks list, where three of them appear. + name: docker runner uses: ./.github/workflows/check.yaml with: fallback: ubuntu-latest docker-linux: + # Deliberately independent of the check job: it runs GitHub-hosted whatever + # the heartbeat says, so depending on check would only mean a cancelled + # check cancels this too. The heartbeat is announced by check itself. runs-on: ubuntu-latest steps: - uses: actions/checkout@v4 diff --git a/.github/workflows/test-inline.yaml b/.github/workflows/test-inline.yaml index 75a6bec..899d4ac 100644 --- a/.github/workflows/test-inline.yaml +++ b/.github/workflows/test-inline.yaml @@ -12,6 +12,10 @@ permissions: jobs: check: + # This workflow deliberately inlines the heartbeat check that check.yaml + # offers as a reusable workflow: it is what proves the copy-paste-inline + # integration style still works, so the duplication is the point. + name: inline runner runs-on: ubuntu-latest outputs: runner: ${{ steps.check.outputs.runner }} diff --git a/.gitignore b/.gitignore index 1453312..d93ae3c 100644 --- a/.gitignore +++ b/.gitignore @@ -22,6 +22,11 @@ bundled-runner/ *.swp *.swo +# Playwright's defaults, for a run that forgets --config test/playwright.config.ts. +# The config routes output into build/; these are where it lands without it. +/test-results/ +/playwright-report/ + # OS .DS_Store Thumbs.db diff --git a/.localmostrc b/.localmostrc index 54077e4..b6c76a7 100644 --- a/.localmostrc +++ b/.localmostrc @@ -15,6 +15,13 @@ shared: - path: ./ mode: ro network: bridge + # The docker e2e creates one network per run, joins a container to it and + # removes it, so the network path has real-CLI coverage rather than unit + # tests alone - twice now a body the tests accepted was refused on the + # wire. Internal: these carry no traffic off the machine. + networks: + - name: "localmost-e2e-*" + internal: true network: allow: diff --git a/docs/roadmap/localmostrc.md b/docs/roadmap/localmostrc.md index c50bf34..bd4c2de 100644 --- a/docs/roadmap/localmostrc.md +++ b/docs/roadmap/localmostrc.md @@ -250,7 +250,10 @@ workflows: - Each workflow gets exactly what it needs, nothing more **Workflow matching:** -- Keys under `workflows:` match the workflow filename (without `.yml`/`.yaml`) +- Keys under `workflows:` match the workflow filename (without `.yml`/`.yaml`), + taken from `github.workflow_ref`. Where a job arrives without that — an older + runner service — the workflow's `name:` is used instead, so name a section + after the file and the two agree - `build` matches `.github/workflows/build.yml` - For matrix workflows, all jobs in the workflow share the workflow's policy @@ -287,13 +290,19 @@ Actions are CLI-shaped, so a policy reads the way a workflow author thinks: | Action | Covers | Conditions | |---|---|---| | `pull` | image pulls | `registries` — the registry each pulled image comes from | -| `run` | container create, start, attach, wait and remove | `images` — the image a container is created from; `mounts` — workspace paths a container may bind, each `ro` or `rw`; `network` — the container's network mode | -| `build` | image builds | `context` — where the build context may resolve | - -Conditions are checked against the request itself. Mount and context paths are -resolved through symlinks and must stay inside the job workspace, so `../` -traversal and absolute host paths fail structurally rather than by pattern -match, and a container may write to a mount only where the policy says `rw`. +| `run` | container create, start, attach, wait, kill, stop, remove and logs; creating a declared network; inspecting a declared image | `images` — the images a container may be created from, and the only images it may inspect; each entry is an anchored glob where `*` stops at `/`, so a content-addressed tag can be declared as `vk/grader:*` while `vk/*:*` reaches one level under `vk` and no further; a glob must say which tags it covers, since a tagless reference means `:latest` — `vk/*` is refused, `vk/*:*` accepted; `networks` — networks the job may create, each an anchored name glob plus whether it is `internal`; `mounts` — workspace paths a container may bind, each `ro` or `rw`; `network` — the container's network mode | +| `build` | image builds, with the classic builder (jobs run with `DOCKER_BUILDKIT=0`, since a BuildKit build streams over a gRPC session the filter cannot inspect) | `context` — which directory the workflow builds from, for the reader and the approval diff | + +Conditions are checked against the request itself. Mount paths are resolved +through symlinks and must stay inside the job workspace, so `../` traversal and +absolute host paths fail structurally rather than by pattern match, and a +container may write to a mount only where the policy says `rw`. + +`build.context` is the exception: it is documentation, not a check. A build +context reaches the daemon as a tar the client already assembled, so there is no +path in the request to test. A local context is confined by the sandbox profile +instead — the job can only read what the profile grants — and the filter refuses +a *remote* context, which would have the daemon fetch it and skip the profile. Anything not listed is denied: an undeclared image, registry, mount or network mode, and every endpoint the proxy does not understand. diff --git a/docs/superpowers/specs/2026-09-05-docker-isolation-design.md b/docs/superpowers/specs/2026-09-05-docker-isolation-design.md index 12d0280..bb1d097 100644 --- a/docs/superpowers/specs/2026-09-05-docker-isolation-design.md +++ b/docs/superpowers/specs/2026-09-05-docker-isolation-design.md @@ -191,8 +191,14 @@ checked against the request body: - **`network`** against `NetworkMode`. - **`images`** against the image reference in the create request. - **`registries`** against the registry of a pull. -- **`context`** against the build context path, with the same resolution rules as - mounts. +- **`context`** documents which directory the workflow builds from. It is not + checked against the request, because there is nothing in the request to check + it against: the Engine API carries a build context as a tar the client already + assembled, so the filter never sees a path. What confines a local context is + the seatbelt profile - the job can only read what the profile grants, so the + tar can only contain workspace content. The filter's job here is to refuse a + *remote* context, which would have the daemon fetch the context itself and so + bypass the profile entirely. Anything not listed is denied. diff --git a/docs/superpowers/specs/2026-09-06-docker-endpoint-families.md b/docs/superpowers/specs/2026-09-06-docker-endpoint-families.md new file mode 100644 index 0000000..e7ba18e --- /dev/null +++ b/docs/superpowers/specs/2026-09-06-docker-endpoint-families.md @@ -0,0 +1,208 @@ +# Docker Filter — Three Endpoint Families a Real Consumer Needs + +An addendum to +[2026-09-05-docker-isolation-design.md](./2026-09-05-docker-isolation-design.md). +Everything here extends the stage 1 filter; nothing here changes its shape. + +> **Status:** design. Prompted by wiring a container-heavy repository (an agent +> eval harness) to the shipped filtering socket — the "end-to-end run on a +> repository that needs the daemon" the original Testing section asks for. + +## Problem + +`pull`, `run` and `build` covered that consumer's pulls, container lifecycle and +image builds unchanged, which is the encouraging part. Three families it needs +classify as `other` and hit `default: deny`: + +1. **Networks.** The harness creates an `--internal` network — no route to + anything — as its *sealing* mechanism: the agent under test runs with no + egress except a broker that accounts for every request. It then reads and + deletes that network. +2. **Image existence.** `docker image inspect` is the natural "do I already have + this?" check, deciding build-vs-pull in a build-once-mount flow. +3. **Killing a container.** Enforcing a wall-clock budget on a container that + overruns it. + +The first is the one that matters most, because the direction is backwards. An +`--internal` network makes a container *less* reachable, not more. With networks +denied, the only containers a job can run are ones on the default bridge — the +filter currently **forces strictly weaker isolation than the workload wants**, +which is the opposite of what a sandbox should do. `run.network` does not help: +it constrains `NetworkMode` at create, and the network has to exist first. + +## Solution + +Three additions, each reusing a mechanism the filter already has. + +### 1. Networks: a declared, owned, bridge-only network + +```yaml +shared: + docker: + run: + networks: + - name: vk-* + internal: true +``` + +`name` is a glob matched against the requested network name. `internal` is the +only other key, and it is a **requirement, not a default**: a policy that wants a +routable network must say `internal: false`, so the approval diff shows it. + +**The driver is unnameable, and that is the point.** The dangerous value in a +network create is not `internal: false` — it is `Driver`. A `macvlan` or `ipvlan` +network puts the container directly on the physical LAN, which is worse than +`--network=host`, and `Options` can carry +`com.docker.network.bridge.host_binding_ipv4`. So the grammar cannot spell a +driver at all: the filter forces `bridge`, and **refuses any create body key it +does not recognise**. That is the same allowlist-of-the-grammar principle the +original spec applies to `HostConfig`, applied to a second body. + +Recognised keys on `POST /networks/create`: `Name`, `Internal`, `CheckDuplicate`, +`Labels`. `Driver` is permitted only when absent or exactly `bridge`. + +The rest — `Scope`, `IPAM`, `Options`, `Attachable`, `Ingress`, `ConfigOnly`, +`ConfigFrom`, `EnableIPv6` — are **gated by value rather than refused outright**, +the same way `HostConfig` treats the keys a plain `docker run` always sends. The +CLI sends all eight unconditionally with inert defaults, so refusing them made +the feature reachable only from a hand-written API client. The default passes; +anything meaningful (a subnet, a non-default IPAM driver, driver options, an +attachable or ingress or config-only network, a config source, a scope) is +refused, naming the key. + +`GET /networks/{id}` and `DELETE /networks/{id}` are scoped to networks this +socket created, exactly as per-container endpoints are scoped to containers it +created. `GET /networks` (list) stays denied: it enumerates the daemon. + +**`NetworkMode` must accept an owned network.** This is the part that is easy to +miss and makes the feature useless without it. Today `evaluateCreate` requires +`HostConfig.NetworkMode` to equal `policy.run.network`. A job that creates +`vk-abc` and runs a container with `--network vk-abc` would still be refused. So +the create gate permits a `NetworkMode` that names a network in the socket's +owned set, in addition to the declared `run.network`. + +### 2. Image reads, scoped by the policy rather than by ownership + +`GET /images/{name}/json` is permitted when the reference normalises to an entry +in `run.images`. + +The consumer suggested scoping this the way containers are scoped — to images the +socket pulled or built. Policy-scoping is better here: an inspect of an image the +policy *already names* discloses nothing the policy has not already granted, and +it avoids a second ownership ledger. Ownership bookkeeping is not free — the +container ledger has already produced one defect (a prefix match that outlives +the container it described), and a second one would need to reconcile pulls by +tag with builds by id. + +`GET /images/json` (list) and `DELETE /images/{name}` stay denied: both are +daemon-wide, and the consumer agrees. + +### 3. Stopping a container the job owns + +`POST /containers/{id}/kill` and `POST /containers/{id}/stop` join +`start`/`attach`/`wait`/`remove` under the `run` action, with the same +own-container scoping. + +`stop` is not in the request but belongs in the same change: a timeout path that +can only `kill` is worse than one that can ask politely first, and both are the +same endpoint family with the same scoping. + +`GET /containers/{id}/logs` joins them too. The original spec's baseline is +"reads about the job's own containers", and logs is exactly such a read; refusing +it contradicts the documented behaviour rather than implementing it. + +## Builds use the classic builder + +`build:` policy describes `POST /build`, and a real `docker build` on a default +install never calls it. BuildKit has been the default builder since Docker 23: +it negotiates a session and streams the build over `POST /grpc`. A consumer +replayed 1,429 captured API requests from a suite that built about twenty +images and found **zero** `POST /build` and 63 `POST /grpc`, with +`DOCKER_BUILDKIT` unset — stock behaviour, not an opt-in. + +So the filter pins each job to the classic builder with `DOCKER_BUILDKIT=0`, +set alongside `DOCKER_HOST` when the worker is spawned. + +The alternative was to filter the BuildKit session, and it is not filterable in +the sense this design means. The session is a bidirectional gRPC stream over +which the client exports host filesystem access to the daemon; "which paths may +this build read" stops being a property of a request body, which is the only +thing the proxy can inspect. Choosing the builder the filter can actually see +keeps the boundary honest, at the cost of BuildKit's cache and speed. The +classic builder is deprecated, so this is a stage-1 answer with a shelf life: +stage 2's managed VM contains a build by construction and would not need it. + +`POST /grpc` and `POST /session` are refused by name, saying that jobs are +pinned to the classic builder — seeing that denial means something set +`DOCKER_BUILDKIT` back on, which is worth reading as an error rather than as an +unknown endpoint. + +## What stays denied + +`GET /containers/json`, `GET /networks`, `GET /images/json` and +`DELETE /images/{name}` are daemon-wide by construction — they enumerate or +mutate things outside the job — and no policy key grants them. + +## Not a filter change + +Mounts and build contexts must resolve inside the job workspace. A consumer +building from `tempfile.mkdtemp()` (i.e. `/var/folders/...`) fails that check +**correctly**; pointing `TMPDIR` inside the workspace is the consumer's fix. It +is recorded here only because it reads like a filter bug from the outside, and +the denial message should make the reason obvious enough that it doesn't. + +## Testing + +Per family, and in the same executable-escape style as the original spec: + +- A network create whose name matches no declared pattern is refused; one that + matches is permitted. +- `internal: false` is refused unless declared; `Driver: macvlan`, `Options`, + `IPAM` and any unrecognised key are each refused, naming the key. +- `GET`/`DELETE` of a network the socket did not create is refused. +- A container created with `NetworkMode` naming an owned network is permitted; + one naming an arbitrary network is refused. +- `GET /images/{name}/json` is permitted for a declared image and refused for an + undeclared one; `GET /images/json` is refused. +- `kill`, `stop` and `logs` are permitted on an owned container and refused on + one the socket did not create. +- An end-to-end run that creates an internal network, runs a container on it, + reads its logs, kills it, and deletes the network. + +## Open questions + +- ~~Whether `name` globs should be anchored.~~ **Decided: yes, anchored, and + `*` stops at `/`.** A consumer measured the first implementation and found + that while it anchored correctly, `*` crossed path separators — `vk/grader:*` + matched `vk/grader:a/b` — which answered the question empirically in the + direction nobody wanted. Both halves now hold, for the same reason: a glob + that quietly spans more than it appears to reads as narrower than it is. + `vk/*:*` reaches one level under `vk` and no further; each extra segment has + to be asked for. Tag globs are unaffected, since a tag cannot contain a + slash, so `vk/grader:*` still covers a content-addressed tag. +- ~~What a glob with no tag covers.~~ **Decided: nothing - it is refused at + validation.** The same consumer measured again and found a second boundary + nobody had written down: a reference with no tag normalises to `:latest`, so + `vk/*` is matched as `vk/*:latest` and covers only the latest tag of each + repository - almost none of what it reads as, and invisible in an approval + diff. Two ways out: treat a tagless glob as `:*`, or refuse it. Refusing it + wins for the reason `docker: true` is refused rather than interpreted - the + grammar does not guess at intent it can ask for - so validation rejects a + tagless glob with a message naming `vk/*:*`. Exact references are untouched: + `alpine` still means `alpine:latest`, which is what it looks like. +- ~~What the filter should do when a body spells one key two ways.~~ + **Decided: refuse the body.** Review raised this as a case-folding bypass and + proposed reading the last duplicate, on the theory that Go's decoder is + last-wins. Measured against a real daemon instead: a create body carrying + `HostConfig`, `hostconfig` and `HOSTCONFIG` came back with fields from **all + three** - Go decodes each key into the same struct field in document order, + so nested objects merge, while scalars and arrays inside one object are + last-wins. Reading the last is therefore as wrong as reading the first, and + emulating the merge means reimplementing `encoding/json`. Since Go's encoder + emits unique exactly-cased keys, no real client sends a case-variant + duplicate - the real CLI's bodies are clean, which the e2e exercises - so the + ambiguity is refused recursively at the evaluator's entry, once, for every + action with a body. +- Whether an owned network should be deleted automatically when the job's worker + exits, as the socket itself is. Leaning yes, for the same reason: nothing + should outlive the job that created it. diff --git a/package-lock.json b/package-lock.json index 8503943..2560662 100644 --- a/package-lock.json +++ b/package-lock.json @@ -1261,6 +1261,17 @@ "url": "https://github.com/electron/packager?sponsor=1" } }, + "node_modules/@electron/packager/node_modules/extract-zip": { + "name": "@electron-internal/extract-zip", + "version": "1.0.5", + "resolved": "https://registry.npmjs.org/@electron-internal/extract-zip/-/extract-zip-1.0.5.tgz", + "integrity": "sha512-+bqFCP98pLI0Tt0XQo1TmlXtwjWchISndDOxCkEcIuUgXWpBnLyRI+2DU+mesvnMMX6L1XDqYNA0lXNDHd/yiA==", + "dev": true, + "license": "BSD-2-Clause", + "engines": { + "node": ">=22.12.0" + } + }, "node_modules/@electron/packager/node_modules/fs-extra": { "version": "11.4.0", "resolved": "https://registry.npmjs.org/fs-extra/-/fs-extra-11.4.0.tgz", @@ -3725,17 +3736,6 @@ "dev": true, "license": "MIT" }, - "node_modules/@types/yauzl": { - "version": "2.10.3", - "resolved": "https://registry.npmjs.org/@types/yauzl/-/yauzl-2.10.3.tgz", - "integrity": "sha512-oJoftv0LSuaDZE3Le4DbKX+KS9G36NzOeSap90UIK0yMA/NhKJhqlSGtNDORNRaIbQfzjXDrQa0ytJ6mNRGz/Q==", - "dev": true, - "license": "MIT", - "optional": true, - "dependencies": { - "@types/node": "*" - } - }, "node_modules/@typescript-eslint/eslint-plugin": { "version": "8.50.0", "resolved": "https://registry.npmjs.org/@typescript-eslint/eslint-plugin/-/eslint-plugin-8.50.0.tgz", @@ -5136,16 +5136,6 @@ "ieee754": "^1.1.13" } }, - "node_modules/buffer-crc32": { - "version": "0.2.13", - "resolved": "https://registry.npmjs.org/buffer-crc32/-/buffer-crc32-0.2.13.tgz", - "integrity": "sha512-VO9Ht/+p3SN7SKWqcrgEzjGbRSJYTx+Q1pTQC0wrWqHx0vpJraQ6GtHx8tvcg1rlK1byhU5gccxgOgj7B0TDkQ==", - "dev": true, - "license": "MIT", - "engines": { - "node": "*" - } - }, "node_modules/buffer-from": { "version": "1.1.2", "resolved": "https://registry.npmjs.org/buffer-from/-/buffer-from-1.1.2.tgz", @@ -7582,43 +7572,6 @@ "node": ">=4" } }, - "node_modules/extract-zip": { - "version": "2.0.1", - "resolved": "https://registry.npmjs.org/extract-zip/-/extract-zip-2.0.1.tgz", - "integrity": "sha512-GDhU9ntwuKyGXdZBUgTIe+vXnWj0fppUEtMDL0+idd5Sta8TGpHssn/eusA9mrPr9qNDym6SxAYZjNvCn/9RBg==", - "dev": true, - "license": "BSD-2-Clause", - "dependencies": { - "debug": "^4.1.1", - "get-stream": "^5.1.0", - "yauzl": "^2.10.0" - }, - "bin": { - "extract-zip": "cli.js" - }, - "engines": { - "node": ">= 10.17.0" - }, - "optionalDependencies": { - "@types/yauzl": "^2.9.1" - } - }, - "node_modules/extract-zip/node_modules/get-stream": { - "version": "5.2.0", - "resolved": "https://registry.npmjs.org/get-stream/-/get-stream-5.2.0.tgz", - "integrity": "sha512-nBF+F1rAZVCu/p7rjzgA+Yb4lfYXrpl7a6VmJrU8wF9I1CKvP/QwPNZHnOlwbTkY6dvtFIzFMSyQXbLoTQPRpA==", - "dev": true, - "license": "MIT", - "dependencies": { - "pump": "^3.0.0" - }, - "engines": { - "node": ">=8" - }, - "funding": { - "url": "https://github.com/sponsors/sindresorhus" - } - }, "node_modules/fast-deep-equal": { "version": "3.1.3", "resolved": "https://registry.npmjs.org/fast-deep-equal/-/fast-deep-equal-3.1.3.tgz", @@ -7705,16 +7658,6 @@ "bser": "2.1.1" } }, - "node_modules/fd-slicer": { - "version": "1.1.0", - "resolved": "https://registry.npmjs.org/fd-slicer/-/fd-slicer-1.1.0.tgz", - "integrity": "sha512-cE1qsB/VwyQozZ+q1dGxR8LBYNZeofhEdUNGSMbQD3Gw2lAzX9Zb3uIU6Ebc/Fmyjo9AWWfnn0AUCHqtevs/8g==", - "dev": true, - "license": "MIT", - "dependencies": { - "pend": "~1.2.0" - } - }, "node_modules/figures": { "version": "6.1.0", "resolved": "https://registry.npmjs.org/figures/-/figures-6.1.0.tgz", @@ -12844,13 +12787,6 @@ "url": "https://github.com/sponsors/jet2jet" } }, - "node_modules/pend": { - "version": "1.2.0", - "resolved": "https://registry.npmjs.org/pend/-/pend-1.2.0.tgz", - "integrity": "sha512-F3asv42UuXchdzt+xXqfW1OGlVBe+mxa2mqI0pg5yAHZPvFmY3Y6drSf/GQ1A86WgWEN9Kzh/WrgKa6iGcHXLg==", - "dev": true, - "license": "MIT" - }, "node_modules/picocolors": { "version": "1.1.1", "resolved": "https://registry.npmjs.org/picocolors/-/picocolors-1.1.1.tgz", @@ -16617,17 +16553,6 @@ "node": ">=8" } }, - "node_modules/yauzl": { - "version": "2.10.0", - "resolved": "https://registry.npmjs.org/yauzl/-/yauzl-2.10.0.tgz", - "integrity": "sha512-p4a9I6X6nu6IhoGmBqAcbJy1mlC4j27vEPZX9F4L4/vZT3Lyq1VkFHw/V/PUcB9Buo+DG3iHkT0x3Qya58zc3g==", - "dev": true, - "license": "MIT", - "dependencies": { - "buffer-crc32": "~0.2.3", - "fd-slicer": "~1.1.0" - } - }, "node_modules/yocto-queue": { "version": "0.1.0", "resolved": "https://registry.npmjs.org/yocto-queue/-/yocto-queue-0.1.0.tgz", diff --git a/package.json b/package.json index 325b013..cfd7400 100644 --- a/package.json +++ b/package.json @@ -101,6 +101,7 @@ "tar": "$tar", "brace-expansion@1": "^1.1.18", "minimatch@3": "^3.1.5", - "tmp": "^0.2.6" + "tmp": "^0.2.6", + "extract-zip": "npm:@electron-internal/extract-zip@^1.0.5" } } diff --git a/src/cli/policy.test.ts b/src/cli/policy.test.ts index e45bc7d..743f972 100644 --- a/src/cli/policy.test.ts +++ b/src/cli/policy.test.ts @@ -1,5 +1,5 @@ import { describe, it, expect } from '@jest/globals'; -import { parsePolicyArgs } from './policy'; +import { parsePolicyArgs, printPolicy } from './policy'; describe('CLI policy command', () => { describe('parsePolicyArgs', () => { @@ -82,3 +82,43 @@ describe('CLI policy command', () => { }); }); }); + +describe('policy show renders the docker grants', () => { + const capture = (policy: unknown): string => { + const lines: string[] = []; + const original = console.log; + console.log = (...args: unknown[]) => void lines.push(args.join(' ')); + try { + printPolicy(policy as never); + } finally { + console.log = original; + } + return lines.join('\n'); + }; + + it('names every container grant, since approving is what these are shown for', () => { + // `localmost policy approve` writes the whole .localmostrc to the cache, + // docker section included, but `show` rendered network, filesystem and env + // only - so the container, mount and network grants were approved unseen. + const out = capture({ + docker: { + pull: { registries: ['docker.io'] }, + run: { + images: ['alpine:3'], + mounts: [{ path: './', mode: 'ro' }], + network: 'bridge', + networks: [{ name: 'localmost-e2e-*', internal: true }], + }, + }, + }); + expect(out).toMatch(/docker pull: docker\.io/); + expect(out).toMatch(/docker run image: alpine:3/); + expect(out).toMatch(/docker mount: \.\/ \(ro\)/); + // Routable vs internal is the part an operator most needs to see. + expect(out).toMatch(/docker network create: localmost-e2e-\* \(internal\)/); + }); + + it('says nothing about docker when none is declared', () => { + expect(capture({ network: { allow: ['github.com'] } })).not.toMatch(/docker/i); + }); +}); diff --git a/src/cli/policy.ts b/src/cli/policy.ts index 15c199e..90347f4 100644 --- a/src/cli/policy.ts +++ b/src/cli/policy.ts @@ -11,6 +11,7 @@ import * as fs from 'fs'; import * as path from 'path'; +import { DescribablePolicy, describePolicy } from '../shared/policy-describe'; import { findLocalmostrc, parseLocalmostrc, @@ -101,70 +102,30 @@ shared: } } -interface PrintablePolicy { - network?: { allow?: string[]; deny?: string[] }; - filesystem?: { read?: string[]; write?: string[]; deny?: string[] }; - env?: { allow?: string[]; deny?: string[] }; -} /** * Print a policy section. + * + * Exported so a test can drive it directly: this is what an operator reads + * before running `localmost policy approve`, so what it leaves out is approved + * unseen. */ -function printPolicy(policy: PrintablePolicy): void { - if (!policy || Object.keys(policy).length === 0) { +export function printPolicy(policy: DescribablePolicy): void { + const grants = describePolicy(policy); + if (grants.length === 0) { console.log(' (empty - uses defaults only)'); return; } - if (policy.network) { - if (policy.network.allow?.length) { - console.log(' Network allow:'); - for (const domain of policy.network.allow) { - console.log(` ${colors.green}+${colors.reset} ${domain}`); - } - } - if (policy.network.deny?.length) { - console.log(' Network deny:'); - for (const domain of policy.network.deny) { - console.log(` ${colors.red}-${colors.reset} ${domain}`); - } - } - } - - if (policy.filesystem) { - if (policy.filesystem.read?.length) { - console.log(' Filesystem read:'); - for (const filePath of policy.filesystem.read) { - console.log(` ${colors.cyan}r${colors.reset} ${filePath}`); - } - } - if (policy.filesystem.write?.length) { - console.log(' Filesystem write:'); - for (const filePath of policy.filesystem.write) { - console.log(` ${colors.green}w${colors.reset} ${filePath}`); - } - } - if (policy.filesystem.deny?.length) { - console.log(' Filesystem deny:'); - for (const filePath of policy.filesystem.deny) { - console.log(` ${colors.red}-${colors.reset} ${filePath}`); - } - } - } - - if (policy.env) { - if (policy.env.allow?.length) { - console.log(' Environment allow:'); - for (const name of policy.env.allow) { - console.log(` ${colors.green}+${colors.reset} ${name}`); - } - } - if (policy.env.deny?.length) { - console.log(' Environment deny:'); - for (const name of policy.env.deny) { - console.log(` ${colors.red}-${colors.reset} ${name}`); - } + const colorFor: Record = { '+': colors.green, '-': colors.red, r: colors.cyan, w: colors.green }; + let group = ''; + for (const grant of grants) { + if (grant.group !== group) { + group = grant.group; + console.log(` ${group}:`); } + const color = colorFor[grant.marker] ?? colors.green; + console.log(` ${color}${grant.marker}${colors.reset} ${grant.value}`); } } diff --git a/src/main/broker-proxy-service.test.ts b/src/main/broker-proxy-service.test.ts index f6df427..8051b2e 100644 --- a/src/main/broker-proxy-service.test.ts +++ b/src/main/broker-proxy-service.test.ts @@ -451,6 +451,28 @@ describe('extractGitHubJobInfo', () => { }); }); +describe('the workflow a per-workflow policy section keys on', () => { + it('uses the workflow filename, which is what .localmostrc keys are documented to match', () => { + const info = extractGitHubJobInfo({ github: { d: [ + { k: 'workflow', v: 'CI / build and test' }, + { k: 'workflow_ref', v: 'bfulton/localmost/.github/workflows/ci.yaml@refs/heads/main' }, + ] } }); + expect(info.githubWorkflow).toBe('ci'); + }); + + it('handles a .yml extension and a ref containing slashes', () => { + const info = extractGitHubJobInfo({ github: { d: [ + { k: 'workflow_ref', v: 'o/r/.github/workflows/docker-access.yml@refs/pull/35/merge' }, + ] } }); + expect(info.githubWorkflow).toBe('docker-access'); + }); + + it('falls back to the workflow name when no ref is supplied', () => { + const info = extractGitHubJobInfo({ github: { d: [{ k: 'workflow', v: 'Docker Access' }] } }); + expect(info.githubWorkflow).toBe('Docker Access'); + }); +}); + describe('message routing', () => { interface Instance { sessionId?: string; runner: { agentName: string } } interface RoutingInternals { diff --git a/src/main/broker-proxy-service.ts b/src/main/broker-proxy-service.ts index d8a7341..dc7b815 100644 --- a/src/main/broker-proxy-service.ts +++ b/src/main/broker-proxy-service.ts @@ -261,11 +261,22 @@ interface ContextDictEntry { * Pull the job's GitHub identity out of the broker job details' contextData. * Pure, so the mapping from context keys to job info can be tested directly. */ +/** The workflow filename, without extension, from a github.workflow_ref value. */ +function workflowFilename(ref: string | undefined): string | undefined { + if (!ref) return undefined; + // owner/repo/.github/workflows/@; the ref itself may contain '/'. + const path = ref.split('@')[0]; + const file = path.slice(path.lastIndexOf('/') + 1); + if (!file) return undefined; + return file.replace(/\.ya?ml$/i, ''); +} + export function extractGitHubJobInfo(contextData: { github?: { d?: ContextDictEntry[] }; job?: { d?: ContextDictEntry[] }; } | undefined): GitHubJobInfo { const info: GitHubJobInfo = {}; + let workflowRef: string | undefined; const github = contextData?.github; if (github?.d && Array.isArray(github.d)) { @@ -276,9 +287,18 @@ export function extractGitHubJobInfo(contextData: { if (item.k === 'sha') info.githubSha = item.v; if (item.k === 'ref') info.githubRef = item.v; if (item.k === 'workflow') info.githubWorkflow = item.v; + if (item.k === 'workflow_ref') workflowRef = item.v; } } + // `.localmostrc` keys under `workflows:` match the workflow FILENAME, but + // `github.workflow` is the workflow's `name:` - a free-form string that only + // equals the filename by coincidence. `github.workflow_ref` carries the real + // path (owner/repo/.github/workflows/@), so the filename comes + // from there when the service sends it, and the name remains the fallback. + const fromRef = workflowFilename(workflowRef); + if (fromRef) info.githubWorkflow = fromRef; + // Job ID (check_run_id) is in the job context const job = contextData?.job; if (job?.d && Array.isArray(job.d)) { diff --git a/src/main/docker/docker-backend.test.ts b/src/main/docker/docker-backend.test.ts index cd368f0..a73a653 100644 --- a/src/main/docker/docker-backend.test.ts +++ b/src/main/docker/docker-backend.test.ts @@ -54,3 +54,18 @@ describe('DesktopBackend', () => { expect(backend.workspaceMountRoot('/tmp/sandbox/1')).toBe('/tmp/sandbox/1/checkout'); }); }); + +describe('the root that declared mount paths resolve against', () => { + const backend = new DesktopBackend({ resolve: () => null }); + + it('is the repository checkout, which is what "./" means in a workflow', () => { + // The runner checks out into _work// (GITHUB_WORKSPACE). Rooting + // at _work instead made every declared path narrower than "./" unmatchable: + // "./tmp/fixtures" resolved to _work/tmp/fixtures, which never exists. + expect(backend.workspaceMountRoot('/s/1', 'bfulton/localmost')).toBe('/s/1/_work/localmost/localmost'); + }); + + it('falls back to the work folder when no repository is bound yet', () => { + expect(backend.workspaceMountRoot('/s/1')).toBe('/s/1/_work'); + }); +}); diff --git a/src/main/docker/docker-backend.ts b/src/main/docker/docker-backend.ts index 00048e9..d3f73ac 100644 --- a/src/main/docker/docker-backend.ts +++ b/src/main/docker/docker-backend.ts @@ -19,7 +19,13 @@ export interface DockerBackend { /** The daemon endpoint to forward approved requests to, or null when none. */ resolveEndpoint(): DockerEndpoint | null; /** Absolute host path that job mounts must resolve inside (the job workspace). */ - workspaceMountRoot(sandboxDir: string): string; + /** + * The directory declared mount paths resolve against: the repository + * checkout when the socket is bound to one, since that is what `./` means to + * whoever wrote the policy. Without a repository - a socket not yet bound - + * the work folder is the widest honest answer. + */ + workspaceMountRoot(sandboxDir: string, repository?: string): string; } export interface DesktopBackendOptions { @@ -51,7 +57,12 @@ export class DesktopBackend implements DockerBackend { return this.opts.resolve ? this.opts.resolve() : resolveDockerEndpoint(); } - workspaceMountRoot(sandboxDir: string): string { - return path.join(sandboxDir, this.opts.workspaceSubdir ?? RUNNER_WORK_FOLDER); + workspaceMountRoot(sandboxDir: string, repository?: string): string { + const work = path.join(sandboxDir, this.opts.workspaceSubdir ?? RUNNER_WORK_FOLDER); + // The runner checks out into _work//, which is GITHUB_WORKSPACE + // and what a policy's `./` refers to. Rooting at _work made anything + // narrower than `./` unmatchable, since ./tmp resolved to _work/tmp. + const name = repository?.split('/').pop(); + return name ? path.join(work, name, name) : work; } } diff --git a/src/main/docker/docker-evaluator.test.ts b/src/main/docker/docker-evaluator.test.ts index 03a4091..dad5da4 100644 --- a/src/main/docker/docker-evaluator.test.ts +++ b/src/main/docker/docker-evaluator.test.ts @@ -334,3 +334,429 @@ describe('policy hints', () => { } }); }); + +describe('Go case-insensitive JSON decoding', () => { + // The daemon decodes the create body with Go's encoding/json, which matches + // struct fields case-insensitively as a documented fallback. So a key the + // filter reads as absent is honoured by the daemon: every HostConfig gate is + // bypassed by changing one letter. + const p = { run: { images: ['postgres:16'], mounts: [{ path: './', mode: 'ro' as const }], network: 'bridge' } }; + + it('refuses a lowercased HostConfig carrying privileged and a root bind', () => { + const v = evaluateDockerRequest( + mk('POST', '/v1.45/containers/create', { + Image: 'postgres:16', + hostconfig: { privileged: true, binds: ['/:/host:rw'], pidmode: 'host' }, + }), + ctx(p) + ); + expect(v.allowed).toBe(false); + }); + + it('refuses odd casings of the gated keys inside a correctly-cased HostConfig', () => { + for (const hostConfig of [ + { Privileged: true }, + { PRIVILEGED: true }, + { privileged: true }, + { BINDS: ['/etc:/x'] }, + { binds: ['/etc:/x'] }, + { networkmode: 'host' }, + { NETWORKMODE: 'host' }, + { pidMode: 'host' }, + { devices: [{ PathOnHost: '/dev/kmsg' }] }, + ]) { + const v = evaluateDockerRequest( + mk('POST', '/v1.45/containers/create', { Image: 'postgres:16', HostConfig: hostConfig }), + ctx(p) + ); + expect([JSON.stringify(hostConfig), v.allowed]).toEqual([JSON.stringify(hostConfig), false]); + } + }); + + it('still permits a correctly-cased create the policy allows', () => { + expect(evaluateDockerRequest(mk('POST', '/v1.45/containers/create', { Image: 'postgres:16' }), ctx(p)).allowed).toBe(true); + }); +}); + +describe('kill, stop and logs on the job\'s own container', () => { + const p = { run: { images: ['postgres:16'], network: 'bridge' } }; + + it('permits them on an owned container and refuses them on one it did not create', () => { + const own = ctx(p, ['mine123']); + for (const [method, tpl] of [ + ['POST', '/v1.45/containers/%s/kill'], + ['POST', '/v1.45/containers/%s/stop'], + ['GET', '/v1.45/containers/%s/logs?stdout=1&stderr=1'], + ] as const) { + expect([tpl, evaluateDockerRequest(mk(method, tpl.replace('%s', 'mine123')), own).allowed]).toEqual([tpl, true]); + expect([tpl, evaluateDockerRequest(mk(method, tpl.replace('%s', 'theirs999')), own).allowed]).toEqual([tpl, false]); + } + }); + + it('refuses kill and stop when the policy declares no run action', () => { + const noRun = ctx({ pull: { registries: ['docker.io'] } }, ['mine123']); + expect(evaluateDockerRequest(mk('POST', '/v1.45/containers/mine123/kill'), noRun).allowed).toBe(false); + expect(evaluateDockerRequest(mk('POST', '/v1.45/containers/mine123/stop'), noRun).allowed).toBe(false); + }); +}); + +describe('volume mounts that are really bind mounts', () => { + const p = { run: { images: ['postgres:16'], mounts: [{ path: './', mode: 'ro' as const }], network: 'bridge' } }; + + it('refuses an anonymous volume whose local-driver options bind a host path', () => { + // The local driver with type=none,o=bind,device= IS a bind mount - + // the same thing compose exposes as driver_opts. The entry has no Source, + // so it looked like container-lifecycle storage and skipped every check. + const v = evaluateDockerRequest( + mk('POST', '/v1.45/containers/create', { + Image: 'postgres:16', + HostConfig: { + Mounts: [{ + Type: 'volume', + Target: '/host', + VolumeOptions: { DriverConfig: { Name: 'local', Options: { type: 'none', o: 'bind', device: '/Users/me/.ssh' } } }, + }], + }, + }), + ctx(p) + ); + expect(v.allowed).toBe(false); + }); + + it('refuses it whatever the casing of the driver keys', () => { + const v = evaluateDockerRequest( + mk('POST', '/v1.45/containers/create', { + Image: 'postgres:16', + HostConfig: { Mounts: [{ type: 'volume', target: '/host', volumeoptions: { driverconfig: { Name: 'local', Options: { device: '/' } } } }] }, + }), + ctx(p) + ); + expect(v.allowed).toBe(false); + }); + + it('still permits a plain anonymous volume and a tmpfs, which reach no host path', () => { + for (const m of [{ Type: 'volume', Target: '/data' }, { Type: 'tmpfs', Target: '/tmp' }]) { + expect([m.Type, evaluateDockerRequest(mk('POST', '/v1.45/containers/create', { Image: 'postgres:16', HostConfig: { Mounts: [m] } }), ctx(p)).allowed]) + .toEqual([m.Type, true]); + } + }); +}); + +describe('HostConfig is an allowlist, not a blocklist', () => { + const p = { run: { images: ['postgres:16'], mounts: [{ path: './', mode: 'ro' as const }], network: 'bridge' } }; + const create = (hostConfig: Record) => + evaluateDockerRequest(mk('POST', '/v1.45/containers/create', { Image: 'postgres:16', HostConfig: hostConfig }), ctx(p)); + + it('refuses publishing container ports onto the operator host', () => { + // -p 8080:80. Nothing in the grammar can name it, and it exposes a + // service on the operator's interfaces, outside the proxy's egress control. + expect(create({ PortBindings: { '80/tcp': [{ HostPort: '8080' }] } }).allowed).toBe(false); + expect(create({ PublishAllPorts: true }).allowed).toBe(false); + }); + + it('refuses any HostConfig key the grammar cannot name, even one invented later', () => { + for (const key of ['StorageOpt', 'SomeFutureEscape', 'Anything', 'NextApiVersionKey']) { + expect([key, create({ [key]: ['x'] }).allowed]).toEqual([key, false]); + } + }); + + it('refuses the keys that only reach outside the container when non-empty', () => { + expect(create({ Links: ['other:db'] }).allowed).toBe(false); + expect(create({ VolumeDriver: 'local' }).allowed).toBe(false); + expect(create({ ExtraHosts: ['evil:1.2.3.4'] }).allowed).toBe(false); + expect(create({ GroupAdd: ['staff'] }).allowed).toBe(false); + expect(create({ Cgroup: '/other' }).allowed).toBe(false); + }); + + it('refuses a --cidfile that would write to a host path, while allowing the empty default', () => { + expect(create({ ContainerIDFile: '/tmp/pwned.cid' }).allowed).toBe(false); + expect(create({ ContainerIDFile: '' }).allowed).toBe(true); + }); + + it('still permits the keys a plain docker run actually sends', () => { + expect(create({}).allowed).toBe(true); + expect(create({ AutoRemove: true, NetworkMode: 'bridge', Binds: [], RestartPolicy: { Name: '', MaximumRetryCount: 0 }, LogConfig: { Type: '', Config: {} }, ConsoleSize: [0, 0] }).allowed).toBe(true); + }); +}); + +describe('build query parameters', () => { + const p = { run: { images: ['postgres:16'], network: 'bridge' }, build: { context: './' } }; + const build = (qs: string, policy: DockerPolicy = p) => + evaluateDockerRequest(mk('POST', `/v1.45/build${qs}`), ctx(policy)); + + it('refuses host and container networking, which the run path already forbids', () => { + expect(build('?networkmode=host').allowed).toBe(false); + expect(build('?networkmode=container%3Aabc').allowed).toBe(false); + }); + + it('refuses an undeclared build network, and permits the declared one', () => { + expect(build('?networkmode=some-other-net').allowed).toBe(false); + expect(build('?networkmode=bridge').allowed).toBe(true); + expect(build('?networkmode=none').allowed).toBe(true); + }); + + it('refuses build parameters that reach the host or the daemon config', () => { + for (const qs of ['?remote=https%3A%2F%2Fevil%2Fctx', '?extrahosts=evil%3A1.2.3.4', '?cachefrom=%5B%22other%3Alatest%22%5D', '?ulimits=x', '?securityopt=seccomp%3Dunconfined', '?outputs=type%3Dlocal%2Cdest%3D%2Ftmp']) { + expect([qs, build(qs).allowed]).toEqual([qs, false]); + } + }); + + it('permits the parameters an ordinary docker build sends', () => { + expect(build('?t=app%3Alatest&dockerfile=Dockerfile&rm=1&buildargs=%7B%7D&labels=%7B%7D&shmsize=0&version=1').allowed).toBe(true); + }); +}); + +describe('networks', () => { + const p: DockerPolicy = { run: { images: ['alpine:3'], network: 'bridge', networks: [{ name: 'vk-*', internal: true }] } }; + const create = (body: unknown, c = ctx(p)) => evaluateDockerRequest(mk('POST', '/v1.45/networks/create', body), c); + + it('permits creating a declared internal network', () => { + expect(create({ Name: 'vk-run1', Internal: true, CheckDuplicate: true }).allowed).toBe(true); + }); + + it('refuses a name no declaration matches, anchoring the glob', () => { + expect(create({ Name: 'other', Internal: true }).allowed).toBe(false); + expect(create({ Name: 'not-vk-run1', Internal: true }).allowed).toBe(false); + }); + + it('refuses a routable network where the declaration says internal', () => { + expect(create({ Name: 'vk-run1', Internal: false }).allowed).toBe(false); + expect(create({ Name: 'vk-run1' }).allowed).toBe(false); + }); + + it('refuses any create key the grammar cannot spell, driver above all', () => { + for (const extra of [{ Driver: 'macvlan' }, { Options: { parent: 'en0' } }, { IPAM: { Config: [{ Subnet: '10.0.0.0/8' }] } }, { Attachable: true }, { Ingress: true }, { ConfigOnly: true }]) { + const body = { Name: 'vk-run1', Internal: true, ...extra }; + expect([Object.keys(extra)[0], create(body).allowed]).toEqual([Object.keys(extra)[0], false]); + } + // The default driver, stated explicitly, is the one the filter would use anyway. + expect(create({ Name: 'vk-run1', Internal: true, Driver: 'bridge' }).allowed).toBe(true); + }); + + it('never lists the daemon\'s networks', () => { + expect(evaluateDockerRequest(mk('GET', '/v1.45/networks'), ctx(p)).allowed).toBe(false); + }); + + it('scopes reading and deleting a network to ones this socket created', () => { + const own = ctx(p, { ownNetworkIds: new Set(['net123']) }); + expect(evaluateDockerRequest(mk('GET', '/v1.45/networks/net123'), own).allowed).toBe(true); + expect(evaluateDockerRequest(mk('DELETE', '/v1.45/networks/net123'), own).allowed).toBe(true); + expect(evaluateDockerRequest(mk('GET', '/v1.45/networks/theirs'), own).allowed).toBe(false); + expect(evaluateDockerRequest(mk('DELETE', '/v1.45/networks/theirs'), own).allowed).toBe(false); + }); + + it('lets a container join a network this job created, which is the point of declaring one', () => { + const own = ctx(p, { ownNetworkIds: new Set(['vk-run1']) }); + const body = { Image: 'alpine:3', HostConfig: { NetworkMode: 'vk-run1' } }; + expect(evaluateDockerRequest(mk('POST', '/v1.45/containers/create', body), own).allowed).toBe(true); + // An arbitrary network the job did not create is still refused. + const other = { Image: 'alpine:3', HostConfig: { NetworkMode: 'someone-elses' } }; + expect(evaluateDockerRequest(mk('POST', '/v1.45/containers/create', other), own).allowed).toBe(false); + }); +}); + +describe('image inspect', () => { + const p: DockerPolicy = { run: { images: ['alpine:3', 'ghcr.io/o/app:1'], network: 'bridge' }, pull: { registries: ['docker.io'] } }; + const inspect = (ref: string, c = ctx(p)) => + evaluateDockerRequest(mk('GET', `/v1.45/images/${encodeURIComponent(ref)}/json`), c); + + it('permits inspecting an image the policy already names', () => { + // Scoped by the policy rather than by a second ownership ledger: an + // inspect of an image run.images already grants discloses nothing new. + expect(inspect('alpine:3').allowed).toBe(true); + expect(inspect('docker.io/library/alpine:3').allowed).toBe(true); + expect(inspect('ghcr.io/o/app:1').allowed).toBe(true); + }); + + it('refuses an image the policy does not name', () => { + expect(inspect('postgres:16').allowed).toBe(false); + expect(inspect('ghcr.io/o/other:1').allowed).toBe(false); + }); + + it('refuses it when the policy declares no run action at all', () => { + expect(inspect('alpine:3', ctx({ pull: { registries: ['docker.io'] } })).allowed).toBe(false); + }); + + it('never lists or deletes images, which are daemon-wide', () => { + expect(evaluateDockerRequest(mk('GET', '/v1.45/images/json'), ctx(p)).allowed).toBe(false); + expect(evaluateDockerRequest(mk('DELETE', '/v1.45/images/alpine:3'), ctx(p)).allowed).toBe(false); + }); +}); + +describe('network create as the real CLI sends it', () => { + const p: DockerPolicy = { run: { images: ['alpine:3'], network: 'bridge', networks: [{ name: 'vk-*', internal: true }] } }; + // Captured off the wire from docker CLI 29.3.1. Every one of these keys is + // sent unconditionally, with an inert default. + const cliBody = (over: Record = {}) => ({ + Name: 'vk-probe-net', Driver: 'bridge', Scope: '', + IPAM: { Driver: 'default', Options: {}, Config: [] }, + Internal: true, Attachable: false, Ingress: false, ConfigOnly: false, + ConfigFrom: null, Options: {}, Labels: {}, ...over, + }); + const create = (body: unknown, c = ctx(p)) => evaluateDockerRequest(mk('POST', '/v1.45/networks/create', body), c); + + it('permits what `docker network create --internal` actually sends', () => { + expect(create(cliBody()).allowed).toBe(true); + }); + + it('still refuses those same keys when they carry a meaningful value', () => { + for (const over of [ + { Scope: 'swarm' }, + { IPAM: { Driver: 'default', Options: {}, Config: [{ Subnet: '10.0.0.0/8' }] } }, + { IPAM: { Driver: 'macvlan', Options: {}, Config: [] } }, + { IPAM: { Driver: 'default', Options: { parent: 'en0' }, Config: [] } }, + { Attachable: true }, { Ingress: true }, { ConfigOnly: true }, + { ConfigFrom: { Network: 'other' } }, + { Options: { 'com.docker.network.bridge.host_binding_ipv4': '0.0.0.0' } }, + { EnableIPv6: true }, + ]) { + expect([Object.keys(over)[0], create(cliBody(over)).allowed]).toEqual([Object.keys(over)[0], false]); + } + }); + + it('is fail-closed when casings disagree, as Go would decode them', () => { + // Go matches struct fields case-insensitively, so a second casing with a + // different value may be the one the daemon honours. + expect(create({ ...cliBody(), internal: false }).allowed).toBe(false); + expect(create({ ...cliBody(), name: 'not-declared' }).allowed).toBe(false); + expect(create({ ...cliBody(), driver: 'macvlan' }).allowed).toBe(false); + }); +}); + +describe('image globs', () => { + // A content-addressed tag cannot be known when the policy is written. + const p: DockerPolicy = { run: { images: ['vk/grader:*', 'alpine:3'], network: 'bridge' } }; + + it('permits creating and inspecting an image matching a declared glob', () => { + const body = { Image: 'vk/grader:7f2-0123456789ab' }; + expect(evaluateDockerRequest(mk('POST', '/v1.45/containers/create', body), ctx(p)).allowed).toBe(true); + expect(evaluateDockerRequest(mk('GET', '/v1.45/images/vk%2Fgrader%3A7f2-0123456789ab/json'), ctx(p)).allowed).toBe(true); + }); + + it('anchors the glob, so a lookalike repository does not match', () => { + for (const image of ['evil/vk/grader:x', 'notvk/grader:x', 'vk/grader-evil:x']) { + expect([image, evaluateDockerRequest(mk('POST', '/v1.45/containers/create', { Image: image }), ctx(p)).allowed]) + .toEqual([image, false]); + } + }); + + it('leaves an exact declaration exact', () => { + expect(evaluateDockerRequest(mk('POST', '/v1.45/containers/create', { Image: 'alpine:3' }), ctx(p)).allowed).toBe(true); + expect(evaluateDockerRequest(mk('POST', '/v1.45/containers/create', { Image: 'alpine:4' }), ctx(p)).allowed).toBe(false); + }); +}); + +describe('what * spans in a declared glob', () => { + const withImages = (images: string[]) => ctx({ run: { images, network: 'bridge' } }); + const create = (image: string, images: string[]) => + evaluateDockerRequest(mk('POST', '/v1.45/containers/create', { Image: image }), withImages(images)).allowed; + + it('spans a tag but not a path separator', () => { + // The spec left this open. Decided here: `*` stops at `/`, so a declared + // repository cannot be widened into deeper paths by a reference that adds + // segments. A tag glob - the content-addressed case - is unaffected, + // because a tag cannot contain a slash. + expect(create('vk/grader:7f2-0123456789ab', ['vk/grader:*'])).toBe(true); + expect(create('vk/grader:a/b', ['vk/grader:*'])).toBe(false); + }); + + it('still anchors, so a lookalike repository never matches', () => { + expect(create('evil/vk/grader:x', ['vk/grader:*'])).toBe(false); + }); + + it('needs a segment of its own to span one', () => { + // `vk/*:*` reaches one level under vk, and no further. + expect(create('vk/app:1', ['vk/*:*'])).toBe(true); + expect(create('vk/team/app:1', ['vk/*:*'])).toBe(false); + }); + + it('is bounded by the tag too, which is why a tagless glob is refused upstream', () => { + // Normalisation appends :latest to a tagless reference on both sides, so a + // tagless glob is matched as `vk/*:latest` - it covers latest and nothing + // else, however wide it reads. validateDockerPolicy rejects the form for + // that reason; this pins the behaviour the rejection exists to prevent. + expect(create('vk/app', ['vk/*'])).toBe(true); + expect(create('vk/app:1', ['vk/*'])).toBe(false); + expect(create('vk/app:1', ['vk/*:*'])).toBe(true); + }); +}); + +describe('BuildKit endpoints', () => { + const p: DockerPolicy = { run: { images: ['alpine:3'], network: 'bridge' }, build: { context: './' } }; + + it('refuses a BuildKit session, and says why rather than shrugging', () => { + // A real `docker build` on a default install issues zero POST /build: it + // negotiates a session and streams over /grpc. Denying it generically read + // as "unknown endpoint" when the real answer is "that builder cannot be + // filtered, and we pinned you off it". + for (const url of ['/v1.45/grpc', '/v1.45/session']) { + const v = evaluateDockerRequest(mk('POST', url), ctx(p)); + expect([url, v.allowed]).toEqual([url, false]); + expect(v.reason).toMatch(/BuildKit/i); + expect(v.reason).toMatch(/DOCKER_BUILDKIT/); + } + }); + + it('still permits the classic build the policy describes', () => { + expect(evaluateDockerRequest(mk('POST', '/v1.45/build?t=app%3A1'), ctx(p)).allowed).toBe(true); + }); +}); + +describe('duplicate keys that differ only in case', () => { + const runPolicy: DockerPolicy = { run: { images: ['alpine:3'], network: 'bridge' } }; + + it('refuses a body carrying two casings of the same key, rather than guessing which one counts', () => { + // Measured against a real daemon: with `HostConfig`, `hostconfig` and + // `HOSTCONFIG` all present, Go's decoder MERGED all three into one struct + // (AutoRemove from the first, Memory from the second, OomScoreAdj from the + // third). Scalars and arrays inside one object are last-wins instead. + // No filter can read one of those objects and know what the daemon will + // do, and picking the last is as wrong as picking the first - the merge + // keeps fields from both. Go's encoder never emits case-variant duplicates, + // so a body containing them is not a client we model. + const v = evaluateDockerRequest( + mk('POST', '/v1.45/containers/create', { + Image: 'alpine:3', + HostConfig: { NetworkMode: 'bridge' }, + hostconfig: { Binds: ['/etc:/host-etc'] }, + }), + ctx(runPolicy) + ); + expect(v.allowed).toBe(false); + expect(v.reason).toMatch(/case/i); + expect(v.reason).toMatch(/HostConfig|hostconfig/); + }); + + it('finds them however deep they are nested', () => { + const v = evaluateDockerRequest( + mk('POST', '/v1.45/containers/create', { + Image: 'alpine:3', + HostConfig: { NetworkMode: 'bridge', Mounts: [{ Type: 'bind', Source: '/ws', type: 'tmpfs' }] }, + }), + ctx(runPolicy) + ); + expect(v.allowed).toBe(false); + expect(v.reason).toMatch(/case/i); + }); + + it('leaves an ordinary body alone, including keys that merely resemble each other', () => { + const v = evaluateDockerRequest( + mk('POST', '/v1.45/containers/create', { + Image: 'alpine:3', + HostConfig: { NetworkMode: 'bridge', Memory: 0, MemorySwap: 0, Binds: [] }, + }), + ctx(runPolicy) + ); + expect(v.allowed).toBe(true); + }); + + it('applies to every action with a body, not just create', () => { + const p: DockerPolicy = { run: { images: ['alpine:3'], network: 'bridge', networks: [{ name: 'vk-1', internal: true }] } }; + const v = evaluateDockerRequest( + mk('POST', '/v1.45/networks/create', { Name: 'vk-1', Internal: true, internal: false }), + ctx(p) + ); + expect(v.allowed).toBe(false); + expect(v.reason).toMatch(/case/i); + }); +}); diff --git a/src/main/docker/docker-evaluator.ts b/src/main/docker/docker-evaluator.ts index 584144c..87da22b 100644 --- a/src/main/docker/docker-evaluator.ts +++ b/src/main/docker/docker-evaluator.ts @@ -13,8 +13,8 @@ import * as fs from 'fs'; import * as path from 'path'; -import { DockerPolicy, DockerMount, MountMode } from '../../shared/docker-policy'; -import { DockerRequest, DockerAction, classifyDockerRequest, containerIdFrom } from './docker-request'; +import { DockerPolicy, DockerMount, DockerNetworkPolicy, MountMode } from '../../shared/docker-policy'; +import { DockerRequest, DockerAction, classifyDockerRequest, containerIdFrom, imageRefFrom, networkIdFrom } from './docker-request'; export interface DockerEvalContext { /** The bound policy; null until the worker claims a job, which denies all. */ @@ -33,6 +33,12 @@ export interface DockerEvalContext { * operator and with other jobs, so an unscoped id reaches outside this job. */ ownContainerIds?: ReadonlySet; + /** + * Networks created through this socket, by id and by name. A container may + * join one of these as well as the declared `run.network`, which is the + * whole point of letting a job create one. + */ + ownNetworkIds?: ReadonlySet; /** Injected for tests; defaults to fs.realpathSync. Must throw when the path does not exist. */ realpath?: (p: string) => string; } @@ -43,6 +49,13 @@ export interface DockerVerdict { reason?: string; /** The policy that would permit it, as YAML under `docker:` (for --updaterc discovery). */ policyHint?: string; + /** + * A create body whose mount sources are rewritten to the paths this verdict + * actually checked. Forwarding the spelling the client sent would let the + * daemon resolve it a second time, and the job can swap a symlink in the gap + * between the two resolutions; forwarding what was checked closes that. + */ + rewrittenBody?: unknown; } const ALLOW: DockerVerdict = { allowed: true }; @@ -55,6 +68,31 @@ const deny = (reason: string, policyHint?: string): DockerVerdict => // OWN containers, which is enforced per id below. const BASELINE: ReadonlySet = new Set(['ping', 'version', 'info']); +/** + * Every value whose key case-insensitively equals `name`. + * + * The daemon decodes these bodies with Go's encoding/json, which matches a + * struct field by exact name and then, as a documented fallback, case + * -insensitively. Reading `hostConfig.Privileged` in JS therefore sees nothing + * in a body that says "privileged", while the daemon honours it - so the + * filter must consider every casing, not the one it expects. + */ +function valuesFor(obj: Record, name: string): unknown[] { + const wanted = name.toLowerCase(); + const out: unknown[] = []; + for (const [key, value] of Object.entries(obj)) { + if (key.toLowerCase() === wanted) out.push(value); + } + return out; +} + +/** The value the daemon would use: the exact-cased key if present, else any case-insensitive match. */ +function pick(obj: Record, name: string): unknown { + if (Object.prototype.hasOwnProperty.call(obj, name)) return obj[name]; + const matches = valuesFor(obj, name); + return matches.length > 0 ? matches[0] : undefined; +} + const isPlainObject = (value: unknown): value is Record => typeof value === 'object' && value !== null && !Array.isArray(value); @@ -73,6 +111,8 @@ const hints = { registry: (registry: string) => `docker:\n pull:\n registries:\n - ${registry}`, build: 'docker:\n build:\n context: "./"', privileged: 'docker:\n privileged: true', + network_declaration: (name: string, internal: boolean) => + `docker:\n run:\n networks:\n - name: ${yamlString(name)}\n internal: ${internal}`, }; // ----------------------------------------------------------------------------- @@ -127,8 +167,56 @@ const oneOf = (...allowed: string[]) => (v: unknown): boolean => isUnset(v) || a * only permitted values are the defaults the docker CLI sends for it; the * shapes are bounded by the API version the proxy pins. */ +/** + * HostConfig keys the filter understands and will forward. + * + * An allowlist, not a blocklist: enumerating the dangerous keys means every + * key nobody thought of - and every key a future API version adds - is + * forwarded unexamined. PortBindings was exactly that, publishing a container + * port on the operator's interfaces, outside the proxy that controls the job's + * egress. A key absent from this list is refused, which is the same principle + * the grammar applies to itself: what cannot be named cannot be requested. + * + * These are the keys an ordinary `docker run` sends. Each is either inert + * (resource limits, logging, restart behaviour) or gated below. + */ +const HOST_CONFIG_KNOWN: ReadonlySet = new Set([ + // Gated below by value, or checked by the mount and network logic. + 'privileged', 'binds', 'mounts', 'networkmode', 'containeridfile', 'portbindings', 'publishallports', + 'pidmode', 'ipcmode', 'utsmode', 'usernsmode', 'cgroupnsmode', 'cgroupparent', 'cgroup', + 'devices', 'devicerequests', 'devicecgrouprules', 'securityopt', 'capadd', 'sysctls', 'runtime', + 'isolation', 'maskedpaths', 'readonlypaths', 'volumesfrom', 'extrahosts', 'groupadd', 'links', + 'volumedriver', + // Inert: they bound the container, they do not widen it. Dropping capabilities + // and setting resource limits or DNS search only ever restricts. + 'capdrop', 'autoremove', 'restartpolicy', 'logconfig', 'consolesize', 'readonlyrootfs', 'init', + 'oomscoreadj', 'oomkilldisable', 'shmsize', 'memory', 'memoryswap', 'memoryreservation', + 'memoryswappiness', 'kernelmemory', 'nanocpus', 'cpushares', 'cpuperiod', 'cpuquota', + 'cpurealtimeperiod', 'cpurealtimeruntime', 'cpusetcpus', 'cpusetmems', 'cpucount', 'cpupercent', + 'blkioweight', 'blkioweightdevice', 'blkiodevicereadbps', 'blkiodevicewritebps', + 'blkiodevicereadiops', 'blkiodevicewriteiops', 'pidslimit', 'dns', 'dnsoptions', 'dnssearch', + 'annotations', 'tmpfs', 'ulimits', 'iomaximumbandwidth', 'iomaximumiops', +]); + const HOST_CONFIG_GATES: ReadonlyArray<{ key: string; permitted: (v: unknown) => boolean; flag: string }> = [ + // The daemon writes the new container's id to this HOST path, so a non-empty + // value creates or truncates a file anywhere the daemon can reach. The CLI + // always sends it, empty. + { key: 'ContainerIDFile', permitted: isEmptyString, flag: '--cidfile' }, + // Publishing binds a listening socket on the operator's interfaces, exposing + // a container service to their network and outside the proxy that controls + // this job's egress. The CLI sends both, empty, on every run. + { key: 'PortBindings', permitted: isEmptyObject, flag: '-p/--publish' }, + { key: 'PublishAllPorts', permitted: (v: unknown) => isUnset(v) || v === false, flag: '-P/--publish-all' }, { key: 'PidMode', permitted: isEmptyString, flag: '--pid' }, + // Each is sent empty by every ordinary run, and each reaches outside the + // container when it is not: a cgroup to join, hosts entries, extra groups, + // a link to another job's container, or a volume driver that can bind-mount. + { key: 'Cgroup', permitted: isEmptyString, flag: '--cgroup' }, + { key: 'ExtraHosts', permitted: isEmptyArray, flag: '--add-host' }, + { key: 'GroupAdd', permitted: isEmptyArray, flag: '--group-add' }, + { key: 'Links', permitted: isEmptyArray, flag: '--link' }, + { key: 'VolumeDriver', permitted: isEmptyString, flag: '--volume-driver' }, { key: 'IpcMode', permitted: oneOf('', 'private', 'none', 'shareable'), flag: '--ipc' }, { key: 'UTSMode', permitted: isEmptyString, flag: '--uts' }, { key: 'UsernsMode', permitted: isEmptyString, flag: '--userns' }, @@ -192,31 +280,44 @@ function parseBind(bind: string): MountRequest | string { /** Parse one entry of HostConfig.Mounts; null when it needs no host check. */ function parseMount(mount: unknown): MountRequest | string | null { if (!isPlainObject(mount)) return 'each entry of HostConfig.Mounts must be an object'; - const type = mount.Type; + const type = pick(mount, 'Type'); if (type === 'tmpfs') return null; if (type === 'volume') { - if (isEmptyString(mount.Source)) return null; // anonymous: lives with the container - return `"${mount.Source}" is a named volume, not a workspace path; only declared workspace mounts are permitted`; + // A volume is only container-lifecycle storage while it uses the default + // driver with no options. The built-in local driver with + // type=none,o=bind,device= IS a bind mount - the mechanism compose + // exposes as driver_opts - so an "anonymous" volume carrying driver + // options reaches an arbitrary host path, read-write, having skipped every + // mount check because it declares no Source. + const volumeOptions = pick(mount, 'VolumeOptions'); + if (isPlainObject(volumeOptions) && !isUnset(pick(volumeOptions, 'DriverConfig'))) { + return 'a volume with DriverConfig is not permitted: a volume driver can bind-mount a host path, which only a declared workspace mount may do'; + } + if (isEmptyString(pick(mount, 'Source'))) return null; // anonymous: lives with the container + return `"${String(pick(mount, 'Source'))}" is a named volume, not a workspace path; only declared workspace mounts are permitted`; } if (type !== 'bind') return `mount type "${String(type)}" is not permitted`; - if (typeof mount.Source !== 'string' || !path.isAbsolute(mount.Source)) { + const source = pick(mount, 'Source'); + if (typeof source !== 'string' || !path.isAbsolute(source)) { return 'a bind mount needs an absolute Source'; } - const options = mount.BindOptions; + const options = pick(mount, 'BindOptions'); if (options !== undefined && options !== null) { if (!isPlainObject(options)) return 'BindOptions must be an object'; - const propagation = options.Propagation ?? ''; + const propagation = pick(options, 'Propagation') ?? ''; if (!PROPAGATIONS.has(propagation as string)) { return `mount propagation "${String(propagation)}" is not permitted`; } } - return { source: mount.Source, mode: mount.ReadOnly === true ? 'ro' : 'rw' }; + // Any casing that says read-only counts; a mount is rw only when none does. + const readOnly = valuesFor(mount, 'ReadOnly').some((v) => v === true); + return { source, mode: readOnly ? 'ro' : 'rw' }; } function collectMounts(hostConfig: Record): MountRequest[] | string { const requests: MountRequest[] = []; - const binds = hostConfig.Binds; - if (!isUnset(binds)) { + for (const binds of valuesFor(hostConfig, 'Binds')) { + if (isUnset(binds)) continue; if (!Array.isArray(binds)) return 'HostConfig.Binds must be an array'; for (const bind of binds) { if (typeof bind !== 'string') return 'each entry of HostConfig.Binds must be a string'; @@ -225,8 +326,8 @@ function collectMounts(hostConfig: Record): MountRequest[] | st requests.push(parsed); } } - const mounts = hostConfig.Mounts; - if (!isUnset(mounts)) { + for (const mounts of valuesFor(hostConfig, 'Mounts')) { + if (isUnset(mounts)) continue; if (!Array.isArray(mounts)) return 'HostConfig.Mounts must be an array'; for (const mount of mounts) { const parsed = parseMount(mount); @@ -244,7 +345,12 @@ function declaredMountPermits(declared: DockerMount, root: string, resolved: str return declared.mode === 'rw' || mode === 'ro'; } -function checkMounts(hostConfig: Record, ctx: DockerEvalContext, declared: DockerMount[]): DockerVerdict { +function checkMounts( + hostConfig: Record, + ctx: DockerEvalContext, + declared: DockerMount[], + resolutions?: Map +): DockerVerdict { const requests = collectMounts(hostConfig); if (typeof requests === 'string') return deny(requests); const realpath = ctx.realpath ?? ((p: string) => fs.realpathSync(p)); @@ -270,6 +376,7 @@ function checkMounts(hostConfig: Record, ctx: DockerEvalContext hints.mount(relative, mode) ); } + resolutions?.set(source, resolved); } return ALLOW; } @@ -278,6 +385,43 @@ function checkMounts(hostConfig: Record, ctx: DockerEvalContext // Actions // ----------------------------------------------------------------------------- +/** A copy of the create body with every bind source replaced by its resolved path. */ +function pinMountSources(body: Record, resolved: Map): unknown { + const pinned: Record = { ...body }; + for (const hostConfigKey of Object.keys(pinned)) { + if (hostConfigKey.toLowerCase() !== 'hostconfig') continue; + const hostConfig = pinned[hostConfigKey]; + if (!isPlainObject(hostConfig)) continue; + const copy: Record = { ...hostConfig }; + for (const key of Object.keys(copy)) { + const name = key.toLowerCase(); + if (name === 'binds' && Array.isArray(copy[key])) { + copy[key] = (copy[key] as unknown[]).map((bind) => { + if (typeof bind !== 'string') return bind; + const parts = bind.split(':'); + const target = resolved.get(parts[0]); + if (target === undefined) return bind; + return [target, ...parts.slice(1)].join(':'); + }); + } + if (name === 'mounts' && Array.isArray(copy[key])) { + copy[key] = (copy[key] as unknown[]).map((mount) => { + if (!isPlainObject(mount)) return mount; + const entry: Record = { ...mount }; + for (const mountKey of Object.keys(entry)) { + if (mountKey.toLowerCase() !== 'source') continue; + const source = entry[mountKey]; + if (typeof source === 'string' && resolved.has(source)) entry[mountKey] = resolved.get(source); + } + return entry; + }); + } + } + pinned[hostConfigKey] = copy; + } + return pinned; +} + function evaluateCreate(req: DockerRequest, ctx: DockerEvalContext, policy: DockerPolicy): DockerVerdict { const body = req.body; if (!isPlainObject(body)) return deny('container create requires a JSON object body'); @@ -286,12 +430,13 @@ function evaluateCreate(req: DockerRequest, ctx: DockerEvalContext, policy: Dock return deny('the repository docker policy declares no run action', image ? hints.image(image) : hints.run); } - const hostConfig = body.HostConfig ?? {}; + const hostConfig = pick(body, 'HostConfig') ?? {}; if (!isPlainObject(hostConfig)) return deny('HostConfig must be an object'); // Host-reaching settings first: none of these can be permitted by policy, // so the verdict does not depend on anything else in the request. - if (hostConfig.Privileged === true) { + const privilegedValues = valuesFor(hostConfig, 'Privileged'); + if (privilegedValues.some((v) => v === true)) { if (!policy.privileged) { return deny( 'privileged containers are not declared in the repository docker policy; `privileged: true` requires a managed VM backend', @@ -301,42 +446,73 @@ function evaluateCreate(req: DockerRequest, ctx: DockerEvalContext, policy: Dock if (!ctx.supportsPrivileged) { return deny('the repository docker policy declares privileged, which requires a managed VM backend; this daemon is not one'); } - } else if (!isUnset(hostConfig.Privileged) && hostConfig.Privileged !== false) { + } else if (privilegedValues.some((v) => !isUnset(v) && v !== false)) { return deny('HostConfig.Privileged must be a boolean'); } + for (const key of Object.keys(hostConfig)) { + if (!HOST_CONFIG_KNOWN.has(key.toLowerCase())) { + return deny( + `HostConfig.${key} is not a setting the localmost docker socket understands, so it cannot be forwarded` + ); + } + } + for (const gate of HOST_CONFIG_GATES) { - if (!gate.permitted(hostConfig[gate.key])) { + // Every casing must pass: one that does not is a value the daemon honours. + if (!valuesFor(hostConfig, gate.key).every((v) => gate.permitted(v))) { return deny(`${gate.flag} (HostConfig.${gate.key}) reaches the host and cannot be permitted by policy`); } } // Image. - if (typeof body.Image !== 'string' || body.Image === '') return deny('container create requires an Image'); - const wanted = normalizeImage(body.Image); - if (!(policy.run.images ?? []).some((declared) => normalizeImage(declared) === wanted)) { - return deny( - `image "${body.Image}" is not declared in the repository docker policy (run.images)`, - hints.image(body.Image) - ); + const imageValues = valuesFor(body, 'Image'); + const image = pick(body, 'Image'); + if (typeof image !== 'string' || image === '') return deny('container create requires an Image'); + // Every casing must name a declared image: the daemon uses one of them, and + // which one is not worth depending on. + for (const candidate of imageValues) { + if (typeof candidate !== 'string' || candidate === '') return deny('container create requires an Image'); + const wanted = normalizeImage(candidate); + if (!(policy.run.images ?? []).some((declared) => globMatches(normalizeImage(declared), wanted))) { + return deny( + `image "${candidate}" is not declared in the repository docker policy (run.images)`, + hints.image(candidate) + ); + } } // Network. Absent, empty and "default" are the daemon default, bridge. - const rawMode = hostConfig.NetworkMode; - let mode: string; - if (isUnset(rawMode) || rawMode === '' || rawMode === 'default') mode = 'bridge'; - else if (typeof rawMode === 'string') mode = rawMode; - else return deny('HostConfig.NetworkMode must be a string'); - if (mode === 'host' || mode.startsWith('container:')) { - return deny(`--network=${mode} (HostConfig.NetworkMode) reaches the host and cannot be permitted by policy`); - } - if (mode !== 'none' && mode !== policy.run.network) { + const modeValues = valuesFor(hostConfig, 'NetworkMode'); + const rawModes: unknown[] = modeValues.length > 0 ? modeValues : [undefined]; + let mode = 'bridge'; + for (const rawMode of rawModes) { + let candidate: string; + if (isUnset(rawMode) || rawMode === '' || rawMode === 'default') candidate = 'bridge'; + else if (typeof rawMode === 'string') candidate = rawMode; + else return deny('HostConfig.NetworkMode must be a string'); + if (candidate === 'host' || candidate.startsWith('container:')) { + return deny(`--network=${candidate} (HostConfig.NetworkMode) reaches the host and cannot be permitted by policy`); + } + // The most restrictive reading wins when casings disagree. + if (candidate !== 'bridge') mode = candidate; + } + // A network this job created is as good as the declared one: creating it was + // already checked against run.networks, and refusing to join it would make + // declaring one pointless. + if (mode !== 'none' && mode !== policy.run.network && !ctx.ownNetworkIds?.has(mode)) { return deny( `network mode "${mode}" is not declared in the repository docker policy (run.network)`, hints.network(mode) ); } - return checkMounts(hostConfig, ctx, policy.run.mounts ?? []); + // Pin every mount source to the path that was actually checked, so the + // daemon mounts what the filter judged rather than re-resolving a name the + // job can point somewhere else in between. + const resolutions = new Map(); + const verdict = checkMounts(hostConfig, ctx, policy.run.mounts ?? [], resolutions); + if (!verdict.allowed || resolutions.size === 0) return verdict; + return { allowed: true, rewrittenBody: pinMountSources(body, resolutions) }; } function evaluatePull(req: DockerRequest, policy: DockerPolicy): DockerVerdict { @@ -358,6 +534,139 @@ function evaluatePull(req: DockerRequest, policy: DockerPolicy): DockerVerdict { return ALLOW; } +/** + * Build query parameters the filter understands. + * + * An allowlist for the same reason HostConfig is one: `docker build` carries + * its whole configuration in the query string, so anything not enumerated is + * forwarded unexamined. `networkmode` is gated separately below, since it is + * the same host reach the run path already refuses. + */ +const BUILD_PARAMS_KNOWN: ReadonlySet = new Set([ + 't', 'dockerfile', 'q', 'nocache', 'rm', 'forcerm', 'pull', 'buildargs', 'labels', 'target', + 'shmsize', 'memory', 'memswap', 'cpushares', 'cpusetcpus', 'cpuperiod', 'cpuquota', 'squash', + 'platform', 'version', 'buildid', 'session', +]); + +/** Keys a network create may carry freely: they name the network or are inert. */ +const NETWORK_CREATE_KNOWN: ReadonlySet = new Set(['name', 'internal', 'checkduplicate', 'labels', 'driver']); + +/** Is an IPAM block the default one the CLI always sends, granting nothing? */ +const isDefaultIpam = (v: unknown): boolean => { + if (isUnset(v)) return true; + if (!isPlainObject(v)) return false; + const driver = pick(v, 'Driver'); + if (!isUnset(driver) && driver !== '' && driver !== 'default') return false; + return isEmptyObject(pick(v, 'Options')) && isEmptyArray(pick(v, 'Config')); +}; + +/** + * Keys the docker CLI sends on every `network create` with an inert value. + * + * Refusing them outright made the feature reachable only from a hand-written + * API client - the CLI sends all of these unconditionally. So they are gated by + * value, exactly as HostConfig gates the keys a plain `docker run` always + * sends: the default passes, anything meaningful is refused. + */ +const NETWORK_CREATE_GATES: ReadonlyArray<{ key: string; permitted: (v: unknown) => boolean; why: string }> = [ + { key: 'Scope', permitted: isEmptyString, why: 'a scope reaches beyond this daemon' }, + { key: 'IPAM', permitted: isDefaultIpam, why: 'an IPAM driver or subnet places the network on a chosen address range' }, + { key: 'Options', permitted: isEmptyObject, why: 'driver options can bind a bridge to a host address' }, + { key: 'Attachable', permitted: (v) => isUnset(v) || v === false, why: 'an attachable network can be joined from outside this job' }, + { key: 'Ingress', permitted: (v) => isUnset(v) || v === false, why: 'an ingress network is swarm routing mesh' }, + { key: 'ConfigOnly', permitted: (v) => isUnset(v) || v === false, why: 'a config-only network is a template for others' }, + { key: 'ConfigFrom', permitted: (v) => isUnset(v) || isEmptyObject(v), why: 'it copies configuration from another network' }, + { key: 'EnableIPv6', permitted: (v) => isUnset(v) || v === false, why: 'IPv6 is not part of what the grammar can describe' }, +]; + +/** + * An anchored glob. `*` matches any run of characters except `/`, and nothing + * else is special. + * + * Anchored so a declared name cannot be widened by a prefix: `vk-*` does not + * match `other-vk-abc`. Stopping at `/` for the same reason one level down - a + * glob that silently spans path separators reads as narrower than it is, so + * `vk/*:*` reaches one level under `vk` and no further, and each extra segment + * has to be asked for. A tag glob is unaffected, since a tag cannot contain a + * slash: `vk/grader:*` still covers a content-addressed tag. + * + * The tag is a second boundary, and it comes from normalisation rather than + * from here: a declaration with no tag normalises to `:latest`, so `vk/*` is + * matched as `vk/*:latest` and covers only latest. That reads as far wider + * than it is, so validation refuses a tagless glob and names `vk/*:*`; the + * behaviour is pinned by test rather than relied upon. + */ +function globMatches(pattern: string, value: string): boolean { + const escaped = pattern.replace(/[.*+?^${}()|[\]\\]/g, (c) => (c === '*' ? '\u0000' : `\\${c}`)); + return new RegExp(`^${escaped.split('\u0000').join('[^/]*')}$`).test(value); +} + +function evaluateNetworkCreate(req: DockerRequest, policy: DockerPolicy): DockerVerdict { + const declared: DockerNetworkPolicy[] = policy.run?.networks ?? []; + if (declared.length === 0) { + return deny( + 'the repository docker policy declares no networks', + hints.network_declaration('name-of-your-network', true) + ); + } + const body = req.body; + if (!isPlainObject(body)) return deny('network create requires a JSON body'); + + const gatedKeys = new Set(NETWORK_CREATE_GATES.map((g) => g.key.toLowerCase())); + for (const key of Object.keys(body)) { + const name = key.toLowerCase(); + if (NETWORK_CREATE_KNOWN.has(name) || gatedKeys.has(name)) continue; + return deny(`network create parameter "${key}" is not one the localmost docker socket understands`); + } + for (const gate of NETWORK_CREATE_GATES) { + // Every casing must pass: the daemon decodes these case-insensitively. + if (!valuesFor(body, gate.key).every((v) => gate.permitted(v))) { + return deny(`network ${gate.key} is not permitted: ${gate.why}`); + } + } + + // The filter creates a plain bridge or nothing. macvlan and ipvlan put a + // container on the physical LAN, which is worse than host networking. + for (const driver of valuesFor(body, 'Driver')) { + if (!isUnset(driver) && driver !== '' && driver !== 'bridge') { + return deny(`network driver "${String(driver)}" is not permitted; the localmost docker socket creates bridge networks only`); + } + } + + const names = valuesFor(body, 'Name'); + if (names.length === 0) return deny('network create requires a Name'); + const internalValues = valuesFor(body, 'Internal'); + const internal = internalValues.length > 0 && internalValues.every((v) => v === true); + + // Every casing must name a declared network, since which one the daemon uses + // is not worth depending on. + for (const name of names) { + if (typeof name !== 'string' || name === '') return deny('network create requires a Name'); + const match = declared.find((n) => globMatches(n.name, name)); + if (!match) { + return deny( + `network "${name}" is not declared in the repository docker policy (run.networks)`, + hints.network_declaration(name, internal) + ); + } + if (match.internal && !internal) { + return deny( + `network "${name}" is declared internal, so it cannot be created routable`, + hints.network_declaration(match.name, false) + ); + } + } + return ALLOW; +} + +/** Permit a per-network request only against a network this socket created. */ +function evaluateOwnNetwork(req: DockerRequest, ctx: DockerEvalContext): DockerVerdict { + const id = networkIdFrom(req); + if (!id) return deny(`${req.method} ${req.path} is not permitted through the localmost docker socket`); + if (ctx.ownNetworkIds?.has(id)) return ALLOW; + return deny(`network "${id}" was not created through this job's docker socket`); +} + function evaluateBuild(req: DockerRequest, policy: DockerPolicy): DockerVerdict { if (!policy.build) return deny('the repository docker policy declares no build action', hints.build); // The Engine API carries the context as a tar the client assembled from @@ -367,21 +676,47 @@ function evaluateBuild(req: DockerRequest, policy: DockerPolicy): DockerVerdict if (req.query.remote !== undefined) { return deny('a remote build context is not permitted; send the context with the request'); } + + for (const key of Object.keys(req.query)) { + const name = key.toLowerCase(); + if (name === 'networkmode') continue; + if (!BUILD_PARAMS_KNOWN.has(name)) { + return deny(`build parameter "${key}" is not one the localmost docker socket understands, so it cannot be forwarded`); + } + } + + // A build runs containers, and its network is chosen here rather than in a + // HostConfig - so the same rule the run path applies has to apply here too, + // or `docker build --network host` walks through a door create keeps shut. + const rawMode = req.query.networkmode ?? req.query.NetworkMode; + if (rawMode !== undefined && rawMode !== '' && rawMode !== 'default') { + if (rawMode === 'host' || rawMode.startsWith('container:')) { + return deny(`--network=${rawMode} on a build reaches the host and cannot be permitted by policy`); + } + if (rawMode !== 'none' && rawMode !== policy.run?.network) { + return deny( + `build network "${rawMode}" is not declared in the repository docker policy (run.network)`, + hints.network(rawMode) + ); + } + } + return ALLOW; } /** - * Permit a per-container request only against a container this socket created. - * The daemon accepts a unique id prefix, so a known id whose prefix was given - * counts as the same container; anything else is another job's, or the - * operator's, and is refused. + * Permit a per-container request only against a container this socket created, + * addressed by the id the daemon assigned or the name the job asked for. + * Anything else is another job's container, or the operator's, and is refused. */ function evaluateOwnContainer(req: DockerRequest, ctx: DockerEvalContext): DockerVerdict { const id = containerIdFrom(req); if (!id) return deny(`${req.method} ${req.path} is not permitted through the localmost docker socket`); - const own = ctx.ownContainerIds; - if (own && (own.has(id) || [...own].some((known) => known.startsWith(id)))) return ALLOW; + // Exact match only. A bare prefix used to count, on the reasoning that the + // daemon accepts one - but a prefix of a container this job has since + // removed can resolve on the shared daemon to somebody else's. + if (ctx.ownContainerIds?.has(id)) return ALLOW; return deny( `container "${id}" was not created through this job's docker socket; only this job's own containers can be addressed` @@ -392,6 +727,47 @@ function evaluateOwnContainer(req: DockerRequest, ctx: DockerEvalContext): Docke // Entry point // ----------------------------------------------------------------------------- +/** + * The first key in `value` that has a case-variant twin, named with its path. + * + * Measured against a real daemon rather than reasoned about: a create body + * carrying `HostConfig`, `hostconfig` and `HOSTCONFIG` came back with fields + * from ALL THREE - Go decodes each key into the same struct field in document + * order, so nested objects merge, while scalars and arrays inside one object + * are last-wins. There is no single value a filter can read and be right + * about: reading the first misses what the later ones added, and reading the + * last misses what the first one set. + * + * So the ambiguity is refused instead of modelled. Go's encoder emits unique, + * exactly-cased keys, so no real client sends a case-variant duplicate; a body + * that does is either a client the filter does not model or an attempt to be + * judged on one value and served another. + */ +function caseAmbiguity(value: unknown, at = 'the request body'): string | undefined { + if (Array.isArray(value)) { + for (const [i, item] of value.entries()) { + const found = caseAmbiguity(item, `${at}[${i}]`); + if (found) return found; + } + return undefined; + } + if (!isPlainObject(value)) return undefined; + const seen = new Map(); + for (const key of Object.keys(value)) { + const folded = key.toLowerCase(); + const first = seen.get(folded); + if (first !== undefined) { + return `${at} names both "${first}" and "${key}", which the daemon reads as the same key: it decodes them case-insensitively and merges or overwrites, so the value it would use is not the value this filter can read. Send each key once.`; + } + seen.set(folded, key); + } + for (const [key, child] of Object.entries(value)) { + const found = caseAmbiguity(child, `${at}.${key}`); + if (found) return found; + } + return undefined; +} + export function evaluateDockerRequest(req: DockerRequest, ctx: DockerEvalContext): DockerVerdict { const action = classifyDockerRequest(req); if (BASELINE.has(action)) return ALLOW; @@ -402,10 +778,17 @@ export function evaluateDockerRequest(req: DockerRequest, ctx: DockerEvalContext // A body the parser could not read is a request the filter cannot judge. if (req.bodyError) return deny(req.bodyError); + // Nor can it judge a body whose keys the daemon would read differently than + // it does. Checked once, here, so every action with a body is covered. + const ambiguous = caseAmbiguity(req.body); + if (ambiguous) return deny(ambiguous); + switch (action) { case 'create': return evaluateCreate(req, ctx, policy); case 'inspect': + case 'logs': + // Reads about the job's own container: the documented baseline, scoped. return evaluateOwnContainer(req, ctx); case 'list': // No policy key grants it: it would enumerate the whole daemon. @@ -416,12 +799,46 @@ export function evaluateDockerRequest(req: DockerRequest, ctx: DockerEvalContext case 'attach': case 'wait': case 'remove': + case 'kill': + case 'stop': if (!policy.run) return deny('the repository docker policy declares no run action', hints.run); return evaluateOwnContainer(req, ctx); case 'pull': return evaluatePull(req, policy); case 'build': return evaluateBuild(req, policy); + case 'image-inspect': { + // Scoped by the policy, not by a second ownership ledger: an inspect of + // an image run.images already names discloses nothing the policy has not + // granted, and the container ledger has already produced one defect. + if (!policy.run) return deny('the repository docker policy declares no run action', hints.run); + const ref = imageRefFrom(req); + if (!ref) return deny(`${req.method} ${req.path} is not permitted through the localmost docker socket`); + const wanted = normalizeImage(ref); + if (!(policy.run.images ?? []).some((declared) => globMatches(normalizeImage(declared), wanted))) { + return deny( + `image "${ref}" is not declared in the repository docker policy (run.images)`, + hints.image(ref) + ); + } + return ALLOW; + } + case 'network-create': + return evaluateNetworkCreate(req, policy); + case 'network-inspect': + case 'network-remove': + return evaluateOwnNetwork(req, ctx); + case 'buildkit': + return deny( + 'BuildKit builds cannot be filtered: the build streams over a gRPC session that exports host ' + + 'filesystem access to the daemon, so no request carries the paths it reads. Jobs are pinned to ' + + 'the classic builder with DOCKER_BUILDKIT=0, which `build:` policy does describe - seeing this ' + + 'means something set DOCKER_BUILDKIT back on.' + ); + case 'network-list': + return deny( + 'listing networks is not permitted through the localmost docker socket; it would enumerate networks outside this job' + ); default: return deny(`${req.method} ${req.path} is not permitted through the localmost docker socket`); } diff --git a/src/main/docker/docker-filter-proxy.test.ts b/src/main/docker/docker-filter-proxy.test.ts index 5d8a630..8475556 100644 --- a/src/main/docker/docker-filter-proxy.test.ts +++ b/src/main/docker/docker-filter-proxy.test.ts @@ -610,3 +610,224 @@ describe('container ownership tracking', () => { expect((await request(sock, 'GET', '/v1.45/containers/theirs999/json')).status).toBe(403); }); }); + +const rawUpgrade = (sock: string, method: string, url: string): Promise<{ head: string; socket: net.Socket }> => + new Promise((resolve, reject) => { + const socket = net.connect(sock); + let buffered = ''; + const onData = (data: Buffer) => { + buffered += data.toString(); + const end = buffered.indexOf('\r\n\r\n'); + if (end === -1) return; + socket.off('data', onData); + resolve({ head: buffered.slice(0, end), socket }); + }; + socket.on('data', onData); + socket.on('error', reject); + socket.on('connect', () => { + socket.write(`${method} ${url} HTTP/1.1\r\nHost: docker\r\nConnection: Upgrade\r\nUpgrade: tcp\r\n\r\n`); + }); + }); + +describe('upgrade requests', () => { + it('does not turn a permitted baseline read into a raw daemon tunnel', async () => { + const dir = tmp(); + const daemon = await fakeDaemon(dir); + const { proxy, sock } = await startProxy(dir, { backend: backendWith(daemon.sock, dir) }); + proxy.bind('owner/repo', { run: { images: ['postgres:16'], network: 'bridge' } }); + + // GET /_ping is in the always-on baseline, so the policy permits it. If an + // Upgrade header alone opens a raw pipe, the job holds an unfiltered socket + // to the daemon and can pipeline anything over it. + const { head, socket } = await rawUpgrade(sock, 'GET', '/v1.45/_ping'); + expect(head).not.toMatch(/101/); + + // Prove no tunnel: a denied request written on the same socket must not be + // answered by the daemon. + const smuggled = await new Promise((resolve) => { + let got = ''; + socket.on('data', (d: Buffer) => { got += d.toString(); }); + socket.write('GET /v1.45/containers/json HTTP/1.1\r\nHost: docker\r\n\r\n'); + setTimeout(() => resolve(got), 300); + }); + socket.destroy(); + expect(smuggled).not.toMatch(/"ok"\s*:\s*true|Names|\[\s*\{/); + }); + + it('refuses an upgrade on a container the socket does not own', async () => { + const dir = tmp(); + const daemon = await fakeAttachDaemon(dir); + const { proxy, sock } = await startProxy(dir, { backend: backendWith(daemon.sock, dir) }); + proxy.bind('owner/repo', { run: { images: ['postgres:16'], network: 'bridge' } }); + + const { head, socket } = await attach(sock, '/v1.45/containers/theirs999/attach?stream=1'); + socket.destroy(); + expect(head).not.toMatch(/101/); + }); +}); + +describe('mount sources are pinned before forwarding', () => { + it('sends the daemon the resolved path, so a swapped symlink cannot change what is mounted', async () => { + const dir = tmp(); + const workspace = fs.realpathSync.native(dir); + const real = path.join(workspace, 'inside'); + const link = path.join(workspace, 'link'); + fs.mkdirSync(real); + fs.symlinkSync(real, link); + + const daemon = await fakeDaemon(dir); + const { proxy, sock } = await startProxy(dir, { + backend: { name: 'test', supportsPrivileged: false, resolveEndpoint: () => ({ socketPath: daemon.sock }), workspaceMountRoot: () => workspace }, + }); + proxy.bind('owner/repo', { run: { images: ['postgres:16'], mounts: [{ path: './', mode: 'rw' }], network: 'bridge' } }); + + const reply = await request(sock, 'POST', '/v1.45/containers/create', { + Image: 'postgres:16', + HostConfig: { Binds: [`${link}:/ws`] }, + }); + expect(reply.status).toBe(201); + + // The filter resolved `link` to decide. If it forwards the spelling it was + // given, the daemon resolves it again at mount time and the job can swap + // the symlink in between. + const create = daemon.seen.find((s) => s.url.includes('/containers/create'))!; + const binds = (JSON.parse(create.body.toString()) as { HostConfig: { Binds: string[] } }).HostConfig.Binds; + expect(binds[0]).toBe(`${real}:/ws`); + }); +}); + +describe('a request target the filter cannot read', () => { + it('is refused, and the connection does not hang', async () => { + const dir = tmp(); + const daemon = await fakeDaemon(dir); + const { proxy, sock } = await startProxy(dir, { backend: backendWith(daemon.sock, dir) }); + proxy.bind('owner/repo', { run: { images: ['postgres:16'], network: 'bridge' } }); + + // Resolve on the response head, not on close: HTTP/1.1 keep-alive means a + // correctly-answered request leaves the socket open. + const answered = await new Promise((resolve, reject) => { + let buffered = ''; + const client = net.connect(sock); + const done = (v: string) => { clearTimeout(timer); client.destroy(); resolve(v); }; + const timer = setTimeout(() => { client.destroy(); reject(new Error('no answer within 3s: the connection hung')); }, 3000); + client.on('connect', () => client.write('GET //evil/v1.45/containers/json HTTP/1.1\r\nHost: docker\r\n\r\n')); + client.on('data', (c: Buffer) => { buffered += c.toString(); if (buffered.includes('\r\n\r\n')) done(buffered); }); + client.on('error', (e) => { clearTimeout(timer); reject(e); }); + client.on('close', () => { clearTimeout(timer); resolve(buffered); }); + }); + + // 400, naming the target: a target the filter cannot read is a bad + // request, not a policy denial, and saying so is the difference between + // "fix your URL" and "ask your operator for a grant". + expect(answered).toMatch(/^HTTP\/1\.[01] 400/); + expect(answered).toMatch(/origin-form|could not be parsed/); + // Nothing reached the daemon. + expect(daemon.seen).toHaveLength(0); + expect(proxy.isRunning()).toBe(true); + }); +}); + +describe('which containers a job may address', () => { + const setup = async () => { + const dir = tmp(); + const daemon = await fakeDaemon(dir); + const { proxy, sock } = await startProxy(dir, { backend: backendWith(daemon.sock, dir) }); + proxy.bind('owner/repo', { run: { images: ['postgres:16'], network: 'bridge' } }); + return { sock, daemon }; + }; + + it('lets a job address the container it created by --name', async () => { + const { sock } = await setup(); + // docker run --name mine ... -> POST /containers/create?name=mine, and + // every later call addresses it as "mine", never as the id. + expect((await request(sock, 'POST', '/v1.45/containers/create?name=mine', { Image: 'postgres:16' })).status).toBe(201); + expect((await request(sock, 'POST', '/v1.45/containers/mine/start')).status).toBeLessThan(400); + expect((await request(sock, 'GET', '/v1.45/containers/mine/json')).status).toBeLessThan(400); + }); + + it('forgets a container once it is removed, so its name cannot be reused', async () => { + const { sock } = await setup(); + await request(sock, 'POST', '/v1.45/containers/create?name=mine', { Image: 'postgres:16' }); + expect((await request(sock, 'DELETE', '/v1.45/containers/mine')).status).toBeLessThan(400); + // The container is gone; the daemon may hand that name to anyone next. + expect((await request(sock, 'GET', '/v1.45/containers/mine/json')).status).toBe(403); + expect((await request(sock, 'GET', '/v1.45/containers/abc123/json')).status).toBe(403); + }); + + it('does not accept a bare prefix of an owned id', async () => { + const { sock } = await setup(); + // The fake daemon answers create with Id abc123. A prefix could resolve on + // the real daemon to a container this job never created. + expect((await request(sock, 'POST', '/v1.45/containers/create', { Image: 'postgres:16' })).status).toBe(201); + expect((await request(sock, 'GET', '/v1.45/containers/abc123/json')).status).toBeLessThan(400); + expect((await request(sock, 'GET', '/v1.45/containers/ab/json')).status).toBe(403); + }); +}); + +describe('networks a job creates', () => { + it('may be read, joined and deleted, and are forgotten once removed', async () => { + const dir = tmp(); + const daemon = await networkDaemon(dir); + const { proxy, sock } = await startProxy(dir, { backend: backendWith(daemon.sock, dir) }); + proxy.bind('owner/repo', { + run: { images: ['alpine:3'], network: 'bridge', networks: [{ name: 'vk-*', internal: true }] }, + }); + + expect((await request(sock, 'POST', '/v1.45/networks/create', { Name: 'vk-1', Internal: true })).status).toBe(201); + + // Both the id the daemon assigned and the name the job asked for. + expect((await request(sock, 'GET', '/v1.45/networks/net123')).status).toBeLessThan(400); + expect((await request(sock, 'GET', '/v1.45/networks/vk-1')).status).toBeLessThan(400); + // A container may join it. + expect((await request(sock, 'POST', '/v1.45/containers/create', { Image: 'alpine:3', HostConfig: { NetworkMode: 'vk-1' } })).status).toBe(201); + // Someone else's network is still refused. + expect((await request(sock, 'GET', '/v1.45/networks/theirs')).status).toBe(403); + + expect((await request(sock, 'DELETE', '/v1.45/networks/vk-1')).status).toBeLessThan(400); + expect((await request(sock, 'GET', '/v1.45/networks/vk-1')).status).toBe(403); + expect((await request(sock, 'GET', '/v1.45/networks/net123')).status).toBe(403); + }); + + it('are recorded under the name whatever casing the client spelled the key with', async () => { + // The daemon decodes `name` into the same field as `Name`, so it creates + // the network either way, and the evaluator already judges either way. + // Reading only `Name` here left the network created but unaddressable: the + // job could not join, inspect or delete what it had just made. + const dir = tmp(); + const daemon = await networkDaemon(dir); + const { proxy, sock } = await startProxy(dir, { backend: backendWith(daemon.sock, dir) }); + proxy.bind('owner/repo', { + run: { images: ['alpine:3'], network: 'bridge', networks: [{ name: 'vk-*', internal: true }] }, + }); + + expect((await request(sock, 'POST', '/v1.45/networks/create', { name: 'vk-1', internal: true })).status).toBe(201); + + expect((await request(sock, 'GET', '/v1.45/networks/vk-1')).status).toBeLessThan(400); + expect((await request(sock, 'POST', '/v1.45/containers/create', { Image: 'alpine:3', HostConfig: { NetworkMode: 'vk-1' } })).status).toBe(201); + }); +}); + +/** A fake daemon that also answers network create. */ +const networkDaemon = (dir: string): Promise<{ sock: string }> => + new Promise((resolve) => { + const sock = path.join(dir, 'netd.sock'); + const server = http.createServer((req, res) => { + const chunks: Buffer[] = []; + req.on('data', (c: Buffer) => chunks.push(c)); + req.on('end', () => { + const p = req.url!.replace(/^\/v\d+\.\d+/, '').split('?')[0]; + if (p === '/networks/create') { + res.writeHead(201, { 'Content-Type': 'application/json' }); + res.end(JSON.stringify({ Id: 'net123', Warning: '' })); + } else if (p === '/containers/create') { + res.writeHead(201, { 'Content-Type': 'application/json' }); + res.end(JSON.stringify({ Id: 'abc123', Warnings: [] })); + } else { + res.writeHead(200, { 'Content-Type': 'application/json' }); + res.end(JSON.stringify({ ok: true })); + } + }); + }); + servers.push(server); + server.listen(sock, () => resolve({ sock })); + }); diff --git a/src/main/docker/docker-filter-proxy.ts b/src/main/docker/docker-filter-proxy.ts index 37c1151..6e81455 100644 --- a/src/main/docker/docker-filter-proxy.ts +++ b/src/main/docker/docker-filter-proxy.ts @@ -17,7 +17,7 @@ import * as net from 'net'; import * as path from 'path'; import { DockerPolicy } from '../../shared/docker-policy'; import { DockerBackend } from './docker-backend'; -import { DockerRequest, classifyDockerRequest, parseDockerRequest } from './docker-request'; +import { DockerRequest, classifyDockerRequest, containerIdFrom, networkIdFrom, parseDockerRequest } from './docker-request'; import { evaluateDockerRequest, registryOf } from './docker-evaluator'; export interface DockerFilterProxyLogEntry { @@ -49,6 +49,9 @@ const MAX_JSON_BODY_BYTES = 1024 * 1024; /** How much of an upload is drained so an early answer reaches the client, before the connection is cut. */ const MAX_DRAIN_BYTES = 8 * MAX_JSON_BODY_BYTES; +/** A response head larger than this is not an upgrade handshake. */ +const MAX_UPGRADE_HEAD_BYTES = 64 * 1024; + /** Hop-by-hop headers: each leg of the relay decides these for itself. */ const HOP_BY_HOP = ['connection', 'keep-alive', 'proxy-connection']; @@ -79,6 +82,24 @@ function flattenHeaders(headers: http.IncomingHttpHeaders): Record, name: string): unknown => { + const wanted = name.toLowerCase(); + for (const [key, value] of Object.entries(obj)) if (key.toLowerCase() === wanted) return value; + return undefined; +}; + +const isPlainRecord = (v: unknown): v is Record => + typeof v === 'object' && v !== null && !Array.isArray(v); + const isJsonContentType = (contentType: string | undefined): boolean => contentType !== undefined && contentType.split(';')[0].trim().toLowerCase() === 'application/json'; @@ -93,6 +114,11 @@ export class DockerFilterProxy { * job's container by naming its id. */ private readonly ownContainerIds = new Set(); + /** Each identifier this socket may address, mapped to the container it names. */ + private readonly ownContainerAliases = new Map(); + /** Networks created through this socket, by id and by the name the job asked for. */ + private readonly ownNetworkIds = new Set(); + private readonly ownNetworkAliases = new Map(); private repository: string | undefined; private readonly backend: DockerBackend; private readonly onLog: (entry: DockerFilterProxyLogEntry) => void; @@ -123,6 +149,40 @@ export class DockerFilterProxy { this.realpath = options.realpath ?? ((p) => fs.realpathSync(p)); } + /** Record an identifier the job may use for a container it created. */ + private own(alias: string, containerId: string): void { + this.ownContainerAliases.set(alias, containerId); + this.ownContainerIds.add(alias); + } + + /** Record an identifier for a network the job created. */ + private ownNetwork(alias: string, networkId: string): void { + this.ownNetworkAliases.set(alias, networkId); + this.ownNetworkIds.add(alias); + } + + /** Forget every identifier for a network the job has removed. */ + private disownNetwork(alias: string): void { + const networkId = this.ownNetworkAliases.get(alias); + if (networkId === undefined) return; + for (const [known, owner] of [...this.ownNetworkAliases]) { + if (owner !== networkId) continue; + this.ownNetworkAliases.delete(known); + this.ownNetworkIds.delete(known); + } + } + + /** Forget every identifier for a container the job has removed. */ + private disown(alias: string): void { + const containerId = this.ownContainerAliases.get(alias); + if (containerId === undefined) return; + for (const [known, owner] of [...this.ownContainerAliases]) { + if (owner !== containerId) continue; + this.ownContainerAliases.delete(known); + this.ownContainerIds.delete(known); + } + } + /** * Bind the socket to a repository and its policy. Until this is called the * socket denies everything but the baseline; the caller binds only once @@ -203,7 +263,7 @@ export class DockerFilterProxy { /** The workspace the backend roots mounts at, resolved so symlinked sandbox dirs compare equal. */ private workspaceRoot(): string { - const root = this.backend.workspaceMountRoot(path.dirname(this.socketPath ?? '')); + const root = this.backend.workspaceMountRoot(path.dirname(this.socketPath ?? ''), this.repository); try { return this.realpath(root); } catch { @@ -255,7 +315,14 @@ export class DockerFilterProxy { } /** Null when the request may proceed; otherwise the status and message that refuse it. */ - private decide(req: DockerRequest): { status: number; message: string } | null { + private decide(req: DockerRequest): { refusal: { status: number; message: string } | null; rewrittenBody?: unknown } { + // A target the parser could not read is a request the filter cannot judge. + // Before this, the parse threw out of the request handler: no refusal was + // written and the connection sat open until the client gave up. + if (req.targetError) { + this.onLog({ level: 'info', message: `refused ${req.method} ${req.raw.url}: ${req.targetError}` }); + return { refusal: { status: 400, message: req.targetError } }; + } if (req.apiVersion) { const version = parseApiVersion(req.apiVersion); if ( @@ -266,7 +333,7 @@ export class DockerFilterProxy { `API version ${req.apiVersion} is not supported by the localmost docker socket ` + `(supported: v${bareVersion(this.minApiVersion)} to v${bareVersion(this.maxApiVersion)})`; this.onLog({ level: 'info', message: `refused ${req.method} ${req.path}: ${message}` }); - return { status: 400, message }; + return { refusal: { status: 400, message } }; } } @@ -275,6 +342,7 @@ export class DockerFilterProxy { workspaceRoot: this.workspaceRoot(), supportsPrivileged: this.backend.supportsPrivileged, ownContainerIds: this.ownContainerIds, + ownNetworkIds: this.ownNetworkIds, realpath: this.realpath, }); if (!verdict.allowed) { @@ -284,9 +352,9 @@ export class DockerFilterProxy { message: `denied ${req.method} ${req.path}: ${message}`, ...(verdict.policyHint !== undefined ? { policyHint: verdict.policyHint } : {}), }); - return { status: 403, message }; + return { refusal: { status: 403, message } }; } - return null; + return { refusal: null, rewrittenBody: verdict.rewrittenBody }; } /** Said once per socket: a declaration is a permission, not a requirement. */ @@ -348,13 +416,17 @@ export class DockerFilterProxy { res: http.ServerResponse, bufferedBody: Buffer | null ): void { - const refusal = this.decide(parsed); + const { refusal, rewrittenBody } = this.decide(parsed); if (refusal) { this.writeRefusal(res, refusal.status, refusal.message); if (bufferedBody === null) this.endAfterDrain(req, res); else res.end(); return; } + // The verdict may pin the body it approved - mount sources resolved to the + // paths actually checked - so the daemon mounts what the filter judged + // rather than re-resolving a name the job can repoint in between. + const body = rewrittenBody !== undefined ? Buffer.from(JSON.stringify(rewrittenBody)) : bufferedBody; const endpoint = this.backend.resolveEndpoint(); if (!endpoint) { this.warnNoDaemon(); @@ -363,7 +435,7 @@ export class DockerFilterProxy { else res.end(); return; } - this.forward(parsed, req, res, bufferedBody, endpoint.socketPath); + this.forward(parsed, req, res, body, endpoint.socketPath); } /** The URL as forwarded: an unversioned request is pinned to the version we understand. */ @@ -414,14 +486,27 @@ export class DockerFilterProxy { { socketPath, path: this.forwardedUrl(parsed), method: parsed.method, headers, agent: this.upstreamAgent }, (upstreamRes) => { upstreamRes.on('error', () => res.destroy()); + // A container the daemon actually removed is no longer this job's to + // address; its name in particular may be handed to anyone next. + const removedStatus = upstreamRes.statusCode ?? 502; + if (action === 'remove' && removedStatus >= 200 && removedStatus < 300) { + const addressed = containerIdFrom(parsed); + if (addressed) this.disown(addressed); + } + if (action === 'network-remove' && removedStatus >= 200 && removedStatus < 300) { + const addressed = networkIdFrom(parsed); + if (addressed) this.disownNetwork(addressed); + } const relayed = action === 'ping' ? this.relayPing(upstreamRes, res) : action === 'version' ? this.relayVersion(upstreamRes, res) : action === 'create' - ? this.relayCreate(upstreamRes, res) - : this.relay(upstreamRes, res); + ? this.relayCreate(upstreamRes, res, parsed) + : action === 'network-create' + ? this.relayNetworkCreate(upstreamRes, res, parsed) + : this.relay(upstreamRes, res); relayed.then(() => { answered = true; if (bufferedBody !== null) { @@ -499,7 +584,38 @@ export class DockerFilterProxy { * to containers this job actually created. The body is small and the client * needs the id before it can proceed, so buffering it costs nothing. */ - private relayCreate(upstreamRes: http.IncomingMessage, res: http.ServerResponse): Promise { + /** Relay a network create and record the network, by id and by requested name. */ + private relayNetworkCreate(upstreamRes: http.IncomingMessage, res: http.ServerResponse, requested: DockerRequest): Promise { + return new Promise((resolve) => { + const chunks: Buffer[] = []; + upstreamRes.on('data', (c: Buffer) => chunks.push(c)); + upstreamRes.on('end', () => { + const raw = Buffer.concat(chunks); + const status = upstreamRes.statusCode ?? 502; + if (status >= 200 && status < 300) { + try { + const parsed = JSON.parse(raw.toString('utf8')) as Record; + if (typeof parsed.Id === 'string' && parsed.Id.length > 0) { + this.ownNetwork(parsed.Id, parsed.Id); + const body = requested.body; + const name = isPlainRecord(body) ? readFolded(body, 'Name') : undefined; + if (typeof name === 'string' && name.length > 0) this.ownNetwork(name, parsed.Id); + } + } catch { + // An unreadable create response leaves the network unowned, which + // fails closed: the job cannot address what it cannot name. + } + } + const headers = { ...this.relayedHeaders(upstreamRes), 'content-length': String(raw.length) }; + delete headers['transfer-encoding']; + res.writeHead(status, headers); + if (raw.length > 0) res.write(raw); + resolve(); + }); + }); + } + + private relayCreate(upstreamRes: http.IncomingMessage, res: http.ServerResponse, requested: DockerRequest): Promise { return new Promise((resolve) => { const chunks: Buffer[] = []; upstreamRes.on('data', (c: Buffer) => chunks.push(c)); @@ -510,7 +626,14 @@ export class DockerFilterProxy { if (status >= 200 && status < 300) { try { const parsed = JSON.parse(raw.toString('utf8')) as Record; - if (typeof parsed.Id === 'string' && parsed.Id.length > 0) this.ownContainerIds.add(parsed.Id); + if (typeof parsed.Id === 'string' && parsed.Id.length > 0) { + // A job addresses its container by whichever identifier it + // knows: the id the daemon just assigned, or the --name it + // asked for, which is the only one it ever sees when it uses one. + this.own(parsed.Id, parsed.Id); + const name = requested.query.name; + if (name) this.own(name, parsed.Id); + } } catch { // An unreadable create response leaves the container unowned: the // job cannot address it, which fails closed rather than open. @@ -582,7 +705,19 @@ export class DockerFilterProxy { const url = req.url ?? '/'; const parsed = parseDockerRequest({ method, url, headers, body: Buffer.alloc(0) }); - const refusal = this.decide(parsed); + // Only attach is an upgrade. Without this, any request the policy permits + // - including a baseline /_ping - could be sent with an Upgrade header to + // open a raw pipe to the daemon, and everything pipelined over that pipe + // would bypass the filter entirely. + const action = classifyDockerRequest(parsed); + if (action !== 'attach') { + const message = `${parsed.method} ${parsed.path} cannot be upgraded through the localmost docker socket`; + this.onLog({ level: 'info', message: `denied upgrade ${parsed.method} ${parsed.path}: ${message}` }); + this.refuseRaw(client, 400, message); + return; + } + + const { refusal } = this.decide(parsed); if (refusal) { this.refuseRaw(client, refusal.status, refusal.message); return; @@ -605,8 +740,38 @@ export class DockerFilterProxy { } upstream.write(lines.join('\r\n') + '\r\n\r\n'); if (head.length > 0) upstream.write(head); - upstream.pipe(client); - client.pipe(upstream); + + // Pipe only once the daemon has actually agreed to upgrade. Piping on + // connect would hand the job a raw socket even when the daemon answered + // with an ordinary response, which is a tunnel by another name. + let banner = ''; + const onUpstreamHead = (chunk: Buffer): void => { + banner += chunk.toString('latin1'); + const end = banner.indexOf('\r\n\r\n'); + if (end === -1) { + // A daemon that never finishes a response head is not upgrading. + if (banner.length > MAX_UPGRADE_HEAD_BYTES) { + upstream.destroy(); + client.destroy(); + } + return; + } + upstream.off('data', onUpstreamHead); + + const statusLine = banner.slice(0, banner.indexOf('\r\n')); + if (!/^HTTP\/1\.[01] 101\b/.test(statusLine)) { + // Relay what the daemon said, then close. No raw pipe is established. + client.write(Buffer.from(banner, 'latin1')); + client.end(); + upstream.destroy(); + return; + } + + client.write(Buffer.from(banner, 'latin1')); + upstream.pipe(client); + client.pipe(upstream); + }; + upstream.on('data', onUpstreamHead); }); this.connections.add(upstream); upstream.on('close', () => this.connections.delete(upstream)); diff --git a/src/main/docker/docker-request.test.ts b/src/main/docker/docker-request.test.ts index d378683..b595473 100644 --- a/src/main/docker/docker-request.test.ts +++ b/src/main/docker/docker-request.test.ts @@ -1,5 +1,5 @@ import { describe, it, expect } from '@jest/globals'; -import { parseDockerRequest, classifyDockerRequest, DockerAction } from './docker-request'; +import { parseDockerRequest, classifyDockerRequest, containerIdFrom, DockerAction } from './docker-request'; const mk = (method: string, url: string, headers: Record = {}, body = Buffer.alloc(0)) => parseDockerRequest({ method, url, headers, body }); @@ -87,7 +87,10 @@ describe('classifyDockerRequest', () => { it('classifies everything outside the map as other', () => { const others: Array<[string, string]> = [ - ['POST', '/v1.45/networks/create'], + // /networks/create is a mapped action now; these are not. + ['PUT', '/v1.45/networks/create'], + ['POST', '/v1.45/networks/net123'], + ['POST', '/v1.45/volumes/create'], ['POST', '/v1.45/containers/abc123/exec'], ['GET', '/v1.45/build'], ['GET', '/v1.45/containers/create'], @@ -103,3 +106,47 @@ describe('classifyDockerRequest', () => { } }); }); + +describe('container lifecycle endpoints the run action covers', () => { + const mk = (m: string, u: string) => parseDockerRequest({ method: m, url: u, headers: {}, body: Buffer.alloc(0) }); + + it.each([ + ['POST', '/v1.45/containers/abc/kill', 'kill'], + ['POST', '/v1.45/containers/abc/stop', 'stop'], + ['GET', '/v1.45/containers/abc/logs?stdout=1', 'logs'], + ])('maps %s %s to %s', (method, url, action) => { + expect(classifyDockerRequest(mk(method, url))).toBe(action); + }); + + it('extracts the container id from each of them, so they can be scoped', () => { + for (const [m, u] of [['POST', '/v1.45/containers/abc/kill'], ['POST', '/v1.45/containers/abc/stop'], ['GET', '/v1.45/containers/abc/logs']] as const) { + expect(containerIdFrom(mk(m, u))).toBe('abc'); + } + }); +}); + +describe('request targets that are not plain origin-form paths', () => { + const parse = (url: string) => parseDockerRequest({ method: 'GET', url, headers: {}, body: Buffer.alloc(0) }); + + it('does not throw on a target the URL parser rejects', () => { + for (const url of ['//', 'http://[', 'http://user@[::1]:99999/x']) { + expect(() => parse(url)).not.toThrow(); + expect(parse(url).targetError).toBeTruthy(); + } + }); + + it('refuses a target carrying an authority, which the filter and the daemon would read differently', () => { + // `//evil/x` parses to host=evil, path=/x here, while the daemon reads the + // request target as written. Judging one and forwarding the other is how a + // filter gets talked past. + expect(parse('//evil/v1.45/containers/json').targetError).toBeTruthy(); + expect(parse('http://evil/v1.45/_ping').targetError).toBeTruthy(); + }); + + it('leaves an ordinary path alone', () => { + const req = parse('/v1.45/containers/json?all=1'); + expect(req.targetError).toBeUndefined(); + expect(req.path).toBe('/containers/json'); + expect(req.query.all).toBe('1'); + }); +}); diff --git a/src/main/docker/docker-request.ts b/src/main/docker/docker-request.ts index 3cb8f74..d9837d3 100644 --- a/src/main/docker/docker-request.ts +++ b/src/main/docker/docker-request.ts @@ -25,6 +25,15 @@ export type DockerAction = | 'attach' | 'wait' | 'remove' + | 'kill' + | 'stop' + | 'logs' + | 'network-create' + | 'network-inspect' + | 'network-remove' + | 'network-list' + | 'image-inspect' + | 'buildkit' | 'build' | 'other'; @@ -41,6 +50,14 @@ export interface DockerRequest { body?: unknown; /** Set when the content type promised JSON and the body did not parse. */ bodyError?: string; + /** + * Set when the request target is not a plain origin-form path. The filter + * refuses these rather than guessing: a target carrying an authority + * (`//evil/x`, `http://evil/x`) is read one way by the URL parser here and + * another by the daemon, and judging one while forwarding the other is how a + * filter gets talked past. + */ + targetError?: string; raw: { method: string; url: string; headers: Record; body: Buffer }; } @@ -58,9 +75,27 @@ const isJsonContentType = (contentType: string | undefined): boolean => contentType !== undefined && contentType.split(';')[0].trim().toLowerCase() === 'application/json'; export function parseDockerRequest(raw: DockerRequest['raw']): DockerRequest { + // Only origin-form is accepted. Anything else either throws here (`//`, + // `http://[`) or parses to a different path than the daemon will read, and + // both are refusals rather than guesses. + if (!raw.url.startsWith('/') || raw.url.startsWith('//')) { + return { + method: raw.method, path: raw.url, query: {}, raw, + targetError: `request target "${raw.url}" is not a plain path; the localmost docker socket accepts origin-form targets only`, + }; + } + // The base is a placeholder so a path-only URL parses; only pathname and // search are read from the result. - const url = new URL(raw.url, 'http://docker'); + let url: URL; + try { + url = new URL(raw.url, 'http://docker'); + } catch { + return { + method: raw.method, path: raw.url, query: {}, raw, + targetError: `request target "${raw.url}" could not be parsed`, + }; + } let path = url.pathname; let apiVersion: string | undefined; @@ -103,20 +138,62 @@ const ENDPOINTS: ReadonlyArray<{ method: string; path: RegExp; action: DockerAct // container on the daemon, including other jobs'. { method: 'GET', path: /^\/containers\/json$/, action: 'list' }, { method: 'POST', path: /^\/images\/create$/, action: 'pull' }, + // The reference may carry a registry, a path and a tag, so it is anything up + // to the trailing /json. Listing is deliberately absent: it is daemon-wide. + { method: 'GET', path: /^\/images\/(?!json$).+\/json$/, action: 'image-inspect' }, { method: 'POST', path: /^\/containers\/create$/, action: 'create' }, { method: 'POST', path: new RegExp(`^/containers/${ID}/start$`), action: 'start' }, { method: 'POST', path: new RegExp(`^/containers/${ID}/attach$`), action: 'attach' }, { method: 'POST', path: new RegExp(`^/containers/${ID}/wait$`), action: 'wait' }, + { method: 'POST', path: new RegExp(`^/containers/${ID}/kill$`), action: 'kill' }, + { method: 'POST', path: new RegExp(`^/containers/${ID}/stop$`), action: 'stop' }, + // A read about the job's own container, like inspect. + { method: 'GET', path: new RegExp(`^/containers/${ID}/logs$`), action: 'logs' }, { method: 'DELETE', path: new RegExp(`^/containers/${ID}$`), action: 'remove' }, + { method: 'POST', path: /^\/networks\/create$/, action: 'network-create' }, + { method: 'GET', path: new RegExp(`^/networks/${ID}$`), action: 'network-inspect' }, + { method: 'DELETE', path: new RegExp(`^/networks/${ID}$`), action: 'network-remove' }, + // Listing enumerates the daemon, like the container list; no key grants it. + { method: 'GET', path: /^\/networks$/, action: 'network-list' }, { method: 'POST', path: /^\/build$/, action: 'build' }, + // BuildKit's session and stream. Named so the refusal can say why, rather + // than falling through to "unknown endpoint". + { method: 'POST', path: /^\/grpc$/, action: 'buildkit' }, + { method: 'POST', path: /^\/session$/, action: 'buildkit' }, ]; +/** The image reference an inspect addresses, decoded, or undefined. */ +export function imageRefFrom(req: DockerRequest): string | undefined { + const match = /^\/images\/(.+)\/json$/.exec(req.path); + if (!match) return undefined; + try { + return decodeURIComponent(match[1]); + } catch { + return match[1]; + } +} + +/** Per-network endpoints, for scoping to networks this socket created. */ +const NETWORK_ID_PATHS: ReadonlyArray = [new RegExp(`^/networks/(${ID})$`)]; + +/** The network a request addresses, or undefined when it addresses none. */ +export function networkIdFrom(req: DockerRequest): string | undefined { + for (const pattern of NETWORK_ID_PATHS) { + const match = pattern.exec(req.path); + if (match) return match[1]; + } + return undefined; +} + /** Per-container endpoints, for scoping an action to the containers this socket created. */ const CONTAINER_ID_PATHS: ReadonlyArray = [ new RegExp(`^/containers/(${ID})/json$`), new RegExp(`^/containers/(${ID})/start$`), new RegExp(`^/containers/(${ID})/attach$`), new RegExp(`^/containers/(${ID})/wait$`), + new RegExp(`^/containers/(${ID})/kill$`), + new RegExp(`^/containers/(${ID})/stop$`), + new RegExp(`^/containers/(${ID})/logs$`), new RegExp(`^/containers/(${ID})$`), ]; diff --git a/src/main/docker/registry-auth.test.ts b/src/main/docker/registry-auth.test.ts new file mode 100644 index 0000000..eb195ea --- /dev/null +++ b/src/main/docker/registry-auth.test.ts @@ -0,0 +1,62 @@ +import { describe, it, expect, jest } from '@jest/globals'; +import { resolveRegistryAuth, RegistryAuthDeps } from './registry-auth'; + +const decode = (header: string | undefined) => + header === undefined ? undefined : JSON.parse(Buffer.from(header, 'base64').toString('utf-8')); + +const deps = (over: Partial): RegistryAuthDeps => ({ + readConfig: () => null, + runHelper: () => null, + ...over, +}); + +describe('resolveRegistryAuth', () => { + it('uses the credential store, which is how Docker Desktop keeps secrets', () => { + const runHelper = jest.fn(() => ({ ServerURL: 'quay.io', Username: 'me', Secret: 's3cret' })); + const header = resolveRegistryAuth('quay.io', deps({ + readConfig: () => ({ credsStore: 'desktop', auths: { 'quay.io': {} } }), + runHelper: runHelper as RegistryAuthDeps['runHelper'], + })); + expect(runHelper).toHaveBeenCalledWith('desktop', 'quay.io'); + expect(decode(header)).toEqual({ username: 'me', password: 's3cret', serveraddress: 'quay.io' }); + }); + + it('prefers a per-registry helper over the general store', () => { + const runHelper = jest.fn(() => ({ Username: 'x', Secret: 'y' })); + resolveRegistryAuth('quay.io', deps({ + readConfig: () => ({ credsStore: 'desktop', credHelpers: { 'quay.io': 'ecr-login' } }), + runHelper: runHelper as RegistryAuthDeps['runHelper'], + })); + expect(runHelper).toHaveBeenCalledWith('ecr-login', 'quay.io'); + }); + + it('reads the default registry under the key docker writes it as', () => { + const runHelper = jest.fn(() => ({ Username: 'me', Secret: 'p' })); + resolveRegistryAuth('docker.io', deps({ + readConfig: () => ({ credsStore: 'desktop' }), + runHelper: runHelper as RegistryAuthDeps['runHelper'], + })); + expect(runHelper).toHaveBeenCalledWith('desktop', 'https://index.docker.io/v1/'); + }); + + it('falls back to an inline auths entry', () => { + const header = resolveRegistryAuth('quay.io', deps({ + readConfig: () => ({ auths: { 'quay.io': { auth: Buffer.from('user:pass').toString('base64') } } }), + })); + expect(decode(header)).toEqual({ username: 'user', password: 'pass', serveraddress: 'quay.io' }); + }); + + it('carries an identity token as a token, not as a password', () => { + const header = resolveRegistryAuth('quay.io', deps({ + readConfig: () => ({ credsStore: 'desktop' }), + runHelper: () => ({ Username: '', Secret: 'tok' }), + })); + expect(decode(header)).toEqual({ identitytoken: 'tok', serveraddress: 'quay.io' }); + }); + + it('returns nothing rather than throwing when there is no credential', () => { + expect(resolveRegistryAuth('quay.io', deps({}))).toBeUndefined(); + expect(resolveRegistryAuth('quay.io', deps({ readConfig: () => ({ credsStore: 'desktop' }) }))).toBeUndefined(); + expect(resolveRegistryAuth('quay.io', deps({ readConfig: () => ({ auths: { 'quay.io': { auth: 'not-base64-pair' } } }) }))).toBeUndefined(); + }); +}); diff --git a/src/main/docker/registry-auth.ts b/src/main/docker/registry-auth.ts new file mode 100644 index 0000000..6c2aefd --- /dev/null +++ b/src/main/docker/registry-auth.ts @@ -0,0 +1,124 @@ +/** + * Registry credentials, resolved in the app and attached by the filtering + * socket. + * + * The point of this module is what the job never sees. Under the old `docker: + * credentials` level a job read `~/.docker/config.json` itself, so using a + * private registry meant handing the repository the operator's secrets. Here + * the app reads them - outside the sandbox, where `~/.docker` stays denied - + * and the proxy attaches an X-Registry-Auth header to a pull the policy already + * permits. Naming a registry in `pull.registries` is the whole grant. + * + * The resolution order follows the docker CLI: a per-registry credential + * helper, then the configured credential store, then an inline `auths` entry. + */ + +import { execFileSync } from 'child_process'; +import * as fs from 'fs'; +import * as os from 'os'; +import * as path from 'path'; + +/** Docker's own name for the default registry, as it appears in config.json. */ +const DEFAULT_REGISTRY = 'docker.io'; +const DEFAULT_REGISTRY_KEY = 'https://index.docker.io/v1/'; + +interface DockerConfig { + auths?: Record; + credsStore?: string; + credHelpers?: Record; +} + +/** What a credential helper prints on stdout. */ +interface HelperCredentials { + ServerURL?: string; + Username?: string; + Secret?: string; +} + +export interface RegistryAuthDeps { + readConfig: () => DockerConfig | null; + /** Run `docker-credential- get` with `serverUrl` on stdin. */ + runHelper: (helper: string, serverUrl: string) => HelperCredentials | null; +} + +const configPath = (): string => path.join(os.homedir(), '.docker', 'config.json'); + +const nodeDeps: RegistryAuthDeps = { + readConfig: () => { + try { + return JSON.parse(fs.readFileSync(configPath(), 'utf-8')) as DockerConfig; + } catch { + // No config, or one we cannot read: the job simply pulls unauthenticated. + return null; + } + }, + runHelper: (helper, serverUrl) => { + try { + const stdout = execFileSync(`docker-credential-${helper}`, ['get'], { + input: serverUrl, + encoding: 'utf-8', + timeout: 10_000, + }); + return JSON.parse(stdout) as HelperCredentials; + } catch { + // A helper that errors means no stored credential for this registry, + // which is the same as having none. + return null; + } + }, +}; + +/** Every key a registry may be stored under, most specific first. */ +function configKeys(registry: string): string[] { + if (registry === DEFAULT_REGISTRY || registry === 'index.docker.io') { + return [DEFAULT_REGISTRY_KEY, 'index.docker.io', DEFAULT_REGISTRY]; + } + return [registry, `https://${registry}`, `${registry}/v1/`, `https://${registry}/v1/`]; +} + +/** The value of an X-Registry-Auth header: base64 of the AuthConfig JSON. */ +function encode(auth: Record): string { + return Buffer.from(JSON.stringify(auth)).toString('base64'); +} + +/** + * The X-Registry-Auth value for a registry, or undefined when the operator has + * no credential for it. Never throws: a pull that cannot be authenticated is + * still a pull, and an anonymous one may well succeed. + */ +export function resolveRegistryAuth(registry: string, deps: RegistryAuthDeps = nodeDeps): string | undefined { + const config = deps.readConfig(); + if (!config) return undefined; + + const keys = configKeys(registry); + const serveraddress = keys[0]; + + const helper = keys.map((key) => config.credHelpers?.[key]).find((h) => h !== undefined) ?? config.credsStore; + if (helper) { + const credentials = deps.runHelper(helper, serveraddress); + if (credentials?.Secret) { + // A helper answers with the literal username when the secret is + // an identity token rather than a password. + return credentials.Username === '' + ? encode({ identitytoken: credentials.Secret, serveraddress }) + : encode({ username: credentials.Username ?? '', password: credentials.Secret, serveraddress }); + } + } + + for (const key of keys) { + const entry = config.auths?.[key]; + if (!entry) continue; + if (entry.identitytoken) return encode({ identitytoken: entry.identitytoken, serveraddress }); + if (!entry.auth) continue; + const decoded = Buffer.from(entry.auth, 'base64').toString('utf-8'); + const separator = decoded.indexOf(':'); + if (separator === -1) continue; + return encode({ + username: decoded.slice(0, separator), + password: decoded.slice(separator + 1), + serveraddress, + }); + } + + return undefined; +} diff --git a/src/main/index.ts b/src/main/index.ts index b609b47..4b0df56 100644 --- a/src/main/index.ts +++ b/src/main/index.ts @@ -102,6 +102,7 @@ import { // Zustand store import { initStore, connectWindow, cleanupStore, store } from './store/init'; import { getEffectivePolicy, effectivePolicyLevel } from '../shared/localmostrc'; +import { resolveRegistryAuth } from './docker/registry-auth'; import { decidePolicyForJob, recordPendingPolicy, @@ -268,6 +269,10 @@ app.whenReady().then(async () => { const runnerManager = new RunnerManager({ onLog: sendLog, + // Resolved here, in the app, where ~/.docker is readable. The job never + // sees a credential: the filtering socket attaches this to a pull the + // policy already permits, so naming a registry is the whole grant. + attachRegistryAuth: (registry: string) => resolveRegistryAuth(registry), onStatusChange: sendStatusUpdate, onJobHistoryUpdate: sendJobHistoryUpdate, onReregistrationNeeded: reRegisterSingleInstance, diff --git a/src/main/ipc-handlers/policy.test.ts b/src/main/ipc-handlers/policy.test.ts new file mode 100644 index 0000000..a19c61a --- /dev/null +++ b/src/main/ipc-handlers/policy.test.ts @@ -0,0 +1,52 @@ +import { describe, it, expect } from '@jest/globals'; + +jest.mock('electron', () => ({ ipcMain: { handle: jest.fn() } })); +jest.mock('../policy-cache', () => ({ + listCachedPolicies: jest.fn(), approvePolicy: jest.fn(), denyPolicy: jest.fn(), + removeCachedPolicy: jest.fn(), recordPolicyDecision: jest.fn(), +})); +jest.mock('../app-state', () => ({ getRunnerManager: jest.fn(), getLogger: jest.fn() })); + +import { summarizeGrants } from './policy'; + +describe('summarizeGrants', () => { + it('shows docker grants, which an operator is consenting to when they approve', () => { + const grants = summarizeGrants({ + shared: { + docker: { + pull: { registries: ['docker.io'] }, + run: { images: ['alpine:3'], mounts: [{ path: './', mode: 'ro' }], network: 'bridge' }, + }, + }, + }); + expect(grants.join('\n')).toMatch(/docker pull: docker\.io/); + expect(grants.join('\n')).toMatch(/docker run image: alpine:3/); + expect(grants.join('\n')).toMatch(/docker mount: \.\/ \(ro\)/); + expect(grants.join('\n')).toMatch(/docker network: bridge/); + }); + + it('shows a bare action block, which grants the action itself', () => { + expect(summarizeGrants({ shared: { docker: { build: {} } } }).join('\n')).toMatch(/docker build/); + expect(summarizeGrants({ shared: { docker: { run: {} } } }).join('\n')).toMatch(/docker run/); + }); + + it('shows docker grants from a per-workflow section too', () => { + const grants = summarizeGrants({ workflows: { integration: { docker: { run: { images: ['redis:7'] } } } } }); + expect(grants.join('\n')).toMatch(/integration: docker run image: redis:7/); + }); + + it('still shows the non-docker grants', () => { + const grants = summarizeGrants({ shared: { network: { allow: ['example.com'] }, filesystem: { write: ['~/.npm'] } } }); + expect(grants).toEqual(['network: example.com', 'write: ~/.npm']); + }); +}); + +describe('network grants on the approval screen', () => { + it('shows a declared network and whether it is routable', () => { + const grants = summarizeGrants({ + shared: { docker: { run: { networks: [{ name: 'vk-*', internal: true }, { name: 'build', internal: false }] } } }, + }); + expect(grants.join('\n')).toMatch(/docker network create: vk-\* \(internal\)/); + expect(grants.join('\n')).toMatch(/docker network create: build \(routable\)/); + }); +}); diff --git a/src/main/ipc-handlers/policy.ts b/src/main/ipc-handlers/policy.ts index 3e2a53c..3ae4ff2 100644 --- a/src/main/ipc-handlers/policy.ts +++ b/src/main/ipc-handlers/policy.ts @@ -17,6 +17,8 @@ import { } from '../policy-cache'; import { getRunnerManager, getLogger } from '../app-state'; +import { DockerPolicy } from '../../shared/docker-policy'; +import { describePolicy } from '../../shared/policy-describe'; /** * Describe what a policy grants, in the terms a reviewer cares about. @@ -24,24 +26,20 @@ import { interface PolicySection { network?: { allow?: string[] }; filesystem?: { read?: string[]; write?: string[] }; - sockets?: { allow?: string[] }; + docker?: DockerPolicy; } +/** + * What a docker policy grants, in the reviewer's terms. + * + * Every action block is named even when it carries no conditions: `run: {}` is + * a real grant - it permits creating and running containers - and an approval + * screen that showed nothing for it would be asking consent for an invisible + * capability. + */ + function describeSection(section: PolicySection, prefix: string): string[] { - const grants: string[] = []; - for (const host of section.network?.allow || []) { - grants.push(`${prefix}network: ${host}`); - } - for (const p of section.filesystem?.read || []) { - grants.push(`${prefix}read: ${p}`); - } - for (const p of section.filesystem?.write || []) { - grants.push(`${prefix}write: ${p}`); - } - for (const p of section.sockets?.allow || []) { - grants.push(`${prefix}socket: ${p}`); - } - return grants; + return describePolicy(section, prefix).map((grant) => grant.summary); } /** @@ -51,7 +49,7 @@ function describeSection(section: PolicySection, prefix: string): string[] { * `workflows:` that appears nowhere in `shared`, and approving what the UI * showed would otherwise approve more than was shown. */ -function summarizeGrants(config: { +export function summarizeGrants(config: { shared?: PolicySection; workflows?: Record; }): string[] { diff --git a/src/main/runner-manager.test.ts b/src/main/runner-manager.test.ts index 603408f..4c20db6 100644 --- a/src/main/runner-manager.test.ts +++ b/src/main/runner-manager.test.ts @@ -1328,6 +1328,10 @@ describe('RunnerManager', () => { expect(socket.start.mock.invocationCallOrder[0]).toBeLessThan(mockSpawnSandboxed.mock.invocationCallOrder[0]); const options = mockSpawnSandboxed.mock.calls[0][2]!; expect(options.env?.DOCKER_HOST).toBe(`unix://${socketPath}`); + // BuildKit, the default builder since Docker 23, streams a build over a + // gRPC session the filter cannot inspect. The classic builder is the one + // `build:` policy actually describes, so the job is pinned to it. + expect(options.env?.DOCKER_BUILDKIT).toBe('0'); // The profile grants this socket by name; the daemon's is no longer handed over. expect(options).toHaveProperty('dockerSocket', socketPath); expect(options).not.toHaveProperty('dockerGrants'); @@ -1530,3 +1534,114 @@ describe('docker access', () => { expect(stamp({ run: { images: ['postgres:16'] } })).toEqual(stamp({ run: { images: ['postgres:16'] } })); }); }); + +describe('job-start detection against injected output', () => { + const startedNames = (events: JobEvent[]) => events.filter((e) => e.type === 'started').map((e) => e.jobName); + + const setup = () => { + const events: JobEvent[] = []; + const manager = new RunnerManager({ + onLog: jest.fn(), + onStatusChange: jest.fn(), + onJobHistoryUpdate: jest.fn(), + onJobEvent: (e: JobEvent) => events.push(e), + }); + const helper = new RunnerManagerTestHelper(manager); + helper.setInstance(1, { name: 'runner-1', status: 'listening' }); + return { helper, events }; + }; + + it('ignores "Running job:" embedded in a line the job merely printed', async () => { + const { helper, events } = setup(); + + // A commit message, PR title or any echoed text can carry this. Here it + // arrives the way it really did: inside the job's contextData JSON. + await helper.parseRunnerOutput(1, '{"k":"message","v":"fix: match the `Running job: ` line properly"}'); + + expect(startedNames(events)).toEqual([]); + }); + + it('ignores a second job start on a worker already running one', async () => { + const { helper, events } = setup(); + + await helper.parseRunnerOutput(1, 'Running job: build'); + // The runner is --once: one spawn runs exactly one job, so anything after + // the first start is not a job, whatever it calls itself. + await helper.parseRunnerOutput(1, 'Running job: evil'); + + expect(startedNames(events)).toEqual(['build']); + }); + + it('still detects a genuine job start', async () => { + const { helper, events } = setup(); + await helper.parseRunnerOutput(1, 'Running job: build'); + expect(startedNames(events)).toEqual(['build']); + }); +}); + +describe('a worker constrained by policy drift stays constrained', () => { + it('does not reopen the docker socket or restore hosts when the job starts', async () => { + const docker = { pull: { registries: ['docker.io'] }, run: { images: ['alpine:3'] } }; + const manager = new RunnerManager({ + onLog: jest.fn(), + onStatusChange: jest.fn(), + onJobHistoryUpdate: jest.fn(), + getRepoPolicy: async () => ({ + hosts: ['example.com'], level: 'strict' as const, readPaths: [], writePaths: [], docker, + }), + }); + const helper = new RunnerManagerTestHelper(manager); + const proxy = { setPolicyAllowedHosts: jest.fn(), setPolicyLevel: jest.fn(), getStats: jest.fn(), getPolicyLevel: jest.fn() }; + const dockerSocket = { bind: jest.fn(), boundRepository: jest.fn() }; + helper.setProxy(1, proxy); + helper.setDockerProxy(1, dockerSocket); + // A stamp that cannot match the policy above: the approved policy moved + // after this worker was built, so its profile is out of date. + helper.setInstance(1, { + name: 'runner-1', status: 'busy', policyStamp: 'stale-stamp', + currentJob: { name: 'build', repository: 'owner/repo', startedAt: 'now', id: 'job-1', targetDisplayName: 'owner/repo', githubSha: 'abc1234' }, + }); + helper.setPendingTargetContext('1', { targetId: 't1', targetDisplayName: 'owner/repo', githubSha: 'abc1234' }); + + // The claim detects drift: network cut to nothing, docker socket left closed. + await helper.applyPolicyOnClaim(1, 'owner/repo', 'abc1234'); + expect(proxy.setPolicyAllowedHosts).toHaveBeenLastCalledWith([]); + expect(dockerSocket.bind).not.toHaveBeenCalled(); + + // The job-start refresh must not undo that. It runs without isClaim, so it + // never re-checks drift, and it used to fall straight through to widening. + await helper.applyRepoPolicy(1); + + expect(dockerSocket.bind).not.toHaveBeenCalled(); + expect(proxy.setPolicyAllowedHosts).toHaveBeenLastCalledWith([]); + }); +}); + +describe('a released slot does not carry the finished job\'s context', () => { + it('does not judge the next worker in that slot against the previous repository', async () => { + const docker = { run: { images: ['alpine:3'] } }; + const manager = new RunnerManager({ + onLog: jest.fn(), onStatusChange: jest.fn(), onJobHistoryUpdate: jest.fn(), + getRepoPolicy: async () => ({ hosts: [], level: 'strict' as const, readPaths: [], writePaths: [], docker }), + }); + const helper = new RunnerManagerTestHelper(manager); + + // Slot 1 ran a job for owner/first, then the worker went away. + helper.setInstance(1, { name: 'runner-1', status: 'listening' }); + helper.setPendingTargetContext('1', { targetId: 't1', targetDisplayName: 'owner/first', githubSha: 'aaa1111' }); + helper.releaseInstanceSlot(1); + + // The slot is reused for a different repository, by a worker that did not + // go through spawnWorkerForJob and so records no context of its own. + const dockerSocket = { bind: jest.fn(), boundRepository: jest.fn() }; + helper.setProxy(1, { setPolicyAllowedHosts: jest.fn(), setPolicyLevel: jest.fn() }); + helper.setDockerProxy(1, dockerSocket); + helper.setInstance(1, { name: 'runner-1', status: 'busy', claimedRepository: 'owner/second' }); + + await helper.applyPolicyOnClaim(1, 'owner/second', 'bbb2222'); + + // With the previous job's context still in the slot, this worker is judged + // against owner/first and its socket never opens for the job it is running. + expect(dockerSocket.bind).toHaveBeenCalledWith('owner/second', docker); + }); +}); diff --git a/src/main/runner-manager.ts b/src/main/runner-manager.ts index 0dfb7ae..80fe8ce 100644 --- a/src/main/runner-manager.ts +++ b/src/main/runner-manager.ts @@ -38,6 +38,8 @@ interface RunnerInstance { * the approved policy must not serve a job under it. */ policyStamp?: string; + /** Set when a claim found the approved policy had moved; the worker stays constrained. */ + policyDrifted?: boolean; /** * The repository whose job this worker claimed, as the broker reported it. * The docker socket opens only for this repository, and only when it is @@ -991,6 +993,13 @@ export class RunnerManager { const dockerSocketPath = path.join(sandboxDir, DOCKER_SOCKET_NAME); const dockerSocket = await this.startDockerProxy(instanceNum, dockerSocketPath); env.DOCKER_HOST = `unix://${dockerSocketPath}`; + // Pin the job to the classic builder. BuildKit - the default since + // Docker 23 - does not use POST /build at all: it negotiates a session + // and streams the build over gRPC, exporting host filesystem access to + // the daemon as it goes. "Which paths may this build read" then stops + // being a property of any request the filter can see, so `build:` policy + // would describe an endpoint a real `docker build` never calls. + env.DOCKER_BUILDKIT = '0'; instance.process = spawnSandboxed(runnerBinary, ['--once'], { cwd: sandboxDir, @@ -1293,6 +1302,12 @@ export class RunnerManager { instance.status = 'offline'; } this.instances.delete(instanceNum); + // The context describes the job this slot just finished. Left behind, the + // next worker to take the slot is judged against the previous repository - + // its docker socket refuses the job it is actually running, and a spawn + // that records no context of its own would resolve the previous + // repository's filesystem policy. + this.pendingTargetContext.delete(String(instanceNum)); this.updateAggregateStatus(); } @@ -1419,14 +1434,25 @@ export class RunnerManager { return; } - // Detect job start - const jobStartMatch = line.match(/Running job:\s*(.+)/i); + // Detect job start. + // + // Anchored, because this reads the job's own output: any text a job prints + // can contain "Running job: x" - a commit message, a PR title, a checked-out + // file - and an unanchored match turned that into a phantom job, complete + // with history entry, notification, and a worker marked busy. The runner + // emits this at the start of a line, optionally behind its own timestamp. + const jobStartMatch = line.match(/^\s*(?:\d{4}-\d{2}-\d{2}[T ][\d:.]+Z?:?\s*)?Running job:\s*(.+?)\s*$/i); if (jobStartMatch) { const jobName = jobStartMatch[1].trim(); - // Avoid duplicate job start detection - if (instance.status === 'busy' && instance.currentJob?.name === jobName) { - this.log('debug', `[instance ${instanceNum}] Ignoring duplicate job start: ${jobName}`); + // A worker runs with --once: one spawn is exactly one job. So a start on + // a worker that already has a job is never a second job - it is the job's + // output echoing something that looks like one. + if (instance.status === 'busy' || instance.currentJob) { + this.log( + 'debug', + `[instance ${instanceNum}] Ignoring job start while already running ${instance.currentJob?.name ?? 'a job'}: ${jobName}` + ); return; } @@ -1722,6 +1748,14 @@ export class RunnerManager { // this worker would run the job under the old boundary - so it is refused // rather than run. Approving through the app retires workers eagerly; this // also covers approving through the CLI, which writes the cache directly. + if (instance?.policyDrifted) { + this.log( + 'debug', + `[instance ${instanceNum}] Policy drifted for this worker; leaving it constrained rather than reapplying` + ); + return; + } + const currentStamp = this.stampFor(policy); if (isClaim && instance?.policyStamp && instance.policyStamp !== currentStamp) { // The filesystem half is fixed in this worker's profile and cannot be @@ -1731,7 +1765,11 @@ export class RunnerManager { // worker is retired so nothing further lands on it - this constrains the // job rather than refusing it, which the proxy cannot do on its own. // The docker socket stays as it was born, closed: nothing on this path - // opens it. + // opens it. Sticky, because the job-start refresh runs without isClaim + // and so never re-checks drift - without this it fell straight through + // to the widening below, restoring the hosts and rebinding the socket + // this branch had just closed. + if (instance) instance.policyDrifted = true; proxy.setPolicyAllowedHosts([]); proxy.setPolicyLevel('strict'); this.log( diff --git a/src/shared/docker-policy.test.ts b/src/shared/docker-policy.test.ts index aa3521a..587291e 100644 --- a/src/shared/docker-policy.test.ts +++ b/src/shared/docker-policy.test.ts @@ -45,12 +45,13 @@ describe('validateDockerPolicy', () => { expect(collect({ run: { images: ['postgres:16'], mounts: [{ path: './', mode: 'ro' }], network: 'bridge' } })).toEqual([]); }); - it('accepts pull, build and privileged alongside run', () => { + it('accepts pull, build and run together', () => { + // privileged is deliberately absent: it is rejected until a managed VM + // backend exists, and has a case of its own below. expect(collect({ pull: { registries: ['docker.io', 'ghcr.io'] }, run: { images: ['postgres:16'] }, build: { context: './' }, - privileged: true, })).toEqual([]); }); @@ -314,3 +315,126 @@ describe('serializeDockerPolicy quoting', () => { expect(reparsed.config?.shared?.docker?.privileged).toBeUndefined(); }); }); + +describe('diffDockerPolicy on bare action blocks', () => { + it('reports the action itself appearing, not just its conditions', () => { + // `run: {}` permits creating and running containers. Diffing only the + // leaves showed nothing, so the grant reached approval invisibly. + const cases: Array<[DockerPolicy, string]> = [ + [{ run: {} }, 'shared.docker.run'], + [{ build: {} }, 'shared.docker.build'], + [{ pull: { registries: [] } }, 'shared.docker.pull'], + ]; + for (const [block, path] of cases) { + const diffs = diffDockerPolicy(undefined, block, 'shared.docker'); + expect([path, diffs.map((d) => d.path)]).toEqual([path, expect.arrayContaining([path])]); + expect([path, diffs.every((d) => d.type === 'added')]).toEqual([path, true]); + } + }); + + it('reports an action being removed as well', () => { + const diffs = diffDockerPolicy({ build: {} }, undefined, 'shared.docker'); + expect(diffs.map((d) => d.path)).toContain('shared.docker.build'); + expect(diffs[0].type).toBe('removed'); + }); + + it('does not double-report an action that merely changed its conditions', () => { + const diffs = diffDockerPolicy({ run: { images: ['a'] } }, { run: { images: ['b'] } }, 'shared.docker'); + expect(diffs.map((d) => d.path)).not.toContain('shared.docker.run'); + expect(diffs.map((d) => d.path)).toContain('shared.docker.run.images'); + }); +}); + +describe('privileged at validation time', () => { + const collect = (value: unknown, path = 'shared.docker') => { + const errs: string[] = []; + validateDockerPolicy(value, path, (m) => errs.push(m)); + return errs; + }; + + it('rejects privileged: true, naming the backend it would require', () => { + // The design keeps privileged in the grammar so the gap stays honest, and + // rejects it until a managed VM can contain it. Accepting it here and + // refusing every request later reads as a broken policy, not a stage. + expect(collect({ privileged: true }).join('\n')).toMatch(/managed VM/i); + }); + + it('accepts privileged: false, which grants nothing', () => { + expect(collect({ privileged: false })).toEqual([]); + }); +}); + +describe('run.networks grammar', () => { + const collect = (value: unknown, path = 'shared.docker') => { + const errs: string[] = []; + validateDockerPolicy(value, path, (m) => errs.push(m)); + return errs; + }; + + it('accepts a declared network by name glob and internal flag', () => { + expect(collect({ run: { networks: [{ name: 'vk-*', internal: true }] } })).toEqual([]); + expect(collect({ run: { networks: [{ name: 'build', internal: false }] } })).toEqual([]); + }); + + it('requires internal to be stated, so a routable network is never the default', () => { + expect(collect({ run: { networks: [{ name: 'vk-*' }] } }).join('\n')).toMatch(/internal/i); + }); + + it('refuses a network key the grammar cannot spell, driver above all', () => { + for (const entry of [{ name: 'x', internal: true, driver: 'macvlan' }, { name: 'x', internal: true, options: {} }, { name: 'x', internal: true, ipam: {} }]) { + expect([Object.keys(entry).join(','), collect({ run: { networks: [entry] } }).length > 0]).toEqual([Object.keys(entry).join(','), true]); + } + }); + + it('composes shared and workflow networks additively', () => { + const merged = mergeDockerPolicy( + { run: { networks: [{ name: 'vk-*', internal: true }] } }, + { run: { networks: [{ name: 'build', internal: false }] } } + ); + expect(merged?.run?.networks).toEqual([{ name: 'vk-*', internal: true }, { name: 'build', internal: false }]); + }); + + it('shows a declared network in the approval diff', () => { + const diffs = diffDockerPolicy(undefined, { run: { networks: [{ name: 'vk-*', internal: true }] } }, 'shared.docker'); + expect(diffs.map((d) => d.path)).toContain('shared.docker.run.networks'); + expect(diffs[0].newValue).toMatch(/vk-\*/); + }); +}); + +describe('network entries in the approval diff', () => { + it('says internal or routable explicitly, since both are consent-relevant', () => { + const diffs = diffDockerPolicy(undefined, { + run: { networks: [{ name: 'vk-*', internal: true }, { name: 'open', internal: false }] }, + }, 'shared.docker'); + const values = diffs.filter((d) => d.path.endsWith('run.networks')).map((d) => d.newValue); + expect(values).toEqual(expect.arrayContaining(['vk-* (internal)', 'open (routable)'])); + }); +}); + +describe('a glob in run.images must say what tag it covers', () => { + const collect = (value: unknown, path = 'shared.docker') => { + const errs: string[] = []; + validateDockerPolicy(value, path, (m) => errs.push(m)); + return errs; + }; + + it('rejects a tagless glob, naming the form that means what it looks like', () => { + // Normalisation appends :latest to a tagless reference, so `vk/*` silently + // means "any repo under vk, but only its latest tag" - almost none of them. + // Guessing :* instead would be the same class of guess as accepting + // `docker: true`, so it is refused with the fix in the message. + const errs = collect({ run: { images: ['vk/*'] } }).join('\n'); + expect(errs).toMatch(/vk\/\*:\*/); + expect(errs).toMatch(/tag/i); + }); + + it('accepts a glob that carries a tag, globbed or exact', () => { + expect(collect({ run: { images: ['vk/*:*'] } })).toEqual([]); + expect(collect({ run: { images: ['vk/grader:*'] } })).toEqual([]); + expect(collect({ run: { images: ['vk/*:1'] } })).toEqual([]); + }); + + it('leaves exact references alone, tagless or not', () => { + expect(collect({ run: { images: ['alpine', 'alpine:3', 'ghcr.io/o/app:1'] } })).toEqual([]); + }); +}); diff --git a/src/shared/docker-policy.ts b/src/shared/docker-policy.ts index a52d815..86b40fa 100644 --- a/src/shared/docker-policy.ts +++ b/src/shared/docker-policy.ts @@ -20,9 +20,22 @@ export interface DockerMount { mode: MountMode; } -/** Container create, start, attach, wait and remove. */ +/** A network the job may create: a name glob, and whether it is internal. */ +export interface DockerNetworkPolicy { + /** Anchored glob; `*` matches any run of characters. */ + name: string; + /** + * Whether the network is cut off from anything outside it. Required rather + * than defaulted: a routable network is a real grant and has to be asked for + * in a way the approval diff shows. + */ + internal: boolean; +} + +/** Container create, start, attach, wait, kill, stop, remove and logs. */ export interface DockerRunPolicy { images?: string[]; + networks?: DockerNetworkPolicy[]; mounts?: DockerMount[]; network?: string; } @@ -89,6 +102,15 @@ export function validateDockerPolicy(value: unknown, path: string, push: (messag if (value.build !== undefined) validateBuild(value.build, `${path}.build`, push); if (value.privileged !== undefined && typeof value.privileged !== 'boolean') { push(`${path}.privileged must be a boolean`); + } else if (value.privileged === true) { + // Kept in the grammar so the capability gap stays visible, and refused + // until a backend exists that can contain it. Accepting the declaration + // here and then refusing every request it implies would read as a broken + // policy rather than a stage that has not shipped. + push( + `${path}.privileged requires a managed VM backend, which this build does not have; ` + + 'remove it, or run the work without privileged containers' + ); } } @@ -97,8 +119,28 @@ function validateRun(value: unknown, path: string, push: (message: string) => vo push(`${path} must be an object`); return; } - if (value.images !== undefined) validateStringArray(value.images, `${path}.images`, push); + if (value.images !== undefined) { + validateStringArray(value.images, `${path}.images`, push); + if (Array.isArray(value.images)) { + for (const image of value.images) { + if (typeof image !== 'string' || !image.includes('*')) continue; + // A reference with no tag normalises to :latest, so a tagless glob + // means "any repository here, but only its latest tag" - which is not + // what it looks like, and an approval diff cannot show the difference. + // Guessing :* instead would be the same guess this grammar refuses when + // it rejects `docker: true`, so say what to write instead. + const lastSegment = image.slice(image.lastIndexOf('/') + 1); + if (!lastSegment.includes(':') && !lastSegment.includes('@')) { + push( + `${path}.images entry "${image}" globs a repository but names no tag, which matches only ` + + `its "latest" tag. Write "${image}:*" for any tag, or name the tag you mean.` + ); + } + } + } + } if (value.mounts !== undefined) validateMounts(value.mounts, `${path}.mounts`, push); + if (value.networks !== undefined) validateNetworks(value.networks, `${path}.networks`, push); if (value.network !== undefined && typeof value.network !== 'string') { push(`${path}.network must be a string`); } else if (value.network === 'host' || (typeof value.network === 'string' && value.network.startsWith('container:'))) { @@ -108,6 +150,36 @@ function validateRun(value: unknown, path: string, push: (message: string) => vo } } +/** Keys a declared network may carry. Driver above all is absent by design. */ +const NETWORK_KEYS: readonly string[] = ['name', 'internal']; + +function validateNetworks(value: unknown, path: string, push: (message: string) => void): void { + if (!Array.isArray(value)) { + push(`${path} must be an array`); + return; + } + value.forEach((entry, i) => { + const at = `${path}[${i}]`; + if (!isPlainObject(entry)) { + push(`${at} must be an object with name and internal`); + return; + } + for (const key of Object.keys(entry)) { + if (!NETWORK_KEYS.includes(key)) { + // A macvlan or ipvlan network puts the container on the physical LAN, + // which is worse than host networking, and driver options can bind a + // bridge to a host address. None of it can be named, so none of it can + // be asked for; the filter always creates a plain internal bridge. + push(`${at}.${key} cannot be declared: a network may only name itself and say whether it is internal`); + } + } + if (typeof entry.name !== 'string' || entry.name === '') push(`${at}.name must be a non-empty string`); + if (typeof entry.internal !== 'boolean') { + push(`${at}.internal must be stated as true or false: a routable network is a grant of its own`); + } + }); +} + function validateMounts(value: unknown, path: string, push: (message: string) => void): void { if (!Array.isArray(value)) { push(`${path} must be an array`); @@ -191,11 +263,29 @@ function mergeRun(base?: DockerRunPolicy, override?: DockerRunPolicy): DockerRun if (images) run.images = images; const mounts = mergeMounts(base?.mounts, override?.mounts); if (mounts) run.mounts = mounts; + const networks = mergeNetworks(base?.networks, override?.networks); + if (networks) run.networks = networks; const network = override?.network ?? base?.network; if (network !== undefined) run.network = network; return run; } +function mergeNetworks( + base?: DockerNetworkPolicy[], + override?: DockerNetworkPolicy[] +): DockerNetworkPolicy[] | undefined { + if (!base && !override) return undefined; + const merged: DockerNetworkPolicy[] = []; + const seen = new Set(); + for (const entry of [...(base ?? []), ...(override ?? [])]) { + const key = `${entry.name}:${entry.internal}`; + if (seen.has(key)) continue; + seen.add(key); + merged.push(entry); + } + return merged.length > 0 ? merged : undefined; +} + function mergeBuild(base?: DockerBuildPolicy, override?: DockerBuildPolicy): DockerBuildPolicy | undefined { if (!base && !override) return undefined; const context = override?.context ?? base?.context; @@ -235,6 +325,9 @@ export interface DockerPolicyDiff { /** A mount as one string, in the shape a -v flag takes, so it diffs per grant. */ const mountKey = (m: DockerMount): string => `${m.path}:${m.mode}`; +// Both states are named: a routable network is a real grant, and showing it as +// a bare name left the approval diff silent about the part that matters. +const networkKey = (n: DockerNetworkPolicy): string => `${n.name} (${n.internal ? 'internal' : 'routable'})`; function diffLists(oldList: string[] | undefined, newList: string[] | undefined, path: string, diffs: DockerPolicyDiff[]): void { const oldSet = new Set(oldList ?? []); @@ -269,10 +362,28 @@ export function diffDockerPolicy( diffLists(oldP?.pull?.registries, newP?.pull?.registries, `${prefix}.pull.registries`, diffs); diffLists(oldP?.run?.images, newP?.run?.images, `${prefix}.run.images`, diffs); diffLists(oldP?.run?.mounts?.map(mountKey), newP?.run?.mounts?.map(mountKey), `${prefix}.run.mounts`, diffs); + diffLists(oldP?.run?.networks?.map(networkKey), newP?.run?.networks?.map(networkKey), `${prefix}.run.networks`, diffs); diffScalar(oldP?.run?.network, newP?.run?.network, `${prefix}.run.network`, diffs); diffScalar(oldP?.build?.context, newP?.build?.context, `${prefix}.build.context`, diffs); // false grants nothing, the same as absent. diffScalar(oldP?.privileged ? 'true' : undefined, newP?.privileged ? 'true' : undefined, `${prefix}.privileged`, diffs); + + // An action block with no conditions is still a grant - `run: {}` permits + // creating and running containers - and diffing only conditions showed an + // approver nothing for it at all. Named here only when the block is + // otherwise invisible, so a block that changed its conditions is not + // reported twice. + for (const action of ['pull', 'run', 'build'] as const) { + const had = oldP?.[action] !== undefined; + const has = newP?.[action] !== undefined; + if (had === has) continue; + if (diffs.some((d) => d.path.startsWith(`${prefix}.${action}.`))) continue; + diffs.push( + has + ? { path: `${prefix}.${action}`, type: 'added', newValue: action } + : { path: `${prefix}.${action}`, type: 'removed', oldValue: action } + ); + } return diffs; } @@ -304,8 +415,8 @@ export function serializeDockerPolicy(policy: DockerPolicy, indent: string): str } if (policy.run) { - const { images, mounts, network } = policy.run; - if (!images?.length && !mounts?.length && network === undefined) { + const { images, mounts, network, networks } = policy.run; + if (!images?.length && !mounts?.length && !networks?.length && network === undefined) { lines.push(`${i1}run: {}`); } else { lines.push(`${i1}run:`); @@ -313,6 +424,13 @@ export function serializeDockerPolicy(policy: DockerPolicy, indent: string): str lines.push(`${i2}images:`); for (const image of images) lines.push(`${i3}- ${quote(image)}`); } + if (networks?.length) { + lines.push(`${i2}networks:`); + for (const n of networks) { + lines.push(`${i3}- name: ${quote(n.name)}`); + lines.push(`${i3} internal: ${n.internal}`); + } + } if (mounts?.length) { lines.push(`${i2}mounts:`); for (const mount of mounts) { @@ -362,3 +480,40 @@ export function parseDockerPolicyHint(hint: string): DockerPolicy | undefined { if (errors.length > 0) return undefined; return loaded.docker as DockerPolicy; } + +/** + * The container grants a docker policy makes, one line each, for anything that + * asks an operator to approve them. Shared so the CLI and the app describe the + * same policy the same way: `localmost policy show` once rendered network, + * filesystem and env only, and approved the docker section unseen. + */ +export function describeDockerGrants(docker: DockerPolicy | undefined, prefix: string): string[] { + if (!docker) return []; + const grants: string[] = []; + if (docker.pull) { + const registries = docker.pull.registries ?? []; + if (registries.length === 0) grants.push(`${prefix}docker pull`); + for (const registry of registries) grants.push(`${prefix}docker pull: ${registry}`); + } + if (docker.run) { + const { images = [], mounts = [], network, networks = [] } = docker.run; + if (images.length === 0 && mounts.length === 0 && networks.length === 0 && network === undefined) { + grants.push(`${prefix}docker run`); + } + for (const image of images) grants.push(`${prefix}docker run image: ${image}`); + for (const mount of mounts) grants.push(`${prefix}docker mount: ${mount.path} (${mount.mode})`); + // Creating a network is a grant, and whether it is routable is the part an + // operator most needs to see. + for (const n of networks) { + grants.push(`${prefix}docker network create: ${n.name} (${n.internal ? 'internal' : 'routable'})`); + } + if (network !== undefined) grants.push(`${prefix}docker network: ${network}`); + } + if (docker.build) { + grants.push(docker.build.context === undefined + ? `${prefix}docker build` + : `${prefix}docker build: ${docker.build.context}`); + } + if (docker.privileged) grants.push(`${prefix}docker privileged`); + return grants; +} diff --git a/src/shared/localmostrc.test.ts b/src/shared/localmostrc.test.ts index 9d8faf1..3f34a18 100644 --- a/src/shared/localmostrc.test.ts +++ b/src/shared/localmostrc.test.ts @@ -967,7 +967,6 @@ describe('docker policy through serialization', () => { pull: { registries: ['docker.io', 'ghcr.io'] }, run: { images: ['postgres:16'], mounts: [{ path: './', mode: 'ro' }], network: 'bridge' }, build: { context: './' }, - privileged: true, }, }, workflows: { @@ -990,3 +989,47 @@ describe('docker policy through serialization', () => { expect(serializeLocalmostrc(config)).not.toContain('docker'); }); }); + +describe('a policy key the grammar does not know', () => { + const parse = (body: string) => parseLocalmostrcContent(`version: 1\nshared:\n${body}`); + + it('is refused rather than ignored, since an ignored key grants nothing while looking like it grants', () => { + // The failure this prevents: a misspelled key validates clean, shows up in + // no approval diff because nothing parses it, and silently applies none of + // what it appears to declare. Already seen once with `build:`. + const result = parse(' dokcer:\n run:\n images: ["alpine:3"]\n'); + expect(result.success).toBe(false); + expect(result.errors.map((e) => e.message).join('\n')).toMatch(/dokcer/); + }); + + it('names the keys that are accepted, so the fix is in the message', () => { + const errors = parse(' filesystm:\n read: ["/etc"]\n').errors.map((e) => e.message).join('\n'); + for (const key of ['network', 'filesystem', 'env', 'docker']) expect(errors).toContain(key); + }); + + it('still accepts every key the grammar does know', () => { + const ok = parse( + ' network:\n allow: ["github.com"]\n' + + ' filesystem:\n read: ["/etc"]\n' + + ' env:\n allow: ["CI"]\n' + + ' docker:\n run:\n images: ["alpine:3"]\n' + ); + expect(ok.errors).toEqual([]); + expect(ok.success).toBe(true); + }); +}); + +describe('secrets is a workflow-scoped key', () => { + it('is accepted under a workflow', () => { + const r = parseLocalmostrcContent( + 'version: 1\nworkflows:\n deploy:\n secrets:\n require: ["DEPLOY_KEY"]\n' + ); + expect(r.errors).toEqual([]); + }); + + it('is refused at shared scope, where nothing reads it', () => { + const r = parseLocalmostrcContent('version: 1\nshared:\n secrets:\n require: ["DEPLOY_KEY"]\n'); + expect(r.success).toBe(false); + expect(r.errors.map((e) => e.message).join('\n')).toMatch(/shared\.secrets is not a policy key/); + }); +}); diff --git a/src/shared/localmostrc.ts b/src/shared/localmostrc.ts index 67c1e7a..2208985 100644 --- a/src/shared/localmostrc.ts +++ b/src/shared/localmostrc.ts @@ -4,6 +4,7 @@ * Handles parsing, validation, and merging of declarative sandbox policies. */ import * as yaml from 'js-yaml'; +import { POLICY_SECTION_KEYS, WORKFLOW_POLICY_KEYS } from './policy-describe'; import * as fs from 'fs'; import * as path from 'path'; import { SandboxPolicy, NetworkPolicy, FilesystemPolicy, EnvPolicy } from './sandbox-profile'; @@ -178,7 +179,8 @@ export function parseLocalmostrcContent(content: string): ParseResult { errors.push({ message: '"workflows" must be an object' }); } else { for (const [workflowName, policy] of Object.entries(config.workflows as Record)) { - validatePolicy(policy, `workflows.${workflowName}`, errors); + // A workflow may also require secrets; the shared scope may not. + validatePolicy(policy, `workflows.${workflowName}`, errors, WORKFLOW_POLICY_KEYS); validateSecretsPolicy(policy, `workflows.${workflowName}`, errors); } } @@ -207,7 +209,12 @@ export function parseLocalmostrcContent(content: string): ParseResult { /** * Validate a sandbox policy object. */ -function validatePolicy(policy: unknown, path: string, errors: ParseError[]): void { +function validatePolicy( + policy: unknown, + path: string, + errors: ParseError[], + accepted: readonly string[] = POLICY_SECTION_KEYS +): void { if (policy === null || policy === undefined) { return; // Empty policy is valid } @@ -219,6 +226,18 @@ function validatePolicy(policy: unknown, path: string, errors: ParseError[]): vo const p = policy as Record; + // A key nobody parses grants nothing while reading as though it grants + // something, and shows up in no approval diff because no parser produced it. + // The keys are listed in one place, shared with what describes a policy, so + // a new one cannot be accepted without also being shown. + for (const key of Object.keys(p)) { + if (accepted.includes(key)) continue; + if (key === 'sockets') continue; // Has its own message, below. + errors.push({ + message: `${path}.${key} is not a policy key. Accepted keys: ${accepted.join(', ')}.`, + }); + } + // Validate network policy if (p.network !== undefined) { validateNetworkPolicy(p.network, `${path}.network`, errors); diff --git a/src/shared/policy-describe.test.ts b/src/shared/policy-describe.test.ts new file mode 100644 index 0000000..7a13758 --- /dev/null +++ b/src/shared/policy-describe.test.ts @@ -0,0 +1,54 @@ +import { describe, it, expect } from '@jest/globals'; +import { describePolicy, POLICY_SECTION_KEYS, WORKFLOW_POLICY_KEYS } from './policy-describe'; + +/** + * A section declaring something under every key a policy may carry. The guard + * test below leans on it: whatever the grammar grows, it has to appear here + * and it has to come back out of describePolicy. + */ +const everything = { + network: { allow: ['github.com'], deny: ['evil.example'] }, + filesystem: { read: ['/etc'], write: ['~/.npm'], deny: ['~/.ssh'] }, + env: { allow: ['CI'], deny: ['AWS_SECRET_ACCESS_KEY'] }, + docker: { + pull: { registries: ['docker.io'] }, + run: { images: ['alpine:3'], mounts: [{ path: './', mode: 'ro' as const }], networks: [{ name: 'vk-*', internal: true }] }, + }, + secrets: { require: ['DEPLOY_KEY'] }, +}; + +describe('describePolicy', () => { + it('names every value the policy declares, whatever key it sits under', () => { + const text = describePolicy(everything).map((g) => `${g.group} ${g.marker} ${g.value} ${g.summary}`).join('\n'); + for (const value of [ + 'github.com', 'evil.example', '/etc', '~/.npm', '~/.ssh', + 'CI', 'AWS_SECRET_ACCESS_KEY', 'docker.io', 'alpine:3', 'vk-*', 'DEPLOY_KEY', + ]) { + expect(text).toContain(value); + } + }); + + it('covers every key the grammar accepts, so a new one cannot be enforced unseen', () => { + // The defect this guards: a key that validates and is enforced but that no + // renderer prints is approved without being read. It has happened twice - + // `docker:` was missing from the CLI, `env:` from the app. + for (const key of WORKFLOW_POLICY_KEYS) { + const only = { [key]: (everything as Record)[key] }; + expect(describePolicy(only).length).toBeGreaterThan(0); + } + }); + + it('keeps the workflow-only key out of the shared list, which is what validation scopes on', () => { + expect(POLICY_SECTION_KEYS).not.toContain('secrets'); + expect(WORKFLOW_POLICY_KEYS).toContain('secrets'); + }); + + it('describes nothing for an empty section', () => { + expect(describePolicy({})).toEqual([]); + }); + + it('prefixes the flat summary, which is how a workflow scope is shown', () => { + const [grant] = describePolicy({ network: { allow: ['github.com'] } }, 'ci: '); + expect(grant.summary).toBe('ci: network: github.com'); + }); +}); diff --git a/src/shared/policy-describe.ts b/src/shared/policy-describe.ts new file mode 100644 index 0000000..2d94266 --- /dev/null +++ b/src/shared/policy-describe.ts @@ -0,0 +1,77 @@ +/** + * One description of a policy, for everything that shows one to a person. + * + * There were three renderers before this: the CLI's `policy show`, the app's + * approval summary, and the diff. Each enumerated the policy keys by hand, and + * each left out a different one - `docker:` never printed in the CLI, `env:` + * never printed in the app, and the app still described `sockets:`, a key the + * grammar had stopped accepting. A grant that is enforced but never rendered + * is approved without being read, which is the whole failure this file exists + * to prevent. + * + * So the keys are enumerated once, here. Presentation stays with the caller: + * `group` and `marker` are for a grouped, coloured listing, `summary` is the + * flat one-line form. Adding a key to the grammar means adding it here, and + * the guard test in policy-describe.test.ts fails until it is. + */ + +import { DockerPolicy, describeDockerGrants } from './docker-policy'; + +/** Every key a policy section may declare at any scope. */ +export const POLICY_SECTION_KEYS = ['network', 'filesystem', 'env', 'docker'] as const; + +/** + * What a workflow-scoped policy may declare on top of those: which secrets the + * workflow requires, which is a grant like any other and is shown like one. + */ +export const WORKFLOW_POLICY_KEYS = [...POLICY_SECTION_KEYS, 'secrets'] as const; + +export type PolicySectionKey = (typeof POLICY_SECTION_KEYS)[number]; + +/** A section of a policy, as the grammar accepts it. */ +export interface DescribablePolicy { + network?: { allow?: string[]; deny?: string[] }; + filesystem?: { read?: string[]; write?: string[]; deny?: string[] }; + env?: { allow?: string[]; deny?: string[] }; + docker?: DockerPolicy; + /** Workflow scope only. */ + secrets?: { require?: string[] }; +} + +export interface PolicyGrant { + /** Heading for a grouped listing, printed once per run of grants. */ + group: string; + /** Single character marking what the entry does: + grant, - deny, r/w access. */ + marker: string; + /** The declared value, as written. */ + value: string; + /** The flat one-line form, already prefixed. */ + summary: string; +} + +export function describePolicy(policy: DescribablePolicy, prefix = ''): PolicyGrant[] { + const grants: PolicyGrant[] = []; + const add = (group: string, marker: string, label: string, values: string[] | undefined) => { + for (const value of values ?? []) { + grants.push({ group, marker, value, summary: `${prefix}${label}: ${value}` }); + } + }; + + add('Network allow', '+', 'network', policy.network?.allow); + add('Network deny', '-', 'network denied', policy.network?.deny); + add('Filesystem read', 'r', 'read', policy.filesystem?.read); + add('Filesystem write', 'w', 'write', policy.filesystem?.write); + add('Filesystem deny', '-', 'denied', policy.filesystem?.deny); + add('Environment allow', '+', 'env', policy.env?.allow); + add('Environment deny', '-', 'env denied', policy.env?.deny); + + add('Secrets required', '+', 'secret', policy.secrets?.require); + + // Docker describes itself: what a container grant means is the docker + // grammar's business, and the line it produces is already the flat form. + for (const grant of describeDockerGrants(policy.docker, '')) { + grants.push({ group: 'Docker', marker: '+', value: grant, summary: `${prefix}${grant}` }); + } + + return grants; +} diff --git a/test/e2e/docker.spec.ts b/test/e2e/docker.spec.ts index 28bcd4d..b839972 100644 --- a/test/e2e/docker.spec.ts +++ b/test/e2e/docker.spec.ts @@ -39,6 +39,9 @@ import { DockerPolicy } from '../../src/shared/docker-policy'; const IMAGE = 'alpine:3'; +/** The repository this socket is bound to; the checkout layout follows from it. */ +const REPOSITORY = 'owner/repo'; + /** * What a repository using Docker declares: one image, the workspace read-only, * the default network. Outside a job this file binds it; inside a job the @@ -46,7 +49,12 @@ const IMAGE = 'alpine:3'; */ const policy: DockerPolicy = { pull: { registries: ['docker.io'] }, - run: { images: [IMAGE], mounts: [{ path: './', mode: 'ro' }], network: 'bridge' }, + run: { + images: [IMAGE], + mounts: [{ path: './', mode: 'ro' }], + network: 'bridge', + networks: [{ name: 'localmost-e2e-*', internal: true }], + }, }; /** The docker CLI a job would run, found on PATH the way the job's shell finds it. */ @@ -107,6 +115,7 @@ test.describe('a job using docker through the filtering socket', () => { let workspace: string; let env: NodeJS.ProcessEnv; const nonce = `hello-${process.pid}-${Date.now()}`; + const network = `localmost-e2e-${process.pid}`; // A real directory outside any workspace, so the refusal is "outside the job // workspace" rather than "cannot be resolved". Stands in for ~/.ssh. @@ -129,10 +138,12 @@ test.describe('a job using docker through the filtering socket', () => { workspace = fs.realpathSync.native(jobWorkspace); } else { const backend = new DesktopBackend(); - // The checkout dir the backend roots mounts at, resolved as the daemon - // sees it: tmpdir is under /var, a symlink. - const workDir = backend.workspaceMountRoot(scratch); - fs.mkdirSync(workDir); + // The checkout the backend roots mounts at, for the repository this + // socket is bound to below: the runner lays it out as + // _work//, and declared paths resolve against it. Resolved + // as the daemon sees it, since tmpdir is under /var, a symlink. + const workDir = backend.workspaceMountRoot(scratch, REPOSITORY); + fs.mkdirSync(workDir, { recursive: true }); workspace = fs.realpathSync.native(workDir); socketPath = path.join(scratch, 'docker.sock'); @@ -140,7 +151,7 @@ test.describe('a job using docker through the filtering socket', () => { logs = captured; proxy = new DockerFilterProxy({ backend, onLog: (entry) => captured.push(entry) }); await proxy.start(socketPath); - proxy.bind('owner/repo', policy); + proxy.bind(REPOSITORY, policy); } fs.writeFileSync(path.join(workspace, 'hello.txt'), `${nonce}\n`); @@ -158,6 +169,8 @@ test.describe('a job using docker through the filtering socket', () => { }); test.afterAll(async () => { + // Before the proxy stops, and tolerant of a test that already removed it. + if (env) await docker('network', 'rm', network).catch(() => undefined); await proxy?.stop(); if (scratch) fs.rmSync(scratch, { recursive: true, force: true }); if (jobWorkspace) fs.rmSync(jobWorkspace, { recursive: true, force: true }); @@ -227,4 +240,49 @@ test.describe('a job using docker through the filtering socket', () => { expect(denial?.policyHint).toMatch(/path: "\.\/"\n\s*mode: rw/); } }); + test('creates a declared network, joins a container to it, and removes it', async () => { + // The unit tests judge a body this file cannot see. Twice a create body + // they accepted was refused on the wire, because the real CLI sends keys + // the allowlist was never shown - so the network path is driven by the + // real CLI here, not only by fixtures. + const at = mark(); + + const created = await docker('network', 'create', '--internal', network); + expect(created.code, created.stderr).toBe(0); + + // Addressable by the name the job chose, not only by the id the daemon + // assigned: the proxy records both when it relays the create. + const inspect = await docker('network', 'inspect', network); + expect(inspect.code, inspect.stderr).toBe(0); + + const joined = await docker('run', '--rm', '--network', network, IMAGE, 'true'); + expect(joined.code, joined.stderr).toBe(0); + + const removed = await docker('network', 'rm', network); + expect(removed.code, removed.stderr).toBe(0); + + if (logs) { + const since = logsSince(at); + expect(since.some((l) => /forwarded POST \/networks\/create/.test(l.message))).toBe(true); + expect(since.filter((l) => /^(denied|refused) /.test(l.message))).toEqual([]); + } + }); + + test('refuses a network the policy does not declare, and one declared internal made routable', async () => { + const at = mark(); + + const undeclared = await docker('network', 'create', 'not-declared-by-policy'); + expect(undeclared.code).not.toBe(0); + expect(undeclared.stderr).toMatch(/not declared in the repository docker policy \(run\.networks\)/); + + // The name matches, but dropping --internal asks for a routable network, + // which is strictly more reachable than what the policy granted. + const routable = await docker('network', 'create', `${network}-routable`); + expect(routable.code).not.toBe(0); + expect(routable.stderr).toMatch(/declared internal, so it cannot be created routable/); + + if (logs) { + expect(logsSince(at).some((l) => /denied POST \/networks\/create/.test(l.message))).toBe(true); + } + }); });