diff --git a/CHANGELOG.md b/CHANGELOG.md index 78c4601..4914716 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -12,6 +12,24 @@ useful than saying where the real one is. ## [Unreleased] +### Changed + +- **`allow-registries` now applies to `docker plugin install`** + ([#420](https://github.com/wslkit/skrog/issues/420)). A plugin pull names its + registry, so it is judged exactly like `docker pull` — including + `require-digest` if you have set it. Previously the plugin endpoints were + grouped with swarm as "unattributable" and inherited the permissive **build** + default, so a plain `allow-registries` let a plugin from any registry + through. A plugin gets host device and mount access where an image gets a + container, which made that a worse hole than the build one the default was + chosen to tolerate. + + **This can refuse something that worked before:** installing a plugin from a + registry your allowlist does not list now returns 403. A plugin from a listed + registry is unaffected. Swarm and `/swarm/init` are unchanged — they still + need `deny-unattributable-builds`, because a TaskSpec genuinely cannot be + attributed. + ### Fixed The concurrency findings from the pre-0.6.0 review, which were filed but not diff --git a/docs/policy.md b/docs/policy.md index b80fce3..557b62e 100644 --- a/docs/policy.md +++ b/docs/policy.md @@ -241,14 +241,26 @@ supplies the `allow-registries` that makes it bite. That is the intended outcome — the administrator said "no unattributable builds where images are restricted", and they are. -**Plugins and swarm services.** `POST /plugins/pull` and the swarm/service -endpoints carry no attributable image reference either, and they are refused -only when `deny-unattributable-builds` is on — which is off by default. So with -a plain `allow-registries`, `docker plugin install evil.example.com/p` is -allowed, and a Docker plugin gets host device and mount access: a worse outcome -than the build hole the default was chosen to tolerate. Tracked as -[#420](https://github.com/wslkit/skrog/issues/420); set -`deny-unattributable-builds` if this matters to you today. +**Swarm services.** `POST /services/create` and `/swarm/init` run an image +from a TaskSpec this gate does not parse, so they cannot be attributed to a +registry. They are refused only when `deny-unattributable-builds` is on — which +is off by default. Refusing beats parsing a TaskSpec and getting it subtly +wrong, which is how two earlier bypasses happened. + +> **Plugins used to be in this list, and should not have been** +> ([#420](https://github.com/wslkit/skrog/issues/420)). `docker plugin install` +> names its registry in the request, so it *is* attributable — and because it +> was lumped in with swarm it inherited the permissive **build** default, so a +> plain `allow-registries` let `docker plugin install evil.example.com/p` +> through. A plugin gets host device and mount access where an image gets a +> container, which made it a worse hole than the build one the default was +> chosen to tolerate. +> +> **`allow-registries` now applies to plugins**, with no opt-in, exactly as it +> does to `docker pull` — including `require-digest` if you have set it. A +> plugin from a listed registry installs as before. A plugin pull that somehow +> names no registry keeps the old conservative treatment rather than passing +> unjudged. **A registry mirror.** `skrog cache enable --upstream ` wires `registry-mirrors` into the engine, and the upstream is not checked against @@ -387,10 +399,13 @@ Also judged: `POST /volumes/create`, when `allow-bind-sources` is set — a container create that follows carries only the volume's name ([#419](https://github.com/wslkit/skrog/issues/419)). -Not judged: `POST /plugins/pull` and the swarm/service endpoints, which are -refused only when `deny-unattributable-builds` is on — and that is off by -default ([#420](https://github.com/wslkit/skrog/issues/420)). A Docker plugin -gets host device and mount access, so this is the gap worth knowing about. +Also judged: `POST /plugins/pull` and `/plugins/{name}/upgrade`, against +`allow-registries`, because a plugin names the registry it comes from +([#420](https://github.com/wslkit/skrog/issues/420)). + +Not judged: the swarm and service endpoints, which run an image from a +TaskSpec this gate does not parse and so are refused only when +`deny-unattributable-builds` is on. Resource caps on an unset container — the one *mutating* rule in the original proposal — are deliberately not implemented: mutating a user's request diff --git a/internal/pipeproxy/imagegate_test.go b/internal/pipeproxy/imagegate_test.go index dba2f39..d2b7627 100644 --- a/internal/pipeproxy/imagegate_test.go +++ b/internal/pipeproxy/imagegate_test.go @@ -378,12 +378,15 @@ func TestLowercaseCreateFieldsAreStillJudged(t *testing.T) { // traffic that cannot be attributed must not proceed. func TestUnattributableEndpointsAreRefused(t *testing.T) { for _, path := range []string{ - "/v1.44/plugins/pull?remote=evil.example.com/rogue", - "/plugins/pull", - "/v1.44/plugins/evil%2Frogue/upgrade?remote=evil.example.com/rogue", + // Swarm genuinely cannot be attributed without parsing a TaskSpec, + // and refusing beats parsing it and getting it subtly wrong. "/v1.44/services/create", "/v1.44/services/abc123/update", "/v1.44/swarm/init", + // A plugin pull with NO remote: attributable in principle, not in + // this request. It keeps the conservative treatment rather than + // passing unjudged (#420). + "/plugins/pull", } { gate := &fakeImageGate{denyBuild: "allowlist is in force"} resp, engineReached := driveRequest(t, gate, @@ -398,6 +401,54 @@ func TestUnattributableEndpointsAreRefused(t *testing.T) { } } +// A plugin pull that NAMES its registry is judged as the pull it is (#420), +// not as an unattributable build. +// +// This is the hole the split closes. These endpoints used to inherit the build +// default — allowed unless deny-unattributable-builds was explicitly set — so +// a plain allow-registries let `docker plugin install evil.example.com/p` +// through. A plugin gets host device and mount access where an image gets a +// container, which made it a worse hole than the build one the permissive +// default was chosen to tolerate. +func TestPluginPullIsJudgedAsAPull(t *testing.T) { + for _, path := range []string{ + "/v1.44/plugins/pull?remote=evil.example.com/rogue", + "/v1.44/plugins/evil%2Frogue/upgrade?remote=evil.example.com/rogue", + "/plugins/pull?remote=evil.example.com/rogue&name=rogue", + } { + // Note: denyBuild is EMPTY. The whole point is that the registry + // allowlist alone refuses this, with no opt-in. + gate := &fakeImageGate{denyPull: "policy does not allow images from evil.example.com"} + resp, engineReached := driveRequest(t, gate, + "POST "+path+" HTTP/1.1\r\nHost: d\r\nContent-Length: 0\r\n\r\n") + + if resp.StatusCode != http.StatusForbidden { + t.Errorf("%s: status = %d, want 403 — allow-registries alone must refuse this", path, resp.StatusCode) + } + if engineReached() { + t.Errorf("%s: reached the engine", path) + } + if gate.sawPull != "evil.example.com/rogue" { + t.Errorf("%s: gate saw pull %q, want the remote", path, gate.sawPull) + } + } +} + +// ...and a plugin from an ALLOWED registry still installs. The rule restricts +// where plugins come from; it does not ban the feature. +func TestPluginPullFromAnAllowedRegistryProceeds(t *testing.T) { + gate := &fakeImageGate{} // allows everything + resp, engineReached := driveRequest(t, gate, + "POST /v1.44/plugins/pull?remote=registry.example.com/ok HTTP/1.1\r\nHost: d\r\nContent-Length: 0\r\n\r\n") + + if resp.StatusCode != http.StatusOK || !engineReached() { + t.Errorf("a permitted plugin was blocked (status %d, reached=%v)", resp.StatusCode, engineReached()) + } + if gate.sawPull != "registry.example.com/ok" { + t.Errorf("gate saw pull %q, want the remote", gate.sawPull) + } +} + // And with no allowlist they pass untouched: this must not break swarm or // plugins on a machine with no policy deployed. func TestUnattributableEndpointsPassWithoutAPolicy(t *testing.T) { diff --git a/internal/pipeproxy/rewrite.go b/internal/pipeproxy/rewrite.go index e9abe63..ba459aa 100644 --- a/internal/pipeproxy/rewrite.go +++ b/internal/pipeproxy/rewrite.go @@ -184,6 +184,11 @@ func rewriteBinds(client net.Conn, engine io.ReadWriteCloser, audit AuditSink, g if reason, no := ig.DenyPull(image); no { denied = errors.New(reason) } + } else if remote, isPlugin := pluginPullTarget(req); isPlugin { + // Judged as a pull, because that is what it is (#420). + if reason, no := ig.DenyPull(remote); no { + denied = errors.New(reason) + } } else if image, isPush := pushTarget(req); isPush { if reason, no := ig.DenyPush(image); no { denied = errors.New(reason) @@ -827,20 +832,40 @@ var ( // unattributablePath matches endpoints that can fetch or run an image // WITHOUT naming it anywhere this gate can judge (#322). // - // /plugins/pull, /plugins/*/upgrade fetch from an arbitrary registry - // named in `remote`, and a plugin gets - // host device and mount access. // /services/create, /services/*/update, /swarm/init - // a swarm task pulls and runs an image - // from a spec this gate does not parse. + // a swarm task pulls and runs an image from a TaskSpec this gate + // does not parse. // // They are refused on exactly the same ground as a build: with an allowlist // in force, traffic that cannot be attributed to an allowed registry must not // proceed. Refusing beats parsing a swarm TaskSpec and getting it subtly // wrong, which is how the first two bypasses in this file happened. + // + // The plugin endpoints are still here, but they are now the FALLBACK + // rather than the rule (#420). + // + // Treating them as unattributable was wrong: /plugins/pull and + // /plugins/{name}/upgrade name their registry in `remote`, which the + // comment above said all along. Lumping them in with swarm meant they + // inherited the BUILD default — allowed unless deny-unattributable-builds + // is explicitly set — so a plain allow-registries let `docker plugin + // install evil.example.com/p` through. A plugin gets host device and mount + // access where an image gets a container, so that was a worse hole than the + // build one the permissive default was chosen to tolerate. + // + // pluginPullTarget runs first and judges them as the pulls they are. They + // reach here only when `remote` is absent — a malformed request, or a + // shape we did not anticipate — and then the old conservative treatment + // applies rather than passing unjudged. Fail closed on the case we cannot + // read, judge precisely the case we can. unattributablePath = regexp.MustCompile( `^(/v[0-9.]+)?/(plugins/pull|plugins/.+/upgrade|services/create|services/.+/update|swarm/init)$`) + // pluginPullPath matches the two endpoints that fetch a plugin from a + // registry. `docker plugin install` is a pull followed by an enable, and + // the pull is where the bytes come from. + pluginPullPath = regexp.MustCompile(`^(/v[0-9.]+)?/plugins/(pull|.+/upgrade)$`) + // pushPath matches `docker push` and `docker plugin push` (#353). // // The reference is in the PATH rather than the query, unescaped and @@ -890,6 +915,42 @@ func pushTarget(req *http.Request) (string, bool) { // // The body is buffered and restored, so the request still forwards byte for // byte -- the caller writes req.Body downstream. +// pluginPullTarget is the plugin reference a /plugins/pull or +// /plugins/{name}/upgrade is fetching (#420). +// +// The reference is in `remote`, plainly, which is why lumping these in with +// swarm as "unattributable" was wrong. A plugin is distributed as an image +// from a registry, so the answer goes to the same DenyPull the image rules +// already use: an allowlist that forbids a registry for `docker pull` and +// permits it for `docker plugin install` is not an allowlist, and a plugin +// gets host device and mount access where an image gets a container. +// +// The empty-remote case returns false rather than an empty reference, so a +// malformed request reaches the engine and is rejected there with a far better +// message than a guess here would produce. +func pluginPullTarget(req *http.Request) (remote string, isPluginPull bool) { + if req.Method != http.MethodPost || !pluginPullPath.MatchString(req.URL.Path) { + return "", false + } + // Same body-shadows-query precedence as pullTarget, for the same reason: + // it is what dockerd does, and judging the other one judges nothing. + get := req.URL.Query().Get + if isFormEncoded(req) { + if form, ok := formBody(req); ok { + get = func(k string) string { + if v, ok := form[k]; ok && len(v) > 0 { + return v[0] + } + return req.URL.Query().Get(k) + } + } + } + if r := strings.TrimSpace(get("remote")); r != "" { + return r, true + } + return "", false +} + func pullTarget(req *http.Request) (image string, isPull bool) { if req.Method != http.MethodPost || !imageCreatePath.MatchString(req.URL.Path) { return "", false