From 24e57f611cbb1cecc9d57bfb87d3223365bfaf89 Mon Sep 17 00:00:00 2001 From: Adrian Bienkowski Date: Mon, 12 Oct 2026 12:44:27 -0400 Subject: [PATCH 1/3] spec: deny attach in the router model (#76) --- AGENTS.md | 2 +- Makefile | 1 + spec/README.md | 24 ++++++--- spec/docker_socket_policy.qnt | 6 ++- spec/router.qnt | 95 +++++++++++++++++++++++++++++------ 5 files changed, 104 insertions(+), 24 deletions(-) diff --git a/AGENTS.md b/AGENTS.md index eddb8ef..22734c5 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -43,7 +43,7 @@ deploy/ — Docker Compose + integration tests - 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`) +- 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`, and `router_pre76`) ## Test Conventions - Go: stdlib `testing` package, `go test ./...` diff --git a/Makefile b/Makefile index f00b64f..d7293c3 100644 --- a/Makefile +++ b/Makefile @@ -103,6 +103,7 @@ test-spec: $(QUINT) test $(ROUTER_SPEC) --main=router_pre53 $(QUINT) test $(ROUTER_SPEC) --main=router_pre55 $(QUINT) test $(ROUTER_SPEC) --main=router_readonly --match='^readonly.*Test$$' + $(QUINT) test $(ROUTER_SPEC) --main=router_pre76 verify-ts: $(QUINT) run $(SPEC) --max-steps=50 --invariants allInvariants --backend typescript diff --git a/spec/README.md b/spec/README.md index d9ee517..6361841 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), `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 | +| `router.qnt` | The percent-encoded path deny, the empty interior segment deny, the segment-exact exec deny and `POST /containers/create` route, the segment-exact attach deny, 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 and #76), `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) and `router_pre76` (all three before #76, letting attach through). 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,13 +34,14 @@ 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 five instances +# Router model: typecheck, run the table tests on all six 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$' +quint test spec/router.qnt --main=router_pre76 # Formal model-checking via Apalache (exhaustive, requires Java) quint verify --max-steps=10 --invariants allInvariants spec/docker_socket_policy.qnt @@ -86,7 +87,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"]`. 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: +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 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 fifth 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"]`. @@ -95,9 +96,10 @@ 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. +- `attachNeverAllowed` holds when every path whose first segment is `containers`, with `attach` at some later segment, is denied. Like `execNeverAllowed`, it writes the positions out instead of calling `isAttachPath`. - `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`. +On `router`, `soundTest` asserts `lifecycleOnlyTargetsRealNames`, `percentSoundTest` asserts `routerSeesWhatDaemonSees`, `execSoundTest` asserts `execNeverAllowed`, `execPrefixSoundTest` asserts `execPrefixNamesRoute`, `createExactSoundTest` asserts `createOnlyExact`, `emptyInteriorSoundTest` asserts `emptyInteriorDenied`, `emptySegmentsHideNoExecTest` asserts `emptySegmentsHideNoExec` and `attachSoundTest` asserts `attachNeverAllowed`. | Row (`run Test`) | Request | Outcome | Instances | |-----|---------|---------|-----------| @@ -142,6 +144,15 @@ 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 the attach deny ([#76](https://github.com/ChainSafe/docker-socket-policy/issues/76)), all on `router`. The model has at most three segments, so `/containers/mycontainer/attach` stands for `/containers/mycontainer/attach/ws`. The implementations deny these with `attach is not allowed`: + +| Row (`run Test`) | Request | Outcome | +|-----|---------|---------| +| `attachGetDenied` | `GET /containers/mycontainer/attach` | deny | +| `attachPostDenied` (pin) | `POST /containers/mycontainer/attach` | deny | +| `attachHeadDenied` | `HEAD /containers/mycontainer/attach` | deny | +| `attachNamedContainerDenied` | `GET /containers/attach/ws` | deny (a container literally named `attach`) | + 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 | @@ -154,7 +165,7 @@ Rows for `--readonly` ([#75](https://github.com/ChainSafe/docker-socket-policy/i 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. +Each implementation's router tests use the same row names in comments (added with the #48, #53, #49, #55 and #76 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)). ### Modeling Notes @@ -171,12 +182,13 @@ 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 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. +- **`router.qnt` is not the full router.** `route` models the read-only check, which runs first in all three routers, the percent check, the empty interior segment check, the exec check, the attach 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. +- **Attach is a router rule; the upgrade refusal is not modelled (#76).** Before #76, `GET /containers/{name}/attach/ws` with `Upgrade: websocket` fell through to the GET passthrough, and Go relayed the daemon's `101`, giving the caller the container's stdin. The router now denies `attach` as a whole segment after `containers`, for every method, right after the exec check. As with `/containers/exec/…`, a container literally named `attach` is denied too (`attachNamedContainerDenied`). `attachNeverAllowed` fails on `router_pre76`, and setting `router`'s `ALLOW_ATTACH` to `true` makes `attachSoundTest` and three of the four `attach*` rows fail; `attachPostDenied` is a pin, since the lifecycle branch already denies `POST` when the last segment is not an action. The handler also refuses every request with an `Upgrade` header, or a `Connection` header listing `upgrade`, with `protocol upgrade not allowed`, before routing. That check is on headers, which the model does not have, so like the query string it is pinned by the language handler tests and the integration tests, not by the model. - **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/docker_socket_policy.qnt b/spec/docker_socket_policy.qnt index 88244af..2d59b2c 100644 --- a/spec/docker_socket_policy.qnt +++ b/spec/docker_socket_policy.qnt @@ -182,7 +182,8 @@ module docker_socket_policy { { method: "*", path: "/exec/*", kind: "deny" }, { method: "POST", path: "/containers/:name/rename",kind: "deny" }, { method: "POST", path: "/containers/:name/update",kind: "deny" }, - { method: "POST", path: "/containers/:name/attach",kind: "deny" }, + { method: "*", path: "/containers/:name/attach",kind: "deny" }, + { method: "*", path: "/containers/:name/attach/ws",kind: "deny" }, { method: "POST", path: "/containers/:name/prune", kind: "deny" }, { method: "POST", path: "/networks/create", kind: "deny" }, { method: "POST", path: "/volumes/create", kind: "deny" } @@ -455,7 +456,8 @@ module docker_socket_policy { endpointsTable.contains({ method: "*", path: "/exec/*", kind: "deny" }), endpointsTable.contains({ method: "POST", path: "/containers/:name/rename",kind: "deny" }), endpointsTable.contains({ method: "POST", path: "/containers/:name/update",kind: "deny" }), - endpointsTable.contains({ method: "POST", path: "/containers/:name/attach",kind: "deny" }), + endpointsTable.contains({ method: "*", path: "/containers/:name/attach",kind: "deny" }), + endpointsTable.contains({ method: "*", path: "/containers/:name/attach/ws",kind: "deny" }), endpointsTable.contains({ method: "POST", path: "/containers/:name/prune", kind: "deny" }), endpointsTable.contains({ method: "POST", path: "/networks/create", kind: "deny" }), endpointsTable.contains({ method: "POST", path: "/volumes/create", kind: "deny" }) diff --git a/spec/router.qnt b/spec/router.qnt index a524cec..d950125 100644 --- a/spec/router.qnt +++ b/spec/router.qnt @@ -3,8 +3,9 @@ // How the router extracts a container name from a request path, how the // container-lifecycle branch routes on it (#48), the percent-encoded // path deny that runs first in the router (#53), the empty interior segment -// deny that follows it (#55), and the segment-exact exec deny and -// POST /containers/create route (#49). Mirrors +// deny that follows it (#55), the segment-exact exec deny and +// POST /containers/create route (#49), and the segment-exact attach deny +// (#76). Mirrors // `extract_container_name` and the lifecycle branch of `Router::route` in // rs/src/proxy.rs. This is not the full router: the API-version strip and // the other checks that run between the percent check and the lifecycle @@ -42,6 +43,12 @@ // the middleware chain, which runs on the create route alone; that was // handler wiring, not routing, so no instance models it. // +// ALLOW_ATTACH = true models all three before #76, where attach fell +// through to the GET passthrough (and, for other methods, the lifecycle +// branch's default deny). false models all three after the fix: a path +// whose first segment is `containers` with `attach` as any later segment +// is denied, for every method, right after the exec check. +// // 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 @@ -53,6 +60,7 @@ module router_model { const ALLOW_PERCENT: bool const ALLOW_EMPTY_INTERIOR: bool const READONLY: bool + const ALLOW_ATTACH: bool // ─── Daemon decoding ──────────────────────────────────────────────── @@ -115,6 +123,13 @@ module router_model { segs[0] == "containers" and segs.indices().exists(i => i >= 1 and segs[i] == "exec"), } + // Attach matches whole segments only (#76), as exec does under + // `containers`: `attach` anywhere after `containers`. A segment such as + // `attach-runner` is a name like any other. + pure def isAttachPath(segs: List[str]): bool = + if (segs.length() == 0) false + else segs[0] == "containers" and segs.indices().exists(i => i >= 1 and segs[i] == "attach") + // An empty segment before the last one, i.e. `//` in the path. A trailing // empty segment (`/containers/`) is not interior. pure def hasEmptyInterior(segs: List[str]): bool = @@ -125,7 +140,7 @@ module router_model { // 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 + // and the attach check (#76) follow, 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. @@ -134,6 +149,7 @@ module router_model { 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 (not(ALLOW_ATTACH) and isAttachPath(segs)) "deny" else if (method == "POST" and segs == ["containers", "create"]) "routeCreate" else if (hasContainerName(segs)) { val name = containerNameOf(segs) @@ -151,7 +167,7 @@ module router_model { pure val WORDS = Set( "containers", "", "json", "create", "exec", "mycontainer", "beacon", "start", "stop", "%2F", "%2f", "%6A%73%6F%6E", "beacon%2Fstart", - "exec-runner", "abc", "extra", + "exec-runner", "abc", "extra", "attach", "ws", ) pure val METHODS = Set("GET", "HEAD", "POST", "DELETE") @@ -190,6 +206,16 @@ module router_model { }) implies isDenied(route(m, segs)) )) + // A path whose first segment is `containers` with `attach` at some later + // segment is denied for every method (#76). Ranges over GET too. Written + // out rather than taken from isAttachPath, as execNeverAllowed is. + pure val attachNeverAllowed: bool = + METHODS.forall(m => PATHS.forall(segs => + (segs.length() > 0 and segs[0] == "containers" + and segs.indices().exists(i => i >= 1 and segs[i] == "attach")) + implies isDenied(route(m, segs)) + )) + // `exec-runner` only starts with `exec`. Renaming it to the plain unknown // name `mycontainer`, wherever it appears, never changes the outcome (#49). pure def renameExecLike(segs: List[str]): List[str] = @@ -232,11 +258,13 @@ module router_model { // ─── 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 READONLY = false instance. The rows + // Rows whose outcome depends on none of ALLOW_EMPTY_NAME, ALLOW_PERCENT, + // ALLOW_EMPTY_INTERIOR and ALLOW_ATTACH run on every READONLY = false + // instance. The rows // that do and the // property tests (soundTest, percentSoundTest, execSoundTest, - // execPrefixSoundTest, createExactSoundTest, emptyInteriorSoundTest) live + // execPrefixSoundTest, createExactSoundTest, emptyInteriorSoundTest, + // attachSoundTest) live // in `router`. `router_readonly` runs only its own `readonly*` tests // (`--match`): with READONLY = true the write rows here are denied. @@ -293,11 +321,11 @@ module router_model { // ─── Instances ────────────────────────────────────────────────────────── -// Go, TypeScript, and Rust after #48, #53, #49 and #55: an empty segment is -// not a name, and a percent-encoded path or one with an empty interior -// segment is denied. +// Go, TypeScript, and Rust after #48, #53, #49, #55 and #76: an empty +// segment is not a name, a percent-encoded path or one with an empty +// interior segment is denied, and attach is denied. module router { - import router_model(ALLOW_EMPTY_NAME = false, ALLOW_PERCENT = false, ALLOW_EMPTY_INTERIOR = false, READONLY = false).* + import router_model(ALLOW_EMPTY_NAME = false, ALLOW_PERCENT = false, ALLOW_EMPTY_INTERIOR = false, READONLY = false, ALLOW_ATTACH = false).* run emptyNameDeleteDeniedTest = assert(route("DELETE", ["containers", ""]) == "deny") @@ -356,6 +384,29 @@ module router { run emptySegmentsHideNoExecTest = assert(emptySegmentsHideNoExec) + + // Attach is denied for every method (#76). The model has at most three + // segments, so `/containers/mycontainer/attach/ws` is not expressible; + // `/containers/mycontainer/attach` stands for it. + + run attachGetDeniedTest = + assert(route("GET", ["containers", "mycontainer", "attach"]) == "deny") + + // A pin, not a discriminator: with ALLOW_ATTACH = true the lifecycle + // branch still denies it, since `attach` is not an action. + run attachPostDeniedTest = + assert(route("POST", ["containers", "mycontainer", "attach"]) == "deny") + + run attachHeadDeniedTest = + assert(route("HEAD", ["containers", "mycontainer", "attach"]) == "deny") + + // A container literally named `attach` is denied too, as + // `/containers/exec/...` is for exec. + run attachNamedContainerDeniedTest = + assert(route("GET", ["containers", "attach", "ws"]) == "deny") + + run attachSoundTest = + assert(attachNeverAllowed) } // Rust before #48: `/containers/` and `/containers//start` reach @@ -364,7 +415,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, READONLY = false).* + import router_model(ALLOW_EMPTY_NAME = true, ALLOW_PERCENT = false, ALLOW_EMPTY_INTERIOR = true, READONLY = false, ALLOW_ATTACH = false).* run pre48UnsoundTest = assert(and { @@ -378,7 +429,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, READONLY = false).* + import router_model(ALLOW_EMPTY_NAME = false, ALLOW_PERCENT = true, ALLOW_EMPTY_INTERIOR = false, READONLY = false, ALLOW_ATTACH = false).* run pre53UnsoundTest = assert(and { @@ -394,7 +445,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).* + import router_model(ALLOW_EMPTY_NAME = false, ALLOW_PERCENT = false, ALLOW_EMPTY_INTERIOR = true, READONLY = false, ALLOW_ATTACH = false).* run pre55UnsoundTest = assert(and { @@ -408,7 +459,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).* + import router_model(ALLOW_EMPTY_NAME = false, ALLOW_PERCENT = false, ALLOW_EMPTY_INTERIOR = false, READONLY = true, ALLOW_ATTACH = false).* run readonlySoundTest = assert(readonlyAllowsOnlyReads) @@ -428,3 +479,17 @@ module router_readonly { run readonlyHeadAllowedTest = assert(route("HEAD", ["containers", "mycontainer"]) == "allowRead") } + +// All three before #76: attach fell through to the GET passthrough, so +// `GET /containers/{name}/attach/ws` with `Upgrade: websocket` was forwarded +// 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).* + + run pre76UnsoundTest = + assert(and { + not(attachNeverAllowed), + route("GET", ["containers", "mycontainer", "attach"]) == "allowRead", + }) +} From d110b8a816e33b585df7fbddb2dfe8990a3a1356 Mon Sep 17 00:00:00 2001 From: Adrian Bienkowski Date: Mon, 12 Oct 2026 12:53:21 -0400 Subject: [PATCH 2/3] test: pin the attach deny and the upgrade refusal (#76) --- deploy/test.sh | 24 +++++++++ go/internal/proxy/handler_test.go | 76 ++++++++++++++++++++++++++ go/internal/proxy/router_test.go | 50 +++++++++++++++++ rs/src/handler.rs | 75 ++++++++++++++++++++++++++ rs/src/proxy.rs | 43 +++++++++++++++ ts/src/handler.test.ts | 89 +++++++++++++++++++++++++++++-- ts/src/proxy.test.ts | 37 +++++++++++++ 7 files changed, 391 insertions(+), 3 deletions(-) diff --git a/deploy/test.sh b/deploy/test.sh index c111d56..bdcb874 100755 --- a/deploy/test.sh +++ b/deploy/test.sh @@ -448,6 +448,30 @@ 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' +# #76: attach is denied by the router for every method, and any protocol +# upgrade is refused by the handler before routing. Before #76 a websocket +# attach went through the GET passthrough and Go relayed the daemon's 101, +# handing the caller the container's stdin. The target is this test +# container (its hostname is its short ID), so on a proxy without the fix the +# daemon answers 101, not 404. curl does not speak websocket over http://, so +# it reports the status and waits for the body until --max-time. +upgrade_get() { + : >"$BODY" + out=$(curl -s -o "$BODY" -w '%{http_code}' $TIMEOUT --unix-socket "$PROXY_SOCK" \ + -H 'Upgrade: websocket' -H 'Connection: Upgrade' -H 'Sec-WebSocket-Version: 13' \ + -H 'Sec-WebSocket-Key: dGhlIHNhbXBsZSBub25jZQ==' "$1" 2>/dev/null || true) + echo "${out:-000}" +} + +S=$(upgrade_get "$PROXY/containers/$(hostname)/attach/ws?stdin=1&stream=1") +check_denied "GET /containers/*/attach/ws with Upgrade: websocket -> 403 (upgrade refused)" "$S" "$BODY" 'protocol upgrade not allowed' + +S=$(get_status "$PROXY/containers/no-such-container/attach/ws") +check_denied "GET /containers/*/attach/ws -> 403 (attach denied)" "$S" "$BODY" 'attach is not allowed' + +S=$(upgrade_get "$PROXY/_ping") +check_denied "GET /_ping with Upgrade: websocket -> 403 (upgrade refused)" "$S" "$BODY" 'protocol upgrade not allowed' + # ─── Read-only mode (#75) ──────────────────────────────── # The same config behind --readonly. Every request whose method is not GET or diff --git a/go/internal/proxy/handler_test.go b/go/internal/proxy/handler_test.go index 1e9baa9..0f459b9 100644 --- a/go/internal/proxy/handler_test.go +++ b/go/internal/proxy/handler_test.go @@ -377,3 +377,79 @@ container_config: t.Fatalf("ContentLength %d does not match body size %d", rec.lastRequest.ContentLength, len(forwardedBody)) } } + +// #76: any protocol upgrade is refused before the body is read and before +// routing, so the transport never sees it. Before #76 a websocket attach went +// through the GET passthrough and Go relayed the daemon's 101. +func TestHandlerRefusesProtocolUpgrade(t *testing.T) { + dir := t.TempDir() + os.WriteFile(filepath.Join(dir, "default.yaml"), []byte("service_name: default\nallowed_image_prefixes:\n - scratch\n"), 0644) + policyManager, 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) + } + defer auditLog.Close() + const refused = "protocol upgrade not allowed" + + tests := []struct { + name string + target string + headers map[string]string + }{ + {"websocket", "/containers/json", map[string]string{"Upgrade": "websocket", "Connection": "Upgrade"}}, + {"connection token only", "/_ping", map[string]string{"Connection": "keep-alive, Upgrade"}}, + {"upgrade header only", "/_ping", map[string]string{"Upgrade": "h2c"}}, + } + var mismatches []string + for _, tt := range tests { + rec := &recorderTransport{} + h := NewHandler(NewRouter(policyManager), middleware.NewChain(), auditLog, rec) + req := httptest.NewRequest("GET", tt.target, nil) + for k, v := range tt.headers { + req.Header.Set(k, v) + } + w := httptest.NewRecorder() + h.ServeHTTP(w, req) + if got := strings.TrimSpace(w.Body.String()); w.Code != http.StatusForbidden || got != refused { + mismatches = append(mismatches, tt.name+": "+http.StatusText(w.Code)+" "+got) + } + if rec.lastRequest != nil { + mismatches = append(mismatches, tt.name+": forwarded") + } + } + + rec := &recorderTransport{} + h := NewHandler(NewRouter(policyManager), middleware.NewChain(), auditLog, rec) + w := httptest.NewRecorder() + h.ServeHTTP(w, httptest.NewRequest("GET", "/containers/json", nil)) + if w.Code != http.StatusOK || rec.lastRequest == nil { + mismatches = append(mismatches, "plain GET /containers/json: not forwarded") + } + + data, err := os.ReadFile(logPath) + if err != nil { + t.Fatalf("reading audit log: %v", err) + } + lines := strings.Split(strings.TrimSpace(string(data)), "\n") + var entry struct { + Method string `json:"method"` + URI string `json:"uri"` + Decision string `json:"decision"` + Reason string `json:"reason"` + } + if err := json.Unmarshal([]byte(lines[0]), &entry); err != nil { + t.Fatalf("audit line 0: %v", err) + } + if entry.Method != "GET" || entry.URI != "/containers/json" || entry.Decision != "DENY" || entry.Reason != refused { + mismatches = append(mismatches, "audit entry: "+lines[0]) + } + + if len(mismatches) > 0 { + t.Fatalf("protocol upgrade mismatches:\n%s", strings.Join(mismatches, "\n")) + } +} diff --git a/go/internal/proxy/router_test.go b/go/internal/proxy/router_test.go index fc21b71..e225b9e 100644 --- a/go/internal/proxy/router_test.go +++ b/go/internal/proxy/router_test.go @@ -745,3 +745,53 @@ allowed_image_prefixes: } }) } + +// TestRouteAttach is the cross-language parity guard for #76. +// +// attach as a whole segment after /containers/{name} is denied for every +// method, right after the exec check. Before #76 GET .../attach/ws fell +// through to the GET passthrough, and the 101 upgrade gave the caller the +// container's stdin. Rows mirror the attach* runs in spec/router.qnt. +func TestRouteAttach(t *testing.T) { + m := newTestManager(t, map[string]string{ + "beacon.yaml": ` +service_name: beacon +allowed_image_prefixes: + - chainsafe/lodestar +`, + }) + r := NewRouter(m) + attach := "attach is not allowed" + + tests := []struct { + method string + path string + want Action + wantMsg string + }{ + // attachGetDeniedTest (#76): the websocket attach endpoint. + {"GET", "/containers/mycontainer/attach/ws", ActionDeny, attach}, + // attachPostDeniedTest (#76) + {"POST", "/containers/mycontainer/attach", ActionDeny, attach}, + // attachHeadDeniedTest (#76) + {"HEAD", "/containers/mycontainer/attach", ActionDeny, attach}, + // #76: the check runs after the API-version strip. + {"GET", "/v1.45/containers/mycontainer/attach/ws", ActionDeny, attach}, + // #76: every method. + {"DELETE", "/containers/mycontainer/attach", ActionDeny, attach}, + // #76 control: attach-runner is a name like any other. + {"GET", "/containers/attach-runner/json", ActionAllow, ""}, + // #76 control: other container reads stay allowed. + {"GET", "/containers/mycontainer/logs", ActionAllow, ""}, + {"GET", "/containers/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 || 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) + } + }) + } +} diff --git a/rs/src/handler.rs b/rs/src/handler.rs index 35722ca..4629175 100644 --- a/rs/src/handler.rs +++ b/rs/src/handler.rs @@ -530,4 +530,79 @@ mod tests { } assert!(mismatches.is_empty(), "{}", mismatches.join("\n")); } + + /// #76: any protocol upgrade is refused before the body is read and + /// before routing, so the transport never sees it. Before #76 a websocket + /// attach went through the GET passthrough to the daemon. + #[tokio::test] + async fn test_handler_refuses_protocol_upgrade() { + let log_path = std::env::temp_dir().join(format!( + "handler-upgrade-{}-{}.log", + std::process::id(), + std::time::SystemTime::now() + .duration_since(std::time::UNIX_EPOCH) + .unwrap() + .as_nanos() + )); + let _ = std::fs::remove_file(&log_path); + let manager = Manager::from_map(std::collections::HashMap::new()); + let router = Arc::new(Router::new(manager)); + 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: captured_uri.clone(), + }; + let handler = Handler::new(router, Chain::new(), audit, Box::new(transport)); + let refused = "protocol upgrade not allowed"; + + let cases: [(&str, &str, &[(&str, &str)]); 3] = [ + ( + "websocket", + "/containers/json", + &[("Upgrade", "websocket"), ("Connection", "Upgrade")], + ), + ( + "connection token only", + "/_ping", + &[("Connection", "keep-alive, Upgrade")], + ), + ("upgrade header only", "/_ping", &[("Upgrade", "h2c")]), + ]; + let mut mismatches = Vec::new(); + for (name, target, headers) in cases { + let mut req = Request::get(format!("http://localhost{}", target)); + for (k, v) in headers { + req = req.header(*k, *v); + } + let resp = handler.handle(req.body(Full::new(Bytes::new())).unwrap()).await; + let status = resp.status(); + let body = resp.into_body().collect().await.unwrap().to_bytes(); + if status != StatusCode::FORBIDDEN || body != refused { + mismatches.push(format!("{}: got {} {:?}", name, status, String::from_utf8_lossy(&body))); + } + if captured_uri.lock().unwrap().take().is_some() { + mismatches.push(format!("{}: forwarded", name)); + } + } + + let req = Request::get("http://localhost/containers/json") + .body(Full::new(Bytes::new())) + .unwrap(); + let resp = handler.handle(req).await; + if resp.status() != StatusCode::OK || captured_uri.lock().unwrap().is_none() { + mismatches.push("plain GET /containers/json: not forwarded".to_string()); + } + + let contents = std::fs::read_to_string(&log_path).unwrap(); + let _ = std::fs::remove_file(&log_path); + let first: serde_json::Value = serde_json::from_str(contents.lines().next().unwrap()).unwrap(); + if first["method"] != "GET" + || first["uri"] != "/containers/json" + || first["decision"] != "DENY" + || first["reason"] != refused + { + mismatches.push(format!("audit entry: {}", first)); + } + assert!(mismatches.is_empty(), "mismatches:\n{}", mismatches.join("\n")); + } } diff --git a/rs/src/proxy.rs b/rs/src/proxy.rs index 4f1404e..cf0bfae 100644 --- a/rs/src/proxy.rs +++ b/rs/src/proxy.rs @@ -896,6 +896,49 @@ mod tests { assert!(mismatches.is_empty(), "mismatches:\n{}", mismatches.join("\n")); } + /// Cross-language parity guard for #76. attach as a whole segment after + /// /containers/{name} is denied for every method, right after the exec + /// check. Before #76 GET .../attach/ws fell through to the GET + /// passthrough. Rows mirror the attach* runs in spec/router.qnt. + #[test] + fn test_route_attach() { + let router = Router::new(make_manager(vec!["alpine"])); + let attach = Some("attach is not allowed"); + let cases = [ + // attachGetDeniedTest (#76): the websocket attach endpoint. + ("GET", "/containers/mycontainer/attach/ws", Action::Deny, attach), + // attachPostDeniedTest (#76) + ("POST", "/containers/mycontainer/attach", Action::Deny, attach), + // attachHeadDeniedTest (#76) + ("HEAD", "/containers/mycontainer/attach", Action::Deny, attach), + // #76: the check runs after the API-version strip. + ("GET", "/v1.45/containers/mycontainer/attach/ws", Action::Deny, attach), + // #76: every method. + ("DELETE", "/containers/mycontainer/attach", Action::Deny, attach), + // #76 control: attach-runner is a name like any other. + ("GET", "/containers/attach-runner/json", Action::Allow, None), + // #76 control: other container reads stay allowed. + ("GET", "/containers/mycontainer/logs", Action::Allow, None), + ("GET", "/containers/json", Action::Allow, None), + ]; + let mut mismatches = Vec::new(); + for (method, path, want, want_msg) in cases { + let got = router.route(method, path, None); + 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("-") + )); + } + } + 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/ts/src/handler.test.ts b/ts/src/handler.test.ts index 37a6ced..2c7af30 100644 --- a/ts/src/handler.test.ts +++ b/ts/src/handler.test.ts @@ -4,7 +4,8 @@ import { mkdtempSync, readFileSync, writeFileSync, rmSync } from "node:fs"; import { join } from "node:path"; import { tmpdir } from "node:os"; import { Readable } from "node:stream"; -import type { IncomingMessage, ServerResponse } from "node:http"; +import { createServer, request, type IncomingMessage, type ServerResponse } from "node:http"; +import type { AddressInfo } from "node:net"; import { Handler } from "./handler.js"; import { Chain } from "./middleware.js"; import { Router } from "./proxy.js"; @@ -41,11 +42,19 @@ function makeResponse(): { res: ServerResponse; recorder: RecorderRes } { return { res, recorder }; } -function makeRequest(method: string, url: string, body?: string): IncomingMessage { +function makeRequest( + method: string, + url: string, + body?: string, + headers: Record = {}, +): IncomingMessage { const stream = Readable.from(body ? [Buffer.from(body)] : []); (stream as IncomingMessage).method = method; (stream as IncomingMessage).url = url; - (stream as IncomingMessage).headers = body ? { "content-length": String(Buffer.byteLength(body)) } : {}; + (stream as IncomingMessage).headers = { + ...(body ? { "content-length": String(Buffer.byteLength(body)) } : {}), + ...headers, + }; return stream as IncomingMessage; } @@ -221,4 +230,78 @@ describe("Handler", () => { assert.equal(entry.uri, target, `${method} ${target}`); }); }); + + // #76: any protocol upgrade is refused before the body is read and before + // routing, so the transport never sees it. Before #76 a websocket attach + // went through the GET passthrough to the daemon. + it("refuses a protocol upgrade", async () => { + const dir = makeEnv(defaultConfig); + const { handler, transport } = newHandler(dir); + const refused = "protocol upgrade not allowed"; + const cases: [string, string, Record][] = [ + ["websocket", "/containers/json", { upgrade: "websocket", connection: "Upgrade" }], + ["connection token only", "/_ping", { connection: "keep-alive, Upgrade" }], + ["upgrade header only", "/_ping", { upgrade: "h2c" }], + ]; + const mismatches: string[] = []; + for (const [name, target, headers] of cases) { + const { res, recorder } = makeResponse(); + await handler.handle(makeRequest("GET", target, undefined, headers), res); + if (recorder.statusCode !== 403 || recorder.body !== refused) { + mismatches.push(`${name}: got ${recorder.statusCode} ${JSON.stringify(recorder.body)}`); + } + if (transport.lastRequest) mismatches.push(`${name}: forwarded`); + transport.lastRequest = undefined; + } + await handler.handle(makeRequest("GET", "/containers/json"), makeResponse().res); + if (!transport.lastRequest) mismatches.push("plain GET /containers/json: not forwarded"); + + const first = JSON.parse(readFileSync(join(dir, "audit.log"), "utf-8").split("\n")[0]!) as Record; + if ( + first.method !== "GET" || + first.uri !== "/containers/json" || + first.decision !== "DENY" || + first.reason !== refused + ) { + mismatches.push(`audit entry: ${JSON.stringify(first)}`); + } + assert.deepEqual(mismatches, []); + }); + + // #76, TS-only: Node's http.Server routes a request with Upgrade and + // Connection: Upgrade to the "upgrade" event when it has a listener. The + // proxy registers none, so the request reaches the handler, which refuses it. + it("refuses a protocol upgrade through a real http.Server", async () => { + const dir = makeEnv(defaultConfig); + const { handler, transport } = newHandler(dir); + const server = createServer((req, res) => void handler.handle(req, res)); + await new Promise((resolve) => server.listen(0, "127.0.0.1", resolve)); + const { port } = server.address() as AddressInfo; + try { + const got = await new Promise((resolve, reject) => { + const req = request( + { + host: "127.0.0.1", + port, + path: "/containers/mycontainer/attach/ws?stdin=1&stream=1", + headers: { Upgrade: "websocket", Connection: "Upgrade" }, + }, + (res) => { + let body = ""; + res.on("data", (c: Buffer) => (body += c.toString())); + res.on("end", () => resolve(`${res.statusCode} ${body}`)); + }, + ); + req.on("upgrade", (res) => resolve(`upgraded ${res.statusCode}`)); + req.on("error", reject); + req.setTimeout(2000, () => req.destroy(new Error("no response within 2s"))); + req.end(); + }); + assert.equal(got, "403 protocol upgrade not allowed"); + assert.equal(transport.lastRequest, undefined); + } finally { + server.closeAllConnections(); + await new Promise((resolve) => server.close(resolve)); + } + }); }); diff --git a/ts/src/proxy.test.ts b/ts/src/proxy.test.ts index 19f69e6..9807c2d 100644 --- a/ts/src/proxy.test.ts +++ b/ts/src/proxy.test.ts @@ -502,4 +502,41 @@ describe("Router", () => { } assert.deepEqual(mismatches, []); }); + + // Cross-language parity guard for #76. attach as a whole segment after + // /containers/{name} is denied for every method, right after the exec + // check. Before #76 GET .../attach/ws fell through to the GET passthrough. + // Rows mirror the attach* runs in spec/router.qnt. + it("denies attach for every method", () => { + const attach = "attach is not allowed"; + const cases: [string, string, Action, string | undefined][] = [ + // attachGetDeniedTest (#76): the websocket attach endpoint. + ["GET", "/containers/mycontainer/attach/ws", Action.Deny, attach], + // #76, TS-only: the TS router receives the raw target, query included. + ["GET", "/containers/mycontainer/attach/ws?stdin=1&stream=1", Action.Deny, attach], + // attachPostDeniedTest (#76) + ["POST", "/containers/mycontainer/attach", Action.Deny, attach], + // attachHeadDeniedTest (#76) + ["HEAD", "/containers/mycontainer/attach", Action.Deny, attach], + // #76: the check runs after the API-version strip. + ["GET", "/v1.45/containers/mycontainer/attach/ws", Action.Deny, attach], + // #76: every method. + ["DELETE", "/containers/mycontainer/attach", Action.Deny, attach], + // #76 control: attach-runner is a name like any other. + ["GET", "/containers/attach-runner/json", Action.Allow, undefined], + // #76 control: other container reads stay allowed. + ["GET", "/containers/mycontainer/logs", Action.Allow, undefined], + ["GET", "/containers/json", Action.Allow, undefined], + ]; + const mismatches: string[] = []; + for (const [method, path, want, wantMsg] of cases) { + const r = router.route(method, path); + if (r.action !== want || r.denyMsg !== wantMsg) { + mismatches.push( + `${method} ${path}: got ${Action[r.action]}/${r.denyMsg ?? "-"}, want ${Action[want]}/${wantMsg ?? "-"}`, + ); + } + } + assert.deepEqual(mismatches, []); + }); }); From ac8d766afae36bce7e3b2c1f39a1d308b46b46d8 Mon Sep 17 00:00:00 2001 From: Adrian Bienkowski Date: Mon, 12 Oct 2026 12:55:21 -0400 Subject: [PATCH 3/3] fix: deny attach and refuse protocol upgrades in all three (#76) --- AGENTS.md | 10 +++++----- README.md | 18 +++++++++++------- deploy/test.sh | 5 +++-- go/internal/proxy/handler.go | 27 +++++++++++++++++++++++++++ go/internal/proxy/router.go | 11 +++++++++++ rs/src/handler.rs | 22 ++++++++++++++++++++++ rs/src/proxy.rs | 10 ++++++++++ ts/src/handler.ts | 19 +++++++++++++++++++ ts/src/proxy.ts | 8 ++++++++ 9 files changed, 116 insertions(+), 14 deletions(-) diff --git a/AGENTS.md b/AGENTS.md index 22734c5..fe9f89c 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -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: 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) +- Go: 110 unit tests (main/listener: 24, policy: 10, middleware: 25, proxy: 47, audit: 4) +- Rust: 142 unit tests (main/listener: 23, policy: 15, middleware: 41, proxy: 47, handler: 10, audit: 4, transport: 2) +- TypeScript: 160 unit tests, 1 skipped (flags: 44, listen: 13 incl. 1 skipped concurrency test (#46), middleware: 32, proxy: 34, policy: 10, handler: 13, shutdown: 5, transport: 5, audit: 4) +- Integration, per implementation: 53 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`, and `router_pre76`) ## 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` (50 test cases) and `make test-integration-sock` (15 socket cases) via Docker Compose +- Integration: `make test-integration` (53 test cases) and `make test-integration-sock` (15 socket cases) via Docker Compose ## Contribution Workflow diff --git a/README.md b/README.md index 4136dc0..f3deff5 100644 --- a/README.md +++ b/README.md @@ -12,7 +12,7 @@ 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**: 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, 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, attach 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 + 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 | +| Go | [go/](go/) | 110 unit + 53 integration + 15 socket | stdlib net/http + yaml.v3 | +| Rust | [rs/](rs/) | 142 unit + 53 integration + 15 socket | tokio, hyper, serde, clap | +| TypeScript | [ts/](ts/) | 160 unit (1 skipped) + 53 integration + 15 socket | Node 22 ESM, built-in http | ### Build All @@ -198,13 +198,14 @@ docker pull attacker/malware:latest # denied: image not in allowlist | 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)). +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)). Attach is denied by the router for every method, so a websocket attach (`GET /containers/{name}/attach/ws`) cannot reach a container's stdin, with or without `--readonly` ([#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 | Request with an `Upgrade` header, or `Connection: upgrade` | **DENIED** (checked before routing) | +| Not GET/HEAD | Any (with `--readonly`) | **DENIED** (checked first in the router) | | Any | Path containing `%` | **DENIED** (see below) | | Any | Path containing `//` | **DENIED** | | POST | `/containers/create` | Validated by middleware chain | @@ -212,6 +213,7 @@ The chain runs only on container create. `--readonly` is not a gate: the router | DELETE | `/containers/{name}` | Allowed on known containers | | Any | `/containers/…/exec` (an `exec` segment anywhere under `/containers`) | **DENIED** | | Any | `/exec/*` | **DENIED** | +| Any | `/containers/{name}/attach…` (an `attach` segment anywhere under `/containers`) | **DENIED** | | POST | `/containers/{name}/rename\|update` | **DENIED** | | POST | `/images/create` | Validated by registry gate | | POST | `/auth` | **DENIED** | @@ -224,6 +226,8 @@ Any request whose path contains a percent-encoded byte (`%`) is denied with 403 Exec is matched on whole path segments, so containers named like `exec-runner` work normally, and exec inspect (`GET /exec/{id}/json`) is denied too. A path with an empty segment (`//`, as in `//exec/{id}/json`) is denied for every method, so an empty segment cannot hide the exec namespace; a single trailing slash is fine, and `//` in the query string is not inspected. Absolute-form request targets (`GET http://host/…`) never reach exec: Go and Rust route them on the parsed path, so exec is denied and other paths are routed as usual, while the TypeScript proxy sees the raw target and denies every absolute-form request (its `//` after the scheme is an empty segment). The Docker CLI never sends absolute form. +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. + ## Configuration ### CLI Flags @@ -441,7 +445,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 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. +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)), exec ([#49](https://github.com/ChainSafe/docker-socket-policy/issues/49)) and attach ([#76](https://github.com/ChainSafe/docker-socket-policy/issues/76)), 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 9 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/test.sh b/deploy/test.sh index bdcb874..92a1523 100755 --- a/deploy/test.sh +++ b/deploy/test.sh @@ -3,8 +3,9 @@ # 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 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). +# ContainerConfigMutator are all exercised; exec and attach are denied by the +# router, protocol upgrades by the handler (#76), and every write on a second +# proxy run with --readonly (#75). set -e diff --git a/go/internal/proxy/handler.go b/go/internal/proxy/handler.go index bd585f4..d6c5a94 100644 --- a/go/internal/proxy/handler.go +++ b/go/internal/proxy/handler.go @@ -6,6 +6,7 @@ import ( "io" "log/slog" "net/http" + "strings" "github.com/ChainSafe/docker-socket-policy/go/internal/audit" "github.com/ChainSafe/docker-socket-policy/go/internal/middleware" @@ -32,6 +33,16 @@ func NewHandler(router *Router, chain *middleware.Chain, auditLog *audit.Logger, } func (h *Handler) ServeHTTP(w http.ResponseWriter, r *http.Request) { + // Refuse protocol upgrades before anything else: the reverse proxy would + // relay a 101 and hand the caller a raw stream to the daemon (#76). + if isUpgradeRequest(r.Header) { + const msg = "protocol upgrade not allowed" + slog.Warn("denied", "method", r.Method, "path", r.URL.EscapedPath(), "reason", msg) + h.auditLog.Deny(r.Method, r.RequestURI, msg, map[string]interface{}{"method": r.Method, "path": r.URL.EscapedPath()}) + http.Error(w, msg, http.StatusForbidden) + return + } + body, err := io.ReadAll(r.Body) if err != nil { slog.Warn("failed to read request body", "error", err) @@ -96,3 +107,19 @@ func (h *Handler) ServeHTTP(w http.ResponseWriter, r *http.Request) { h.transport.ServeHTTP(w, r) } + +// isUpgradeRequest reports an Upgrade header, or a Connection header that +// lists the upgrade token. +func isUpgradeRequest(h http.Header) bool { + if _, ok := h["Upgrade"]; ok { + return true + } + for _, v := range h.Values("Connection") { + for _, tok := range strings.Split(v, ",") { + if strings.EqualFold(strings.TrimSpace(tok), "upgrade") { + return true + } + } + } + return false +} diff --git a/go/internal/proxy/router.go b/go/internal/proxy/router.go index 189689b..3438dc6 100644 --- a/go/internal/proxy/router.go +++ b/go/internal/proxy/router.go @@ -71,6 +71,10 @@ func (r *Router) Route(method, path string, body map[string]interface{}) *RouteR return &RouteResult{Action: ActionDeny, DenyMsg: "exec is not allowed"} } + if isAttachPath(path) { + return &RouteResult{Action: ActionDeny, DenyMsg: "attach is not allowed"} + } + if strings.HasPrefix(path, "/build") { return &RouteResult{Action: ActionDeny, DenyMsg: "build is not allowed"} } @@ -216,6 +220,13 @@ func isExecPath(path string) bool { return segs[0] == "exec" || (segs[0] == "containers" && slices.Contains(segs[1:], "exec")) } +// isAttachPath matches attach on whole segments after /containers, so a name +// like attach-runner is not attach (#76). +func isAttachPath(path string) bool { + segs := strings.Split(strings.TrimPrefix(path, "/"), "/") + return segs[0] == "containers" && slices.Contains(segs[1:], "attach") +} + // 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 diff --git a/rs/src/handler.rs b/rs/src/handler.rs index 4629175..d7c67f9 100644 --- a/rs/src/handler.rs +++ b/rs/src/handler.rs @@ -45,6 +45,18 @@ impl Handler { let headers = parts.headers.clone(); let version = parts.version; + // Refuse protocol upgrades before anything else: forwarding one would + // hand the caller a raw stream to the daemon (#76). + if is_upgrade_request(&headers) { + let msg = "protocol upgrade not allowed"; + warn!("denied {} {}: {}", method, path, msg); + self.audit.deny(&method, &target, msg); + return Response::builder() + .status(StatusCode::FORBIDDEN) + .body(Full::new(Bytes::from(msg))) + .unwrap(); + } + let mut body_bytes = match body.collect().await { Ok(collected) => collected.to_bytes(), Err(_) => { @@ -126,6 +138,16 @@ impl Handler { } } +/// 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) + || headers.get_all(header::CONNECTION).iter().any(|v| { + v.as_bytes() + .split(|&b| b == b',') + .any(|tok| tok.trim_ascii().eq_ignore_ascii_case(b"upgrade")) + }) +} + #[cfg(test)] mod tests { use super::*; diff --git a/rs/src/proxy.rs b/rs/src/proxy.rs index cf0bfae..6b9140a 100644 --- a/rs/src/proxy.rs +++ b/rs/src/proxy.rs @@ -76,6 +76,9 @@ impl Router { if is_exec_path(path) { return deny("exec is not allowed"); } + if is_attach_path(path) { + return deny("attach is not allowed"); + } if path.starts_with("/build") { return deny("build is not allowed"); } @@ -252,6 +255,13 @@ fn is_exec_path(path: &str) -> bool { } } +// Attach is matched on whole segments after /containers, so a name like +// attach-runner is not attach (#76). +fn is_attach_path(path: &str) -> bool { + let mut segs = path.strip_prefix('/').unwrap_or(path).split('/'); + segs.next() == Some("containers") && segs.any(|s| s == "attach") +} + fn extract_container_name(path: &str) -> Option<&str> { let path = path.strip_prefix('/').unwrap_or(path); let parts: Vec<&str> = path.split('/').collect(); diff --git a/ts/src/handler.ts b/ts/src/handler.ts index a220232..58d0a6a 100644 --- a/ts/src/handler.ts +++ b/ts/src/handler.ts @@ -18,6 +18,18 @@ export class Handler { const method = req.method ?? "GET"; const path = req.url ?? "/"; + // Refuse protocol upgrades before anything else: forwarding one would + // hand the caller a raw stream to the daemon (#76). The server registers + // no "upgrade" listener, so Node delivers these requests here. + if (isUpgradeRequest(req)) { + const msg = "protocol upgrade not allowed"; + console.warn(`[${requestId}] DENIED ${method} ${path}: ${msg}`); + this.audit.deny(method, path, msg); + res.writeHead(403); + res.end(msg); + return; + } + const bodyBytes = await readBody(req); let bodyJSON: Record | undefined; @@ -62,6 +74,13 @@ export class Handler { } } +// An Upgrade header, or a Connection header that lists the upgrade token. +function isUpgradeRequest(req: IncomingMessage): boolean { + if (req.headers.upgrade !== undefined) return true; + const connection = req.headers.connection ?? ""; + return connection.split(",").some((tok) => tok.trim().toLowerCase() === "upgrade"); +} + function readBody(req: IncomingMessage): Promise { return new Promise((resolve) => { const chunks: Buffer[] = []; diff --git a/ts/src/proxy.ts b/ts/src/proxy.ts index 33bbd04..959927e 100644 --- a/ts/src/proxy.ts +++ b/ts/src/proxy.ts @@ -49,6 +49,7 @@ export class Router { // Denied endpoints if (path.startsWith("/auth")) return { action: Action.Deny, denyMsg: "auth endpoint is not allowed" }; if (isExecPath(path)) return { action: Action.Deny, denyMsg: "exec is not allowed" }; + if (isAttachPath(path)) return { action: Action.Deny, denyMsg: "attach is not allowed" }; if (path.startsWith("/build")) return { action: Action.Deny, denyMsg: "build is not allowed" }; if (path.startsWith("/commit")) return { action: Action.Deny, denyMsg: "commit is not allowed" }; @@ -145,6 +146,13 @@ function isExecPath(path: string): boolean { return first === "exec" || (first === "containers" && rest.includes("exec")); } +// Attach is matched on whole segments after /containers, so a name like +// attach-runner is not attach (#76). +function isAttachPath(path: string): boolean { + const [first, ...rest] = path.replace(/^\//, "").split("/"); + return first === "containers" && rest.includes("attach"); +} + function extractContainerName(path: string): string | undefined { const parts = path.replace(/^\//, "").split("/"); if (parts[0] === "containers" && parts[1] && !["create", "json", "exec"].includes(parts[1])) {