From 182f041dfe6d7cf3fc64b4d60e144f6e35abff93 Mon Sep 17 00:00:00 2001 From: Zoltan Csizmadia Date: Sat, 19 Sep 2026 11:35:32 -0500 Subject: [PATCH] fix: judge docker plugin install against allow-registries Closes #420. The plugin endpoints were grouped with swarm in unattributablePath and so inherited the BUILD default: allowed unless deny-unattributable-builds is explicitly set, which is off. A plain allow-registries therefore let docker plugin install evil.example.com/rogue 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. The premise was wrong, not just the default ------------------------------------------- I filed #420 saying these endpoints "carry no attributable image reference". True of swarm. Not true of plugins -- and the comment directly above the regex said so all along: they "fetch from an arbitrary registry named in `remote`". The reference is in the query. So this is not a new policy or a stricter default. It is the rule the operator already wrote, applied to an endpoint that was missed. pluginPullTarget reads `remote` with the same body-shadows-query precedence pullTarget uses -- dockerd's own -- and the answer goes to DenyPull. Consequences, stated rather than discovered later: - allow-registries refuses a plugin from an unlisted registry, with no opt-in. That is the fix. - require-digest applies too, because a plugin IS an image from a registry and DenyPull is one rule set. An unpinned plugin install on a machine with require-digest set will now be refused. - a plugin from a LISTED registry installs exactly as before. - swarm is unchanged: a TaskSpec genuinely cannot be attributed, and refusing beats parsing it and getting it subtly wrong. Fail closed on what we cannot read ---------------------------------- The plugin paths stay in unattributablePath as a FALLBACK. pluginPullTarget runs first and judges the attributable case precisely; a plugin pull that names no remote -- malformed, or a shape not anticipated -- falls through to the old conservative treatment rather than passing unjudged. Verification ------------ Negative control, with the plugin branch disabled: status = 200, want 403 -- allow-registries alone must refuse this reached the engine The permitted case is asserted too, so this cannot have bought safety by banning plugins: a pull from an allowed registry reaches the engine and returns 200, and the gate is checked to have seen the remote rather than an empty string. An existing test asserted the old grouping. It is split rather than deleted: swarm and the no-remote plugin pull keep their DenyBuild assertions, and the attributable plugin pulls move to a test that names what changed. --- CHANGELOG.md | 18 +++++++ docs/policy.md | 39 ++++++++++----- internal/pipeproxy/imagegate_test.go | 57 ++++++++++++++++++++-- internal/pipeproxy/rewrite.go | 71 ++++++++++++++++++++++++++-- 4 files changed, 165 insertions(+), 20 deletions(-) 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