From 0d9818890f4042d9583c1011b710537d96f300aa Mon Sep 17 00:00:00 2001 From: Zoltan Csizmadia Date: Sat, 19 Sep 2026 11:21:59 -0500 Subject: [PATCH] fix: judge POST /volumes/create against allow-bind-sources Closes #419. docs/policy.md said, for a long time: "Named volumes are not binds -- `-v myvol:/data` has no host path to restrict and is never denied by this rule." True of the common case, false in general. A local-driver volume can name a host path: docker volume create -d local -o type=none -o o=bind \ -o device=/mnt/c/secrets esc docker run -v esc:/out ubuntu cat /out/... /volumes/create was judged by nothing, and the container create that follows carries only the volume's NAME -- which bindSources deliberately skips, because a name is not a path. So with allow-bind-sources: [C:\work] in force, those two commands read C:\secrets. The rule was not weak here; it was absent. device=/ is the same trick against the whole guest filesystem. The device is a GUEST path -- dockerd is what opens it -- so it is mapped back from /mnt//... to Windows form before comparison, the inverse of the translation the bridge already applies to binds. Two consequences, both documented: - a device under no Windows drive (/, /etc, /var/lib/docker) is refused. It is under no allowed root, and that is the point. - a third-party volume driver is refused while this rule is in force, because its options are its own vocabulary and cannot be checked. Reporting "checked" would be a lie. The local driver -- the default -- is unaffected. Wired on BOTH backends in the same change ----------------------------------------- combinedGate gets DenyVolumeCreate and a compile-time VolumeGate guard. This is deliberate and it is not speculative: deny-unattributable-builds shipped in this same release consulting both layers for create/pull/push and only the WSL layer for build, so the rule was inert on the wslc backend while `policy show` reported it active. A new gate method is exactly where that recurs. Verification ------------ Tested at both levels, because one of them is not enough and this release is the reason. deny-unattributable-builds passed every Rules test while being a no-op in the product, since nothing drove the path the bridge takes. So: Rules tests for the rule, Watcher tests for what the bridge installs, and an end-to-end test through RewriteBindsGuarded with the real policy.Watcher. Negative control -- with the route removed: status = 201, want 403 the request reached the engine; a denial must stop at the bridge The permitted cases are asserted too, so the rule cannot have bought safety by refusing ordinary named volumes: a plain volume and a device inside the allowed root both reach the engine and return 201. --- CHANGELOG.md | 35 +++++++ cmd/skrog/wslcgate.go | 24 ++++- docs/policy.md | 48 +++++++--- internal/pipeproxy/rewrite.go | 75 +++++++++++++++ internal/pipeproxy/volumegate_test.go | 110 +++++++++++++++++++++ internal/policy/volume.go | 133 ++++++++++++++++++++++++++ internal/policy/volume_test.go | 126 ++++++++++++++++++++++++ 7 files changed, 535 insertions(+), 16 deletions(-) create mode 100644 internal/pipeproxy/volumegate_test.go create mode 100644 internal/policy/volume.go create mode 100644 internal/policy/volume_test.go diff --git a/CHANGELOG.md b/CHANGELOG.md index 0248d2f..78c4601 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -12,6 +12,41 @@ useful than saying where the real one is. ## [Unreleased] +### Fixed + +The concurrency findings from the pre-0.6.0 review, which were filed but not +fixed in time for it, plus the first of the policy gaps. + +- **A stalled upload could wedge the bridge and permanently disable + idle-stop** ([#435](https://github.com/wslkit/skrog/issues/435)). The + teardown for an abandoned request body closed the engine and then waited + forever — which frees a writer blocked *writing*, and does nothing for one + blocked *reading* a client that went quiet. `ActiveConns` then never dropped, + so `maybeIdleStop` vetoed for the life of the process, silently, and shutdown + hung holding the single-instance lock. A sleeping laptop mid-`docker build` + was enough. +- **A 101 upgrade with a still-streaming body shared one `bufio.Reader` + between two goroutines** ([#436](https://github.com/wslkit/skrog/issues/436)) + — the heap-corruption class of #166, which this package had already fixed + once. The upgrade is now refused in that state: a failed `docker exec` is + visible and retryable, a corrupted heap is neither. +- **A wedged `wslservice` could hang every docker command** + ([#437](https://github.com/wslkit/skrog/issues/437)). The health probe ran + under the reconciler's mutex with a context that never fires. Three parts: + the COM call is bounded, `Engine.Running` gained an error so a *failed* probe + is no longer read as a *stopped engine* (which used to provoke starting an + engine that was already running), and the probe no longer holds the lock. + A panicking COM call is also recovered and reported rather than taking the + supervisor down. +- **`docker volume create` could reach a path `allow-bind-sources` forbids** + ([#419](https://github.com/wslkit/skrog/issues/419)). `POST /volumes/create` + was judged by nothing, and a `local`-driver volume can name a host path + through `-o type=none -o o=bind -o device=...`. The container that mounted it + afterwards carried only the volume's *name*, so nothing downstream caught it + either. Now judged, including `device=/`. See + [docs/policy.md](docs/policy.md) for what this means for third-party volume + drivers. + ## [0.6.0] — 2026-09-18 **The first release not flagged as a pre-release.** Every earlier tag, diff --git a/cmd/skrog/wslcgate.go b/cmd/skrog/wslcgate.go index aedf1a8..262d843 100644 --- a/cmd/skrog/wslcgate.go +++ b/cmd/skrog/wslcgate.go @@ -77,10 +77,30 @@ func (g combinedGate) DenyBuild() (string, bool) { return "", false } +// DenyVolumeCreate consults only policy.yaml (#419). +// +// Unlike create/pull/push/build there is no WSL half: the administrator's +// WSLContainerRegistryAllowlist governs registries, and a volume has none. +// Delegating to one layer is the whole rule here, not an omission. +// +// This method exists at all because of what happened to +// deny-unattributable-builds: combinedGate consulted both layers for +// create/pull/push and only the WSL layer for build, so the rule was inert on +// this backend while `policy show` reported it active. A new gate method is +// exactly where that recurs, so allow-bind-sources gets wired on both backends +// in the same change that adds it. +func (g combinedGate) DenyVolumeCreate(body map[string]any) (string, bool) { + if vg, ok := g.skrog.(pipeproxy.VolumeGate); ok && g.skrog != nil { + return vg.DenyVolumeCreate(body) + } + return "", false +} + // Without these, dropping a method here would not fail the build — combinedGate // would quietly stop satisfying ImageGate and every pull, build and push on // this backend would pass unjudged. var ( - _ pipeproxy.Gate = combinedGate{} - _ pipeproxy.ImageGate = combinedGate{} + _ pipeproxy.Gate = combinedGate{} + _ pipeproxy.ImageGate = combinedGate{} + _ pipeproxy.VolumeGate = combinedGate{} ) diff --git a/docs/policy.md b/docs/policy.md index 6375c6a..b80fce3 100644 --- a/docs/policy.md +++ b/docs/policy.md @@ -142,13 +142,31 @@ would be trivially bypassed. `c:/work` is the same root. **Named volumes are not binds** — `-v myvol:/data` carries only a name at create time, so this rule does not apply to it. -> That is a gap, not just a scope note. A `local`-driver volume *can* name a -> host path — `docker volume create -o type=none -o o=bind -o device=/mnt/c/...` -> — and `POST /volumes/create` is not judged at all, so a volume made that way -> reaches a directory `allow-bind-sources` would have refused. Tracked as -> [#419](https://github.com/wslkit/skrog/issues/419). Until it is closed, read -> this rule as covering `-v :`, not as covering every route -> to a host directory. +> **But a volume that names a host path is judged as one.** A `local`-driver +> volume *can* point at a directory: +> +> ``` +> docker volume create -d local -o type=none -o o=bind -o device=/mnt/c/secrets esc +> ``` +> +> `POST /volumes/create` was not judged at all until +> [#419](https://github.com/wslkit/skrog/issues/419), so that volume — and any +> container later mounting it — reached a directory the rule would have +> refused. The container create that follows carries only the volume's *name*, +> and a name is not a path, so nothing downstream could catch it either. +> +> It is judged now. The `device` is a guest path, so it is mapped back from +> `/mnt//...` to Windows form before being compared against the allowed +> roots. Two consequences worth knowing: +> +> - **A device that is not under a Windows drive is refused** — `device=/`, +> `/etc`, `/var/lib/docker`. They are under no allowed root, and refusing is +> the point of the rule. +> - **A third-party volume driver is refused while this rule is in force**, +> because its options are its own vocabulary and Skrog cannot tell whether +> they name a host path. Saying "checked" would be a lie. The `local` driver +> — the default, and what `docker volume create` uses unless told otherwise — +> is unaffected. **`allow-registries` blocks Docker Hub unless you list it.** Docker's own rule is that the first component of an image reference is a registry only if it @@ -364,13 +382,15 @@ rule applies where. With `deny-unattributable-builds` set, the build endpoints (`/build`, `/session`, `/grpc`) and the other calls that carry no attributable image reference are refused too. -Not judged: `POST /volumes/create`. A `local`-driver volume created with -`-o type=none -o o=bind -o device=` does have a host path, and it is not -checked against `allow-bind-sources` -([#419](https://github.com/wslkit/skrog/issues/419)). `POST /plugins/pull` and -the swarm/service endpoints are only refused when -`deny-unattributable-builds` is on, which is off by default -([#420](https://github.com/wslkit/skrog/issues/420)). +Also judged: `POST /volumes/create`, when `allow-bind-sources` is set — a +`local`-driver volume can name a host path through its driver options, and the +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. 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/rewrite.go b/internal/pipeproxy/rewrite.go index 4e7c67d..e9abe63 100644 --- a/internal/pipeproxy/rewrite.go +++ b/internal/pipeproxy/rewrite.go @@ -200,6 +200,19 @@ func rewriteBinds(client net.Conn, engine io.ReadWriteCloser, audit AuditSink, g } } + if isVolumeCreate(req) && gate != nil { + denied, err := judgeVolumeCreate(req, gate) + switch { + case denied != nil: + observe(audit, reqStart, req, http.StatusForbidden, denied) + trace("DENY %s %s: %v", req.Method, req.URL.Path, denied) + return writeError(client, http.StatusForbidden, denied) + case err != nil: + observe(audit, reqStart, req, http.StatusBadRequest, err) + return writeError(client, http.StatusBadRequest, err) + } + } + if isContainerCreate(req) { denied, err := rewriteCreateBody(req, gate, translate) switch { @@ -442,6 +455,68 @@ func isContainerCreate(req *http.Request) bool { return req.Method == http.MethodPost && containerCreatePath.MatchString(req.URL.Path) } +// volumeCreatePath matches POST /volumes/create (#419). +var volumeCreatePath = regexp.MustCompile(`^(/v[0-9.]+)?/volumes/create$`) + +func isVolumeCreate(req *http.Request) bool { + return req.Method == http.MethodPost && volumeCreatePath.MatchString(req.URL.Path) +} + +// VolumeGate judges volume creation. +// +// Separate from Gate because the body is a different shape and only one rule +// applies -- a volume carries no image, capabilities or namespaces. A gate +// that does not implement it leaves volumes unjudged, which is what every gate +// did before #419. +type VolumeGate interface { + // DenyVolumeCreate judges a POST /volumes/create body. A local-driver + // volume can name a host path through DriverOpts (type=none, o=bind, + // device=...), which allow-bind-sources must reach: the container create + // that follows carries only the volume's name, and a name is not a path. + DenyVolumeCreate(body map[string]any) (reason string, denied bool) +} + +// judgeVolumeCreate reads the body, asks the gate, and restores it either way. +// +// Read-only: unlike a container create there is nothing to rewrite. The device +// is a GUEST path, because dockerd is what will open it -- so it must not be +// translated, and the gate maps it back to Windows form itself for comparison +// against the allowlist. +func judgeVolumeCreate(req *http.Request, gate Gate) (denied, err error) { + vg, ok := gate.(VolumeGate) + if !ok || req.Body == nil { + return nil, nil + } + raw, err := io.ReadAll(req.Body) + req.Body.Close() + if err != nil { + return nil, fmt.Errorf("read volume create body: %w", err) + } + req.Body = io.NopCloser(bytes.NewReader(raw)) + if len(raw) == 0 { + return nil, nil + } + + dec := json.NewDecoder(bytes.NewReader(raw)) + dec.UseNumber() + var body map[string]any + if err := dec.Decode(&body); err != nil { + // Not JSON we understand. Pass it to the engine, which will reject it + // far more precisely than a guess here would. + return nil, nil + } + // Same reasoning as container create: a body that spells a guarded field + // two ways would have the gate judge one and the daemon act on the other. + if field, bad := apibody.Ambiguous(raw); bad { + return errors.New("request body spells " + field + + " more than one way; refusing rather than guessing which the engine would use"), nil + } + if reason, no := vg.DenyVolumeCreate(body); no { + return errors.New(reason), nil + } + return nil, nil +} + // isHijack reports whether the connection stops being HTTP after this response. // // Only 101 Switching Protocols qualifies. An earlier version also treated the diff --git a/internal/pipeproxy/volumegate_test.go b/internal/pipeproxy/volumegate_test.go new file mode 100644 index 0000000..c53dd7f --- /dev/null +++ b/internal/pipeproxy/volumegate_test.go @@ -0,0 +1,110 @@ +package pipeproxy_test + +import ( + "bufio" + "io" + "net" + "net/http" + "os" + "path/filepath" + "strconv" + "testing" + "time" + + "github.com/wslkit/skrog/internal/pipeproxy" + "github.com/wslkit/skrog/internal/policy" +) + +// postVolumeCreate drives one /volumes/create through the real bridge and +// reports both what the client saw and whether the engine was ever reached. +func postVolumeCreate(t *testing.T, gate pipeproxy.Gate, body string) (*http.Response, bool) { + t.Helper() + client, bridgeClient := net.Pipe() + engineSide, bridgeEngine := net.Pipe() + t.Cleanup(func() { client.Close(); engineSide.Close() }) + + go pipeproxy.RewriteBindsGuarded(nil, gate)(bridgeClient, bridgeEngine) + + reached := make(chan struct{}, 1) + go func() { + br := bufio.NewReader(engineSide) + for { + line, err := br.ReadString('\n') + if err != nil { + return + } + if line == "\r\n" { + break + } + } + reached <- struct{}{} + // Answer FIRST. Draining before replying would block on this + // unbuffered pipe until EOF, and the reply would never be written -- + // which reads as "the bridge swallowed it" rather than as a bug here. + engineSide.Write([]byte("HTTP/1.1 201 Created\r\nContent-Length: 0\r\n\r\n")) + io.Copy(io.Discard, engineSide) + }() + + req := "POST /v1.45/volumes/create HTTP/1.1\r\nHost: d\r\nContent-Type: application/json\r\n" + + "Content-Length: " + strconv.Itoa(len(body)) + "\r\n\r\n" + body + go client.Write([]byte(req)) + + client.SetReadDeadline(time.Now().Add(10 * time.Second)) + resp, err := http.ReadResponse(bufio.NewReader(client), nil) + if err != nil { + t.Fatalf("reading response: %v", err) + } + select { + case <-reached: + return resp, true + default: + return resp, false + } +} + +// End to end through the bridge with the REAL policy gate (#419). +// +// The policy-level tests are not enough on their own, and this release is the +// reason: deny-unattributable-builds passed every Rules test while being a +// no-op in the product, because nothing drove the path the bridge takes. +func TestVolumeCreateIsJudgedByTheBridge(t *testing.T) { + dir := t.TempDir() + t.Setenv(policy.MachineDirEnv, t.TempDir()) + rules := "allow-bind-sources:\n - C:\\work\n" + if err := os.WriteFile(filepath.Join(dir, "policy.yaml"), []byte(rules), 0o644); err != nil { + t.Fatal(err) + } + gate := policy.NewWatcher(dir) + + t.Run("a device outside the allowlist is refused at the bridge", func(t *testing.T) { + resp, reached := postVolumeCreate(t, gate, + `{"Name":"esc","Driver":"local","DriverOpts":{"type":"none","o":"bind","device":"/mnt/c/secrets"}}`) + if resp.StatusCode != http.StatusForbidden { + t.Errorf("status = %d, want 403", resp.StatusCode) + } + if reached { + t.Error("the request reached the engine; a denial must stop at the bridge") + } + }) + + t.Run("an ordinary named volume passes through", func(t *testing.T) { + resp, reached := postVolumeCreate(t, gate, `{"Name":"data"}`) + if resp.StatusCode != http.StatusCreated { + t.Errorf("status = %d, want 201", resp.StatusCode) + } + if !reached { + t.Error("a permitted volume never reached the engine") + } + }) + + t.Run("a device inside the allowlist passes through", func(t *testing.T) { + resp, reached := postVolumeCreate(t, gate, + `{"Name":"ok","Driver":"local","DriverOpts":{"type":"none","o":"bind","device":"/mnt/c/work/proj"}}`) + if resp.StatusCode != http.StatusCreated { + t.Errorf("status = %d, want 201", resp.StatusCode) + } + if !reached { + t.Error("a permitted volume never reached the engine") + } + }) +} diff --git a/internal/policy/volume.go b/internal/policy/volume.go new file mode 100644 index 0000000..1e18deb --- /dev/null +++ b/internal/policy/volume.go @@ -0,0 +1,133 @@ +package policy + +import ( + "fmt" + "strings" + + "github.com/wslkit/skrog/internal/apibody" +) + +// Volume-create admission control (#419). +// +// docs/policy.md said, for a long time: "Named volumes are not binds -- +// `-v myvol:/data` has no host path to restrict and is never denied by this +// rule." True of the common case, and false in general. +// +// A local-driver volume CAN name a host path: +// +// docker volume create -d local -o type=none -o o=bind -o device=/mnt/c/secrets esc +// docker run -v esc:/out ubuntu cat /out/... +// +// POST /volumes/create was judged by nothing, and the container create that +// follows carries only the volume's NAME -- which bindSources deliberately +// skips, because a name is not a path. So with allow-bind-sources: [C:\work] +// in force, those two commands read C:\secrets. The rule was not weak here; it +// was absent. +// +// device=/ is the same trick against the whole guest filesystem. + +// DenyVolumeCreate judges a POST /volumes/create body. +// +// Only allow-bind-sources applies: a volume carries no image, no capabilities +// and no namespaces, so the other rules have nothing to look at. +func (r Rules) DenyVolumeCreate(body map[string]any) (reason string, denied bool) { + if len(r.AllowBindSources) == 0 { + return "", false + } + opts, ok := apibody.Map(body, "DriverOpts") + if !ok { + return "", false + } + device := strings.TrimSpace(apibody.String(opts, "device")) + if device == "" { + // No host path named, so nothing for this rule to restrict -- which is + // the ordinary named volume the docs describe, living in the engine's + // own storage. + return "", false + } + + // The driver is what decides whether "device" means a host path at all. + // Empty means local, which is dockerd's own default. + if d := strings.ToLower(strings.TrimSpace(apibody.String(body, "Driver"))); d != "" && d != "local" { + // A third-party driver's options are its own vocabulary and this rule + // cannot read them. Say so rather than pretend to have checked. + return fmt.Sprintf( + "policy does not allow volume driver %q while allow-bind-sources is in force: "+ + "its options cannot be checked against the allowed roots (allowed: %s)", + d, strings.Join(r.AllowBindSources, ", ")), true + } + + win, ok := guestPathToWindows(device) + if !ok { + // A guest path that is not under /mnt/ is not under any + // Windows root either, so it cannot be allowed by a Windows-path + // allowlist. This is the device=/ case, and refusing is the whole + // point of the rule. + return fmt.Sprintf( + "policy does not allow a volume on the guest path %s "+ + "(bind sources allowed: %s)", + device, strings.Join(r.AllowBindSources, ", ")), true + } + if !underAny(win, r.AllowBindSources) { + return fmt.Sprintf( + "policy does not allow bind mounts from %s (allowed: %s)", + device, strings.Join(r.AllowBindSources, ", ")), true + } + return "", false +} + +// guestPathToWindows maps /mnt//... back to :/..., so a device +// can be compared against an allowlist written in Windows terms. +// +// This is the inverse of winpath.ToWSL, which is what the bridge already +// applies to bind sources on their way to the engine. It is done here rather +// than by importing winpath because only the /mnt form matters: anything else +// is a guest-only path, and reporting THAT as untranslatable is a result the +// caller needs, not a failure. +// +// ok is false for a path that names no Windows drive. +func guestPathToWindows(p string) (string, bool) { + s := strings.ToLower(strings.ReplaceAll(strings.TrimSpace(p), `\`, "/")) + // Already in Windows form (c:/x). Accept it: a caller may write it that + // way even though dockerd would not. + if len(s) >= 2 && s[1] == ':' { + return s, true + } + rest, found := strings.CutPrefix(s, "/mnt/") + if !found || rest == "" { + return "", false + } + // /mnt/c -> c:/ /mnt/c/x -> c:/x + drive := rest[:1] + if drive < "a" || drive > "z" { + return "", false + } + switch { + case len(rest) == 1: + return drive + ":/", true + case rest[1] == '/': + return drive + ":/" + rest[2:], true + default: + // /mnt/wsl/... and friends: a real guest path, not a drive. + return "", false + } +} + +// DenyVolumeCreate on the Watcher, which is what the bridge installs as its +// gate -- Rules is not. +// +// Spelled out because this release already shipped a rule that existed only on +// Rules: deny-unattributable-builds was documented, reported active by `policy +// show`, and a no-op on every backend, because Watcher.DenyBuild returned a +// hardcoded allow and every test called Rules.DenyBuild. The compile-time +// interface guard did not catch it either, since the method existed. +// +// Carries the same unreadable-file refusal as DenyCreate (#254): a rule file +// the operator wrote and we cannot parse must not silently allow the requests +// it was written to judge. +func (w *Watcher) DenyVolumeCreate(body map[string]any) (string, bool) { + if err := w.Unavailable(); err != nil { + return unreadableRules(err), true + } + return w.Rules().DenyVolumeCreate(body) +} diff --git a/internal/policy/volume_test.go b/internal/policy/volume_test.go new file mode 100644 index 0000000..82950a4 --- /dev/null +++ b/internal/policy/volume_test.go @@ -0,0 +1,126 @@ +package policy + +import ( + "path/filepath" + "strings" + "testing" +) + +func volBody(driver, device string) map[string]any { + b := map[string]any{"Name": "v"} + if driver != "" { + b["Driver"] = driver + } + if device != "" { + b["DriverOpts"] = map[string]any{"type": "none", "o": "bind", "device": device} + } + return b +} + +// The exploit the rule exists to stop (#419): a local volume naming a host +// path outside the allowlist. +func TestVolumeCreateRefusesADeviceOutsideTheAllowlist(t *testing.T) { + r := Rules{AllowBindSources: []string{`C:\work`}} + reason, denied := r.DenyVolumeCreate(volBody("local", "/mnt/c/secrets")) + if !denied { + t.Fatal(`a local volume reached C:\secrets with allow-bind-sources: [C:\work]`) + } + for _, want := range []string{"/mnt/c/secrets", `C:\work`} { + if !strings.Contains(reason, want) { + t.Errorf("reason does not mention %q: %q", want, reason) + } + } +} + +// device=/ is the same trick against the whole guest filesystem, and it is not +// under any Windows root, so it must be refused rather than silently allowed +// for being untranslatable. +func TestVolumeCreateRefusesAGuestPathThatIsNotADrive(t *testing.T) { + r := Rules{AllowBindSources: []string{`C:\work`}} + for _, device := range []string{"/", "/etc", "/var/lib/docker", "/mnt/wsl"} { + if _, denied := r.DenyVolumeCreate(volBody("local", device)); !denied { + t.Errorf("device=%q was allowed; it is under no Windows root", device) + } + } +} + +// A device inside an allowed root is fine — the rule restricts, it does not +// forbid the feature. +func TestVolumeCreateAllowsADeviceInsideTheAllowlist(t *testing.T) { + r := Rules{AllowBindSources: []string{`C:\work`}} + for _, device := range []string{"/mnt/c/work", "/mnt/c/work/proj", `C:\work\proj`} { + if reason, denied := r.DenyVolumeCreate(volBody("local", device)); denied { + t.Errorf(`device=%q inside C:\work was refused: %s`, device, reason) + } + } +} + +// The ordinary named volume — no device at all — must stay allowed, or the +// rule would break every compose stack on a machine with an allowlist. +func TestVolumeCreateAllowsAnOrdinaryNamedVolume(t *testing.T) { + r := Rules{AllowBindSources: []string{`C:\work`}} + if _, denied := r.DenyVolumeCreate(volBody("local", "")); denied { + t.Error("a plain named volume was refused") + } + if _, denied := r.DenyVolumeCreate(map[string]any{"Name": "v"}); denied { + t.Error("a volume with no DriverOpts at all was refused") + } +} + +// No allowlist, no rule. Refusing volumes on a machine that has not restricted +// bind sources would close a hole that is not open. +func TestVolumeCreateIsInertWithoutAnAllowlist(t *testing.T) { + if _, denied := (Rules{}).DenyVolumeCreate(volBody("local", "/mnt/c/secrets")); denied { + t.Error("a volume was refused with no allow-bind-sources set") + } +} + +// A third-party driver's options are its own vocabulary, so "checked" would be +// a lie. Refuse and say why. +func TestVolumeCreateRefusesAnUncheckableDriver(t *testing.T) { + r := Rules{AllowBindSources: []string{`C:\work`}} + reason, denied := r.DenyVolumeCreate(volBody("some-vendor-driver", "/mnt/c/work")) + if !denied { + t.Fatal("an unknown driver's options were treated as checked") + } + if !strings.Contains(reason, "some-vendor-driver") { + t.Errorf("reason does not name the driver: %q", reason) + } +} + +// Driver omitted means local, which is dockerd's own default — so the rule has +// to apply to it, or `-o device=` without `-d local` walks straight through. +func TestVolumeCreateTreatsAnOmittedDriverAsLocal(t *testing.T) { + r := Rules{AllowBindSources: []string{`C:\work`}} + if _, denied := r.DenyVolumeCreate(volBody("", "/mnt/c/secrets")); !denied { + t.Error("a volume with no Driver field bypassed the rule") + } +} + +// The Watcher is what the bridge installs as its gate, and Rules is not. +// deny-unattributable-builds shipped as a no-op in this same release because +// every test called Rules and nothing called the method the product reaches. +func TestWatcherDenyVolumeCreateConsultsTheRules(t *testing.T) { + dir := t.TempDir() + t.Setenv(MachineDirEnv, t.TempDir()) + write(t, filepath.Join(dir, FileName), "allow-bind-sources:\n - C:\\work\n") + + w := NewWatcher(dir) + if _, denied := w.DenyVolumeCreate(volBody("local", "/mnt/c/secrets")); !denied { + t.Fatal("Watcher.DenyVolumeCreate allowed an out-of-tree device; the gate is a no-op") + } + if _, denied := w.DenyVolumeCreate(volBody("local", "/mnt/c/work/x")); denied { + t.Error("Watcher.DenyVolumeCreate refused a device inside the allowed root") + } +} + +// An unreadable rule file refuses, like every other gate method (#254). +func TestWatcherDenyVolumeCreateFailsClosedOnABrokenFile(t *testing.T) { + machineDir := t.TempDir() + t.Setenv(MachineDirEnv, machineDir) + write(t, filepath.Join(machineDir, FileName), "deny-priviledged: true\n") + + if _, denied := NewWatcher(t.TempDir()).DenyVolumeCreate(volBody("local", "/mnt/c/work")); !denied { + t.Error("a broken machine policy allowed a volume unjudged") + } +}