diff --git a/AGENTS.md b/AGENTS.md index 56c28a6..eddb8ef 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -27,7 +27,7 @@ deploy/ — Docker Compose + integration tests ## Architecture (same across all 3 languages) - `policy/` — Policy types + Manager (loads YAML from config dir) -- `middleware/` — Gate trait + Chain: ReadonlyGate, RegistryGate, MountSourceGate, EnvFileGate, CmdGate, ContainerConfigMutator +- `middleware/` — Gate trait + Chain: RegistryGate, MountSourceGate, EnvFileGate, CmdGate, ContainerConfigMutator (`--readonly` and exec are denied by the router) - `proxy/` — Router (default-deny), Handler (orchestrates chain), Transport (forwards to Docker socket) - `audit/` — JSON-structured logging @@ -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: 108 unit tests (main/listener: 24, policy: 10, middleware: 27, proxy: 43, audit: 4) -- Rust: 143 unit tests (main/listener: 23, policy: 15, middleware: 46, proxy: 45, handler: 8, audit: 4, transport: 2) -- TypeScript: 161 unit tests, 1 skipped (flags: 44, listen: 13 incl. 1 skipped concurrency test (#46), middleware: 38, proxy: 32, policy: 10, handler: 10, shutdown: 5, transport: 5, audit: 4) -- Integration, per implementation: 45 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`) +- Go: 108 unit tests (main/listener: 24, policy: 10, middleware: 25, proxy: 45, audit: 4) +- Rust: 140 unit tests (main/listener: 23, policy: 15, middleware: 41, proxy: 46, handler: 9, audit: 4, transport: 2) +- TypeScript: 157 unit tests, 1 skipped (flags: 44, listen: 13 incl. 1 skipped concurrency test (#46), middleware: 32, proxy: 33, policy: 10, handler: 11, shutdown: 5, transport: 5, audit: 4) +- Integration, per implementation: 50 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`, and `router_readonly`, whose `readonly*` tests run with `--match`) ## 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` (45 test cases) and `make test-integration-sock` (15 socket cases) via Docker Compose +- Integration: `make test-integration` (50 test cases) and `make test-integration-sock` (15 socket cases) via Docker Compose ## Contribution Workflow diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index 9961454..4e0d90c 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -34,7 +34,7 @@ To bypass a hook once, pass `--no-verify` to `git commit` or `git push`. CI runs All three languages implement the same architecture: - `policy/` — Policy types + Manager (loads YAML) -- `middleware/` — Gate trait + Chain (5 gates + 1 mutator: ReadonlyGate, RegistryGate, MountSourceGate, EnvFileGate, CmdGate, ContainerConfigMutator; exec is denied by the router) +- `middleware/` — Gate trait + Chain (4 gates + 1 mutator: RegistryGate, MountSourceGate, EnvFileGate, CmdGate, ContainerConfigMutator; `--readonly` and exec are denied by the router) - `proxy/` — Router (default-deny) + Handler + Transport Changes should be applied to **all three implementations** unless they are language-specific (e.g., CLI flag parsing). diff --git a/Makefile b/Makefile index d6862fb..f00b64f 100644 --- a/Makefile +++ b/Makefile @@ -102,6 +102,7 @@ test-spec: $(QUINT) test $(ROUTER_SPEC) --main=router_pre48 $(QUINT) test $(ROUTER_SPEC) --main=router_pre53 $(QUINT) test $(ROUTER_SPEC) --main=router_pre55 + $(QUINT) test $(ROUTER_SPEC) --main=router_readonly --match='^readonly.*Test$$' verify-ts: $(QUINT) run $(SPEC) --max-steps=50 --invariants allInvariants --backend typescript diff --git a/README.md b/README.md index 0b668a5..4136dc0 100644 --- a/README.md +++ b/README.md @@ -10,9 +10,9 @@ 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)) -- **Middleware pipeline**: 5 validation gates + 1 config mutator chain +- **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 percent-encoded paths, empty segments, exec and the container lifecycle +- **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, exec and the container lifecycle - **Audit logging**: JSON-structured logs for all requests and decisions - **Three implementations**: [Go](go/), [Rust](rs/), [TypeScript](ts/) — equal peer languages - **Minimal dependencies**: Zero external deps for Go, crate-based for Rust, npm for TypeScript @@ -100,9 +100,9 @@ All three implementations expose the same API surface, share the same [Quint spe | Language | Directory | Tests | Stack | |----------|-----------|-------|-------| -| Go | [go/](go/) | 108 unit + 45 integration + 15 socket | stdlib net/http + yaml.v3 | -| Rust | [rs/](rs/) | 143 unit + 45 integration + 15 socket | tokio, hyper, serde, clap | -| TypeScript | [ts/](ts/) | 161 unit (1 skipped) + 45 integration + 15 socket | Node 22 ESM, built-in http | +| Go | [go/](go/) | 108 unit + 50 integration + 15 socket | stdlib net/http + yaml.v3 | +| Rust | [rs/](rs/) | 140 unit + 50 integration + 15 socket | tokio, hyper, serde, clap | +| TypeScript | [ts/](ts/) | 157 unit (1 skipped) + 50 integration + 15 socket | Node 22 ESM, built-in http | ### Build All @@ -193,16 +193,18 @@ docker pull attacker/malware:latest # denied: image not in allowlist | Middleware | Type | What it checks | |------------|------|----------------| | ContainerConfigMutator | Mutator | Forces `network_mode`, `user`, `security_options`, `restart_policy` from policy | -| ReadonlyGate | Gate | Denies `POST`, `PUT`, `DELETE`, `PATCH` (optional `--readonly` flag). Like every gate it runs only on container create, so lifecycle actions and `DELETE` are not blocked yet ([#75](https://github.com/ChainSafe/docker-socket-policy/issues/75)) | | RegistryGate | Gate | Validates image ref against `allowed_image_prefixes` | | MountSourceGate | Gate | Validates volume binds against whitelist | | EnvFileGate | Gate | Strips `Env` field from create body; env must come from locked `env_file` | | CmdGate | Gate | Validates each CLI flag in `Cmd` array against allowlist + denylist | +The chain runs only on container create. `--readonly` is not a gate: the router denies every request that is not `GET` or `HEAD`, on every path, before any other check ([#75](https://github.com/ChainSafe/docker-socket-policy/issues/75)). A websocket attach (`GET /containers/{name}/attach/ws` with `Upgrade`) is a `GET` and is not denied yet; the Go proxy lets it write to a container's stdin ([#76](https://github.com/ChainSafe/docker-socket-policy/issues/76)). + ## Endpoint Access | HTTP Method | Path | Action | |-------------|------|--------| +| Not GET/HEAD | Any (with `--readonly`) | **DENIED** (checked first) | | Any | Path containing `%` | **DENIED** (see below) | | Any | Path containing `//` | **DENIED** | | POST | `/containers/create` | Validated by middleware chain | @@ -232,7 +234,7 @@ Exec is matched on whole path segments, so containers named like `exec-runner` w | `--docker-host` | `/var/run/docker.sock` | Docker daemon socket path (Unix socket only) | | `--config-dir` | `/etc/docker-socket-policy/services` | Policy config directory | | `--log-file` | `/var/log/docker-socket-policy.log` | Audit log path | -| `--readonly` | `false` | Enable read-only mode | +| `--readonly` | `false` | Enable read-only mode: the router denies every request that is not `GET` or `HEAD` | | `--listen-socket-group` | `docker-socket-policy` | Group owning the socket (default `docker-socket-policy`; `""` = the proxy's own group) | > **Unix socket security boundary**: the proxy listens on a Unix socket only, @@ -439,7 +441,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 9 invariants on every state transition. 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 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)) and exec ([#49](https://github.com/ChainSafe/docker-socket-policy/issues/49)), 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 7 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 9 invariants on every state transition. 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)) and exec ([#49](https://github.com/ChainSafe/docker-socket-policy/issues/49)), 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 8 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 CI pipeline runs verification on every push and PR. A violation blocks the build. diff --git a/deploy/docker-compose.yml b/deploy/docker-compose.yml index 17fbc2b..f0b3783 100644 --- a/deploy/docker-compose.yml +++ b/deploy/docker-compose.yml @@ -16,14 +16,33 @@ services: - --config-dir=/etc/docker-socket-policy/services - --log-file=/tmp/docker-socket-policy.log + # The same proxy with --readonly (#75), on its own socket in the shared volume. + proxy-readonly: + build: + context: ../${IMPL:-go} + dockerfile: Dockerfile + user: "0:0" + volumes: + - /var/run/docker.sock:/var/run/docker.sock + - ./config:/etc/docker-socket-policy/services:ro + - proxy-sock:/sock + command: + - --docker-host=/var/run/docker.sock + - --listen-socket=/sock/readonly.sock + - --config-dir=/etc/docker-socket-policy/services + - --log-file=/tmp/docker-socket-policy-readonly.log + - --readonly + test: build: context: . dockerfile: Dockerfile.test depends_on: - proxy + - proxy-readonly environment: PROXY_SOCK: /sock/proxy.sock + PROXY_RO_SOCK: /sock/readonly.sock # Hostname the proxy would have been reachable on when it still had a # TCP listener, so the suite can assert that it no longer is. PROXY_HOST: proxy diff --git a/deploy/test.sh b/deploy/test.sh index 4ecd2e7..c111d56 100755 --- a/deploy/test.sh +++ b/deploy/test.sh @@ -2,8 +2,9 @@ # Integration tests for docker-socket-policy. # Sends HTTP requests directly to the proxy's Docker API and checks status codes, # and the deny message on every expected proxy 403. -# The proxy's ReadonlyGate, RegistryGate, MountSourceGate, EnvFileGate, -# CmdGate, and ContainerConfigMutator are all exercised; exec is denied by the router. +# The proxy's RegistryGate, MountSourceGate, EnvFileGate, CmdGate, and +# ContainerConfigMutator are all exercised; exec is denied by the router, and so +# is every write on a second proxy run with --readonly (#75). set -e @@ -29,24 +30,31 @@ fi # instead of failing it — and the readiness counter below would never advance. TIMEOUT="--max-time 10 --connect-timeout 2" -# Wait for the proxy to create and serve its socket. -echo "Waiting for proxy at ${PROXY_SOCK}..." -i=0 -while [ $i -lt 30 ]; do - if curl -sf $TIMEOUT --unix-socket "$PROXY_SOCK" "$PROXY/_ping" >/dev/null 2>&1; then - echo "Proxy ready." - break +# The --readonly proxy (#75) serves the same config on its own socket. +PROXY_RO_SOCK="${PROXY_RO_SOCK:-/sock/readonly.sock}" + +# Wait for a proxy to create and serve its socket. +wait_for_sock() { + echo "Waiting for proxy at $1..." + i=0 + while [ $i -lt 30 ]; do + if curl -sf $TIMEOUT --unix-socket "$1" "$PROXY/_ping" >/dev/null 2>&1; then + echo "Proxy ready." + break + fi + printf "." + sleep 1 + i=$((i + 1)) + done + if [ $i -eq 30 ]; then + echo "" + echo "ERROR: Proxy at $1 failed to respond within 30s" + exit 1 fi - printf "." - sleep 1 - i=$((i + 1)) -done -if [ $i -eq 30 ]; then echo "" - echo "ERROR: Proxy failed to respond within 30s" - exit 1 -fi -echo "" +} +wait_for_sock "$PROXY_SOCK" +wait_for_sock "$PROXY_RO_SOCK" # Helpers: report the HTTP status code of a request sent over the Unix socket. # curl exits non-zero when it cannot connect at all, which under `set -e` would @@ -440,6 +448,34 @@ S="${S:-000}" check_denied "GET http://docker/exec/*/json -> 403 (absolute-form target)" "$S" "$BODY" \ 'exec is not allowed' 'empty path segment not allowed' +# ─── Read-only mode (#75) ──────────────────────────────── + +# The same config behind --readonly. Every request whose method is not GET or +# HEAD is denied by the router. Before #75 the flag only reached the gate +# chain, which runs on create alone, so these reached the daemon (a 404 for +# the unknown container below, i.e. a leak, not a proxy 403). +echo "" +echo "--- Read-only mode ---" + +# The request helpers read PROXY_SOCK, so this switches every check below to +# the read-only proxy. Keep this section last. +PROXY_SOCK="$PROXY_RO_SOCK" + +S=$(get_status "$PROXY/_ping") +check "GET /_ping -> 200 (read-only)" "200" "$S" + +S=$(post_empty "$PROXY/containers/no-such-container/stop") +check_denied "POST /containers/*/stop -> 403 (read-only)" "$S" "$BODY" 'read-only mode: POST requests are not allowed' + +S=$(delete_status "$PROXY/containers/no-such-container") +check_denied "DELETE /containers/* -> 403 (read-only)" "$S" "$BODY" 'read-only mode: DELETE requests are not allowed' + +S=$(post_json '{"Image":"chainsafe/lodestar:beacon","Cmd":["--rcConfig","/data/config.yml"]}' "$PROXY/containers/create") +check_denied "POST /containers/create with allowed image -> 403 (read-only)" "$S" "$BODY" 'read-only mode: POST requests are not allowed' + +S=$(post_empty "$PROXY/images/create?fromImage=chainsafe/lodestar&tag=beacon") +check_denied "POST /images/create?fromImage=chainsafe/lodestar -> 403 (read-only)" "$S" "$BODY" 'read-only mode: POST requests are not allowed' + # ─── Summary ────────────────────────────────────────── echo "" diff --git a/go/internal/middleware/chain.go b/go/internal/middleware/chain.go index 5ae4b01..d9bfda1 100644 --- a/go/internal/middleware/chain.go +++ b/go/internal/middleware/chain.go @@ -26,12 +26,9 @@ type Chain struct { mutators []Mutator } -func NewChain(readonly bool) *Chain { +func NewChain() *Chain { c := &Chain{} - if readonly { - c.gates = append(c.gates, &ReadonlyGate{}) - } c.gates = append(c.gates, &RegistryGate{}) c.gates = append(c.gates, &MountSourceGate{}) c.gates = append(c.gates, &EnvFileGate{}) diff --git a/go/internal/middleware/chain_test.go b/go/internal/middleware/chain_test.go index 8843112..d414859 100644 --- a/go/internal/middleware/chain_test.go +++ b/go/internal/middleware/chain_test.go @@ -64,37 +64,8 @@ func testPolicy(overrides map[string]interface{}) *policy.Policy { return p } -func TestChain_ReadonlyDeniesWrites(t *testing.T) { - for _, method := range []string{"POST", "PUT", "DELETE", "PATCH"} { - c := NewChain(true) - r := testRequest(method, "/containers/create") - body := map[string]interface{}{"Image": "chainsafe/lodestar:next"} - p := testPolicy(nil) - result := c.Execute(r, p, body) - if result.Allowed { - t.Fatalf("expected %s to be denied in read-only mode", method) - } - if !strings.Contains(result.Reason, "read-only") { - t.Fatalf("expected read-only reason, got: %s", result.Reason) - } - } -} - -func TestChain_ReadonlyAllowsReads(t *testing.T) { - c := NewChain(true) - for _, method := range []string{"GET", "HEAD"} { - r := testRequest(method, "/containers/json") - body := map[string]interface{}{} - p := testPolicy(nil) - result := c.Execute(r, p, body) - if !result.Allowed { - t.Fatalf("expected %s to be allowed in read-only mode, got: %s", method, result.Reason) - } - } -} - func TestRegistryGate_ValidImage(t *testing.T) { - c := NewChain(false) + c := NewChain() r := testRequest("POST", "/containers/create") body := map[string]interface{}{"Image": "chainsafe/lodestar:next"} p := testPolicy(nil) @@ -105,7 +76,7 @@ func TestRegistryGate_ValidImage(t *testing.T) { } func TestRegistryGate_DeniedImage(t *testing.T) { - c := NewChain(false) + c := NewChain() r := testRequest("POST", "/containers/create") body := map[string]interface{}{"Image": "ubuntu:latest"} p := testPolicy(nil) @@ -116,7 +87,7 @@ func TestRegistryGate_DeniedImage(t *testing.T) { } func TestRegistryGate_ValidDigest(t *testing.T) { - c := NewChain(false) + c := NewChain() r := testRequest("POST", "/containers/create") body := map[string]interface{}{"Image": "chainsafe/lodestar@sha256:abcdef0123456789abcdef0123456789abcdef0123456789abcdef0123456789"} p := testPolicy(nil) @@ -127,7 +98,7 @@ func TestRegistryGate_ValidDigest(t *testing.T) { } func TestRegistryGate_DigestNotAllowed(t *testing.T) { - c := NewChain(false) + c := NewChain() r := testRequest("POST", "/containers/create") body := map[string]interface{}{"Image": "chainsafe/lodestar@sha256:abcdef0123456789abcdef0123456789abcdef0123456789abcdef0123456789"} p := testPolicy(map[string]interface{}{ @@ -141,7 +112,7 @@ func TestRegistryGate_DigestNotAllowed(t *testing.T) { } func TestMountSourceGate_AllowedVolume(t *testing.T) { - c := NewChain(false) + c := NewChain() r := testRequest("POST", "/containers/create") body := map[string]interface{}{ "Image": "chainsafe/lodestar:next", @@ -157,7 +128,7 @@ func TestMountSourceGate_AllowedVolume(t *testing.T) { } func TestMountSourceGate_DeniedVolume(t *testing.T) { - c := NewChain(false) + c := NewChain() r := testRequest("POST", "/containers/create") body := map[string]interface{}{ "Image": "chainsafe/lodestar:next", @@ -173,7 +144,7 @@ func TestMountSourceGate_DeniedVolume(t *testing.T) { } func TestEnvFileGate_RejectsInlineEnv(t *testing.T) { - c := NewChain(false) + c := NewChain() r := testRequest("POST", "/containers/create") body := map[string]interface{}{ "Image": "chainsafe/lodestar:next", @@ -187,7 +158,7 @@ func TestEnvFileGate_RejectsInlineEnv(t *testing.T) { } func TestEnvFileGate_AllowsNoEnv(t *testing.T) { - c := NewChain(false) + c := NewChain() r := testRequest("POST", "/containers/create") body := map[string]interface{}{ "Image": "chainsafe/lodestar:next", @@ -200,7 +171,7 @@ func TestEnvFileGate_AllowsNoEnv(t *testing.T) { } func TestEnvFileGate_SkipsWhenNoEnvFile(t *testing.T) { - c := NewChain(false) + c := NewChain() r := testRequest("POST", "/containers/create") body := map[string]interface{}{ "Image": "chainsafe/lodestar:next", @@ -214,7 +185,7 @@ func TestEnvFileGate_SkipsWhenNoEnvFile(t *testing.T) { } func TestCmdGate_AllowsAllowedFlag(t *testing.T) { - c := NewChain(false) + c := NewChain() r := testRequest("POST", "/containers/create") body := map[string]interface{}{ "Image": "chainsafe/lodestar:next", @@ -228,7 +199,7 @@ func TestCmdGate_AllowsAllowedFlag(t *testing.T) { } func TestCmdGate_DeniesDeniedFlag(t *testing.T) { - c := NewChain(false) + c := NewChain() r := testRequest("POST", "/containers/create") body := map[string]interface{}{ "Image": "chainsafe/lodestar:next", @@ -242,7 +213,7 @@ func TestCmdGate_DeniesDeniedFlag(t *testing.T) { } func TestCmdGate_DeniesNotAllowedFlag(t *testing.T) { - c := NewChain(false) + c := NewChain() r := testRequest("POST", "/containers/create") body := map[string]interface{}{ "Image": "chainsafe/lodestar:next", @@ -256,7 +227,7 @@ func TestCmdGate_DeniesNotAllowedFlag(t *testing.T) { } func TestCmdGate_ValidatesFlagValue(t *testing.T) { - c := NewChain(false) + c := NewChain() r := testRequest("POST", "/containers/create") body := map[string]interface{}{ "Image": "chainsafe/lodestar:next", @@ -270,7 +241,7 @@ func TestCmdGate_ValidatesFlagValue(t *testing.T) { } func TestCmdGate_RejectsInvalidFlagValue(t *testing.T) { - c := NewChain(false) + c := NewChain() r := testRequest("POST", "/containers/create") body := map[string]interface{}{ "Image": "chainsafe/lodestar:next", @@ -284,7 +255,7 @@ func TestCmdGate_RejectsInvalidFlagValue(t *testing.T) { } func TestCmdGate_SafeCharset(t *testing.T) { - c := NewChain(false) + c := NewChain() r := testRequest("POST", "/containers/create") body := map[string]interface{}{ "Image": "chainsafe/lodestar:next", @@ -298,7 +269,7 @@ func TestCmdGate_SafeCharset(t *testing.T) { } func TestContainerConfigMutator_SetsNetworkMode(t *testing.T) { - c := NewChain(false) + c := NewChain() r := testRequest("POST", "/containers/create") body := map[string]interface{}{ "Image": "chainsafe/lodestar:next", @@ -314,7 +285,7 @@ func TestContainerConfigMutator_SetsNetworkMode(t *testing.T) { } func TestContainerConfigMutator_OverridesHostConfig(t *testing.T) { - c := NewChain(false) + c := NewChain() r := testRequest("POST", "/containers/create") body := map[string]interface{}{ "Image": "chainsafe/lodestar:next", @@ -331,7 +302,7 @@ func TestContainerConfigMutator_OverridesHostConfig(t *testing.T) { } func TestChain_MutatorRunsBeforeGates(t *testing.T) { - c := NewChain(false) + c := NewChain() r := testRequest("POST", "/containers/create") body := map[string]interface{}{ "Image": "chainsafe/lodestar:next", @@ -347,7 +318,7 @@ func TestChain_MutatorRunsBeforeGates(t *testing.T) { } func TestRegistryGate_RejectsInvalidDigestFormat(t *testing.T) { - c := NewChain(false) + c := NewChain() r := testRequest("POST", "/containers/create") body := map[string]interface{}{"Image": "chainsafe/lodestar@sha256:xyz"} p := testPolicy(nil) @@ -358,7 +329,7 @@ func TestRegistryGate_RejectsInvalidDigestFormat(t *testing.T) { } func TestRegistryGate_RejectsOversizedTag(t *testing.T) { - c := NewChain(false) + c := NewChain() r := testRequest("POST", "/containers/create") longTag := strings.Repeat("a", 129) body := map[string]interface{}{"Image": "chainsafe/lodestar:" + longTag} @@ -370,7 +341,7 @@ func TestRegistryGate_RejectsOversizedTag(t *testing.T) { } func TestRegistryGate_RejectsTagOutsidePattern(t *testing.T) { - c := NewChain(false) + c := NewChain() r := testRequest("POST", "/containers/create") body := map[string]interface{}{"Image": "chainsafe/lodestar:2.1"} p := testPolicy(nil) @@ -382,7 +353,7 @@ func TestRegistryGate_RejectsTagOutsidePattern(t *testing.T) { } func TestCmdGate_ValidatesInlineFlagValue(t *testing.T) { - c := NewChain(false) + c := NewChain() r := testRequest("POST", "/containers/create") body := map[string]interface{}{ "Image": "chainsafe/lodestar:next", @@ -396,7 +367,7 @@ func TestCmdGate_ValidatesInlineFlagValue(t *testing.T) { } func TestCmdGate_RejectsInvalidInlineFlagValue(t *testing.T) { - c := NewChain(false) + c := NewChain() r := testRequest("POST", "/containers/create") body := map[string]interface{}{ "Image": "chainsafe/lodestar:next", @@ -410,7 +381,7 @@ func TestCmdGate_RejectsInvalidInlineFlagValue(t *testing.T) { } func TestEnvFileGate_RejectsHostConfigEnv(t *testing.T) { - c := NewChain(false) + c := NewChain() r := testRequest("POST", "/containers/create") body := map[string]interface{}{ "Image": "chainsafe/lodestar:next", @@ -426,7 +397,7 @@ func TestEnvFileGate_RejectsHostConfigEnv(t *testing.T) { } func TestMountSourceGate_DeniesTopLevelVolumes(t *testing.T) { - c := NewChain(false) + c := NewChain() r := testRequest("POST", "/containers/create") body := map[string]interface{}{ "Image": "chainsafe/lodestar:next", diff --git a/go/internal/middleware/readonly.go b/go/internal/middleware/readonly.go deleted file mode 100644 index 4401cc8..0000000 --- a/go/internal/middleware/readonly.go +++ /dev/null @@ -1,18 +0,0 @@ -package middleware - -import ( - "fmt" - "net/http" - - "github.com/ChainSafe/docker-socket-policy/go/internal/policy" -) - -type ReadonlyGate struct{} - -func (g *ReadonlyGate) Check(r *http.Request, p *policy.Policy, body map[string]interface{}) error { - switch r.Method { - case "POST", "PUT", "DELETE", "PATCH": - return fmt.Errorf("read-only mode: %s requests are not allowed", r.Method) - } - return nil -} diff --git a/go/internal/proxy/handler_test.go b/go/internal/proxy/handler_test.go index 5990ec7..1e9baa9 100644 --- a/go/internal/proxy/handler_test.go +++ b/go/internal/proxy/handler_test.go @@ -67,7 +67,7 @@ allowed_image_prefixes: t.Cleanup(func() { auditLog.Close() }) router := NewRouter(policyManager) - chain := middleware.NewChain(false) + chain := middleware.NewChain() return NewHandler(router, chain, auditLog, transport) } @@ -175,7 +175,7 @@ func TestHandlerAuditURIIsRequestTarget(t *testing.T) { t.Fatalf("NewLogger failed: %v", err) } defer auditLog.Close() - h := NewHandler(NewRouter(policyManager), middleware.NewChain(false), auditLog, &recorderTransport{}) + h := NewHandler(NewRouter(policyManager), middleware.NewChain(), auditLog, &recorderTransport{}) targets := []struct{ method, target, decision string }{ {"GET", "/containers/json?all=1", "ALLOW"}, @@ -210,6 +210,28 @@ func TestHandlerAuditURIIsRequestTarget(t *testing.T) { } } +// #75: --readonly is enforced in the router, so a delete never reaches the +// daemon. Before #75 the flag only reached the chain, which runs on create. +func TestHandler_ReadonlyDeniesDelete(t *testing.T) { + rec := &recorderTransport{} + h := newTestHandler(t, nil, rec) + h.router = h.router.WithReadonly(true) + + req := httptest.NewRequest("DELETE", "/containers/mycontainer", nil) + w := httptest.NewRecorder() + h.ServeHTTP(w, req) + + if w.Code != http.StatusForbidden { + t.Fatalf("expected 403, got %d", w.Code) + } + if got := strings.TrimSpace(w.Body.String()); got != "read-only mode: DELETE requests are not allowed" { + t.Fatalf("deny reason = %q", got) + } + if rec.lastRequest != nil { + t.Fatal("expected no forward in read-only mode") + } +} + func TestHandler_CreateContainerValid(t *testing.T) { rec := &recorderTransport{} h := newTestHandler(t, map[string]string{ diff --git a/go/internal/proxy/router.go b/go/internal/proxy/router.go index 24b6f15..189689b 100644 --- a/go/internal/proxy/router.go +++ b/go/internal/proxy/router.go @@ -27,14 +27,26 @@ type RouteResult struct { } type Router struct { - manager *policy.Manager + manager *policy.Manager + readonly bool } func NewRouter(manager *policy.Manager) *Router { return &Router{manager: manager} } +// WithReadonly sets read-only mode (--readonly) and returns the router. +func (r *Router) WithReadonly(readonly bool) *Router { + r.readonly = readonly + return r +} + func (r *Router) Route(method, path string, body map[string]interface{}) *RouteResult { + // Read-only mode denies every write, for every path (#75). + if r.readonly && method != "GET" && method != "HEAD" { + return &RouteResult{Action: ActionDeny, DenyMsg: fmt.Sprintf("read-only mode: %s requests are not allowed", method)} + } + // The daemon decodes the path before routing, so deny any escape (#53). if strings.Contains(path, "%") { return &RouteResult{Action: ActionDeny, DenyMsg: "percent-encoded path not allowed"} diff --git a/go/internal/proxy/router_test.go b/go/internal/proxy/router_test.go index 39f4cd7..fc21b71 100644 --- a/go/internal/proxy/router_test.go +++ b/go/internal/proxy/router_test.go @@ -678,3 +678,70 @@ func TestExtractContainerNameSkipsEmptySegment(t *testing.T) { } } } + +// TestRouteReadonly is the cross-language parity guard for #75. +// +// With --readonly, every request whose method is not GET or HEAD is denied +// first in the router, for every path. Before #75 the flag only reached the +// middleware chain, which runs on the create route alone, so lifecycle +// POSTs, DELETE /containers/{name} and image pulls were forwarded. Rows +// mirror the readonly* runs in spec/router.qnt. +func TestRouteReadonly(t *testing.T) { + m := newTestManager(t, map[string]string{ + "beacon.yaml": ` +service_name: beacon +allowed_image_prefixes: + - chainsafe/lodestar +`, + }) + r := NewRouter(m).WithReadonly(true) + post := "read-only mode: POST requests are not allowed" + createBody := map[string]interface{}{"Image": "chainsafe/lodestar:latest"} + pullBody := map[string]interface{}{"fromImage": "chainsafe/lodestar", "tag": "latest"} + + tests := []struct { + method string + path string + body map[string]interface{} + want Action + wantMsg string + }{ + // readonlyGetAllowedTest (#75) + {"GET", "/containers/json", nil, ActionAllow, ""}, + // readonlyHeadAllowedTest (#75) + {"HEAD", "/containers/mycontainer", nil, ActionAllow, ""}, + // #75: the read-only endpoints stay allowed. + {"GET", "/_ping", nil, ActionAllow, ""}, + // readonlyLifecycleStopDeniedTest (#75) + {"POST", "/containers/mycontainer/stop", nil, ActionDeny, post}, + // #75: the other lifecycle actions too. + {"POST", "/containers/mycontainer/start", nil, ActionDeny, post}, + // readonlyDeleteDeniedTest (#75) + {"DELETE", "/containers/mycontainer", nil, ActionDeny, "read-only mode: DELETE requests are not allowed"}, + // #75: an image pull of an allowed image. + {"POST", "/images/create", pullBody, ActionDeny, post}, + // readonlyCreateDeniedTest (#75): an allowed image, so only the flag decides. + {"POST", "/containers/create", createBody, ActionDeny, post}, + // #75: methods outside the spec's universe. + {"PUT", "/containers/mycontainer/archive", nil, ActionDeny, "read-only mode: PUT requests are not allowed"}, + // #75: the check runs before the API-version strip. + {"POST", "/v1.45/containers/mycontainer/stop", nil, ActionDeny, post}, + } + for _, tt := range tests { + t.Run(tt.method+" "+tt.path, func(t *testing.T) { + got := r.Route(tt.method, tt.path, tt.body) + if got.Action != tt.want || got.DenyMsg != tt.wantMsg { + t.Fatalf("Route(%s, %s) = %v/%q, want %v/%q", + tt.method, tt.path, got.Action, got.DenyMsg, tt.want, tt.wantMsg) + } + }) + } + + // #75: the flag gates the rule; without it a lifecycle POST is routed as before. + t.Run("readonly off", func(t *testing.T) { + got := NewRouter(m).Route("POST", "/containers/mycontainer/stop", nil) + if got.DenyMsg == post { + t.Fatalf("Route(POST, /containers/mycontainer/stop) without --readonly = %v/%q", got.Action, got.DenyMsg) + } + }) +} diff --git a/go/main.go b/go/main.go index 4ba0f03..a87c701 100644 --- a/go/main.go +++ b/go/main.go @@ -36,7 +36,7 @@ func main() { logFile := flag.String("log-file", "/var/log/docker-socket-policy.log", "Audit log file (JSON)") readonly := flag.Bool("readonly", false, - "Enable read-only mode (deny all POST/PUT/DELETE)") + "Enable read-only mode (deny every request that is not GET or HEAD)") // The value is read with flagValueIfSet, never through the returned // pointer: absent (default group) and "" (own group) must stay distinct. flag.String("listen-socket-group", "", @@ -78,8 +78,8 @@ func main() { } slog.Info("loaded policies", "count", len(policyManager.List()), "config_dir", *configDir) - router := proxy.NewRouter(policyManager) - chain := middleware.NewChain(*readonly) + router := proxy.NewRouter(policyManager).WithReadonly(*readonly) + chain := middleware.NewChain() transport := proxy.NewTransport(*dockerHost) handler := proxy.NewHandler(router, chain, auditLog, transport) diff --git a/rs/src/handler.rs b/rs/src/handler.rs index 74f6111..35722ca 100644 --- a/rs/src/handler.rs +++ b/rs/src/handler.rs @@ -210,7 +210,7 @@ mod tests { ); let manager = Manager::from_map(policies); let router = Arc::new(Router::new(manager)); - let chain = Chain::new(false); + let chain = Chain::new(); let audit = AuditLogger::new("/dev/null").unwrap(); let (mock_transport, captured, captured_headers) = MockTransport::new(); let handler = Handler::new(router, chain, audit, Box::new(mock_transport)); @@ -304,7 +304,7 @@ mod tests { ); let manager = Manager::from_map(policies); let router = Arc::new(Router::new(manager)); - let chain = Chain::new(false); + let chain = Chain::new(); let audit = AuditLogger::new("/dev/null").unwrap(); let transport = FailingTransport { kind }; Handler::new(router, chain, audit, Box::new(transport)) @@ -360,7 +360,7 @@ mod tests { ); let manager = Manager::from_map(policies); let router = Arc::new(Router::new(manager)); - let chain = Chain::new(false); + let chain = Chain::new(); let audit = AuditLogger::new("/dev/null").unwrap(); let (mock_transport, captured, _captured_headers) = MockTransport::new(); let handler = Handler::new(router, chain, audit, Box::new(mock_transport)); @@ -400,7 +400,7 @@ mod tests { fn make_uri_recording_handler() -> (Handler, Arc>>) { let manager = Manager::from_map(std::collections::HashMap::new()); let router = Arc::new(Router::new(manager)); - let chain = Chain::new(false); + let chain = Chain::new(); let audit = AuditLogger::new("/dev/null").unwrap(); let captured_uri = Arc::new(std::sync::Mutex::new(None)); let transport = UriRecordingTransport { @@ -409,6 +409,32 @@ mod tests { (Handler::new(router, chain, audit, Box::new(transport)), captured_uri) } + /// #75: --readonly is enforced in the router, so a delete never reaches + /// the daemon. Before #75 the flag only reached the chain, which runs on + /// create. + #[tokio::test] + async fn test_handler_readonly_denies_delete() { + let manager = Manager::from_map(std::collections::HashMap::new()); + let router = Arc::new(Router::new(manager).with_readonly(true)); + let audit = AuditLogger::new("/dev/null").unwrap(); + let captured_uri = Arc::new(std::sync::Mutex::new(None)); + let transport = UriRecordingTransport { + captured_uri: captured_uri.clone(), + }; + let handler = Handler::new(router, Chain::new(), audit, Box::new(transport)); + let req = Request::delete("http://localhost/containers/mycontainer") + .body(Full::new(Bytes::new())) + .unwrap(); + let resp = handler.handle(req).await; + assert_eq!(resp.status(), StatusCode::FORBIDDEN); + let body = resp.into_body().collect().await.unwrap().to_bytes(); + assert_eq!( + String::from_utf8_lossy(&body), + "read-only mode: DELETE requests are not allowed" + ); + assert!(captured_uri.lock().unwrap().is_none(), "expected no forward"); + } + /// #53: Rust routed on the raw name and forwarded it; the daemon decodes /// it to "foo bar" and acts on that container. #[tokio::test] @@ -474,7 +500,7 @@ mod tests { let audit = AuditLogger::new(log_path.to_str().unwrap()).unwrap(); let captured_uri = Arc::new(std::sync::Mutex::new(None)); let transport = UriRecordingTransport { captured_uri }; - let handler = Handler::new(router, Chain::new(false), audit, Box::new(transport)); + let handler = Handler::new(router, Chain::new(), audit, Box::new(transport)); let targets = [ ("GET", "/containers/json?all=1", "ALLOW"), diff --git a/rs/src/main.rs b/rs/src/main.rs index bca469c..69c3429 100644 --- a/rs/src/main.rs +++ b/rs/src/main.rs @@ -102,8 +102,8 @@ async fn main() -> Result<(), Box> { let policy_manager = policy::Manager::new(&cli.config_dir)?; tracing::info!("loaded {} policies", policy_manager.list().len()); - let router = Arc::new(proxy::Router::new(policy_manager)); - let chain = middleware::Chain::new(cli.readonly); + let router = Arc::new(proxy::Router::new(policy_manager).with_readonly(cli.readonly)); + let chain = middleware::Chain::new(); let audit = match audit::AuditLogger::new(&cli.log_file) { Ok(logger) => logger, Err(e) => { @@ -1050,7 +1050,7 @@ mod tests { fn test_handler(delay: std::time::Duration) -> Arc { let manager = policy::Manager::from_map(std::collections::HashMap::new()); let router = Arc::new(proxy::Router::new(manager)); - let chain = middleware::Chain::new(false); + let chain = middleware::Chain::new(); let audit = audit::AuditLogger::new("/dev/null").unwrap(); Arc::new(handler::Handler::new( router, diff --git a/rs/src/middleware.rs b/rs/src/middleware.rs index 097633c..d0095dc 100644 --- a/rs/src/middleware.rs +++ b/rs/src/middleware.rs @@ -32,11 +32,8 @@ pub struct GateResult { } impl Chain { - pub fn new(readonly: bool) -> Self { + pub fn new() -> Self { let mut gates: Vec> = Vec::new(); - if readonly { - gates.push(Box::new(ReadonlyGate)); - } gates.push(Box::new(RegistryGate)); gates.push(Box::new(MountSourceGate)); gates.push(Box::new(EnvFileGate)); @@ -85,22 +82,6 @@ fn charset_check(s: &str) -> bool { .all(|c| matches!(c, 'A'..='Z' | 'a'..='z' | '0'..='9' | '.' | '_' | '=' | ':' | '/' | ',' | '-')) } -pub struct ReadonlyGate; -impl Gate for ReadonlyGate { - fn check( - &self, - method: &str, - _path: &str, - _policy: &Policy, - _body: &HashMap, - ) -> std::result::Result<(), String> { - match method { - "POST" | "PUT" | "DELETE" | "PATCH" => Err(format!("read-only mode: {} requests are not allowed", method)), - _ => Ok(()), - } - } -} - pub struct RegistryGate; impl Gate for RegistryGate { fn check( @@ -372,40 +353,6 @@ fn body_from_json(json: serde_json::Value) -> HashMap mod tests { use super::*; - #[test] - fn test_readonly_gate_allows_get() { - let gate = ReadonlyGate; - let policy = make_policy("test", vec!["alpine"]); - let body = HashMap::new(); - assert!(gate.check("GET", "/containers/json", &policy, &body).is_ok()); - } - - #[test] - fn test_readonly_gate_denies_post() { - let gate = ReadonlyGate; - let policy = make_policy("test", vec!["alpine"]); - let body = HashMap::new(); - let result = gate.check("POST", "/containers/create", &policy, &body); - assert!(result.is_err()); - assert!(result.unwrap_err().contains("read-only")); - } - - #[test] - fn test_readonly_gate_denies_put() { - let gate = ReadonlyGate; - let policy = make_policy("test", vec!["alpine"]); - let body = HashMap::new(); - assert!(gate.check("PUT", "/some/path", &policy, &body).is_err()); - } - - #[test] - fn test_readonly_gate_denies_delete() { - let gate = ReadonlyGate; - let policy = make_policy("test", vec!["alpine"]); - let body = HashMap::new(); - assert!(gate.check("DELETE", "/containers/x", &policy, &body).is_err()); - } - #[test] fn test_registry_gate_allows_allowed_image() { let gate = RegistryGate; @@ -660,7 +607,7 @@ mod tests { #[test] fn test_chain_execute_allows() { - let chain = Chain::new(false); + let chain = Chain::new(); let policy = make_policy("test", vec!["alpine"]); let body = body_from_json(serde_json::json!({"Image": "alpine"})); let result = chain.execute("POST", "/containers/create", &policy, &body); @@ -670,7 +617,7 @@ mod tests { #[test] fn test_chain_execute_denies_unknown_image_via_registry_gate() { - let chain = Chain::new(false); + let chain = Chain::new(); let policy = make_policy("test", vec!["alpine"]); let body = body_from_json(serde_json::json!({"Image": "ubuntu"})); let result = chain.execute("POST", "/containers/create", &policy, &body); @@ -678,19 +625,9 @@ mod tests { assert!(result.reason.contains("not in allowed prefixes")); } - #[test] - fn test_chain_execute_readonly_denies_post() { - let chain = Chain::new(true); - let policy = make_policy("test", vec!["alpine"]); - let body = body_from_json(serde_json::json!({"Image": "alpine"})); - let result = chain.execute("POST", "/containers/create", &policy, &body); - assert!(!result.allowed); - assert!(result.reason.contains("read-only")); - } - #[test] fn test_chain_execute_with_mutator_sets_hostconfig() { - let chain = Chain::new(false); + let chain = Chain::new(); let mut policy = make_policy("test", vec!["alpine"]); policy.container_config = Some(ContainerConfig { network_mode: Some("bridge".into()), diff --git a/rs/src/proxy.rs b/rs/src/proxy.rs index 701e307..4f1404e 100644 --- a/rs/src/proxy.rs +++ b/rs/src/proxy.rs @@ -21,14 +21,29 @@ pub struct RouteResult { pub struct Router { manager: Manager, + readonly: bool, } impl Router { pub fn new(manager: Manager) -> Self { - Router { manager } + Router { + manager, + readonly: false, + } + } + + /// Sets read-only mode (--readonly) and returns the router. + pub fn with_readonly(mut self, readonly: bool) -> Self { + self.readonly = readonly; + self } pub fn route(&self, method: &str, path: &str, body: Option<&HashMap>) -> RouteResult { + // Read-only mode denies every write, for every path (#75). + if self.readonly && method != "GET" && method != "HEAD" { + return deny(&format!("read-only mode: {} requests are not allowed", method)); + } + // The daemon decodes the path before routing, so deny any escape (#53). if path.contains('%') { return deny("percent-encoded path not allowed"); @@ -809,6 +824,78 @@ mod tests { assert!(mismatches.is_empty(), "mismatches:\n{}", mismatches.join("\n")); } + /// Cross-language parity guard for #75. With --readonly, every request + /// whose method is not GET or HEAD is denied first in the router, for + /// every path. Before #75 the flag only reached the middleware chain, + /// which runs on the create route alone, so lifecycle POSTs, DELETE + /// /containers/{name} and image pulls were forwarded. Rows mirror the + /// readonly* runs in spec/router.qnt. + #[test] + fn test_route_readonly() { + let router = Router::new(make_manager(vec!["chainsafe/lodestar"])).with_readonly(true); + let post = Some("read-only mode: POST requests are not allowed"); + let create_body: HashMap = + serde_json::from_value(serde_json::json!({"Image": "chainsafe/lodestar:latest"})).unwrap(); + let pull_body: HashMap = + serde_json::from_value(serde_json::json!({"fromImage": "chainsafe/lodestar", "tag": "latest"})).unwrap(); + let cases = [ + // readonlyGetAllowedTest (#75) + ("GET", "/containers/json", None, Action::Allow, None), + // readonlyHeadAllowedTest (#75) + ("HEAD", "/containers/mycontainer", None, Action::Allow, None), + // #75: the read-only endpoints stay allowed. + ("GET", "/_ping", None, Action::Allow, None), + // readonlyLifecycleStopDeniedTest (#75) + ("POST", "/containers/mycontainer/stop", None, Action::Deny, post), + // #75: the other lifecycle actions too. + ("POST", "/containers/mycontainer/start", None, Action::Deny, post), + // readonlyDeleteDeniedTest (#75) + ( + "DELETE", + "/containers/mycontainer", + None, + Action::Deny, + Some("read-only mode: DELETE requests are not allowed"), + ), + // #75: an image pull of an allowed image. + ("POST", "/images/create", Some(&pull_body), Action::Deny, post), + // readonlyCreateDeniedTest (#75): an allowed image, so only the flag decides. + ("POST", "/containers/create", Some(&create_body), Action::Deny, post), + // #75: methods outside the spec's universe. + ( + "PUT", + "/containers/mycontainer/archive", + None, + Action::Deny, + Some("read-only mode: PUT requests are not allowed"), + ), + // #75: the check runs before the API-version strip. + ("POST", "/v1.45/containers/mycontainer/stop", None, Action::Deny, post), + ]; + let mut mismatches = Vec::new(); + for (method, path, body, want, want_msg) in cases { + let got = router.route(method, path, body); + if got.action != want || got.deny_msg.as_deref() != want_msg { + mismatches.push(format!( + "{} {}: got {:?}/{}, want {:?}/{}", + method, + path, + got.action, + got.deny_msg.as_deref().unwrap_or("-"), + want, + want_msg.unwrap_or("-") + )); + } + } + // #75: the flag gates the rule; without it a lifecycle POST is routed as before. + let off = + Router::new(make_manager(vec!["chainsafe/lodestar"])).route("POST", "/containers/mycontainer/stop", None); + if off.deny_msg.as_deref() == post { + mismatches.push("POST /containers/mycontainer/stop without --readonly: got the read-only deny".to_string()); + } + assert!(mismatches.is_empty(), "mismatches:\n{}", mismatches.join("\n")); + } + #[test] fn test_extract_container_name_skips_empty_segment() { for path in ["/containers/", "/containers//start"] { diff --git a/spec/README.md b/spec/README.md index ceb617f..d9ee517 100644 --- a/spec/README.md +++ b/spec/README.md @@ -9,7 +9,7 @@ This directory contains a [Quint](https://quint-lang.org/) formal specification | `docker_socket_policy.qnt` | Request-handling spec: policy types, state machine, endpoint routing table, 9 invariants (6 P0 / 3 P1), 6 attack scenario simulations | | `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 segment-exact exec deny and `POST /containers/create` route, 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 and #55), `router_pre48` (Rust's extraction rule before #48), `router_pre53` (Rust and TypeScript before #53, routing on the raw path) and `router_pre55` (all three before #55, letting empty interior 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 | +| `router.qnt` | The percent-encoded path deny, the empty interior segment deny, the segment-exact exec deny and `POST /containers/create` route, 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 and #55), `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) and `router_readonly` (as `router`, with `--readonly`, #75). Every instance includes the #49 exec and create rule, so the pre instances isolate one earlier rule each rather than being full snapshots | ## How to Run @@ -34,12 +34,13 @@ 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 four instances +# Router model: typecheck, run the table tests on all five instances quint typecheck spec/router.qnt quint test spec/router.qnt --main=router quint test spec/router.qnt --main=router_pre48 quint test spec/router.qnt --main=router_pre53 quint test spec/router.qnt --main=router_pre55 +quint test spec/router.qnt --main=router_readonly --match='^readonly.*Test$' # Formal model-checking via Apalache (exhaustive, requires Java) quint verify --max-steps=10 --invariants allInvariants spec/docker_socket_policy.qnt @@ -85,7 +86,7 @@ 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"]`. The first check in `route` denies any path with an encoded segment, for every method (#53). The second 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. The third 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 fourth routes `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 first check in `route` after it denies any path with an encoded segment, for every method (#53). The second 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. The third 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 fourth routes `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: - `lifecycleOnlyTargetsRealNames` holds when each lifecycle allow (`allowKnown` or `allowUnknown`) targets a non-empty, non-reserved name. - `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"]`. @@ -94,6 +95,7 @@ A path is a list of raw, still percent-encoded segments with the query string re - `createOnlyExact` holds when `routeCreate` is the outcome for exactly `POST /containers/create`. - `emptyInteriorDenied` holds when every path with an empty segment before the last one is denied. - `emptySegmentsHideNoExec` holds when every path that is an exec path once its empty segments are dropped is denied. +- `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` and `emptySegmentsHideNoExecTest` asserts `emptySegmentsHideNoExec`. @@ -140,6 +142,18 @@ Rows for empty interior segments and HEAD ([#55](https://github.com/ChainSafe/do | `emptyInteriorGetDenied` | `GET /containers//json` | deny | `router` | | `headNamedContainerAllowed` | `HEAD /containers/mycontainer` | allow (read) | all | +Rows for `--readonly` ([#75](https://github.com/ChainSafe/docker-socket-policy/issues/75)), all on `router_readonly`, which also runs `readonlySoundTest` (asserts `readonlyAllowsOnlyReads`). The implementations deny these with `read-only mode: requests are not allowed`: + +| Row (`run Test`) | Request | Outcome | +|-----|---------|---------| +| `readonlyLifecycleStopDenied` | `POST /containers/mycontainer/stop` | deny | +| `readonlyDeleteDenied` | `DELETE /containers/mycontainer` | deny | +| `readonlyCreateDenied` | `POST /containers/create` | deny | +| `readonlyGetAllowed` | `GET /containers/json` | allow (read) | +| `readonlyHeadAllowed` | `HEAD /containers/mycontainer` | allow (read) | + +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 and #55 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. ### Modeling Notes @@ -157,11 +171,12 @@ Two invariants are structurally tautological within the Quint model — they can - **The query string is outside the model.** Paths are segment lists with the query string already removed, so the model says nothing about `%` in the query. The implementations never inspect the query string for this rule; encoded queries such as `?filters=%7B…%7D` are routine and must still be forwarded. That is pinned by the language handler tests and the integration tests, not by the model. - **GET is covered.** The percent check runs before the GET passthrough, and `routerSeesWhatDaemonSees` ranges over `GET` as well as `POST` and `DELETE`. On `router_pre53` the passthrough lets `GET /containers/%2F` through, and the property fails for it too. - **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 `routerSeesWhatDaemonSees` cannot express Go's risk: decoding that differs from the daemon's, such as encoded version segments ([#57](https://github.com/ChainSafe/docker-socket-policy/issues/57)). Go's change is covered by the language handler tests. -- **`router.qnt` is not the full router.** `route` models the percent check, which runs first in all three routers, the exec check, the exact `POST /containers/create` route and the container-lifecycle branch, in that order. It leaves out the API-version strip and the other checks that the routers run before the lifecycle branch, such as the read-only endpoints, auth, and the build and commit denials, as well as `POST /images/create` and the endpoints after the lifecycle branch. For paths that those checks catch, the model's outcome can differ from what an implementation does: `GET /build` is `allowRead` in the model, for example, while the routers deny it as `build is not allowed`. The `images` rows of #49 (`GET /images/exec` allowed, `POST /images/create/extra` denied) are therefore language-only rows, as are `GET /containers/myexec/json` and `DELETE /containers/executor`: the model's words are atoms, so it cannot express a name that merely contains `exec`. Only the properties and the table rows are claims about the code. +- **`router.qnt` is not the full router.** `route` models the read-only check, which runs first in all three routers, the percent check, the exec check, the exact `POST /containers/create` route and the container-lifecycle branch, in that order. It leaves out the API-version strip and the other checks that the routers run before the lifecycle branch, such as the read-only endpoints, auth, and the build and commit denials, as well as `POST /images/create` and the endpoints after the lifecycle branch. For paths that those checks catch, the model's outcome can differ from what an implementation does: `GET /build` is `allowRead` in the model, for example, while the routers deny it as `build is not allowed`. The `images` rows of #49 (`GET /images/exec` allowed, `POST /images/create/extra` denied) are therefore language-only rows, as are `GET /containers/myexec/json` and `DELETE /containers/executor`: the model's words are atoms, so it cannot express a name that merely contains `exec`. Only the properties and the table rows are claims about the code. - **Exec and create match whole segments (#49).** Before #49 the routers disagreed: Go matched `exec` only in the name position (`/containers/exec`), so `DELETE /containers/mycontainer/exec` went down the lifecycle branch and was allowed. Rust matched the substring `/exec` anywhere after `/containers/`, and TypeScript anywhere in the path, so Rust and TypeScript denied containers named `exec-runner` or `executor`. Neither Go nor Rust checked the `/exec/…` namespace, so both let `GET /exec//json` through the GET passthrough. Go also accepted `POST /containers/create/extra` as a container create. In the model the exec check runs before the create route, the lifecycle branch and the GET passthrough, so it denies every method, GET included: exec inspect shows the full command line. `reservedExecDeleteDenied` (`DELETE /containers/exec`) is decided by the exec check, as in all three implementations, and now asserts `denyExec`. `exec` stays in the reserved set, but no row, in the model or in the language tests, would catch it being dropped from there. - **The #49 properties are not vacuous.** None of them is shown on a separate instance; replacing the rule in `route` makes them fail. With a substring-style rule (a segment that starts with `exec`, modeled as the set `exec`, `exec-runner`), `execPrefixSoundTest` and the three `execPrefixName*Allowed` rows fail. With Go's rule before #49 (`exec` only as the second segment under `containers`), `execSoundTest` and the four `exec*Denied` rows fail. `execSubpathPostDenied` and `execNamespacePostDenied` fail there only because the outcome is the default `deny`, not `denyExec`: the request is still refused, as before #49, but not by the exec check. With Go's prefix match for create, `createExactSoundTest` and `createSubpathDenied` fail. `execNeverAllowed` writes out the exec positions itself instead of calling `isExecPath`, so that a narrower `isExecPath` cannot narrow the property with it. - **HEAD is routed like GET (#55).** `METHODS` is `GET`, `HEAD`, `POST`, `DELETE`. Before #55 the lifecycle branches disagreed on `HEAD /containers/x`: Go and TypeScript fell through to the GET/HEAD passthrough and allowed it, while Rust denied it. All three now allow it like `GET`, and the model's lifecycle branch allows `GET` and `HEAD` alike. - **Empty interior segments are denied (#55).** Before #55, `GET //exec//json` (and `/v1.45//exec/…`) had an empty first segment, so the exec check did not match and the GET passthrough forwarded it. A live daemon answers such paths with a `301` to the clean path, so nothing leaked, but the router had allowed an exec path. `emptyInteriorDenied` states the rule, and on `router` it is close to a tautology, because `route` tests `hasEmptyInterior` directly. `emptySegmentsHideNoExec` states the security claim: a path that is an exec path once its empty segments are dropped, as the daemon's redirect does, is denied for every method. Both fail on `router_pre55`, and setting `router`'s `ALLOW_EMPTY_INTERIOR` to `true` makes `emptyInteriorSoundTest`, `emptySegmentsHideNoExecTest`, `emptyLeadingExecGetDenied` and `emptyInteriorGetDenied` fail. `router_pre48` keeps the rule off: with it on, `POST /containers//start` is denied before it reaches the name branch, and the instance could no longer show the #48 extraction bug. For the same reason, on `router` the empty-segment check now decides `emptyNameStartDenied`; the #48 extraction stays covered there by `emptyNameDeleteDenied` (a trailing slash) and on `router_pre48`. Dot segments (`/./exec/…`, `/x/../exec/…`) are not modelled and not denied yet (#70); the daemon redirects them to the clean path as it does for `//`. The implementations check for `//` in the path without its query, so `GET /containers/json?filters=a//b` is still forwarded; that is pinned by the TypeScript router test, since Go and Rust route on the path alone. +- **`--readonly` is a router rule (#75).** Before #75 the flag reached only the middleware chain, and the handler runs the chain only on the create route, so lifecycle `POST`s, `DELETE /containers/{name}` and `POST /images/create` were forwarded in read-only mode. That was handler wiring, not routing, so there is no `pre75` instance: the `READONLY = false` instances are the behaviour without the flag, which before the fix was also the behaviour with it for every route but create. `readonlyAllowsOnlyReads` does not mention `READONLY`, so it is not vacuous: setting `router_readonly`'s `READONLY` to `false` makes `readonlySoundTest` and the three `readonly*Denied` rows fail. `METHODS` has no `PUT`; the implementations' tests cover it. - **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 diff --git a/spec/router.qnt b/spec/router.qnt index 9bd6f65..a524cec 100644 --- a/spec/router.qnt +++ b/spec/router.qnt @@ -36,6 +36,12 @@ // the fix: a path with `//` in it is denied, for every method. A single // trailing slash, as in `/containers/`, is not interior. // +// READONLY = true models `--readonly` (#75): every request whose method is +// not GET or HEAD is denied, for every path, before any other check. +// false models the proxy without the flag. Before #75 the flag only reached +// the middleware chain, which runs on the create route alone; that was +// handler wiring, not routing, so no instance models it. +// // 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 @@ -46,6 +52,7 @@ module router_model { const ALLOW_EMPTY_NAME: bool const ALLOW_PERCENT: bool const ALLOW_EMPTY_INTERIOR: bool + const READONLY: bool // ─── Daemon decoding ──────────────────────────────────────────────── @@ -116,13 +123,15 @@ module router_model { pure def isDenied(outcome: str): bool = outcome == "deny" or outcome == "denyExec" - // The percent check (#53) comes first, then the empty interior segment - // check (#55). The exec check (#49) follows, before + // The read-only check (#75) comes first, then the percent check (#53), + // then the empty interior segment check (#55). The exec check (#49) + // follows, before // the create route, the lifecycle branch and the GET passthrough, so it // denies every method. POST /containers/create matches only the exact // segments; a longer path falls through. pure def route(method: str, segs: List[str]): str = - if (not(ALLOW_PERCENT) and hasEncodedSeg(segs)) "deny" + if (READONLY and not(method == "GET" or method == "HEAD")) "deny" + else if (not(ALLOW_PERCENT) and hasEncodedSeg(segs)) "deny" else if (not(ALLOW_EMPTY_INTERIOR) and hasEmptyInterior(segs)) "deny" else if (isExecPath(segs)) "denyExec" else if (method == "POST" and segs == ["containers", "create"]) "routeCreate" @@ -140,7 +149,7 @@ module router_model { // ─── Path universe ────────────────────────────────────────────────── pure val WORDS = Set( - "containers", "", "json", "create", "exec", "mycontainer", "beacon", "start", + "containers", "", "json", "create", "exec", "mycontainer", "beacon", "start", "stop", "%2F", "%2f", "%6A%73%6F%6E", "beacon%2Fstart", "exec-runner", "abc", "extra", ) @@ -212,13 +221,24 @@ module router_model { isExecPath(segs.select(s => s != "")) implies isDenied(route(m, segs)) )) + // Every request that is not GET or HEAD is denied, for every path (#75). + // The read-only claim: it holds only on `router_readonly`. It does not + // mention READONLY, so that the instance with the flag off fails it + // rather than passing it vacuously. + pure val readonlyAllowsOnlyReads: bool = + METHODS.forall(m => PATHS.forall(segs => + not(m == "GET" or m == "HEAD") implies isDenied(route(m, segs)) + )) + // ─── Tests: one run per table row ─────────────────────────────────── // // Rows whose outcome depends on none of ALLOW_EMPTY_NAME, ALLOW_PERCENT - // and ALLOW_EMPTY_INTERIOR run on every instance. The rows that do and the + // and ALLOW_EMPTY_INTERIOR run on every READONLY = false instance. The rows + // that do and the // property tests (soundTest, percentSoundTest, execSoundTest, // execPrefixSoundTest, createExactSoundTest, emptyInteriorSoundTest) live - // in `router`. + // in `router`. `router_readonly` runs only its own `readonly*` tests + // (`--match`): with READONLY = true the write rows here are denied. run emptyNameGetAllowedTest = assert(route("GET", ["containers", ""]) == "allowRead") @@ -277,7 +297,7 @@ module router_model { // not a name, and a percent-encoded path or one with an empty interior // segment is denied. module router { - import router_model(ALLOW_EMPTY_NAME = false, ALLOW_PERCENT = false, ALLOW_EMPTY_INTERIOR = false).* + import router_model(ALLOW_EMPTY_NAME = false, ALLOW_PERCENT = false, ALLOW_EMPTY_INTERIOR = false, READONLY = false).* run emptyNameDeleteDeniedTest = assert(route("DELETE", ["containers", ""]) == "deny") @@ -344,7 +364,7 @@ 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).* + import router_model(ALLOW_EMPTY_NAME = true, ALLOW_PERCENT = false, ALLOW_EMPTY_INTERIOR = true, READONLY = false).* run pre48UnsoundTest = assert(and { @@ -358,7 +378,7 @@ 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).* + import router_model(ALLOW_EMPTY_NAME = false, ALLOW_PERCENT = true, ALLOW_EMPTY_INTERIOR = false, READONLY = false).* run pre53UnsoundTest = assert(and { @@ -374,7 +394,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).* + import router_model(ALLOW_EMPTY_NAME = false, ALLOW_PERCENT = false, ALLOW_EMPTY_INTERIOR = true, READONLY = false).* run pre55UnsoundTest = assert(and { @@ -383,3 +403,28 @@ module router_pre55 { route("GET", ["", "exec", "abc"]) == "allowRead", }) } + +// `--readonly` (#75): as `router`, plus the read-only check. Run with +// `--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).* + + run readonlySoundTest = + assert(readonlyAllowsOnlyReads) + + run readonlyLifecycleStopDeniedTest = + assert(route("POST", ["containers", "mycontainer", "stop"]) == "deny") + + run readonlyDeleteDeniedTest = + assert(route("DELETE", ["containers", "mycontainer"]) == "deny") + + run readonlyCreateDeniedTest = + assert(route("POST", ["containers", "create"]) == "deny") + + run readonlyGetAllowedTest = + assert(route("GET", ["containers", "json"]) == "allowRead") + + run readonlyHeadAllowedTest = + assert(route("HEAD", ["containers", "mycontainer"]) == "allowRead") +} diff --git a/ts/src/handler.test.ts b/ts/src/handler.test.ts index 7621f6e..37a6ced 100644 --- a/ts/src/handler.test.ts +++ b/ts/src/handler.test.ts @@ -60,7 +60,7 @@ function makeEnv(configFiles: Record) { function newHandler(dir: string) { const manager = new Manager(dir); const router = new Router(manager); - const chain = new Chain(false); + const chain = new Chain(); const audit = new AuditLogger(join(dir, "audit.log")); const transport = new FakeTransport(); return { handler: new Handler(router, chain, audit, transport), transport }; @@ -95,6 +95,20 @@ describe("Handler", () => { assert.ok(recorder.body.includes("build")); }); + // #75: --readonly is enforced in the router, so a delete never reaches the + // daemon. Before #75 the flag only reached the chain, which runs on create. + it("denies a delete in read-only mode without forwarding it", async () => { + const dir = makeEnv(defaultConfig); + const router = new Router(new Manager(dir)).withReadonly(true); + const transport = new FakeTransport(); + const handler = new Handler(router, new Chain(), new AuditLogger(join(dir, "audit.log")), transport); + const { res, recorder } = makeResponse(); + await handler.handle(makeRequest("DELETE", "/containers/mycontainer"), res); + assert.equal(recorder.statusCode, 403); + assert.equal(recorder.body, "read-only mode: DELETE requests are not allowed"); + assert.equal(transport.lastRequest, undefined); + }); + it("forwards a valid container create with the modified body", async () => { const dir = makeEnv(beaconConfig); const { handler, transport } = newHandler(dir); diff --git a/ts/src/index.ts b/ts/src/index.ts index 745af6e..88fd153 100644 --- a/ts/src/index.ts +++ b/ts/src/index.ts @@ -63,8 +63,8 @@ console.log(`loading policies from ${configDir}...`); const policyManager = new Manager(configDir); console.log(`loaded ${policyManager.list().length} policies`); -const router = new Router(policyManager); -const chain = new Chain(readonly); +const router = new Router(policyManager).withReadonly(readonly); +const chain = new Chain(); const audit = new AuditLogger(logFile); const transport = new Transport(dockerHost); const handler = new Handler(router, chain, audit, transport); diff --git a/ts/src/middleware.test.ts b/ts/src/middleware.test.ts index 59d18d6..841b31f 100644 --- a/ts/src/middleware.test.ts +++ b/ts/src/middleware.test.ts @@ -1,14 +1,6 @@ import { describe, it } from "node:test"; import assert from "node:assert/strict"; -import { - Chain, - ReadonlyGate, - RegistryGate, - MountSourceGate, - EnvFileGate, - CmdGate, - ContainerConfigMutator, -} from "./middleware.js"; +import { Chain, RegistryGate, MountSourceGate, EnvFileGate, CmdGate, ContainerConfigMutator } from "./middleware.js"; import type { Policy } from "./policy.js"; function makePolicy(overrides: Partial = {}): Policy { @@ -19,22 +11,6 @@ function makePolicy(overrides: Partial = {}): Policy { }; } -describe("ReadonlyGate", () => { - const gate = new ReadonlyGate(); - - for (const method of ["POST", "PUT", "DELETE", "PATCH"]) { - it(`denies ${method} requests`, () => { - const result = gate.check(method, "/containers/create", makePolicy(), {}); - assert.notEqual(result, null); - assert.ok(result?.includes("read-only")); - }); - } - - it("allows GET requests", () => { - assert.equal(gate.check("GET", "/containers/json", makePolicy(), {}), null); - }); -}); - describe("RegistryGate", () => { const gate = new RegistryGate(); @@ -280,18 +256,11 @@ describe("ContainerConfigMutator", () => { describe("Chain", () => { it("allows valid create container request", () => { - const chain = new Chain(false); + const chain = new Chain(); const policy = makePolicy({ allowed_cli_flags: ["--port"] }); const body: Record = { Image: "nginx:latest" }; const result = chain.execute("POST", "/containers/create", policy, body); assert.ok(result.allowed); assert.ok(result.modifiedBody); }); - - it("denies readonly mode write request", () => { - const chain = new Chain(true); - const result = chain.execute("POST", "/containers/create", makePolicy(), { Image: "nginx:latest" }); - assert.equal(result.allowed, false); - assert.ok(result.reason.includes("read-only")); - }); }); diff --git a/ts/src/middleware.ts b/ts/src/middleware.ts index dde83b8..836ea07 100644 --- a/ts/src/middleware.ts +++ b/ts/src/middleware.ts @@ -12,13 +12,10 @@ export class Chain { private gates: Gate[]; private mutators: Mutator[]; - constructor(readonly: boolean) { + constructor() { this.gates = []; this.mutators = [new ContainerConfigMutator()]; - if (readonly) { - this.gates.push(new ReadonlyGate()); - } this.gates.push(new RegistryGate()); this.gates.push(new MountSourceGate()); this.gates.push(new EnvFileGate()); @@ -48,14 +45,6 @@ export class Chain { const SAFE_CHARSET = /^[A-Za-z0-9._=:/,\-]{1,256}$/; -export class ReadonlyGate implements Gate { - check(method: string, _path: string, _policy: Policy, _body: Record): string | null { - return ["POST", "PUT", "DELETE", "PATCH"].includes(method) - ? `read-only mode: ${method} requests are not allowed` - : null; - } -} - export class RegistryGate implements Gate { check(_method: string, _path: string, policy: Policy, body: Record): string | null { const image = body["Image"] as string | undefined; diff --git a/ts/src/proxy.test.ts b/ts/src/proxy.test.ts index d4f2ceb..19f69e6 100644 --- a/ts/src/proxy.test.ts +++ b/ts/src/proxy.test.ts @@ -441,4 +441,65 @@ describe("Router", () => { } assert.deepEqual(mismatches, []); }); + + // Cross-language parity guard for #75. With --readonly, every request whose + // method is not GET or HEAD is denied first in the router, for every path. + // Before #75 the flag only reached the middleware chain, which runs on the + // create route alone, so lifecycle POSTs, DELETE /containers/{name} and + // image pulls were forwarded. Rows mirror the readonly* runs in + // spec/router.qnt. + it("denies every write in read-only mode", () => { + writeFileSync( + join(tmpDir, "beacon.yaml"), + "service_name: beacon\nallowed_image_prefixes:\n - chainsafe/lodestar\n", + ); + const m = new Manager(tmpDir); + const ro = new Router(m).withReadonly(true); + const post = "read-only mode: POST requests are not allowed"; + const createBody = { Image: "chainsafe/lodestar:latest" }; + const pullBody = { fromImage: "chainsafe/lodestar", tag: "latest" }; + const cases: [string, string, Record | undefined, Action, string | undefined][] = [ + // readonlyGetAllowedTest (#75) + ["GET", "/containers/json", undefined, Action.Allow, undefined], + // readonlyHeadAllowedTest (#75) + ["HEAD", "/containers/mycontainer", undefined, Action.Allow, undefined], + // #75: the read-only endpoints stay allowed. + ["GET", "/_ping", undefined, Action.Allow, undefined], + // readonlyLifecycleStopDeniedTest (#75) + ["POST", "/containers/mycontainer/stop", undefined, Action.Deny, post], + // #75: the other lifecycle actions too. + ["POST", "/containers/mycontainer/start", undefined, Action.Deny, post], + // readonlyDeleteDeniedTest (#75) + ["DELETE", "/containers/mycontainer", undefined, Action.Deny, "read-only mode: DELETE requests are not allowed"], + // #75: an image pull of an allowed image. + ["POST", "/images/create?fromImage=chainsafe/lodestar&tag=latest", pullBody, Action.Deny, post], + // readonlyCreateDeniedTest (#75): an allowed image, so only the flag decides. + ["POST", "/containers/create", createBody, Action.Deny, post], + // #75: methods outside the spec's universe. + [ + "PUT", + "/containers/mycontainer/archive", + undefined, + Action.Deny, + "read-only mode: PUT requests are not allowed", + ], + // #75: the check runs before the API-version strip. + ["POST", "/v1.45/containers/mycontainer/stop", undefined, Action.Deny, post], + ]; + const mismatches: string[] = []; + for (const [method, path, body, want, wantMsg] of cases) { + const r = ro.route(method, path, body); + if (r.action !== want || r.denyMsg !== wantMsg) { + mismatches.push( + `${method} ${path}: got ${Action[r.action]}/${r.denyMsg ?? "-"}, want ${Action[want]}/${wantMsg ?? "-"}`, + ); + } + } + // #75: the flag gates the rule; without it a lifecycle POST is routed as before. + const off = new Router(m).route("POST", "/containers/mycontainer/stop"); + if (off.denyMsg === post) { + mismatches.push("POST /containers/mycontainer/stop without --readonly: got the read-only deny"); + } + assert.deepEqual(mismatches, []); + }); }); diff --git a/ts/src/proxy.ts b/ts/src/proxy.ts index 95f48a4..33bbd04 100644 --- a/ts/src/proxy.ts +++ b/ts/src/proxy.ts @@ -18,9 +18,21 @@ export interface RouteResult { } export class Router { + private readonlyMode = false; + constructor(private manager: Manager) {} + // Sets read-only mode (--readonly) and returns the router. + withReadonly(readonly: boolean): this { + this.readonlyMode = readonly; + return this; + } + route(method: string, path: string, body?: Record): RouteResult { + // Read-only mode denies every write, for every path (#75). + if (this.readonlyMode && method !== "GET" && method !== "HEAD") { + return { action: Action.Deny, denyMsg: `read-only mode: ${method} requests are not allowed` }; + } const qmIdx = path.indexOf("?"); const cleanPath = qmIdx !== -1 ? path.slice(0, qmIdx) : path; // The daemon decodes the path before routing, so deny any escape (#53).