diff --git a/AGENTS.md b/AGENTS.md index 046fce5..79f3283 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -21,7 +21,7 @@ deploy/ — Docker Compose + integration tests - `make fmt-all` / `make fmt-go` / `make fmt-rs` / `make fmt-ts` — apply gofmt / rustfmt / prettier (each `lint-*` target checks formatting) - `make hooks` — enable `.githooks` (pre-commit format check, pre-push `make lint-all`) - `make verify` — Quint spec simulation (request handling + listener) -- `make test-spec` — Quint `run` tests for `spec/listener.qnt`, `spec/router.qnt` and the network endpoints module of `spec/docker_socket_policy.qnt` +- `make test-spec` — Quint `run` tests for `spec/listener.qnt`, `spec/router.qnt` and `spec/docker_socket_policy.qnt` (network endpoints, the create model and policy selection) - `make test-integration` — Docker Compose integration tests - `make test-integration-sock` — listening-socket integration tests (`IMPL=rs|ts` for the others) @@ -39,17 +39,17 @@ deploy/ — Docker Compose + integration tests - Zero external deps where possible (Go: yaml.v3, Rust: tokio/hyper/serde/clap, TS: yaml) ## Test Coverage -- Go: 119 unit tests (main/listener: 24, policy: 11, middleware: 28, proxy: 52, audit: 4); the middleware count includes `TestHostConfigRows`, whose 131 #88 rows run as subtests -- Rust: 146 unit tests (main/listener: 23, policy: 13, middleware: 44, proxy: 47, handler: 13, audit: 4, transport: 2); `test_host_config_rows` runs the 131 #88 rows -- TypeScript: 300 unit tests, 1 skipped (flags: 44, listen: 13 incl. 1 skipped concurrency test (#46), middleware: 165 incl. the 131 #88 rows, proxy: 35, policy: 12, handler: 17, shutdown: 5, transport: 5, audit: 4) -- Integration, per implementation: 94 tests via deploy/test.sh and 15 socket tests via deploy/test-sock.sh (docker-compose) -- Quint: `make test-spec` runs the `spec/listener.qnt` `run` tests (instances `listener_locked`, `listener_unlocked`) and the `spec/router.qnt` `run` tests (instances `router`, `router_pre48`, `router_pre53`, `router_pre55`, `router_readonly`, whose `readonly*` tests run with `--match`, `router_pre76`, `router_pre78` and `router_pre70`; and, for the image pull, `pull`, `pull_pre83`, `pull_body_ignored` and `pull_pre84`), and the `spec/docker_socket_policy.qnt` `run` tests (instances `network_endpoints`, `network_endpoints_pre87`, `create` and `create_pre88`) +- Go: 131 unit tests (main/listener: 24, policy: 14, middleware: 28, proxy: 61, audit: 4); the middleware count includes `TestHostConfigRows`, whose 131 #88 rows run as subtests +- Rust: 158 unit tests (main/listener: 23, policy: 16, middleware: 44, proxy: 52, handler: 17, audit: 4, transport: 2); `test_host_config_rows` runs the 131 #88 rows +- TypeScript: 312 unit tests, 1 skipped (flags: 44, listen: 13 incl. 1 skipped concurrency test (#46), middleware: 165 incl. the 131 #88 rows, proxy: 40, policy: 15, handler: 21, shutdown: 5, transport: 5, audit: 4) +- Integration, per implementation: 108 tests via deploy/test.sh and 15 socket tests via deploy/test-sock.sh (docker-compose) +- Quint: `make test-spec` runs the `spec/listener.qnt` `run` tests (instances `listener_locked`, `listener_unlocked`) and the `spec/router.qnt` `run` tests (instances `router`, `router_pre48`, `router_pre53`, `router_pre55`, `router_readonly`, whose `readonly*` tests run with `--match`, `router_pre76`, `router_pre78`, `router_pre70` and `router_pre86`; and, for the image pull, `pull`, `pull_pre83`, `pull_body_ignored` and `pull_pre84`), and the `spec/docker_socket_policy.qnt` `run` tests (instances `network_endpoints`, `network_endpoints_pre87`, `create` and `create_pre88`, and the policy-selection tests of the main module `docker_socket_policy`, #85) ## Test Conventions - Go: stdlib `testing` package, `go test ./...` - Rust: `#[cfg(test)]` inline modules, `cargo test` - TypeScript: `node:test` framework, `npm run build && node --test dist/*.test.js` -- Integration: `make test-integration` (94 test cases) and `make test-integration-sock` (15 socket cases) via Docker Compose +- Integration: `make test-integration` (108 test cases) and `make test-integration-sock` (15 socket cases) via Docker Compose ## Contribution Workflow diff --git a/Makefile b/Makefile index f6e18d1..7def010 100644 --- a/Makefile +++ b/Makefile @@ -114,6 +114,8 @@ test-spec: $(QUINT) test $(ROUTER_SPEC) --main=pull_pre84 $(QUINT) test $(SPEC) --main=create $(QUINT) test $(SPEC) --main=create_pre88 + $(QUINT) test $(ROUTER_SPEC) --main=router_pre86 + $(QUINT) test $(SPEC) --main=docker_socket_policy --max-samples=100 verify-ts: $(QUINT) run $(SPEC) --max-steps=50 --invariants allInvariants --backend typescript diff --git a/README.md b/README.md index de02ba3..e30cd06 100644 --- a/README.md +++ b/README.md @@ -9,7 +9,7 @@ A validating Docker API proxy that enforces **per-service container policies** t Key features: -- **Policy-driven**: Per-service YAML policies, selected by image name, control images, volumes, flags, and env vars. The listening socket is the trust boundary: every caller of a socket can use every policy behind it (see [Trust model](#trust-model)) +- **Policy-driven**: Per-service YAML policies, selected by image and container name, control images, volumes, flags, and env vars. The listening socket is the trust boundary: every caller of a socket can use every policy behind it (see [Trust model](#trust-model)) - **Middleware pipeline**: 4 validation gates + 1 config mutator chain - **Default-deny router**: Only explicitly allowed endpoints pass through - **Formally verified**: [Quint](https://quint-lang.org/) specification: 9 request-handling invariants, 6 listener invariants, and router properties for `--readonly`, percent-encoded paths, empty segments, dot segments, exec, attach, the read-only passthrough and the container lifecycle @@ -100,9 +100,9 @@ All three implementations expose the same API surface, share the same [Quint spe | Language | Directory | Tests | Stack | |----------|-----------|-------|-------| -| Go | [go/](go/) | 119 unit + 94 integration + 15 socket | stdlib net/http + yaml.v3 | -| Rust | [rs/](rs/) | 146 unit + 94 integration + 15 socket | tokio, hyper, serde, clap | -| TypeScript | [ts/](ts/) | 300 unit (1 skipped) + 94 integration + 15 socket | Node 22 ESM, built-in http | +| Go | [go/](go/) | 131 unit + 108 integration + 15 socket | stdlib net/http + yaml.v3 | +| Rust | [rs/](rs/) | 158 unit + 108 integration + 15 socket | tokio, hyper, serde, clap | +| TypeScript | [ts/](ts/) | 312 unit (1 skipped) + 108 integration + 15 socket | Node 22 ESM, built-in http | ### Build All @@ -147,6 +147,8 @@ Create a YAML policy in the config directory: ```yaml # config/beacon.yaml service_name: beacon +container_names: # optional; see below + - beacon allowed_image_prefixes: - chainsafe/lodestar container_config: @@ -180,6 +182,45 @@ the bind must carry `:ro`, or the mount `ReadOnly: true`, and a bind without a mode is read-write in Docker, so it is denied. See [Container create: HostConfig rules](#container-create-hostconfig-rules). +`container_names` is optional. It lists the container names the policy +manages, each a valid Docker name (`[a-zA-Z0-9][a-zA-Z0-9_.-]*`; anything else +fails startup). It does two things +([#85](https://github.com/ChainSafe/docker-socket-policy/issues/85), +[#86](https://github.com/ChainSafe/docker-socket-policy/issues/86)): + +- **Create selects one policy.** `POST /containers/create?name=N` uses the one + policy whose `allowed_image_prefixes` match the image and whose + `container_names` contain `N`; a policy without `container_names` accepts any + name. No match is denied (`image X not allowed by any policy`, or + `container name N not allowed for image X by any policy`), and so is more than + one (`ambiguous policy for image X and name N: a, b`). A create without + `?name=` can only use a policy without `container_names` + (`image X requires a container name` otherwise). This is how two services + built from one image, such as a Lodestar beacon node and validator client, + get their own users and volumes. An image pull has no container name and + creates no container, so it is allowed when any policy for the image accepts + its tag or digest. +- **Lifecycle actions reach only covered containers.** A policy covers its + `container_names`, or its `service_name` when `container_names` is unset. + `start`, `stop`, `restart`, `kill`, `wait`, `pause`, `unpause` and `DELETE` + are forwarded only for a container whose name is covered (see + [Endpoint Access](#endpoint-access)). + +Policies are checked when the proxy starts, and it refuses to start (naming +both files) if two policies use the same `service_name`, or if two policies +could both match one create: their image prefixes share a namespace (equal, or +one is a `/`-prefix of the other) and either one has no `container_names` or +their `container_names` overlap. Two services that share an image therefore +both need `container_names`, with no name in common: + +```yaml +# config/beacon.yaml # config/validator.yaml +service_name: beacon service_name: validator +container_names: [beacon] container_names: [validator] +allowed_image_prefixes: allowed_image_prefixes: + - chainsafe/lodestar - chainsafe/lodestar +``` + ### Use the Proxy ```bash @@ -250,8 +291,8 @@ The daemon matches JSON field names without regard to case, and the last match w | Any | Path containing `//` | **DENIED** | | Any | Path with a `.` or `..` segment | **DENIED** | | POST | `/containers/create` | Validated by middleware chain | -| POST | `/containers/{name}/start\|stop\|restart\|kill\|wait\|pause\|unpause` | Allowed on known containers | -| DELETE | `/containers/{name}` | Allowed on known containers | +| POST | `/containers/{name}/start\|stop\|restart\|kill\|wait\|pause\|unpause` | Allowed only on a container a policy covers (see below) | +| DELETE | `/containers/{name}` | Allowed only on a container a policy covers (see below) | | Any | `/containers/…/exec` (an `exec` segment anywhere under `/containers`) | **DENIED** | | Any | `/exec/*` | **DENIED** | | Any | `/containers/{name}/attach…` (an `attach` segment anywhere under `/containers`) | **DENIED** | @@ -269,6 +310,8 @@ Exec is matched on whole path segments, so containers named like `exec-runner` w Attach is matched the same way: an `attach` segment under `/containers` is denied for every method, so `attach-runner` is an ordinary name, while a container literally named `attach` cannot be reached by name. Protocol upgrades are refused outright: any request with an `Upgrade` header, or a `Connection` header that lists `upgrade`, gets 403 `protocol upgrade not allowed` before it is routed, whatever its path ([#76](https://github.com/ChainSafe/docker-socket-policy/issues/76)). Forwarding one would hand the caller a raw stream to the daemon. Every Docker endpoint that upgrades (attach, exec start, the build session) is denied anyway; the refusal keeps any path the router allows from becoming one. +Lifecycle actions and `DELETE` are forwarded only for a container that a loaded policy covers: its name is in some policy's `container_names`, or is the `service_name` of a policy without `container_names` ([#86](https://github.com/ChainSafe/docker-socket-policy/issues/86)). If `{name}` is exactly one policy's covered name, the request is forwarded. Otherwise, because it may be a full or short container ID, or a covered name that only looks like an ID prefix (all lowercase hex), the proxy first asks the daemon `GET /containers/{name}/json` and checks the `Name` it reports. A covered name is forwarded; an uncovered name or a daemon 404 is denied with 403 `container {name} is not managed by any policy`, and any other daemon error with 403 `cannot resolve container {name}: …`. The audit log records the resolved name. Containers outside every policy, such as operator containers for monitoring, cannot be stopped or removed through the proxy. Reads (`GET`) on any container are unchanged. Before #86 these endpoints were forwarded for any container. + `/_ping`, `/version`, `/info` and `/events` are ordinary reads: `GET` and `HEAD` pass, and every other method is denied with `endpoint is not allowed`, `OPTIONS` (a CORS preflight) included ([#78](https://github.com/ChainSafe/docker-socket-policy/issues/78)). `CONNECT` is refused with 403 `CONNECT is not allowed` before anything else, whatever its target or headers; the TypeScript proxy answers it on the raw socket, since Node never hands `CONNECT` to the request handler. An image pull is routed on its query string, as the Docker Engine API defines it and the Docker CLI sends it: `POST /images/create?fromImage=docker.io%2Flibrary%2Falpine&tag=3.19` and no body ([#83](https://github.com/ChainSafe/docker-socket-policy/issues/83)). A pull with any body is denied (`image pull must not have a request body`), whatever its `Content-Type` or framing: the daemon reads a form body's `fromImage` over the query's, so a body could name an image the router never checked. The image gets the same checks as `Image` on create: its normalised repository must match a policy's prefix, then the tag must match that policy's `image_tag_pattern` and a digest needs `image_digest_allowed` (the CLI sends a digest in `tag`). The proxy denies a pull with no `fromImage` (`fromImage query parameter is required for image pull`), an import (`image import (fromSrc) is not allowed`: it would store any tarball under an allowed name), a tag in both `fromImage` and `tag`, a repeated `fromImage`, `tag` or `fromSrc`, a query the daemon would reject, and a pull with neither tag nor digest, which makes the daemon pull every tag of the repository (`image pull without a tag or digest pulls every tag and is not allowed`; `docker pull alpine` sends `tag=latest`, `docker pull --all-tags` is denied). @@ -422,12 +465,18 @@ callers. Anyone who can `connect(2)` to the socket, which means any member of its group, can create containers under **every** policy loaded from that proxy's `--config-dir`. -A policy is a per-service *template*, chosen by the `Image` of the request. It -fixes what a container from that image may look like: volumes, network mode, -env file, user, CLI flags, and no other host access (`HostConfig` is -default-deny, #88). A policy says nothing about *who* may ask for it. A -caller who names `chainsafe/lodestar` gets the policy for that image, whichever -service the caller belongs to. This is intended, not a gap: callers on a +A policy is a per-service *template*, chosen by the `Image` of the request and, +when policies set `container_names`, by the container name. It fixes what a +container from that image may look like: volumes, network mode, env file, user, +CLI flags, and no other host access (`HostConfig` is default-deny, #88). A +policy says nothing about *who* may ask for it. A caller who names +`chainsafe/lodestar` gets the policy for that image, whichever service the +caller belongs to. Container names are routing, not access control: the caller +picks the name, so a caller who may use the beacon policy may also name a +container `validator` and get the validator policy. Covered names do bound +lifecycle actions: a caller can stop or remove only containers that some +policy behind the socket covers, not every container on the host. This is +intended, not a gap: callers on a shared socket are all the same caller as far as the kernel can tell (for example, several developers sharing one account), so no proxy-side check could tell them apart. @@ -493,7 +542,7 @@ not at the socket. ## Formal Verification -This project includes a [Quint](https://quint-lang.org/) formal specification that models the security invariants as a state machine. Random-simulation verification runs 10,000 sampled traces of up to 100 steps each, checking all 11 invariants on every state transition. The same file models the create check on `HostConfig` ([#88](https://github.com/ChainSafe/docker-socket-policy/issues/88)): the default-deny field rules, `Mounts`, bind options and read-only volumes, and the case-insensitive field names the daemon accepts; its properties are checked exhaustively over every single-field, single-bind and single-mount request of the model, and instance `create_pre88` shows that they failed before the fix. A second module, `spec/listener.qnt`, models listening-socket startup (group selection, existing-path checks, the single-instance lock) with 6 more invariants. A third module, `spec/router.qnt`, models the router's denies for writes under `--readonly` ([#75](https://github.com/ChainSafe/docker-socket-policy/issues/75)), percent-encoded paths ([#53](https://github.com/ChainSafe/docker-socket-policy/issues/53)), empty interior segments ([#55](https://github.com/ChainSafe/docker-socket-policy/issues/55)), dot segments ([#70](https://github.com/ChainSafe/docker-socket-policy/issues/70)), exec ([#49](https://github.com/ChainSafe/docker-socket-policy/issues/49)) and attach ([#76](https://github.com/ChainSafe/docker-socket-policy/issues/76)), the GET/HEAD-only read passthrough ([#78](https://github.com/ChainSafe/docker-socket-policy/issues/78)), the `POST /containers/create` route, container-name extraction and the container-lifecycle branch, not the full routing table ([#24](https://github.com/ChainSafe/docker-socket-policy/issues/24), [#48](https://github.com/ChainSafe/docker-socket-policy/issues/48)). Its 12 properties are checked exhaustively over the methods `GET`, `HEAD`, `POST`, `DELETE` and every path of 1 to 3 segments over the model's word set; exec is denied by the router, not by a middleware gate. +This project includes a [Quint](https://quint-lang.org/) formal specification that models the security invariants as a state machine. Random-simulation verification runs 10,000 sampled traces of up to 100 steps each, checking all 12 invariants on every state transition, among them that every create used the one policy matching its image and name ([#85](https://github.com/ChainSafe/docker-socket-policy/issues/85)). The same file models the create check on `HostConfig` ([#88](https://github.com/ChainSafe/docker-socket-policy/issues/88)): the default-deny field rules, `Mounts`, bind options and read-only volumes, and the case-insensitive field names the daemon accepts; its properties are checked exhaustively over every single-field, single-bind and single-mount request of the model, and instance `create_pre88` shows that they failed before the fix. A second module, `spec/listener.qnt`, models listening-socket startup (group selection, existing-path checks, the single-instance lock) with 6 more invariants. A third module, `spec/router.qnt`, models the router's denies for writes under `--readonly` ([#75](https://github.com/ChainSafe/docker-socket-policy/issues/75)), percent-encoded paths ([#53](https://github.com/ChainSafe/docker-socket-policy/issues/53)), empty interior segments ([#55](https://github.com/ChainSafe/docker-socket-policy/issues/55)), dot segments ([#70](https://github.com/ChainSafe/docker-socket-policy/issues/70)), exec ([#49](https://github.com/ChainSafe/docker-socket-policy/issues/49)) and attach ([#76](https://github.com/ChainSafe/docker-socket-policy/issues/76)), the GET/HEAD-only read passthrough ([#78](https://github.com/ChainSafe/docker-socket-policy/issues/78)), the `POST /containers/create` route, container-name extraction and the container-lifecycle branch with the daemon lookup that limits it to covered containers ([#86](https://github.com/ChainSafe/docker-socket-policy/issues/86)), not the full routing table ([#24](https://github.com/ChainSafe/docker-socket-policy/issues/24), [#48](https://github.com/ChainSafe/docker-socket-policy/issues/48)). Its 13 properties are checked exhaustively over the methods `GET`, `HEAD`, `POST`, `DELETE` and every path of 1 to 3 segments over the model's word set; exec is denied by the router, not by a middleware gate. The request-handling spec also has a module, `network_endpoints_model`, for the endpoint entries under a forced `network_mode` ([#87](https://github.com/ChainSafe/docker-socket-policy/issues/87)). Its 3 properties (extra networks are denied, an allowed create names no network but the forced one, and every allowed create starts) are checked over five network modes and every endpoint map over seven keys; `network_endpoints_pre87` shows all three failing before the fix. @@ -503,7 +552,7 @@ The CI pipeline runs verification on every push and PR. A violation blocks the b make typecheck # Quint type-check (proves type safety) make verify # Random-simulation verification (default evaluator) make verify BACKEND=rust # Same, using the faster Rust backend -make test-spec # Quint `run` tests for listener.qnt, router.qnt, network_endpoints and the create model (one per table row) +make test-spec # Quint `run` tests for listener.qnt, router.qnt, network_endpoints, the create model and policy selection (one per table row) make validate # All checks: typecheck + verify + go vet + go test ``` diff --git a/config/beacon.yaml b/config/beacon.yaml index 6d3ea71..2fd96c2 100644 --- a/config/beacon.yaml +++ b/config/beacon.yaml @@ -1,6 +1,13 @@ # Per-service policy for the Lodestar beacon node. # Test with: docker-socket-policy --config-dir=./config service_name: beacon +# Optional (#85, #86). Creates use this policy only for these names, and +# lifecycle actions (start, stop, rm, ...) reach only containers it covers: +# these names, or service_name when container_names is unset. A second policy +# for chainsafe/lodestar (a validator client, say) must set its own, +# disjoint container_names, or the proxy refuses to start. +container_names: + - beacon user_id: '2001' group_id: '2001' diff --git a/deploy/config/dsp-it-a.yaml b/deploy/config/dsp-it-a.yaml new file mode 100644 index 0000000..15e5a4f --- /dev/null +++ b/deploy/config/dsp-it-a.yaml @@ -0,0 +1,15 @@ +# Integration-test policy for #85 and #86 (deploy/test.sh). dsp-it-a and +# dsp-it-b allow one image, the test runner's own, and are told apart by +# container_names; each forces its own user so the test can see which one a +# create got. Without disjoint container_names the two would fail to load. +service_name: dsp-it-a +container_names: + - dsp-it-a + +allowed_image_prefixes: + - docker-socket-policy-test-go-test + - docker-socket-policy-test-rs-test + - docker-socket-policy-test-ts-test + +container_config: + user: '1111:1111' diff --git a/deploy/config/dsp-it-b.yaml b/deploy/config/dsp-it-b.yaml new file mode 100644 index 0000000..09b4382 --- /dev/null +++ b/deploy/config/dsp-it-b.yaml @@ -0,0 +1,15 @@ +# Integration-test policy for #85 and #86 (deploy/test.sh). dsp-it-a and +# dsp-it-b allow one image, the test runner's own, and are told apart by +# container_names; each forces its own user so the test can see which one a +# create got. Without disjoint container_names the two would fail to load. +service_name: dsp-it-b +container_names: + - dsp-it-b + +allowed_image_prefixes: + - docker-socket-policy-test-go-test + - docker-socket-policy-test-rs-test + - docker-socket-policy-test-ts-test + +container_config: + user: '2222:2222' diff --git a/deploy/test.sh b/deploy/test.sh index 252c0f9..fe47d75 100755 --- a/deploy/test.sh +++ b/deploy/test.sh @@ -7,7 +7,9 @@ # the last two by starting real containers (#87); exec and attach are denied by the # router, protocol upgrades by the handler (#76), non-read methods on the read # endpoints by the router and CONNECT by the handler (#78), and every write on a -# second proxy run with --readonly (#75). +# second proxy run with --readonly (#75). Creates select their policy by +# container name (#85), and lifecycle actions reach only covered containers +# (#86). set -e @@ -487,11 +489,13 @@ check_denied "POST /containers//start -> 403 (empty name, not a container)" "$S" # Dotted API-version prefix (#52). The Docker CLI versions every request # (/v1.45/containers/...). The proxy must strip the prefix and route the rest -# like the unversioned path: a lifecycle call reaches the daemon (404, no such -# container — not a proxy 403), and a reserved segment is still denied. +# like the unversioned path: a lifecycle call reaches the lifecycle branch, and +# a reserved segment is still denied. No policy covers no-such-container, so +# the proxy asks the daemon (404) and denies it as unmanaged (#86); the +# default deny would name the endpoint instead. # TypeScript only stripped undotted prefixes, so these fell to the default deny. S=$(post_empty "$PROXY/v1.45/containers/no-such-container/start") -check "POST /v1.45/containers/*/start -> 404 (daemon answered, not proxy 403)" "404" "$S" +check_denied "POST /v1.45/containers/*/start -> 403 (lifecycle branch, unmanaged)" "$S" "$BODY" 'container no-such-container is not managed by any policy' S=$(delete_status "$PROXY/v1.45/containers/json") check_denied "DELETE /v1.45/containers/json -> 403 (reserved survives version strip)" "$S" "$BODY" 'endpoint DELETE /containers/json is not allowed' @@ -541,7 +545,7 @@ S=$(post_json '{"Image":"chainsafe/lodestar:beacon","Cmd":["--rcConfig","/data/c check_denied "POST /containers/create/extra -> 403 (not the create endpoint)" "$S" "$BODY" 'endpoint POST /containers/create/extra is not allowed' S=$(post_empty "$PROXY/containers/exec-nosuch/start") -check "POST /containers/exec-nosuch/start -> 404 (daemon answered, not proxy 403)" "404" "$S" +check_denied "POST /containers/exec-nosuch/start -> 403 (a name, not exec; unmanaged, #86)" "$S" "$BODY" 'container exec-nosuch is not managed by any policy' S=$(get_status "$PROXY/exec/0000000000000000000000000000000000000000000000000000000000000000/json") check_denied "GET /exec/*/json -> 403 (exec inspect denied)" "$S" "$BODY" 'exec is not allowed' @@ -697,6 +701,112 @@ check_denied "create with an extra endpoint under network_mode host -> 403" "$S" # Without the deny the create succeeds; do not leave the container behind. delete_status "$PROXY/containers/net-host?force=true" >/dev/null +# ─── Container names (#85) and lifecycle coverage (#86) ────── + +# deploy/config/dsp-it-a.yaml and dsp-it-b.yaml allow this test runner's own +# image, which every run has locally, under container_names dsp-it-a and +# dsp-it-b, and force different users. This runner's container is covered by +# no policy: it stands for an operator container. Its hostname is its short ID. +echo "" +echo "--- Container names (#85) and lifecycle coverage (#86) ---" + +SELF=$(hostname) +: >"$BODY" +curl -s -o "$BODY" $TIMEOUT --unix-socket "$PROXY_SOCK" "$PROXY/containers/$SELF/json" 2>/dev/null || true +SELF_ID=$(sed -n 's/.*"Id":"\([0-9a-f]\{64\}\)".*/\1/p' "$BODY") +SELF_NAME=$(sed -n 's/.*"Name":"\/\([^"]*\)".*/\1/p' "$BODY") +IT_IMAGE=$(sed -n 's/.*"Image":"\(docker-socket-policy-test-[a-z]*-test[^"]*\)".*/\1/p' "$BODY") +if [ -z "$SELF_ID" ] || [ -z "$SELF_NAME" ] || [ -z "$IT_IMAGE" ]; then + echo " FAIL: could not inspect the test container (id=$SELF_ID name=$SELF_NAME image=$IT_IMAGE)" + FAIL=$((FAIL+1)) +fi +IT_BODY="{\"Image\":\"$IT_IMAGE\",\"Cmd\":[\"sleep\",\"300\"]}" + +# Leftovers from an interrupted run. Both names are covered, so this is allowed. +delete_status "$PROXY/containers/dsp-it-a?force=1" >/dev/null +delete_status "$PROXY/containers/dsp-it-b?force=1" >/dev/null + +# #85: both policies allow the image. Before #85 the one applied was whichever +# came first in map order (Go, Rust) or load order (TypeScript), so one of the +# two names got the other's user. Five rounds, each create inspected. +OK_A=0 +OK_B=0 +i=0 +while [ $i -lt 5 ]; do + for n in a b; do + case "$n" in a) want='1111:1111' ;; b) want='2222:2222' ;; esac + S=$(post_json "$IT_BODY" "$PROXY/containers/create?name=dsp-it-$n") + : >"$BODY" + curl -s -o "$BODY" $TIMEOUT --unix-socket "$PROXY_SOCK" "$PROXY/containers/dsp-it-$n/json" 2>/dev/null || true + if [ "$S" = "201" ] && grep -qF "\"User\":\"$want\"" "$BODY"; then + case "$n" in a) OK_A=$((OK_A+1)) ;; b) OK_B=$((OK_B+1)) ;; esac + fi + delete_status "$PROXY/containers/dsp-it-$n?force=1" >/dev/null + done + i=$((i + 1)) +done +check "create ?name=dsp-it-a x5 -> 201 with policy dsp-it-a's user every time (#85)" "5" "$OK_A" +check "create ?name=dsp-it-b x5 -> 201 with policy dsp-it-b's user every time (#85)" "5" "$OK_B" + +# remove_if_created STATUS: a proxy without the fix creates these; remove the +# container so it does not outlive the run (or block the next one). +remove_if_created() { + if [ "$1" = "201" ]; then + id=$(sed -n 's/.*"Id":"\([0-9a-f]\{64\}\)".*/\1/p' "$BODY") + cp "$BODY" "$BODY.keep" + [ -n "$id" ] && delete_status "$PROXY/containers/$id?force=1" >/dev/null + mv "$BODY.keep" "$BODY" + fi +} + +S=$(post_json "$IT_BODY" "$PROXY/containers/create?name=dsp-it-c") +remove_if_created "$S" +check_denied "create ?name=dsp-it-c -> 403 (no policy accepts the name, #85)" "$S" "$BODY" "container name dsp-it-c not allowed for image $IT_IMAGE by any policy" + +# The daemon takes the first of a repeated name (r.Form.Get); the proxy denies +# the request rather than guess which one an auditor would read. +S=$(post_json "$IT_BODY" "$PROXY/containers/create?name=dsp-it-a&name=dsp-it-c") +remove_if_created "$S" +check_denied "create ?name=dsp-it-a&name=dsp-it-c -> 403 (duplicate name, #85)" "$S" "$BODY" 'duplicate name query parameter' + +S=$(post_json "$IT_BODY" "$PROXY/containers/create") +remove_if_created "$S" +check_denied "create without ?name= -> 403 (both policies set container_names, #85)" "$S" "$BODY" "image $IT_IMAGE requires a container name" + +# #86: lifecycle actions and DELETE on a covered container, by name, full ID +# and short ID, reach the daemon. +S=$(post_json "$IT_BODY" "$PROXY/containers/create?name=dsp-it-a") +IT_ID=$(sed -n 's/.*"Id":"\([0-9a-f]\{64\}\)".*/\1/p' "$BODY") +check "create ?name=dsp-it-a -> 201" "201" "$S" +IT_SHORT=$(echo "$IT_ID" | cut -c1-12) + +S=$(post_empty "$PROXY/containers/dsp-it-a/start") +check "POST /containers/dsp-it-a/start -> 204 (covered name)" "204" "$S" + +S=$(post_empty "$PROXY/containers/$IT_ID/kill") +check "POST /containers//kill -> 204 (resolved, covered)" "204" "$S" + +S=$(post_empty "$PROXY/v1.45/containers/$IT_SHORT/start") +check "POST /v1.45/containers//start -> 204 (resolved, covered)" "204" "$S" + +S=$(delete_status "$PROXY/containers/$IT_SHORT?force=1") +check "DELETE /containers/?force=1 -> 204 (resolved, covered)" "204" "$S" + +# #86: this runner's container is covered by no policy. Before #86 each of +# these reached the daemon: start answered 304 (already running) and DELETE +# without force 409 (running). Nothing here can stop the runner. +S=$(post_empty "$PROXY/containers/$SELF_NAME/start") +check_denied "POST /containers//start -> 403 (uncovered, #86)" "$S" "$BODY" "container $SELF_NAME is not managed by any policy" + +S=$(post_empty "$PROXY/containers/$SELF_ID/start") +check_denied "POST /containers//start -> 403 (uncovered, #86)" "$S" "$BODY" "container $SELF_ID is not managed by any policy" + +S=$(delete_status "$PROXY/v1.45/containers/$SELF") +check_denied "DELETE /v1.45/containers/ -> 403 (uncovered, #86)" "$S" "$BODY" "container $SELF is not managed by any policy" + +S=$(delete_status "$PROXY/containers/no-such-container") +check_denied "DELETE /containers/no-such-container -> 403 (daemon 404, #86)" "$S" "$BODY" 'container no-such-container is not managed by any policy' + # ─── Read-only mode (#75) ──────────────────────────────── # The same config behind --readonly. Every request whose method is not GET or diff --git a/go/internal/policy/manager.go b/go/internal/policy/manager.go index bf157fb..93b7ded 100644 --- a/go/internal/policy/manager.go +++ b/go/internal/policy/manager.go @@ -5,19 +5,53 @@ import ( "log/slog" "os" "path/filepath" + "regexp" + "sort" + "strings" "gopkg.in/yaml.v3" ) +// validContainerName is a Docker container name without the optional leading +// slash (#85). +var validContainerName = regexp.MustCompile(`^[a-zA-Z0-9][a-zA-Z0-9_.-]*$`) + +// ValidContainerName reports whether name is a valid Docker container name. +func ValidContainerName(name string) bool { + return validContainerName.MatchString(name) +} + type Manager struct { - policiesByName map[string]*Policy - policiesByImage map[string]*Policy + policiesByName map[string]*Policy + // ordered holds the policies sorted by service_name, so selection never + // depends on map iteration order (#85). + ordered []*Policy +} + +// NewManagerFromPolicies builds a manager without load-time validation, as +// Rust's Manager::from_map and TypeScript's Manager.fromPolicies do. Tests +// use it to reach states that NewManager refuses, such as ambiguity. The +// prefixes are used as given, so they must already be normalised. +func NewManagerFromPolicies(policies ...*Policy) *Manager { + m := &Manager{policiesByName: make(map[string]*Policy)} + for _, p := range policies { + m.policiesByName[p.ServiceName] = p + } + m.sortPolicies() + return m +} + +func (m *Manager) sortPolicies() { + m.ordered = m.ordered[:0] + for _, p := range m.policiesByName { + m.ordered = append(m.ordered, p) + } + sort.Slice(m.ordered, func(i, j int) bool { return m.ordered[i].ServiceName < m.ordered[j].ServiceName }) } func NewManager(configDir string) (*Manager, error) { m := &Manager{ - policiesByName: make(map[string]*Policy), - policiesByImage: make(map[string]*Policy), + policiesByName: make(map[string]*Policy), } entries, err := os.ReadDir(configDir) @@ -29,6 +63,7 @@ func NewManager(configDir string) (*Manager, error) { return nil, fmt.Errorf("failed to read config directory %s: %w", configDir, err) } + fileOf := make(map[string]string) for _, entry := range entries { if entry.IsDir() { continue @@ -44,12 +79,19 @@ func NewManager(configDir string) (*Manager, error) { return nil, fmt.Errorf("failed to load policy %s: %w", entry.Name(), err) } - m.policiesByName[p.ServiceName] = p - for _, prefix := range p.AllowedImagePrefixes { - m.policiesByImage[prefix] = p + // A second file with the same service_name used to replace the + // first silently (#85). + if prev, ok := fileOf[p.ServiceName]; ok { + return nil, fmt.Errorf("policies in %s and %s both use service_name %q", prev, entry.Name(), p.ServiceName) } + fileOf[p.ServiceName] = entry.Name() + m.policiesByName[p.ServiceName] = p } + m.sortPolicies() + if err := m.checkConflicts(fileOf); err != nil { + return nil, err + } return m, nil } @@ -70,6 +112,14 @@ func loadPolicy(filePath string) (*Policy, error) { if len(p.AllowedImagePrefixes) == 0 { return nil, fmt.Errorf("policy file %s missing required field 'allowed_image_prefixes'", filePath) } + if p.ContainerNames != nil && len(p.ContainerNames) == 0 { + return nil, fmt.Errorf("policy file %s: container_names must not be empty", filePath) + } + for _, n := range p.ContainerNames { + if !ValidContainerName(n) { + return nil, fmt.Errorf("policy file %s: invalid container name %q in container_names", filePath, n) + } + } // Prefixes are compared in normalised form (#84). for i, prefix := range p.AllowedImagePrefixes { @@ -83,6 +133,62 @@ func loadPolicy(filePath string) (*Policy, error) { return &p, nil } +// checkConflicts fails when two policies could both be selected for one +// create: their normalised image prefixes share a namespace, and one of them +// accepts any name or their container_names intersect (#85). +func (m *Manager) checkConflicts(fileOf map[string]string) error { + for i, a := range m.ordered { + for _, b := range m.ordered[i+1:] { + pa, pb, ok := overlappingPrefixes(a.AllowedImagePrefixes, b.AllowedImagePrefixes) + if !ok { + continue + } + var reason string + switch { + case a.ContainerNames == nil: + reason = fmt.Sprintf("%q has no container_names", a.ServiceName) + case b.ContainerNames == nil: + reason = fmt.Sprintf("%q has no container_names", b.ServiceName) + default: + shared := sharedName(a.ContainerNames, b.ContainerNames) + if shared == "" { + continue + } + reason = fmt.Sprintf("both accept container name %q", shared) + } + return fmt.Errorf("conflicting policies %q (%s) and %q (%s): image prefixes %q and %q overlap and %s", + a.ServiceName, fileOf[a.ServiceName], b.ServiceName, fileOf[b.ServiceName], pa, pb, reason) + } + } + return nil +} + +// overlappingPrefixes returns the first pair of prefixes where one equals the +// other or is a namespace (/) prefix of it. +func overlappingPrefixes(as, bs []string) (string, string, bool) { + for _, a := range as { + for _, b := range bs { + if a == b || strings.HasPrefix(a, b+"/") || strings.HasPrefix(b, a+"/") { + return a, b, true + } + } + } + return "", "", false +} + +// sharedName returns the smallest name in both lists, or "". +func sharedName(as, bs []string) string { + shared := "" + for _, a := range as { + for _, b := range bs { + if a == b && (shared == "" || a < shared) { + shared = a + } + } + } + return shared +} + func (m *Manager) Get(serviceName string) (*Policy, error) { p, ok := m.policiesByName[serviceName] if !ok { @@ -91,27 +197,88 @@ func (m *Manager) Get(serviceName string) (*Policy, error) { return p, nil } -// GetByImage returns the policy whose prefixes match the reference's -// normalised repository (#84). An invalid reference matches nothing. +// GetByImage returns the first policy, sorted by service_name, whose prefixes +// match the reference's normalised repository (#84, #85). An invalid +// reference matches nothing. func (m *Manager) GetByImage(imageRef string) (*Policy, error) { ref, err := ParseImageRef(imageRef) if err != nil { return nil, err } - for _, p := range m.policiesByName { - if MatchesImagePrefix(ref.Name, p.AllowedImagePrefixes) { - return p, nil - } + if ps := m.MatchingImage(ref.Name); len(ps) > 0 { + return ps[0], nil } return nil, fmt.Errorf("no policy found for image: %s", imageRef) } +// MatchingImage returns the policies, sorted by service_name, whose prefixes +// match a normalised repository name (#85). +func (m *Manager) MatchingImage(name string) []*Policy { + var out []*Policy + for _, p := range m.ordered { + if MatchesImagePrefix(name, p.AllowedImagePrefixes) { + out = append(out, p) + } + } + return out +} + +// SelectForCreate returns the one policy whose image prefixes match the +// normalised repository of imageRef and which accepts the container name +// (empty for a create without ?name=). No match, or more than one, is an +// error whose text is the deny message (#85). +func (m *Manager) SelectForCreate(imageRef, name string) (*Policy, error) { + ref, err := ParseImageRef(imageRef) + if err != nil { + return nil, err + } + + matching := m.MatchingImage(ref.Name) + var candidates []*Policy + for _, p := range matching { + if p.AcceptsName(name) { + candidates = append(candidates, p) + } + } + + switch { + case len(matching) == 0: + return nil, fmt.Errorf("image %s not allowed by any policy", imageRef) + case len(candidates) == 0 && name == "": + return nil, fmt.Errorf("image %s requires a container name", imageRef) + case len(candidates) == 0: + return nil, fmt.Errorf("container name %s not allowed for image %s by any policy", name, imageRef) + case len(candidates) > 1: + names := make([]string, len(candidates)) + for i, p := range candidates { + names[i] = p.ServiceName + } + return nil, fmt.Errorf("ambiguous policy for image %s and name %s: %s", imageRef, name, strings.Join(names, ", ")) + } + return candidates[0], nil +} + +// Covering returns the policies, sorted by service_name, whose covered names +// include name (#86). +func (m *Manager) Covering(name string) []*Policy { + var out []*Policy + for _, p := range m.ordered { + for _, n := range p.CoveredNames() { + if n == name { + out = append(out, p) + break + } + } + } + return out +} + func (m *Manager) List() []string { var names []string - for name := range m.policiesByName { - names = append(names, name) + for _, p := range m.ordered { + names = append(names, p.ServiceName) } return names } diff --git a/go/internal/policy/manager_test.go b/go/internal/policy/manager_test.go index 46b08b4..60c7519 100644 --- a/go/internal/policy/manager_test.go +++ b/go/internal/policy/manager_test.go @@ -19,11 +19,13 @@ func TestNewManager_LoadsValidPolicies(t *testing.T) { dir := t.TempDir() writePolicyFile(t, dir, "beacon.yaml", ` service_name: beacon +container_names: [beacon] allowed_image_prefixes: - chainsafe/lodestar `) writePolicyFile(t, dir, "validator.yaml", ` service_name: validator +container_names: [validator] allowed_image_prefixes: - chainsafe/lodestar `) @@ -286,3 +288,110 @@ func TestManagerNormalizesPrefixes(t *testing.T) { } } } + +// TestNewManager_LoadTimeValidation is the cross-language parity guard for +// the #85 load-time checks. Two policies conflict when their image prefixes +// share a namespace and one has no container_names or their container_names +// intersect. Before #85 every row loaded, and a duplicate service_name +// silently replaced the first file. +func TestNewManager_LoadTimeValidation(t *testing.T) { + tests := []struct { + name string + files map[string]string + wantErr string // "" means it loads + }{ + {"same prefix, no container_names", map[string]string{ + "a.yaml": "service_name: a\nallowed_image_prefixes: [alpine]\n", + "b.yaml": "service_name: b\nallowed_image_prefixes: [alpine]\n", + }, `conflicting policies "a" (a.yaml) and "b" (b.yaml): image prefixes "alpine" and "alpine" overlap and "a" has no container_names`}, + {"one side has no container_names", map[string]string{ + "a.yaml": "service_name: a\ncontainer_names: [x]\nallowed_image_prefixes: [chainsafe/lodestar]\n", + "b.yaml": "service_name: b\nallowed_image_prefixes: [chainsafe]\n", + }, `conflicting policies "a" (a.yaml) and "b" (b.yaml): image prefixes "chainsafe/lodestar" and "chainsafe" overlap and "b" has no container_names`}, + {"shared container name", map[string]string{ + "a.yaml": "service_name: a\ncontainer_names: [x, y]\nallowed_image_prefixes: [chainsafe/lodestar]\n", + "b.yaml": "service_name: b\ncontainer_names: [z, y]\nallowed_image_prefixes: [chainsafe/lodestar]\n", + }, `conflicting policies "a" (a.yaml) and "b" (b.yaml): image prefixes "chainsafe/lodestar" and "chainsafe/lodestar" overlap and both accept container name "y"`}, + {"disjoint container names", map[string]string{ + "a.yaml": "service_name: a\ncontainer_names: [beacon]\nallowed_image_prefixes: [chainsafe/lodestar]\n", + "b.yaml": "service_name: b\ncontainer_names: [validator]\nallowed_image_prefixes: [chainsafe/lodestar]\n", + }, ""}, + {"sibling prefixes do not overlap", map[string]string{ + "a.yaml": "service_name: a\nallowed_image_prefixes: [chainsafe/lodestar]\n", + "b.yaml": "service_name: b\nallowed_image_prefixes: [chainsafe/lodestar-backup]\n", + }, ""}, + {"duplicate service_name", map[string]string{ + "a.yaml": "service_name: svc\nallowed_image_prefixes: [alpine]\n", + "b.yaml": "service_name: svc\nallowed_image_prefixes: [busybox]\n", + }, `policies in a.yaml and b.yaml both use service_name "svc"`}, + {"invalid container name", map[string]string{ + "a.yaml": "service_name: a\ncontainer_names: [-bad]\nallowed_image_prefixes: [alpine]\n", + }, `invalid container name "-bad" in container_names`}, + {"container name with a slash", map[string]string{ + "a.yaml": "service_name: a\ncontainer_names: [a/b]\nallowed_image_prefixes: [alpine]\n", + }, `invalid container name "a/b" in container_names`}, + {"empty container_names", map[string]string{ + "a.yaml": "service_name: a\ncontainer_names: []\nallowed_image_prefixes: [alpine]\n", + }, `container_names must not be empty`}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + dir := t.TempDir() + for name, content := range tt.files { + writePolicyFile(t, dir, name, content) + } + _, err := NewManager(dir) + switch { + case tt.wantErr == "" && err != nil: + t.Fatalf("NewManager failed: %v", err) + case tt.wantErr != "" && err == nil: + t.Fatalf("NewManager loaded, want error %q", tt.wantErr) + case tt.wantErr != "" && !strings.Contains(err.Error(), tt.wantErr): + t.Fatalf("NewManager error %q, want it to contain %q", err, tt.wantErr) + } + }) + } +} + +// TestSelectForCreate covers #85 selection: the one policy matching the +// image and accepting the name, the nameless create, and ambiguity. +func TestSelectForCreate(t *testing.T) { + m := NewManagerFromPolicies( + &Policy{ServiceName: "validator", ContainerNames: []string{"validator"}, AllowedImagePrefixes: []string{"chainsafe/lodestar"}}, + &Policy{ServiceName: "beacon", ContainerNames: []string{"beacon"}, AllowedImagePrefixes: []string{"chainsafe/lodestar"}}, + &Policy{ServiceName: "any", AllowedImagePrefixes: []string{"chainsafe"}}, + ) + for i := 0; i < 50; i++ { + for _, name := range []string{"beacon", "validator"} { + _, err := m.SelectForCreate("chainsafe/lodestar:v1", name) + want := "ambiguous policy for image chainsafe/lodestar:v1 and name " + name + ": any, " + name + if err == nil || err.Error() != want { + t.Fatalf("SelectForCreate(%s) error = %v, want %q", name, err, want) + } + } + } + if p, err := m.SelectForCreate("chainsafe/lodestar:v1", ""); err != nil || p.ServiceName != "any" { + t.Fatalf("nameless: got %v, %v; want any", p, err) + } + + m = NewManagerFromPolicies(m.policiesByName["beacon"], m.policiesByName["validator"]) + for i := 0; i < 50; i++ { + for _, name := range []string{"beacon", "validator"} { + if p, err := m.SelectForCreate("chainsafe/lodestar:v1", name); err != nil || p.ServiceName != name { + t.Fatalf("iteration %d: SelectForCreate(%s) = %v, %v", i, name, p, err) + } + } + } +} + +func TestCovering(t *testing.T) { + m := NewManagerFromPolicies( + &Policy{ServiceName: "beacon", AllowedImagePrefixes: []string{"chainsafe/lodestar"}}, + &Policy{ServiceName: "vc", ContainerNames: []string{"validator"}, AllowedImagePrefixes: []string{"chainsafe/lodestar"}}, + ) + for name, want := range map[string]int{"beacon": 1, "validator": 1, "vc": 0, "other": 0} { + if got := len(m.Covering(name)); got != want { + t.Errorf("Covering(%q) = %d policies, want %d", name, got, want) + } + } +} diff --git a/go/internal/policy/types.go b/go/internal/policy/types.go index d6b1783..6564e13 100644 --- a/go/internal/policy/types.go +++ b/go/internal/policy/types.go @@ -4,6 +4,7 @@ import "encoding/json" type Policy struct { ServiceName string `yaml:"service_name"` + ContainerNames []string `yaml:"container_names"` UserID string `yaml:"user_id"` GroupID string `yaml:"group_id"` AllowedImagePrefixes []string `yaml:"allowed_image_prefixes"` @@ -18,6 +19,30 @@ type Policy struct { DeniedFlags []string `yaml:"denied_flags"` } +// CoveredNames are the container names the policy manages: its +// container_names, or its service_name when container_names is unset (#85, +// #86). Lifecycle actions and DELETE are forwarded only for covered names. +func (p *Policy) CoveredNames() []string { + if p.ContainerNames != nil { + return p.ContainerNames + } + return []string{p.ServiceName} +} + +// AcceptsName reports whether a create named name may use the policy: a +// policy without container_names accepts any name, including none (#85). +func (p *Policy) AcceptsName(name string) bool { + if p.ContainerNames == nil { + return true + } + for _, n := range p.ContainerNames { + if n == name { + return true + } + } + return false +} + type ContainerConfig struct { NetworkMode string `yaml:"network_mode"` RestartPolicy string `yaml:"restart_policy"` diff --git a/go/internal/proxy/handler.go b/go/internal/proxy/handler.go index 8aa7c91..1bc7e2f 100644 --- a/go/internal/proxy/handler.go +++ b/go/internal/proxy/handler.go @@ -3,9 +3,11 @@ package proxy import ( "bytes" "encoding/json" + "fmt" "io" "log/slog" "net/http" + "net/url" "strings" "github.com/ChainSafe/docker-socket-policy/go/internal/audit" @@ -80,6 +82,12 @@ func (h *Handler) ServeHTTP(w http.ResponseWriter, r *http.Request) { } route := h.router.RouteRequest(r.Method, target, len(body) > 0, bodyJSON) + // A lifecycle action or DELETE on a name that no policy covers directly: + // ask the daemon which container it is (#86). + if route.Action == ActionResolveContainer { + route = h.resolveContainer(r, route.Container) + } + extra := map[string]interface{}{ "method": r.Method, "path": path, @@ -90,6 +98,12 @@ func (h *Handler) ServeHTTP(w http.ResponseWriter, r *http.Request) { if route.Image != "" { extra["image"] = route.Image } + if route.Container != "" { + extra["container"] = route.Container + } + if route.ResolvedName != "" { + extra["resolved_name"] = route.ResolvedName + } if route.Action == ActionDeny { slog.Warn("denied", "method", r.Method, "path", path, "reason", route.DenyMsg) @@ -123,6 +137,91 @@ func (h *Handler) ServeHTTP(w http.ResponseWriter, r *http.Request) { h.transport.ServeHTTP(w, r) } +// maxInspectBody bounds the inspect response read during resolution. +const maxInspectBody = 4 << 20 + +// resolveContainer asks the daemon for GET /containers/{id}/json and allows +// the request only if the container's name is covered by a policy. A 404, an +// uncovered name or any other failure denies: it fails closed (#86). +func (h *Handler) resolveContainer(r *http.Request, id string) *RouteResult { + notManaged := &RouteResult{Action: ActionDeny, Container: id, DenyMsg: fmt.Sprintf("container %s is not managed by any policy", id)} + cannotResolve := func(reason string) *RouteResult { + return &RouteResult{Action: ActionDeny, Container: id, DenyMsg: fmt.Sprintf("cannot resolve container %s: %s", id, reason)} + } + + req, err := http.NewRequestWithContext(r.Context(), http.MethodGet, "/containers/"+url.PathEscape(id)+"/json", nil) + if err != nil { + return cannotResolve(err.Error()) + } + rec := newCaptureWriter(maxInspectBody) + h.transport.ServeHTTP(rec, req) + + switch { + case rec.status == http.StatusNotFound: + return notManaged + case rec.status != http.StatusOK: + return cannotResolve(fmt.Sprintf("daemon returned %d", rec.status)) + case rec.overflow: + return cannotResolve("inspect response too large") + } + + var inspect struct { + Name string `json:"Name"` + } + if err := json.Unmarshal(rec.body.Bytes(), &inspect); err != nil || inspect.Name == "" { + return cannotResolve("unreadable inspect response") + } + name := strings.TrimPrefix(inspect.Name, "/") + + ps := h.router.manager.Covering(name) + if len(ps) == 0 { + notManaged.ResolvedName = name + return notManaged + } + res := &RouteResult{Action: ActionAllow, Container: id, ResolvedName: name} + if len(ps) == 1 { + res.Service = ps[0].ServiceName + res.Policy = ps[0] + } + return res +} + +// captureWriter is an http.ResponseWriter that keeps the status and up to +// limit bytes of the body. +type captureWriter struct { + header http.Header + status int + body bytes.Buffer + limit int + overflow bool +} + +func newCaptureWriter(limit int) *captureWriter { + return &captureWriter{header: make(http.Header), limit: limit} +} + +func (c *captureWriter) Header() http.Header { return c.header } + +func (c *captureWriter) WriteHeader(status int) { + if c.status == 0 { + c.status = status + } +} + +func (c *captureWriter) Write(p []byte) (int, error) { + if c.status == 0 { + c.status = http.StatusOK + } + if room := c.limit - c.body.Len(); len(p) > room { + c.overflow = true + if room > 0 { + c.body.Write(p[:room]) + } + return len(p), nil + } + return c.body.Write(p) +} + // isUpgradeRequest reports an Upgrade header, or a Connection header that // lists the upgrade token. func isUpgradeRequest(h http.Header) bool { diff --git a/go/internal/proxy/handler_test.go b/go/internal/proxy/handler_test.go index 42e413c..f4b1c24 100644 --- a/go/internal/proxy/handler_test.go +++ b/go/internal/proxy/handler_test.go @@ -7,6 +7,7 @@ import ( "io" "net/http" "net/http/httptest" + "net/url" "os" "path/filepath" "strings" @@ -599,3 +600,194 @@ func TestHandler_PullRoutesOnQuery(t *testing.T) { t.Errorf("deny body = %q", w.Body.String()) } } + +// daemonTransport answers GET /containers/{id}/json from a table of +// id -> container name, as the daemon does, and records every request. +type daemonTransport struct { + names map[string]string + status int // when non-zero, every inspect answers this status + requests []string +} + +func (t *daemonTransport) ServeHTTP(w http.ResponseWriter, r *http.Request) { + t.requests = append(t.requests, r.Method+" "+r.URL.EscapedPath()) + if r.Method == "GET" && strings.HasPrefix(r.URL.Path, "/containers/") && strings.HasSuffix(r.URL.Path, "/json") { + if t.status != 0 { + http.Error(w, "boom", t.status) + return + } + id := strings.TrimSuffix(strings.TrimPrefix(r.URL.Path, "/containers/"), "/json") + name, ok := t.names[id] + if !ok { + http.Error(w, `{"message":"No such container: `+id+`"}`, http.StatusNotFound) + return + } + json.NewEncoder(w).Encode(map[string]string{"Id": "4f6e1d2c9a7b", "Name": "/" + name}) + return + } + w.WriteHeader(http.StatusNoContent) +} + +const ( + beaconID = "4f6e1d2c9a7b8c3d4e5f60718293a4b5c6d7e8f9012345678901234567890abc" + monitoringID = "9d8c7b6a5f4e3d2c1b0a99887766554433221100ffeeddccbbaa998877665544" +) + +func newDaemonTransport() *daemonTransport { + return &daemonTransport{names: map[string]string{ + "beacon": "beacon", + beaconID: "beacon", + beaconID[:12]: "beacon", + "dsp-monitoring": "dsp-monitoring", + monitoringID: "dsp-monitoring", + monitoringID[:12]: "dsp-monitoring", + }} +} + +func newCoverageHandler(t *testing.T, transport Transport) (*Handler, string) { + t.Helper() + dir := t.TempDir() + os.WriteFile(filepath.Join(dir, "beacon.yaml"), []byte("service_name: beacon\nallowed_image_prefixes:\n - chainsafe/lodestar\n"), 0644) + m, err := policy.NewManager(dir) + if err != nil { + t.Fatalf("NewManager failed: %v", err) + } + logPath := filepath.Join(t.TempDir(), "audit.log") + auditLog, err := audit.NewLogger(logPath) + if err != nil { + t.Fatalf("NewLogger failed: %v", err) + } + t.Cleanup(func() { auditLog.Close() }) + return NewHandler(NewRouter(m), middleware.NewChain(), auditLog, transport), logPath +} + +// TestHandlerLifecycleCoverage is the handler half of #86: a lifecycle action +// or DELETE is forwarded only if its target is a container a policy covers. +// A name, full ID or short ID is resolved with GET /containers/{id}/json. +// Before #86 every row was forwarded. +func TestHandlerLifecycleCoverage(t *testing.T) { + notManaged := func(id string) string { return "container " + id + " is not managed by any policy" } + tests := []struct { + name string + method string + target string + status int + body string + inspects []string // inspect requests the proxy sends before deciding + }{ + {"covered name", "POST", "/v1.45/containers/beacon/stop?t=1", http.StatusNoContent, "", nil}, + {"covered full ID", "POST", "/containers/" + beaconID + "/kill", http.StatusNoContent, "", []string{"GET /containers/" + beaconID + "/json"}}, + {"covered short ID", "DELETE", "/containers/" + beaconID[:12] + "?force=1", http.StatusNoContent, "", []string{"GET /containers/" + beaconID[:12] + "/json"}}, + // uncoveredNameStopDeniedTest: an operator container. + {"uncovered name", "POST", "/containers/dsp-monitoring/stop", http.StatusForbidden, notManaged("dsp-monitoring"), []string{"GET /containers/dsp-monitoring/json"}}, + {"uncovered full ID", "POST", "/containers/" + monitoringID + "/kill", http.StatusForbidden, notManaged(monitoringID), []string{"GET /containers/" + monitoringID + "/json"}}, + {"uncovered short ID", "DELETE", "/containers/" + monitoringID[:12], http.StatusForbidden, notManaged(monitoringID[:12]), []string{"GET /containers/" + monitoringID[:12] + "/json"}}, + // realNameDeleteDeniedTest: the daemon answers 404. + {"unknown name", "DELETE", "/containers/mycontainer", http.StatusForbidden, notManaged("mycontainer"), []string{"GET /containers/mycontainer/json"}}, + // execPrefixNameStartDeniedTest + {"unknown exec-prefixed name", "POST", "/containers/exec-runner/start", http.StatusForbidden, notManaged("exec-runner"), []string{"GET /containers/exec-runner/json"}}, + // A read is not resolved. + {"inspect read", "GET", "/containers/dsp-monitoring/json", http.StatusOK, "", nil}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + d := newDaemonTransport() + h, _ := newCoverageHandler(t, d) + rec := httptest.NewRecorder() + h.ServeHTTP(rec, httptest.NewRequest(tt.method, tt.target, nil)) + if rec.Code != tt.status { + t.Fatalf("%s %s: status %d, want %d (%s)", tt.method, tt.target, rec.Code, tt.status, rec.Body.String()) + } + if tt.body != "" && strings.TrimSpace(rec.Body.String()) != tt.body { + t.Fatalf("%s %s: body %q, want %q", tt.method, tt.target, rec.Body.String(), tt.body) + } + want := append([]string{}, tt.inspects...) + if tt.status != http.StatusForbidden { + u, _ := url.Parse(tt.target) + want = append(want, tt.method+" "+u.EscapedPath()) + } + if strings.Join(d.requests, "\n") != strings.Join(want, "\n") { + t.Fatalf("%s %s: daemon saw %q, want %q", tt.method, tt.target, d.requests, want) + } + }) + } +} + +// A daemon error other than 404 fails closed with its own message (#86). +func TestHandlerLifecycleResolveFailsClosed(t *testing.T) { + d := newDaemonTransport() + d.status = http.StatusInternalServerError + h, _ := newCoverageHandler(t, d) + rec := httptest.NewRecorder() + h.ServeHTTP(rec, httptest.NewRequest("POST", "/containers/"+beaconID[:12]+"/stop", nil)) + want := "cannot resolve container " + beaconID[:12] + ": daemon returned 500" + if rec.Code != http.StatusForbidden || strings.TrimSpace(rec.Body.String()) != want { + t.Fatalf("status %d body %q, want 403 %q", rec.Code, rec.Body.String(), want) + } + if len(d.requests) != 1 { + t.Fatalf("daemon saw %q, want only the inspect", d.requests) + } +} + +// The resolved name is audited with the request (#86). +func TestHandlerLifecycleAuditsResolvedName(t *testing.T) { + d := newDaemonTransport() + h, logPath := newCoverageHandler(t, d) + h.ServeHTTP(httptest.NewRecorder(), httptest.NewRequest("POST", "/containers/"+beaconID[:12]+"/start", nil)) + h.ServeHTTP(httptest.NewRecorder(), httptest.NewRequest("POST", "/containers/"+monitoringID[:12]+"/start", nil)) + data, err := os.ReadFile(logPath) + if err != nil { + t.Fatalf("reading audit log: %v", err) + } + lines := strings.Split(strings.TrimSpace(string(data)), "\n") + if len(lines) != 2 { + t.Fatalf("audit log has %d lines, want 2:\n%s", len(lines), data) + } + for i, want := range []struct{ decision, name, service string }{{"ALLOW", "beacon", "beacon"}, {"DENY", "dsp-monitoring", ""}} { + var entry struct { + Decision string `json:"decision"` + Extra map[string]string `json:"extra"` + } + if err := json.Unmarshal([]byte(lines[i]), &entry); err != nil { + t.Fatalf("line %d: %v", i, err) + } + if entry.Decision != want.decision || entry.Extra["resolved_name"] != want.name || entry.Extra["service"] != want.service { + t.Fatalf("line %d = %s, want %s with resolved_name %q service %q", i, lines[i], want.decision, want.name, want.service) + } + } +} + +// TestHandlerCreateSelectsByName is the #85 reproduction through the handler: +// two policies for one image force different users, and each create named +// for one of them gets that policy's user, every time. +func TestHandlerCreateSelectsByName(t *testing.T) { + rec := &recorderTransport{} + h := newTestHandler(t, map[string]string{ + "a.yaml": "service_name: svc-a\ncontainer_names: [svc-a]\nallowed_image_prefixes: [alpine]\ncontainer_config: { user: \"1111:1111\" }\n", + "b.yaml": "service_name: svc-b\ncontainer_names: [svc-b]\nallowed_image_prefixes: [alpine]\ncontainer_config: { user: \"2222:2222\" }\n", + }, rec) + for i := 0; i < 50; i++ { + for _, tc := range []struct{ name, user string }{{"svc-a", "1111:1111"}, {"svc-b", "2222:2222"}} { + rec.lastRequest = nil + w := httptest.NewRecorder() + h.ServeHTTP(w, httptest.NewRequest("POST", "/v1.45/containers/create?name="+tc.name, strings.NewReader(`{"Image":"alpine:3.20","Cmd":["true"]}`))) + if w.Code != http.StatusOK || rec.lastRequest == nil { + t.Fatalf("iteration %d %s: status %d (%s)", i, tc.name, w.Code, w.Body.String()) + } + var fwd map[string]interface{} + b, _ := io.ReadAll(rec.lastRequest.Body) + if err := json.Unmarshal(b, &fwd); err != nil { + t.Fatalf("forwarded body: %v", err) + } + if fwd["User"] != tc.user { + t.Fatalf("iteration %d: create ?name=%s forwarded User %v, want %s", i, tc.name, fwd["User"], tc.user) + } + } + } + + w := httptest.NewRecorder() + h.ServeHTTP(w, httptest.NewRequest("POST", "/containers/create", strings.NewReader(`{"Image":"alpine:3.20"}`))) + if w.Code != http.StatusForbidden || strings.TrimSpace(w.Body.String()) != "image alpine:3.20 requires a container name" { + t.Fatalf("nameless create: status %d body %q", w.Code, w.Body.String()) + } +} diff --git a/go/internal/proxy/router.go b/go/internal/proxy/router.go index fbeb3a9..4b3697e 100644 --- a/go/internal/proxy/router.go +++ b/go/internal/proxy/router.go @@ -15,6 +15,10 @@ const ( ActionDeny Action = iota ActionAllow ActionCreateContainer + // ActionResolveContainer: a lifecycle action or DELETE on a container + // that is not directly a covered name. The handler asks the daemon which + // container it is and allows the request only if its name is covered (#86). + ActionResolveContainer ) type RouteResult struct { @@ -23,8 +27,11 @@ type RouteResult struct { Policy *policy.Policy Body map[string]interface{} Container string - Image string - DenyMsg string + // ResolvedName is the daemon's name for Container, set when the handler + // resolved it (#86). + ResolvedName string + Image string + DenyMsg string } type Router struct { @@ -99,31 +106,31 @@ func (r *Router) RouteRequest(method, target string, hasBody bool, body map[stri } if path == "/containers/create" && method == "POST" { - return r.routeCreate(body) + return r.routeCreate(body, rawQuery) } if containerName := extractContainerName(path); containerName != "" { switch { case strings.HasSuffix(path, "/start") && method == "POST": - return r.routeByContainerName(containerName, "start") + return r.routeByContainerName(containerName) case strings.HasSuffix(path, "/stop") && method == "POST": - return r.routeByContainerName(containerName, "stop") + return r.routeByContainerName(containerName) case strings.HasSuffix(path, "/restart") && method == "POST": - return r.routeByContainerName(containerName, "restart") + return r.routeByContainerName(containerName) case strings.HasSuffix(path, "/kill") && method == "POST": - return r.routeByContainerName(containerName, "kill") + return r.routeByContainerName(containerName) case strings.HasSuffix(path, "/wait") && method == "POST": - return r.routeByContainerName(containerName, "wait") + return r.routeByContainerName(containerName) case strings.HasSuffix(path, "/pause") && method == "POST": - return r.routeByContainerName(containerName, "pause") + return r.routeByContainerName(containerName) case strings.HasSuffix(path, "/unpause") && method == "POST": - return r.routeByContainerName(containerName, "unpause") + return r.routeByContainerName(containerName) case strings.HasSuffix(path, "/rename") && method == "POST": return &RouteResult{Action: ActionDeny, DenyMsg: "rename is not allowed"} case strings.HasSuffix(path, "/update") && method == "POST": return &RouteResult{Action: ActionDeny, DenyMsg: "update is not allowed"} case method == "DELETE": - return r.routeByContainerName(containerName, "rm") + return r.routeByContainerName(containerName) default: if method == "GET" { return &RouteResult{Action: ActionAllow} @@ -145,7 +152,7 @@ func (r *Router) RouteRequest(method, target string, hasBody bool, body map[stri } } -func (r *Router) routeCreate(body map[string]interface{}) *RouteResult { +func (r *Router) routeCreate(body map[string]interface{}, rawQuery string) *RouteResult { if len(body) == 0 { return &RouteResult{Action: ActionDeny, DenyMsg: "empty request body"} } @@ -158,34 +165,78 @@ func (r *Router) routeCreate(body map[string]interface{}) *RouteResult { return &RouteResult{Action: ActionDeny, DenyMsg: err.Error()} } - p, err := r.manager.GetByImage(image) + name, msg := createName(rawQuery) + if msg != "" { + return &RouteResult{Action: ActionDeny, DenyMsg: msg} + } + + p, err := r.manager.SelectForCreate(image, name) if err != nil { - return &RouteResult{Action: ActionDeny, DenyMsg: fmt.Sprintf("image %s not allowed by any policy", image)} + return &RouteResult{Action: ActionDeny, DenyMsg: err.Error()} } return &RouteResult{ - Action: ActionCreateContainer, - Service: p.ServiceName, - Policy: p, - Body: body, - Image: image, + Action: ActionCreateContainer, + Service: p.ServiceName, + Policy: p, + Body: body, + Container: name, + Image: image, + } +} + +// createName returns the ?name= of a create, without the one leading slash +// the daemon also strips, or "" when there is none. The query is parsed as the +// daemon parses it, with url.ParseQuery, as the image pull does (#83). msg is +// the deny message for a query the daemon would reject or an invalid name +// (#85). +func createName(rawQuery string) (name, msg string) { + q, err := url.ParseQuery(rawQuery) + if err != nil { + return "", "invalid query string" + } + // The daemon takes the first of a repeated name (r.Form.Get). Deny it + // rather than pick, as the pull does for its parameters. + if len(q["name"]) > 1 { + return "", "duplicate name query parameter" + } + name = strings.TrimPrefix(q.Get("name"), "/") + if name != "" && !policy.ValidContainerName(name) { + return "", "invalid container name" } + return name, "" } -func (r *Router) routeByContainerName(containerName, action string) *RouteResult { - if p, err := r.manager.Get(containerName); err == nil { +// routeByContainerName allows a lifecycle action or DELETE on a name that +// exactly one policy covers. Anything else, including a name that could also +// be an ID prefix, needs the daemon to say which container it is (#86). +func (r *Router) routeByContainerName(containerName string) *RouteResult { + if ps := r.manager.Covering(containerName); len(ps) == 1 && !couldBeID(containerName) { return &RouteResult{ - Action: ActionAllow, - Service: p.ServiceName, - Policy: p, + Action: ActionAllow, + Service: ps[0].ServiceName, + Policy: ps[0], + Container: containerName, } } return &RouteResult{ - Action: ActionAllow, + Action: ActionResolveContainer, Container: containerName, } } +// couldBeID reports a name of lowercase hex digits only. The daemon looks up +// an ID, then a name, then an ID prefix, so if no container has this name it +// would act on the container whose ID starts with it (#86). +func couldBeID(name string) bool { + for _, c := range name { + if !(c >= '0' && c <= '9' || c >= 'a' && c <= 'f') { + return false + } + } + return name != "" +} + // routeImagePull routes an image pull (#83). The Docker CLI sends fromImage // and tag in the query string and no body. The image then gets the create checks: a policy prefix on the // normalised repository (#84), and that policy's tag and digest rules. @@ -237,15 +288,24 @@ func (r *Router) routeImagePull(rawQuery string, hasBody bool) *RouteResult { return deny("image pull without a tag or digest pulls every tag and is not allowed") } - p, err := r.manager.GetByImage(ref.Name) - if err != nil { + // Several policies may allow the image once container_names tells their + // creates apart (#85). A pull creates no container, so any of them may + // accept the tag or digest; with none, the first one's reason is given. + matching := r.manager.MatchingImage(ref.Name) + if len(matching) == 0 { return deny(fmt.Sprintf("image %s not allowed by any policy", fromImage)) } - if err := policy.CheckImageTag(p, ref); err != nil { - return deny(err.Error()) + var firstErr error + for _, p := range matching { + err := policy.CheckImageTag(p, ref) + if err == nil { + return &RouteResult{Action: ActionAllow, Service: p.ServiceName, Image: fromImage} + } + if firstErr == nil { + firstErr = err + } } - - return &RouteResult{Action: ActionAllow, Service: p.ServiceName, Image: fromImage} + return deny(firstErr.Error()) } // stripAPIVersion strips one leading /v/ or /v./ (ASCII digits only), else returns path unchanged (#57). @@ -303,9 +363,9 @@ func isAttachPath(path string) bool { // reservedContainerSegments are Docker endpoints that sit where a container // name would: /containers/json lists, /containers/create creates. Treating one -// as a container name routes the request down the lifecycle path, where an -// unknown container is allowed through — so DELETE /containers/json would be -// allowed rather than denied. Rust (rs/src/proxy.rs) and TypeScript +// as a container name routes the request down the lifecycle path, where +// DELETE /containers/json would be treated as a container removal rather than +// denied. Rust (rs/src/proxy.rs) and TypeScript // (ts/src/proxy.ts) exclude the same set. var reservedContainerSegments = map[string]bool{ "create": true, diff --git a/go/internal/proxy/router_test.go b/go/internal/proxy/router_test.go index 6aba9f2..3bb572b 100644 --- a/go/internal/proxy/router_test.go +++ b/go/internal/proxy/router_test.go @@ -414,9 +414,9 @@ func TestRouterDenyPostOnReadOnly(t *testing.T) { // /containers/ is ambiguous: is usually a container name, but Docker // also has reserved endpoints at that position (/containers/json to list, // /containers/create to create). Treating a reserved word as a container name -// sends the request down the lifecycle path, where an unknown container is -// allowed through — so DELETE /containers/json was allowed in Go while Rust -// and TypeScript denied it. +// sends the request down the lifecycle path, where an unknown container was +// allowed through before #86 — so DELETE /containers/json was allowed in Go +// while Rust and TypeScript denied it. // // GET stays allowed either way: it reaches the GET/HEAD passthrough instead. func TestRouterReservedPathSegments(t *testing.T) { @@ -444,9 +444,11 @@ allowed_image_prefixes: {"DELETE", "/containers/exec", ActionDeny, "exec is not allowed"}, // Listing and inspecting stay allowed via the GET/HEAD passthrough. {"GET", "/containers/json", ActionAllow, ""}, - // A real container name is still routed as a container. - // realNameDeleteAllowedTest - {"DELETE", "/containers/mycontainer", ActionAllow, ""}, + // A real container name is still routed as a container. No policy + // covers it, so the daemon is asked which container it is, and the + // handler denies it (#86). + // realNameDeleteDeniedTest + {"DELETE", "/containers/mycontainer", ActionResolveContainer, ""}, {"GET", "/containers/mycontainer", ActionAllow, ""}, // The reserved word as a *sub*-resource is a normal inspect. // reservedInSubpathAllowedTest @@ -543,8 +545,8 @@ allowed_image_prefixes: body map[string]interface{} want Action }{ - // #52: dotted version, container lifecycle delete. - {"DELETE", "/v1.43/containers/foo", nil, ActionAllow}, + // #52: dotted version, container lifecycle delete (uncovered, #86). + {"DELETE", "/v1.43/containers/foo", nil, ActionResolveContainer}, // #52: dotted version, container lifecycle start. {"POST", "/v1.43/containers/beacon/start", nil, ActionAllow}, // #52: dotted version, create with a policy-allowed image. @@ -553,10 +555,10 @@ allowed_image_prefixes: {"POST", "/v1/containers/beacon/start", nil, ActionAllow}, // #52: the reserved segment survives the strip (#24 parity). {"DELETE", "/v1.43/containers/json", nil, ActionDeny}, - // #57: undotted version, container lifecycle delete. - {"DELETE", "/v1/containers/foo", nil, ActionAllow}, + // #57: undotted version, container lifecycle delete (uncovered, #86). + {"DELETE", "/v1/containers/foo", nil, ActionResolveContainer}, // #57: multi-digit major version. - {"DELETE", "/v10.0/containers/foo", nil, ActionAllow}, + {"DELETE", "/v10.0/containers/foo", nil, ActionResolveContainer}, // #57: /volumes/ is not a version; the daemon routes this as a volume removal. {"DELETE", "/volumes/containers/foo", nil, ActionDeny}, // #57: /version/ is not a version prefix. @@ -630,8 +632,8 @@ allowed_image_prefixes: {"DELETE", "/v%31/containers/foo", ActionDeny}, // #53: network names that need escaping are denied, GET included. {"GET", "/networks/a%20b", ActionDeny}, - // #53 control: a plain name is still routed as a container. - {"DELETE", "/containers/foo", ActionAllow}, + // #53 control: a plain name is still routed as a container (#86). + {"DELETE", "/containers/foo", ActionResolveContainer}, } for _, tt := range tests { t.Run(tt.method+" "+tt.path, func(t *testing.T) { @@ -678,10 +680,11 @@ allowed_image_prefixes: {"DELETE", "/containers/mycontainer/exec", nil, ActionDeny, "exec is not allowed"}, // execSubpathPostDeniedTest (#49) {"POST", "/containers/mycontainer/exec", nil, ActionDeny, "exec is not allowed"}, - // execPrefixNameStartAllowedTest (#49): an unknown container. - {"POST", "/containers/exec-runner/start", nil, ActionAllow, ""}, - // execPrefixNameDeleteAllowedTest (#49): an unknown container. - {"DELETE", "/containers/exec-runner", nil, ActionAllow, ""}, + // execPrefixNameStartDeniedTest (#49, #86): an uncovered container, + // routed for resolution; the handler denies it. + {"POST", "/containers/exec-runner/start", nil, ActionResolveContainer, ""}, + // execPrefixNameDeleteDeniedTest (#49, #86) + {"DELETE", "/containers/exec-runner", nil, ActionResolveContainer, ""}, // execPrefixNameGetAllowedTest (#49) {"GET", "/containers/exec-runner/json", nil, ActionAllow, ""}, // execNamespaceGetDeniedTest (#49): exec inspect leaks command lines. @@ -697,7 +700,7 @@ allowed_image_prefixes: // A name ending in exec is a plain name (#49, language-only). {"GET", "/containers/myexec/json", nil, ActionAllow, ""}, // A name starting with exec is a plain name (#49, language-only). - {"DELETE", "/containers/executor", nil, ActionAllow, ""}, + {"DELETE", "/containers/executor", nil, ActionResolveContainer, ""}, // The reserved name, decided by the exec check (#49, language-only). {"GET", "/containers/exec/json", nil, ActionDeny, "exec is not allowed"}, // A top-level name starting with exec is not the exec namespace (#49, language-only). @@ -1031,3 +1034,222 @@ allowed_image_prefixes: }) } } + +// lodestarPolicies is the #85 layout: beacon and validator run one image. +var lodestarPolicies = map[string]string{ + "beacon.yaml": ` +service_name: beacon +container_names: [beacon] +allowed_image_prefixes: + - chainsafe/lodestar +`, + "validator.yaml": ` +service_name: validator +container_names: [validator, validator-2] +allowed_image_prefixes: + - chainsafe/lodestar +`, + "monitoring.yaml": ` +service_name: monitoring +allowed_image_prefixes: + - prom/prometheus +`, +} + +// TestRouteCreateSelectsByName is the cross-language parity guard for #85. +// +// POST /containers/create selects the one policy whose image prefixes match +// and whose container_names contain ?name=; a policy without container_names +// accepts any name. Before #85 the policy was whichever matched the image +// first in map order (Go, Rust) or load order (TypeScript). +func TestRouteCreateSelectsByName(t *testing.T) { + r := NewRouter(newTestManager(t, lodestarPolicies)) + lodestar := map[string]interface{}{"Image": "chainsafe/lodestar:v1.20.0"} + prom := map[string]interface{}{"Image": "prom/prometheus:latest"} + + tests := []struct { + query string + body map[string]interface{} + want Action + service string + wantMsg string + }{ + {"name=beacon", lodestar, ActionCreateContainer, "beacon", ""}, + {"name=validator", lodestar, ActionCreateContainer, "validator", ""}, + {"name=validator-2", lodestar, ActionCreateContainer, "validator", ""}, + // The daemon strips one leading slash, and the name is decoded. + {"name=/validator", lodestar, ActionCreateContainer, "validator", ""}, + {"name=%2Fbeacon&platform=", lodestar, ActionCreateContainer, "beacon", ""}, + {"platform=linux&name=beacon", lodestar, ActionCreateContainer, "beacon", ""}, + // A name no policy for the image accepts. + {"name=other", lodestar, ActionDeny, "", "container name other not allowed for image chainsafe/lodestar:v1.20.0 by any policy"}, + // A create without ?name= only matches a policy without container_names. + {"", lodestar, ActionDeny, "", "image chainsafe/lodestar:v1.20.0 requires a container name"}, + {"name=", lodestar, ActionDeny, "", "image chainsafe/lodestar:v1.20.0 requires a container name"}, + {"", prom, ActionCreateContainer, "monitoring", ""}, + {"name=anything", prom, ActionCreateContainer, "monitoring", ""}, + // The image decides first. + {"name=beacon", map[string]interface{}{"Image": "alpine:latest"}, ActionDeny, "", "image alpine:latest not allowed by any policy"}, + // An invalid or undecodable name is denied. + {"name=-beacon", lodestar, ActionDeny, "", "invalid container name"}, + {"name=bea%2Fcon", lodestar, ActionDeny, "", "invalid container name"}, + // Decoded as the daemon's r.Form does. + {"name=%62eacon", lodestar, ActionCreateContainer, "beacon", ""}, + {"name=validator%2D2", lodestar, ActionCreateContainer, "validator", ""}, + // The daemon takes the first of a repeated name; the proxy denies it. + {"name=beacon&name=monitoring", lodestar, ActionDeny, "", "duplicate name query parameter"}, + {"name=beacon&n%61me=validator", lodestar, ActionDeny, "", "duplicate name query parameter"}, + // A query the daemon rejects is denied, as for the pull (#83). + {"name=%zz", lodestar, ActionDeny, "", "invalid query string"}, + {"platform=%zz&name=beacon", lodestar, ActionDeny, "", "invalid query string"}, + {"name=beacon;x=1", lodestar, ActionDeny, "", "invalid query string"}, + } + for _, tt := range tests { + t.Run(tt.query, func(t *testing.T) { + got := r.Route("POST", "/v1.45/containers/create?"+tt.query, tt.body) + if got.Action != tt.want { + t.Fatalf("create ?%s = %v, want %v (deny msg: %q)", tt.query, got.Action, tt.want, got.DenyMsg) + } + if got.Service != tt.service { + t.Fatalf("create ?%s service = %q, want %q", tt.query, got.Service, tt.service) + } + if got.DenyMsg != tt.wantMsg { + t.Fatalf("create ?%s deny msg = %q, want %q", tt.query, got.DenyMsg, tt.wantMsg) + } + }) + } +} + +// TestRouteCreateSelectionIsStable repeats the #85 reproduction: with two +// policies for one image, every create gets its own policy, every time. +func TestRouteCreateSelectionIsStable(t *testing.T) { + r := NewRouter(newTestManager(t, lodestarPolicies)) + body := map[string]interface{}{"Image": "chainsafe/lodestar:v1.20.0"} + for i := 0; i < 50; i++ { + for _, name := range []string{"beacon", "validator"} { + got := r.Route("POST", "/containers/create?name="+name, body) + if got.Action != ActionCreateContainer || got.Service != name { + t.Fatalf("iteration %d: create ?name=%s got %v service %q (%q)", i, name, got.Action, got.Service, got.DenyMsg) + } + } + } +} + +// TestRouteCreateAmbiguous: two policies that both accept one image and name +// are refused at load time; built without that check, the router denies. +func TestRouteCreateAmbiguous(t *testing.T) { + m := policy.NewManagerFromPolicies( + &policy.Policy{ServiceName: "b", AllowedImagePrefixes: []string{"chainsafe/lodestar"}}, + &policy.Policy{ServiceName: "a", AllowedImagePrefixes: []string{"chainsafe"}, ContainerNames: []string{"beacon"}}, + ) + r := NewRouter(m) + body := map[string]interface{}{"Image": "chainsafe/lodestar:v1"} + got := r.Route("POST", "/containers/create?name=beacon", body) + want := "ambiguous policy for image chainsafe/lodestar:v1 and name beacon: a, b" + if got.Action != ActionDeny || got.DenyMsg != want { + t.Fatalf("got %v %q, want deny %q", got.Action, got.DenyMsg, want) + } + got = r.Route("POST", "/containers/create?name=validator", body) + if got.Action != ActionCreateContainer || got.Service != "b" { + t.Fatalf("name=validator: got %v %q (%q), want b", got.Action, got.Service, got.DenyMsg) + } +} + +// TestRouteLifecycleCoverage is the cross-language parity guard for #86. +// +// A lifecycle action or DELETE is allowed directly only on a name that +// exactly one policy covers (container_names, or service_name when unset). +// Any other target is routed for resolution: the handler asks the daemon +// which container it is. Rows mirror the covered* and uncovered* runs in +// spec/router.qnt. +func TestRouteLifecycleCoverage(t *testing.T) { + files := map[string]string{ + "validator.yaml": lodestarPolicies["validator.yaml"], + "monitoring.yaml": lodestarPolicies["monitoring.yaml"], + "vc.yaml": ` +service_name: vc-svc +container_names: [vc-1] +allowed_image_prefixes: + - example/vc +`, + "cafe.yaml": ` +service_name: cafe +allowed_image_prefixes: + - cafe/app +`, + } + r := NewRouter(newTestManager(t, files)) + + tests := []struct { + method string + path string + want Action + service string + }{ + // coveredNameStartAllowedTest + {"POST", "/containers/validator/start", ActionAllow, "validator"}, + {"POST", "/v1.45/containers/validator-2/kill", ActionAllow, "validator"}, + {"DELETE", "/containers/validator", ActionAllow, "validator"}, + // A policy without container_names covers its service_name. + {"POST", "/containers/monitoring/restart", ActionAllow, "monitoring"}, + // With container_names set, service_name is not covered. + {"POST", "/containers/vc-svc/stop", ActionResolveContainer, ""}, + {"POST", "/containers/vc-1/stop", ActionAllow, "vc-svc"}, + // uncoveredNameStopDeniedTest: an operator container, resolved first. + {"POST", "/containers/mycontainer/stop", ActionResolveContainer, ""}, + // An ID (full or short) is resolved by the daemon. + {"POST", "/containers/4f6e1d2c9a7b/kill", ActionResolveContainer, ""}, + {"DELETE", "/containers/4f6e1d2c9a7b8c3d4e5f60718293a4b5c6d7e8f9012345678901234567890abc", ActionResolveContainer, ""}, + // A covered name that could also be an ID prefix is resolved too. + {"POST", "/containers/cafe/start", ActionResolveContainer, ""}, + // Reads are not lifecycle actions. + {"GET", "/containers/mycontainer/json", ActionAllow, ""}, + } + for _, tt := range tests { + t.Run(tt.method+" "+tt.path, func(t *testing.T) { + got := r.Route(tt.method, tt.path, nil) + if got.Action != tt.want { + t.Fatalf("Route(%s, %s) = %v, want %v (deny msg: %q)", tt.method, tt.path, got.Action, tt.want, got.DenyMsg) + } + if got.Service != tt.service { + t.Fatalf("Route(%s, %s) service = %q, want %q", tt.method, tt.path, got.Service, tt.service) + } + }) + } + + // --readonly still denies first (#75). + ro := NewRouter(newTestManager(t, files)).WithReadonly(true) + if got := ro.Route("POST", "/containers/validator/start", nil); got.Action != ActionDeny { + t.Fatalf("readonly start: got %v, want deny", got.Action) + } +} + +// TestRoutePullSharedImage: several policies may allow one image once +// container_names tells their creates apart (#85). A pull creates no +// container, so it is allowed when any policy for the image accepts its tag +// or digest, and denied with the first policy's reason (sorted by +// service_name) when none does. +func TestRoutePullSharedImage(t *testing.T) { + r := NewRouter(newTestManager(t, map[string]string{ + "vc-b.yaml": "service_name: vc-b\ncontainer_names: [vc-b]\nallowed_image_prefixes: [example/vc]\nimage_tag_pattern: '^v2$'\n", + "vc-a.yaml": "service_name: vc-a\ncontainer_names: [vc-a]\nallowed_image_prefixes: [example/vc]\nimage_tag_pattern: '^v1$'\n", + })) + tests := []struct { + tag string + want Action + service string + wantMsg string + }{ + {"v1", ActionAllow, "vc-a", ""}, + {"v2", ActionAllow, "vc-b", ""}, + {"v3", ActionDeny, "", `image tag "v3" does not match pattern "^v1$"`}, + } + for i := 0; i < 50; i++ { + for _, tt := range tests { + got := r.Route("POST", "/images/create?fromImage=example%2Fvc&tag="+tt.tag, nil) + if got.Action != tt.want || got.Service != tt.service || got.DenyMsg != tt.wantMsg { + t.Fatalf("iteration %d: pull tag %s = %v %q %q, want %v %q %q", i, tt.tag, got.Action, got.Service, got.DenyMsg, tt.want, tt.service, tt.wantMsg) + } + } + } +} diff --git a/rs/src/audit.rs b/rs/src/audit.rs index 12c727d..7ea69c8 100644 --- a/rs/src/audit.rs +++ b/rs/src/audit.rs @@ -25,6 +25,10 @@ impl AuditLogger { self.log("ALLOW", method, uri, ""); } + pub fn allow_reason(&self, method: &str, uri: &str, reason: &str) { + self.log("ALLOW", method, uri, reason); + } + pub fn deny(&self, method: &str, uri: &str, reason: &str) { self.log("DENY", method, uri, reason); } diff --git a/rs/src/handler.rs b/rs/src/handler.rs index f6ce688..5fc4c45 100644 --- a/rs/src/handler.rs +++ b/rs/src/handler.rs @@ -1,6 +1,6 @@ use crate::audit::AuditLogger; use crate::middleware::Chain; -use crate::proxy::{Action, Router}; +use crate::proxy::{Action, RouteResult, Router}; use crate::transport::Transport; use crate::transport::TransportError; use bytes::Bytes; @@ -89,14 +89,26 @@ impl Handler { // Route on the raw target: the path still percent-encoded (#53), and the // query, which the image pull reads (#83). - let route = self + let mut route = self .router .route_request(&method, &target, !body_bytes.is_empty(), body_json.as_ref()); + // A lifecycle action or DELETE on a name that no policy covers + // directly: ask the daemon which container it is (#86). + if route.action == Action::ResolveContainer { + let id = route.container.clone().unwrap_or_default(); + route = self.resolve_container(&id).await; + } + if route.action == Action::Deny { let msg = route.deny_msg.unwrap_or_default(); warn!("denied {} {}: {}", method, path, msg); - self.audit.deny(&method, &target, &msg); + match &route.resolved_name { + Some(name) => self + .audit + .deny(&method, &target, &format!("{} (resolved to {})", msg, name)), + None => self.audit.deny(&method, &target, &msg), + } return Response::builder() .status(StatusCode::FORBIDDEN) .body(Full::new(Bytes::from(msg))) @@ -121,7 +133,18 @@ impl Handler { } info!("allowed {} {}", method, path); - self.audit.allow(&method, &target); + match &route.resolved_name { + Some(name) => self.audit.allow_reason( + &method, + &target, + &format!( + "container {} resolved to {}", + route.container.as_deref().unwrap_or(""), + name + ), + ), + None => self.audit.allow(&method, &target), + } let mut forwarded = Request::builder() .method(method.as_str()) @@ -154,6 +177,97 @@ impl Handler { } } +impl Handler { + /// Asks the daemon for GET /containers/{id}/json and allows the request + /// only if the container's name is covered by a policy. A 404, an + /// uncovered name or any other failure denies: it fails closed (#86). + async fn resolve_container(&self, id: &str) -> RouteResult { + let not_managed = |resolved: Option| RouteResult { + resolved_name: resolved, + ..deny_result(format!("container {} is not managed by any policy", id)) + }; + let cannot_resolve = |reason: String| deny_result(format!("cannot resolve container {}: {}", id, reason)); + + let req = match Request::builder() + .method("GET") + .uri(format!("/containers/{}/json", path_escape(id))) + .header(header::HOST, "docker") + .body(Full::new(Bytes::new())) + { + Ok(r) => r, + Err(e) => return cannot_resolve(e.to_string()), + }; + let resp = match self.transport.forward(req).await { + Ok(r) => r, + Err(e) => return cannot_resolve(e.to_string()), + }; + match resp.status() { + StatusCode::OK => {} + StatusCode::NOT_FOUND => return not_managed(None), + other => return cannot_resolve(format!("daemon returned {}", other.as_u16())), + } + let body = match resp.into_body().collect().await { + Ok(c) => c.to_bytes(), + Err(_) => return cannot_resolve("unreadable inspect response".into()), + }; + let name = serde_json::from_slice::(&body) + .ok() + .and_then(|v| v.get("Name").and_then(|n| n.as_str()).map(str::to_string)) + .filter(|n| !n.is_empty()); + let Some(name) = name else { + return cannot_resolve("unreadable inspect response".into()); + }; + let name = name.strip_prefix('/').unwrap_or(&name).to_string(); + + let covering = self.router.manager().covering(&name); + if covering.is_empty() { + return not_managed(Some(name)); + } + let policy = if covering.len() == 1 { + Some(covering[0].clone()) + } else { + None + }; + RouteResult { + action: Action::Allow, + service: policy.as_ref().map(|p| p.service_name.clone()), + policy, + body: None, + container: Some(id.to_string()), + resolved_name: Some(name), + image: None, + deny_msg: None, + } + } +} + +fn deny_result(msg: String) -> RouteResult { + RouteResult { + action: Action::Deny, + service: None, + policy: None, + body: None, + container: None, + resolved_name: None, + image: None, + deny_msg: Some(msg), + } +} + +/// Percent-encodes everything but RFC 3986 unreserved characters, so the id +/// stays one path segment. +fn path_escape(s: &str) -> String { + let mut out = String::with_capacity(s.len()); + for b in s.bytes() { + if b.is_ascii_alphanumeric() || matches!(b, b'-' | b'.' | b'_' | b'~') { + out.push(b as char); + } else { + out.push_str(&format!("%{:02X}", b)); + } + } + out +} + /// An Upgrade header, or a Connection header that lists the upgrade token. fn is_upgrade_request(headers: &hyper::HeaderMap) -> bool { headers.contains_key(header::UPGRADE) @@ -225,6 +339,7 @@ mod tests { "svc".into(), Policy { service_name: "svc".into(), + container_names: None, user_id: None, group_id: None, allowed_image_prefixes: vec!["alpine".into()], @@ -319,6 +434,7 @@ mod tests { "svc".into(), Policy { service_name: "svc".into(), + container_names: None, user_id: None, group_id: None, allowed_image_prefixes: vec!["alpine".into()], @@ -375,6 +491,7 @@ mod tests { "svc".into(), Policy { service_name: "svc".into(), + container_names: None, user_id: None, group_id: None, allowed_image_prefixes: vec!["alpine".into()], @@ -805,4 +922,293 @@ mod tests { ); assert!(captured_uri.lock().unwrap().is_none(), "expected no forward"); } + + const BEACON_ID: &str = "4f6e1d2c9a7b8c3d4e5f60718293a4b5c6d7e8f9012345678901234567890abc"; + const MONITORING_ID: &str = "9d8c7b6a5f4e3d2c1b0a99887766554433221100ffeeddccbbaa998877665544"; + + /// Answers GET /containers/{id}/json from a table of id -> container + /// name, as the daemon does, and records every request. + struct DaemonTransport { + names: HashMap, + status: Option, + requests: Arc>>, + last_body: Arc>>, + } + + #[async_trait] + impl Transport for DaemonTransport { + async fn forward(&self, req: Request>) -> Result>, TransportError> { + let path = req.uri().path().to_string(); + self.requests.lock().unwrap().push(format!("{} {}", req.method(), path)); + if req.method() == Method::GET && path.starts_with("/containers/") && path.ends_with("/json") { + if let Some(status) = self.status { + return Ok(Response::builder() + .status(status) + .body(Full::new(Bytes::from("boom"))) + .unwrap()); + } + let id = &path["/containers/".len()..path.len() - "/json".len()]; + return Ok(match self.names.get(id) { + Some(name) => Response::builder() + .status(StatusCode::OK) + .body(Full::new(Bytes::from( + serde_json::json!({"Id": "4f6e1d2c9a7b", "Name": format!("/{name}")}).to_string(), + ))) + .unwrap(), + None => Response::builder() + .status(StatusCode::NOT_FOUND) + .body(Full::new(Bytes::from("no such container"))) + .unwrap(), + }); + } + let body = req.into_body().collect().await.unwrap().to_bytes(); + *self.last_body.lock().unwrap() = Some(body); + Ok(Response::builder() + .status(StatusCode::NO_CONTENT) + .body(Full::new(Bytes::new())) + .unwrap()) + } + } + + struct DaemonHandle { + requests: Arc>>, + last_body: Arc>>, + log_path: std::path::PathBuf, + } + + fn make_daemon_handler(files: &[(&str, &str)], status: Option) -> (Handler, DaemonHandle) { + let names: HashMap = [ + ("beacon", "beacon"), + (BEACON_ID, "beacon"), + (&BEACON_ID[..12], "beacon"), + ("dsp-monitoring", "dsp-monitoring"), + (MONITORING_ID, "dsp-monitoring"), + (&MONITORING_ID[..12], "dsp-monitoring"), + ] + .iter() + .map(|(k, v)| (k.to_string(), v.to_string())) + .collect(); + let requests = Arc::new(std::sync::Mutex::new(Vec::new())); + let last_body = Arc::new(std::sync::Mutex::new(None)); + let transport = DaemonTransport { + names, + status, + requests: requests.clone(), + last_body: last_body.clone(), + }; + let log_path = std::env::temp_dir().join(format!( + "handler-coverage-{}-{}.log", + std::process::id(), + std::time::SystemTime::now() + .duration_since(std::time::UNIX_EPOCH) + .unwrap() + .as_nanos() + )); + let manager = crate::policy::tests::load_dir(files).unwrap(); + let audit = AuditLogger::new(log_path.to_str().unwrap()).unwrap(); + let handler = Handler::new(Arc::new(Router::new(manager)), Chain::new(), audit, Box::new(transport)); + ( + handler, + DaemonHandle { + requests, + last_body, + log_path, + }, + ) + } + + const BEACON_ONLY: [(&str, &str); 1] = [( + "beacon.yaml", + "service_name: beacon\nallowed_image_prefixes:\n - chainsafe/lodestar\n", + )]; + + async fn send(handler: &Handler, method: &str, target: &str, body: &str) -> (StatusCode, String) { + let req = Request::builder() + .method(method) + .uri(format!("http://localhost{target}")) + .body(Full::new(Bytes::from(body.to_string()))) + .unwrap(); + let resp = handler.handle(req).await; + let status = resp.status(); + let body = resp.into_body().collect().await.unwrap().to_bytes(); + (status, String::from_utf8_lossy(&body).into_owned()) + } + + /// The handler half of #86: a lifecycle action or DELETE is forwarded + /// only if its target is a container a policy covers. A name, full ID or + /// short ID is resolved with GET /containers/{id}/json. Before #86 every + /// row was forwarded. + #[tokio::test] + async fn test_handler_lifecycle_coverage() { + let not_managed = |id: &str| format!("container {id} is not managed by any policy"); + let inspect = |id: &str| vec![format!("GET /containers/{id}/json")]; + let beacon_short = &BEACON_ID[..12]; + let mon_short = &MONITORING_ID[..12]; + let cases: Vec<(&str, &str, String, StatusCode, String, Vec)> = vec![ + ( + "covered name", + "POST", + "/v1.45/containers/beacon/stop?t=1".into(), + StatusCode::NO_CONTENT, + String::new(), + vec![], + ), + ( + "covered full ID", + "POST", + format!("/containers/{BEACON_ID}/kill"), + StatusCode::NO_CONTENT, + String::new(), + inspect(BEACON_ID), + ), + ( + "covered short ID", + "DELETE", + format!("/containers/{beacon_short}?force=1"), + StatusCode::NO_CONTENT, + String::new(), + inspect(beacon_short), + ), + // uncoveredNameStopDeniedTest: an operator container. + ( + "uncovered name", + "POST", + "/containers/dsp-monitoring/stop".into(), + StatusCode::FORBIDDEN, + not_managed("dsp-monitoring"), + inspect("dsp-monitoring"), + ), + ( + "uncovered full ID", + "POST", + format!("/containers/{MONITORING_ID}/kill"), + StatusCode::FORBIDDEN, + not_managed(MONITORING_ID), + inspect(MONITORING_ID), + ), + ( + "uncovered short ID", + "DELETE", + format!("/containers/{mon_short}"), + StatusCode::FORBIDDEN, + not_managed(mon_short), + inspect(mon_short), + ), + // realNameDeleteDeniedTest: the daemon answers 404. + ( + "unknown name", + "DELETE", + "/containers/mycontainer".into(), + StatusCode::FORBIDDEN, + not_managed("mycontainer"), + inspect("mycontainer"), + ), + // execPrefixNameStartDeniedTest + ( + "unknown exec-prefixed name", + "POST", + "/containers/exec-runner/start".into(), + StatusCode::FORBIDDEN, + not_managed("exec-runner"), + inspect("exec-runner"), + ), + ]; + let mut mismatches = Vec::new(); + for (name, method, target, status, body, inspects) in cases { + let (handler, d) = make_daemon_handler(&BEACON_ONLY, None); + let (got_status, got_body) = send(&handler, method, &target, "").await; + let _ = std::fs::remove_file(&d.log_path); + if got_status != status || (!body.is_empty() && got_body != body) { + mismatches.push(format!("{name}: got {got_status} {got_body:?}, want {status} {body:?}")); + } + let mut want = inspects; + if status != StatusCode::FORBIDDEN { + want.push(format!("{method} {}", target.split('?').next().unwrap())); + } + let seen = d.requests.lock().unwrap().clone(); + if seen != want { + mismatches.push(format!("{name}: daemon saw {seen:?}, want {want:?}")); + } + } + assert!(mismatches.is_empty(), "mismatches:\n{}", mismatches.join("\n")); + } + + /// A daemon error other than 404 fails closed with its own message (#86). + #[tokio::test] + async fn test_handler_lifecycle_resolve_fails_closed() { + let (handler, d) = make_daemon_handler(&BEACON_ONLY, Some(StatusCode::INTERNAL_SERVER_ERROR)); + let short = &BEACON_ID[..12]; + let (status, body) = send(&handler, "POST", &format!("/containers/{short}/stop"), "").await; + let _ = std::fs::remove_file(&d.log_path); + assert_eq!(status, StatusCode::FORBIDDEN); + assert_eq!(body, format!("cannot resolve container {short}: daemon returned 500")); + assert_eq!(d.requests.lock().unwrap().len(), 1, "only the inspect"); + } + + /// The resolved name is audited with the request (#86). + #[tokio::test] + async fn test_handler_lifecycle_audits_resolved_name() { + let (handler, d) = make_daemon_handler(&BEACON_ONLY, None); + send(&handler, "POST", &format!("/containers/{}/start", &BEACON_ID[..12]), "").await; + send( + &handler, + "POST", + &format!("/containers/{}/start", &MONITORING_ID[..12]), + "", + ) + .await; + let contents = std::fs::read_to_string(&d.log_path).unwrap(); + let _ = std::fs::remove_file(&d.log_path); + let lines: Vec = contents.lines().map(|l| serde_json::from_str(l).unwrap()).collect(); + assert_eq!(lines.len(), 2, "{contents}"); + assert_eq!(lines[0]["decision"], "ALLOW"); + assert_eq!( + lines[0]["reason"], + format!("container {} resolved to beacon", &BEACON_ID[..12]) + ); + assert_eq!(lines[1]["decision"], "DENY"); + assert_eq!( + lines[1]["reason"], + format!( + "container {} is not managed by any policy (resolved to dsp-monitoring)", + &MONITORING_ID[..12] + ) + ); + } + + /// The #85 reproduction through the handler: two policies for one image + /// force different users, and each create named for one of them gets + /// that policy's user, every time. + #[tokio::test] + async fn test_handler_create_selects_by_name() { + let files = [ + ( + "a.yaml", + "service_name: svc-a\ncontainer_names: [svc-a]\nallowed_image_prefixes: [alpine]\ncontainer_config: { user: \"1111:1111\" }\n", + ), + ( + "b.yaml", + "service_name: svc-b\ncontainer_names: [svc-b]\nallowed_image_prefixes: [alpine]\ncontainer_config: { user: \"2222:2222\" }\n", + ), + ]; + let (handler, d) = make_daemon_handler(&files, None); + let body = r#"{"Image":"alpine:3.20","Cmd":["true"]}"#; + for i in 0..50 { + for (name, user) in [("svc-a", "1111:1111"), ("svc-b", "2222:2222")] { + *d.last_body.lock().unwrap() = None; + let (status, msg) = + send(&handler, "POST", &format!("/v1.45/containers/create?name={name}"), body).await; + assert_eq!(status, StatusCode::NO_CONTENT, "iteration {i} {name}: {msg}"); + let fwd: serde_json::Value = + serde_json::from_slice(d.last_body.lock().unwrap().as_ref().unwrap()).unwrap(); + assert_eq!(fwd["User"], user, "iteration {i}: create ?name={name}"); + } + } + let (status, msg) = send(&handler, "POST", "/containers/create", r#"{"Image":"alpine:3.20"}"#).await; + let _ = std::fs::remove_file(&d.log_path); + assert_eq!( + (status, msg.as_str()), + (StatusCode::FORBIDDEN, "image alpine:3.20 requires a container name") + ); + } } diff --git a/rs/src/middleware.rs b/rs/src/middleware.rs index d8621a9..b282653 100644 --- a/rs/src/middleware.rs +++ b/rs/src/middleware.rs @@ -982,6 +982,7 @@ impl Mutator for ContainerConfigMutator { fn make_policy(name: &str, prefixes: Vec<&str>) -> Policy { Policy { service_name: name.to_string(), + container_names: None, user_id: None, group_id: None, allowed_image_prefixes: prefixes.iter().map(|s| s.to_string()).collect(), diff --git a/rs/src/policy.rs b/rs/src/policy.rs index 8e42e87..ac76fcb 100644 --- a/rs/src/policy.rs +++ b/rs/src/policy.rs @@ -1,10 +1,11 @@ use serde::Deserialize; -use std::collections::HashMap; +use std::collections::{BTreeMap, HashMap}; use std::sync::OnceLock; #[derive(Debug, Clone, Deserialize)] pub struct Policy { pub service_name: String, + pub container_names: Option>, pub user_id: Option, pub group_id: Option, pub allowed_image_prefixes: Vec, @@ -20,6 +21,38 @@ pub struct Policy { pub denied_flags: Option>, } +impl Policy { + /// The container names the policy manages: its container_names, or its + /// service_name when container_names is unset (#85, #86). Lifecycle + /// actions and DELETE are forwarded only for covered names. + pub fn covered_names(&self) -> Vec<&str> { + match &self.container_names { + Some(names) => names.iter().map(|s| s.as_str()).collect(), + None => vec![self.service_name.as_str()], + } + } + + /// Whether a create named `name` may use the policy: a policy without + /// container_names accepts any name, including none (#85). + pub fn accepts_name(&self, name: &str) -> bool { + match &self.container_names { + Some(names) => names.iter().any(|n| n == name), + None => true, + } + } +} + +/// A Docker container name without the optional leading slash (#85): +/// `[a-zA-Z0-9][a-zA-Z0-9_.-]*`. +pub fn valid_container_name(name: &str) -> bool { + let mut bytes = name.bytes(); + match bytes.next() { + Some(b) if b.is_ascii_alphanumeric() => {} + _ => return false, + } + bytes.all(|b| b.is_ascii_alphanumeric() || b == b'_' || b == b'.' || b == b'-') +} + #[derive(Debug, Clone, Deserialize)] pub struct ContainerConfig { pub network_mode: Option, @@ -44,18 +77,23 @@ pub struct FlagRule { } pub struct Manager { - policies_by_name: HashMap, + // Sorted by service_name, so selection never depends on map iteration + // order (#85). + policies_by_name: BTreeMap, } impl Manager { + /// Builds a manager without load-time validation, as Go's + /// NewManagerFromPolicies and TypeScript's Manager.fromPolicies do. Tests + /// use it to reach states that `new` refuses, such as ambiguity. pub fn from_map(policies: HashMap) -> Self { Manager { - policies_by_name: policies, + policies_by_name: policies.into_iter().collect(), } } pub fn new(config_dir: &str) -> Result> { - let mut policies_by_name = HashMap::new(); + let mut policies_by_name = BTreeMap::new(); let entries = match std::fs::read_dir(config_dir) { Ok(d) => d, Err(e) if e.kind() == std::io::ErrorKind::NotFound => { @@ -68,12 +106,22 @@ impl Manager { Err(e) => return Err(e.into()), }; + let mut paths = Vec::new(); for entry in entries { - let entry = entry?; - let path = entry.path(); + let path = entry?.path(); if path.extension().map_or(true, |e| e != "yaml" && e != "yml") { continue; } + paths.push(path); + } + paths.sort(); + + let mut file_of: HashMap = HashMap::new(); + for path in paths { + let file = path + .file_name() + .map(|f| f.to_string_lossy().into_owned()) + .unwrap_or_default(); let data = std::fs::read_to_string(&path)?; let policy: Policy = serde_yaml::from_str(&data)?; if policy.service_name.is_empty() { @@ -82,6 +130,28 @@ impl Manager { if policy.allowed_image_prefixes.is_empty() { return Err("missing allowed_image_prefixes".into()); } + if let Some(names) = &policy.container_names { + if names.is_empty() { + return Err(format!("policy file {}: container_names must not be empty", file).into()); + } + if let Some(bad) = names.iter().find(|n| !valid_container_name(n)) { + return Err(format!( + "policy file {}: invalid container name {:?} in container_names", + file, bad + ) + .into()); + } + } + // A second file with the same service_name used to replace the + // first silently (#85). + if let Some(prev) = file_of.get(&policy.service_name) { + return Err(format!( + "policies in {} and {} both use service_name {:?}", + prev, file, policy.service_name + ) + .into()); + } + file_of.insert(policy.service_name.clone(), file); // Prefixes are compared in normalised form (#84). let mut policy = policy; let mut prefixes = Vec::with_capacity(policy.allowed_image_prefixes.len()); @@ -99,7 +169,42 @@ impl Manager { policies_by_name.insert(policy.service_name.clone(), policy); } - Ok(Manager { policies_by_name }) + let m = Manager { policies_by_name }; + m.check_conflicts(&file_of)?; + Ok(m) + } + + /// Fails when two policies could both be selected for one create: their + /// image prefixes share a namespace, and one of them accepts any name or + /// their container_names intersect (#85). + fn check_conflicts(&self, file_of: &HashMap) -> Result<(), String> { + let ordered: Vec<&Policy> = self.policies_by_name.values().collect(); + for (i, a) in ordered.iter().enumerate() { + for b in &ordered[i + 1..] { + let Some((pa, pb)) = overlapping_prefixes(&a.allowed_image_prefixes, &b.allowed_image_prefixes) else { + continue; + }; + let reason = match (&a.container_names, &b.container_names) { + (None, _) => format!("{:?} has no container_names", a.service_name), + (_, None) => format!("{:?} has no container_names", b.service_name), + (Some(an), Some(bn)) => match an.iter().filter(|n| bn.contains(n)).min() { + Some(shared) => format!("both accept container name {:?}", shared), + None => continue, + }, + }; + return Err(format!( + "conflicting policies {:?} ({}) and {:?} ({}): image prefixes {:?} and {:?} overlap and {}", + a.service_name, + file_of.get(&a.service_name).map(String::as_str).unwrap_or(""), + b.service_name, + file_of.get(&b.service_name).map(String::as_str).unwrap_or(""), + pa, + pb, + reason + )); + } + } + Ok(()) } pub fn get(&self, name: &str) -> Option<&Policy> { @@ -110,9 +215,53 @@ impl Manager { /// (#84). An invalid reference matches nothing. pub fn get_by_image(&self, image_ref: &str) -> Option<&Policy> { let name = parse_image_ref(image_ref).ok()?.name; + self.matching_image(&name).into_iter().next() + } + + /// The policies, sorted by service_name, whose prefixes match a + /// normalised repository name (#85). + pub fn matching_image(&self, name: &str) -> Vec<&Policy> { self.policies_by_name .values() - .find(|p| matches_image_prefix(&name, &p.allowed_image_prefixes)) + .filter(|p| matches_image_prefix(name, &p.allowed_image_prefixes)) + .collect() + } + + /// The one policy whose image prefixes match the normalised repository + /// of `image_ref` and which accepts the container name (empty for a + /// create without ?name=). No match, or more than one, is an error whose + /// text is the deny message (#85). + pub fn select_for_create(&self, image_ref: &str, name: &str) -> Result<&Policy, String> { + let image_name = parse_image_ref(image_ref)?.name; + let matching = self.matching_image(&image_name); + let candidates: Vec<&Policy> = matching.iter().copied().filter(|p| p.accepts_name(name)).collect(); + match candidates.as_slice() { + [p] => Ok(p), + _ if matching.is_empty() => Err(format!("image {} not allowed by any policy", image_ref)), + [] if name.is_empty() => Err(format!("image {} requires a container name", image_ref)), + [] => Err(format!( + "container name {} not allowed for image {} by any policy", + name, image_ref + )), + many => Err(format!( + "ambiguous policy for image {} and name {}: {}", + image_ref, + name, + many.iter() + .map(|p| p.service_name.as_str()) + .collect::>() + .join(", ") + )), + } + } + + /// The policies, sorted by service_name, whose covered names include + /// `name` (#86). + pub fn covering(&self, name: &str) -> Vec<&Policy> { + self.policies_by_name + .values() + .filter(|p| p.covered_names().contains(&name)) + .collect() } pub fn list(&self) -> Vec<&str> { @@ -120,6 +269,19 @@ impl Manager { } } +// The first pair of prefixes where one equals the other or is a namespace (/) +// prefix of it. +fn overlapping_prefixes<'a>(a: &'a [String], b: &'a [String]) -> Option<(&'a str, &'a str)> { + for pa in a { + for pb in b { + if pa == pb || pa.starts_with(&format!("{}/", pb)) || pb.starts_with(&format!("{}/", pa)) { + return Some((pa, pb)); + } + } + } + None +} + /// A parsed image reference (#84). `name` is the normalised repository: the /// registry host, lower-cased, and the path; Docker Hub's host is dropped, /// and so is `library/` before a single-component name, so `alpine`, @@ -268,7 +430,7 @@ pub fn check_image_tag(policy: &Policy, r: &ImageRef) -> Result<(), String> { } #[cfg(test)] -mod tests { +pub(crate) mod tests { use super::*; #[test] @@ -435,6 +597,7 @@ denied_flags: "svc1".into(), Policy { service_name: "svc1".into(), + container_names: None, user_id: None, group_id: None, allowed_image_prefixes: vec!["alpine".into()], @@ -462,6 +625,7 @@ denied_flags: "svc1".into(), Policy { service_name: "svc1".into(), + container_names: None, user_id: None, group_id: None, allowed_image_prefixes: vec!["alpine".into()], @@ -489,6 +653,7 @@ denied_flags: "svc1".into(), Policy { service_name: "svc1".into(), + container_names: None, user_id: None, group_id: None, allowed_image_prefixes: vec!["myregistry.io/team".into()], @@ -522,6 +687,7 @@ denied_flags: "svc1".into(), Policy { service_name: "svc1".into(), + container_names: None, user_id: None, group_id: None, allowed_image_prefixes: vec!["alpine".into()], @@ -548,6 +714,7 @@ denied_flags: "a".into(), Policy { service_name: "a".into(), + container_names: None, user_id: None, group_id: None, allowed_image_prefixes: vec!["img".into()], @@ -566,6 +733,7 @@ denied_flags: "b".into(), Policy { service_name: "b".into(), + container_names: None, user_id: None, group_id: None, allowed_image_prefixes: vec!["img".into()], @@ -629,4 +797,207 @@ value_pattern: ^\d+[kmg]$ assert_eq!(rule.flag, "--memory"); assert_eq!(rule.value_pattern, "^\\d+[kmg]$"); } + + /// Writes policy files into a fresh directory and loads it. + pub(crate) fn load_dir(files: &[(&str, &str)]) -> Result { + use std::sync::atomic::{AtomicUsize, Ordering}; + static N: AtomicUsize = AtomicUsize::new(0); + let dir = std::env::temp_dir().join(format!( + "dsp-policy-test-{}-{}", + std::process::id(), + N.fetch_add(1, Ordering::SeqCst) + )); + std::fs::create_dir_all(&dir).unwrap(); + // Listener tests change the process umask concurrently; set the mode. + std::fs::set_permissions(&dir, std::os::unix::fs::PermissionsExt::from_mode(0o700)).unwrap(); + for (name, content) in files { + std::fs::write(dir.join(name), content).unwrap(); + } + let result = Manager::new(dir.to_str().unwrap()).map_err(|e| e.to_string()); + std::fs::remove_dir_all(&dir).unwrap(); + result + } + + fn policy(service: &str, names: Option<&[&str]>, prefixes: &[&str]) -> Policy { + Policy { + service_name: service.into(), + container_names: names.map(|n| n.iter().map(|s| s.to_string()).collect()), + user_id: None, + group_id: None, + allowed_image_prefixes: prefixes.iter().map(|s| s.to_string()).collect(), + image_tag_pattern: None, + image_digest_allowed: false, + container_config: None, + volumes: None, + ports: None, + env_file: None, + allowed_cli_flags: None, + flag_rules: None, + denied_flags: None, + } + } + + fn manager_of(policies: Vec) -> Manager { + Manager::from_map(policies.into_iter().map(|p| (p.service_name.clone(), p)).collect()) + } + + /// The cross-language parity guard for the #85 load-time checks. Two + /// policies conflict when their image prefixes share a namespace and one + /// has no container_names or their container_names intersect. Before #85 + /// every row loaded, and a duplicate service_name silently replaced the + /// first file. + #[test] + fn test_manager_load_time_validation() { + let cases: Vec<(&str, Vec<(&str, &str)>, &str)> = vec![ + ( + "same prefix, no container_names", + vec![ + ("a.yaml", "service_name: a\nallowed_image_prefixes: [alpine]\n"), + ("b.yaml", "service_name: b\nallowed_image_prefixes: [alpine]\n"), + ], + r#"conflicting policies "a" (a.yaml) and "b" (b.yaml): image prefixes "alpine" and "alpine" overlap and "a" has no container_names"#, + ), + ( + "one side has no container_names", + vec![ + ( + "a.yaml", + "service_name: a\ncontainer_names: [x]\nallowed_image_prefixes: [chainsafe/lodestar]\n", + ), + ("b.yaml", "service_name: b\nallowed_image_prefixes: [chainsafe]\n"), + ], + r#"conflicting policies "a" (a.yaml) and "b" (b.yaml): image prefixes "chainsafe/lodestar" and "chainsafe" overlap and "b" has no container_names"#, + ), + ( + "shared container name", + vec![ + ( + "a.yaml", + "service_name: a\ncontainer_names: [x, y]\nallowed_image_prefixes: [chainsafe/lodestar]\n", + ), + ( + "b.yaml", + "service_name: b\ncontainer_names: [z, y]\nallowed_image_prefixes: [chainsafe/lodestar]\n", + ), + ], + r#"conflicting policies "a" (a.yaml) and "b" (b.yaml): image prefixes "chainsafe/lodestar" and "chainsafe/lodestar" overlap and both accept container name "y""#, + ), + ( + "disjoint container names", + vec![ + ( + "a.yaml", + "service_name: a\ncontainer_names: [beacon]\nallowed_image_prefixes: [chainsafe/lodestar]\n", + ), + ( + "b.yaml", + "service_name: b\ncontainer_names: [validator]\nallowed_image_prefixes: [chainsafe/lodestar]\n", + ), + ], + "", + ), + ( + "sibling prefixes do not overlap", + vec![ + ( + "a.yaml", + "service_name: a\nallowed_image_prefixes: [chainsafe/lodestar]\n", + ), + ( + "b.yaml", + "service_name: b\nallowed_image_prefixes: [chainsafe/lodestar-backup]\n", + ), + ], + "", + ), + ( + "duplicate service_name", + vec![ + ("a.yaml", "service_name: svc\nallowed_image_prefixes: [alpine]\n"), + ("b.yaml", "service_name: svc\nallowed_image_prefixes: [busybox]\n"), + ], + r#"policies in a.yaml and b.yaml both use service_name "svc""#, + ), + ( + "invalid container name", + vec![( + "a.yaml", + "service_name: a\ncontainer_names: [-bad]\nallowed_image_prefixes: [alpine]\n", + )], + r#"invalid container name "-bad" in container_names"#, + ), + ( + "container name with a slash", + vec![( + "a.yaml", + "service_name: a\ncontainer_names: [a/b]\nallowed_image_prefixes: [alpine]\n", + )], + r#"invalid container name "a/b" in container_names"#, + ), + ( + "empty container_names", + vec![( + "a.yaml", + "service_name: a\ncontainer_names: []\nallowed_image_prefixes: [alpine]\n", + )], + "container_names must not be empty", + ), + ]; + let mut mismatches = Vec::new(); + for (name, files, want) in cases { + match (load_dir(&files), want) { + (Ok(_), "") => {} + (Ok(_), want) => mismatches.push(format!("{name}: loaded, want error {want:?}")), + (Err(e), "") => mismatches.push(format!("{name}: {e}")), + (Err(e), want) if !e.contains(want) => mismatches.push(format!("{name}: error {e:?}, want {want:?}")), + _ => {} + } + } + assert!(mismatches.is_empty(), "{}", mismatches.join("\n")); + } + + /// #85 selection: the one policy matching the image and accepting the + /// name, the nameless create, and ambiguity. + #[test] + fn test_manager_select_for_create() { + let m = manager_of(vec![ + policy("validator", Some(&["validator"]), &["chainsafe/lodestar"]), + policy("beacon", Some(&["beacon"]), &["chainsafe/lodestar"]), + policy("any", None, &["chainsafe"]), + ]); + for _ in 0..50 { + for name in ["beacon", "validator"] { + let want = format!("ambiguous policy for image chainsafe/lodestar:v1 and name {name}: any, {name}"); + assert_eq!(m.select_for_create("chainsafe/lodestar:v1", name).err(), Some(want)); + } + } + assert_eq!( + m.select_for_create("chainsafe/lodestar:v1", "").unwrap().service_name, + "any" + ); + + let m = manager_of(vec![ + policy("validator", Some(&["validator"]), &["chainsafe/lodestar"]), + policy("beacon", Some(&["beacon"]), &["chainsafe/lodestar"]), + ]); + for _ in 0..50 { + for name in ["beacon", "validator"] { + assert_eq!( + m.select_for_create("chainsafe/lodestar:v1", name).unwrap().service_name, + name + ); + } + } + } + + #[test] + fn test_manager_covering() { + let m = manager_of(vec![ + policy("beacon", None, &["chainsafe/lodestar"]), + policy("vc", Some(&["validator"]), &["chainsafe/lodestar"]), + ]); + for (name, want) in [("beacon", 1), ("validator", 1), ("vc", 0), ("other", 0)] { + assert_eq!(m.covering(name).len(), want, "covering({name})"); + } + } } diff --git a/rs/src/proxy.rs b/rs/src/proxy.rs index 5197406..61062b7 100644 --- a/rs/src/proxy.rs +++ b/rs/src/proxy.rs @@ -1,4 +1,4 @@ -use crate::policy::{check_image_tag, parse_image_ref, Manager, Policy}; +use crate::policy::{check_image_tag, parse_image_ref, valid_container_name, Manager, Policy}; use std::collections::HashMap; #[derive(Debug, Clone, PartialEq)] @@ -6,6 +6,10 @@ pub enum Action { Deny, Allow, CreateContainer, + /// A lifecycle action or DELETE on a container that is not directly a + /// covered name. The handler asks the daemon which container it is and + /// allows the request only if its name is covered (#86). + ResolveContainer, } #[derive(Debug, Clone)] @@ -15,6 +19,9 @@ pub struct RouteResult { pub policy: Option, pub body: Option>, pub container: Option, + /// The daemon's name for `container`, set when the handler resolved it + /// (#86). + pub resolved_name: Option, pub image: Option, pub deny_msg: Option, } @@ -32,6 +39,10 @@ impl Router { } } + pub fn manager(&self) -> &Manager { + &self.manager + } + /// Sets read-only mode (--readonly) and returns the router. pub fn with_readonly(mut self, readonly: bool) -> Self { self.readonly = readonly; @@ -99,7 +110,7 @@ impl Router { // Container create if method == "POST" && path == "/containers/create" { - return self.route_create(body); + return self.route_create(body, query); } // Container lifecycle @@ -133,7 +144,7 @@ impl Router { deny(&format!("endpoint {} {} is not allowed", method, path)) } - fn route_create(&self, body: Option<&HashMap>) -> RouteResult { + fn route_create(&self, body: Option<&HashMap>, query: &str) -> RouteResult { let body = match body { Some(b) if !b.is_empty() => b, _ => return deny("empty request body"), @@ -147,37 +158,49 @@ impl Router { return deny(&msg); } - match self.manager.get_by_image(image) { - Some(policy) => RouteResult { + let name = match create_name(query) { + Ok(n) => n, + Err(msg) => return deny(msg), + }; + + match self.manager.select_for_create(image, &name) { + Ok(policy) => RouteResult { action: Action::CreateContainer, service: Some(policy.service_name.clone()), policy: Some(policy.clone()), body: Some(body.clone()), - container: None, + container: if name.is_empty() { None } else { Some(name) }, + resolved_name: None, image: Some(image.to_string()), deny_msg: None, }, - None => deny(&format!("image {} not allowed by any policy", image)), + Err(msg) => deny(&msg), } } + /// Allows a lifecycle action or DELETE on a name that exactly one policy + /// covers. Anything else, including a name that could also be an ID + /// prefix, needs the daemon to say which container it is (#86). fn route_by_name(&self, name: &str) -> RouteResult { - match self.manager.get(name) { - Some(policy) => RouteResult { + let covering = self.manager.covering(name); + match covering.as_slice() { + [policy] if !could_be_id(name) => RouteResult { action: Action::Allow, service: Some(policy.service_name.clone()), - policy: Some(policy.clone()), + policy: Some((*policy).clone()), body: None, - container: None, + container: Some(name.to_string()), + resolved_name: None, image: None, deny_msg: None, }, - None => RouteResult { - action: Action::Allow, + _ => RouteResult { + action: Action::ResolveContainer, service: None, policy: None, body: None, container: Some(name.to_string()), + resolved_name: None, image: None, deny_msg: None, }, @@ -235,22 +258,35 @@ impl Router { if image_ref.tag.is_empty() && image_ref.digest.is_empty() { return deny("image pull without a tag or digest pulls every tag and is not allowed"); } - let policy = match self.manager.get_by_image(&image_ref.name) { - Some(p) => p, - None => return deny(&format!("image {} not allowed by any policy", from_image)), - }; - if let Err(msg) = check_image_tag(policy, &image_ref) { - return deny(&msg); + // Several policies may allow the image once container_names tells + // their creates apart (#85). A pull creates no container, so any of + // them may accept the tag or digest; with none, the first one's + // reason is given. + let matching = self.manager.matching_image(&image_ref.name); + if matching.is_empty() { + return deny(&format!("image {} not allowed by any policy", from_image)); } - RouteResult { - action: Action::Allow, - service: Some(policy.service_name.clone()), - policy: None, - body: None, - container: None, - image: Some(from_image.to_string()), - deny_msg: None, + let mut first_err = None; + for policy in matching { + match check_image_tag(policy, &image_ref) { + Ok(()) => { + return RouteResult { + action: Action::Allow, + service: Some(policy.service_name.clone()), + policy: None, + body: None, + container: None, + resolved_name: None, + image: Some(from_image.to_string()), + deny_msg: None, + } + } + Err(msg) => { + first_err.get_or_insert(msg); + } + } } + deny(&first_err.unwrap_or_default()) } } @@ -305,6 +341,7 @@ fn allow() -> RouteResult { policy: None, body: None, container: None, + resolved_name: None, image: None, deny_msg: None, } @@ -317,11 +354,42 @@ fn deny(msg: &str) -> RouteResult { policy: None, body: None, container: None, + resolved_name: None, image: None, deny_msg: Some(msg.to_string()), } } +/// The ?name= of a create, without the one leading slash the daemon also +/// strips, or "" when there is none. The query is parsed as the daemon parses +/// it, as the image pull does (#83). The error is the deny message for a query +/// the daemon would reject or an invalid name (#85). +fn create_name(query: &str) -> Result { + let params = parse_query(query).ok_or("invalid query string")?; + // The daemon takes the first of a repeated name (r.Form.Get). Deny it + // rather than pick, as the pull does for its parameters. + if params.iter().filter(|(k, _)| k == "name").count() > 1 { + return Err("duplicate name query parameter"); + } + let name = params + .iter() + .find(|(k, _)| k == "name") + .map(|(_, v)| v.as_str()) + .unwrap_or(""); + let name = name.strip_prefix('/').unwrap_or(name); + if !name.is_empty() && !valid_container_name(name) { + return Err("invalid container name"); + } + Ok(name.to_string()) +} + +/// A name of lowercase hex digits only. The daemon looks up an ID, then a +/// name, then an ID prefix, so if no container has this name it would act on +/// the container whose ID starts with it (#86). +fn could_be_id(name: &str) -> bool { + !name.is_empty() && name.bytes().all(|b| b.is_ascii_digit() || (b'a'..=b'f').contains(&b)) +} + // Strips one leading /v/ or /v./ (ASCII digits only); anything else is left as is (#57). fn strip_api_version(path: &str) -> &str { let Some(rest) = path.strip_prefix("/v") else { @@ -398,6 +466,7 @@ mod tests { "myservice".into(), Policy { service_name: "myservice".into(), + container_names: None, user_id: None, group_id: None, allowed_image_prefixes: prefixes.iter().map(|s| s.to_string()).collect(), @@ -626,9 +695,11 @@ mod tests { ("DELETE", "/containers/exec", Action::Deny, Some("exec is not allowed")), // Listing stays allowed, via the GET/HEAD passthrough. ("GET", "/containers/json", Action::Allow, None), - // A real container name is still routed as a container. - // realNameDeleteAllowedTest - ("DELETE", "/containers/mycontainer", Action::Allow, None), + // A real container name is still routed as a container. No policy + // covers it, so the daemon is asked which container it is, and the + // handler denies it (#86). + // realNameDeleteDeniedTest + ("DELETE", "/containers/mycontainer", Action::ResolveContainer, None), ("GET", "/containers/mycontainer", Action::Allow, None), // Reserved words are only reserved in the name position. // reservedInSubpathAllowedTest @@ -709,10 +780,10 @@ mod tests { let create_body: HashMap = serde_json::from_value(serde_json::json!({"Image": "alpine:latest"})).unwrap(); let cases = [ - // #52: dotted version, container lifecycle delete. - ("DELETE", "/v1.43/containers/foo", None, Action::Allow), - // #52: dotted version, container lifecycle start. - ("POST", "/v1.43/containers/beacon/start", None, Action::Allow), + // #52: dotted version, container lifecycle delete (uncovered, #86). + ("DELETE", "/v1.43/containers/foo", None, Action::ResolveContainer), + // #52: dotted version, container lifecycle start (covered). + ("POST", "/v1.43/containers/myservice/start", None, Action::Allow), // #52: dotted version, create with a policy-allowed image. ( "POST", @@ -721,13 +792,13 @@ mod tests { Action::CreateContainer, ), // #52: undotted control — stripped correctly everywhere already. - ("POST", "/v1/containers/beacon/start", None, Action::Allow), + ("POST", "/v1/containers/myservice/start", None, Action::Allow), // #52: the reserved segment survives the strip (#24 parity). ("DELETE", "/v1.43/containers/json", None, Action::Deny), - // #57: undotted version, container lifecycle delete. - ("DELETE", "/v1/containers/foo", None, Action::Allow), + // #57: undotted version, container lifecycle delete (uncovered, #86). + ("DELETE", "/v1/containers/foo", None, Action::ResolveContainer), // #57: multi-digit major version. - ("DELETE", "/v10.0/containers/foo", None, Action::Allow), + ("DELETE", "/v10.0/containers/foo", None, Action::ResolveContainer), // #57: /volumes/ is not a version; the daemon routes this as a volume removal. ("DELETE", "/volumes/containers/foo", None, Action::Deny), // #57: /version/ is not a version prefix. @@ -795,8 +866,8 @@ mod tests { ("DELETE", "/v%31/containers/foo", Action::Deny), // #53: network names that need escaping are denied, GET included. ("GET", "/networks/a%20b", Action::Deny), - // #53 control: a plain name is still routed as a container. - ("DELETE", "/containers/foo", Action::Allow), + // #53 control: a plain name is still routed as a container (#86). + ("DELETE", "/containers/foo", Action::ResolveContainer), ]; let mut mismatches = Vec::new(); for (method, path, want) in cases { @@ -831,10 +902,23 @@ mod tests { ("DELETE", "/containers/mycontainer/exec", None, Action::Deny, exec), // execSubpathPostDeniedTest (#49) ("POST", "/containers/mycontainer/exec", None, Action::Deny, exec), - // execPrefixNameStartAllowedTest (#49): an unknown container. - ("POST", "/containers/exec-runner/start", None, Action::Allow, None), - // execPrefixNameDeleteAllowedTest (#49): an unknown container. - ("DELETE", "/containers/exec-runner", None, Action::Allow, None), + // execPrefixNameStartDeniedTest (#49, #86): an uncovered container, + // routed for resolution; the handler denies it. + ( + "POST", + "/containers/exec-runner/start", + None, + Action::ResolveContainer, + None, + ), + // execPrefixNameDeleteDeniedTest (#49, #86) + ( + "DELETE", + "/containers/exec-runner", + None, + Action::ResolveContainer, + None, + ), // execPrefixNameGetAllowedTest (#49) ("GET", "/containers/exec-runner/json", None, Action::Allow, None), // execNamespaceGetDeniedTest (#49): exec inspect leaks command lines. @@ -862,7 +946,7 @@ mod tests { // A name ending in exec is a plain name (#49, language-only). ("GET", "/containers/myexec/json", None, Action::Allow, None), // A name starting with exec is a plain name (#49, language-only). - ("DELETE", "/containers/executor", None, Action::Allow, None), + ("DELETE", "/containers/executor", None, Action::ResolveContainer, None), // The reserved name, decided by the exec check (#49, language-only). ("GET", "/containers/exec/json", None, Action::Deny, exec), // A top-level name starting with exec is not the exec namespace (#49, language-only). @@ -1648,7 +1732,8 @@ mod tests { fn test_route_container_by_name_no_policy() { let router = Router::new(make_manager(vec!["alpine"])); let result = router.route("POST", "/containers/unknown-container/start", None); - assert_eq!(result.action, Action::Allow); + // Not covered: resolved by the handler, never allowed directly (#86). + assert_eq!(result.action, Action::ResolveContainer); assert!(result.service.is_none()); assert_eq!(result.container, Some("unknown-container".into())); } @@ -1685,4 +1770,360 @@ mod tests { assert!(result.policy.is_none()); assert!(result.body.is_none()); } + + /// The #85 layout: beacon and validator run one image. + pub(crate) const LODESTAR_POLICIES: [(&str, &str); 3] = [ + ( + "beacon.yaml", + "service_name: beacon\ncontainer_names: [beacon]\nallowed_image_prefixes:\n - chainsafe/lodestar\n", + ), + ( + "validator.yaml", + "service_name: validator\ncontainer_names: [validator, validator-2]\nallowed_image_prefixes:\n - chainsafe/lodestar\n", + ), + ( + "monitoring.yaml", + "service_name: monitoring\nallowed_image_prefixes:\n - prom/prometheus\n", + ), + ]; + + fn body_of(v: serde_json::Value) -> HashMap { + serde_json::from_value(v).unwrap() + } + + /// The cross-language parity guard for #85. POST /containers/create + /// selects the one policy whose image prefixes match and whose + /// container_names contain ?name=; a policy without container_names + /// accepts any name. Before #85 the policy was whichever matched the + /// image first in map order (Go, Rust) or load order (TypeScript). + #[test] + fn test_route_create_selects_by_name() { + let router = Router::new(crate::policy::tests::load_dir(&LODESTAR_POLICIES).unwrap()); + let lodestar = body_of(serde_json::json!({"Image": "chainsafe/lodestar:v1.20.0"})); + let prom = body_of(serde_json::json!({"Image": "prom/prometheus:latest"})); + let alpine = body_of(serde_json::json!({"Image": "alpine:latest"})); + let cases: Vec<( + &str, + &HashMap, + Action, + Option<&str>, + Option<&str>, + )> = vec![ + ("name=beacon", &lodestar, Action::CreateContainer, Some("beacon"), None), + ( + "name=validator", + &lodestar, + Action::CreateContainer, + Some("validator"), + None, + ), + ( + "name=validator-2", + &lodestar, + Action::CreateContainer, + Some("validator"), + None, + ), + // The daemon strips one leading slash, and the name is decoded. + ( + "name=/validator", + &lodestar, + Action::CreateContainer, + Some("validator"), + None, + ), + ( + "name=%2Fbeacon&platform=", + &lodestar, + Action::CreateContainer, + Some("beacon"), + None, + ), + ( + "platform=linux&name=beacon", + &lodestar, + Action::CreateContainer, + Some("beacon"), + None, + ), + // A name no policy for the image accepts. + ( + "name=other", + &lodestar, + Action::Deny, + None, + Some("container name other not allowed for image chainsafe/lodestar:v1.20.0 by any policy"), + ), + // A create without ?name= only matches a policy without container_names. + ( + "", + &lodestar, + Action::Deny, + None, + Some("image chainsafe/lodestar:v1.20.0 requires a container name"), + ), + ( + "name=", + &lodestar, + Action::Deny, + None, + Some("image chainsafe/lodestar:v1.20.0 requires a container name"), + ), + ("", &prom, Action::CreateContainer, Some("monitoring"), None), + ( + "name=anything", + &prom, + Action::CreateContainer, + Some("monitoring"), + None, + ), + // The image decides first. + ( + "name=beacon", + &alpine, + Action::Deny, + None, + Some("image alpine:latest not allowed by any policy"), + ), + // An invalid or undecodable name is denied. + ( + "name=-beacon", + &lodestar, + Action::Deny, + None, + Some("invalid container name"), + ), + ( + "name=bea%2Fcon", + &lodestar, + Action::Deny, + None, + Some("invalid container name"), + ), + ("name=%zz", &lodestar, Action::Deny, None, Some("invalid query string")), + // Decoded as the daemon's r.Form does. + ( + "name=%62eacon", + &lodestar, + Action::CreateContainer, + Some("beacon"), + None, + ), + ( + "name=validator%2D2", + &lodestar, + Action::CreateContainer, + Some("validator"), + None, + ), + // The daemon takes the first of a repeated name; the proxy denies it. + ( + "name=beacon&name=monitoring", + &lodestar, + Action::Deny, + None, + Some("duplicate name query parameter"), + ), + ( + "name=beacon&n%61me=validator", + &lodestar, + Action::Deny, + None, + Some("duplicate name query parameter"), + ), + // A query the daemon rejects is denied, as for the pull (#83). + ( + "platform=%zz&name=beacon", + &lodestar, + Action::Deny, + None, + Some("invalid query string"), + ), + ( + "name=beacon;x=1", + &lodestar, + Action::Deny, + None, + Some("invalid query string"), + ), + ]; + let mut mismatches = Vec::new(); + for (query, body, action, service, msg) in cases { + let got = router.route("POST", &format!("/v1.45/containers/create?{query}"), Some(body)); + if got.action != action || got.service.as_deref() != service || got.deny_msg.as_deref() != msg { + mismatches.push(format!( + "create ?{query}: got {:?} {:?} {:?}, want {:?} {:?} {:?}", + got.action, got.service, got.deny_msg, action, service, msg + )); + } + } + assert!(mismatches.is_empty(), "{}", mismatches.join("\n")); + } + + /// The #85 reproduction: with two policies for one image, every create + /// gets its own policy, every time. + #[test] + fn test_route_create_selection_is_stable() { + let router = Router::new(crate::policy::tests::load_dir(&LODESTAR_POLICIES).unwrap()); + let body = body_of(serde_json::json!({"Image": "chainsafe/lodestar:v1.20.0"})); + for i in 0..50 { + for name in ["beacon", "validator"] { + let got = router.route("POST", &format!("/containers/create?name={name}"), Some(&body)); + assert_eq!( + (got.action, got.service.as_deref()), + (Action::CreateContainer, Some(name)), + "iteration {i}: {:?}", + got.deny_msg + ); + } + } + } + + /// Two policies that both accept one image and name are refused at load + /// time; built without that check, the router denies. + #[test] + fn test_route_create_ambiguous() { + let mut b = make_manager(vec!["chainsafe/lodestar"]) + .get("myservice") + .unwrap() + .clone(); + b.service_name = "b".into(); + let mut a = b.clone(); + a.service_name = "a".into(); + a.allowed_image_prefixes = vec!["chainsafe".into()]; + a.container_names = Some(vec!["beacon".into()]); + let router = Router::new(Manager::from_map(HashMap::from([("a".into(), a), ("b".into(), b)]))); + let body = body_of(serde_json::json!({"Image": "chainsafe/lodestar:v1"})); + let got = router.route("POST", "/containers/create?name=beacon", Some(&body)); + assert_eq!(got.action, Action::Deny); + assert_eq!( + got.deny_msg.as_deref(), + Some("ambiguous policy for image chainsafe/lodestar:v1 and name beacon: a, b") + ); + let got = router.route("POST", "/containers/create?name=validator", Some(&body)); + assert_eq!( + (got.action, got.service.as_deref()), + (Action::CreateContainer, Some("b")) + ); + } + + /// The cross-language parity guard for #86. A lifecycle action or DELETE + /// is allowed directly only on a name that exactly one policy covers + /// (container_names, or service_name when unset). Any other target is + /// routed for resolution: the handler asks the daemon which container it + /// is. Rows mirror the covered* and uncovered* runs in spec/router.qnt. + #[test] + fn test_route_lifecycle_coverage() { + let files = [ + LODESTAR_POLICIES[1], + LODESTAR_POLICIES[2], + ( + "vc.yaml", + "service_name: vc-svc\ncontainer_names: [vc-1]\nallowed_image_prefixes:\n - example/vc\n", + ), + ( + "cafe.yaml", + "service_name: cafe\nallowed_image_prefixes:\n - cafe/app\n", + ), + ]; + let router = Router::new(crate::policy::tests::load_dir(&files).unwrap()); + let resolve = Action::ResolveContainer; + let cases = [ + // coveredNameStartAllowedTest + ("POST", "/containers/validator/start", Action::Allow, Some("validator")), + ( + "POST", + "/v1.45/containers/validator-2/kill", + Action::Allow, + Some("validator"), + ), + ("DELETE", "/containers/validator", Action::Allow, Some("validator")), + // A policy without container_names covers its service_name. + ( + "POST", + "/containers/monitoring/restart", + Action::Allow, + Some("monitoring"), + ), + // With container_names set, service_name is not covered. + ("POST", "/containers/vc-svc/stop", resolve.clone(), None), + ("POST", "/containers/vc-1/stop", Action::Allow, Some("vc-svc")), + // uncoveredNameStopDeniedTest: an operator container, resolved first. + ("POST", "/containers/mycontainer/stop", resolve.clone(), None), + // An ID (full or short) is resolved by the daemon. + ("POST", "/containers/4f6e1d2c9a7b/kill", resolve.clone(), None), + ( + "DELETE", + "/containers/4f6e1d2c9a7b8c3d4e5f60718293a4b5c6d7e8f9012345678901234567890abc", + resolve.clone(), + None, + ), + // A covered name that could also be an ID prefix is resolved too. + ("POST", "/containers/cafe/start", resolve.clone(), None), + // Reads are not lifecycle actions. + ("GET", "/containers/mycontainer/json", Action::Allow, None), + ]; + let mut mismatches = Vec::new(); + for (method, path, action, service) in cases { + let got = router.route(method, path, None); + if got.action != action || got.service.as_deref() != service { + mismatches.push(format!( + "{method} {path}: got {:?} {:?}, want {:?} {:?}", + got.action, got.service, action, service + )); + } + } + assert!(mismatches.is_empty(), "{}", mismatches.join("\n")); + + // --readonly still denies first (#75). + let ro = Router::new(crate::policy::tests::load_dir(&files).unwrap()).with_readonly(true); + assert_eq!( + ro.route("POST", "/containers/validator/start", None).action, + Action::Deny + ); + } + + /// Several policies may allow one image once container_names tells + /// their creates apart (#85). A pull creates no container, so it is + /// allowed when any policy for the image accepts its tag or digest, and + /// denied with the first policy's reason (sorted by service_name) when + /// none does. + #[test] + fn test_route_pull_shared_image() { + let files = [ + ( + "vc-b.yaml", + "service_name: vc-b\ncontainer_names: [vc-b]\nallowed_image_prefixes: [example/vc]\nimage_tag_pattern: '^v2$'\n", + ), + ( + "vc-a.yaml", + "service_name: vc-a\ncontainer_names: [vc-a]\nallowed_image_prefixes: [example/vc]\nimage_tag_pattern: '^v1$'\n", + ), + ]; + let router = Router::new(crate::policy::tests::load_dir(&files).unwrap()); + let cases = [ + ("v1", Action::Allow, Some("vc-a"), None), + ("v2", Action::Allow, Some("vc-b"), None), + ( + "v3", + Action::Deny, + None, + Some(r#"image tag "v3" does not match pattern "^v1$""#), + ), + ]; + for i in 0..50 { + for (tag, action, service, msg) in &cases { + let got = router.route( + "POST", + &format!("/images/create?fromImage=example%2Fvc&tag={tag}"), + None, + ); + assert_eq!( + (&got.action, got.service.as_deref(), got.deny_msg.as_deref()), + (action, *service, *msg), + "iteration {i}: pull tag {tag}" + ); + } + } + } } diff --git a/spec/README.md b/spec/README.md index 56309ab..80e8f7d 100644 --- a/spec/README.md +++ b/spec/README.md @@ -6,10 +6,10 @@ This directory contains a [Quint](https://quint-lang.org/) formal specification | File | Purpose | |------|---------| -| `docker_socket_policy.qnt` | Request-handling spec: policy types, state machine, endpoint routing table, 11 invariants (8 P0 / 3 P1), 6 attack scenario simulations. Module `network_endpoints_model` models `NetworkingConfig.EndpointsConfig` under a forced `network_mode` (#87), one `run` test per table row, instances `network_endpoints` (all three after #87) and `network_endpoints_pre87`. Also the create check on `HostConfig` (module `create_model`, #88): the default-deny field rules, `Mounts`, bind options, read-only volumes and case-insensitive field names, with one `run` test per table row. Instances `create` (all three after #88) and `create_pre88` (all three before #88); the state machine uses the `create` rules | +| `docker_socket_policy.qnt` | Request-handling spec: policy types, state machine, endpoint routing table, policy selection by image and container name (#85), 12 invariants (9 P0 / 3 P1), 6 attack scenario simulations, and `run` tests for selection. Module `network_endpoints_model` models `NetworkingConfig.EndpointsConfig` under a forced `network_mode` (#87), one `run` test per table row, instances `network_endpoints` (all three after #87) and `network_endpoints_pre87`. Also the create check on `HostConfig` (module `create_model`, #88): the default-deny field rules, `Mounts`, bind options, read-only volumes and case-insensitive field names, with one `run` test per table row. Instances `create` (all three after #88) and `create_pre88` (all three before #88); the state machine uses the `create` rules | | `listener.qnt` | Listening-socket startup: flag/group selection, existing-path checks, single-instance lock, 6 invariants, one `run` test per design-table row. Instances `listener_locked` (Go, Rust) and `listener_unlocked` (TypeScript) | | `listener-design.md` | Design of the listening socket (dockerd parity) that `listener.qnt` models | -| `router.qnt` | The percent-encoded path deny, the empty interior segment deny, the dot segment deny, the segment-exact exec deny and `POST /containers/create` route, the segment-exact attach deny, the GET/HEAD-only read passthrough with `/_ping` standing for the read endpoints, container-name extraction and the container-lifecycle branch of the router only, not the full router. The daemon's percent-decoding as a fixed table. One `run` test per table row. Instances `router` (Go, TypeScript, and Rust after #48, #53, #49, #55, #76, #78 and #70), `router_pre48` (Rust's extraction rule before #48), `router_pre53` (Rust and TypeScript before #53, routing on the raw path), `router_pre55` (all three before #55, letting empty interior segments through), `router_readonly` (as `router`, with `--readonly`, #75), `router_pre76` (all three before #76, letting attach through), `router_pre78` (all three before #78, allowing the read endpoints for every method) and `router_pre70` (all three before #70, letting `.` and `..` segments through). Every instance includes the #49 exec and create rule, so the pre instances isolate one earlier rule each rather than being full snapshots. Module `pull_model` models the `POST /images/create` branch: which of the body and the query names the image, and image-name normalisation as a fixed table. Instances `pull` (all three after #83 and #84), `pull_pre83` (all three before #83, reading `fromImage` from the body), `pull_body_ignored` (the first #83 fix, which forwarded a pull's body) and `pull_pre84` (all three before #84, matching the spelling as sent) | +| `router.qnt` | The percent-encoded path deny, the empty interior segment deny, the dot segment deny, the segment-exact exec deny and `POST /containers/create` route, the segment-exact attach deny, the GET/HEAD-only read passthrough with `/_ping` standing for the read endpoints, container-name extraction and the container-lifecycle branch of the router only, not the full router, plus the handler's daemon lookup that limits lifecycle actions to covered containers (#86). The daemon's percent-decoding as a fixed table. One `run` test per table row. Instances `router` (Go, TypeScript, and Rust after #48, #53, #49, #55, #76, #78 and #70), `router_pre48` (Rust's extraction rule before #48), `router_pre53` (Rust and TypeScript before #53, routing on the raw path), `router_pre55` (all three before #55, letting empty interior segments through), `router_readonly` (as `router`, with `--readonly`, #75), `router_pre76` (all three before #76, letting attach through), `router_pre78` (all three before #78, allowing the read endpoints for every method), `router_pre70` (all three before #70, letting `.` and `..` segments through) and `router_pre86` (all three before #86, forwarding lifecycle actions on uncovered containers). Every instance includes the #49 exec and create rule, so the pre instances isolate one earlier rule each rather than being full snapshots. Module `pull_model` models the `POST /images/create` branch: which of the body and the query names the image, and image-name normalisation as a fixed table. Instances `pull` (all three after #83 and #84), `pull_pre83` (all three before #83, reading `fromImage` from the body), `pull_body_ignored` (the first #83 fix, which forwarded a pull's body) and `pull_pre84` (all three before #84, matching the spelling as sent) | ## How to Run @@ -34,7 +34,7 @@ quint run spec/listener.qnt --main=listener_locked --max-steps=30 --invariant al quint test spec/listener.qnt --main=listener_locked quint test spec/listener.qnt --main=listener_unlocked -# Router model: typecheck, run the table tests on all twelve instances +# Router model: typecheck, run the table tests on all thirteen instances quint typecheck spec/router.qnt quint test spec/router.qnt --main=router quint test spec/router.qnt --main=router_pre48 @@ -44,6 +44,7 @@ quint test spec/router.qnt --main=router_readonly --match='^readonly.*Test$' quint test spec/router.qnt --main=router_pre76 quint test spec/router.qnt --main=router_pre78 quint test spec/router.qnt --main=router_pre70 +quint test spec/router.qnt --main=router_pre86 quint test spec/router.qnt --main=pull quint test spec/router.qnt --main=pull_pre83 quint test spec/router.qnt --main=pull_body_ignored @@ -56,6 +57,9 @@ quint test spec/docker_socket_policy.qnt --main=network_endpoints_pre87 quint test spec/docker_socket_policy.qnt --main=create quint test spec/docker_socket_policy.qnt --main=create_pre88 +# Request-handling model: the policy selection tests (#85) +quint test spec/docker_socket_policy.qnt --main=docker_socket_policy --max-samples=100 + # Formal model-checking via Apalache (exhaustive, requires Java) quint verify --max-steps=10 --invariants allInvariants spec/docker_socket_policy.qnt ``` @@ -74,6 +78,7 @@ quint verify --max-steps=10 --invariants allInvariants spec/docker_socket_policy | `proxyLives` | Proxy process stays running | Error-handling recovery | | `hostConfigInAllowlist` | Every `HostConfig` field the daemon read is allowed, with an allowed value (#88) | HostConfigGate → `createAllowed()` | | `hostMountsInWhitelist` | Every host path the daemon mounted is a policy volume, read-only when the volume is, and never propagating (#88) | MountSourceGate → `createAllowed()` | +| `policySelectionUnique` | Every container was created under the one policy whose image prefixes match its image and whose `containerNames` accept its name (#85) | `createContainer` requires exactly one candidate from `selectPolicies`; the router's `SelectForCreate` / `select_for_create` / `selectForCreate` | ### P1 Security Invariants (Should Never Fail) @@ -102,9 +107,10 @@ quint run spec/listener.qnt --main=listener_unlocked --max-steps=30 --invariant ### Router path parsing (`router.qnt`) -A path is a list of raw, still percent-encoded segments with the query string removed, so `/containers/` is `["containers", ""]`, `/containers//start` is `["containers", "", "start"]` and `/containers/%2F` is `["containers", "%2F"]`. With `READONLY` set, the read-only check runs before all of these and denies every method other than `GET` and `HEAD`, for every path (#75). The percent check, first in `route` after it, denies any path with an encoded segment, for every method (#53). The empty-segment check denies any path with an empty segment before the last one, i.e. `//` in the path, for every method (#55); a single trailing slash, as in `/containers/`, is not interior. Right after it, any path with a segment that is exactly `.` or `..` is denied, for every method (#70); `.hidden`, `...` and `a.b` are names. With `ALLOW_ANY_METHOD_PING` set, `router_pre78` allows `/_ping` for every method between the dot segment check and the exec check, as the routers did before #78. The exec check denies exec, for every method, as `denyExec` (#49): when the first segment is `exec` (the `/exec//…` namespace), or when the first segment is `containers` and any later segment is exactly `exec`. A segment that only starts with or contains `exec`, such as `exec-runner`, is not matched. The attach check denies attach, for every method (#76): when the first segment is `containers` and any later segment is exactly `attach`; `attach-runner` is a name like any other. The create route then sends `POST` with exactly the segments `["containers", "create"]` to `routeCreate`; a longer path falls through. After these, the second segment is a container name unless it is empty or one of `create`, `json`, `exec`. `HEAD` is routed like `GET` (#55). Every property checks every method in `GET`, `HEAD`, `POST`, `DELETE` against every path of 1 to 3 segments: +A path is a list of raw, still percent-encoded segments with the query string removed, so `/containers/` is `["containers", ""]`, `/containers//start` is `["containers", "", "start"]` and `/containers/%2F` is `["containers", "%2F"]`. With `READONLY` set, the read-only check runs before all of these and denies every method other than `GET` and `HEAD`, for every path (#75). The percent check, first in `route` after it, denies any path with an encoded segment, for every method (#53). The empty-segment check denies any path with an empty segment before the last one, i.e. `//` in the path, for every method (#55); a single trailing slash, as in `/containers/`, is not interior. Right after it, any path with a segment that is exactly `.` or `..` is denied, for every method (#70); `.hidden`, `...` and `a.b` are names. With `ALLOW_ANY_METHOD_PING` set, `router_pre78` allows `/_ping` for every method between the dot segment check and the exec check, as the routers did before #78. The exec check denies exec, for every method, as `denyExec` (#49): when the first segment is `exec` (the `/exec//…` namespace), or when the first segment is `containers` and any later segment is exactly `exec`. A segment that only starts with or contains `exec`, such as `exec-runner`, is not matched. The attach check denies attach, for every method (#76): when the first segment is `containers` and any later segment is exactly `attach`; `attach-runner` is a name like any other. The create route then sends `POST` with exactly the segments `["containers", "create"]` to `routeCreate`; a longer path falls through. After these, the second segment is a container name unless it is empty or one of `create`, `json`, `exec`. `HEAD` is routed like `GET` (#55). In the lifecycle branch, a lifecycle `POST` or a `DELETE` on a name in `COVERED_NAMES` is `allowKnown`; any other name is `resolve` (#86), and `handle` completes it with the daemon's lookup, the table `DAEMON_CONTAINERS` (`beacon` and its ID `f00d` name the covered container, `mycontainer` and its ID `c0ffee` an operator container, anything else is a 404): `allowResolved` if the container's name is covered, `deny` otherwise. With `ALLOW_UNCOVERED` set, `router_pre86` allows any other name as `allowUnknown`, as all three did before #86. Every property checks every method in `GET`, `HEAD`, `POST`, `DELETE` against every path of 1 to 3 segments: -- `lifecycleOnlyTargetsRealNames` holds when each lifecycle allow (`allowKnown` or `allowUnknown`) targets a non-empty, non-reserved name. +- `lifecycleOnlyTargetsRealNames` holds when each lifecycle allow or lookup (`allowKnown`, `allowUnknown` or `resolve`) targets a non-empty, non-reserved name. +- `lifecycleOnlyCovered` holds when no lifecycle action or `DELETE` that `handle` allows (`allowKnown`, `allowUnknown`, `allowResolved`) targets an existing container whose name no policy covers, by name or by ID (#86). It does not mention `ALLOW_UNCOVERED` or `resolve`, so `router_pre86` fails it. - `routerSeesWhatDaemonSees` holds when every request the router does not deny reads the same to the daemon: decoding leaves its path unchanged. The daemon's decoding is the table `DAEMON_DECODING`: `%2F` and `%2f` decode to `["", ""]` (a `/` splits the segment), `%6A%73%6F%6E` to `["json"]`, and `beacon%2Fstart` to `["beacon", "start"]`. - `execNeverAllowed` holds when every path with `exec` in an exec position is denied. The exec positions are written out in the property, not taken from `route`'s `isExecPath`. - `execPrefixNamesRoute` holds when renaming `exec-runner` to the plain unknown name `mycontainer`, wherever it appears in the path, never changes the outcome. @@ -117,7 +123,7 @@ A path is a list of raw, still percent-encoded segments with the query string re - `readPassthroughOnlyForReads` holds when every request routed to the read passthrough (`allowRead`) is a `GET` or a `HEAD` (#78). It does not mention `ALLOW_ANY_METHOD_PING`, so `router_pre78` fails it. - `readonlyAllowsOnlyReads` holds when every request whose method is not `GET` or `HEAD` is denied, for every path. It is the read-only claim and holds only with `READONLY = true`; the router's read-only check, which runs before every other check, denies such requests (#75). -On `router`, `soundTest` asserts `lifecycleOnlyTargetsRealNames`, `percentSoundTest` asserts `routerSeesWhatDaemonSees`, `execSoundTest` asserts `execNeverAllowed`, `execPrefixSoundTest` asserts `execPrefixNamesRoute`, `createExactSoundTest` asserts `createOnlyExact`, `emptyInteriorSoundTest` asserts `emptyInteriorDenied`, `emptySegmentsHideNoExecTest` asserts `emptySegmentsHideNoExec`, `attachSoundTest` asserts `attachNeverAllowed`, `readPassthroughSoundTest` asserts `readPassthroughOnlyForReads` and `dotSoundTest` asserts `dotSegmentsDenied` and `dotSegmentsHideNoExec`. +On `router`, `soundTest` asserts `lifecycleOnlyTargetsRealNames`, `percentSoundTest` asserts `routerSeesWhatDaemonSees`, `execSoundTest` asserts `execNeverAllowed`, `execPrefixSoundTest` asserts `execPrefixNamesRoute`, `createExactSoundTest` asserts `createOnlyExact`, `emptyInteriorSoundTest` asserts `emptyInteriorDenied`, `emptySegmentsHideNoExecTest` asserts `emptySegmentsHideNoExec`, `attachSoundTest` asserts `attachNeverAllowed`, `readPassthroughSoundTest` asserts `readPassthroughOnlyForReads`, `dotSoundTest` asserts `dotSegmentsDenied` and `dotSegmentsHideNoExec`, and `coveredSoundTest` asserts `lifecycleOnlyCovered`. | Row (`run Test`) | Request | Outcome | Instances | |-----|---------|---------|-----------| @@ -127,7 +133,7 @@ On `router`, `soundTest` asserts `lifecycleOnlyTargetsRealNames`, `percentSoundT | `reservedJsonDeleteDenied` | `DELETE /containers/json` | deny | all | | `reservedCreateDeleteDenied` | `DELETE /containers/create` | deny | all | | `reservedExecDeleteDenied` | `DELETE /containers/exec` | deny (exec) | all | -| `realNameDeleteAllowed` | `DELETE /containers/mycontainer` | allow (unknown container) | all | +| `realNameDeleteDenied` | `DELETE /containers/mycontainer` | deny (`resolve`, then uncovered, #86) | `router` | | `reservedInSubpathAllowed` | `GET /containers/mycontainer/json` | allow | all | Rows for the percent-encoded path deny ([#53](https://github.com/ChainSafe/docker-socket-policy/issues/53)), all on `router`: @@ -147,8 +153,8 @@ Rows for segment-exact exec and create matching ([#49](https://github.com/ChainS |-----|---------|---------| | `execSubpathDeleteDenied` | `DELETE /containers/mycontainer/exec` | deny (exec) | | `execSubpathPostDenied` | `POST /containers/mycontainer/exec` | deny (exec) | -| `execPrefixNameStartAllowed` | `POST /containers/exec-runner/start` | allow (unknown container) | -| `execPrefixNameDeleteAllowed` | `DELETE /containers/exec-runner` | allow (unknown container) | +| `execPrefixNameStartDenied` | `POST /containers/exec-runner/start` | deny (`resolve`, then daemon 404, #86; `router` only) | +| `execPrefixNameDeleteDenied` | `DELETE /containers/exec-runner` | deny (`resolve`, then daemon 404, #86; `router` only) | | `execPrefixNameGetAllowed` | `GET /containers/exec-runner/json` | allow (read) | | `execNamespaceGetDenied` | `GET /exec/abc/json` | deny (exec) | | `execNamespacePostDenied` | `POST /exec/abc/start` | deny (exec) | @@ -198,9 +204,32 @@ Rows for `--readonly` ([#75](https://github.com/ChainSafe/docker-socket-policy/i | `readonlyGetAllowed` | `GET /containers/json` | allow (read) | | `readonlyHeadAllowed` | `HEAD /containers/mycontainer` | allow (read) | +Rows for lifecycle actions on covered containers only ([#86](https://github.com/ChainSafe/docker-socket-policy/issues/86)), all on `router`. "resolve" is the router's outcome; the handler then asks the daemon `GET /containers/{x}/json`. The implementations deny with `container {x} is not managed by any policy` (a 404 or an uncovered name) or `cannot resolve container {x}: …` (any other daemon error, which the table does not model). `realNameDeleteDenied`, `execPrefixNameStartDenied` and `execPrefixNameDeleteDenied` above were allowed as an unknown container before #86: + +| Row (`run Test`) | Request | Router | Handler | +|-----|---------|--------|---------| +| `coveredNameStartAllowed` | `POST /containers/beacon/start` | allowKnown | allowKnown | +| `coveredIdStartAllowed` | `POST /containers/f00d/start` (the covered container's ID) | resolve | allowResolved | +| `uncoveredNameStopDenied` | `POST /containers/mycontainer/stop` (an operator container) | resolve | deny | +| `uncoveredIdStopDenied` | `POST /containers/c0ffee/stop` (its ID) | resolve | deny | +| `missingContainerDeleteDenied` | `DELETE /containers/abc` | resolve | deny (daemon 404) | + The rows marked "all" above run on every `READONLY = false` instance; `router_readonly` runs only its own `readonly*` tests (`--match='^readonly.*Test$'`), since with the flag on the shared write rows are denied. -Each implementation's router tests use the same row names in comments (added with the #48, #53, #49, #55, #76, #78 and #70 fixes). `router_pre48` runs `pre48UnsoundTest`, which asserts that `lifecycleOnlyTargetsRealNames` fails and that both `emptyName*Denied` requests are allowed as an unknown container ([#48](https://github.com/ChainSafe/docker-socket-policy/issues/48)). `router_pre53` runs `pre53UnsoundTest`, which asserts that `routerSeesWhatDaemonSees` fails, that `DELETE /containers/%2F` and `POST /containers/%2F/start` are allowed as an unknown container, and that `GET /containers/%2F` is allowed through the passthrough. `router_pre55` runs `pre55UnsoundTest`, which asserts that `emptyInteriorDenied` fails and that `GET //exec/abc` is allowed through the passthrough: its first segment is empty, not `exec`, so the exec check misses it. `router_pre76` runs `pre76UnsoundTest`, which asserts that `attachNeverAllowed` fails and that `GET /containers/mycontainer/attach` is allowed through the passthrough ([#76](https://github.com/ChainSafe/docker-socket-policy/issues/76)). `router_pre78` runs `pre78UnsoundTest`, which asserts that `readPassthroughOnlyForReads` fails and that `POST /_ping` and `DELETE /_ping` are allowed as reads ([#78](https://github.com/ChainSafe/docker-socket-policy/issues/78)). `router_pre70` runs `pre70UnsoundTest`, which asserts that `dotSegmentsDenied` and `dotSegmentsHideNoExec` fail and that `GET /./exec/abc` is allowed through the passthrough ([#70](https://github.com/ChainSafe/docker-socket-policy/issues/70)). +Each implementation's router tests use the same row names in comments (added with the #48, #53, #49, #55, #76, #78 and #70 fixes). `router_pre48` runs `pre48UnsoundTest`, which asserts that `lifecycleOnlyTargetsRealNames` fails and that both `emptyName*Denied` requests go down the lifecycle branch as a name (`resolve`; before #86 they were allowed as an unknown container) ([#48](https://github.com/ChainSafe/docker-socket-policy/issues/48)). `router_pre53` runs `pre53UnsoundTest`, which asserts that `routerSeesWhatDaemonSees` fails, that `DELETE /containers/%2F` and `POST /containers/%2F/start` go down the lifecycle branch as a name (`resolve`; before #86, allowed as an unknown container), and that `GET /containers/%2F` is allowed through the passthrough. `router_pre55` runs `pre55UnsoundTest`, which asserts that `emptyInteriorDenied` fails and that `GET //exec/abc` is allowed through the passthrough: its first segment is empty, not `exec`, so the exec check misses it. `router_pre76` runs `pre76UnsoundTest`, which asserts that `attachNeverAllowed` fails and that `GET /containers/mycontainer/attach` is allowed through the passthrough ([#76](https://github.com/ChainSafe/docker-socket-policy/issues/76)). `router_pre78` runs `pre78UnsoundTest`, which asserts that `readPassthroughOnlyForReads` fails and that `POST /_ping` and `DELETE /_ping` are allowed as reads ([#78](https://github.com/ChainSafe/docker-socket-policy/issues/78)). `router_pre70` runs `pre70UnsoundTest`, which asserts that `dotSegmentsDenied` and `dotSegmentsHideNoExec` fail and that `GET /./exec/abc` is allowed through the passthrough ([#70](https://github.com/ChainSafe/docker-socket-policy/issues/70)). `router_pre86` runs `pre86UnsoundTest`, which asserts that `lifecycleOnlyCovered` fails and that `DELETE /containers/mycontainer`, `POST /containers/c0ffee/stop` (an operator container by ID) and `POST /containers/exec-runner/start` are allowed as an unknown container ([#86](https://github.com/ChainSafe/docker-socket-policy/issues/86)). + +### Policy selection (`docker_socket_policy.qnt`, #85) + +`Policy` has `containerNames`; the empty set stands for an unset `container_names`, since an empty list is a load error. `selectPolicies(ps, image, name)` is the policies whose image prefixes match and which accept the name (any name when `containerNames` is empty). `createContainer` requires exactly one candidate and applies it; zero or several deny the create. The two policies, `beacon` and `validator`, both allow `chainsafe/lodestar` and are told apart by `containerNames`, as in the Lodestar deployment that #85 came from. The #88 create scenarios in `step` (`beacon-privileged`, `mount-root`, …) are among the beacon policy's names, so they reach the HostConfig checks instead of stopping at selection; `tools` sets none and accepts any name. `make test-spec` runs the module's `run` tests: + +| Test | Asserts | +|------|---------| +| `pre85AmbiguousTest` | Without `containerNames` (`PRE85_POLICIES`, the policies before #85) both policies match `validator-0`; with them only `validator` does, and only `beacon` matches `beacon-0` | +| `createSelectsByNameTest` | A create of `validator-0` gets the `validator` policy, and `policySelectionUnique` holds after it | +| `createUnknownNameDeniedTest` | A lodestar create named `monitoring`, which neither policy accepts, is disabled | +| `createNamelessDeniedTest` | A lodestar create without a name (`""`) is disabled, as both policies set `containerNames` | + +Selecting from every image match instead (the pre-#85 `oneOf`) makes the last two fail: with no volumes or flags, either policy's guards pass. Load-time validation (two policies whose prefixes share a namespace and that could accept one name fail startup, as does a duplicate `service_name`) is not modelled; it is what makes "several candidates" unreachable in a running proxy, and the language tests pin it. ### Network endpoints under a forced `network_mode` (`docker_socket_policy.qnt`, #87) @@ -311,7 +340,7 @@ Two invariants are structurally tautological within the Quint model — they can - **`proxyLives`** — `proxyRunning` is set once in `init` and every action preserves it (`proxyRunning' = proxyRunning`); nothing in the model ever sets it `false`. The real guarantee ("a panic/error on one request doesn't crash the whole proxy") is enforced by language-specific mechanisms outside the model: Go's stdlib `net/http.Server` recovers per-request panics, Rust's `tokio::spawn` isolates panics per connection task, and TypeScript's request handler wraps `handle()` in a `.catch()`. These are exercised by each implementation's own test suite, not by the Quint simulation. - **`routingTableComplete`** — checks that `endpointsTable` (a fixed constant) contains a fixed list of literals declared in the same file. It documents the intended routing table but doesn't cross-check it against any of the three Router implementations; that comparison has to be done manually (or via `quint-analyzer`) against `go/internal/proxy/router.go`, `rs/src/proxy.rs`, and `ts/src/proxy.ts`. -- **The model has no caller, by design.** `createContainer` picks a policy from the request's image, and nothing models who sent the request. An invariant such as "a container's `serviceName` equals the caller's service" therefore cannot be stated, and that is deliberate: callers who share a socket cannot be told apart. Several developers may share one account, for example, so the proxy has no identity to check. The trust boundary is the socket itself. Separating trust domains means one proxy instance per domain, each with its own socket group, and the kernel enforces that separation outside this model. See the README's "Trust model" section and [#39](https://github.com/ChainSafe/docker-socket-policy/issues/39). +- **The model has no caller, by design.** `createContainer` picks a policy from the request's image and container name, and nothing models who sent the request. An invariant such as "a container's `serviceName` equals the caller's service" therefore cannot be stated, and that is deliberate: callers who share a socket cannot be told apart. Several developers may share one account, for example, so the proxy has no identity to check. The trust boundary is the socket itself. Separating trust domains means one proxy instance per domain, each with its own socket group, and the kernel enforces that separation outside this model. See the README's "Trust model" section and [#39](https://github.com/ChainSafe/docker-socket-policy/issues/39). - **`listener.qnt` checks the design, not the code.** Nothing in the model is derived from the Go, Rust or TypeScript sources. Conformance rests on each implementation's unit and integration tests, which carry the same names as the Quint `run`s (`groupDefaultPresent`, `pathStaleReplaced`, …) so every design-table row can be traced across all four. `raceWithoutLockTest` (in `listener_unlocked`) is the formal record of the TypeScript gap: Node has no `flock`, so two TypeScript instances starting together can orphan one another's socket ([#46](https://github.com/ChainSafe/docker-socket-policy/issues/46)). - **`lifecycleOnlyTargetsRealNames` is close to a tautology.** `route` only allows by name when `hasContainerName` holds, and `hasContainerName` is nearly the property itself. It is not vacuous: on `router_pre48`, where an empty segment counts as a name, the property fails, and `pre48UnsoundTest` asserts that. Most of the evidence comes from the table rows, whose names the language router tests reuse (added with the #48 fix). @@ -334,6 +363,9 @@ Two invariants are structurally tautological within the Quint model — they can - **Image references are tokens, normalisation is a table (#84).** Quint cannot split a string, so `pull_model` cannot parse `[registry/]repository[:tag][@digest]`. `CANONICAL` lists the spellings in the universe and the repository each one names; the implementations compute the same thing with one parser per language, used for the request's image (create `Image` and pull `fromImage`), for the registry gate and for each policy prefix at load time. The parser's rules (the tag is after the last `:` that follows the last `/`; the first component is a registry when it contains `.` or `:` or is `localhost`; `docker.io`, `index.docker.io` and `registry-1.docker.io` are dropped, and so is `library/` before a single-component name; only the registry is lower-cased, and upper case in the path is invalid) are pinned by the same normalisation table in the Go, Rust and TypeScript tests, not by the model. So are the namespace-boundary prefix match (`prefix` or `prefix/…`), which the model reduces to equality, and the tag and digest rules. - **The create model is a small universe (#88).** Values are atoms (`"null"`, `"zero"`, `"[]"` and a few non-zero ones), a list field holds one entry, and the field names stand for the classes in the README's HostConfig table: `CapAdd`, `Devices`, `GroupAdd` for the must-be-zero fields, `FutureField` for a field the proxy does not know, `CapDrop` and `MemorySwappiness` for the any-value fields, `PidMode` and `IpcMode` for the namespace modes. Case folding is a fixed table (`CASE_FOLD`), as percent-decoding is in `router.qnt`: Quint cannot fold strings. The implementations fold ASCII case plus the Kelvin sign and the long s, as Go's `encoding/json` does, and check the top level of the body, `Mounts` entries and `BindOptions` too; the language tables cover those (`caseVariantLongSDenied`, `caseVariantKelvinDenied`, `topLevel*VariantDenied`, `mountCaseVariantDenied`, `networkingConfigVariantLongSDenied`). The other nested objects need no check: the mutator replaces `RestartPolicy` and `LogConfig` whole when the policy sets them and they are any-value otherwise, and no gate reads `Healthcheck` or an endpoint entry's settings by key. Dropping the `NetworkingConfig` variant check from `createAllowed` makes the two `networkingConfigVariant*` rows and `createSoundTest` fail. Requests are single-field, single-bind or single-mount; a request that combines a canonical field with a variant needs no separate case, because the daemon may keep either value and the property checks every value it may read. Bind parsing (`src:dst:opts`, the escaped colon in Go), the zero-value test on JSON types and the type checks (a `Binds` entry that is not a string) are language-only rows. - **Mutating the create rules makes the rows fail.** Dropping the case-variant check from `fieldOk` makes `caseVariantZeroSecurityOptDenied` and `createSoundTest` fail (`caseVariantPrivilegedDenied` still passes: `privileged` is not an allowlisted name, so the zero-value rule denies its `true`). Dropping the read-only clause from `mountOk` makes `mountBindReadOnlyVolumeWritableDenied` and `createSoundTest` fail. +- **Lifecycle coverage is a router rule plus one daemon lookup (#86).** Before #86 a lifecycle `POST` or `DELETE` on a name no policy covered was forwarded (`allowUnknown`), so every caller could stop or remove every container on the host, operator containers included. The router now allows directly only a name that exactly one policy covers; any other target needs the daemon's answer, because the path may carry a full or short ID. The router model returns `resolve` for those, and `handle` stands for the handler's `GET /containers/{x}/json`, with the daemon's answers as the table `DAEMON_CONTAINERS`. `lifecycleOnlyCovered` fails on `router_pre86`, and setting `router`'s `ALLOW_UNCOVERED` to `true` makes `coveredSoundTest`, the three flipped rows and four of the five #86 rows fail (`coveredNameStartAllowed` passes either way). Three things are outside the model and pinned by the language tests: a covered name that is all lowercase hex is resolved too, since if no container has that name the daemon falls back to the container whose ID starts with it (Quint cannot inspect characters); a daemon error other than 404 denies; and the lookup and the action are two requests, so a container renamed or recreated in between by someone with direct daemon access is not caught (renames are denied through the proxy). +- **A pull checks every policy for the image (#85).** With `container_names`, several policies may allow one image. A pull creates no container and has no name, so the implementations allow it when any of them accepts the tag or digest, and deny it with the first one's reason (sorted by `service_name`) otherwise. `pull_model` reduces policies to the set of allowed repositories, so this is pinned by the language tests (`TestRoutePullSharedImage` and its peers). +- **Container names are routing (#85).** The caller picks the name, so `container_names` decides which policy a create gets, not whether the caller may have it: the model still has no caller. `policySelectionUnique` is close to a tautology, since `createContainer` requires exactly one candidate; the `run` tests above carry the evidence. - **Listener fault bias.** `step` crashes an instance on 1 in 10 draws instead of half of all steps, so random runs actually interleave live instances. Every crash stays reachable from every phase, so the reachable state space is unchanged. ### Attack Scenarios Prevented by Invariants @@ -365,7 +397,7 @@ Two invariants are structurally tautological within the Quint model — they can ┌──────────────────────────────────────────────────────┐ │ │ │ createContainer(name, imageRef, ...) │ - │ │ guards: nondet, imageNameAllowed, flagAllowed, │ + │ │ guards: one selectPolicies match, flagAllowed, │ │ │ volumeAllowed, envAllowed, │ │ │ createAllowed (HostConfig, #88) │ │ │ ValidImage only (invalid rejected) │ diff --git a/spec/docker_socket_policy.qnt b/spec/docker_socket_policy.qnt index 9bfa69a..4ce2236 100644 --- a/spec/docker_socket_policy.qnt +++ b/spec/docker_socket_policy.qnt @@ -10,8 +10,12 @@ module policy { user: str } + // containerNames is the policy's container_names (#85). The empty set + // stands for "unset": an empty container_names list is a load error, so + // the two cannot be confused. type Policy = { serviceName: str, + containerNames: Set[str], allowedImagePrefixes: Set[str], imageTagPattern: str, imageDigestAllowed: bool, @@ -30,6 +34,19 @@ module policy { policy.allowedImagePrefixes.contains(name) } + // A create named `name` may use the policy: a policy without + // container_names accepts any name, including none (#85). + pure def acceptsName(name: str, policy: Policy): bool = { + policy.containerNames.size() == 0 or policy.containerNames.contains(name) + } + + // The policies a create of `image` named `name` could use. The proxy + // forwards a create only when this is exactly one policy, and applies that + // one (#85). Before #85 any policy that matched the image could be applied. + pure def selectPolicies(ps: Set[Policy], image: str, name: str): Set[Policy] = { + ps.filter(p => imageNameAllowed(image, p) and acceptsName(name, p)) + } + pure def flagAllowed(flag: str, policy: Policy): bool = { policy.allowedCliFlags.contains(flag) and not(policy.deniedFlags.contains(flag)) @@ -362,6 +379,7 @@ module create_model { pure val BEACON: Policy = { serviceName: "beacon", + containerNames: Set(), allowedImagePrefixes: Set("chainsafe/lodestar"), imageTagPattern: "", imageDigestAllowed: false, @@ -383,6 +401,7 @@ module create_model { // No volumes and no container_config. pure val TOOLS: Policy = { serviceName: "tools", + containerNames: Set(), allowedImagePrefixes: Set("example/tools"), imageTagPattern: "", imageDigestAllowed: false, @@ -649,6 +668,10 @@ module docker_socket_policy { val policies: Set[Policy] = Set( { serviceName: "beacon", + // The #88 create scenarios below are named for beacon too, so they + // reach the HostConfig checks rather than stopping at selection (#85). + containerNames: Set("beacon-0", "beacon-1", "beacon-2", "beacon-privileged", "beacon-jwt-ro", + "mount-root", "jwt-rw", "cap-add", "pid-host", "lower-privileged"), allowedImagePrefixes: Set( "chainsafe/lodestar", "ethpandaops/lodestar" @@ -676,6 +699,7 @@ module docker_socket_policy { }, { serviceName: "validator", + containerNames: Set("validator-0", "validator-1", "validator-2"), allowedImagePrefixes: Set( "chainsafe/lodestar" ), @@ -765,8 +789,12 @@ module docker_socket_policy { hc: CreateReq ): bool = { if (imageRef.kind == "valid") { - nondet matched = oneOf(policies.filter(p => imageNameAllowed(imageRef.imageName, p))) + // The one policy that matches the image and accepts the name (#85): + // zero or several candidates deny the create. + val candidates = selectPolicies(policies, imageRef.imageName, name) + nondet matched = oneOf(candidates) all { + candidates.size() == 1, flags.forall(f => flagAllowed(f, matched)), volumes.forall(v => volumeAllowed(v, matched)), envAllowed(envVars, matched), @@ -1023,6 +1051,12 @@ module docker_socket_policy { val proxyLives = proxyRunning + // Every container was created under the one policy that matches its image + // and accepts its name (#85): selection is unique, never a choice. + val policySelectionUnique = containers.forall(c => + selectPolicies(policies, c.imageRef.imageName, c.name).map(p => p.serviceName) == Set(c.serviceName) + ) + val volumesInWhitelist = containers.forall(c => policies.exists(p => p.serviceName == c.serviceName @@ -1080,7 +1114,8 @@ module docker_socket_policy { envOnlyFromFile, proxyLives, hostConfigInAllowlist, - hostMountsInWhitelist + hostMountsInWhitelist, + policySelectionUnique } val allInvariants = allP0Invariants.and(all { @@ -1091,6 +1126,49 @@ module docker_socket_policy { // Attack scenarios are documented in spec/README.md. + // ─── Policy selection (#85) ────────────────────────────────────────── + // + // Beacon and validator run one image. Without container_names (the + // policies before #85) both match every lodestar create, and the proxy + // applied whichever it found first. + pure val PRE85_POLICIES: Set[Policy] = policies.map(p => { ...p, containerNames: Set() }) + + run pre85AmbiguousTest = + assert(and { + selectPolicies(PRE85_POLICIES, "chainsafe/lodestar", "validator-0").size() == 2, + selectPolicies(policies, "chainsafe/lodestar", "validator-0").map(p => p.serviceName) == Set("validator"), + selectPolicies(policies, "chainsafe/lodestar", "beacon-0").map(p => p.serviceName) == Set("beacon"), + }) + + // A create named for a policy gets that policy. + run createSelectsByNameTest = + init + .then(createContainer("validator-0", + { kind: "valid", imageName: "chainsafe/lodestar", tag: "validator", digest: "" }, + Set("--dataDir"), Set("/home/validator"), Set(), NO_HOST_CONFIG)) + .then(all { + assert(containers.map(c => c.serviceName) == Set("validator")), + assert(policySelectionUnique), + containers' = containers, + proxyRunning' = proxyRunning, + }) + + // A name no policy for the image accepts is denied, as is a create without + // a name ("") when every matching policy sets container_names. + run createUnknownNameDeniedTest = + init + .then(createContainer("monitoring", + { kind: "valid", imageName: "chainsafe/lodestar", tag: "beacon", digest: "" }, + Set(), Set(), Set(), NO_HOST_CONFIG)) + .fail() + + run createNamelessDeniedTest = + init + .then(createContainer("", + { kind: "valid", imageName: "chainsafe/lodestar", tag: "beacon", digest: "" }, + Set(), Set(), Set(), NO_HOST_CONFIG)) + .fail() + // P0 invariants must never fail under any sequence of actions. // P1 invariants should never fail under normal operation. // The quint verify command checks all invariants across all diff --git a/spec/router.qnt b/spec/router.qnt index f0cd6b4..0b61897 100644 --- a/spec/router.qnt +++ b/spec/router.qnt @@ -64,6 +64,16 @@ // right after the empty interior segment check. `.hidden`, `...` and `a.b` // are names like any other. // +// ALLOW_UNCOVERED = true models all three before #86, where a lifecycle +// action or DELETE on a name no policy covers was forwarded as an unknown +// container (`allowUnknown`). false models all three after the fix: a name +// that exactly one policy covers is allowed (`allowKnown`); any other target +// (an uncovered name, a full or short ID) is `resolve`d: the handler asks the +// daemon `GET /containers/{x}/json` and allows the request only if the +// container's name is covered (`allowResolved`), denying a 404, an uncovered +// name or any other daemon error. The daemon's lookup is the fixed table +// DAEMON_CONTAINERS, and `handle` is `route` followed by that lookup. +// // No instance models Go before #53. Go decoded before routing, which is // route(m, decodePath(segs)); with this table that agrees with the daemon // by construction, so the property cannot express Go's risk: decoding that @@ -78,6 +88,7 @@ module router_model { const ALLOW_ATTACH: bool const ALLOW_ANY_METHOD_PING: bool const ALLOW_DOT_SEGMENTS: bool + const ALLOW_UNCOVERED: bool // ─── Daemon decoding ──────────────────────────────────────────────── @@ -106,7 +117,25 @@ module router_model { // ─── Router ───────────────────────────────────────────────────────── pure val RESERVED = Set("create", "json", "exec") - pure val KNOWN_SERVICES = Set("beacon") + // The names the loaded policies cover: their container_names, or their + // service_name when container_names is unset (#86). + pure val COVERED_NAMES = Set("beacon") + + // ─── Daemon containers (#86) ──────────────────────────────────────── + + // What the daemon resolves an identifier to: a name or an ID (full or + // short) maps to the name of the container it denotes. `f00d` is the + // covered container's ID, `mycontainer` an operator container no policy + // covers and `c0ffee` its ID. Any other identifier is a 404. + pure val DAEMON_CONTAINERS: str -> str = Map( + "beacon" -> "beacon", + "f00d" -> "beacon", + "mycontainer" -> "mycontainer", + "c0ffee" -> "mycontainer", + ) + + pure def daemonHas(id: str): bool = + DAEMON_CONTAINERS.keys().contains(id) pure val BY_NAME_ACTIONS = Set("start", "stop", "restart", "kill", "wait", "pause", "unpause") pure val DENIED_ACTIONS = Set("rename", "update") @@ -128,7 +157,9 @@ module router_model { if (segs.length() == 0) "" else segs[segs.length() - 1] pure def routeByName(name: str): str = - if (KNOWN_SERVICES.contains(name)) "allowKnown" else "allowUnknown" + if (COVERED_NAMES.contains(name)) "allowKnown" + else if (ALLOW_UNCOVERED) "allowUnknown" + else "resolve" // Exec matches whole segments only (#49): the `/exec//…` namespace, // or `exec` anywhere after `containers`. A segment such as `exec-runner` @@ -185,12 +216,27 @@ module router_model { else if (method == "GET" or method == "HEAD") "allowRead" else "deny" + // The router's outcome, followed by the handler's daemon lookup for a + // `resolve` (#86): a container whose name is covered is allowed, anything + // else (a 404, an uncovered name) is denied. A daemon error other than 404 + // also denies; the table has none. + pure def handle(method: str, segs: List[str]): str = { + val r = route(method, segs) + if (r != "resolve") r + else if (daemonHas(segs[1]) and COVERED_NAMES.contains(DAEMON_CONTAINERS.get(segs[1]))) "allowResolved" + else "deny" + } + + pure def isLifecycleAllow(outcome: str): bool = + Set("allowKnown", "allowUnknown", "allowResolved").contains(outcome) + // ─── Path universe ────────────────────────────────────────────────── pure val WORDS = Set( "containers", "", "json", "create", "exec", "mycontainer", "beacon", "start", "stop", "%2F", "%2f", "%6A%73%6F%6E", "beacon%2Fstart", "exec-runner", "abc", "extra", "attach", "ws", "_ping", ".", "..", + "f00d", "c0ffee", ) pure val METHODS = Set("GET", "HEAD", "POST", "DELETE") @@ -201,16 +247,28 @@ module router_model { // ─── Properties ───────────────────────────────────────────────────── - // A lifecycle allow only ever targets a real, non-reserved name. + // A lifecycle allow, or a lookup for one, only ever targets a real, + // non-reserved name. pure val lifecycleOnlyTargetsRealNames: bool = METHODS.forall(m => PATHS.forall(segs => - Set("allowKnown", "allowUnknown").contains(route(m, segs)) implies and { + Set("allowKnown", "allowUnknown", "resolve").contains(route(m, segs)) implies and { segs.length() >= 2, segs[1] != "", not(RESERVED.contains(segs[1])), } )) + // No lifecycle action or DELETE is forwarded for an existing container + // outside every policy's covered names (#86), whether the request names + // it or gives its ID. A forwarded request for an identifier the daemon + // does not know gets a 404 from the daemon. It does not mention + // ALLOW_UNCOVERED or `resolve`, so that router_pre86 fails it. + pure val lifecycleOnlyCovered: bool = + METHODS.forall(m => PATHS.forall(segs => + isLifecycleAllow(handle(m, segs)) implies + (not(daemonHas(segs[1])) or COVERED_NAMES.contains(DAEMON_CONTAINERS.get(segs[1]))) + )) + // Any request the router does not deny reads the same to the daemon: // decoding leaves its path unchanged (#53). Ranges over GET too. pure val routerSeesWhatDaemonSees: bool = @@ -338,9 +396,6 @@ module router_model { run reservedExecDeleteDeniedTest = assert(route("DELETE", ["containers", "exec"]) == "denyExec") - run realNameDeleteAllowedTest = - assert(route("DELETE", ["containers", "mycontainer"]) == "allowUnknown") - run reservedInSubpathAllowedTest = assert(route("GET", ["containers", "mycontainer", "json"]) == "allowRead") @@ -352,12 +407,6 @@ module router_model { run execSubpathPostDeniedTest = assert(route("POST", ["containers", "mycontainer", "exec"]) == "denyExec") - run execPrefixNameStartAllowedTest = - assert(route("POST", ["containers", "exec-runner", "start"]) == "allowUnknown") - - run execPrefixNameDeleteAllowedTest = - assert(route("DELETE", ["containers", "exec-runner"]) == "allowUnknown") - run execPrefixNameGetAllowedTest = assert(route("GET", ["containers", "exec-runner", "json"]) == "allowRead") @@ -391,7 +440,7 @@ module router_model { // interior segment or a dot segment is denied, attach is denied, and the // read endpoints are read-only. module router { - import router_model(ALLOW_EMPTY_NAME = false, ALLOW_PERCENT = false, ALLOW_EMPTY_INTERIOR = false, READONLY = false, ALLOW_ATTACH = false, ALLOW_ANY_METHOD_PING = false, ALLOW_DOT_SEGMENTS = false).* + import router_model(ALLOW_EMPTY_NAME = false, ALLOW_PERCENT = false, ALLOW_EMPTY_INTERIOR = false, READONLY = false, ALLOW_ATTACH = false, ALLOW_ANY_METHOD_PING = false, ALLOW_DOT_SEGMENTS = false, ALLOW_UNCOVERED = false).* run emptyNameDeleteDeniedTest = assert(route("DELETE", ["containers", ""]) == "deny") @@ -503,6 +552,52 @@ module router { dotSegmentsDenied, dotSegmentsHideNoExec, }) + + // Lifecycle actions and DELETE only on covered containers (#86). Before + // #86 the first three rows were allowed as an unknown container. + + run realNameDeleteDeniedTest = + assert(and { + route("DELETE", ["containers", "mycontainer"]) == "resolve", + handle("DELETE", ["containers", "mycontainer"]) == "deny", + }) + + // The daemon answers 404 for these two. + run execPrefixNameStartDeniedTest = + assert(and { + route("POST", ["containers", "exec-runner", "start"]) == "resolve", + handle("POST", ["containers", "exec-runner", "start"]) == "deny", + }) + + run execPrefixNameDeleteDeniedTest = + assert(and { + route("DELETE", ["containers", "exec-runner"]) == "resolve", + handle("DELETE", ["containers", "exec-runner"]) == "deny", + }) + + run coveredNameStartAllowedTest = + assert(and { + route("POST", ["containers", "beacon", "start"]) == "allowKnown", + handle("POST", ["containers", "beacon", "start"]) == "allowKnown", + }) + + run coveredIdStartAllowedTest = + assert(and { + route("POST", ["containers", "f00d", "start"]) == "resolve", + handle("POST", ["containers", "f00d", "start"]) == "allowResolved", + }) + + run uncoveredNameStopDeniedTest = + assert(handle("POST", ["containers", "mycontainer", "stop"]) == "deny") + + run uncoveredIdStopDeniedTest = + assert(handle("POST", ["containers", "c0ffee", "stop"]) == "deny") + + run missingContainerDeleteDeniedTest = + assert(handle("DELETE", ["containers", "abc"]) == "deny") + + run coveredSoundTest = + assert(lifecycleOnlyCovered) } // Rust before #48: `/containers/` and `/containers//start` reach @@ -511,13 +606,13 @@ module router { // is off: with it on, `/containers//start` would be denied before it reached // the name branch, and the instance could not show #48. module router_pre48 { - import router_model(ALLOW_EMPTY_NAME = true, ALLOW_PERCENT = false, ALLOW_EMPTY_INTERIOR = true, READONLY = false, ALLOW_ATTACH = false, ALLOW_ANY_METHOD_PING = false, ALLOW_DOT_SEGMENTS = false).* + import router_model(ALLOW_EMPTY_NAME = true, ALLOW_PERCENT = false, ALLOW_EMPTY_INTERIOR = true, READONLY = false, ALLOW_ATTACH = false, ALLOW_ANY_METHOD_PING = false, ALLOW_DOT_SEGMENTS = false, ALLOW_UNCOVERED = false).* run pre48UnsoundTest = assert(and { not(lifecycleOnlyTargetsRealNames), - route("DELETE", ["containers", ""]) == "allowUnknown", - route("POST", ["containers", "", "start"]) == "allowUnknown", + route("DELETE", ["containers", ""]) == "resolve", + route("POST", ["containers", "", "start"]) == "resolve", }) } @@ -525,13 +620,13 @@ module router_pre48 { // encoded name such as `%2F` counted as an unknown container and the // request was forwarded. The daemon decodes it and reads a different path. module router_pre53 { - import router_model(ALLOW_EMPTY_NAME = false, ALLOW_PERCENT = true, ALLOW_EMPTY_INTERIOR = false, READONLY = false, ALLOW_ATTACH = false, ALLOW_ANY_METHOD_PING = false, ALLOW_DOT_SEGMENTS = false).* + import router_model(ALLOW_EMPTY_NAME = false, ALLOW_PERCENT = true, ALLOW_EMPTY_INTERIOR = false, READONLY = false, ALLOW_ATTACH = false, ALLOW_ANY_METHOD_PING = false, ALLOW_DOT_SEGMENTS = false, ALLOW_UNCOVERED = false).* run pre53UnsoundTest = assert(and { not(routerSeesWhatDaemonSees), - route("DELETE", ["containers", "%2F"]) == "allowUnknown", - route("POST", ["containers", "%2F", "start"]) == "allowUnknown", + route("DELETE", ["containers", "%2F"]) == "resolve", + route("POST", ["containers", "%2F", "start"]) == "resolve", route("GET", ["containers", "%2F"]) == "allowRead", }) } @@ -541,7 +636,7 @@ module router_pre53 { // check missed it and the GET passthrough forwarded it. The daemon answers // with a 301 to the clean path, so nothing leaked, but the router allowed it. module router_pre55 { - import router_model(ALLOW_EMPTY_NAME = false, ALLOW_PERCENT = false, ALLOW_EMPTY_INTERIOR = true, READONLY = false, ALLOW_ATTACH = false, ALLOW_ANY_METHOD_PING = false, ALLOW_DOT_SEGMENTS = false).* + import router_model(ALLOW_EMPTY_NAME = false, ALLOW_PERCENT = false, ALLOW_EMPTY_INTERIOR = true, READONLY = false, ALLOW_ATTACH = false, ALLOW_ANY_METHOD_PING = false, ALLOW_DOT_SEGMENTS = false, ALLOW_UNCOVERED = false).* run pre55UnsoundTest = assert(and { @@ -555,7 +650,7 @@ module router_pre55 { // `--match='^readonly.*Test$'`; the shared write rows in router_model assume the // flag is off. module router_readonly { - import router_model(ALLOW_EMPTY_NAME = false, ALLOW_PERCENT = false, ALLOW_EMPTY_INTERIOR = false, READONLY = true, ALLOW_ATTACH = false, ALLOW_ANY_METHOD_PING = false, ALLOW_DOT_SEGMENTS = false).* + import router_model(ALLOW_EMPTY_NAME = false, ALLOW_PERCENT = false, ALLOW_EMPTY_INTERIOR = false, READONLY = true, ALLOW_ATTACH = false, ALLOW_ANY_METHOD_PING = false, ALLOW_DOT_SEGMENTS = false, ALLOW_UNCOVERED = false).* run readonlySoundTest = assert(readonlyAllowsOnlyReads) @@ -581,7 +676,7 @@ module router_readonly { // and Go relayed the 101, giving the caller the container's stdin. Other // flags as `router`. module router_pre76 { - import router_model(ALLOW_EMPTY_NAME = false, ALLOW_PERCENT = false, ALLOW_EMPTY_INTERIOR = false, READONLY = false, ALLOW_ATTACH = true, ALLOW_ANY_METHOD_PING = false, ALLOW_DOT_SEGMENTS = false).* + import router_model(ALLOW_EMPTY_NAME = false, ALLOW_PERCENT = false, ALLOW_EMPTY_INTERIOR = false, READONLY = false, ALLOW_ATTACH = true, ALLOW_ANY_METHOD_PING = false, ALLOW_DOT_SEGMENTS = false, ALLOW_UNCOVERED = false).* run pre76UnsoundTest = assert(and { @@ -594,7 +689,7 @@ module router_pre76 { // `/info` and `/events…` for every method, so `POST /_ping` and // `DELETE /version` reached the daemon. Other flags as `router`. module router_pre78 { - import router_model(ALLOW_EMPTY_NAME = false, ALLOW_PERCENT = false, ALLOW_EMPTY_INTERIOR = false, READONLY = false, ALLOW_ATTACH = false, ALLOW_ANY_METHOD_PING = true, ALLOW_DOT_SEGMENTS = false).* + import router_model(ALLOW_EMPTY_NAME = false, ALLOW_PERCENT = false, ALLOW_EMPTY_INTERIOR = false, READONLY = false, ALLOW_ATTACH = false, ALLOW_ANY_METHOD_PING = true, ALLOW_DOT_SEGMENTS = false, ALLOW_UNCOVERED = false).* run pre78UnsoundTest = assert(and { @@ -610,7 +705,7 @@ module router_pre78 { // with a 301 to the clean path, so nothing leaked, but the router allowed it. // Other flags as `router`. module router_pre70 { - import router_model(ALLOW_EMPTY_NAME = false, ALLOW_PERCENT = false, ALLOW_EMPTY_INTERIOR = false, READONLY = false, ALLOW_ATTACH = false, ALLOW_ANY_METHOD_PING = false, ALLOW_DOT_SEGMENTS = true).* + import router_model(ALLOW_EMPTY_NAME = false, ALLOW_PERCENT = false, ALLOW_EMPTY_INTERIOR = false, READONLY = false, ALLOW_ATTACH = false, ALLOW_ANY_METHOD_PING = false, ALLOW_DOT_SEGMENTS = true, ALLOW_UNCOVERED = false).* run pre70UnsoundTest = assert(and { @@ -872,3 +967,18 @@ module pull_pre84 { routePull(cliPull("alpine")) == "allowPull", }) } + +// All three before #86: a lifecycle action or DELETE on a name no policy +// covers, an operator container's name or its ID included, was forwarded as +// an unknown container. Other flags as `router`. +module router_pre86 { + import router_model(ALLOW_EMPTY_NAME = false, ALLOW_PERCENT = false, ALLOW_EMPTY_INTERIOR = false, READONLY = false, ALLOW_ATTACH = false, ALLOW_ANY_METHOD_PING = false, ALLOW_DOT_SEGMENTS = false, ALLOW_UNCOVERED = true).* + + run pre86UnsoundTest = + assert(and { + not(lifecycleOnlyCovered), + route("DELETE", ["containers", "mycontainer"]) == "allowUnknown", + route("POST", ["containers", "c0ffee", "stop"]) == "allowUnknown", + route("POST", ["containers", "exec-runner", "start"]) == "allowUnknown", + }) +} diff --git a/ts/src/audit.ts b/ts/src/audit.ts index 8a77d29..04b9527 100644 --- a/ts/src/audit.ts +++ b/ts/src/audit.ts @@ -8,6 +8,10 @@ export class AuditLogger { this.log("ALLOW", method, uri, ""); } + allowReason(method: string, uri: string, reason: string) { + this.log("ALLOW", method, uri, reason); + } + deny(method: string, uri: string, reason: string) { this.log("DENY", method, uri, reason); } diff --git a/ts/src/handler.test.ts b/ts/src/handler.test.ts index c5caca5..8f01cfd 100644 --- a/ts/src/handler.test.ts +++ b/ts/src/handler.test.ts @@ -11,7 +11,7 @@ import { Chain } from "./middleware.js"; import { Router } from "./proxy.js"; import { Manager } from "./policy.js"; import { AuditLogger } from "./audit.js"; -import { Transport } from "./transport.js"; +import { Transport, type DaemonResponse } from "./transport.js"; class FakeTransport extends Transport { lastBody?: Buffer; @@ -447,3 +447,170 @@ describe("Handler image pull (#83)", () => { } }); }); + +const BEACON_ID = "4f6e1d2c9a7b8c3d4e5f60718293a4b5c6d7e8f9012345678901234567890abc"; +const MONITORING_ID = "9d8c7b6a5f4e3d2c1b0a99887766554433221100ffeeddccbbaa998877665544"; + +// Answers GET /containers/{id}/json from a table of id -> container name, as +// the daemon does, and records every request. +class DaemonTransport extends Transport { + requests: string[] = []; + lastBody?: Buffer; + status?: number; + names = new Map([ + ["beacon", "beacon"], + [BEACON_ID, "beacon"], + [BEACON_ID.slice(0, 12), "beacon"], + ["dsp-monitoring", "dsp-monitoring"], + [MONITORING_ID, "dsp-monitoring"], + [MONITORING_ID.slice(0, 12), "dsp-monitoring"], + ]); + + override async get(path: string): Promise { + this.requests.push(`GET ${path}`); + if (this.status !== undefined) return { statusCode: this.status, body: Buffer.from("boom") }; + const id = path.slice("/containers/".length, -"/json".length); + const name = this.names.get(id); + if (name === undefined) return { statusCode: 404, body: Buffer.from("no such container") }; + return { statusCode: 200, body: Buffer.from(JSON.stringify({ Id: "4f6e1d2c9a7b", Name: `/${name}` })) }; + } + + override async forward(req: IncomingMessage, res: ServerResponse, body?: Buffer): Promise { + this.requests.push(`${req.method} ${(req.url ?? "").split("?")[0]}`); + this.lastBody = body; + res.writeHead(204); + res.end(); + } +} + +function newDaemonHandler(files: Record) { + const dir = makeEnv(files); + const transport = new DaemonTransport(); + const handler = new Handler( + new Router(new Manager(dir)), + new Chain(), + new AuditLogger(join(dir, "audit.log")), + transport, + ); + return { handler, transport, dir }; +} + +describe("Handler container coverage", () => { + const notManaged = (id: string) => `container ${id} is not managed by any policy`; + const inspect = (id: string) => [`GET /containers/${id}/json`]; + const beaconShort = BEACON_ID.slice(0, 12); + const monShort = MONITORING_ID.slice(0, 12); + + // The handler half of #86: a lifecycle action or DELETE is forwarded only + // if its target is a container a policy covers. A name, full ID or short ID + // is resolved with GET /containers/{id}/json. Before #86 every row was + // forwarded. + it("forwards lifecycle actions only for covered containers", async () => { + const cases: [string, string, string, number, string, string[]][] = [ + ["covered name", "POST", "/v1.45/containers/beacon/stop?t=1", 204, "", []], + ["covered full ID", "POST", `/containers/${BEACON_ID}/kill`, 204, "", inspect(BEACON_ID)], + ["covered short ID", "DELETE", `/containers/${beaconShort}?force=1`, 204, "", inspect(beaconShort)], + // uncoveredNameStopDeniedTest: an operator container. + [ + "uncovered name", + "POST", + "/containers/dsp-monitoring/stop", + 403, + notManaged("dsp-monitoring"), + inspect("dsp-monitoring"), + ], + [ + "uncovered full ID", + "POST", + `/containers/${MONITORING_ID}/kill`, + 403, + notManaged(MONITORING_ID), + inspect(MONITORING_ID), + ], + ["uncovered short ID", "DELETE", `/containers/${monShort}`, 403, notManaged(monShort), inspect(monShort)], + // realNameDeleteDeniedTest: the daemon answers 404. + ["unknown name", "DELETE", "/containers/mycontainer", 403, notManaged("mycontainer"), inspect("mycontainer")], + // execPrefixNameStartDeniedTest + [ + "unknown exec-prefixed name", + "POST", + "/containers/exec-runner/start", + 403, + notManaged("exec-runner"), + inspect("exec-runner"), + ], + ]; + const mismatches: string[] = []; + for (const [name, method, target, status, body, inspects] of cases) { + const { handler, transport } = newDaemonHandler(beaconConfig); + const { res, recorder } = makeResponse(); + await handler.handle(makeRequest(method, target), res); + if (recorder.statusCode !== status || (body !== "" && recorder.body !== body)) { + mismatches.push(`${name}: got ${recorder.statusCode} ${recorder.body}, want ${status} ${body}`); + } + const want = [...inspects]; + if (status !== 403) want.push(`${method} ${target.split("?")[0]}`); + if (JSON.stringify(transport.requests) !== JSON.stringify(want)) { + mismatches.push(`${name}: daemon saw ${JSON.stringify(transport.requests)}, want ${JSON.stringify(want)}`); + } + } + assert.deepEqual(mismatches, []); + }); + + // A daemon error other than 404 fails closed with its own message (#86). + it("fails closed when the daemon cannot resolve a container", async () => { + const { handler, transport } = newDaemonHandler(beaconConfig); + transport.status = 500; + const { res, recorder } = makeResponse(); + await handler.handle(makeRequest("POST", `/containers/${beaconShort}/stop`), res); + assert.equal(recorder.statusCode, 403); + assert.equal(recorder.body, `cannot resolve container ${beaconShort}: daemon returned 500`); + assert.deepEqual(transport.requests, inspect(beaconShort)); + }); + + // The resolved name is audited with the request (#86). + it("audits the resolved name", async () => { + const { handler, dir } = newDaemonHandler(beaconConfig); + await handler.handle(makeRequest("POST", `/containers/${beaconShort}/start`), makeResponse().res); + await handler.handle(makeRequest("POST", `/containers/${monShort}/start`), makeResponse().res); + const lines = readFileSync(join(dir, "audit.log"), "utf-8") + .trim() + .split("\n") + .map((l) => JSON.parse(l) as { decision: string; reason: string }); + assert.equal(lines.length, 2); + assert.equal(lines[0]!.decision, "ALLOW"); + assert.equal(lines[0]!.reason, `container ${beaconShort} resolved to beacon`); + assert.equal(lines[1]!.decision, "DENY"); + assert.equal(lines[1]!.reason, `container ${monShort} is not managed by any policy (resolved to dsp-monitoring)`); + }); + + // The #85 reproduction through the handler: two policies for one image + // force different users, and each create named for one of them gets that + // policy's user, every time. + it("applies the policy selected by the create name", async () => { + const { handler, transport } = newDaemonHandler({ + "a.yaml": + 'service_name: svc-a\ncontainer_names: [svc-a]\nallowed_image_prefixes: [alpine]\ncontainer_config: { user: "1111:1111" }\n', + "b.yaml": + 'service_name: svc-b\ncontainer_names: [svc-b]\nallowed_image_prefixes: [alpine]\ncontainer_config: { user: "2222:2222" }\n', + }); + const body = JSON.stringify({ Image: "alpine:3.20", Cmd: ["true"] }); + for (let i = 0; i < 50; i++) { + for (const [name, user] of [ + ["svc-a", "1111:1111"], + ["svc-b", "2222:2222"], + ] as const) { + transport.lastBody = undefined; + const { res, recorder } = makeResponse(); + await handler.handle(makeRequest("POST", `/v1.45/containers/create?name=${name}`, body), res); + assert.equal(recorder.statusCode, 204, `iteration ${i} ${name}: ${recorder.body}`); + const fwd = JSON.parse(transport.lastBody!.toString("utf-8")) as { User?: string }; + assert.equal(fwd.User, user, `iteration ${i}: create ?name=${name}`); + } + } + const { res, recorder } = makeResponse(); + await handler.handle(makeRequest("POST", "/containers/create", JSON.stringify({ Image: "alpine:3.20" })), res); + assert.equal(recorder.statusCode, 403); + assert.equal(recorder.body, "image alpine:3.20 requires a container name"); + }); +}); diff --git a/ts/src/handler.ts b/ts/src/handler.ts index 07a4520..3badb4a 100644 --- a/ts/src/handler.ts +++ b/ts/src/handler.ts @@ -3,7 +3,7 @@ import type { IncomingMessage, ServerResponse } from "node:http"; import type { Duplex } from "node:stream"; import { AuditLogger } from "./audit.js"; import { Chain } from "./middleware.js"; -import { Action, Router } from "./proxy.js"; +import { Action, type RouteResult, Router } from "./proxy.js"; import { Transport } from "./transport.js"; export class Handler { @@ -53,12 +53,18 @@ export class Handler { } } - const route = this.router.route(method, path, bodyJSON, bodyBytes.length > 0); + let route = this.router.route(method, path, bodyJSON, bodyBytes.length > 0); + + // A lifecycle action or DELETE on a name that no policy covers directly: + // ask the daemon which container it is (#86). + if (route.action === Action.ResolveContainer) { + route = await this.resolveContainer(route.container ?? ""); + } if (route.action === Action.Deny) { const msg = route.denyMsg ?? "denied"; console.warn(`[${requestId}] DENIED ${method} ${path}: ${msg}`); - this.audit.deny(method, path, msg); + this.audit.deny(method, path, route.resolvedName ? `${msg} (resolved to ${route.resolvedName})` : msg); res.writeHead(403); res.end(msg); return; @@ -81,10 +87,59 @@ export class Handler { } console.log(`[${requestId}] ALLOWED ${method} ${path}`); - this.audit.allow(method, path); + if (route.resolvedName) { + this.audit.allowReason(method, path, `container ${route.container} resolved to ${route.resolvedName}`); + } else { + this.audit.allow(method, path); + } await this.transport.forward(req, res, bodyBytes.length > 0 ? bodyBytes : undefined); } + // Asks the daemon for GET /containers/{id}/json and allows the request only + // if the container's name is covered by a policy. A 404, an uncovered name + // or any other failure denies: it fails closed (#86). + private async resolveContainer(id: string): Promise { + const notManaged = (resolvedName?: string): RouteResult => ({ + action: Action.Deny, + container: id, + denyMsg: `container ${id} is not managed by any policy`, + ...(resolvedName ? { resolvedName } : {}), + }); + const cannotResolve = (reason: string): RouteResult => ({ + action: Action.Deny, + container: id, + denyMsg: `cannot resolve container ${id}: ${reason}`, + }); + + let resp; + try { + resp = await this.transport.get(`/containers/${encodeURIComponent(id)}/json`); + } catch (err) { + return cannotResolve(err instanceof Error ? err.message : String(err)); + } + if (resp.statusCode === 404) return notManaged(); + if (resp.statusCode !== 200) return cannotResolve(`daemon returned ${resp.statusCode}`); + + let name: unknown; + try { + name = JSON.parse(resp.body.toString("utf-8"))?.Name; + } catch { + name = undefined; + } + if (typeof name !== "string" || name === "") return cannotResolve("unreadable inspect response"); + if (name.startsWith("/")) name = name.slice(1); + const resolvedName = name as string; + + const covering = this.router.manager.covering(resolvedName); + if (covering.length === 0) return notManaged(resolvedName); + return { + action: Action.Allow, + container: id, + resolvedName, + ...(covering.length === 1 ? { service: covering[0].service_name, policy: covering[0] } : {}), + }; + } + // Answers a CONNECT request with 403 on the raw socket (#78). Node emits // CONNECT as the server's "connect" event, never as a request, and without // a listener it destroys the socket, so the caller got no response and diff --git a/ts/src/policy.test.ts b/ts/src/policy.test.ts index 57b6fd7..96e64fc 100644 --- a/ts/src/policy.test.ts +++ b/ts/src/policy.test.ts @@ -152,3 +152,151 @@ describe("parseImageRef (#84)", () => { } }); }); + +describe("Manager container names", () => { + let tmpDir: string; + + beforeEach(() => { + tmpDir = mkdtempSync(join(tmpdir(), "policy-names-")); + }); + + afterEach(() => { + rmSync(tmpDir, { recursive: true, force: true }); + }); + + // The cross-language parity guard for the #85 load-time checks. Two + // policies conflict when their image prefixes share a namespace and one has + // no container_names or their container_names intersect. Before #85 every + // row loaded, and a duplicate service_name silently replaced the first file. + it("validates policies at load time", () => { + const cases: [string, Record, string][] = [ + [ + "same prefix, no container_names", + { + "a.yaml": "service_name: a\nallowed_image_prefixes: [alpine]\n", + "b.yaml": "service_name: b\nallowed_image_prefixes: [alpine]\n", + }, + 'conflicting policies "a" (a.yaml) and "b" (b.yaml): image prefixes "alpine" and "alpine" overlap and "a" has no container_names', + ], + [ + "one side has no container_names", + { + "a.yaml": "service_name: a\ncontainer_names: [x]\nallowed_image_prefixes: [chainsafe/lodestar]\n", + "b.yaml": "service_name: b\nallowed_image_prefixes: [chainsafe]\n", + }, + 'conflicting policies "a" (a.yaml) and "b" (b.yaml): image prefixes "chainsafe/lodestar" and "chainsafe" overlap and "b" has no container_names', + ], + [ + "shared container name", + { + "a.yaml": "service_name: a\ncontainer_names: [x, y]\nallowed_image_prefixes: [chainsafe/lodestar]\n", + "b.yaml": "service_name: b\ncontainer_names: [z, y]\nallowed_image_prefixes: [chainsafe/lodestar]\n", + }, + 'conflicting policies "a" (a.yaml) and "b" (b.yaml): image prefixes "chainsafe/lodestar" and "chainsafe/lodestar" overlap and both accept container name "y"', + ], + [ + "disjoint container names", + { + "a.yaml": "service_name: a\ncontainer_names: [beacon]\nallowed_image_prefixes: [chainsafe/lodestar]\n", + "b.yaml": "service_name: b\ncontainer_names: [validator]\nallowed_image_prefixes: [chainsafe/lodestar]\n", + }, + "", + ], + [ + "sibling prefixes do not overlap", + { + "a.yaml": "service_name: a\nallowed_image_prefixes: [chainsafe/lodestar]\n", + "b.yaml": "service_name: b\nallowed_image_prefixes: [chainsafe/lodestar-backup]\n", + }, + "", + ], + [ + "duplicate service_name", + { + "a.yaml": "service_name: svc\nallowed_image_prefixes: [alpine]\n", + "b.yaml": "service_name: svc\nallowed_image_prefixes: [busybox]\n", + }, + 'policies in a.yaml and b.yaml both use service_name "svc"', + ], + [ + "invalid container name", + { "a.yaml": "service_name: a\ncontainer_names: [-bad]\nallowed_image_prefixes: [alpine]\n" }, + 'invalid container name "-bad" in container_names', + ], + [ + "container name with a slash", + { "a.yaml": "service_name: a\ncontainer_names: [a/b]\nallowed_image_prefixes: [alpine]\n" }, + 'invalid container name "a/b" in container_names', + ], + [ + "empty container_names", + { "a.yaml": "service_name: a\ncontainer_names: []\nallowed_image_prefixes: [alpine]\n" }, + "container_names must not be empty", + ], + ]; + const mismatches: string[] = []; + for (const [name, files, want] of cases) { + const dir = mkdtempSync(join(tmpDir, "case-")); + for (const [file, content] of Object.entries(files)) writeFileSync(join(dir, file), content); + let err: string | undefined; + try { + new Manager(dir); + } catch (e) { + err = (e as Error).message; + } + if (want === "" && err !== undefined) mismatches.push(`${name}: ${err}`); + if (want !== "" && err === undefined) mismatches.push(`${name}: loaded, want error ${want}`); + if (want !== "" && err !== undefined && !err.includes(want)) + mismatches.push(`${name}: error ${err}, want ${want}`); + } + assert.deepEqual(mismatches, []); + }); + + // #85 selection: the one policy matching the image and accepting the name, + // the nameless create, and ambiguity. + it("selects the create policy by image and name", () => { + const beacon = { + service_name: "beacon", + container_names: ["beacon"], + allowed_image_prefixes: ["chainsafe/lodestar"], + }; + const validator = { + service_name: "validator", + container_names: ["validator"], + allowed_image_prefixes: ["chainsafe/lodestar"], + }; + let m = Manager.fromPolicies([validator, beacon, { service_name: "any", allowed_image_prefixes: ["chainsafe"] }]); + for (let i = 0; i < 50; i++) { + for (const name of ["beacon", "validator"]) { + assert.deepEqual(m.selectForCreate("chainsafe/lodestar:v1", name), { + error: `ambiguous policy for image chainsafe/lodestar:v1 and name ${name}: any, ${name}`, + }); + } + } + const nameless = m.selectForCreate("chainsafe/lodestar:v1", ""); + assert.ok("policy" in nameless && nameless.policy.service_name === "any"); + + m = Manager.fromPolicies([validator, beacon]); + for (let i = 0; i < 50; i++) { + for (const name of ["beacon", "validator"]) { + const got = m.selectForCreate("chainsafe/lodestar:v1", name); + assert.ok("policy" in got && got.policy.service_name === name, `iteration ${i}: ${JSON.stringify(got)}`); + } + } + }); + + it("reports the policies covering a name", () => { + const m = Manager.fromPolicies([ + { service_name: "beacon", allowed_image_prefixes: ["chainsafe/lodestar"] }, + { service_name: "vc", container_names: ["validator"], allowed_image_prefixes: ["chainsafe/lodestar"] }, + ]); + for (const [name, want] of [ + ["beacon", 1], + ["validator", 1], + ["vc", 0], + ["other", 0], + ] as const) { + assert.equal(m.covering(name).length, want, `covering(${name})`); + } + }); +}); diff --git a/ts/src/policy.ts b/ts/src/policy.ts index e458a7e..00b9af8 100644 --- a/ts/src/policy.ts +++ b/ts/src/policy.ts @@ -4,6 +4,7 @@ import { parse } from "yaml"; export interface Policy { service_name: string; + container_names?: string[]; user_id?: string; group_id?: string; allowed_image_prefixes: string[]; @@ -38,10 +39,43 @@ export interface FlagRule { value_pattern: string; } +// A Docker container name without the optional leading slash (#85). +const VALID_CONTAINER_NAME = /^[a-zA-Z0-9][a-zA-Z0-9_.-]*$/; + +export function validContainerName(name: string): boolean { + return VALID_CONTAINER_NAME.test(name); +} + +// The container names a policy manages: its container_names, or its +// service_name when container_names is unset (#85, #86). Lifecycle actions and +// DELETE are forwarded only for covered names. +export function coveredNames(p: Policy): string[] { + return p.container_names ?? [p.service_name]; +} + +// Whether a create named `name` may use the policy: a policy without +// container_names accepts any name, including none (#85). +export function acceptsName(p: Policy, name: string): boolean { + return p.container_names === undefined || p.container_names === null || p.container_names.includes(name); +} + export class Manager { private policiesByName = new Map(); + // Sorted by service_name, so selection never depends on load order (#85). + private ordered: Policy[] = []; + + // Builds a manager without load-time validation, as Go's + // NewManagerFromPolicies and Rust's Manager::from_map do. Tests use it to + // reach states that the constructor refuses, such as ambiguity. + static fromPolicies(policies: Policy[]): Manager { + const m = new Manager(undefined); + for (const p of policies) m.policiesByName.set(p.service_name, p); + m.sortPolicies(); + return m; + } - constructor(configDir: string) { + constructor(configDir: string | undefined) { + if (configDir === undefined) return; let entries: string[]; try { entries = readdirSync(configDir); @@ -54,7 +88,8 @@ export class Manager { } } - for (const entry of entries) { + const fileOf = new Map(); + for (const entry of [...entries].sort()) { if (!entry.endsWith(".yaml") && !entry.endsWith(".yml")) continue; const filePath = join(configDir, entry); @@ -67,6 +102,25 @@ export class Manager { if (!policy.allowed_image_prefixes?.length) { throw new Error(`policy file ${entry} missing required field 'allowed_image_prefixes'`); } + if (policy.container_names === null) delete policy.container_names; + if (policy.container_names !== undefined) { + if (!Array.isArray(policy.container_names) || policy.container_names.length === 0) { + throw new Error(`policy file ${entry}: container_names must not be empty`); + } + const bad = policy.container_names.find((n) => typeof n !== "string" || !validContainerName(n)); + if (bad !== undefined) { + throw new Error(`policy file ${entry}: invalid container name ${JSON.stringify(bad)} in container_names`); + } + } + // A second file with the same service_name used to replace the first + // silently (#85). + const prev = fileOf.get(policy.service_name); + if (prev !== undefined) { + throw new Error( + `policies in ${prev} and ${entry} both use service_name ${JSON.stringify(policy.service_name)}`, + ); + } + fileOf.set(policy.service_name, entry); // Prefixes are compared in normalised form (#84). policy.allowed_image_prefixes = policy.allowed_image_prefixes.map((prefix) => { try { @@ -78,6 +132,43 @@ export class Manager { this.policiesByName.set(policy.service_name, policy); } + + this.sortPolicies(); + this.checkConflicts(fileOf); + } + + private sortPolicies(): void { + this.ordered = [...this.policiesByName.values()].sort((a, b) => + a.service_name < b.service_name ? -1 : a.service_name > b.service_name ? 1 : 0, + ); + } + + // Fails when two policies could both be selected for one create: their + // image prefixes share a namespace, and one of them accepts any name or + // their container_names intersect (#85). + private checkConflicts(fileOf: Map): void { + const q = JSON.stringify; + for (let i = 0; i < this.ordered.length; i++) { + for (const b of this.ordered.slice(i + 1)) { + const a = this.ordered[i]; + const pair = overlappingPrefixes(a.allowed_image_prefixes, b.allowed_image_prefixes); + if (!pair) continue; + let reason: string; + if (a.container_names === undefined) { + reason = `${q(a.service_name)} has no container_names`; + } else if (b.container_names === undefined) { + reason = `${q(b.service_name)} has no container_names`; + } else { + const shared = a.container_names.filter((n) => b.container_names!.includes(n)).sort(); + if (shared.length === 0) continue; + reason = `both accept container name ${q(shared[0])}`; + } + throw new Error( + `conflicting policies ${q(a.service_name)} (${fileOf.get(a.service_name)}) and ${q(b.service_name)} ` + + `(${fileOf.get(b.service_name)}): image prefixes ${q(pair[0])} and ${q(pair[1])} overlap and ${reason}`, + ); + } + } } get(serviceName: string): Policy | undefined { @@ -93,15 +184,61 @@ export class Manager { } catch { return undefined; } - for (const policy of this.policiesByName.values()) { - if (matchesImagePrefix(name, policy.allowed_image_prefixes)) return policy; + return this.matchingImage(name)[0]; + } + + // The policies, sorted by service_name, whose prefixes match a normalised + // repository name (#85). + matchingImage(name: string): Policy[] { + return this.ordered.filter((p) => matchesImagePrefix(name, p.allowed_image_prefixes)); + } + + // The one policy whose image prefixes match the normalised repository of + // imageRef and which accepts the container name ("" for a create without + // ?name=). No match, or more than one, is an error whose text is the deny + // message (#85). + selectForCreate(imageRef: string, name: string): { policy: Policy } | { error: string } { + let imageName: string; + try { + imageName = parseImageRef(imageRef).name; + } catch (err) { + return { error: (err as Error).message }; } - return undefined; + const matching = this.matchingImage(imageName); + const candidates = matching.filter((p) => acceptsName(p, name)); + if (matching.length === 0) return { error: `image ${imageRef} not allowed by any policy` }; + if (candidates.length === 0 && name === "") return { error: `image ${imageRef} requires a container name` }; + if (candidates.length === 0) { + return { error: `container name ${name} not allowed for image ${imageRef} by any policy` }; + } + if (candidates.length > 1) { + return { + error: `ambiguous policy for image ${imageRef} and name ${name}: ${candidates.map((p) => p.service_name).join(", ")}`, + }; + } + return { policy: candidates[0] }; + } + + // The policies, sorted by service_name, whose covered names include name + // (#86). + covering(name: string): Policy[] { + return this.ordered.filter((p) => coveredNames(p).includes(name)); } list(): string[] { - return [...this.policiesByName.keys()]; + return this.ordered.map((p) => p.service_name); + } +} + +// The first pair of prefixes where one equals the other or is a namespace (/) +// prefix of it. +function overlappingPrefixes(as: string[], bs: string[]): [string, string] | undefined { + for (const a of as) { + for (const b of bs) { + if (a === b || a.startsWith(b + "/") || b.startsWith(a + "/")) return [a, b]; + } } + return undefined; } // A parsed image reference (#84). `name` is the normalised repository: the diff --git a/ts/src/proxy.test.ts b/ts/src/proxy.test.ts index 89897c4..0bdeddb 100644 --- a/ts/src/proxy.test.ts +++ b/ts/src/proxy.test.ts @@ -103,9 +103,10 @@ describe("Router", () => { assert.equal(r.service, "nginx-svc"); }); - it("allows POST /containers/:name/start for unknown container", () => { + it("resolves POST /containers/:name/start for an uncovered container", () => { + // Not covered: resolved by the handler, never allowed directly (#86). const r = router.route("POST", "/containers/my-arbitrary/start"); - assert.equal(r.action, Action.Allow); + assert.equal(r.action, Action.ResolveContainer); assert.equal(r.container, "my-arbitrary"); }); @@ -174,9 +175,11 @@ describe("Router", () => { ["DELETE", "/containers/exec", Action.Deny, "exec is not allowed"], // Listing stays allowed, via the GET/HEAD passthrough. ["GET", "/containers/json", Action.Allow], - // A real container name is still routed as a container. - // realNameDeleteAllowedTest - ["DELETE", "/containers/mycontainer", Action.Allow], + // A real container name is still routed as a container. No policy + // covers it, so the daemon is asked which container it is, and the + // handler denies it (#86). + // realNameDeleteDeniedTest + ["DELETE", "/containers/mycontainer", Action.ResolveContainer], ["GET", "/containers/mycontainer", Action.Allow], // Reserved words are only reserved in the name position. // reservedInSubpathAllowedTest @@ -224,20 +227,20 @@ describe("Router", () => { // ^/v\d+(\.\d+)?/ with ASCII digits, in all three languages (#52, #57). it("routes dotted API-version paths like the unversioned ones", () => { const cases: [string, string, Record | undefined, Action][] = [ - // #52: dotted version, container lifecycle delete. - ["DELETE", "/v1.43/containers/foo", undefined, Action.Allow], - // #52: dotted version, container lifecycle start. - ["POST", "/v1.43/containers/beacon/start", undefined, Action.Allow], + // #52: dotted version, container lifecycle delete (uncovered, #86). + ["DELETE", "/v1.43/containers/foo", undefined, Action.ResolveContainer], + // #52: dotted version, container lifecycle start (covered). + ["POST", "/v1.43/containers/nginx-svc/start", undefined, Action.Allow], // #52: dotted version, create with a policy-allowed image. ["POST", "/v1.43/containers/create", { Image: "nginx:latest" }, Action.CreateContainer], // #52: undotted control — stripped correctly everywhere already. - ["POST", "/v1/containers/beacon/start", undefined, Action.Allow], + ["POST", "/v1/containers/nginx-svc/start", undefined, Action.Allow], // #52: the reserved segment survives the strip (#24 parity). ["DELETE", "/v1.43/containers/json", undefined, Action.Deny], - // #57: undotted version, container lifecycle delete. - ["DELETE", "/v1/containers/foo", undefined, Action.Allow], + // #57: undotted version, container lifecycle delete (uncovered, #86). + ["DELETE", "/v1/containers/foo", undefined, Action.ResolveContainer], // #57: multi-digit major version. - ["DELETE", "/v10.0/containers/foo", undefined, Action.Allow], + ["DELETE", "/v10.0/containers/foo", undefined, Action.ResolveContainer], // #57: /volumes/ is not a version; the daemon routes this as a volume removal. ["DELETE", "/volumes/containers/foo", undefined, Action.Deny], // #57: /version/ is not a version prefix. @@ -295,13 +298,13 @@ describe("Router", () => { ["DELETE", "/v%31/containers/foo", Action.Deny], // #53: network names that need escaping are denied, GET included. ["GET", "/networks/a%20b", Action.Deny], - // #53 control: a plain name is still routed as a container. - ["DELETE", "/containers/foo", Action.Allow], + // #53 control: a plain name is still routed as a container (#86). + ["DELETE", "/containers/foo", Action.ResolveContainer], // #53, TS-only: only the TS router receives the query string (Go and Rust // route URL.EscapedPath() / uri.path()). A % in the query is not checked. ["GET", "/containers/json?filters=%7B%7D", Action.Allow], // Non-GET, so the GET passthrough cannot hide a check placed before the split. - ["DELETE", "/containers/foo?force=%31", Action.Allow], + ["DELETE", "/containers/foo?force=%31", Action.ResolveContainer], ]; const mismatches: string[] = []; for (const [method, path, want] of cases) { @@ -330,10 +333,11 @@ describe("Router", () => { ["DELETE", "/containers/mycontainer/exec", undefined, Action.Deny, exec], // execSubpathPostDeniedTest (#49) ["POST", "/containers/mycontainer/exec", undefined, Action.Deny, exec], - // execPrefixNameStartAllowedTest (#49): an unknown container. - ["POST", "/containers/exec-runner/start", undefined, Action.Allow, undefined], - // execPrefixNameDeleteAllowedTest (#49): an unknown container. - ["DELETE", "/containers/exec-runner", undefined, Action.Allow, undefined], + // execPrefixNameStartDeniedTest (#49, #86): an uncovered container, + // routed for resolution; the handler denies it. + ["POST", "/containers/exec-runner/start", undefined, Action.ResolveContainer, undefined], + // execPrefixNameDeleteDeniedTest (#49, #86) + ["DELETE", "/containers/exec-runner", undefined, Action.ResolveContainer, undefined], // execPrefixNameGetAllowedTest (#49) ["GET", "/containers/exec-runner/json", undefined, Action.Allow, undefined], // execNamespaceGetDeniedTest (#49): exec inspect leaks command lines. @@ -349,7 +353,7 @@ describe("Router", () => { // A name ending in exec is a plain name (#49, language-only). ["GET", "/containers/myexec/json", undefined, Action.Allow, undefined], // A name starting with exec is a plain name (#49, language-only). - ["DELETE", "/containers/executor", undefined, Action.Allow, undefined], + ["DELETE", "/containers/executor", undefined, Action.ResolveContainer, undefined], // The reserved name, decided by the exec check (#49, language-only). ["GET", "/containers/exec/json", undefined, Action.Deny, exec], // A top-level name starting with exec is not the exec namespace (#49, language-only). @@ -818,3 +822,197 @@ describe("Router image pull and normalisation (#83, #84)", () => { assert.deepEqual(mismatches, []); }); }); + +// The #85 layout: beacon and validator run one image. +const LODESTAR_POLICIES: Record = { + "beacon.yaml": "service_name: beacon\ncontainer_names: [beacon]\nallowed_image_prefixes:\n - chainsafe/lodestar\n", + "validator.yaml": + "service_name: validator\ncontainer_names: [validator, validator-2]\nallowed_image_prefixes:\n - chainsafe/lodestar\n", + "monitoring.yaml": "service_name: monitoring\nallowed_image_prefixes:\n - prom/prometheus\n", +}; + +function loadManager(files: Record): Manager { + const dir = mkdtempSync(join(tmpdir(), "proxy-names-")); + try { + for (const [name, content] of Object.entries(files)) writeFileSync(join(dir, name), content); + return new Manager(dir); + } finally { + rmSync(dir, { recursive: true, force: true }); + } +} + +describe("Router container names", () => { + // The cross-language parity guard for #85. POST /containers/create selects + // the one policy whose image prefixes match and whose container_names + // contain ?name=; a policy without container_names accepts any name. Before + // #85 the policy was whichever matched the image first in map order (Go, + // Rust) or load order (TypeScript). + it("selects the create policy by image and name", () => { + const router = new Router(loadManager(LODESTAR_POLICIES)); + const lodestar = { Image: "chainsafe/lodestar:v1.20.0" }; + const prom = { Image: "prom/prometheus:latest" }; + const cases: [string, Record, Action, string | undefined, string | undefined][] = [ + ["name=beacon", lodestar, Action.CreateContainer, "beacon", undefined], + ["name=validator", lodestar, Action.CreateContainer, "validator", undefined], + ["name=validator-2", lodestar, Action.CreateContainer, "validator", undefined], + // The daemon strips one leading slash, and the name is decoded. + ["name=/validator", lodestar, Action.CreateContainer, "validator", undefined], + ["name=%2Fbeacon&platform=", lodestar, Action.CreateContainer, "beacon", undefined], + ["platform=linux&name=beacon", lodestar, Action.CreateContainer, "beacon", undefined], + // A name no policy for the image accepts. + [ + "name=other", + lodestar, + Action.Deny, + undefined, + "container name other not allowed for image chainsafe/lodestar:v1.20.0 by any policy", + ], + // A create without ?name= only matches a policy without container_names. + ["", lodestar, Action.Deny, undefined, "image chainsafe/lodestar:v1.20.0 requires a container name"], + ["name=", lodestar, Action.Deny, undefined, "image chainsafe/lodestar:v1.20.0 requires a container name"], + ["", prom, Action.CreateContainer, "monitoring", undefined], + ["name=anything", prom, Action.CreateContainer, "monitoring", undefined], + // The image decides first. + [ + "name=beacon", + { Image: "alpine:latest" }, + Action.Deny, + undefined, + "image alpine:latest not allowed by any policy", + ], + // An invalid or undecodable name is denied. + ["name=-beacon", lodestar, Action.Deny, undefined, "invalid container name"], + ["name=bea%2Fcon", lodestar, Action.Deny, undefined, "invalid container name"], + // Decoded as the daemon's r.Form does. + ["name=%62eacon", lodestar, Action.CreateContainer, "beacon", undefined], + ["name=validator%2D2", lodestar, Action.CreateContainer, "validator", undefined], + // The daemon takes the first of a repeated name; the proxy denies it. + ["name=beacon&name=monitoring", lodestar, Action.Deny, undefined, "duplicate name query parameter"], + ["name=beacon&n%61me=validator", lodestar, Action.Deny, undefined, "duplicate name query parameter"], + // A query the daemon rejects is denied, as for the pull (#83). + ["name=%zz", lodestar, Action.Deny, undefined, "invalid query string"], + ["platform=%zz&name=beacon", lodestar, Action.Deny, undefined, "invalid query string"], + ["name=beacon;x=1", lodestar, Action.Deny, undefined, "invalid query string"], + ]; + const mismatches: string[] = []; + for (const [query, body, want, service, msg] of cases) { + const target = "/v1.45/containers/create" + (query ? `?${query}` : ""); + const r = router.route("POST", target, body); + if (r.action !== want || r.service !== service || r.denyMsg !== msg) { + mismatches.push( + `create ?${query}: got ${Action[r.action]}/${r.service}/${r.denyMsg}, want ${Action[want]}/${service}/${msg}`, + ); + } + } + assert.deepEqual(mismatches, []); + }); + + // The #85 reproduction: with two policies for one image, every create gets + // its own policy, every time. + it("selects the same policy for a name every time", () => { + const router = new Router(loadManager(LODESTAR_POLICIES)); + for (let i = 0; i < 50; i++) { + for (const name of ["beacon", "validator"]) { + const r = router.route("POST", `/containers/create?name=${name}`, { Image: "chainsafe/lodestar:v1.20.0" }); + assert.equal(r.action, Action.CreateContainer, `iteration ${i}: ${r.denyMsg}`); + assert.equal(r.service, name, `iteration ${i}`); + } + } + }); + + // Two policies that both accept one image and name are refused at load + // time; built without that check, the router denies. + it("denies an ambiguous create", () => { + const router = new Router( + Manager.fromPolicies([ + { service_name: "b", allowed_image_prefixes: ["chainsafe/lodestar"] }, + { service_name: "a", allowed_image_prefixes: ["chainsafe"], container_names: ["beacon"] }, + ]), + ); + const body = { Image: "chainsafe/lodestar:v1" }; + let r = router.route("POST", "/containers/create?name=beacon", body); + assert.equal(r.action, Action.Deny); + assert.equal(r.denyMsg, "ambiguous policy for image chainsafe/lodestar:v1 and name beacon: a, b"); + r = router.route("POST", "/containers/create?name=validator", body); + assert.equal(r.action, Action.CreateContainer); + assert.equal(r.service, "b"); + }); + + // The cross-language parity guard for #86. A lifecycle action or DELETE is + // allowed directly only on a name that exactly one policy covers + // (container_names, or service_name when unset). Any other target is routed + // for resolution: the handler asks the daemon which container it is. Rows + // mirror the covered* and uncovered* runs in spec/router.qnt. + it("allows lifecycle actions directly only on covered names", () => { + const files = { + "validator.yaml": LODESTAR_POLICIES["validator.yaml"]!, + "monitoring.yaml": LODESTAR_POLICIES["monitoring.yaml"]!, + "vc.yaml": "service_name: vc-svc\ncontainer_names: [vc-1]\nallowed_image_prefixes:\n - example/vc\n", + "cafe.yaml": "service_name: cafe\nallowed_image_prefixes:\n - cafe/app\n", + }; + const router = new Router(loadManager(files)); + const resolve = Action.ResolveContainer; + const cases: [string, string, Action, string | undefined][] = [ + // coveredNameStartAllowedTest + ["POST", "/containers/validator/start", Action.Allow, "validator"], + ["POST", "/v1.45/containers/validator-2/kill", Action.Allow, "validator"], + ["DELETE", "/containers/validator", Action.Allow, "validator"], + // A policy without container_names covers its service_name. + ["POST", "/containers/monitoring/restart", Action.Allow, "monitoring"], + // With container_names set, service_name is not covered. + ["POST", "/containers/vc-svc/stop", resolve, undefined], + ["POST", "/containers/vc-1/stop", Action.Allow, "vc-svc"], + // uncoveredNameStopDeniedTest: an operator container, resolved first. + ["POST", "/containers/mycontainer/stop", resolve, undefined], + // An ID (full or short) is resolved by the daemon. + ["POST", "/containers/4f6e1d2c9a7b/kill", resolve, undefined], + ["DELETE", "/containers/4f6e1d2c9a7b8c3d4e5f60718293a4b5c6d7e8f9012345678901234567890abc", resolve, undefined], + // A covered name that could also be an ID prefix is resolved too. + ["POST", "/containers/cafe/start", resolve, undefined], + // Reads are not lifecycle actions. + ["GET", "/containers/mycontainer/json", Action.Allow, undefined], + ]; + const mismatches: string[] = []; + for (const [method, path, want, service] of cases) { + const r = router.route(method, path); + if (r.action !== want || r.service !== service) { + mismatches.push(`${method} ${path}: got ${Action[r.action]}/${r.service}, want ${Action[want]}/${service}`); + } + } + assert.deepEqual(mismatches, []); + + // --readonly still denies first (#75). + const ro = new Router(loadManager(files)).withReadonly(true); + assert.equal(ro.route("POST", "/containers/validator/start").action, Action.Deny); + }); +}); + +describe("Router pull with shared images", () => { + // Several policies may allow one image once container_names tells their + // creates apart (#85). A pull creates no container, so it is allowed when + // any policy for the image accepts its tag or digest, and denied with the + // first policy's reason (sorted by service_name) when none does. + it("allows a pull that any policy for the image accepts", () => { + const router = new Router( + loadManager({ + "vc-b.yaml": + "service_name: vc-b\ncontainer_names: [vc-b]\nallowed_image_prefixes: [example/vc]\nimage_tag_pattern: '^v2$'\n", + "vc-a.yaml": + "service_name: vc-a\ncontainer_names: [vc-a]\nallowed_image_prefixes: [example/vc]\nimage_tag_pattern: '^v1$'\n", + }), + ); + const cases: [string, Action, string | undefined, string | undefined][] = [ + ["v1", Action.Allow, "vc-a", undefined], + ["v2", Action.Allow, "vc-b", undefined], + ["v3", Action.Deny, undefined, 'image tag "v3" does not match pattern "^v1$"'], + ]; + for (let i = 0; i < 50; i++) { + for (const [tag, want, service, msg] of cases) { + const r = router.route("POST", `/images/create?fromImage=example%2Fvc&tag=${tag}`); + assert.equal(r.action, want, `iteration ${i}: pull tag ${tag}: ${r.denyMsg}`); + assert.equal(r.service, service, `iteration ${i}: pull tag ${tag}`); + assert.equal(r.denyMsg, msg, `iteration ${i}: pull tag ${tag}`); + } + } + }); +}); diff --git a/ts/src/proxy.ts b/ts/src/proxy.ts index 02a104b..5c8ccdc 100644 --- a/ts/src/proxy.ts +++ b/ts/src/proxy.ts @@ -1,10 +1,14 @@ import type { ImageRef, Policy } from "./policy.js"; -import { Manager, checkImageTag, parseImageRef } from "./policy.js"; +import { Manager, checkImageTag, parseImageRef, validContainerName } from "./policy.js"; export enum Action { Deny, Allow, CreateContainer, + // A lifecycle action or DELETE on a container that is not directly a + // covered name. The handler asks the daemon which container it is and + // allows the request only if its name is covered (#86). + ResolveContainer, } export interface RouteResult { @@ -13,6 +17,8 @@ export interface RouteResult { policy?: Policy; body?: Record; container?: string; + // The daemon's name for `container`, set when the handler resolved it (#86). + resolvedName?: string; image?: string; denyMsg?: string; } @@ -20,7 +26,7 @@ export interface RouteResult { export class Router { private readonlyMode = false; - constructor(private manager: Manager) {} + constructor(readonly manager: Manager) {} // Sets read-only mode (--readonly) and returns the router. withReadonly(readonly: boolean): this { @@ -57,7 +63,7 @@ export class Router { // Container create if (method === "POST" && path === "/containers/create") { - return this.routeCreate(body); + return this.routeCreate(body, query); } // Container lifecycle @@ -91,7 +97,7 @@ export class Router { return { action: Action.Deny, denyMsg: `endpoint ${method} ${path} is not allowed` }; } - private routeCreate(body?: Record): RouteResult { + private routeCreate(body: Record | undefined, query: string): RouteResult { if (!body || !Object.keys(body).length) { return { action: Action.Deny, denyMsg: "empty request body" }; } @@ -106,20 +112,36 @@ export class Router { return { action: Action.Deny, denyMsg: (err as Error).message }; } - const policy = this.manager.getByImage(image); - if (!policy) { - return { action: Action.Deny, denyMsg: `image ${image} not allowed by any policy` }; + const parsed = createName(query); + if ("error" in parsed) { + return { action: Action.Deny, denyMsg: parsed.error }; } + const name = parsed.name; - return { action: Action.CreateContainer, service: policy.service_name, policy, body, image }; + const selected = this.manager.selectForCreate(image, name); + if ("error" in selected) { + return { action: Action.Deny, denyMsg: selected.error }; + } + const policy = selected.policy; + return { + action: Action.CreateContainer, + service: policy.service_name, + policy, + body, + image, + ...(name ? { container: name } : {}), + }; } + // Allows a lifecycle action or DELETE on a name that exactly one policy + // covers. Anything else, including a name that could also be an ID prefix, + // needs the daemon to say which container it is (#86). private routeByName(name: string): RouteResult { - const policy = this.manager.get(name); - if (policy) { - return { action: Action.Allow, service: policy.service_name, policy }; + const covering = this.manager.covering(name); + if (covering.length === 1 && !couldBeId(name)) { + return { action: Action.Allow, service: covering[0].service_name, policy: covering[0], container: name }; } - return { action: Action.Allow, container: name }; + return { action: Action.ResolveContainer, container: name }; } // Image pull (#83). The Docker CLI sends fromImage and tag in the query @@ -156,11 +178,18 @@ export class Router { } // With neither, the daemon pulls every tag of the repository. if (!ref.tag && !ref.digest) return deny("image pull without a tag or digest pulls every tag and is not allowed"); - const policy = this.manager.getByImage(ref.name); - if (!policy) return deny(`image ${fromImage} not allowed by any policy`); - const reason = checkImageTag(policy, ref); - if (reason !== null) return deny(reason); - return { action: Action.Allow, service: policy.service_name, image: fromImage }; + // Several policies may allow the image once container_names tells their + // creates apart (#85). A pull creates no container, so any of them may + // accept the tag or digest; with none, the first one's reason is given. + const matching = this.manager.matchingImage(ref.name); + if (matching.length === 0) return deny(`image ${fromImage} not allowed by any policy`); + let firstReason: string | undefined; + for (const policy of matching) { + const reason = checkImageTag(policy, ref); + if (reason === null) return { action: Action.Allow, service: policy.service_name, image: fromImage }; + firstReason ??= reason; + } + return deny(firstReason!); } } @@ -203,6 +232,29 @@ function queryUnescape(s: string): string | undefined { return Buffer.from(bytes).toString("utf-8"); } +// The ?name= of a create, without the one leading slash the daemon also +// strips, or "" when there is none. The query is parsed as the daemon parses +// it, as the image pull does (#83). error is the deny message for a query the +// daemon would reject or an invalid name (#85). +function createName(query: string): { name: string } | { error: string } { + const params = parseQuery(query); + if (!params) return { error: "invalid query string" }; + // The daemon takes the first of a repeated name (r.Form.Get). Deny it rather + // than pick, as the pull does for its parameters. + if (params.filter(([k]) => k === "name").length > 1) return { error: "duplicate name query parameter" }; + let name = params.find(([k]) => k === "name")?.[1] ?? ""; + if (name.startsWith("/")) name = name.slice(1); + if (name !== "" && !validContainerName(name)) return { error: "invalid container name" }; + return { name }; +} + +// A name of lowercase hex digits only. The daemon looks up an ID, then a name, +// then an ID prefix, so if no container has this name it would act on the +// container whose ID starts with it (#86). +function couldBeId(name: string): boolean { + return /^[0-9a-f]+$/.test(name); +} + function stripAPIVersion(path: string): string { // Accepts /v and /v. (the Docker CLI sends /v1.43/) (#52). const match = path.match(/^\/v\d+(\.\d+)?\//); diff --git a/ts/src/transport.ts b/ts/src/transport.ts index d130409..1b859e4 100644 --- a/ts/src/transport.ts +++ b/ts/src/transport.ts @@ -10,9 +10,44 @@ export function isPermissionDenied(err: NodeJS.ErrnoException): boolean { return err.code === "EACCES" || err.code === "EPERM"; } +export interface DaemonResponse { + statusCode: number; + body: Buffer; +} + +// Bounds the response read by get(). +const MAX_GET_BODY = 4 << 20; + export class Transport { constructor(private socketPath: string = DEFAULT_SOCKET_PATH) {} + // Sends GET path to the daemon and returns its response, for the proxy's + // own lookups (#86). Nothing from the caller's request is forwarded. + get(path: string): Promise { + return new Promise((resolve, reject) => { + const dockerReq = request( + { socketPath: this.socketPath, path, method: "GET", headers: { Host: "docker" } }, + (dockerRes) => { + const chunks: Buffer[] = []; + let size = 0; + dockerRes.on("data", (chunk: Buffer) => { + size += chunk.length; + if (size > MAX_GET_BODY) { + dockerRes.destroy(); + reject(new Error("response too large")); + return; + } + chunks.push(chunk); + }); + dockerRes.on("end", () => resolve({ statusCode: dockerRes.statusCode ?? 500, body: Buffer.concat(chunks) })); + dockerRes.on("error", reject); + }, + ); + dockerReq.on("error", reject); + dockerReq.end(); + }); + } + forward(req: IncomingMessage, res: ServerResponse, body?: Buffer): Promise { return new Promise((resolve, reject) => { const headers: OutgoingHttpHeaders = { ...req.headers, Host: "docker" };