fix: judge POST /volumes/create against allow-bind-sources

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,

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

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 <hostpath>:<target>`, 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/<drive>/...` 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=<path>` 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

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

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")
+		}
+	})
+}