From a6af6de970a1a686bf3508b77061522bbacf1d18 Mon Sep 17 00:00:00 2001 From: mintaka Date: Sat, 5 Sep 2026 20:27:51 -0400 Subject: [PATCH 1/3] feat(microvm): virtio-fs mount-ns isolation + volume-quota verification (RIG-2497) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit V6 of the frozen microVM Runner backend design (record § Plan > V6): per-session virtio-fs isolation proven under real KVM, plus the D7 verify-never-assign volume-quota preflight. Executes against the frozen record (`docs/designs/infra/runtime/compass-elastic-session-runtime/microvm-runner.md`, D7 + Approach (d)). ## What lands - **Quota verification (D7: verify, never assign).** `VolumeQuota{Bytes,Inodes}` (the expected bound), `verifyVolumeQuota(path, want, probe) → (QuotaReading, error)`, and `MicroVMConfig.QuotaRequired` (appended, additive). The Runner never *assigns* quota (that needs `CAP_SYS_ADMIN` it lacks); it only *verifies* an operator-provisioned project quota is active — a rootless, read-only check wired into `verifyMicroVMSupport` as step 5. When `QuotaRequired` is set (multi-tenant profile) an absent quota fails startup naming the volume + fix; unset (Dogfood single-tenant) it logs utilization and proceeds. - **The rootless quota read is `statfs(2)`-derived, not `quotactl`.** `quotactl(Q_XGETQUOTA, PRJQUOTA)` EPERMs rootless (the kernel gates a non-self quota id on `CAP_SYS_ADMIN`); `FS_IOC_FSGETXATTR` yields only the project-id label, not whether enforcement is live. On XFS and ext4 the kernel rewrites a project-quota'd directory's `statfs` totals to the project's limit+usage, so path-totals < mount-root-totals *is* the kernel reporting an enforced quota — answered unprivileged, with the utilization in the same call. - **The isolation-proving KVM suite.** Real guests booted under KVM: traversal-confined (dot-dot, symlink-to-host, absolute-path, deep-dot-dot — all blocked), cross-session-unreachable (two live guests; A cannot reach B's volume by any vector), and host-ownership parity (guest-authored files land host-side `uid:gid` matching podman's `--userns=keep-id` target). The real ENOSPC/EDQUOT-in-guest leg is honestly gated (`requireQuotaFS`): it skips where no quota'd filesystem + root exist (a `COMPASS_REQUIRE_QUOTA_FS=1` hard-fail switch for the CI/managed profile), never fake-passing. The verification *logic* is proven by the hermetic unit tests, which run everywhere. ## Load-bearing fix: guest writes to /workspace were broken The parity test surfaced a real defect masked because no prior test wrote to the volume: under `--sandbox=namespace` alone, rootless virtiofsd could not become namespace-root (`Couldn't set the process uid as root: -1`), so it could not chown a newly created inode to the requesting guest id — **every** guest create on the share failed `EINVAL`. The share was effectively read-only to the guest. The fix is the record's named mechanism (§(d): "virtiofsd does its own uid/gid translation via that userns"): `--uid-map`/`--gid-map` map a subordinate id to namespace-root (so the daemon can chown) and the agent id to the invoking host `(uid, gid)`. gid maps to the host *gid*, not the uid — the guest agent runs `uid==gid`, but a host user's gid differs (e.g. `1000:100`), and collapsing gid onto uid is exactly the parity break the test detects. Rootless throughout (a `/etc/subuid` subordinate id, no `CAP_SYS_ADMIN`); `--uid-map` over `--translate-uid` also keeps POSIX ACLs available on the share. `BootConfig.AgentUID` carries the guest uid (zero disables the mapping, preserving the V2a spike harness); `Create` threads `spec.UID` through. ## Verification All gates green on a real-KVM box (Intel, nested virt): build+vet (default + `-tags microvm`, darwin cross-build of the non-linux stub), hermetic `-race` suite, the KVM isolation suite (real boots, `COMPASS_REQUIRE_MICROVM=1`), full tagged regression (boot/contract/egress/Q-budget unregressed), golangci-lint (tagged + untagged) 0 issues, nilaway clean (default + microvm). No OTel metric registration (V7 owns the metric set — V6 only exposes `QuotaReading.UsedRatio()`), no teardown/reap (V7), no benchmark (V8). `go/internal/vfs` untouched. `ContainerRuntime` interface unchanged. Spec-impact: none. Refs RIG-2497 Co-authored-by: Matt Wilkinson --- go/internal/runtime/microvm.go | 9 + go/internal/runtime/microvm/config.go | 14 +- go/internal/runtime/microvm/launch.go | 73 ++- .../runtime/microvm_isolation_microvm_test.go | 509 ++++++++++++++++++ go/internal/runtime/microvm_lifecycle.go | 12 +- go/internal/runtime/microvm_lifecycle_test.go | 8 +- go/internal/runtime/microvm_preflight.go | 48 +- go/internal/runtime/microvm_preflight_test.go | 146 +++-- go/internal/runtime/microvm_quota.go | 167 ++++++ go/internal/runtime/microvm_quota_linux.go | 112 ++++ go/internal/runtime/microvm_quota_test.go | 311 +++++++++++ .../runtime/microvm_quota_unsupported.go | 23 + 12 files changed, 1390 insertions(+), 42 deletions(-) create mode 100644 go/internal/runtime/microvm_isolation_microvm_test.go create mode 100644 go/internal/runtime/microvm_quota.go create mode 100644 go/internal/runtime/microvm_quota_linux.go create mode 100644 go/internal/runtime/microvm_quota_test.go create mode 100644 go/internal/runtime/microvm_quota_unsupported.go diff --git a/go/internal/runtime/microvm.go b/go/internal/runtime/microvm.go index e845c247..2e271e76 100644 --- a/go/internal/runtime/microvm.go +++ b/go/internal/runtime/microvm.go @@ -55,6 +55,15 @@ type MicroVMConfig struct { // DefaultMemoryMB is the RAM each session guest boots with, in MiB // (hotplug-grown later per D5). Zero leaves it to the VMM's own default. DefaultMemoryMB int + // QuotaRequired selects the D7 quota posture. When set (the multi-tenant + // profile), Runner startup verifies an operator-provisioned project quota is + // active on the session-volume filesystem and fails if absent (D7 + // verify-never-assign: the Runner reads the bound, it never assigns one — + // assignment needs CAP_SYS_ADMIN it lacks). Unset (Dogfood single-tenant): + // no host-enforced quota is required, and the observed utilization is logged + // only. Appended, never reordered: the field is additive to a config + // operators already construct positionally in tests. + QuotaRequired bool } // BackendConfig selects and configures the container runtime backend. Backend diff --git a/go/internal/runtime/microvm/config.go b/go/internal/runtime/microvm/config.go index e76ee0c9..d73c6f54 100644 --- a/go/internal/runtime/microvm/config.go +++ b/go/internal/runtime/microvm/config.go @@ -32,9 +32,17 @@ type BootConfig struct { FSTag string // virtio-fs tag ("workspace") FSSocket string // virtiofsd --socket-path the VMM attaches via --fs FSSharedDir string // virtiofsd --shared-dir: the host tree exported as FSTag→/workspace (per-session checkout dir in V2b; a throwaway temp dir in the V2a spike) - CPUs int - MemoryMB int // always launched with shared=on ((c)) - Net NetConfig + // AgentUID is the in-guest uid the workload runs as (uid==gid, guestd's + // linuxCredential). virtiofsd maps it back to the INVOKING host user's + // (uid,gid) inside its user namespace, so a file the guest agent writes on + // the share lands host-side owned by the invoking user — the same host + // ownership podman's `--userns=keep-id:uid=N,gid=N` produces (record §(d), + // host-ownership parity). Zero disables the mapping (the V2a spike harness, + // which shares a throwaway dir and asserts nothing about ownership). + AgentUID uint32 + CPUs int + MemoryMB int // always launched with shared=on ((c)) + Net NetConfig } // GatewaySocketPath is the host-side AF_UNIX listener path a guest reaches by diff --git a/go/internal/runtime/microvm/launch.go b/go/internal/runtime/microvm/launch.go index 102f9d9a..69c4fc56 100644 --- a/go/internal/runtime/microvm/launch.go +++ b/go/internal/runtime/microvm/launch.go @@ -161,9 +161,11 @@ func launch(ctx context.Context, cfg BootConfig, opts launchOptions) (_ *VM, err logPath: filepath.Join(dir, "virtiofsd.log"), //nolint:gosec // G204: the microVM harness seam — virtiofsdPath is LookPath-resolved and the argv is harness-built from BootConfig, neither user-controlled cmd: exec.CommandContext(ctx, virtiofsdPath, - "--socket-path="+cfg.FSSocket, - "--shared-dir="+cfg.FSSharedDir, - "--sandbox=namespace"), + append([]string{ + "--socket-path=" + cfg.FSSocket, + "--shared-dir=" + cfg.FSSharedDir, + "--sandbox=namespace", + }, virtiofsdIDMapArgs(cfg.AgentUID)...)...), } if startErr := startChild(vm.virtiofsd); startErr != nil { return nil, fmt.Errorf("microvm: starting virtiofsd: %w", startErr) @@ -238,6 +240,71 @@ func launch(ctx context.Context, cfg BootConfig, opts launchOptions) (_ *VM, err return vm, nil } +// virtiofsdIDMapArgs builds the virtiofsd uid/gid mapping that gives the shared +// volume the SAME host-side ownership podman's `--userns=keep-id:uid=N,gid=N` +// produces (record §(d): "virtiofsd does its own uid/gid translation via that +// userns (subuid/subgid + newuidmap) … the target is the same host-side +// ownership on the session volume, so files stay identical between backends"). +// Empty when agentUID is zero (the V2a spike harness, which asserts nothing +// about ownership on its throwaway share). +// +// --uid-map/--gid-map (not --translate-uid/--translate-gid): both reach the same +// ownership, but the map flags are the record's NAMED mechanism — the mapping is +// performed by the user namespace virtiofsd is placed into by +// --sandbox=namespace, rather than internally by the daemon. That distinction is +// load-bearing here for two reasons beyond fidelity to the record: +// +// 1. Rootless, --sandbox=namespace alone leaves virtiofsd unable to become +// namespace-root ("Couldn't set the process uid as root: -1"), and a +// passthrough daemon that is not root in its own userns cannot chown a newly +// created inode to the requesting guest id — so EVERY guest create on the +// share failed EINVAL. Mapping an id to namespace-uid 0 makes the daemon +// root inside the namespace, which is what makes guest writes work at all. +// 2. --translate-uid is documented as incompatible with +// `--posix-acl=always|auto`, so it would foreclose POSIX ACLs on the share. +// +// The two mapped ranges, both one id wide: +// - :0::1: — an id from the invoking user's /etc/subuid range +// becomes namespace-root, so the daemon can chown as above. It is a +// subordinate id the invoking user already owns, so no capability is needed +// (newuidmap is setuid and honors /etc/subuid). +// - :::1: — the in-guest agent id maps to the invoking +// host user, which is the parity target itself. +// +// gid mirrors uid, EXCEPT that the host side is the invoking user's real gid, +// not its uid: the guest agent runs uid==gid==agentUID (guestd linuxCredential) +// while a host user's gid is routinely different (e.g. 1000:100). Collapsing gid +// onto uid here is precisely the parity break the V6 parity test detects. +func virtiofsdIDMapArgs(agentUID uint32) []string { + if agentUID == 0 { + return nil + } + hostUID := os.Getuid() + hostGID := os.Getgid() + agent := strconv.FormatUint(uint64(agentUID), 10) + return []string{ + "--uid-map", idMapSpec("0", strconv.Itoa(subordinateIDBase)), + "--uid-map", idMapSpec(agent, strconv.Itoa(hostUID)), + "--gid-map", idMapSpec("0", strconv.Itoa(subordinateIDBase)), + "--gid-map", idMapSpec(agent, strconv.Itoa(hostGID)), + } +} + +// idMapSpec renders one virtiofsd --uid-map/--gid-map range in its +// `::::` form, always one id wide. +func idMapSpec(namespaceID, hostID string) string { + return ":" + namespaceID + ":" + hostID + ":1:" +} + +// subordinateIDBase is the host subordinate uid/gid mapped to namespace id 0 so +// virtiofsd can act as root INSIDE its user namespace (and therefore chown +// guest-created inodes). It is the conventional first entry of a rootless +// /etc/subuid + /etc/subgid range — the same 100000 base shadow-utils allocates +// and podman's rootless userns consumes — so it needs no capability, only that +// the invoking user has a subordinate range at all (which rootless podman on +// this host already requires, podman.go:22-24). +const subordinateIDBase = 100000 + // vmmArgs builds the cloud-hypervisor argv exactly per the record (lines // 542-547), dropping --fs/--vsock under the net-only smoke. Launch appends to // the guest cmdline (per BootConfig.Cmdline's contract): diff --git a/go/internal/runtime/microvm_isolation_microvm_test.go b/go/internal/runtime/microvm_isolation_microvm_test.go new file mode 100644 index 00000000..b4fad015 --- /dev/null +++ b/go/internal/runtime/microvm_isolation_microvm_test.go @@ -0,0 +1,509 @@ +//go:build microvm && unix + +package runtime + +// The KVM-gated virtio-fs ISOLATION suite (record §Plan V6 Test cycle). This is +// the slice that PROVES inter-tenant isolation rather than exercising a happy +// path, so every assertion here drives a real MicroVMRuntime through +// Create→Start and then execs inside the live guest to ATTEMPT an escape, +// asserting confinement from both sides of the boundary (what the guest can +// name, and what actually landed on the host). +// +// Every test calls microvmtest.Require(t) FIRST, mirroring +// microvm_lifecycle_microvm_test.go: on a KVM-less box it SKIPS, and under +// COMPASS_REQUIRE_MICROVM=1 that skip becomes a hard failure — so a green run +// proves the suite really booted guests. +// +// The three legs, and what each actually proves: +// +// 1. Path traversal (TestMicroVMVolumeTraversalConfined) — the guest attempts +// to read and write outside its volume via `..`, an absolute host path, and +// a symlink planted inside the volume pointing at a host path outside it. +// Confinement is asserted on BOTH sides: the guest cannot read the outside +// canary's content, and the host-side outside tree is byte-for-byte +// unchanged afterwards. The host-side half is the load-bearing one — a guest +// read failing could be a missing file, but an unchanged host tree after a +// write attempt is confinement. +// 2. Cross-session unreachability (TestMicroVMCrossSessionVolumeUnreachable) — +// two sessions boot with distinct volumes; guest A cannot read B's secret by +// any path, and nothing A writes appears in B's host-side volume. +// 3. Host-ownership parity (TestMicroVMHostOwnershipParity) — a file the guest +// agent creates on the shared volume must land with the SAME host-side +// (uid,gid) the podman `--userns=keep-id:uid=N,gid=N` path produces: the +// INVOKING host user, not the in-guest agent id (podman.go createArgs). +// This is the test that decided whether launch.go needed virtiofsd uid/gid +// translation (record §(d)) — see parityTargetUID/GID below. +// +// The quota-enforcement-in-guest leg is gated separately and skips honestly; see +// TestMicroVMVolumeQuotaEnforcedInGuest. + +import ( + "fmt" + "os" + "path/filepath" + "strconv" + "strings" + "syscall" + "testing" + + "github.com/RigelBuild/compass/go/internal/agentuid" + "github.com/RigelBuild/compass/go/internal/microvmtest" +) + +// outsideCanaryBody is the content planted in a host file OUTSIDE the session +// volume. A guest exec that ever prints this string has escaped the volume +// subtree, so it is a distinctive sentinel rather than a generic word. +const outsideCanaryBody = "HOST-ONLY-CANARY-8f3ac1d0-must-never-be-readable-from-a-guest" + +// isolationSession boots one session against a fresh volume dir and returns the +// runtime, its id, and the host-side volume path. Teardown is registered so a +// failed assertion still tears the VM down. The volume is a t.TempDir() child so +// it is removed with the test; the SHORT runroot comes from e2eConfig (the +// AF_UNIX sun_path budget, microvm_lifecycle_microvm_test.go). +func isolationSession(t *testing.T, env microvmtest.Env, name string) (*MicroVMRuntime, ContainerID, string) { + t.Helper() + m := NewMicroVMRuntime(e2eConfig(t, env)) + volume := filepath.Join(t.TempDir(), "volume") + if err := os.MkdirAll(volume, 0o700); err != nil { + t.Fatalf("creating session volume %s: %v", volume, err) + } + id, err := m.Create(t.Context(), ContainerSpec{ + Name: name, + UID: agentuid.AgentUID, + Mounts: []Mount{{HostPath: volume, ContainerPath: workspaceMountPath}}, + }) + if err != nil { + t.Fatalf("Create(%s): %v", name, err) + } + t.Cleanup(func() { + if err := m.Remove(t.Context(), id); err != nil { + t.Errorf("Remove(%s): %v", name, err) + } + }) + if err := m.Start(t.Context(), id); err != nil { + t.Fatalf("Start(%s): %v", name, err) + } + return m, id, volume +} + +// guestSh runs a shell snippet in the guest as the agent uid and returns its +// combined stdout+stderr and exit code. A transport/refusal error is fatal; a +// NON-ZERO EXIT IS NOT — a denied escape attempt is expected to exit non-zero, +// and that is the outcome under test (mirrors rowExecExitCodes' posture). +func guestSh(t *testing.T, m *MicroVMRuntime, id ContainerID, script string) (string, int) { + t.Helper() + out, err := m.Exec(t.Context(), id, + NewExecSpec("sh", "-s").WithStdin(script).AsUser("1000")) + if err != nil { + t.Fatalf("guest exec failed at the transport/refusal layer (not the escape itself): %v", err) + } + return out.Stdout + out.Stderr, out.ExitCode +} + +// TestMicroVMVolumeTraversalConfined is the path-traversal leg: the guest tries +// to escape /workspace three ways and is confined every time, proven from both +// sides of the boundary. +func TestMicroVMVolumeTraversalConfined(t *testing.T) { + env := microvmtest.Require(t) + m, id, volume := isolationSession(t, env, "iso-traversal") + + // The host tree OUTSIDE the volume: a sibling of the volume dir, so a + // `/workspace/..` that actually escaped the share would land right in it. + outside := filepath.Join(filepath.Dir(volume), "outside") + if err := os.MkdirAll(outside, 0o700); err != nil { + t.Fatalf("creating outside dir: %v", err) + } + canary := filepath.Join(outside, "canary.txt") + if err := os.WriteFile(canary, []byte(outsideCanaryBody), 0o600); err != nil { + t.Fatalf("planting host canary: %v", err) + } + + // A symlink INSIDE the volume pointing at the outside host path. This is the + // sharpest probe: virtio-fs passes the link through verbatim, so if the + // guest could resolve it against the host's namespace the escape would + // succeed. It cannot — the target is resolved inside the guest's own mount + // namespace, where that path does not exist. + if err := os.Symlink(canary, filepath.Join(volume, "escape-link")); err != nil { + t.Fatalf("planting escape symlink: %v", err) + } + // A symlink to the outside DIRECTORY too, so a traversal through a link + // (rather than a direct read of one) is covered. + if err := os.Symlink(outside, filepath.Join(volume, "escape-dir")); err != nil { + t.Fatalf("planting escape dir symlink: %v", err) + } + + // Read attempts. Each prints nothing on success-of-confinement; the + // assertion is that the canary body never appears in ANY of them. + reads := map[string]string{ + "dot-dot traversal": "cat /workspace/../canary.txt /workspace/../outside/canary.txt", + "absolute host path": "cat " + canary, + "symlink to host file": "cat /workspace/escape-link", + "symlink to host dir": "cat /workspace/escape-dir/canary.txt", + "deep dot-dot to fs root": "cat /workspace/../../../../../.." + canary, + } + for name, script := range reads { + t.Run("read: "+name, func(t *testing.T) { + out, code := guestSh(t, m, id, script) + if strings.Contains(out, outsideCanaryBody) { + t.Fatalf("guest READ a host file outside its volume via %s — VOLUME ESCAPE.\noutput: %q", name, out) + } + if code == 0 { + t.Errorf("escape attempt %q exited 0 (output %q); a confined read must fail", name, out) + } + t.Logf("confined: %s -> exit %d, %q", name, code, strings.TrimSpace(out)) + }) + } + + // Write attempts. The guest's own root filesystem is writable (an overlay), + // so a `/workspace/../pwned` may well succeed INSIDE the guest — that proves + // nothing either way. The real question is whether any byte landed on the + // HOST outside the volume, which the snapshot comparison below answers. + before := snapshotTree(t, outside) + writes := []string{ + "echo pwned > /workspace/../pwned.txt", + "echo pwned > /workspace/../outside/pwned.txt", + "echo pwned > " + filepath.Join(outside, "pwned-abs.txt"), + "echo pwned > /workspace/escape-dir/pwned-link.txt", + "echo overwritten > /workspace/escape-link", + "rm -f " + canary, + "rm -rf " + outside, + } + for _, script := range writes { + out, code := guestSh(t, m, id, script) + t.Logf("write attempt %q -> exit %d, %q", script, code, strings.TrimSpace(out)) + } + after := snapshotTree(t, outside) + if before != after { + t.Fatalf("the host tree OUTSIDE the session volume changed after guest write attempts — VOLUME ESCAPE.\nbefore: %s\nafter: %s", before, after) + } + // And the canary's content specifically: an in-place overwrite through the + // symlink would keep the tree shape identical while corrupting the file. + body, err := os.ReadFile(canary) + if err != nil { + t.Fatalf("reading the host canary after the guest write attempts: %v", err) + } + if string(body) != outsideCanaryBody { + t.Fatalf("the host canary was REWRITTEN by the guest (now %q) — VOLUME ESCAPE", string(body)) + } + t.Logf("host tree outside the volume unchanged after %d write attempts: %s", len(writes), after) +} + +// TestMicroVMCrossSessionVolumeUnreachable is the multi-tenant leg: two live +// sessions, and guest A cannot reach guest B's volume by any path, nor write +// into it. This is the assertion that makes "another tenant's volume is not +// merely unreadable but unnameable" (record §(d)) a tested property. +func TestMicroVMCrossSessionVolumeUnreachable(t *testing.T) { + env := microvmtest.Require(t) + const tenantBSecret = "TENANT-B-SECRET-4b91e7c2-must-never-be-readable-from-tenant-A" + + mA, idA, volumeA := isolationSession(t, env, "iso-tenant-a") + mB, idB, volumeB := isolationSession(t, env, "iso-tenant-b") + + // B's secret, planted host-side inside B's volume, and also written by B's + // OWN guest — so the test covers both a host-authored and a guest-authored + // file (the latter is what a real tenant's workspace actually contains). + if err := os.WriteFile(filepath.Join(volumeB, "host-secret.txt"), []byte(tenantBSecret), 0o600); err != nil { + t.Fatalf("planting tenant B's host secret: %v", err) + } + if out, code := guestSh(t, mB, idB, "printf '%s' '"+tenantBSecret+"' > /workspace/guest-secret.txt"); code != 0 { + t.Fatalf("tenant B could not write its own workspace file: exit %d, %q", code, out) + } + // A's own volume is populated and readable by A. Without this the + // cross-tenant negatives below could pass because A's share is broken + // rather than because B's is unreachable. + if err := os.WriteFile(filepath.Join(volumeA, "a-own.txt"), []byte("tenant-a-own"), 0o600); err != nil { + t.Fatalf("planting tenant A's own file: %v", err) + } + if out, code := guestSh(t, mA, idA, "cat /workspace/a-own.txt"); code != 0 || !strings.Contains(out, "tenant-a-own") { + t.Fatalf("tenant A cannot read its OWN volume (exit %d, %q); the cross-tenant negatives would be vacuous", code, out) + } + // Sanity: B can read its own secret. Without this, the cross-tenant + // negative below could pass vacuously (e.g. if the share were broken for + // everyone). + if out, code := guestSh(t, mB, idB, "cat /workspace/host-secret.txt /workspace/guest-secret.txt"); code != 0 || !strings.Contains(out, tenantBSecret) { + t.Fatalf("tenant B cannot read its OWN volume (exit %d, %q); the cross-tenant negative would be vacuous", code, out) + } + + // Every way A could try to name B's volume. + attempts := map[string]string{ + "B's absolute host volume path": "cat " + filepath.Join(volumeB, "host-secret.txt") + " " + filepath.Join(volumeB, "guest-secret.txt"), + "B's volume dir listing": "ls -la " + volumeB, + "traversal toward B": "cat /workspace/../volume/host-secret.txt; cat /workspace/../../*/volume/*secret*", + "a symlink A plants to B": "ln -sf " + volumeB + " /workspace/b-link && cat /workspace/b-link/host-secret.txt", + "the whole host tmp tree": "grep -rl 'TENANT-B-SECRET' / 2>/dev/null | head -5", + } + for name, script := range attempts { + t.Run("A cannot reach "+name, func(t *testing.T) { + out, code := guestSh(t, mA, idA, script) + if strings.Contains(out, tenantBSecret) { + t.Fatalf("tenant A READ tenant B's secret via %s — CROSS-TENANT ESCAPE.\noutput: %q", name, out) + } + t.Logf("unreachable: %s -> exit %d, %q", name, code, strings.TrimSpace(truncate(out))) + }) + } + + // A's writes must not land in B's volume. Snapshot B's host-side tree, + // let A try, and compare. + before := snapshotTree(t, volumeB) + for _, script := range []string{ + "echo from-a > " + filepath.Join(volumeB, "pwned.txt"), + "echo from-a > /workspace/b-link/pwned-link.txt", + "rm -f " + filepath.Join(volumeB, "host-secret.txt"), + "rm -rf " + volumeB, + } { + out, code := guestSh(t, mA, idA, script) + t.Logf("A write-into-B attempt %q -> exit %d, %q", script, code, strings.TrimSpace(out)) + } + if after := snapshotTree(t, volumeB); before != after { + t.Fatalf("tenant B's host-side volume changed after tenant A's write attempts — CROSS-TENANT ESCAPE.\nbefore: %s\nafter: %s", before, after) + } + // B must still be able to read its own secret: a "confinement" that worked + // by breaking B's share would otherwise pass the comparison above. + if out, code := guestSh(t, mB, idB, "cat /workspace/host-secret.txt"); code != 0 || !strings.Contains(out, tenantBSecret) { + t.Fatalf("tenant B's own volume is broken after A's attempts (exit %d, %q)", code, out) + } + t.Logf("tenant B's volume intact and unreachable from tenant A: %s", before) +} + +// TestMicroVMHostOwnershipParity is the ownership leg (record §(d)): a file the +// guest agent creates on the shared volume must land with the same host-side +// (uid,gid) the podman backend's `--userns=keep-id:uid=N,gid=N` produces — the +// INVOKING HOST user's ids, so files stay identical between backends and the +// invoking user still owns its own workspace tree. +// +// The gid axis is the discriminating one on a uid-1000 dev box: the in-guest +// agent runs as uid==gid==1000 (guestd linuxCredential), while the invoking host +// user's gid is typically NOT 1000 (here: 100/users). So an untranslated +// virtiofsd stamps gid 1000 where podman's keep-id would stamp the host gid — +// which is exactly what this test detects, and why launch.go carries virtiofsd's +// uid/gid translation. +func TestMicroVMHostOwnershipParity(t *testing.T) { + env := microvmtest.Require(t) + m, id, volume := isolationSession(t, env, "iso-parity") + + // The podman keep-id target: the invoking host user's own ids + // (podman.go createArgs `--userns=keep-id:uid=%d,gid=%d` maps the invoking + // host user onto the baked agent id, so the host-side owner is the invoker). + wantUID, wantGID := os.Getuid(), os.Getgid() + + if out, code := guestSh(t, m, id, "umask 022; echo guest-authored > /workspace/from-guest.txt && mkdir -p /workspace/from-guest-dir && id -u && id -g"); code != 0 { + t.Fatalf("guest could not write its own workspace: exit %d, %q", code, out) + } else { + t.Logf("in-guest identity for the write: %q", strings.TrimSpace(out)) + } + + for _, entry := range []string{"from-guest.txt", "from-guest-dir"} { + path := filepath.Join(volume, entry) + info, err := os.Stat(path) + if err != nil { + t.Fatalf("guest-authored %s is absent host-side: %v", entry, err) + } + stat, ok := info.Sys().(*syscall.Stat_t) + if !ok { + t.Fatalf("stat %s: unexpected Sys type %T", entry, info.Sys()) + } + gotUID, gotGID := int(stat.Uid), int(stat.Gid) + t.Logf("host-side ownership of guest-authored %s: uid=%d gid=%d (podman keep-id target: uid=%d gid=%d)", + entry, gotUID, gotGID, wantUID, wantGID) + if gotUID != wantUID || gotGID != wantGID { + t.Errorf("host-ownership PARITY BROKEN for %s: guest-authored file is %d:%d, "+ + "but the podman --userns=keep-id path yields %d:%d — virtiofsd's uid/gid translation "+ + "(launch.go --translate-uid/--translate-gid) must map the in-guest agent id to the invoking host user", + entry, gotUID, gotGID, wantUID, wantGID) + } + } + + // The other direction: a file the HOST creates must be owned by the agent + // in-guest, or the agent cannot write its own workspace tree — the same + // property keep-id gives the container path. + hostAuthored := filepath.Join(volume, "from-host.txt") + if err := os.WriteFile(hostAuthored, []byte("host-authored"), 0o600); err != nil { + t.Fatalf("writing host-authored file: %v", err) + } + out, code := guestSh(t, m, id, "stat -c '%u %g' /workspace/from-host.txt && echo appended >> /workspace/from-host.txt") + if code != 0 { + t.Fatalf("the guest agent cannot read/append a host-authored workspace file (exit %d, %q); "+ + "the translation must leave the invoking user's files owned by the in-guest agent", code, out) + } + t.Logf("in-guest view of a host-authored file (uid gid): %q", strings.TrimSpace(out)) + if want := strings.Fields(strings.TrimSpace(out)); len(want) >= 2 { + agentID := strconv.Itoa(int(agentuid.AgentUID)) + if want[0] != agentID || want[1] != agentID { + t.Errorf("a host-authored workspace file appears in-guest as %s:%s, want the agent id %s:%s — "+ + "the agent would not own its own checkout", want[0], want[1], agentID, agentID) + } + } + // The host-side owner of that file must be unchanged by the guest's append: + // a translation that rewrote ownership on write would silently reassign the + // invoking user's files. + info, err := os.Stat(hostAuthored) + if err != nil { + t.Fatalf("re-stat host-authored file: %v", err) + } + stat, ok := info.Sys().(*syscall.Stat_t) + if !ok { + t.Fatalf("re-stat: unexpected Sys type %T", info.Sys()) + } + if int(stat.Uid) != wantUID || int(stat.Gid) != wantGID { + t.Errorf("a host-authored file changed owner to %d:%d after a guest append, want %d:%d", + stat.Uid, stat.Gid, wantUID, wantGID) + } +} + +// requireQuotaFS gates the real in-guest quota-enforcement leg on a capability +// this box structurally cannot have: a prjquota-ACTIVE filesystem, whose +// provisioning needs root (loop file + `mkfs.xfs`/`mkfs.ext4 -O quota` + mount +// -o prjquota + a project id + limits — every step CAP_SYS_ADMIN, D7 / Global +// Constraint "Rootless is hard"). It mirrors microvmtest.Require's shape: SKIP +// with the reason named when absent, and HARD FAIL when the operator asserts the +// capability is present via COMPASS_REQUIRE_QUOTA_FS=1, so a managed/CI profile +// that is supposed to have it cannot silently stop exercising it. +// +// $COMPASS_TEST_QUOTA_VOLUME names a directory on a project-quota'd filesystem. +// Returning it only after verifyVolumeQuota confirms an ACTIVE bound is the +// honesty gate: a path on an unquota'd filesystem skips (or hard-fails under the +// require flag) rather than running a test that can never observe EDQUOT and +// passing as if it proved quota. +func requireQuotaFS(t *testing.T) string { + t.Helper() + required := os.Getenv("COMPASS_REQUIRE_QUOTA_FS") != "" + refuse := func(format string, args ...any) string { + t.Helper() + if required { + t.Fatalf("COMPASS_REQUIRE_QUOTA_FS is set but "+format, args...) + } + t.Skipf("no project-quota'd filesystem available: "+format+ + " — provisioning one needs root (loop + mkfs -o prjquota + project id + limits), which the rootless "+ + "Runner and this suite do not have (D7). The verification LOGIC is proven hermetically in "+ + "microvm_quota_test.go; set $COMPASS_TEST_QUOTA_VOLUME to a quota'd dir to run this leg for real.", args...) + return "" + } + + volume := os.Getenv("COMPASS_TEST_QUOTA_VOLUME") + if volume == "" { + return refuse("$COMPASS_TEST_QUOTA_VOLUME is unset") + } + reading, err := verifyVolumeQuota(volume, VolumeQuota{}, readVolumeQuota) + if err != nil { + return refuse("%v", err) + } + t.Logf("quota'd volume %s: %s (used ratio %.4f)", volume, reading, reading.UsedRatio()) + return volume +} + +// TestMicroVMVolumeQuotaEnforcedInGuest is the real resource-exhaustion leg +// (record §Plan V6: "writes past the byte bound and creates past the inode bound +// fail inside the guest with ENOSPC/EDQUOT while the host filesystem stays +// healthy"). It runs ONLY on a genuinely project-quota'd filesystem and skips +// with the reason named otherwise — it never fake-proves quota. +func TestMicroVMVolumeQuotaEnforcedInGuest(t *testing.T) { + env := microvmtest.Require(t) + quotaRoot := requireQuotaFS(t) + + volume, err := os.MkdirTemp(quotaRoot, "iso-quota-") //nolint:usetesting // the volume MUST live on the operator-provided quota'd filesystem, not the test's own TMPDIR — that is the whole capability under test + if err != nil { + t.Fatalf("creating a session volume on the quota'd filesystem: %v", err) + } + t.Cleanup(func() { + if err := os.RemoveAll(volume); err != nil { + t.Errorf("removing quota'd volume %s: %v", volume, err) + } + }) + + before, err := readVolumeQuota(volume) + if err != nil { + t.Fatalf("reading the quota before the fill: %v", err) + } + + m := NewMicroVMRuntime(e2eConfig(t, env)) + id, err := m.Create(t.Context(), ContainerSpec{ + Name: "iso-quota", + UID: agentuid.AgentUID, + Mounts: []Mount{{HostPath: volume, ContainerPath: workspaceMountPath}}, + }) + if err != nil { + t.Fatalf("Create: %v", err) + } + t.Cleanup(func() { + if err := m.Remove(t.Context(), id); err != nil { + t.Errorf("Remove: %v", err) + } + }) + if err := m.Start(t.Context(), id); err != nil { + t.Fatalf("Start: %v", err) + } + + // Write past the byte bound: dd until it fails. The guest MUST hit + // ENOSPC/EDQUOT rather than consuming the whole host filesystem. + fill := "dd if=/dev/zero of=/workspace/fill bs=1M count=" + + strconv.FormatInt(before.LimitBytes/(1<<20)+64, 10) + " 2>&1" + out, code := guestSh(t, m, id, fill) + if code == 0 { + t.Fatalf("the guest wrote past the project byte bound (%d B) without failing — the quota is not enforced.\noutput: %q", + before.LimitBytes, out) + } + if !strings.Contains(out, "No space left") && !strings.Contains(out, "Disk quota exceeded") { + t.Errorf("the over-bound write failed with %q, want an ENOSPC/EDQUOT diagnostic", strings.TrimSpace(out)) + } + t.Logf("over-bound write confined: exit %d, %q", code, strings.TrimSpace(truncate(out))) + + // The HOST filesystem must stay healthy: the mount root's free space is + // still ample, i.e. the guest exhausted its project, not the filesystem. + after, err := readVolumeQuota(volume) + if err != nil { + t.Fatalf("reading the quota after the fill: %v", err) + } + if after.FilesystemBytes-after.UsedBytes <= 0 { + t.Fatalf("the host filesystem is exhausted after the guest fill (%s); the quota failed to contain it", after) + } + t.Logf("host filesystem healthy after the guest fill: %s (used ratio %.4f)", after, after.UsedRatio()) +} + +// snapshotTree renders a stable, comparable description of every entry under +// root: relative path, mode, size, and host uid/gid. It is the host-side half of +// each escape assertion — an unchanged snapshot after a batch of guest write +// attempts is what "confined" means, in a form a diff can show. +func snapshotTree(t *testing.T, root string) string { + t.Helper() + var b strings.Builder + err := filepath.WalkDir(root, func(path string, entry os.DirEntry, err error) error { + if err != nil { + return err + } + rel, relErr := filepath.Rel(root, path) + if relErr != nil { + return relErr + } + info, infoErr := entry.Info() + if infoErr != nil { + return infoErr + } + stat, ok := info.Sys().(*syscall.Stat_t) + if !ok { + return fmt.Errorf("stat %s: unexpected Sys type %T", path, info.Sys()) + } + size := info.Size() + if entry.IsDir() { + // A directory's size is filesystem bookkeeping that shifts as + // entries are added and removed and then re-added, so it is not a + // stable identity — the entry SET (rendered by the walk itself) is. + size = -1 + } + fmt.Fprintf(&b, "%s mode=%s size=%d owner=%d:%d\n", rel, info.Mode(), size, stat.Uid, stat.Gid) + return nil + }) + if err != nil { + t.Fatalf("snapshotting %s: %v", root, err) + } + return b.String() +} + +// truncate bounds a guest output for a log line, so a `grep -r /` that returned +// a lot does not flood the test log. +func truncate(out string) string { + const max = 512 + if len(out) <= max { + return out + } + return out[:max] + "…(truncated)" +} diff --git a/go/internal/runtime/microvm_lifecycle.go b/go/internal/runtime/microvm_lifecycle.go index acea4c41..94eb1815 100644 --- a/go/internal/runtime/microvm_lifecycle.go +++ b/go/internal/runtime/microvm_lifecycle.go @@ -230,7 +230,7 @@ func (m *MicroVMRuntime) Create(_ context.Context, spec ContainerSpec) (Containe session := µvmSession{ id: id, name: spec.Name, - cfg: m.bootConfig(runtimeDir, nonce, shared), + cfg: m.bootConfig(runtimeDir, nonce, shared, spec.UID), uid: spec.UID, env: spec.Env, nonce: nonce, @@ -281,7 +281,7 @@ func (m *MicroVMRuntime) Create(_ context.Context, spec ContainerSpec) (Containe // sizing, and the boot nonce carried on the cmdline as lowercase hex under the // compass.boot_nonce key guestd parses. Split out so spec→BootConfig assembly // is unit-testable without booting. -func (m *MicroVMRuntime) bootConfig(runtimeDir string, nonce []byte, shared Mount) microvm.BootConfig { +func (m *MicroVMRuntime) bootConfig(runtimeDir string, nonce []byte, shared Mount, agentUID uint32) microvm.BootConfig { return microvm.BootConfig{ Kernel: m.config.KernelImage, Initrd: m.config.InitrdImage, @@ -294,8 +294,12 @@ func (m *MicroVMRuntime) bootConfig(runtimeDir string, nonce []byte, shared Moun FSTag: workspaceFSTag, FSSocket: filepath.Join(runtimeDir, "virtiofsd.sock"), FSSharedDir: shared.HostPath, - CPUs: m.config.DefaultCPUs, - MemoryMB: m.config.DefaultMemoryMB, + // The in-guest exec uid, so virtiofsd maps it back to the invoking host + // user and the shared volume carries the same host-side ownership the + // podman --userns=keep-id path produces (record §(d) parity). + AgentUID: agentUID, + CPUs: m.config.DefaultCPUs, + MemoryMB: m.config.DefaultMemoryMB, Net: microvm.NetConfig{ VhostUserSocket: filepath.Join(runtimeDir, "net.sock"), MAC: guestMAC, diff --git a/go/internal/runtime/microvm_lifecycle_test.go b/go/internal/runtime/microvm_lifecycle_test.go index 3b6b3bf0..109ea562 100644 --- a/go/internal/runtime/microvm_lifecycle_test.go +++ b/go/internal/runtime/microvm_lifecycle_test.go @@ -43,7 +43,7 @@ func TestBootConfigAssembly(t *testing.T) { share := Mount{HostPath: "/host/checkout", ContainerPath: "/workspace"} runtimeDir := runRoot + "/microvm/sess1" - cfg := m.bootConfig(runtimeDir, nonce, share) + cfg := m.bootConfig(runtimeDir, nonce, share, 1000) if cfg.Kernel != "/img/kernel" || cfg.Initrd != "/img/initrd" || cfg.Rootfs != "/img/rootfs" { t.Fatalf("boot images = %q/%q/%q, want the config paths", cfg.Kernel, cfg.Initrd, cfg.Rootfs) @@ -54,6 +54,12 @@ func TestBootConfigAssembly(t *testing.T) { if cfg.FSTag != workspaceFSTag { t.Fatalf("FSTag = %q, want %q", cfg.FSTag, workspaceFSTag) } + // The in-guest agent uid rides the BootConfig so virtiofsd can map it back + // to the invoking host user (record §(d) host-ownership parity). Dropping it + // silently reverts the share to unmapped ownership. + if cfg.AgentUID != 1000 { + t.Fatalf("AgentUID = %d, want the spec uid 1000", cfg.AgentUID) + } if cfg.VsockCID != guestVsockCID || cfg.VsockPort != guestVsockPort { t.Fatalf("CID/port = %d/%d, want %d/%d", cfg.VsockCID, cfg.VsockPort, guestVsockCID, guestVsockPort) } diff --git a/go/internal/runtime/microvm_preflight.go b/go/internal/runtime/microvm_preflight.go index fd6409ee..148ae85b 100644 --- a/go/internal/runtime/microvm_preflight.go +++ b/go/internal/runtime/microvm_preflight.go @@ -39,6 +39,12 @@ type preflightProbes struct { statImage func(string) error // hashImage returns the streaming lowercase-hex SHA-256 of the image file. hashImage func(string) (string, error) + // readQuota reads the active project quota scoping a volume path (D7: + // read-only, never assign). Behind the seam so the quota axis is + // hermetically testable — a prjquota-active filesystem cannot be + // provisioned rootless, so a fake reading is the only way this decision is + // covered on a dev box. + readQuota quotaReadFn } // defaultPreflightProbes wires the real host-facing implementations behind the @@ -69,6 +75,7 @@ func defaultPreflightProbes() preflightProbes { return nil }, hashImage: hashFileSHA256, + readQuota: readVolumeQuota, } } @@ -110,7 +117,46 @@ func (m *MicroVMRuntime) verifyMicroVMSupport(ctx context.Context, probes prefli // 4. RunRoot: set, writable, and short enough that a session's worst-case // suffixed gateway socket path fits the AF_UNIX budget (record §(b)/§(e)). - return m.verifyRunRoot(probes) + if err := m.verifyRunRoot(probes); err != nil { + return err + } + + // 5. Session-volume quota (D7): under the multi-tenant profile an + // operator-provisioned project quota MUST be active on the session-volume + // filesystem, or startup fails naming the fix. Otherwise the observed + // utilization is logged and nothing gates. + return m.verifyQuota(probes) +} + +// verifyQuota runs the D7 volume-quota check against the session-volume +// filesystem. The per-session volume dir is not known at startup (P2's volume +// lifecycle mints it per session), so the check targets the RunRoot — the one +// startup-known path on the same host filesystem tree the Runner writes session +// state into, which is what "the session-volume filesystem" means at preflight +// time. Verification is read-only and rootless (microvm_quota.go); the Runner +// never assigns a quota. +// +// QuotaRequired unset (Dogfood, single trusted tenant) never fails: an absent +// quota is the documented posture there, so the reading is logged — including +// the utilization V7 will meter — and startup proceeds. No meter is registered +// here; the coherent metric set is V7's. +func (m *MicroVMRuntime) verifyQuota(probes preflightProbes) error { + reading, err := verifyVolumeQuota(m.config.RunRoot, VolumeQuota{}, probes.readQuota) + if err != nil { + if m.config.QuotaRequired { + return err + } + slog.Warn("microvm preflight: session-volume quota is not verified; the single-tenant profile ships no host-enforced quota", + "run_root", m.config.RunRoot, "reason", err) + return nil + } + slog.Info("microvm preflight: session-volume project quota is active", + "run_root", m.config.RunRoot, + "limit_bytes", reading.LimitBytes, + "used_bytes", reading.UsedBytes, + "used_ratio", reading.UsedRatio(), + "required", m.config.QuotaRequired) + return nil } // imageKnob names the flag and env knob that sets one guest image path, for the diff --git a/go/internal/runtime/microvm_preflight_test.go b/go/internal/runtime/microvm_preflight_test.go index b921f91a..2709bf5d 100644 --- a/go/internal/runtime/microvm_preflight_test.go +++ b/go/internal/runtime/microvm_preflight_test.go @@ -17,8 +17,8 @@ import ( ) // okProbes returns a preflightProbes whose every axis passes: KVM opens, the -// trio resolves at floor, and images stat + hash cleanly. Individual rows -// override the axis they exercise. +// trio resolves at floor, images stat + hash cleanly, and the volume quota +// reads as an active bound. Individual rows override the axis they exercise. func okProbes() preflightProbes { return preflightProbes{ openKVM: func() error { return nil }, @@ -34,6 +34,17 @@ func okProbes() preflightProbes { }, statImage: func(string) error { return nil }, hashImage: func(string) (string, error) { return "deadbeef", nil }, + // An active project quota by default (path totals below the mount + // root's = the kernel's projection), so only a row that overrides this + // axis exercises the D7 quota leg. + readQuota: func(path string) (QuotaReading, error) { + return QuotaReading{ + Path: path, MountRoot: "/", + LimitBytes: 10 << 30, UsedBytes: 1 << 30, + LimitInodes: 1 << 20, UsedInodes: 512, + FilesystemBytes: 1 << 40, FilesystemInodes: 1 << 26, + }, nil + }, } } @@ -67,13 +78,51 @@ func okConfig(t *testing.T) MicroVMConfig { } } +// preflightRow is one verifyMicroVMSupport failure-axis case: mutate the +// all-green config/probes to break (or re-pose) exactly one axis, then assert +// the verdict. Shared by the capability table and the D7 quota table, which are +// separate functions only because one table covering every axis outgrows the +// funlen budget. +type preflightRow struct { + name string + mutate func(cfg *MicroVMConfig, p *preflightProbes) + wantOK bool + wantParts []string +} + +// runPreflightRows drives each row from the all-green baseline: an OK row must +// return nil, and a failing row must name every wantParts fragment so the D3 +// "name the missing capability and the fix" contract is asserted, not just the +// existence of an error. +func runPreflightRows(t *testing.T, rows []preflightRow) { + t.Helper() + for _, tt := range rows { + t.Run(tt.name, func(t *testing.T) { + cfg := okConfig(t) + probes := okProbes() + tt.mutate(&cfg, &probes) + m := NewMicroVMRuntime(cfg) + err := m.verifyMicroVMSupport(t.Context(), probes) + if tt.wantOK { + if err != nil { + t.Fatalf("verifyMicroVMSupport = %v, want nil", err) + } + return + } + if err == nil { + t.Fatalf("verifyMicroVMSupport = nil, want error mentioning %v", tt.wantParts) + } + for _, part := range tt.wantParts { + if !strings.Contains(err.Error(), part) { + t.Errorf("error %q does not name %q", err.Error(), part) + } + } + }) + } +} + func TestVerifyMicroVMSupport(t *testing.T) { - tests := []struct { - name string - mutate func(cfg *MicroVMConfig, p *preflightProbes) - wantOK bool - wantParts []string - }{ + runPreflightRows(t, []preflightRow{ { name: "all green", mutate: func(_ *MicroVMConfig, _ *preflightProbes) {}, @@ -170,30 +219,67 @@ func TestVerifyMicroVMSupport(t *testing.T) { }, wantParts: []string{"not creatable/writable"}, }, + }) +} + +// TestVerifyMicroVMSupportQuota is the D7 session-volume quota axis: with +// QuotaRequired set, an active project quota passes and an absent one is a +// startup error naming the volume and the operator fix; with it unset (Dogfood's +// single trusted tenant) neither an absent quota nor an unreadable one gates. +// The unquota'd readings below are the real shape a plain filesystem produces — +// statfs at the path and at its mount root report the same totals, so nothing is +// projected. +func TestVerifyMicroVMSupportQuota(t *testing.T) { + unquotad := func(path string) (QuotaReading, error) { + return QuotaReading{ + Path: path, MountRoot: "/", + LimitBytes: 1 << 40, UsedBytes: 1 << 30, + FilesystemBytes: 1 << 40, + }, nil } - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - cfg := okConfig(t) - probes := okProbes() - tt.mutate(&cfg, &probes) - m := NewMicroVMRuntime(cfg) - err := m.verifyMicroVMSupport(t.Context(), probes) - if tt.wantOK { - if err != nil { - t.Fatalf("verifyMicroVMSupport = %v, want nil", err) - } - return - } - if err == nil { - t.Fatalf("verifyMicroVMSupport = nil, want error mentioning %v", tt.wantParts) - } - for _, part := range tt.wantParts { - if !strings.Contains(err.Error(), part) { - t.Errorf("error %q does not name %q", err.Error(), part) - } - } - }) + unreadable := func(string) (QuotaReading, error) { + return QuotaReading{}, errors.New("statfs: permission denied") } + + runPreflightRows(t, []preflightRow{ + { + name: "quota present and under limit, required, passes", + mutate: func(cfg *MicroVMConfig, _ *preflightProbes) { + cfg.QuotaRequired = true + }, + wantOK: true, + }, + { + name: "quota absent and required fails naming the volume and the fix", + mutate: func(cfg *MicroVMConfig, p *preflightProbes) { + cfg.QuotaRequired = true + p.readQuota = unquotad + }, + wantParts: []string{"no enforced project quota", "prjquota", "quota-required off"}, + }, + { + name: "quota absent and NOT required passes (Dogfood single tenant)", + mutate: func(_ *MicroVMConfig, p *preflightProbes) { + p.readQuota = unquotad + }, + wantOK: true, + }, + { + name: "quota probe failure with required set fails with the read cause", + mutate: func(cfg *MicroVMConfig, p *preflightProbes) { + cfg.QuotaRequired = true + p.readQuota = unreadable + }, + wantParts: []string{"permission denied"}, + }, + { + name: "quota probe failure without required set passes", + mutate: func(_ *MicroVMConfig, p *preflightProbes) { + p.readQuota = unreadable + }, + wantOK: true, + }, + }) } // TestVerifyMicroVMSupportManifest covers the (d) hash-verification axis: a diff --git a/go/internal/runtime/microvm_quota.go b/go/internal/runtime/microvm_quota.go new file mode 100644 index 00000000..8654ce9a --- /dev/null +++ b/go/internal/runtime/microvm_quota.go @@ -0,0 +1,167 @@ +package runtime + +// microvm_quota.go is V6's session-volume quota VERIFICATION path: the expected +// bound (VolumeQuota), what a host probe observed (QuotaReading), and the pure +// decision that turns a reading into a startup verdict (verifyVolumeQuota). +// +// Per D7 the Runner NEVER assigns quota. Project-quota assignment +// (FS_IOC_FSSETXATTR + quotactl) and the loopback-image fallback (mount(2)) both +// need CAP_SYS_ADMIN the rootless Runner lacks, and the frozen no-copy invariant +// forbids swapping the virtio-fs volume for a quota-bounded block device. So the +// multi-tenant deployment provisions per-directory project quota on the +// session-volume filesystem via operator IaC at deploy, and this file's job is +// exactly one read-only, rootless-safe question: *is that quota active?* +// +// Mechanism — statvfs-derived, deliberately NOT quotactl (record §(d) leaves the +// choice to "the rootless-safe mechanism"). The obvious reads both fail rootless +// or answer the wrong question: +// - quotactl(Q_XGETQUOTA, PRJQUOTA, …) returns the real limits AND usage, but +// the kernel gates a non-self quota id on CAP_SYS_ADMIN, and a project id is +// never "self" — so it EPERMs for exactly the caller this check exists for. +// - FS_IOC_FSGETXATTR yields the directory's project id, but a project id is +// only a *label*: it is set identically whether or not the filesystem is +// mounted with enforcement on, so it cannot answer "is the bound live". +// +// statfs(2) answers both at once, unprivileged. XFS (xfs_qm_statvfs, gated on +// project quota ACCT+ENFD && a non-zero project id && FS_XFLAG_PROJINHERIT) and +// ext4 (ext4_statfs_project, gated on the prjquota mount option, PROJINHERIT and +// a non-zero block hard limit) both REWRITE the statfs block/inode totals of a +// project-quota'd directory to the project's limit and usage. So a statfs on the +// volume that reports SMALLER totals than a statfs at its mount root is precisely +// the kernel telling us an enforced project quota is scoping this subtree — and +// the same call hands back the utilization V7 will meter. No syscall wrapper, no +// unsafe, no capability. +// +// The syscall half lives in microvm_quota_linux.go (with a named-refusal stub in +// microvm_quota_unsupported.go); everything here is pure and hermetically tested. + +import "fmt" + +// VolumeQuota is the EXPECTED bound on a session-volume filesystem: a byte +// ceiling and an inode ceiling. It is the operator-declared bound the preflight +// compares an observed reading against, and the target V7's +// compass_microvm_quota_used_ratio meters against. A zero field means +// unbounded/unknown — a zero VolumeQuota asks the presence-only question ("is +// *some* enforced quota active"), which is what the QuotaRequired preflight +// poses, since MicroVMConfig carries the required-ness flag and not a number. +type VolumeQuota struct { + // Bytes is the expected block ceiling in bytes; zero means unspecified. + Bytes int64 + // Inodes is the expected inode ceiling; zero means unspecified. + Inodes int64 +} + +// QuotaReading is one rootless statfs observation of a volume path: the totals +// the kernel reports FOR THAT PATH (rewritten to the project bound when an +// enforced project quota scopes it) alongside the same totals read at the +// containing mount root (never rewritten). The pair is what makes an active +// quota detectable without a capability: they are identical on an unquota'd +// tree and diverge exactly when the kernel projected a quota onto the path. +type QuotaReading struct { + // Path is the volume path that was probed. + Path string + // MountRoot is the mount point the path resolves into — the reference the + // path's own totals are compared against. + MountRoot string + // LimitBytes is the block total statfs reports for Path: the project's byte + // bound under an enforced project quota, else the whole filesystem's size. + LimitBytes int64 + // UsedBytes is the consumed bytes within LimitBytes' scope. + UsedBytes int64 + // LimitInodes is the inode total statfs reports for Path (0 on filesystems + // that do not report an inode count, e.g. btrfs). + LimitInodes int64 + // UsedInodes is the consumed inodes within LimitInodes' scope. + UsedInodes int64 + // FilesystemBytes is the block total at MountRoot — the unprojected size. + FilesystemBytes int64 + // FilesystemInodes is the inode total at MountRoot — unprojected. + FilesystemInodes int64 +} + +// Active reports whether an ENFORCED project quota is scoping Path: the path's +// own statfs totals are strictly smaller than the mount root's, which only the +// kernel's project-quota projection produces (see the file header). Either axis +// suffices — an operator may bound bytes, inodes, or both — but the inode arm is +// only consulted on a filesystem that reports inode counts at all. +// +// Deliberately conservative in one direction: a project quota whose byte limit +// happens to equal the whole filesystem's size projects no observable difference +// and reads as absent. That is a bound which constrains nothing, and reading it +// as absent fails a QuotaRequired startup closed (a legible operator refusal) +// rather than passing a tenant-exhaustible volume as bounded. +func (r QuotaReading) Active() bool { + if r.LimitBytes > 0 && r.FilesystemBytes > 0 && r.LimitBytes < r.FilesystemBytes { + return true + } + return r.LimitInodes > 0 && r.FilesystemInodes > 0 && r.LimitInodes < r.FilesystemInodes +} + +// UsedRatio is the observed byte utilization in [0,1] — the value the preflight +// logs and V7's compass_microvm_quota_used_ratio will meter. Zero when no limit +// was observed. NOTE: this file registers NO meter; exposing the number is V6's +// job, owning the metric set is V7's. +func (r QuotaReading) UsedRatio() float64 { + if r.LimitBytes <= 0 { + return 0 + } + return float64(r.UsedBytes) / float64(r.LimitBytes) +} + +// String renders the reading as one operator-legible line, so an error or log +// carries the observed numbers rather than a struct dump. +func (r QuotaReading) String() string { + return fmt.Sprintf("path=%s mount=%s limit=%dB used=%dB inodes=%d/%d filesystem=%dB/%d inodes", + r.Path, r.MountRoot, r.LimitBytes, r.UsedBytes, r.UsedInodes, r.LimitInodes, + r.FilesystemBytes, r.FilesystemInodes) +} + +// quotaReadFn reads the active quota scoping a path. It is the effectful seam +// verifyVolumeQuota drives, mirroring preflightProbes' openKVM/statImage split, +// so the decision below is unit-testable against a fabricated reading with no +// quota'd filesystem (which cannot be provisioned rootless). +type quotaReadFn func(path string) (QuotaReading, error) + +// verifyVolumeQuota answers whether an operator-provisioned project quota is +// active on path and at least as large as want, returning the observed reading +// (for the caller's utilization log) alongside the verdict. It VERIFIES ONLY — +// no FS_IOC_FSSETXATTR, no quotactl set, no mount (D7). +// +// nil when an enforced quota is active and meets want (a zero want asks +// presence only). Otherwise a startup error naming the volume, what was +// observed, and the operator fix — the D3 "name the missing capability and the +// fix" posture, never a degrade signal. The caller decides whether an absent +// quota is fatal (QuotaRequired) or merely logged. +func verifyVolumeQuota(path string, want VolumeQuota, read quotaReadFn) (QuotaReading, error) { + if path == "" { + return QuotaReading{}, fmt.Errorf( + "microvm preflight: session-volume quota: no volume path to verify: %s", + "set --microvm-runroot or $COMPASS_MICROVM_RUNROOT") + } + reading, err := read(path) + if err != nil { + return QuotaReading{}, fmt.Errorf("microvm preflight: reading the session-volume quota for %q: %w", path, err) + } + reading.Path = path + if !reading.Active() { + return reading, fmt.Errorf( + "microvm preflight: no enforced project quota on the session-volume filesystem at %q (observed %s): "+ + "provision a per-directory project quota on that filesystem via operator IaC "+ + "(e.g. mount it with prjquota, set a project id + FS_XFLAG_PROJINHERIT on the volume dir, and set its block/inode limits), "+ + "or run the single-tenant profile with quota-required off", + path, reading) + } + if want.Bytes > 0 && reading.LimitBytes < want.Bytes { + return reading, fmt.Errorf( + "microvm preflight: the session-volume project quota at %q bounds %d bytes, below the required %d "+ + "(observed %s): raise the provisioned project block limit", + path, reading.LimitBytes, want.Bytes, reading) + } + if want.Inodes > 0 && reading.LimitInodes < want.Inodes { + return reading, fmt.Errorf( + "microvm preflight: the session-volume project quota at %q bounds %d inodes, below the required %d "+ + "(observed %s): raise the provisioned project inode limit", + path, reading.LimitInodes, want.Inodes, reading) + } + return reading, nil +} diff --git a/go/internal/runtime/microvm_quota_linux.go b/go/internal/runtime/microvm_quota_linux.go new file mode 100644 index 00000000..29dc2fec --- /dev/null +++ b/go/internal/runtime/microvm_quota_linux.go @@ -0,0 +1,112 @@ +//go:build linux + +package runtime + +// The syscall half of V6's quota verification: the rootless statfs(2) probe +// microvm_quota.go's pure decision consumes. Linux-only because the +// project-quota-projected statvfs behavior it reads is a Linux XFS/ext4 kernel +// property (xfs_qm_statvfs / ext4_statfs_project); microvm_quota_unsupported.go +// carries the named refusal for every other unix. + +import ( + "fmt" + "path/filepath" + "syscall" +) + +// readVolumeQuota is the production quotaReadFn: it statfs(2)es the volume path +// and, separately, the mount root the path resolves into, so the pure decision +// can compare the two. On a project-quota'd subtree the kernel rewrites the +// path's block/inode totals to the project's limit and usage while the mount +// root's stay the filesystem's real size — so the divergence IS the proof an +// enforced quota is active, readable with no capability (see microvm_quota.go's +// header for why quotactl and FS_IOC_FSGETXATTR are both the wrong read). +func readVolumeQuota(path string) (QuotaReading, error) { + var at syscall.Statfs_t + if err := syscall.Statfs(path, &at); err != nil { + return QuotaReading{}, fmt.Errorf("statfs %q: %w", path, err) + } + root, err := mountRoot(path) + if err != nil { + return QuotaReading{}, err + } + var atRoot syscall.Statfs_t + if err := syscall.Statfs(root, &atRoot); err != nil { + return QuotaReading{}, fmt.Errorf("statfs mount root %q: %w", root, err) + } + return QuotaReading{ + Path: path, + MountRoot: root, + LimitBytes: blocksToBytes(at.Blocks, at.Bsize), + UsedBytes: blocksToBytes(at.Blocks-at.Bfree, at.Bsize), + LimitInodes: int64(at.Files), //nolint:gosec // G115: a statfs inode count is a kernel-reported magnitude, never near the int64 ceiling + UsedInodes: int64(at.Files - at.Ffree), + FilesystemBytes: blocksToBytes(atRoot.Blocks, atRoot.Bsize), + FilesystemInodes: int64(atRoot.Files), //nolint:gosec // G115: as above — a kernel-reported inode count + }, nil +} + +// blocksToBytes converts a statfs block count at the reported fragment size to +// bytes. A non-positive Bsize (never produced by a healthy filesystem, but the +// field is signed) yields 0 rather than a nonsense product, so the decision +// reads it as "no limit observed" and fails a required check closed. +func blocksToBytes(blocks uint64, bsize int64) int64 { + if bsize <= 0 { + return 0 + } + //nolint:gosec // G115: block counts × fragment size are filesystem-sized magnitudes; a real statfs cannot overflow int64 here + return int64(blocks) * bsize +} + +// mountRoot walks path's ancestors until the device number changes, returning +// the deepest ancestor still on the same filesystem — the mount point path +// belongs to. A project quota does NOT change st_dev (it is an accounting scope +// inside one filesystem, not a separate device), so this reliably reaches the +// unprojected reference point the comparison needs. +// +// Paths are resolved through symlinks first: a symlinked volume dir would +// otherwise walk the link's lexical parents, which may live on a different +// filesystem entirely and make the comparison meaningless. +func mountRoot(path string) (string, error) { + resolved, err := filepath.EvalSymlinks(path) + if err != nil { + return "", fmt.Errorf("resolving %q: %w", path, err) + } + dev, err := deviceOf(resolved) + if err != nil { + return "", err + } + current := resolved + for { + parent := filepath.Dir(current) + if parent == current { + // Reached "/" — the filesystem root is the mount root. + return current, nil + } + parentDev, err := deviceOf(parent) + if err != nil { + // An unreadable ancestor (a 0711 parent an unprivileged Runner + // cannot stat) is not a quota verdict: the deepest ancestor proven + // to be on this device is the best reference available, and the + // comparison against it stays sound. + return current, nil //nolint:nilerr // a stat-blocked ancestor bounds the walk; `current` is still a same-device reference, so this is the answer, not a swallowed failure + } + if parentDev != dev { + return current, nil + } + current = parent + } +} + +// deviceOf returns the st_dev of path, the identity the mount-root walk compares. +// syscall.Stat (not os.Stat) because st_dev is all this needs: it fills a +// caller-owned struct with no os.FileInfo allocation and no Sys() type +// assertion, and it is the same call the readVolumeQuota statfs pair already +// uses, keeping this file on one syscall surface. +func deviceOf(path string) (uint64, error) { + var st syscall.Stat_t + if err := syscall.Stat(path, &st); err != nil { + return 0, fmt.Errorf("stat %q: %w", path, err) + } + return st.Dev, nil +} diff --git a/go/internal/runtime/microvm_quota_test.go b/go/internal/runtime/microvm_quota_test.go new file mode 100644 index 00000000..c9aaa534 --- /dev/null +++ b/go/internal/runtime/microvm_quota_test.go @@ -0,0 +1,311 @@ +//go:build unix + +package runtime + +// Hermetic unit tests for V6's volume-quota VERIFICATION (microvm_quota.go). +// These carry NO microvm build tag on purpose: they are what genuinely proves +// the verification logic on any box, including one where a prjquota-active +// filesystem cannot be provisioned (that needs root — loop + mkfs.xfs -o +// prjquota + a project id + limits — which the rootless Runner and this dev box +// both lack, D7 / Global Constraint "Rootless is hard"). The decision is split +// from the syscall behind quotaReadFn precisely so it is covered here rather +// than left to a leg that skips. +// +// The real statfs probe (readVolumeQuota) is exercised too, but only for what is +// honestly assertable without a quota'd filesystem: that it reads a real path, +// and that an unquota'd tree correctly reads as NOT active. A green here does +// not claim quota enforcement was proven — the guest-side ENOSPC/EDQUOT proof is +// the root-gated leg in microvm_isolation_microvm_test.go. + +import ( + "errors" + "math" + "strings" + "testing" +) + +// quotaReading builds an "active enforced quota" reading: the path's own statfs +// totals are strictly smaller than the mount root's, which is exactly what the +// kernel's project-quota projection produces (xfs_qm_statvfs / ext4_statfs_project). +func quotaReading(limitBytes, usedBytes, limitInodes, usedInodes int64) QuotaReading { + return QuotaReading{ + Path: "/srv/compass/volumes", + MountRoot: "/srv", + LimitBytes: limitBytes, + UsedBytes: usedBytes, + LimitInodes: limitInodes, + UsedInodes: usedInodes, + FilesystemBytes: 1 << 40, // 1 TiB filesystem + FilesystemInodes: 1 << 26, + } +} + +// TestQuotaReadingActive is the core detection predicate: a path whose statfs +// totals are smaller than its mount root's has an enforced project quota +// projected onto it; equal totals mean none. Either axis (bytes or inodes) can +// carry the bound, and a filesystem that reports no inode count at all (btrfs +// reports Files == 0) must not make the inode arm read as active. +func TestQuotaReadingActive(t *testing.T) { + tests := []struct { + name string + reading QuotaReading + want bool + }{ + { + name: "byte bound below filesystem size is active", + reading: quotaReading(10<<30, 1<<30, 0, 0), + want: true, + }, + { + name: "inode bound below filesystem inodes is active", + reading: quotaReading(0, 0, 1<<20, 512), + want: true, + }, + { + name: "both bounds set is active", + reading: quotaReading(10<<30, 1<<30, 1<<20, 512), + want: true, + }, + { + name: "totals equal to the mount root are not active", + // An unquota'd tree: statfs at the path and at the mount root + // report the same filesystem. This is the shape a dev box produces. + reading: QuotaReading{ + LimitBytes: 1 << 40, UsedBytes: 1 << 30, + LimitInodes: 1 << 26, UsedInodes: 4096, + FilesystemBytes: 1 << 40, FilesystemInodes: 1 << 26, + }, + want: false, + }, + { + name: "a bound equal to the whole filesystem reads as absent", + // Conservative by design: such a quota constrains nothing, and + // reading it as absent fails a required startup closed rather than + // passing a tenant-exhaustible volume off as bounded. + reading: QuotaReading{ + LimitBytes: 1 << 40, FilesystemBytes: 1 << 40, + }, + want: false, + }, + { + name: "zero inode counts (btrfs) do not read as an inode bound", + // btrfs reports Files == 0; a naive `LimitInodes < FilesystemInodes` + // would be 0 < 0 = false here, but a reading where only the + // FILESYSTEM inodes are zero must also not go active. + reading: QuotaReading{ + LimitBytes: 1 << 40, UsedBytes: 1 << 30, + LimitInodes: 0, UsedInodes: 0, + FilesystemBytes: 1 << 40, FilesystemInodes: 0, + }, + want: false, + }, + { + name: "the zero reading is not active", + reading: QuotaReading{}, + want: false, + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + if got := tt.reading.Active(); got != tt.want { + t.Fatalf("Active() = %v, want %v (reading: %s)", got, tt.want, tt.reading) + } + }) + } +} + +// TestQuotaReadingUsedRatio pins the utilization value V6 exposes and V7 will +// meter: used/limit, and zero (not NaN, not +Inf) when no limit was observed. +func TestQuotaReadingUsedRatio(t *testing.T) { + tests := []struct { + name string + reading QuotaReading + want float64 + }{ + {name: "quarter full", reading: quotaReading(4<<30, 1<<30, 0, 0), want: 0.25}, + {name: "empty", reading: quotaReading(4<<30, 0, 0, 0), want: 0}, + {name: "full", reading: quotaReading(4<<30, 4<<30, 0, 0), want: 1}, + // No limit must not divide by zero into NaN/+Inf — a metric consumer + // (V7) would otherwise export a poisoned sample. + {name: "no limit observed", reading: QuotaReading{UsedBytes: 1 << 30}, want: 0}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + got := tt.reading.UsedRatio() + if math.IsNaN(got) || math.IsInf(got, 0) { + t.Fatalf("UsedRatio() = %v, want a finite value", got) + } + if got != tt.want { + t.Fatalf("UsedRatio() = %v, want %v", got, tt.want) + } + }) + } +} + +// TestVerifyVolumeQuota is the verification decision per axis, driven through +// the injected probe so no quota'd filesystem is needed: an active quota meeting +// the want passes; an absent one errors naming the volume and the operator fix; +// a present-but-too-small bound errors naming both numbers; a probe failure +// propagates rather than reading as "absent" or "fine". +func TestVerifyVolumeQuota(t *testing.T) { + const volume = "/srv/compass/volumes" + + tests := []struct { + name string + path string + want VolumeQuota + read quotaReadFn + wantOK bool + wantParts []string + }{ + { + name: "active quota, presence-only want passes", + path: volume, + want: VolumeQuota{}, + read: func(string) (QuotaReading, error) { return quotaReading(10<<30, 1<<30, 1<<20, 512), nil }, + wantOK: true, + }, + { + name: "active quota at or above the wanted bound passes", + path: volume, + want: VolumeQuota{Bytes: 8 << 30, Inodes: 1 << 19}, + read: func(string) (QuotaReading, error) { return quotaReading(10<<30, 1<<30, 1<<20, 512), nil }, + wantOK: true, + }, + { + name: "absent quota names the volume and the operator fix", + path: volume, + want: VolumeQuota{}, + read: func(string) (QuotaReading, error) { + return QuotaReading{ + LimitBytes: 1 << 40, UsedBytes: 1 << 30, + FilesystemBytes: 1 << 40, + }, nil + }, + wantParts: []string{volume, "no enforced project quota", "prjquota", "FS_XFLAG_PROJINHERIT", "quota-required off"}, + }, + { + name: "byte bound below the wanted bound names both numbers", + path: volume, + want: VolumeQuota{Bytes: 100 << 30}, + read: func(string) (QuotaReading, error) { return quotaReading(10<<30, 1<<30, 0, 0), nil }, + wantParts: []string{volume, "10737418240", "107374182400", "raise the provisioned project block limit"}, + }, + { + name: "inode bound below the wanted bound names both numbers", + path: volume, + want: VolumeQuota{Inodes: 1 << 24}, + read: func(string) (QuotaReading, error) { return quotaReading(10<<30, 1<<30, 1<<20, 512), nil }, + wantParts: []string{volume, "1048576", "16777216", "raise the provisioned project inode limit"}, + }, + { + name: "a probe failure propagates, never reads as absent-or-fine", + path: volume, + want: VolumeQuota{}, + read: func(string) (QuotaReading, error) { return QuotaReading{}, errors.New("statfs: permission denied") }, + wantParts: []string{volume, "permission denied"}, + }, + { + name: "an empty volume path names the run-root knob", + path: "", + want: VolumeQuota{}, + read: func(string) (QuotaReading, error) { return quotaReading(10<<30, 0, 0, 0), nil }, + wantParts: []string{"--microvm-runroot", "COMPASS_MICROVM_RUNROOT"}, + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + reading, err := verifyVolumeQuota(tt.path, tt.want, tt.read) + if tt.wantOK { + if err != nil { + t.Fatalf("verifyVolumeQuota = %v, want nil", err) + } + // A passing verify must hand back the utilization the caller + // logs and V7 meters, not a zero struct. + if reading.LimitBytes == 0 { + t.Fatalf("verifyVolumeQuota returned reading %+v with no limit; the caller needs the observed bound", reading) + } + return + } + if err == nil { + t.Fatalf("verifyVolumeQuota = nil, want an error mentioning %v", tt.wantParts) + } + for _, part := range tt.wantParts { + if !strings.Contains(err.Error(), part) { + t.Errorf("error %q does not name %q", err.Error(), part) + } + } + }) + } +} + +// TestVerifyVolumeQuotaNeverAssigns is the D7 posture as a structural test: the +// verification path takes a READ function and nothing else, so there is no seam +// through which it could set a project id or a limit. Driven by counting probe +// calls and asserting the reading is returned unmutated apart from Path — +// verification observes, it does not write. +func TestVerifyVolumeQuotaNeverAssigns(t *testing.T) { + reads := 0 + fixed := quotaReading(10<<30, 2<<30, 1<<20, 1024) + read := func(string) (QuotaReading, error) { + reads++ + return fixed, nil + } + + got, err := verifyVolumeQuota("/srv/compass/volumes", VolumeQuota{Bytes: 1 << 30}, read) + if err != nil { + t.Fatalf("verifyVolumeQuota = %v, want nil", err) + } + if reads != 1 { + t.Fatalf("the quota probe ran %d times, want exactly 1 (one read-only observation, D7)", reads) + } + if got.LimitBytes != fixed.LimitBytes || got.UsedBytes != fixed.UsedBytes || + got.LimitInodes != fixed.LimitInodes || got.UsedInodes != fixed.UsedInodes { + t.Fatalf("verifyVolumeQuota returned %+v, want the probed reading unchanged %+v", got, fixed) + } + if got.Path != "/srv/compass/volumes" { + t.Fatalf("returned reading Path = %q, want the verified path", got.Path) + } +} + +// TestReadVolumeQuotaOnRealPath exercises the PRODUCTION statfs probe against a +// real directory. What it can honestly assert without root is bounded but real: +// the probe succeeds, reports a plausible filesystem, resolves a mount root, and +// — since no test box's temp dir carries a project quota — reads as NOT active. +// That negative is load-bearing: it is what proves the detection does not +// false-positive and pass an unbounded volume off as quota'd. +func TestReadVolumeQuotaOnRealPath(t *testing.T) { + dir := t.TempDir() + reading, err := readVolumeQuota(dir) + if err != nil { + t.Fatalf("readVolumeQuota(%q) = %v, want a successful rootless read", dir, err) + } + if reading.LimitBytes <= 0 { + t.Fatalf("reading %s has no block total; statfs must report the filesystem size", reading) + } + if reading.MountRoot == "" { + t.Fatalf("reading %s resolved no mount root", reading) + } + if reading.UsedBytes < 0 || reading.UsedBytes > reading.LimitBytes { + t.Fatalf("reading %s has nonsensical usage", reading) + } + if reading.Active() { + t.Fatalf("reading %s reports an active project quota on a plain temp dir; "+ + "the detection must not false-positive (that would pass an unbounded volume as quota'd)", reading) + } + // The utilization the preflight logs must be finite and in range even with + // no quota — V7 meters this value. + if ratio := reading.UsedRatio(); ratio < 0 || ratio > 1 || math.IsNaN(ratio) { + t.Fatalf("UsedRatio() = %v on reading %s, want a finite ratio in [0,1]", ratio, reading) + } +} + +// TestReadVolumeQuotaAbsentPath: a path that does not exist is a probe ERROR, +// not a silent "no quota". Under QuotaRequired that difference decides whether +// startup fails with the real cause (an unreachable volume) or with a misleading +// missing-quota message. +func TestReadVolumeQuotaAbsentPath(t *testing.T) { + if _, err := readVolumeQuota(t.TempDir() + "/does-not-exist"); err == nil { + t.Fatal("readVolumeQuota on an absent path = nil error, want a failure naming the path") + } +} diff --git a/go/internal/runtime/microvm_quota_unsupported.go b/go/internal/runtime/microvm_quota_unsupported.go new file mode 100644 index 00000000..b9e087b5 --- /dev/null +++ b/go/internal/runtime/microvm_quota_unsupported.go @@ -0,0 +1,23 @@ +//go:build !linux + +package runtime + +// The non-Linux leg of V6's quota probe. The verification mechanism is a Linux +// kernel property (XFS/ext4 project-quota-projected statvfs, see +// microvm_quota.go's header), and the microVM backend only ever boots on a +// KVM-capable Linux host — but the runtime package must still type-check +// everywhere (the darwin CI lane compiles it, microvm.go's header), so the seam +// gets a named refusal rather than a build break or a silent "quota active". +// +// Refusing is the fail-closed answer: QuotaRequired on a platform where the +// bound cannot be observed must be a legible startup error, never a pass. + +import "errors" + +// readVolumeQuota refuses on non-Linux: there is no rootless project-quota read +// here, so the required-quota preflight fails closed with the reason named. +func readVolumeQuota(_ string) (QuotaReading, error) { + return QuotaReading{}, errors.New( + "session-volume project-quota verification is Linux-only (it reads the kernel's " + + "project-quota-projected statfs totals); the microVM backend requires a KVM-capable Linux host") +} From 0f9ba0271624bfcb5c1aa75c62d1bdcdfcf98ee3 Mon Sep 17 00:00:00 2001 From: mintaka Date: Sat, 5 Sep 2026 22:19:41 -0400 Subject: [PATCH 2/3] =?UTF-8?q?fix(microvm):=20fold=20V6=20review=20round?= =?UTF-8?q?=201=20=E2=80=94=20quota=20wiring,=20id-map=20hardening,=20test?= =?UTF-8?q?=20non-vacuity=20(RIG-2497)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Addresses the round-1 review of #912 (2 highs + 6 mediums + 4 lows fixed; the third high — `--uid-map` vs `--translate-uid` capability surface — is a design fork filed as RIG-3330, with its record-faithful hardening landed here). ## Fail-open fixes (the two acted highs) - **QuotaRequired was dead code.** `MicroVMConfig.QuotaRequired` was never wired to any operator knob, so the D7 quota gate could never fire in production — it was settable only from tests. Added `--microvm-quota-required` (+ `$COMPASS_MICROVM_QUOTA_REQUIRED`, via a new `boolOrEnv` that *refuses* an unparseable value rather than silently reading false) and threaded it into the config literal. `selectEngine` split out a testable `backendConfig()` seam; new `backend_flags_test.go` pins flag/env/precedence end to end. - **verifyQuota probed the wrong filesystem.** It targeted `RunRoot` (the short `/tmp` socket dir), not the session-volume filesystem (the durable D9 volume, arriving as `Mount.HostPath`). Added `MicroVMConfig.VolumeRoot` (+ `--microvm-volume-root`) and pointed the check at it. When `QuotaRequired` is set and `VolumeRoot` is unknown, startup now **fails closed** with a named error instead of reporting a verdict about a filesystem it never probed. ## id-map hardening (RIG-3330 interim, no mechanism swap) Kept the record's named `--uid-map`/`--gid-map` mechanism; added `--modcaps=-mknod` (the one capability with no legitimate use on a workspace share) and an explicit security-posture comment stating the namespace-scoped capability set the ns-uid-0 mapping confers (bounded to the volume subtree by the mount-ns pivot_root). The narrower `--translate-uid` alternative is tracked as fork RIG-3330. ## Correctness + test-adequacy fixes - **subuid base was hardcoded 100000** — a silent EINVAL boot failure on any host whose `/etc/subuid` range starts elsewhere. Now parsed from the invoking user's `/etc/subuid` entry (behind a pure seam), with a named error when absent, and added as a startup preflight axis so it fails legibly at startup, not at first boot's opaque socket-wait. New `launch_idmap_test.go` pins the exact argv (injecting a non-100000 base so a regression is caught). - **XFS inode-only quota false-negative** — the mount-root `f_files` is a dynamic estimate (`fakeinos` shrinks as the fs fills); added a tolerance margin + documented the dynamism so a genuinely-quota'd volume is not read as unbounded. - **mountRoot false-positive** — a stat-blocked ancestor was swallowed and could report a bogus active quota (a sibling project's bound); now an inconclusive probe that fails closed under `QuotaRequired`. - **Cross-session test vacuity** — the `ls` row asserted content that `ls` never prints, and the sweep row depended on `grep`, which the guest image does not ship, so it exited 127 having never searched. Rewrote each row's success condition to match what its command emits (a bash+awk sweep printing `path:line`, needle via env, self-`/proc` matches excluded), added the non-zero-exit assertion to every confined row, and added a control test proving the sweep finds a planted canary. - **Hardcoded exec uid** `"1000"` → `agentuid.AgentUID` in the isolation, egress, and vsock-gateway microVM suites (a routine const change would otherwise silently make every escape attempt run as an unmapped uid and pass green). - **Parity leg** now hard-fails on malformed `stat` output instead of silently skipping the host→guest direction, and its error text names the flags the code actually uses. Lows: corrected the non-Linux stub's rationale, documented `UsedRatio`'s projected-vs-whole-fs dual meaning (and log it only when a quota is active), flattened the virtiofsd argv construction, and de-duplicated the quota rationale headers to a single source. ## Verification All gates green on a real-KVM box: build+vet (default + `-tags microvm` + darwin `!linux` stub), hermetic `-race` (runtime + microvm + cmd), the KVM isolation suite (real boots — the stricter cross-session sweep genuinely walks the tree and failed twice on real self-match artifacts during development, proving it no longer passes vacuously), golangci-lint (tagged + untagged) 0 issues, nilaway clean (default + microvm). Spec-impact: none. Refs RIG-2497 Co-authored-by: Matt Wilkinson --- go/cmd/compass-runner/backend_flags_test.go | 204 +++++++++++++++++ go/cmd/compass-runner/main.go | 59 ++++- .../runner/e2e_vsock_gateway_microvm_test.go | 8 +- .../runtime/egress_inguest_microvm_test.go | 8 +- go/internal/runtime/microvm.go | 10 + go/internal/runtime/microvm/launch.go | 83 +++++-- .../runtime/microvm/launch_idmap_test.go | 206 ++++++++++++++++++ go/internal/runtime/microvm/subuid.go | 121 ++++++++++ .../runtime/microvm_isolation_microvm_test.go | 156 +++++++++++-- go/internal/runtime/microvm_preflight.go | 70 ++++-- go/internal/runtime/microvm_preflight_test.go | 67 ++++++ go/internal/runtime/microvm_quota.go | 51 ++++- go/internal/runtime/microvm_quota_linux.go | 43 +++- .../runtime/microvm_quota_linux_test.go | 106 +++++++++ go/internal/runtime/microvm_quota_test.go | 49 ++++- .../runtime/microvm_quota_unsupported.go | 14 +- 16 files changed, 1166 insertions(+), 89 deletions(-) create mode 100644 go/cmd/compass-runner/backend_flags_test.go create mode 100644 go/internal/runtime/microvm/launch_idmap_test.go create mode 100644 go/internal/runtime/microvm/subuid.go create mode 100644 go/internal/runtime/microvm_quota_linux_test.go diff --git a/go/cmd/compass-runner/backend_flags_test.go b/go/cmd/compass-runner/backend_flags_test.go new file mode 100644 index 00000000..8b2ec582 --- /dev/null +++ b/go/cmd/compass-runner/backend_flags_test.go @@ -0,0 +1,204 @@ +//go:build unix + +package main + +// The operator-knob wiring for the microVM backend. Every knob here is dead +// code unless it reaches MicroVMConfig, and a knob that silently does not is the +// worst kind of gap: QuotaRequired was declared on the config, consumed by the +// D7 preflight, and unsettable by any operator — so the multi-tenant gate could +// never fire, on any profile. These tests assert BOTH resolution paths (the flag +// and the environment fallback) for the quota knobs, so that failure mode +// cannot recur silently. + +import ( + "strings" + "testing" + + "github.com/RigelBuild/compass/go/internal/runtime" +) + +// flagsFor builds a backendFlags whose pointers address local values, so a test +// sets a "flag" without touching the global flag set (which flag.Parse would +// own and which cannot be re-registered across tests). +func flagsFor() (backendFlags, *bool, *string) { + var ( + empty string + zero int + backend, vmm, virtiofsd, kernel, rootfs, initrd, manifest = empty, empty, empty, empty, empty, empty, empty + runRoot, volumeRoot = empty, empty + cpus, memoryMB = zero, zero + quotaRequired bool + ) + return backendFlags{ + backend: &backend, + vmm: &vmm, + virtiofsd: &virtiofsd, + kernel: &kernel, + rootfs: &rootfs, + initrd: &initrd, + imageManifest: &manifest, + runRoot: &runRoot, + volumeRoot: &volumeRoot, + cpus: &cpus, + memoryMB: &memoryMB, + quotaRequired: "aRequired, + }, "aRequired, &volumeRoot +} + +// TestQuotaRequiredReachesConfigFromFlag: --microvm-quota-required must land on +// MicroVMConfig.QuotaRequired. Without this the D7 preflight gate is +// unreachable dead code and every Runner runs fail-open. +func TestQuotaRequiredReachesConfigFromFlag(t *testing.T) { + f, quotaRequired, _ := flagsFor() + *quotaRequired = true + + cfg, err := f.backendConfig() + if err != nil { + t.Fatalf("backendConfig() = %v, want nil", err) + } + if !cfg.MicroVM.QuotaRequired { + t.Fatal("--microvm-quota-required did not reach MicroVMConfig.QuotaRequired; " + + "the D7 multi-tenant quota gate would be unsettable and could never fire") + } +} + +// TestQuotaRequiredReachesConfigFromEnv: the documented +// $COMPASS_MICROVM_QUOTA_REQUIRED fallback must resolve too — it is how the +// managed profile sets the knob, since it ships env, not argv. +func TestQuotaRequiredReachesConfigFromEnv(t *testing.T) { + for _, raw := range []string{"true", "1", "TRUE"} { + t.Run(raw, func(t *testing.T) { + t.Setenv("COMPASS_MICROVM_QUOTA_REQUIRED", raw) + f, _, _ := flagsFor() + + cfg, err := f.backendConfig() + if err != nil { + t.Fatalf("backendConfig() = %v, want nil", err) + } + if !cfg.MicroVM.QuotaRequired { + t.Fatalf("$COMPASS_MICROVM_QUOTA_REQUIRED=%q did not reach MicroVMConfig.QuotaRequired", raw) + } + }) + } +} + +// TestQuotaRequiredDefaultsOff pins the transitional default: neither knob set +// means the Dogfood single-tenant posture, where an absent quota is documented +// and logged rather than fatal. +func TestQuotaRequiredDefaultsOff(t *testing.T) { + t.Setenv("COMPASS_MICROVM_QUOTA_REQUIRED", "") + f, _, _ := flagsFor() + + cfg, err := f.backendConfig() + if err != nil { + t.Fatalf("backendConfig() = %v, want nil", err) + } + if cfg.MicroVM.QuotaRequired { + t.Fatal("QuotaRequired defaulted to true with no knob set; the transitional default is the single-tenant posture") + } +} + +// TestQuotaRequiredEnvRefusesGarbage: an unparseable value must REFUSE at +// startup naming the variable, never read as false. Silently defaulting a +// misspelled `=yes` to off is exactly the fail-open this knob exists to close. +func TestQuotaRequiredEnvRefusesGarbage(t *testing.T) { + t.Setenv("COMPASS_MICROVM_QUOTA_REQUIRED", "yes-please") + f, _, _ := flagsFor() + + _, err := f.backendConfig() + if err == nil { + t.Fatal("an unparseable $COMPASS_MICROVM_QUOTA_REQUIRED = nil error; a typo must refuse at startup, not read as false") + } + for _, part := range []string{"COMPASS_MICROVM_QUOTA_REQUIRED", "yes-please"} { + if !strings.Contains(err.Error(), part) { + t.Errorf("error %q does not name %q", err.Error(), part) + } + } +} + +// TestQuotaRequiredFlagWinsOverEnv pins the precedence every other knob has +// (orEnv/intOrEnv): an explicitly-passed flag beats the environment fallback. +func TestQuotaRequiredFlagWinsOverEnv(t *testing.T) { + t.Setenv("COMPASS_MICROVM_QUOTA_REQUIRED", "false") + f, quotaRequired, _ := flagsFor() + *quotaRequired = true + + cfg, err := f.backendConfig() + if err != nil { + t.Fatalf("backendConfig() = %v, want nil", err) + } + if !cfg.MicroVM.QuotaRequired { + t.Fatal("--microvm-quota-required=true lost to $COMPASS_MICROVM_QUOTA_REQUIRED=false; the flag must win") + } +} + +// TestVolumeRootReachesConfig: --microvm-volume-root / its env fallback must +// land on MicroVMConfig.VolumeRoot. It is what makes the quota probe target the +// session-volume filesystem instead of the RunRoot socket dir, so an unwired +// knob would leave the preflight either fail-closed forever or (worse) reporting +// a verdict about the wrong mount. +func TestVolumeRootReachesConfig(t *testing.T) { + t.Run("flag", func(t *testing.T) { + f, _, volumeRoot := flagsFor() + *volumeRoot = "/srv/compass/volumes" + + cfg, err := f.backendConfig() + if err != nil { + t.Fatalf("backendConfig() = %v, want nil", err) + } + if cfg.MicroVM.VolumeRoot != "/srv/compass/volumes" { + t.Fatalf("VolumeRoot = %q, want the flag value", cfg.MicroVM.VolumeRoot) + } + }) + t.Run("env", func(t *testing.T) { + t.Setenv("COMPASS_MICROVM_VOLUME_ROOT", "/mnt/volumes") + f, _, _ := flagsFor() + + cfg, err := f.backendConfig() + if err != nil { + t.Fatalf("backendConfig() = %v, want nil", err) + } + if cfg.MicroVM.VolumeRoot != "/mnt/volumes" { + t.Fatalf("VolumeRoot = %q, want the env value", cfg.MicroVM.VolumeRoot) + } + }) +} + +// TestVolumeRootIsNotTheRunRoot: the two knobs must resolve INDEPENDENTLY. A +// wiring that aliased VolumeRoot to RunRoot would reintroduce the exact bug the +// field exists to fix — a quota verdict read on the short /tmp socket dir rather +// than the durable session-volume filesystem. +func TestVolumeRootIsNotTheRunRoot(t *testing.T) { + t.Setenv("COMPASS_MICROVM_RUNROOT", "/tmp/cvm") + t.Setenv("COMPASS_MICROVM_VOLUME_ROOT", "/srv/compass/volumes") + f, _, _ := flagsFor() + + cfg, err := f.backendConfig() + if err != nil { + t.Fatalf("backendConfig() = %v, want nil", err) + } + if cfg.MicroVM.RunRoot != "/tmp/cvm" { + t.Errorf("RunRoot = %q, want /tmp/cvm", cfg.MicroVM.RunRoot) + } + if cfg.MicroVM.VolumeRoot != "/srv/compass/volumes" { + t.Errorf("VolumeRoot = %q, want /srv/compass/volumes", cfg.MicroVM.VolumeRoot) + } +} + +// TestBackendConfigSelectsMicroVM is the end-to-end shape: the resolved config +// still drives backend selection, so the backendConfig split did not detach the +// knobs from the engine they configure. +func TestBackendConfigSelectsMicroVM(t *testing.T) { + f, quotaRequired, volumeRoot := flagsFor() + *f.backend = "microvm" + *quotaRequired = true + *volumeRoot = "/srv/compass/volumes" + + engine, err := f.selectEngine() + if err != nil { + t.Fatalf("selectEngine() = %v, want the microVM backend", err) + } + if _, ok := engine.(*runtime.MicroVMRuntime); !ok { + t.Fatalf("selectEngine() = %T, want *runtime.MicroVMRuntime", engine) + } +} diff --git a/go/cmd/compass-runner/main.go b/go/cmd/compass-runner/main.go index 1cec32c3..58e1dc0e 100644 --- a/go/cmd/compass-runner/main.go +++ b/go/cmd/compass-runner/main.go @@ -283,7 +283,9 @@ func setupOtel(ctx context.Context) (shutdown func(), err error) { // default flag set before flag.Parse and resolved into a runtime after it. type backendFlags struct { backend, vmm, virtiofsd, kernel, rootfs, initrd, imageManifest, runRoot *string + volumeRoot *string cpus, memoryMB *int + quotaRequired *bool } // registerBackendFlags declares the backend-selection flags. Call before @@ -315,21 +317,46 @@ func registerBackendFlags() backendFlags { memoryMB: flag.Int("microvm-memory-mb", 0, "Default guest RAM in MiB per session (microvm backend); 0 leaves the VMM default. "+ "Defaults to $COMPASS_MICROVM_MEMORY_MB."), + volumeRoot: flag.String("microvm-volume-root", "", + "Parent dir the per-session workspace volumes are minted under (microvm backend); "+ + "the filesystem the D7 session-volume quota is verified on. "+ + "Defaults to $COMPASS_MICROVM_VOLUME_ROOT."), + quotaRequired: flag.Bool("microvm-quota-required", false, + "Require an operator-provisioned project quota on the session-volume filesystem "+ + "(multi-tenant profile); startup fails when none is active (microvm backend). "+ + "Defaults to $COMPASS_MICROVM_QUOTA_REQUIRED."), } } // selectEngine resolves the configured runtime backend from the parsed flags // and their environment fallbacks. func (f backendFlags) selectEngine() (runtime.ContainerRuntime, error) { - cpus, err := intOrEnv(*f.cpus, "COMPASS_MICROVM_CPUS") + cfg, err := f.backendConfig() if err != nil { return nil, err } + return runtime.SelectBackend(cfg) +} + +// backendConfig resolves every backend knob from its flag with an environment +// fallback. Split from selectEngine so the resolution is assertable without +// constructing a real backend: a knob that never reaches the config is dead +// code an operator cannot set (the QuotaRequired fail-open), and only a test +// over THIS function catches that. +func (f backendFlags) backendConfig() (runtime.BackendConfig, error) { + cpus, err := intOrEnv(*f.cpus, "COMPASS_MICROVM_CPUS") + if err != nil { + return runtime.BackendConfig{}, err + } memoryMB, err := intOrEnv(*f.memoryMB, "COMPASS_MICROVM_MEMORY_MB") if err != nil { - return nil, err + return runtime.BackendConfig{}, err } - return runtime.SelectBackend(runtime.BackendConfig{ + quotaRequired, err := boolOrEnv(*f.quotaRequired, "COMPASS_MICROVM_QUOTA_REQUIRED") + if err != nil { + return runtime.BackendConfig{}, err + } + return runtime.BackendConfig{ Backend: orEnv(*f.backend, "COMPASS_RUNTIME_BACKEND"), MicroVM: runtime.MicroVMConfig{ VMMPath: orEnv(*f.vmm, "COMPASS_MICROVM_VMM"), @@ -339,10 +366,12 @@ func (f backendFlags) selectEngine() (runtime.ContainerRuntime, error) { InitrdImage: orEnv(*f.initrd, "COMPASS_MICROVM_INITRD"), ImageManifest: orEnv(*f.imageManifest, "COMPASS_MICROVM_IMAGE_MANIFEST"), RunRoot: orEnv(*f.runRoot, "COMPASS_MICROVM_RUNROOT"), + VolumeRoot: orEnv(*f.volumeRoot, "COMPASS_MICROVM_VOLUME_ROOT"), DefaultCPUs: cpus, DefaultMemoryMB: memoryMB, + QuotaRequired: quotaRequired, }, - }) + }, nil } // orEnv returns flagVal when non-empty, else the named environment variable. @@ -373,6 +402,28 @@ func intOrEnv(flagVal int, envKey string) (int, error) { return parsed, nil } +// boolOrEnv returns flagVal when it is true, else the named environment +// variable parsed as a bool. The flag defaults to false, so a set flag always +// wins and an unset one falls through to the env — the same precedence orEnv +// and intOrEnv give their zero values. An empty env var is false (unset); a +// present-but-unparseable one is an error naming the offending variable and +// value, so `COMPASS_MICROVM_QUOTA_REQUIRED=yes` refuses at startup instead of +// silently reading as false and fail-opening the D7 gate. +func boolOrEnv(flagVal bool, envKey string) (bool, error) { + if flagVal { + return true, nil + } + raw := os.Getenv(envKey) + if raw == "" { + return false, nil + } + parsed, err := strconv.ParseBool(raw) + if err != nil { + return false, fmt.Errorf("$%s=%q is not a boolean (want true/false/1/0): %w", envKey, raw, err) + } + return parsed, nil +} + // parseEgress parses the comma-separated allowlist into a validated EgressPolicy. // An empty list is a valid default-deny policy (no host reachable). func parseEgress(csv string) (runtime.EgressPolicy, error) { diff --git a/go/internal/runner/e2e_vsock_gateway_microvm_test.go b/go/internal/runner/e2e_vsock_gateway_microvm_test.go index 5de32ac6..f4918ae5 100644 --- a/go/internal/runner/e2e_vsock_gateway_microvm_test.go +++ b/go/internal/runner/e2e_vsock_gateway_microvm_test.go @@ -48,6 +48,7 @@ import ( "errors" "fmt" "os" + "strconv" "strings" "testing" "time" @@ -55,6 +56,7 @@ import ( "connectrpc.com/connect" compassv1 "github.com/RigelBuild/compass/go/gen/compass/v1" + "github.com/RigelBuild/compass/go/internal/agentuid" compassv1internal "github.com/RigelBuild/compass/go/internal/gen/compass/v1" "github.com/RigelBuild/compass/go/internal/microvmtest" "github.com/RigelBuild/compass/go/internal/runnertest" @@ -175,7 +177,7 @@ func w3LiveSpec(t *testing.T) runtime.AgentSpec { Workspace: runtime.Workspace{ CheckoutDir: "/workspace", HomeDir: "/workspace", - UID: 1000, + UID: agentuid.AgentUID, }, Mounts: []runtime.Mount{{HostPath: t.TempDir(), ContainerPath: "/workspace"}}, Egress: runtime.EgressPolicy{}, @@ -453,7 +455,7 @@ func runInGuestProbe(t *testing.T, m *runtime.MicroVMRuntime, id runtime.Contain t.Fatalf("marshaling probe request: %v", err) } script := inGuestProbeScript(string(reqBody)) - out, err := m.Exec(ctx, id, runtime.NewExecSpec("bun", "-e", script).AsUser("1000")) + out, err := m.Exec(ctx, id, runtime.NewExecSpec("bun", "-e", script).AsUser(strconv.Itoa(int(agentuid.AgentUID)))) if err != nil { t.Fatalf("in-guest bun probe exec errored (harness fault, not a round-trip verdict): %v", err) } @@ -529,7 +531,7 @@ func inGuestCanReachIP(t *testing.T, m *runtime.MicroVMRuntime, id runtime.Conta ctx, cancel := context.WithTimeout(t.Context(), inGuestProbeTimeout) defer cancel() script := fmt.Sprintf("timeout %d bash -c 'exec 3<>/dev/tcp/%s/443 && echo connected'", guestConnectTimeoutSecs, ip) - out, err := m.Exec(ctx, id, runtime.NewExecSpec("sh", "-c", script).AsUser("1000")) + out, err := m.Exec(ctx, id, runtime.NewExecSpec("sh", "-c", script).AsUser(strconv.Itoa(int(agentuid.AgentUID)))) if err != nil { t.Fatalf("in-guest IP connect probe to %s errored (harness fault, not a firewall verdict): %v", ip, err) } diff --git a/go/internal/runtime/egress_inguest_microvm_test.go b/go/internal/runtime/egress_inguest_microvm_test.go index 15b639a5..558aecf5 100644 --- a/go/internal/runtime/egress_inguest_microvm_test.go +++ b/go/internal/runtime/egress_inguest_microvm_test.go @@ -43,10 +43,12 @@ package runtime import ( "context" + "strconv" "strings" "testing" "time" + "github.com/RigelBuild/compass/go/internal/agentuid" "github.com/RigelBuild/compass/go/internal/microvmtest" ) @@ -84,7 +86,7 @@ func startEgressSession(t *testing.T, egress EgressPolicy, name string) (*MicroV workspace := t.TempDir() id, err := m.Create(t.Context(), ContainerSpec{ Name: name, - UID: 1000, + UID: agentuid.AgentUID, Egress: egress, Mounts: []Mount{{HostPath: workspace, ContainerPath: "/workspace"}}, }) @@ -123,7 +125,7 @@ func canReachIPv4(t *testing.T, m *MicroVMRuntime, id ContainerID, ip string) bo defer cancel() script := "timeout " + guestConnectTimeout + " bash -c 'exec 3<>/dev/tcp/" + ip + "/443 && echo connected'" - out, err := m.Exec(ctx, id, NewExecSpec("sh", "-c", script).AsUser("1000")) + out, err := m.Exec(ctx, id, NewExecSpec("sh", "-c", script).AsUser(strconv.Itoa(int(agentuid.AgentUID)))) if err != nil { t.Fatalf("in-guest connect probe to %s errored (harness fault, not a firewall verdict): %v", ip, err) } @@ -166,7 +168,7 @@ func TestInGuestEgressAgentCannotAlterRuleset(t *testing.T) { // failed command (guest exec model), never a transport error. ctx, cancel := context.WithTimeout(t.Context(), egressProbeTimeout) defer cancel() - flush, err := m.Exec(ctx, id, NewExecSpec("nft", "flush", "ruleset").AsUser("1000")) + flush, err := m.Exec(ctx, id, NewExecSpec("nft", "flush", "ruleset").AsUser(strconv.Itoa(int(agentuid.AgentUID)))) if err != nil { t.Fatalf("nft flush probe errored (harness fault, not a firewall verdict): %v", err) } diff --git a/go/internal/runtime/microvm.go b/go/internal/runtime/microvm.go index 2e271e76..9a5735aa 100644 --- a/go/internal/runtime/microvm.go +++ b/go/internal/runtime/microvm.go @@ -64,6 +64,16 @@ type MicroVMConfig struct { // only. Appended, never reordered: the field is additive to a config // operators already construct positionally in tests. QuotaRequired bool + // VolumeRoot is the parent dir the per-session workspace volumes are minted + // under (P2's volume lifecycle), i.e. a startup-known path ON THE + // SESSION-VOLUME FILESYSTEM. It exists solely so the D7 quota check probes + // the filesystem sessions actually consume: RunRoot is the socket dir, held + // to a SHORT /tmp path by the AF_UNIX sun_path budget, so it is routinely a + // different filesystem entirely and a verdict read there says nothing about + // the durable volume. When QuotaRequired is set and this is empty the + // preflight fails closed rather than reporting a verdict about a filesystem + // it never probed. Appended, never reordered. + VolumeRoot string } // BackendConfig selects and configures the container runtime backend. Backend diff --git a/go/internal/runtime/microvm/launch.go b/go/internal/runtime/microvm/launch.go index 69c4fc56..a504f79c 100644 --- a/go/internal/runtime/microvm/launch.go +++ b/go/internal/runtime/microvm/launch.go @@ -156,16 +156,24 @@ func launch(ctx context.Context, cfg BootConfig, opts launchOptions) (_ *VM, err if lookErr != nil { return nil, fmt.Errorf("microvm: resolving virtiofsd on PATH: %w", lookErr) } + // The subordinate base is READ, never assumed: newuidmap validates the + // requested host range against /etc/subuid, so a host whose range does + // not start at the conventional 100000 would otherwise fail here as an + // opaque "waiting for daemon sockets" timeout. VerifySubordinateIDRange + // runs the same read at startup so this error is rare by construction. + subBase := 0 + if cfg.AgentUID != 0 { + base, subErr := SubordinateIDBase() + if subErr != nil { + return nil, subErr + } + subBase = base + } vm.virtiofsd = &child{ name: "virtiofsd", logPath: filepath.Join(dir, "virtiofsd.log"), //nolint:gosec // G204: the microVM harness seam — virtiofsdPath is LookPath-resolved and the argv is harness-built from BootConfig, neither user-controlled - cmd: exec.CommandContext(ctx, virtiofsdPath, - append([]string{ - "--socket-path=" + cfg.FSSocket, - "--shared-dir=" + cfg.FSSharedDir, - "--sandbox=namespace", - }, virtiofsdIDMapArgs(cfg.AgentUID)...)...), + cmd: exec.CommandContext(ctx, virtiofsdPath, virtiofsdArgs(cfg, subBase)...), } if startErr := startChild(vm.virtiofsd); startErr != nil { return nil, fmt.Errorf("microvm: starting virtiofsd: %w", startErr) @@ -240,6 +248,29 @@ func launch(ctx context.Context, cfg BootConfig, opts launchOptions) (_ *VM, err return vm, nil } +// virtiofsdArgs builds the whole virtiofsd argv: the socket/share/sandbox flags +// every boot carries, the capability trim, and the id mapping (empty under the +// V2a spike, see virtiofsdIDMapArgs). subBase is the host subordinate id mapped +// to namespace-uid 0, read from /etc/subuid by the caller; it is ignored when +// cfg.AgentUID is zero. +// +// --modcaps=-mknod drops CAP_MKNOD from the capability set virtiofsd retains +// under --sandbox=namespace. A workspace share has no legitimate use for device +// nodes — the agent checks out source and writes build output — so the +// capability is pure escape surface, and dropping it is free (see +// virtiofsdIDMapArgs on the rest of the retained set). +func virtiofsdArgs(cfg BootConfig, subBase int) []string { + idMap := virtiofsdIDMapArgs(cfg.AgentUID, subBase) + args := make([]string, 0, 4+len(idMap)) + args = append(args, + "--socket-path="+cfg.FSSocket, + "--shared-dir="+cfg.FSSharedDir, + "--sandbox=namespace", + "--modcaps=-mknod", + ) + return append(args, idMap...) +} + // virtiofsdIDMapArgs builds the virtiofsd uid/gid mapping that gives the shared // volume the SAME host-side ownership podman's `--userns=keep-id:uid=N,gid=N` // produces (record §(d): "virtiofsd does its own uid/gid translation via that @@ -275,17 +306,42 @@ func launch(ctx context.Context, cfg BootConfig, opts launchOptions) (_ *VM, err // not its uid: the guest agent runs uid==gid==agentUID (guestd linuxCredential) // while a host user's gid is routinely different (e.g. 1000:100). Collapsing gid // onto uid here is precisely the parity break the V6 parity test detects. -func virtiofsdIDMapArgs(agentUID uint32) []string { +// +// SECURITY POSTURE — this is NOT pure ownership parity, and the difference is +// deliberate and accepted, not incidental. Mapping a subordinate id to +// namespace-uid 0 makes virtiofsd ROOT IN ITS OWN USER NAMESPACE, so the daemon +// retains a namespace-scoped CAP_CHOWN / CAP_DAC_OVERRIDE / CAP_SETUID / +// CAP_SETGID / CAP_FOWNER / CAP_FSETID / CAP_SETFCAP (virtiofsd README §Usage) +// that a plain rootless daemon with only the invoking user's ambient authority +// would not have. Two things bound the consequence: +// +// - The authority is namespace-scoped and the namespace holds exactly two host +// ids — the subordinate id and the invoking user — so it confers nothing over +// any OTHER host user's files. +// - --sandbox=namespace pivot_roots the daemon into the shared dir, so even +// that authority reaches only the volume subtree it is serving. +// +// CAP_MKNOD is dropped outright by virtiofsdArgs' --modcaps=-mknod: a workspace +// share has no legitimate device nodes, so it is surface with no use. +// +// The alternative — --translate-uid/--translate-gid, which reaches the same +// host-side ownership with NO namespace-uid-0 mapping and therefore none of the +// above capabilities, at the cost of foreclosing POSIX ACLs on the share — is +// tracked as a design fork (RIG-3330) for the record's owner to rule on. It is +// NOT swapped in here: --uid-map/--gid-map is the frozen record's named +// mechanism, and changing it is a design decision, not a review fix. +func virtiofsdIDMapArgs(agentUID uint32, subBase int) []string { if agentUID == 0 { return nil } hostUID := os.Getuid() hostGID := os.Getgid() agent := strconv.FormatUint(uint64(agentUID), 10) + base := strconv.Itoa(subBase) return []string{ - "--uid-map", idMapSpec("0", strconv.Itoa(subordinateIDBase)), + "--uid-map", idMapSpec("0", base), "--uid-map", idMapSpec(agent, strconv.Itoa(hostUID)), - "--gid-map", idMapSpec("0", strconv.Itoa(subordinateIDBase)), + "--gid-map", idMapSpec("0", base), "--gid-map", idMapSpec(agent, strconv.Itoa(hostGID)), } } @@ -296,15 +352,6 @@ func idMapSpec(namespaceID, hostID string) string { return ":" + namespaceID + ":" + hostID + ":1:" } -// subordinateIDBase is the host subordinate uid/gid mapped to namespace id 0 so -// virtiofsd can act as root INSIDE its user namespace (and therefore chown -// guest-created inodes). It is the conventional first entry of a rootless -// /etc/subuid + /etc/subgid range — the same 100000 base shadow-utils allocates -// and podman's rootless userns consumes — so it needs no capability, only that -// the invoking user has a subordinate range at all (which rootless podman on -// this host already requires, podman.go:22-24). -const subordinateIDBase = 100000 - // vmmArgs builds the cloud-hypervisor argv exactly per the record (lines // 542-547), dropping --fs/--vsock under the net-only smoke. Launch appends to // the guest cmdline (per BootConfig.Cmdline's contract): diff --git a/go/internal/runtime/microvm/launch_idmap_test.go b/go/internal/runtime/microvm/launch_idmap_test.go new file mode 100644 index 00000000..5898368f --- /dev/null +++ b/go/internal/runtime/microvm/launch_idmap_test.go @@ -0,0 +1,206 @@ +//go:build unix + +package microvm + +// Hermetic argv assertions for virtiofsd's id mapping and capability trim +// (record §(d) host-ownership parity), following launch_cmdline_test.go's shape: +// no VMM boots, no daemon spawns — the exact flag/value pairs are the contract, +// because a wrong pair fails at first boot as an opaque daemon-socket timeout +// rather than as a legible error. +// +// The subordinate base is injected as a fixed value rather than read from the +// test box's /etc/subuid, which is the whole point of the parse/spawn split in +// subuid.go: the mapping is pinned identically on a host allocated 100000 and +// one allocated 165536. + +import ( + "os" + "slices" + "strconv" + "strings" + "testing" +) + +// testSubBase is a deliberately NON-conventional subordinate base: 100000 would +// pass even against the old hardcode, so the assertions below would not detect a +// regression back to it. +const testSubBase = 165536 + +// flagValues returns every value token following an occurrence of flag in argv, +// in order — so a test asserts both the count and the exact specs of a repeated +// flag rather than a single Contains. +func flagValues(argv []string, flag string) []string { + var out []string + for i, tok := range argv { + if tok == flag && i+1 < len(argv) { + out = append(out, argv[i+1]) + } + } + return out +} + +// TestVirtiofsdIDMapArgsExactSpecs pins all four mapping pairs for a real agent +// uid. The gid arm is the discriminating one: its host side must be os.Getgid(), +// NOT os.Getuid() — a host user's gid is routinely different from its uid (e.g. +// 1000:100), and collapsing gid onto uid is precisely the host-ownership parity +// break the KVM parity leg detects. +func TestVirtiofsdIDMapArgsExactSpecs(t *testing.T) { + const agentUID = 1000 + argv := virtiofsdIDMapArgs(agentUID, testSubBase) + + base := strconv.Itoa(testSubBase) + agent := strconv.Itoa(agentUID) + wantUID := []string{":0:" + base + ":1:", ":" + agent + ":" + strconv.Itoa(os.Getuid()) + ":1:"} + wantGID := []string{":0:" + base + ":1:", ":" + agent + ":" + strconv.Itoa(os.Getgid()) + ":1:"} + + if got := flagValues(argv, "--uid-map"); !slices.Equal(got, wantUID) { + t.Errorf("--uid-map specs = %v, want %v (argv %v)", got, wantUID, argv) + } + if got := flagValues(argv, "--gid-map"); !slices.Equal(got, wantGID) { + t.Errorf("--gid-map specs = %v, want %v (argv %v)", got, wantGID, argv) + } + // The subordinate base must come from the injected value, never from a + // hardcoded 100000 — the correctness bug on any host whose /etc/subuid + // range starts elsewhere. + for _, spec := range append(flagValues(argv, "--uid-map"), flagValues(argv, "--gid-map")...) { + if strings.Contains(spec, ":100000:") && testSubBase != 100000 { + t.Errorf("spec %q carries a hardcoded 100000 base; the subordinate base must be the parsed one (%d)", spec, testSubBase) + } + } +} + +// TestVirtiofsdIDMapArgsUnmappedSpike pins the V2a carve-out: a zero agentUID +// (the spike harness, which shares a throwaway dir and asserts nothing about +// ownership) gets NO mapping at all, so that suite keeps booting unchanged. +func TestVirtiofsdIDMapArgsUnmappedSpike(t *testing.T) { + if argv := virtiofsdIDMapArgs(0, testSubBase); argv != nil { + t.Fatalf("virtiofsdIDMapArgs(0) = %v, want nil (the V2a spike share stays unmapped)", argv) + } +} + +// TestVirtiofsdArgsDropsMknod pins the capability trim on the full argv: a +// workspace share has no legitimate device nodes, so CAP_MKNOD is dropped from +// the set virtiofsd retains as namespace-root — on EVERY boot, including the +// unmapped spike, since the flag costs nothing there. +func TestVirtiofsdArgsDropsMknod(t *testing.T) { + for _, agentUID := range []uint32{0, 1000} { + cfg := BootConfig{ + FSSocket: "/tmp/cvm/virtiofsd.sock", + FSSharedDir: "/tmp/cvm/share", + AgentUID: agentUID, + } + argv := virtiofsdArgs(cfg, testSubBase) + if !slices.Contains(argv, "--modcaps=-mknod") { + t.Errorf("virtiofsdArgs(AgentUID=%d) = %v, want it to carry --modcaps=-mknod", agentUID, argv) + } + // The pre-existing flags must survive the argv restructure. + for _, want := range []string{ + "--socket-path=" + cfg.FSSocket, + "--shared-dir=" + cfg.FSSharedDir, + "--sandbox=namespace", + } { + if !slices.Contains(argv, want) { + t.Errorf("virtiofsdArgs(AgentUID=%d) = %v, want it to carry %q", agentUID, argv, want) + } + } + } +} + +// TestVirtiofsdArgsIncludesMapping is the seam assertion the launch path depends +// on: the full argv carries the mapping for a real agent uid and none for the +// spike, so the restructure into a named local did not drop the id args. +func TestVirtiofsdArgsIncludesMapping(t *testing.T) { + mapped := virtiofsdArgs(BootConfig{AgentUID: 1000}, testSubBase) + if got := len(flagValues(mapped, "--uid-map")); got != 2 { + t.Errorf("mapped argv %v carries %d --uid-map specs, want 2", mapped, got) + } + spike := virtiofsdArgs(BootConfig{AgentUID: 0}, testSubBase) + if got := flagValues(spike, "--uid-map"); len(got) != 0 { + t.Errorf("spike argv %v carries --uid-map specs %v, want none", spike, got) + } +} + +// TestParseSubordinateIDBase pins the subuid(5) parse: the invoking user's entry +// is matched by NAME or by uid, comments and malformed/zero-count entries are +// skipped, and a user with no range is a named error rather than a silent +// fallback to a base the user does not own. +func TestParseSubordinateIDBase(t *testing.T) { + tests := []struct { + name string + content string + uid int + username string + want int + wantErr []string + }{ + { + name: "matched by username", + content: "root:100000:65536\nmattw:165536:65536\n", + uid: 1000, + username: "mattw", + want: 165536, + }, + { + name: "matched by uid when the name is unresolvable", + content: "1000:200000:65536\n", + uid: 1000, + want: 200000, + }, + { + name: "comments and blank lines are skipped", + content: "# subuid(5)\n\nmattw:100000:65536\n", + uid: 1000, + username: "mattw", + want: 100000, + }, + { + name: "a zero-count range is not trusted", + content: "mattw:100000:0\nmattw:300000:65536\n", + uid: 1000, + username: "mattw", + want: 300000, + }, + { + name: "a malformed entry is skipped, not parsed as a base", + content: "mattw:not-a-number:65536\nmattw:400000:1\n", + uid: 1000, + username: "mattw", + want: 400000, + }, + { + name: "no range for the user names the user and the fix", + content: "root:100000:65536\nother:165536:65536\n", + uid: 1000, + username: "mattw", + wantErr: []string{"mattw", "/etc/subuid", "usermod --add-subuids", "newuidmap"}, + }, + { + name: "an empty file names the fix", + content: "", + uid: 1000, + wantErr: []string{"uid 1000", "/etc/subuid"}, + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + got, err := parseSubordinateIDBase(strings.NewReader(tt.content), tt.uid, tt.username) + if len(tt.wantErr) > 0 { + if err == nil { + t.Fatalf("parseSubordinateIDBase = %d, want an error naming %v", got, tt.wantErr) + } + for _, part := range tt.wantErr { + if !strings.Contains(err.Error(), part) { + t.Errorf("error %q does not name %q", err.Error(), part) + } + } + return + } + if err != nil { + t.Fatalf("parseSubordinateIDBase = %v, want base %d", err, tt.want) + } + if got != tt.want { + t.Fatalf("parseSubordinateIDBase = %d, want %d", got, tt.want) + } + }) + } +} diff --git a/go/internal/runtime/microvm/subuid.go b/go/internal/runtime/microvm/subuid.go new file mode 100644 index 00000000..8ebd1f5f --- /dev/null +++ b/go/internal/runtime/microvm/subuid.go @@ -0,0 +1,121 @@ +//go:build unix + +package microvm + +// The /etc/subuid read behind virtiofsd's id mapping. virtiofsd shells out to +// newuidmap(1) for a non-trivial --uid-map, and newuidmap VALIDATES the +// requested host range against subuid(5) ("the range of subordinate user IDs +// must have been set up via subuid(5)", virtiofsd README §--uid-map). So the +// base mapped to namespace-uid 0 is a per-host allocation that must be READ, not +// assumed: shadow-utils happens to allocate 100000 to the first user, but a +// second user on the same box gets 165536, and LDAP/AD-backed or +// container-image-provisioned hosts routinely differ. Assuming it fails in the +// worst way — newuidmap refuses, virtiofsd dies before binding its socket, and +// the boot surfaces as an opaque "waiting for daemon sockets" timeout with the +// real cause only in virtiofsd.log. +// +// Split into a pure parse over an io.Reader plus a thin file-reading wrapper so +// the argv tests can pin the exact mapping against a fixed base with no +// dependence on the test box's own /etc/subuid. + +import ( + "bufio" + "fmt" + "io" + "os" + "os/user" + "strconv" + "strings" +) + +// subordinateIDPath is the subuid(5) map newuidmap validates a requested host +// range against. Its sibling /etc/subgid is deliberately NOT read: the gid map +// reuses this base, matching how shadow-utils allocates the two ranges in +// lockstep and how rootless podman consumes them. +const subordinateIDPath = "/etc/subuid" + +// SubordinateIDBase returns the first subordinate uid allocated to the invoking +// user — the host id virtiofsd's mapping makes namespace-uid 0 so the daemon can +// chown guest-created inodes (see virtiofsdIDMapArgs). It fails with a named +// error, never a fallback, when the user has no subordinate range: silently +// mapping an id the user does not own is exactly the opaque boot failure this +// read exists to prevent. +func SubordinateIDBase() (int, error) { + f, err := os.Open(subordinateIDPath) + if err != nil { + return 0, fmt.Errorf( + "microvm: virtiofsd id-mapping requires a subordinate uid range for the invoking user, "+ + "but %s is unreadable: %w (rootless podman requires the same file; "+ + "provision it with `usermod --add-subuids 100000-165535 `)", + subordinateIDPath, err) + } + // Read-only handle over a small text file; a close error after a completed + // parse cannot affect the already-returned base and is not actionable. + defer func() { _ = f.Close() }() + + uid := os.Getuid() + // The invoking user's NAME as well as its uid: subuid(5) entries key on + // either, and shadow-utils writes the name. + name := "" + if u, lookupErr := user.LookupId(strconv.Itoa(uid)); lookupErr == nil { + name = u.Username + } + return parseSubordinateIDBase(f, uid, name) +} + +// VerifySubordinateIDRange is the startup axis over the same read: it resolves +// the invoking user's subordinate base and discards it, so a host with no +// subordinate range fails preflight with the fix named rather than at the first +// session boot as a daemon-socket timeout. +func VerifySubordinateIDRange() error { + if _, err := SubordinateIDBase(); err != nil { + return err + } + return nil +} + +// parseSubordinateIDBase is the pure half: the first subuid(5) entry whose owner +// field matches the invoking user by name or by uid, returning its base. The +// format is `::` with `#` comments; a malformed or +// zero-count entry is skipped rather than trusted, since a range that grants no +// id cannot back a mapping. +func parseSubordinateIDBase(r io.Reader, uid int, username string) (int, error) { + uidStr := strconv.Itoa(uid) + scanner := bufio.NewScanner(r) + for scanner.Scan() { + line := strings.TrimSpace(scanner.Text()) + if line == "" || strings.HasPrefix(line, "#") { + continue + } + fields := strings.Split(line, ":") + if len(fields) != 3 { + continue + } + owner := strings.TrimSpace(fields[0]) + if owner != uidStr && (username == "" || owner != username) { + continue + } + base, err := strconv.Atoi(strings.TrimSpace(fields[1])) + if err != nil { + continue + } + count, err := strconv.Atoi(strings.TrimSpace(fields[2])) + if err != nil || count < 1 || base < 0 { + continue + } + return base, nil + } + if err := scanner.Err(); err != nil { + return 0, fmt.Errorf("microvm: reading %s: %w", subordinateIDPath, err) + } + who := username + if who == "" { + who = "uid " + uidStr + } + return 0, fmt.Errorf( + "microvm: virtiofsd id-mapping requires a subordinate uid range for %s in %s, but none is allocated "+ + "(newuidmap validates the mapped host range against subuid(5), so virtiofsd would die before binding "+ + "its socket); rootless podman requires the same — provision one with "+ + "`usermod --add-subuids 100000-165535 %s` and the matching --add-subgids", + who, subordinateIDPath, who) +} diff --git a/go/internal/runtime/microvm_isolation_microvm_test.go b/go/internal/runtime/microvm_isolation_microvm_test.go index b4fad015..056c09be 100644 --- a/go/internal/runtime/microvm_isolation_microvm_test.go +++ b/go/internal/runtime/microvm_isolation_microvm_test.go @@ -93,13 +93,88 @@ func isolationSession(t *testing.T, env microvmtest.Env, name string) (*MicroVMR func guestSh(t *testing.T, m *MicroVMRuntime, id ContainerID, script string) (string, int) { t.Helper() out, err := m.Exec(t.Context(), id, - NewExecSpec("sh", "-s").WithStdin(script).AsUser("1000")) + NewExecSpec("sh", "-s").WithStdin(script).AsUser(strconv.Itoa(int(agentuid.AgentUID)))) if err != nil { t.Fatalf("guest exec failed at the transport/refusal layer (not the escape itself): %v", err) } return out.Stdout + out.Stderr, out.ExitCode } +// crossSessionAttempt is one way tenant A could try to reach tenant B's volume, +// paired with what its OUTPUT must not contain. The secret body is the +// discriminator for a `cat`; a command that prints names rather than content +// (`ls`, `grep -r` on paths) needs its own forbidden strings, or the assertion +// is vacuously true and the row proves nothing. +type crossSessionAttempt struct { + script string + forbid []string +} + +// sweepScript builds a recursive content search the guest can actually run. The +// guest image ships bash 5.3 and awk but NO grep and NO find, so a `grep -r` +// row exits 127 without searching anything — a vacuous pass that looks like +// confinement. bash's globstar walks the trees and awk does the matching, and +// the exit status mirrors grep's: 0 when the needle was found, 1 when it was +// not, so a caller can still assert the non-zero exit a confined command owes. +// +// Matching files are printed as `:`, so BOTH discriminators are +// live: the secret body appears in the output if any file's content was read, +// and the path appears if a file under another tenant's volume was reachable at +// all. +// +// Two things stop the sweep from finding ITS OWN needle, which would be a false +// escape report rather than a real one: +// +// - The needle travels in an EXPORTED ENV VAR, never in argv. Passed as +// `awk -v`, it lands in the searcher's own /proc/self/cmdline, so the sweep +// matches the string it is looking for in its own command line. +// - /proc, /sys and /dev are skipped. They are synthetic kernel interfaces +// that cannot hold another tenant's volume, so excluding them removes the +// self-match surface (the environ/cmdline of the running searcher) without +// narrowing what the row actually probes. +func sweepScript(needle, roots string) string { + return "export SWEEP_NEEDLE='" + needle + "'; " + + "shopt -s globstar nullglob dotglob; found=1; " + + "for root in " + roots + "; do " + + "for f in \"$root\"/**/*; do " + + // Collapse repeated slashes before matching: a "/" root globs to + // "//proc/self/environ", which a /proc/* pattern does NOT match — the + // sweep would then read its own environ and report finding the needle + // it was given, a false escape. + "n=$f; while [[ $n == //* ]]; do n=${n#/}; done; " + + "case $n in /proc/*|/sys/*|/dev/*) continue;; esac; " + + "[[ -f $f && -r $f ]] || continue; " + + "if awk 'index($0, ENVIRON[\"SWEEP_NEEDLE\"]) { print FILENAME \":\" $0; hit=1 } END { exit !hit }' \"$f\" 2>/dev/null; then found=0; fi; " + + "done; done 2>/dev/null; exit $found" +} + +// TestMicroVMSweepScriptFindsItsNeedle is the non-vacuity control for +// sweepScript itself: pointed at a tree that DOES contain the needle, it must +// find it and exit 0. Without this the cross-tenant sweep row could pass +// because the search is broken rather than because the volume is unreachable — +// exactly the failure mode that made the original `grep -r` row worthless. +func TestMicroVMSweepScriptFindsItsNeedle(t *testing.T) { + env := microvmtest.Require(t) + m, id, _ := isolationSession(t, env, "iso-sweep-control") + + const needle = "SWEEP-CONTROL-CANARY-2d7f4a91" + if out, code := guestSh(t, m, id, "mkdir -p /workspace/deep/nested && printf '%s' '"+needle+"' > /workspace/deep/nested/planted.txt"); code != 0 { + t.Fatalf("planting the sweep control canary: exit %d, %q", code, out) + } + out, code := guestSh(t, m, id, sweepScript(needle, "/workspace")) + if code != 0 { + t.Fatalf("sweepScript did not find a needle planted in its own search root (exit %d, %q); "+ + "the cross-tenant sweep row would pass vacuously", code, truncate(out)) + } + if !strings.Contains(out, needle) { + t.Fatalf("sweepScript exited 0 but its output %q does not carry the needle; the content check would be vacuous", truncate(out)) + } + if !strings.Contains(out, "planted.txt") { + t.Errorf("sweepScript output %q does not name the matching path; the path check would be vacuous", truncate(out)) + } + t.Logf("sweep control: found the planted canary -> exit %d, %q", code, strings.TrimSpace(truncate(out))) +} + // TestMicroVMVolumeTraversalConfined is the path-traversal leg: the guest tries // to escape /workspace three ways and is confined every time, proven from both // sides of the boundary. @@ -224,20 +299,56 @@ func TestMicroVMCrossSessionVolumeUnreachable(t *testing.T) { t.Fatalf("tenant B cannot read its OWN volume (exit %d, %q); the cross-tenant negative would be vacuous", code, out) } - // Every way A could try to name B's volume. - attempts := map[string]string{ - "B's absolute host volume path": "cat " + filepath.Join(volumeB, "host-secret.txt") + " " + filepath.Join(volumeB, "guest-secret.txt"), - "B's volume dir listing": "ls -la " + volumeB, - "traversal toward B": "cat /workspace/../volume/host-secret.txt; cat /workspace/../../*/volume/*secret*", - "a symlink A plants to B": "ln -sf " + volumeB + " /workspace/b-link && cat /workspace/b-link/host-secret.txt", - "the whole host tmp tree": "grep -rl 'TENANT-B-SECRET' / 2>/dev/null | head -5", - } - for name, script := range attempts { + // Every way A could try to name B's volume. Each row's success condition + // matches WHAT ITS COMMAND EMITS, which the secret-body check alone does + // not: `ls` prints names and never file content, so a Contains(secret) on + // an `ls` is unconditionally true and proves nothing whether B's volume is + // reachable or not. Every row also asserts the NON-ZERO EXIT the traversal + // leg asserts — a confined command must fail, not merely print nothing. + attempts := map[string]crossSessionAttempt{ + "B's absolute host volume path": { + script: "cat " + filepath.Join(volumeB, "host-secret.txt") + " " + filepath.Join(volumeB, "guest-secret.txt"), + }, + "B's volume dir listing": { + // An `ls` that SUCCEEDED and listed B's secrets would pass a + // content check; the discriminator here is the FILENAMES plus the + // exit code. + script: "ls -la " + volumeB, + forbid: []string{"host-secret.txt", "guest-secret.txt"}, + }, + "traversal toward B": { + script: "cat /workspace/../volume/host-secret.txt; cat /workspace/../../*/volume/*secret*", + }, + "a symlink A plants to B": { + script: "ln -sf " + volumeB + " /workspace/b-link && cat /workspace/b-link/host-secret.txt", + }, + "a content sweep of every tree A can name": { + // NOT `grep -r`: the guest image ships no grep (and no find), so + // that row exited 127 without ever searching — vacuous twice over, + // once for printing paths instead of content and once for never + // running. This is the same sweep in what the guest DOES have + // (bash globstar + awk), and sweepScript keeps grep's exit + // semantics: non-zero when nothing matched. + script: sweepScript(tenantBSecret, "/ /tmp /mnt /media /run /var /home /workspace"), + forbid: []string{volumeB}, + }, + } + for name, attempt := range attempts { t.Run("A cannot reach "+name, func(t *testing.T) { - out, code := guestSh(t, mA, idA, script) + out, code := guestSh(t, mA, idA, attempt.script) if strings.Contains(out, tenantBSecret) { t.Fatalf("tenant A READ tenant B's secret via %s — CROSS-TENANT ESCAPE.\noutput: %q", name, out) } + for _, forbidden := range attempt.forbid { + if strings.Contains(out, forbidden) { + t.Fatalf("tenant A's %s NAMED %q — tenant B's volume is reachable from A.\noutput: %q", + name, forbidden, truncate(out)) + } + } + if code == 0 { + t.Errorf("cross-tenant attempt %q exited 0 (output %q); a confined command must fail", + name, truncate(out)) + } t.Logf("unreachable: %s -> exit %d, %q", name, code, strings.TrimSpace(truncate(out))) }) } @@ -307,8 +418,8 @@ func TestMicroVMHostOwnershipParity(t *testing.T) { entry, gotUID, gotGID, wantUID, wantGID) if gotUID != wantUID || gotGID != wantGID { t.Errorf("host-ownership PARITY BROKEN for %s: guest-authored file is %d:%d, "+ - "but the podman --userns=keep-id path yields %d:%d — virtiofsd's uid/gid translation "+ - "(launch.go --translate-uid/--translate-gid) must map the in-guest agent id to the invoking host user", + "but the podman --userns=keep-id path yields %d:%d — virtiofsd's uid/gid mapping "+ + "(launch.go --uid-map/--gid-map) must map the in-guest agent id to the invoking host user", entry, gotUID, gotGID, wantUID, wantGID) } } @@ -326,12 +437,19 @@ func TestMicroVMHostOwnershipParity(t *testing.T) { "the translation must leave the invoking user's files owned by the in-guest agent", code, out) } t.Logf("in-guest view of a host-authored file (uid gid): %q", strings.TrimSpace(out)) - if want := strings.Fields(strings.TrimSpace(out)); len(want) >= 2 { - agentID := strconv.Itoa(int(agentuid.AgentUID)) - if want[0] != agentID || want[1] != agentID { - t.Errorf("a host-authored workspace file appears in-guest as %s:%s, want the agent id %s:%s — "+ - "the agent would not own its own checkout", want[0], want[1], agentID, agentID) - } + // A malformed probe must FAIL, not skip: a `stat` variant emitting fewer + // than two fields would otherwise leave this whole direction unasserted + // while the test passed green. + fields := strings.Fields(strings.TrimSpace(out)) + // The guest's stat output is followed by nothing on success, but the append + // leg shares the exec, so take exactly the first line's two fields. + if len(fields) < 2 { + t.Fatalf("stat output %q did not yield uid+gid; the host->guest ownership direction was NOT asserted", out) + } + agentID := strconv.Itoa(int(agentuid.AgentUID)) + if fields[0] != agentID || fields[1] != agentID { + t.Errorf("a host-authored workspace file appears in-guest as %s:%s, want the agent id %s:%s — "+ + "the agent would not own its own checkout", fields[0], fields[1], agentID, agentID) } // The host-side owner of that file must be unchanged by the guest's append: // a translation that rewrote ownership on write would silently reassign the diff --git a/go/internal/runtime/microvm_preflight.go b/go/internal/runtime/microvm_preflight.go index 148ae85b..ba9c7455 100644 --- a/go/internal/runtime/microvm_preflight.go +++ b/go/internal/runtime/microvm_preflight.go @@ -45,6 +45,11 @@ type preflightProbes struct { // provisioned rootless, so a fake reading is the only way this decision is // covered on a dev box. readQuota quotaReadFn + // verifySubordinateIDs resolves the invoking user's /etc/subuid range, the + // host allocation newuidmap validates virtiofsd's uid/gid mapping against. + // Behind the seam so the axis is testable on a box whose own subuid file + // cannot be arranged to fail. + verifySubordinateIDs func() error } // defaultPreflightProbes wires the real host-facing implementations behind the @@ -74,8 +79,9 @@ func defaultPreflightProbes() preflightProbes { _ = f.Close() return nil }, - hashImage: hashFileSHA256, - readQuota: readVolumeQuota, + hashImage: hashFileSHA256, + readQuota: readVolumeQuota, + verifySubordinateIDs: microvm.VerifySubordinateIDRange, } } @@ -121,37 +127,67 @@ func (m *MicroVMRuntime) verifyMicroVMSupport(ctx context.Context, probes prefli return err } - // 5. Session-volume quota (D7): under the multi-tenant profile an + // 5. Subordinate id range: virtiofsd's uid/gid mapping is validated by + // newuidmap against the invoking user's /etc/subuid entry, so a host with + // no subordinate range must fail HERE with the fix named — not at the first + // session boot, where virtiofsd dies before binding its socket and the + // cause surfaces only as "waiting for daemon sockets". + if err := probes.verifySubordinateIDs(); err != nil { + return fmt.Errorf("microvm preflight: %w", err) + } + + // 6. Session-volume quota (D7): under the multi-tenant profile an // operator-provisioned project quota MUST be active on the session-volume // filesystem, or startup fails naming the fix. Otherwise the observed // utilization is logged and nothing gates. return m.verifyQuota(probes) } -// verifyQuota runs the D7 volume-quota check against the session-volume -// filesystem. The per-session volume dir is not known at startup (P2's volume -// lifecycle mints it per session), so the check targets the RunRoot — the one -// startup-known path on the same host filesystem tree the Runner writes session -// state into, which is what "the session-volume filesystem" means at preflight -// time. Verification is read-only and rootless (microvm_quota.go); the Runner -// never assigns a quota. +// verifyQuota runs the D7 volume-quota check against the SESSION-VOLUME +// filesystem, which is VolumeRoot — the parent dir P2's volume lifecycle mints +// per-session volumes under. It is deliberately NOT the RunRoot: that is the +// socket dir, held to a short /tmp path by the AF_UNIX sun_path budget, so a +// verdict read there is routinely about a different filesystem than the one +// sessions consume. Verification is read-only and rootless (microvm_quota.go); +// the Runner never assigns a quota. // -// QuotaRequired unset (Dogfood, single trusted tenant) never fails: an absent -// quota is the documented posture there, so the reading is logged — including -// the utilization V7 will meter — and startup proceeds. No meter is registered -// here; the coherent metric set is V7's. +// FAIL CLOSED when QuotaRequired is set and VolumeRoot is unknown: reporting a +// verdict about a filesystem that was never probed is worse than refusing, since +// it passes the multi-tenant gate on evidence from an unrelated mount. With +// QuotaRequired unset (Dogfood, single trusted tenant) an unknown volume root is +// logged and startup proceeds — an absent quota is the documented posture there. +// +// No meter is registered here; the coherent metric set is V7's. func (m *MicroVMRuntime) verifyQuota(probes preflightProbes) error { - reading, err := verifyVolumeQuota(m.config.RunRoot, VolumeQuota{}, probes.readQuota) + if m.config.VolumeRoot == "" { + if m.config.QuotaRequired { + return errors.New( + "microvm preflight: session-volume quota is required, but the session-volume filesystem " + + "cannot be identified at startup; set --microvm-volume-root or $COMPASS_MICROVM_VOLUME_ROOT " + + "to the parent dir session volumes are minted under (the run-root is the socket dir and is " + + "routinely a different filesystem, so it is not a valid proxy)") + } + slog.Warn("microvm preflight: session-volume quota is not verified; no volume root is configured", + "fix", "set --microvm-volume-root or $COMPASS_MICROVM_VOLUME_ROOT") + return nil + } + reading, err := verifyVolumeQuota(m.config.VolumeRoot, VolumeQuota{}, probes.readQuota) if err != nil { if m.config.QuotaRequired { return err } slog.Warn("microvm preflight: session-volume quota is not verified; the single-tenant profile ships no host-enforced quota", - "run_root", m.config.RunRoot, "reason", err) + "volume_root", m.config.VolumeRoot, "reason", err) return nil } + // used_ratio is logged ONLY on an active bound, where its denominator is the + // project's own limit and its numerator the project's own usage. With no + // quota projected, statfs reports the whole filesystem, so the same + // expression would silently mean "how full is the host disk" — a different + // number under one key. The raw pair is logged instead, so V7 inherits a + // single-meaning ratio (microvm_quota_linux.go on the dual meaning). slog.Info("microvm preflight: session-volume project quota is active", - "run_root", m.config.RunRoot, + "volume_root", m.config.VolumeRoot, "limit_bytes", reading.LimitBytes, "used_bytes", reading.UsedBytes, "used_ratio", reading.UsedRatio(), diff --git a/go/internal/runtime/microvm_preflight_test.go b/go/internal/runtime/microvm_preflight_test.go index 2709bf5d..67e7a6ac 100644 --- a/go/internal/runtime/microvm_preflight_test.go +++ b/go/internal/runtime/microvm_preflight_test.go @@ -45,6 +45,9 @@ func okProbes() preflightProbes { FilesystemBytes: 1 << 40, FilesystemInodes: 1 << 26, }, nil }, + // The invoking user's /etc/subuid range: all-green by default, so only + // the row exercising that axis fails it. + verifySubordinateIDs: func() error { return nil }, } } @@ -75,6 +78,10 @@ func okConfig(t *testing.T) MicroVMConfig { RootfsImage: rootfs, InitrdImage: initrd, RunRoot: runRoot, + // A VolumeRoot distinct from RunRoot is the production shape: the + // session-volume filesystem is the durable D9 volume, while RunRoot is + // the short /tmp socket dir. The quota axis targets THIS path. + VolumeRoot: dir, } } @@ -219,6 +226,19 @@ func TestVerifyMicroVMSupport(t *testing.T) { }, wantParts: []string{"not creatable/writable"}, }, + { + // The /etc/subuid axis: virtiofsd's mapping is validated by + // newuidmap against the invoking user's subordinate range, so a + // host without one must fail HERE rather than at the first boot as + // an opaque daemon-socket timeout. + name: "no subordinate uid range for the invoking user", + mutate: func(_ *MicroVMConfig, p *preflightProbes) { + p.verifySubordinateIDs = func() error { + return errors.New("virtiofsd id-mapping requires a subordinate uid range for mattw in /etc/subuid") + } + }, + wantParts: []string{"/etc/subuid", "subordinate uid range"}, + }, }) } @@ -279,6 +299,53 @@ func TestVerifyMicroVMSupportQuota(t *testing.T) { }, wantOK: true, }, + { + // FAIL CLOSED: with no VolumeRoot the session-volume filesystem is + // unidentified, and the RunRoot is NOT an acceptable proxy — it is + // the short /tmp socket dir, routinely a different filesystem. A + // verdict about a filesystem that was never probed is worse than a + // refusal, so the required profile refuses. + name: "quota required but the volume root is unknown fails closed", + mutate: func(cfg *MicroVMConfig, p *preflightProbes) { + cfg.QuotaRequired = true + cfg.VolumeRoot = "" + p.readQuota = func(string) (QuotaReading, error) { + t.Error("readQuota must NOT be called with no volume root: probing the run-root proxy " + + "would report a verdict about a filesystem the session volumes do not live on") + return QuotaReading{}, nil + } + }, + wantParts: []string{"cannot be identified at startup", "--microvm-volume-root", "COMPASS_MICROVM_VOLUME_ROOT"}, + }, + { + name: "no volume root without required set passes with a warning", + mutate: func(cfg *MicroVMConfig, _ *preflightProbes) { + cfg.VolumeRoot = "" + }, + wantOK: true, + }, + { + // The probe must target VolumeRoot, never RunRoot: they are + // different filesystems in production, and a quota verdict read on + // the socket dir says nothing about the durable volume. + name: "the quota probe targets the volume root, not the run root", + mutate: func(cfg *MicroVMConfig, p *preflightProbes) { + cfg.QuotaRequired = true + wantPath := cfg.VolumeRoot + runRoot := cfg.RunRoot + p.readQuota = func(path string) (QuotaReading, error) { + if path != wantPath { + t.Errorf("readQuota probed %q, want the volume root %q (run root is %q)", path, wantPath, runRoot) + } + return QuotaReading{ + Path: path, MountRoot: "/", + LimitBytes: 10 << 30, UsedBytes: 1 << 30, + FilesystemBytes: 1 << 40, + }, nil + } + }, + wantOK: true, + }, }) } diff --git a/go/internal/runtime/microvm_quota.go b/go/internal/runtime/microvm_quota.go index 8654ce9a..3412de7c 100644 --- a/go/internal/runtime/microvm_quota.go +++ b/go/internal/runtime/microvm_quota.go @@ -75,7 +75,13 @@ type QuotaReading struct { UsedInodes int64 // FilesystemBytes is the block total at MountRoot — the unprojected size. FilesystemBytes int64 - // FilesystemInodes is the inode total at MountRoot — unprojected. + // FilesystemInodes is the inode total at MountRoot — unprojected, and on + // XFS an ESTIMATE rather than a fixed filesystem property: xfs_statfs_inodes + // computes f_files = min(icount + fakeinos, XFS_MAXINUMBER) where + // fakeinos = XFS_FSB_TO_INO(mp, f_bfree) (fs/xfs/xfs_super.c), because XFS + // allocates inodes on demand — so it SHRINKS as the filesystem fills and is + // read at a different instant than LimitInodes. Active()'s inode arm carries + // a margin for exactly this. FilesystemInodes int64 } @@ -90,17 +96,54 @@ type QuotaReading struct { // and reads as absent. That is a bound which constrains nothing, and reading it // as absent fails a QuotaRequired startup closed (a legible operator refusal) // rather than passing a tenant-exhaustible volume as bounded. +// +// The INODE arm requires a MARGIN, not merely strict inequality, because the +// mount-root inode total it compares against is an XFS estimate that moves +// between the two statfs calls (FilesystemInodes' doc). Bare `<` on two samples +// of one dynamic number can go true from jitter alone — an unquota'd volume +// reading as bounded, the fail-OPEN direction on a security-relevant preflight. +// A real project inode limit is orders of magnitude below the estimate, so the +// margin costs nothing there. +// +// The arm stays INDEPENDENT of the byte arm rather than being gated behind it: +// an inode-only project quota on a filesystem whose byte limit happens to equal +// its size is a real bound, and gating would read it as absent. The residual +// case the margin cannot rescue — a nearly-full XFS whose shrinking estimate +// falls within the margin of a genuine project inode limit — reads as absent and +// therefore REFUSES a QuotaRequired startup with the observed numbers in the +// message, which is the fail-closed direction. func (r QuotaReading) Active() bool { if r.LimitBytes > 0 && r.FilesystemBytes > 0 && r.LimitBytes < r.FilesystemBytes { return true } - return r.LimitInodes > 0 && r.FilesystemInodes > 0 && r.LimitInodes < r.FilesystemInodes + if r.LimitInodes <= 0 || r.FilesystemInodes <= 0 { + return false + } + return r.LimitInodes < r.FilesystemInodes-r.FilesystemInodes/inodeMarginDivisor } +// inodeMarginDivisor sets how much smaller than the mount-root inode estimate a +// path's inode total must be before it counts as a projected bound: 1/16 (6.25%) +// of the estimate. It is a jitter floor, not a tuned threshold — adjacent statfs +// samples of XFS's fakeinos estimate differ by far less, while a provisioned +// project inode limit is smaller by orders of magnitude. +const inodeMarginDivisor = 16 + // UsedRatio is the observed byte utilization in [0,1] — the value the preflight // logs and V7's compass_microvm_quota_used_ratio will meter. Zero when no limit -// was observed. NOTE: this file registers NO meter; exposing the number is V6's -// job, owning the metric set is V7's. +// was observed. +// +// DUAL MEANING, and the caller must gate on Active(): under an enforced project +// quota the kernel has rewritten both totals to the project's own limit and +// usage, so this is the tenant's utilization; with no quota projected the same +// expression is whole-FILESYSTEM utilization, including every other tenant's +// data and the OS's. Logging both under one key would give a dashboard a number +// whose denominator silently changes, so the preflight logs used_ratio ONLY when +// Active() is true and the raw used/total pair otherwise (microvm_preflight.go's +// verifyQuota). V7 inherits a single-meaning number. +// +// NOTE: this file registers NO meter; exposing the number is V6's job, owning +// the metric set is V7's. func (r QuotaReading) UsedRatio() float64 { if r.LimitBytes <= 0 { return 0 diff --git a/go/internal/runtime/microvm_quota_linux.go b/go/internal/runtime/microvm_quota_linux.go index 29dc2fec..e6b0484c 100644 --- a/go/internal/runtime/microvm_quota_linux.go +++ b/go/internal/runtime/microvm_quota_linux.go @@ -3,10 +3,12 @@ package runtime // The syscall half of V6's quota verification: the rootless statfs(2) probe -// microvm_quota.go's pure decision consumes. Linux-only because the -// project-quota-projected statvfs behavior it reads is a Linux XFS/ext4 kernel -// property (xfs_qm_statvfs / ext4_statfs_project); microvm_quota_unsupported.go -// carries the named refusal for every other unix. +// microvm_quota.go's pure decision consumes. Mechanism and the rejected +// alternatives (quotactl, FS_IOC_FSGETXATTR): microvm_quota.go's header. +// +// Linux-only because the project-quota-projected statvfs behavior it reads is a +// Linux XFS/ext4 kernel property (xfs_qm_statvfs / ext4_statfs_project); +// microvm_quota_unsupported.go carries the named refusal for every other unix. import ( "fmt" @@ -35,8 +37,14 @@ func readVolumeQuota(path string) (QuotaReading, error) { return QuotaReading{}, fmt.Errorf("statfs mount root %q: %w", root, err) } return QuotaReading{ - Path: path, - MountRoot: root, + Path: path, + MountRoot: root, + // LimitBytes/UsedBytes carry a DUAL MEANING by construction, per + // QuotaReading's field docs: under an enforced project quota the kernel + // has rewritten these totals to the project's own limit and usage; with + // no quota projected they are the whole filesystem's. Active() is what + // distinguishes the two, and it is why the preflight logs used_ratio + // only when Active() is true. LimitBytes: blocksToBytes(at.Blocks, at.Bsize), UsedBytes: blocksToBytes(at.Blocks-at.Bfree, at.Bsize), LimitInodes: int64(at.Files), //nolint:gosec // G115: a statfs inode count is a kernel-reported magnitude, never near the int64 ceiling @@ -67,6 +75,18 @@ func blocksToBytes(blocks uint64, bsize int64) int64 { // Paths are resolved through symlinks first: a symlinked volume dir would // otherwise walk the link's lexical parents, which may live on a different // filesystem entirely and make the comparison meaningless. +// +// An UNREADABLE ancestor is an INCONCLUSIVE probe, not a mount root. The +// comparison is only sound against an UNPROJECTED reference, and same-device is +// not the same as unprojected: FS_XFLAG_PROJINHERIT propagates a project id down +// the tree, so a walk halted by an EACCES may stop at an ancestor inside the +// SAME project (or inside a larger enclosing one), whose totals are themselves +// rewritten. Comparing against that yields a bogus verdict — reporting a quota +// active because the reference happened to be a bigger project, which is the +// fail-OPEN direction on a security-relevant preflight. So the error propagates +// through readVolumeQuota, and a QuotaRequired startup fails CLOSED naming the +// unreadable ancestor (the posture TestReadVolumeQuotaAbsentPath pins for a +// missing path). func mountRoot(path string) (string, error) { resolved, err := filepath.EvalSymlinks(path) if err != nil { @@ -85,11 +105,12 @@ func mountRoot(path string) (string, error) { } parentDev, err := deviceOf(parent) if err != nil { - // An unreadable ancestor (a 0711 parent an unprivileged Runner - // cannot stat) is not a quota verdict: the deepest ancestor proven - // to be on this device is the best reference available, and the - // comparison against it stays sound. - return current, nil //nolint:nilerr // a stat-blocked ancestor bounds the walk; `current` is still a same-device reference, so this is the answer, not a swallowed failure + return "", fmt.Errorf( + "locating the mount root of %q: ancestor %q is not statable (%w), so no unprojected "+ + "reference point could be reached and whether a project quota scopes the volume is "+ + "INDETERMINATE; make the ancestor path traversable by the Runner uid (chmod o+x) "+ + "or place the volume root on a path the Runner can walk to its mount point", + path, parent, err) } if parentDev != dev { return current, nil diff --git a/go/internal/runtime/microvm_quota_linux_test.go b/go/internal/runtime/microvm_quota_linux_test.go new file mode 100644 index 00000000..16c8eeee --- /dev/null +++ b/go/internal/runtime/microvm_quota_linux_test.go @@ -0,0 +1,106 @@ +//go:build linux + +package runtime + +// Tests for the Linux-only statfs/mount-root half of V6's quota verification +// (microvm_quota_linux.go). Mechanism and the rejected alternatives: +// microvm_quota.go's header. +// +// These are separate from microvm_quota_test.go because mountRoot and deviceOf +// only exist under //go:build linux — the pure decision they feed is covered +// there, on every GOOS. + +import ( + "os" + "path/filepath" + "strings" + "testing" +) + +// TestMountRootUnreadableAncestorIsInconclusive pins the fail-CLOSED semantics +// for a stat-blocked ancestor: the walk cannot reach an UNPROJECTED reference, +// and same-device is not the same as unprojected — FS_XFLAG_PROJINHERIT means a +// halted walk may stop inside the same project (or a larger enclosing one), +// whose totals are themselves rewritten. Comparing against that would report a +// bogus ACTIVE quota, the fail-open direction. So mountRoot returns an error +// naming the blocking path instead of a mount root, and no verdict is rendered. +// +// A 0000 ancestor is refused at path resolution rather than at the ancestor +// walk, since an EACCES that blocks stat(parent) also blocks resolving the leaf +// through it — both routes are errors, which is the point: the ONE thing that +// must never happen is a QuotaReading built against an unproven reference. +func TestMountRootUnreadableAncestorIsInconclusive(t *testing.T) { + if os.Getuid() == 0 { + t.Skip("running as root: a 0000 dir is still traversable, so the EACCES this test needs cannot be produced") + } + base := t.TempDir() + blocked := filepath.Join(base, "blocked") + volume := filepath.Join(blocked, "volume") + if err := os.MkdirAll(volume, 0o700); err != nil { + t.Fatalf("creating volume tree: %v", err) + } + if err := os.Chmod(blocked, 0o000); err != nil { + t.Fatalf("blocking ancestor: %v", err) + } + // Restore the mode so t.TempDir's cleanup can remove the tree. + t.Cleanup(func() { _ = os.Chmod(blocked, 0o700) }) + + root, err := mountRoot(volume) + if err == nil { + t.Fatalf("mountRoot(%q) = %q with no error; a stat-blocked ancestor must be an INCONCLUSIVE probe, "+ + "not a mount root — comparing against a possibly-projected ancestor can report a bogus active quota", volume, root) + } + if root != "" { + t.Errorf("mountRoot returned the reference %q alongside its error; an inconclusive probe must yield no reference", root) + } + // The error must name the path an operator has to fix. + if !strings.Contains(err.Error(), blocked) { + t.Errorf("error %q does not name the blocking path %q", err.Error(), blocked) + } +} + +// TestMountRootResolvesAnUnblockedPath is the positive control for the walk: on +// a path the Runner can traverse to its mount point, mountRoot still resolves a +// real reference. Without it the fail-closed assertion above could pass on a +// mountRoot that had simply stopped working. +// +// The ancestor-walk's own error branch is not reachable through file modes: an +// EACCES that blocks stat(parent) necessarily blocks resolving the leaf through +// that same parent, so EvalSymlinks refuses first (the case above). The branch +// stays because a non-permission stat failure — an ancestor unlinked mid-walk, +// an EIO — must fail closed rather than return a same-device guess. +func TestMountRootResolvesAnUnblockedPath(t *testing.T) { + root, err := mountRoot(t.TempDir()) + if err != nil { + t.Fatalf("mountRoot on an unblocked temp dir = %v, want a resolved mount root", err) + } + if root == "" { + t.Fatal("mountRoot resolved an empty mount root on an unblocked path") + } +} + +// TestReadVolumeQuotaPropagatesInconclusiveMountRoot is the same posture one +// layer up: readVolumeQuota must NOT swallow the inconclusive mount-root probe +// into a reading, because a reading is what Active() renders a verdict from. An +// error here is what makes a QuotaRequired startup fail closed. +func TestReadVolumeQuotaPropagatesInconclusiveMountRoot(t *testing.T) { + if os.Getuid() == 0 { + t.Skip("running as root: a 0000 dir is still traversable, so the EACCES this test needs cannot be produced") + } + base := t.TempDir() + blocked := filepath.Join(base, "blocked") + volume := filepath.Join(blocked, "volume") + if err := os.MkdirAll(volume, 0o700); err != nil { + t.Fatalf("creating volume tree: %v", err) + } + if err := os.Chmod(blocked, 0o000); err != nil { + t.Fatalf("blocking ancestor: %v", err) + } + t.Cleanup(func() { _ = os.Chmod(blocked, 0o700) }) + + reading, err := readVolumeQuota(volume) + if err == nil { + t.Fatalf("readVolumeQuota(%q) = %s with no error; an unreachable mount root must propagate so a "+ + "required-quota startup fails closed with the real cause", volume, reading) + } +} diff --git a/go/internal/runtime/microvm_quota_test.go b/go/internal/runtime/microvm_quota_test.go index c9aaa534..422b88a4 100644 --- a/go/internal/runtime/microvm_quota_test.go +++ b/go/internal/runtime/microvm_quota_test.go @@ -3,13 +3,14 @@ package runtime // Hermetic unit tests for V6's volume-quota VERIFICATION (microvm_quota.go). +// Mechanism and the rejected alternatives: microvm_quota.go's header. +// // These carry NO microvm build tag on purpose: they are what genuinely proves // the verification logic on any box, including one where a prjquota-active -// filesystem cannot be provisioned (that needs root — loop + mkfs.xfs -o -// prjquota + a project id + limits — which the rootless Runner and this dev box -// both lack, D7 / Global Constraint "Rootless is hard"). The decision is split -// from the syscall behind quotaReadFn precisely so it is covered here rather -// than left to a leg that skips. +// filesystem cannot be provisioned (that needs root, D7 / Global Constraint +// "Rootless is hard"). The decision is split from the syscall behind +// quotaReadFn precisely so it is covered here rather than left to a leg that +// skips. // // The real statfs probe (readVolumeQuota) is exercised too, but only for what is // honestly assertable without a quota'd filesystem: that it reads a real path, @@ -99,6 +100,44 @@ func TestQuotaReadingActive(t *testing.T) { }, want: false, }, + { + // The XFS fakeinos case (FilesystemInodes' doc): the mount-root + // inode total is an estimate that SHRINKS as the filesystem fills, + // so on a full-ish XFS it can dip just below a generous project + // inode limit. Bare `<` would read that jitter as a projected + // bound, which is the fail-OPEN direction; the margin rejects it. + name: "inode arm where the mount-root estimate shrank to just above the project limit", + reading: QuotaReading{ + LimitBytes: 1 << 40, UsedBytes: 1 << 39, + LimitInodes: 1_000_000, UsedInodes: 900_000, + FilesystemBytes: 1 << 40, FilesystemInodes: 1_010_000, + }, + want: false, + }, + { + // Just INSIDE the 1/16 margin: 940_000 < 1_010_000 - 63_125. This + // pins the boundary, so a change to inodeMarginDivisor is a + // deliberate edit rather than a silent widening. + name: "an inode bound past the margin is active", + reading: QuotaReading{ + LimitBytes: 1 << 40, UsedBytes: 1 << 39, + LimitInodes: 940_000, UsedInodes: 900_000, + FilesystemBytes: 1 << 40, FilesystemInodes: 1_010_000, + }, + want: true, + }, + { + // An inode-only bound must NOT be gated behind the byte arm: here + // the byte limit equals the filesystem size (so the byte arm is + // silent) while a real inode bound is projected. + name: "an inode-only bound is active even when the byte limit equals the filesystem size", + reading: QuotaReading{ + LimitBytes: 1 << 40, UsedBytes: 1 << 30, + LimitInodes: 1 << 20, UsedInodes: 512, + FilesystemBytes: 1 << 40, FilesystemInodes: 1 << 26, + }, + want: true, + }, { name: "the zero reading is not active", reading: QuotaReading{}, diff --git a/go/internal/runtime/microvm_quota_unsupported.go b/go/internal/runtime/microvm_quota_unsupported.go index b9e087b5..5a3ad449 100644 --- a/go/internal/runtime/microvm_quota_unsupported.go +++ b/go/internal/runtime/microvm_quota_unsupported.go @@ -3,11 +3,15 @@ package runtime // The non-Linux leg of V6's quota probe. The verification mechanism is a Linux -// kernel property (XFS/ext4 project-quota-projected statvfs, see -// microvm_quota.go's header), and the microVM backend only ever boots on a -// KVM-capable Linux host — but the runtime package must still type-check -// everywhere (the darwin CI lane compiles it, microvm.go's header), so the seam -// gets a named refusal rather than a build break or a silent "quota active". +// kernel property (XFS/ext4 project-quota-projected statvfs); mechanism and the +// rejected alternatives: microvm_quota.go's header. +// +// The microVM backend only ever boots on a KVM-capable Linux host, but this +// seam must still RESOLVE on every GOOS the package is built for: +// microvm_quota.go is untagged, so readVolumeQuota — the function its +// quotaReadFn seam is wired to — has to exist under every build constraint or +// the package does not build off Linux at all. Hence a named refusal rather +// than a build break or a silent "quota active". // // Refusing is the fail-closed answer: QuotaRequired on a platform where the // bound cannot be observed must be a legible startup error, never a pass. From 3b798cc9da5b86b4125f4188b0db4adbb5d71ae3 Mon Sep 17 00:00:00 2001 From: mintaka Date: Sun, 6 Sep 2026 00:06:18 -0400 Subject: [PATCH 3/3] =?UTF-8?q?fix(microvm):=20fold=20V6=20review=20round?= =?UTF-8?q?=202=20=E2=80=94=20subgid=20divergence,=20daemon=20liveness,=20?= =?UTF-8?q?mount-point=20quota=20degeneracy=20(RIG-2497)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Round 2 of the #912 review returned 2 highs / 4 mediums / 5 lows, all folded here. The two highs are defects the round-1 fix code itself introduced or left latent, and both fail in the direction that matters: silently, on a host that differs from this one. ## Fail-silent fixes (the highs) - **`/etc/subgid` is a separate allocation.** Round 1 correctly stopped hardcoding the subordinate base, but read only `/etc/subuid` and fed that one value into *both* `--uid-map` and `--gid-map` — while `newgidmap` validates the gid range against `/etc/subgid`, an independently-editable file (`usermod --add-subuids` and `--add-subgids` are separate flags; the code's own remediation text conceded they can diverge). A host whose two ranges differ would die in virtiofsd's gid map at first boot with no useful diagnostic. Both files are now read independently through a path-injected core, the two arms carry their own bases, and `VerifySubordinateIDRange` resolves both so a missing subgid range fails at *startup* naming the file and the fix. The false "allocated in lockstep" claim is gone. This box cannot observe the bug (both files read `mattw:100000:65536`), so the regression test injects divergent bases as fixtures rather than reading the host. - **Readiness ignored liveness.** `waitForSockets` polled only for socket-path existence, and virtiofsd binds its socket *before* the id-map step that can fail — so any mapping failure left the socket on disk, readiness returned nil, and cloud-hypervisor was started against a dead virtiofsd, surfacing as an inscrutable vhost-user error instead of virtiofsd's own message. Readiness is now liveness-aware and its error carries the daemon's name and log tail. Doing this properly required fixing the supervision model underneath it: only the VMM had a reaper, virtiofsd and passt were `Wait`ed lazily during teardown, and liveness was tracked by an atomic set by whichever path happened to `Wait` first — so a poll-loop check would have raced a second `Wait`. There is now exactly one reaper per child, installed at spawn, with `waitErr` published before the `exited` channel closes as the happens-before edge. `Running()` checks exit *first*, since a zombie still answers `Signal(0)`. ## Correctness - **Quota verification refused startup on a correctly-provisioned host.** `mountRoot` never checked whether the path it was handed *is* the mount point — `mountRoot("/tmp") == "/tmp"` — so when the volume root is the mount point of a dedicated quota'd filesystem (the natural production layout), the probe compared a filesystem against itself and `Active()` was false by construction. That case is now a distinct inconclusive verdict naming the fix (point `--microvm-volume-root` at a subdirectory), not a false negative. The former "positive control" would have passed an identity-returning implementation; it now asserts the walk actually climbs and converges from two depths. - The empty-path error still named the retired `--microvm-runroot` knob, with its own test pinning the stale text — a defect regression-locked by its test. Both corrected. ## Test cost and honesty - The cross-session sweep forked one `awk` per file across the whole guest filesystem: ~72s against a hard 120s exec cap, i.e. a flake waiting for a loaded box. Batched (one `awk` per 200 files) and scoped to the trees that discriminate, it now runs in milliseconds. Batching made the existing control partly vacuous — it never fills a batch — so a cross-batch test covers the mid-loop flush and the accumulator, with needles planted in both the first and final batch. Every confined row is now timed against the cap so creep is caught rather than rediscovered. - The quota-fill leg wrote the entire project limit plus 64MiB, which against a realistic quota would blow the exec cap and report a timeout instead of the `EDQUOT` verdict it exists to prove. It now fills only remaining headroom plus margin, and refuses with a named reason when the volume is too large to be a purpose-sized test quota. Lows: pinned the `--modcaps` literal in a shared constant (virtiofsd silently accepts misspelled capability names, so the argv assertion is the only guard), reworded the provisioning hint so `100000` reads as an example rather than canonical, and documented `Active()`'s deliberate byte/inode asymmetry. `boolOrEnv`'s precedence is unchanged and now documented as a contract — an explicit `--microvm-quota-required=false` does not override an env true, which fails toward leaving the multi-tenant gate on — with a test pinning that direction. ## Verification Gates green on a real-KVM box: build + vet (default, `-tags microvm`, darwin `!linux` stub), hermetic `-race`, the KVM suite under `COMPASS_REQUIRE_MICROVM=1` (real boots), golangci-lint 0 issues both tag sets, nilaway clean both. The load-bearing tests are mutation-verified: reverting the liveness check to path-existence-only reproduces the finding's exact failure, and an identity-returning `mountRoot` fails the strengthened control while passing the old one. Spec-impact: none. Refs RIG-2497 Co-authored-by: Matt Wilkinson --- go/cmd/compass-runner/backend_flags_test.go | 31 +++ go/cmd/compass-runner/main.go | 17 ++ go/internal/runtime/microvm/launch.go | 219 ++++++++++----- .../runtime/microvm/launch_idmap_test.go | 250 ++++++++++++++++-- .../runtime/microvm/launch_teardown_test.go | 108 +++++++- go/internal/runtime/microvm/subuid.go | 162 ++++++++---- .../runtime/microvm_isolation_microvm_test.go | 195 +++++++++++++- go/internal/runtime/microvm_quota.go | 14 +- go/internal/runtime/microvm_quota_linux.go | 69 +++-- .../runtime/microvm_quota_linux_test.go | 132 ++++++++- go/internal/runtime/microvm_quota_test.go | 8 +- 11 files changed, 994 insertions(+), 211 deletions(-) diff --git a/go/cmd/compass-runner/backend_flags_test.go b/go/cmd/compass-runner/backend_flags_test.go index 8b2ec582..9c722308 100644 --- a/go/cmd/compass-runner/backend_flags_test.go +++ b/go/cmd/compass-runner/backend_flags_test.go @@ -132,6 +132,37 @@ func TestQuotaRequiredFlagWinsOverEnv(t *testing.T) { } } +// TestQuotaRequiredEnvTrueBeatsExplicitFlagFalse pins the OTHER direction of +// boolOrEnv's precedence, which its sibling above does not cover: an env true +// beats an explicitly-passed `--microvm-quota-required=false`. +// +// This is a DOCUMENTED CONTRACT, not a latent bug (boolOrEnv's doc). A false +// flag is indistinguishable from an unset one under the zero-value convention +// every knob here shares, and the asymmetry resolves in the SAFE direction: the +// input that loses is the one that would turn the multi-tenant quota gate OFF, +// so a conflicting pair leaves the gate ON. Pinning it means a future switch to +// flag.Visit — which WOULD honor the explicit false and thereby move the +// conflict resolution to fail-open — is a deliberate, reviewed change rather +// than an unnoticed regression. +func TestQuotaRequiredEnvTrueBeatsExplicitFlagFalse(t *testing.T) { + t.Setenv("COMPASS_MICROVM_QUOTA_REQUIRED", "true") + f, quotaRequired, _ := flagsFor() + // Explicitly false — which, under the zero-value convention, is + // indistinguishable from "not passed". + *quotaRequired = false + + cfg, err := f.backendConfig() + if err != nil { + t.Fatalf("backendConfig() = %v, want nil", err) + } + if !cfg.MicroVM.QuotaRequired { + t.Fatal("$COMPASS_MICROVM_QUOTA_REQUIRED=true lost to an explicit --microvm-quota-required=false; " + + "the documented contract is that env-true wins (a false flag is indistinguishable from an unset " + + "one, and the conflict must resolve with the multi-tenant gate ON, not off). To disable the gate " + + "on such a box, unset the environment variable") + } +} + // TestVolumeRootReachesConfig: --microvm-volume-root / its env fallback must // land on MicroVMConfig.VolumeRoot. It is what makes the quota probe target the // session-volume filesystem instead of the RunRoot socket dir, so an unwired diff --git a/go/cmd/compass-runner/main.go b/go/cmd/compass-runner/main.go index 58e1dc0e..77d0d0ff 100644 --- a/go/cmd/compass-runner/main.go +++ b/go/cmd/compass-runner/main.go @@ -409,6 +409,23 @@ func intOrEnv(flagVal int, envKey string) (int, error) { // present-but-unparseable one is an error naming the offending variable and // value, so `COMPASS_MICROVM_QUOTA_REQUIRED=yes` refuses at startup instead of // silently reading as false and fail-opening the D7 gate. +// +// PRECEDENCE IS ASYMMETRIC, deliberately, and it is a CONTRACT, not an +// accident: a false flag is indistinguishable from an unset one here (there is +// no flag.Visit and no *bool nil sentinel), so `--microvm-quota-required=false` +// does NOT override `$COMPASS_MICROVM_QUOTA_REQUIRED=true` — the env's true +// wins. Two reasons to keep it that way rather than honoring an explicit false: +// +// - It matches the zero-value convention of every sibling knob (orEnv, +// intOrEnv), so one knob does not read its flags differently from the rest. +// - It fails in the SAFE direction for THIS knob: the losing input is the one +// that would turn the multi-tenant quota gate OFF, so a conflict leaves the +// gate ON. +// +// An operator who genuinely needs the gate off on a box whose environment sets +// it must unset the environment variable; passing the flag is not enough. +// TestQuotaRequiredEnvTrueBeatsExplicitFlagFalse pins this direction so the +// asymmetry cannot be silently "fixed" into the fail-open direction. func boolOrEnv(flagVal bool, envKey string) (bool, error) { if flagVal { return true, nil diff --git a/go/internal/runtime/microvm/launch.go b/go/internal/runtime/microvm/launch.go index a504f79c..e09bcf31 100644 --- a/go/internal/runtime/microvm/launch.go +++ b/go/internal/runtime/microvm/launch.go @@ -13,7 +13,6 @@ import ( "strconv" "strings" "sync" - "sync/atomic" "syscall" "time" @@ -52,6 +51,16 @@ const reapGrace = 5 * time.Second // boot log into the test output. const diagnosticTailBytes = 8 << 10 +// modcapsDropMknod is the exact --modcaps literal virtiofsd is launched with, +// pinned in ONE place because virtiofsd does NOT validate capability names. +// Verified by execution: `--modcaps=-not_a_real_cap` and `--modcaps=-bogus_cap` +// are both accepted silently and the daemon starts normally. So a typo or an +// upstream rename turns the flag into a no-op with no diagnostic anywhere, and +// the argv assertion in launch_idmap_test.go is the ONLY guard — which is why +// the literal lives here and both the launch path and that test reference this +// const, so the two cannot drift apart into a mutually-agreeing typo. +const modcapsDropMknod = "--modcaps=-mknod" + // launchOptions selects which virtio devices cloud-hypervisor is booted with. // The full boot wires every device; the OQ-G net-only smoke omits virtio-fs and // vsock so the passt×CH vhost-user-net negotiation — the spike's primary @@ -70,7 +79,31 @@ type child struct { name string cmd *exec.Cmd logPath string - waited atomic.Bool // set once cmd.Wait has returned, so liveness probes and PSS skip a reaped process + + // exited is closed by this child's SOLE reaper, started by startChild once + // Start has succeeded. It carries the ONE cmd.Wait per child: the readiness + // poll's liveness check (waitForSockets), Shutdown's reap, Running and PSS + // all observe the exit THROUGH this channel instead of calling Wait + // themselves, so no two paths ever race on the same process. Nil exactly + // when the process was never started (cmd.Process is nil too). + exited chan struct{} + // waitErr is cmd.Wait's error, written by the reaper BEFORE it closes + // exited. Reading it is only safe after a receive from exited, which is the + // happens-before edge that makes the unsynchronized field race-free. + waitErr error +} + +// hasExited reports whether the child's reaper has already observed its exit, +// without blocking. It is NOT a signal-0 probe: an exited-but-unwaited process +// is a zombie that Signal(0) reports as alive, which is precisely why the +// liveness check reads the reaper's channel instead. +func (c *child) hasExited() bool { + select { + case <-c.exited: + return true + default: + return false + } } // VM is a running (or partially-started, on the Launch error path) guest and @@ -83,10 +116,10 @@ type VM struct { virtiofsd *child // nil under the net-only smoke (no --fs) passt *child - // vmmExited is closed by the sole VMM reaper (started in launch) once the - // cloud-hypervisor process has been Wait'd, so the caller can observe a - // prompt guest self-power-off instead of a zombie-blind Signal(0) poll. Nil - // only under the hermetic fail-closed path (no VMM on PATH). + // vmmExited is the VMM child's reaper channel (vm.vmm.exited), hoisted onto + // the VM so WaitVMMExit can observe a prompt guest self-power-off instead of + // polling a zombie. Nil only under the hermetic fail-closed path (no VMM on + // PATH), where no VMM child was ever started. vmmExited chan struct{} consolePath string // --serial file: the guest serial console @@ -156,24 +189,28 @@ func launch(ctx context.Context, cfg BootConfig, opts launchOptions) (_ *VM, err if lookErr != nil { return nil, fmt.Errorf("microvm: resolving virtiofsd on PATH: %w", lookErr) } - // The subordinate base is READ, never assumed: newuidmap validates the - // requested host range against /etc/subuid, so a host whose range does - // not start at the conventional 100000 would otherwise fail here as an - // opaque "waiting for daemon sockets" timeout. VerifySubordinateIDRange - // runs the same read at startup so this error is rare by construction. - subBase := 0 + // Both subordinate bases are READ, never assumed and never shared: + // newuidmap validates the requested host uid range against /etc/subuid + // and newgidmap validates the gid range against /etc/subgid, and those + // are INDEPENDENT allocations (subuid.go). Reusing the uid base for the + // gid map boots fine on a shadow-utils-default box and dies on a + // divergent one — after virtiofsd has already bound its socket, which is + // why waitForSockets below is liveness-aware. VerifySubordinateIDRange + // runs the same two reads at startup so this error is rare by + // construction. + subUIDBase, subGIDBase := 0, 0 if cfg.AgentUID != 0 { - base, subErr := SubordinateIDBase() + uidBase, gidBase, subErr := SubordinateIDBases() if subErr != nil { return nil, subErr } - subBase = base + subUIDBase, subGIDBase = uidBase, gidBase } vm.virtiofsd = &child{ name: "virtiofsd", logPath: filepath.Join(dir, "virtiofsd.log"), //nolint:gosec // G204: the microVM harness seam — virtiofsdPath is LookPath-resolved and the argv is harness-built from BootConfig, neither user-controlled - cmd: exec.CommandContext(ctx, virtiofsdPath, virtiofsdArgs(cfg, subBase)...), + cmd: exec.CommandContext(ctx, virtiofsdPath, virtiofsdArgs(cfg, subUIDBase, subGIDBase)...), } if startErr := startChild(vm.virtiofsd); startErr != nil { return nil, fmt.Errorf("microvm: starting virtiofsd: %w", startErr) @@ -210,14 +247,30 @@ func launch(ctx context.Context, cfg BootConfig, opts launchOptions) (_ *VM, err vm.sockets = append(vm.sockets, cfg.Net.VhostUserSocket) // virtiofsd + passt must be serving before cloud-hypervisor connects to - // their sockets. Bounded poll, not a fixed sleep. + // their sockets. Bounded poll, not a fixed sleep — and LIVENESS-AWARE, not + // merely path-existence: virtiofsd binds its AF_UNIX socket BEFORE the + // id-map setup that can fail, so a mapping failure leaves the socket on + // disk behind a dead daemon. A path-only poll would return nil there and + // launch cloud-hypervisor against a corpse, surfacing as an inscrutable + // vhost-user negotiation error instead of virtiofsd's own "couldn't setup + // id mappings" line. + waiting := []*child{vm.passt} ready := []string{cfg.Net.VhostUserSocket} if opts.withFS { + waiting = append(waiting, vm.virtiofsd) ready = append(ready, cfg.FSSocket) } - if waitErr := waitForSockets(ctx, ready, socketReadyTimeout); waitErr != nil { + if waitErr := waitForSockets(ctx, ready, waiting, socketReadyTimeout); waitErr != nil { return nil, fmt.Errorf("microvm: waiting for daemon sockets: %w", waitErr) } + // Belt-and-braces over the poll above: a daemon that dies in the window + // between the last poll iteration and here must not be handed to the VMM. + for _, c := range waiting { + if c.hasExited() { + return nil, fmt.Errorf("microvm: %s exited before cloud-hypervisor was started: %w; log tail:\n%s", + c.name, waitResult(c.name, c.waitErr), tailFile(c.logPath)) + } + } vmmPath, lookErr := exec.LookPath("cloud-hypervisor") if lookErr != nil { @@ -232,16 +285,10 @@ func launch(ctx context.Context, cfg BootConfig, opts launchOptions) (_ *VM, err if startErr := startChild(vm.vmm); startErr != nil { return nil, fmt.Errorf("microvm: starting cloud-hypervisor: %w", startErr) } - // The sole VMM reaper owns the single cmd.Wait for cloud-hypervisor: it - // unblocks WaitVMMExit on a guest self-power-off and lets Shutdown observe - // the exit without a second Wait. The Wait error is deliberately discarded — - // a killed VMM yields an expected *exec.ExitError, mirroring waitResult. - vm.vmmExited = make(chan struct{}) - go func() { - _ = vm.vmm.cmd.Wait() // discard: a killed VMM's *exec.ExitError is the expected teardown outcome (mirrors waitResult) - vm.vmm.waited.Store(true) - close(vm.vmmExited) - }() + // startChild installed the sole reaper that owns cloud-hypervisor's single + // cmd.Wait; hoist its channel onto the VM so WaitVMMExit observes a guest + // self-power-off promptly instead of polling a zombie. + vm.vmmExited = vm.vmm.exited if opts.withVsock { vm.sockets = append(vm.sockets, cfg.VsockSocket) } @@ -250,23 +297,27 @@ func launch(ctx context.Context, cfg BootConfig, opts launchOptions) (_ *VM, err // virtiofsdArgs builds the whole virtiofsd argv: the socket/share/sandbox flags // every boot carries, the capability trim, and the id mapping (empty under the -// V2a spike, see virtiofsdIDMapArgs). subBase is the host subordinate id mapped -// to namespace-uid 0, read from /etc/subuid by the caller; it is ignored when -// cfg.AgentUID is zero. +// V2a spike, see virtiofsdIDMapArgs). subUIDBase and subGIDBase are the host +// subordinate ids mapped to namespace-uid 0 and namespace-gid 0, read from +// /etc/subuid and /etc/subgid INDEPENDENTLY by the caller (subuid.go on why +// they must not be one value); both are ignored when cfg.AgentUID is zero. // -// --modcaps=-mknod drops CAP_MKNOD from the capability set virtiofsd retains +// modcapsDropMknod drops CAP_MKNOD from the capability set virtiofsd retains // under --sandbox=namespace. A workspace share has no legitimate use for device // nodes — the agent checks out source and writes build output — so the // capability is pure escape surface, and dropping it is free (see -// virtiofsdIDMapArgs on the rest of the retained set). -func virtiofsdArgs(cfg BootConfig, subBase int) []string { - idMap := virtiofsdIDMapArgs(cfg.AgentUID, subBase) +// virtiofsdIDMapArgs on the rest of the retained set). The literal is a const +// shared with the test because virtiofsd silently ACCEPTS unknown capability +// names, so the argv assertion is the only guard against a typo — see +// modcapsDropMknod's own doc. +func virtiofsdArgs(cfg BootConfig, subUIDBase, subGIDBase int) []string { + idMap := virtiofsdIDMapArgs(cfg.AgentUID, subUIDBase, subGIDBase) args := make([]string, 0, 4+len(idMap)) args = append(args, "--socket-path="+cfg.FSSocket, "--shared-dir="+cfg.FSSharedDir, "--sandbox=namespace", - "--modcaps=-mknod", + modcapsDropMknod, ) return append(args, idMap...) } @@ -294,18 +345,23 @@ func virtiofsdArgs(cfg BootConfig, subBase int) []string { // 2. --translate-uid is documented as incompatible with // `--posix-acl=always|auto`, so it would foreclose POSIX ACLs on the share. // -// The two mapped ranges, both one id wide: +// The four mapped ranges, all one id wide, TWO PER AXIS: // - :0::1: — an id from the invoking user's /etc/subuid range // becomes namespace-root, so the daemon can chown as above. It is a // subordinate id the invoking user already owns, so no capability is needed // (newuidmap is setuid and honors /etc/subuid). // - :::1: — the in-guest agent id maps to the invoking // host user, which is the parity target itself. -// -// gid mirrors uid, EXCEPT that the host side is the invoking user's real gid, -// not its uid: the guest agent runs uid==gid==agentUID (guestd linuxCredential) -// while a host user's gid is routinely different (e.g. 1000:100). Collapsing gid -// onto uid here is precisely the parity break the V6 parity test detects. +// - :0::1: — the gid arm's namespace-root mapping, from +// /etc/subgid. It is a SEPARATE allocation from the subuid base and is read +// separately (subuid.go): newgidmap validates the gid range against +// /etc/subgid, so reusing subUIDBase here boots on a shadow-utils-default +// host and makes virtiofsd die on a divergent one. +// - :::1: — the guest agent runs uid==gid==agentUID +// (guestd linuxCredential) while a host user's gid is routinely different +// (e.g. 1000:100), so the host side here is the invoking user's real gid. +// Collapsing gid onto uid is precisely the parity break the V6 parity test +// detects. // // SECURITY POSTURE — this is NOT pure ownership parity, and the difference is // deliberate and accepted, not incidental. Mapping a subordinate id to @@ -321,7 +377,7 @@ func virtiofsdArgs(cfg BootConfig, subBase int) []string { // - --sandbox=namespace pivot_roots the daemon into the shared dir, so even // that authority reaches only the volume subtree it is serving. // -// CAP_MKNOD is dropped outright by virtiofsdArgs' --modcaps=-mknod: a workspace +// CAP_MKNOD is dropped outright by virtiofsdArgs' modcapsDropMknod: a workspace // share has no legitimate device nodes, so it is surface with no use. // // The alternative — --translate-uid/--translate-gid, which reaches the same @@ -330,18 +386,17 @@ func virtiofsdArgs(cfg BootConfig, subBase int) []string { // tracked as a design fork (RIG-3330) for the record's owner to rule on. It is // NOT swapped in here: --uid-map/--gid-map is the frozen record's named // mechanism, and changing it is a design decision, not a review fix. -func virtiofsdIDMapArgs(agentUID uint32, subBase int) []string { +func virtiofsdIDMapArgs(agentUID uint32, subUIDBase, subGIDBase int) []string { if agentUID == 0 { return nil } hostUID := os.Getuid() hostGID := os.Getgid() agent := strconv.FormatUint(uint64(agentUID), 10) - base := strconv.Itoa(subBase) return []string{ - "--uid-map", idMapSpec("0", base), + "--uid-map", idMapSpec("0", strconv.Itoa(subUIDBase)), "--uid-map", idMapSpec(agent, strconv.Itoa(hostUID)), - "--gid-map", idMapSpec("0", base), + "--gid-map", idMapSpec("0", strconv.Itoa(subGIDBase)), "--gid-map", idMapSpec(agent, strconv.Itoa(hostGID)), } } @@ -401,6 +456,13 @@ func vmmArgs(cfg BootConfig, consolePath string, opts launchOptions) []string { // bind reliably; a no-op elsewhere) and captures its stdout+stderr to logPath so // a boot failure can surface the daemon's own diagnostics. The real teardown // guarantee is Shutdown, not Pdeathsig (record §(g) lines 300-303). +// +// On a successful Start it also installs the child's SOLE reaper: exactly one +// goroutine per child owning exactly one cmd.Wait, publishing the result on +// c.exited. Every other path — the readiness poll's liveness check, Shutdown's +// reap, Running, PSS — observes the exit through that channel rather than +// Wait'ing itself, so there is never a second concurrent Wait on one process +// (which would race and hand one caller a bogus "waitid: no child processes"). func startChild(c *child) error { logFile, err := os.OpenFile(c.logPath, os.O_CREATE|os.O_WRONLY|os.O_TRUNC, 0o600) if err != nil { @@ -417,19 +479,48 @@ func startChild(c *child) error { startErr := c.cmd.Start() // Our handle on the log file is no longer needed: Start dup'd it into the // child (on success) or it stays unused (on failure). Either way close it. - _ = logFile.Close() // the child holds its own dup; our copy is done with + _ = logFile.Close() // deliberate: the child holds its own dup of the fd, so a close error on our spent copy is not actionable if startErr != nil { return startErr } + c.exited = make(chan struct{}) + go func() { + // The single Wait for this child. waitErr is written BEFORE the close, + // so any reader that has received from c.exited sees it (that is the + // happens-before edge); an *exec.ExitError from a killed daemon is the + // expected teardown outcome and is filtered by waitResult at each + // reader, not here. + c.waitErr = c.cmd.Wait() + close(c.exited) + }() return nil } -// waitForSockets polls until every path exists or the deadline elapses. A -// missing socket at the deadline is a named error naming the first path still -// absent, so a daemon that died on launch fails the boot fast. -func waitForSockets(ctx context.Context, paths []string, timeout time.Duration) error { +// waitForSockets polls until every path in paths exists, one of the children in +// waiting has exited, or the deadline elapses. +// +// LIVENESS, not just path existence: virtiofsd binds its AF_UNIX socket BEFORE +// the id-map setup that can fail, so a mapping failure leaves a mode-srwx socket +// on disk with the daemon already exited 1 (verified by execution). A +// path-existence-only poll returns nil there and the boot proceeds to start +// cloud-hypervisor against a dead daemon, where the real cause ("couldn't setup +// id mappings", in virtiofsd's own log) is replaced by an inscrutable vhost-user +// negotiation error much later. So an exited child short-circuits the poll with +// an error NAMING the daemon and carrying its log tail. +// +// A missing socket at the deadline is likewise a named error naming the first +// path still absent. +func waitForSockets(ctx context.Context, paths []string, waiting []*child, timeout time.Duration) error { deadline := time.Now().Add(timeout) for { + // Liveness first: a dead daemon's leftover socket must not read as + // readiness, so this check precedes the Stat sweep. + for _, c := range waiting { + if c.hasExited() { + return fmt.Errorf("%s exited before its socket was serving: %w; log tail:\n%s", + c.name, waitResult(c.name, c.waitErr), tailFile(c.logPath)) + } + } missing := "" for _, p := range paths { if _, err := os.Stat(p); err != nil { @@ -507,24 +598,22 @@ func (vm *VM) Shutdown(ctx context.Context) error { } // reap terminates an auxiliary daemon gracefully then forcibly: SIGTERM, wait up -// to reapGrace, SIGKILL if it is still alive, then Wait to collect the exit and -// avoid a zombie. +// to reapGrace, SIGKILL if it is still alive. It does NOT Wait — startChild's +// sole reaper owns this child's single cmd.Wait, and reap observes the exit +// through c.exited, so no second Wait can race it. func reap(c *child) error { if termErr := c.cmd.Process.Signal(syscall.SIGTERM); termErr != nil && !errors.Is(termErr, os.ErrProcessDone) { return fmt.Errorf("SIGTERM %s: %w", c.name, termErr) } - done := make(chan error, 1) - go func() { done <- c.cmd.Wait() }() select { - case err := <-done: - c.waited.Store(true) - return waitResult(c.name, err) + case <-c.exited: + return waitResult(c.name, c.waitErr) case <-time.After(reapGrace): if killErr := c.cmd.Process.Kill(); killErr != nil && !errors.Is(killErr, os.ErrProcessDone) { return fmt.Errorf("SIGKILL %s: %w", c.name, killErr) } - c.waited.Store(true) - return waitResult(c.name, <-done) + <-c.exited + return waitResult(c.name, c.waitErr) } } @@ -559,11 +648,13 @@ func waitResult(name string, err error) error { } // Running reports whether the named child's process is still alive. It is used -// by the test to assert Shutdown left no orphan. A process that has been Wait'd -// is definitively gone; otherwise signal 0 probes liveness without affecting it. +// by the test to assert Shutdown left no orphan. A child whose sole reaper has +// already observed the exit is definitively gone — and that check must come +// FIRST, because an exited-but-unreaped process is a zombie that Signal(0) +// reports as alive; otherwise signal 0 probes liveness without affecting it. func (vm *VM) Running(name string) bool { c := vm.childByName(name) - if c == nil || c.cmd.Process == nil || c.waited.Load() { + if c == nil || c.cmd.Process == nil || c.hasExited() { return false } return c.cmd.Process.Signal(syscall.Signal(0)) == nil @@ -579,7 +670,7 @@ func (vm *VM) PSS() (map[string]int64, error) { out := make(map[string]int64) var errs []error for _, c := range []*child{vm.vmm, vm.virtiofsd, vm.passt} { - if c == nil || c.cmd.Process == nil || c.waited.Load() { + if c == nil || c.cmd.Process == nil || c.hasExited() { continue } pss, err := readPSS(c.cmd.Process.Pid) diff --git a/go/internal/runtime/microvm/launch_idmap_test.go b/go/internal/runtime/microvm/launch_idmap_test.go index 5898368f..42eb1f75 100644 --- a/go/internal/runtime/microvm/launch_idmap_test.go +++ b/go/internal/runtime/microvm/launch_idmap_test.go @@ -8,13 +8,16 @@ package microvm // because a wrong pair fails at first boot as an opaque daemon-socket timeout // rather than as a legible error. // -// The subordinate base is injected as a fixed value rather than read from the -// test box's /etc/subuid, which is the whole point of the parse/spawn split in -// subuid.go: the mapping is pinned identically on a host allocated 100000 and -// one allocated 165536. +// The subordinate bases are injected as fixed values rather than read from the +// test box's /etc/subuid and /etc/subgid, which is the whole point of the +// parse/spawn split in subuid.go: the mapping is pinned identically on a host +// allocated 100000 and one allocated 165536 — and, crucially, on a host whose +// two files DIVERGE, which this box's own /etc (both `mattw:100000:65536`) +// cannot exhibit. import ( "os" + "path/filepath" "slices" "strconv" "strings" @@ -26,6 +29,12 @@ import ( // regression back to it. const testSubBase = 165536 +// testSubGIDBase is a subordinate GID base DISTINCT from testSubBase, for the +// rows that must prove the two axes carry independent values. /etc/subuid and +// /etc/subgid are separate allocations, so a gid map built from the subuid base +// boots on a lockstep host and kills virtiofsd on a divergent one. +const testSubGIDBase = 265536 + // flagValues returns every value token following an occurrence of flag in argv, // in order — so a test asserts both the count and the exact specs of a repeated // flag rather than a single Contains. @@ -40,18 +49,25 @@ func flagValues(argv []string, flag string) []string { } // TestVirtiofsdIDMapArgsExactSpecs pins all four mapping pairs for a real agent -// uid. The gid arm is the discriminating one: its host side must be os.Getgid(), +// uid. Two arms are discriminating: the gid arm's HOST side must be os.Getgid(), // NOT os.Getuid() — a host user's gid is routinely different from its uid (e.g. // 1000:100), and collapsing gid onto uid is precisely the host-ownership parity -// break the KVM parity leg detects. +// break the KVM parity leg detects — and the gid arm's NAMESPACE-ROOT side must +// come from the /etc/subgid base, not the /etc/subuid one (see +// TestVirtiofsdIDMapArgsDivergentSubordinateBases). func TestVirtiofsdIDMapArgsExactSpecs(t *testing.T) { const agentUID = 1000 - argv := virtiofsdIDMapArgs(agentUID, testSubBase) + argv := virtiofsdIDMapArgs(agentUID, testSubBase, testSubGIDBase) - base := strconv.Itoa(testSubBase) agent := strconv.Itoa(agentUID) - wantUID := []string{":0:" + base + ":1:", ":" + agent + ":" + strconv.Itoa(os.Getuid()) + ":1:"} - wantGID := []string{":0:" + base + ":1:", ":" + agent + ":" + strconv.Itoa(os.Getgid()) + ":1:"} + wantUID := []string{ + ":0:" + strconv.Itoa(testSubBase) + ":1:", + ":" + agent + ":" + strconv.Itoa(os.Getuid()) + ":1:", + } + wantGID := []string{ + ":0:" + strconv.Itoa(testSubGIDBase) + ":1:", + ":" + agent + ":" + strconv.Itoa(os.Getgid()) + ":1:", + } if got := flagValues(argv, "--uid-map"); !slices.Equal(got, wantUID) { t.Errorf("--uid-map specs = %v, want %v (argv %v)", got, wantUID, argv) @@ -59,21 +75,58 @@ func TestVirtiofsdIDMapArgsExactSpecs(t *testing.T) { if got := flagValues(argv, "--gid-map"); !slices.Equal(got, wantGID) { t.Errorf("--gid-map specs = %v, want %v (argv %v)", got, wantGID, argv) } - // The subordinate base must come from the injected value, never from a - // hardcoded 100000 — the correctness bug on any host whose /etc/subuid - // range starts elsewhere. + // Neither base may come from a hardcoded 100000 — the correctness bug on + // any host whose /etc/subuid or /etc/subgid range starts elsewhere. for _, spec := range append(flagValues(argv, "--uid-map"), flagValues(argv, "--gid-map")...) { - if strings.Contains(spec, ":100000:") && testSubBase != 100000 { - t.Errorf("spec %q carries a hardcoded 100000 base; the subordinate base must be the parsed one (%d)", spec, testSubBase) + if strings.Contains(spec, ":100000:") { + t.Errorf("spec %q carries a hardcoded 100000 base; the subordinate bases must be the parsed ones (%d/%d)", + spec, testSubBase, testSubGIDBase) } } } +// TestVirtiofsdIDMapArgsDivergentSubordinateBases is the row the whole +// subuid/subgid split exists for: with DIFFERENT uid and gid bases injected, the +// two namespace-root arms must carry DIFFERENT host ids. +// +// It cannot be a host-dependent test. /etc/subuid and /etc/subgid on a +// shadow-utils-default box (this one included: both read `mattw:100000:65536`) +// hold the SAME base, which is exactly why a gid map that reuses the subuid base +// passes everywhere it is developed and then dies on a host provisioned with +// `usermod --add-subgids` alone. newgidmap validates the --gid-map host range +// against /etc/subgid, so on such a host virtiofsd refuses the mapping — AFTER +// binding its socket, which is what makes the failure so illegible. +func TestVirtiofsdIDMapArgsDivergentSubordinateBases(t *testing.T) { + const agentUID = 1000 + argv := virtiofsdIDMapArgs(agentUID, testSubBase, testSubGIDBase) + + uidSpecs := flagValues(argv, "--uid-map") + gidSpecs := flagValues(argv, "--gid-map") + // Guarded rather than indexed blind: a mapping that lost an arm entirely + // would panic here and report a crash instead of the missing flag. + if len(uidSpecs) != 2 || len(gidSpecs) != 2 { + t.Fatalf("argv %v carries %d --uid-map and %d --gid-map specs, want 2 of each "+ + "(namespace-root plus the agent id, per axis)", argv, len(uidSpecs), len(gidSpecs)) + } + uidRoot, gidRoot := uidSpecs[0], gidSpecs[0] + if uidRoot == gidRoot { + t.Fatalf("the --uid-map and --gid-map namespace-root specs are both %q with uidBase=%d != gidBase=%d; "+ + "the gid arm is reusing the SUBUID base, which newgidmap validates against /etc/subgid and rejects "+ + "on any host whose two allocations diverge", uidRoot, testSubBase, testSubGIDBase) + } + if want := ":0:" + strconv.Itoa(testSubBase) + ":1:"; uidRoot != want { + t.Errorf("--uid-map namespace-root spec = %q, want %q (the /etc/subuid base)", uidRoot, want) + } + if want := ":0:" + strconv.Itoa(testSubGIDBase) + ":1:"; gidRoot != want { + t.Errorf("--gid-map namespace-root spec = %q, want %q (the /etc/subgid base)", gidRoot, want) + } +} + // TestVirtiofsdIDMapArgsUnmappedSpike pins the V2a carve-out: a zero agentUID // (the spike harness, which shares a throwaway dir and asserts nothing about // ownership) gets NO mapping at all, so that suite keeps booting unchanged. func TestVirtiofsdIDMapArgsUnmappedSpike(t *testing.T) { - if argv := virtiofsdIDMapArgs(0, testSubBase); argv != nil { + if argv := virtiofsdIDMapArgs(0, testSubBase, testSubGIDBase); argv != nil { t.Fatalf("virtiofsdIDMapArgs(0) = %v, want nil (the V2a spike share stays unmapped)", argv) } } @@ -82,16 +135,28 @@ func TestVirtiofsdIDMapArgsUnmappedSpike(t *testing.T) { // workspace share has no legitimate device nodes, so CAP_MKNOD is dropped from // the set virtiofsd retains as namespace-root — on EVERY boot, including the // unmapped spike, since the flag costs nothing there. +// +// The assertion is against the modcapsDropMknod CONST, not a literal, and that +// is load-bearing: virtiofsd does NOT validate capability names — verified by +// execution, `--modcaps=-not_a_real_cap` starts the daemon normally — so a typo +// makes the flag a silent no-op with no diagnostic anywhere, and this argv +// assertion is the only guard. A literal repeated here would agree with a typo +// in launch.go; sharing the const means a typo has to be made once to be wrong +// in both places, which the exact-literal check below then catches. func TestVirtiofsdArgsDropsMknod(t *testing.T) { + if modcapsDropMknod != "--modcaps=-mknod" { + t.Fatalf("modcapsDropMknod = %q, want %q — virtiofsd silently ACCEPTS unknown capability names, "+ + "so a drifted literal is an undetectable no-op that leaves CAP_MKNOD on the share", modcapsDropMknod, "--modcaps=-mknod") + } for _, agentUID := range []uint32{0, 1000} { cfg := BootConfig{ FSSocket: "/tmp/cvm/virtiofsd.sock", FSSharedDir: "/tmp/cvm/share", AgentUID: agentUID, } - argv := virtiofsdArgs(cfg, testSubBase) - if !slices.Contains(argv, "--modcaps=-mknod") { - t.Errorf("virtiofsdArgs(AgentUID=%d) = %v, want it to carry --modcaps=-mknod", agentUID, argv) + argv := virtiofsdArgs(cfg, testSubBase, testSubGIDBase) + if !slices.Contains(argv, modcapsDropMknod) { + t.Errorf("virtiofsdArgs(AgentUID=%d) = %v, want it to carry %q", agentUID, argv, modcapsDropMknod) } // The pre-existing flags must survive the argv restructure. for _, want := range []string{ @@ -108,28 +173,37 @@ func TestVirtiofsdArgsDropsMknod(t *testing.T) { // TestVirtiofsdArgsIncludesMapping is the seam assertion the launch path depends // on: the full argv carries the mapping for a real agent uid and none for the -// spike, so the restructure into a named local did not drop the id args. +// spike, so the restructure into a named local did not drop the id args — and it +// carries the gid base through to the gid arm, so the seam cannot collapse the +// two bases onto one on the way from launch to argv. func TestVirtiofsdArgsIncludesMapping(t *testing.T) { - mapped := virtiofsdArgs(BootConfig{AgentUID: 1000}, testSubBase) + mapped := virtiofsdArgs(BootConfig{AgentUID: 1000}, testSubBase, testSubGIDBase) if got := len(flagValues(mapped, "--uid-map")); got != 2 { t.Errorf("mapped argv %v carries %d --uid-map specs, want 2", mapped, got) } - spike := virtiofsdArgs(BootConfig{AgentUID: 0}, testSubBase) + if got := flagValues(mapped, "--gid-map"); len(got) != 2 || !strings.Contains(got[0], strconv.Itoa(testSubGIDBase)) { + t.Errorf("mapped argv %v --gid-map specs = %v, want 2 specs whose namespace-root arm carries the subgid base %d", + mapped, got, testSubGIDBase) + } + spike := virtiofsdArgs(BootConfig{AgentUID: 0}, testSubBase, testSubGIDBase) if got := flagValues(spike, "--uid-map"); len(got) != 0 { t.Errorf("spike argv %v carries --uid-map specs %v, want none", spike, got) } } -// TestParseSubordinateIDBase pins the subuid(5) parse: the invoking user's entry -// is matched by NAME or by uid, comments and malformed/zero-count entries are -// skipped, and a user with no range is a named error rather than a silent -// fallback to a base the user does not own. +// TestParseSubordinateIDBase pins the subuid(5)/subgid(5) parse (one format, +// both files): the invoking user's entry is matched by NAME or by uid, comments +// and malformed/zero-count entries are skipped, and a user with no range is a +// named error rather than a silent fallback to a base the user does not own. +// The error must name the FILE it was parsing, so a missing subgid range is not +// misreported as a subuid problem. func TestParseSubordinateIDBase(t *testing.T) { tests := []struct { name string content string uid int username string + path string want int wantErr []string }{ @@ -168,22 +242,35 @@ func TestParseSubordinateIDBase(t *testing.T) { want: 400000, }, { - name: "no range for the user names the user and the fix", + name: "no range for the user names the user, the file and the fix", content: "root:100000:65536\nother:165536:65536\n", uid: 1000, username: "mattw", - wantErr: []string{"mattw", "/etc/subuid", "usermod --add-subuids", "newuidmap"}, + path: subordinateUIDPath, + wantErr: []string{ + "mattw", "/etc/subuid", "/etc/subgid", + "usermod --add-subuids", "usermod --add-subgids", "newuidmap", "newgidmap", + }, + }, + { + name: "a missing SUBGID range names the subgid file, not the subuid one", + content: "root:100000:65536\n", + uid: 1000, + username: "mattw", + path: subordinateGIDPath, + wantErr: []string{"mattw", "in /etc/subgid", "usermod --add-subgids", "newgidmap"}, }, { name: "an empty file names the fix", content: "", uid: 1000, + path: subordinateUIDPath, wantErr: []string{"uid 1000", "/etc/subuid"}, }, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - got, err := parseSubordinateIDBase(strings.NewReader(tt.content), tt.uid, tt.username) + got, err := parseSubordinateIDBase(strings.NewReader(tt.content), tt.uid, tt.username, tt.path) if len(tt.wantErr) > 0 { if err == nil { t.Fatalf("parseSubordinateIDBase = %d, want an error naming %v", got, tt.wantErr) @@ -204,3 +291,108 @@ func TestParseSubordinateIDBase(t *testing.T) { }) } } + +// TestSubordinateIDBasesResolvesBothFilesIndependently is HIGH-1's preflight +// axis: the two bases come from the two files SEPARATELY, so a host whose +// allocations diverge yields divergent bases rather than one base used twice. +// +// It drives fixture files through the path-injected core because this box cannot +// exhibit the bug: its /etc/subuid and /etc/subgid both read +// `mattw:100000:65536`, so a gid arm reusing the subuid base is observationally +// identical to a correct one here. +func TestSubordinateIDBasesResolvesBothFilesIndependently(t *testing.T) { + dir := t.TempDir() + // The owner field must match THIS process, since subordinateBase resolves + // the invoking uid/username itself — the file contents are the only + // injectable half. + owner := strconv.Itoa(os.Getuid()) + uidPath := filepath.Join(dir, "subuid") + gidPath := filepath.Join(dir, "subgid") + if err := os.WriteFile(uidPath, []byte(owner+":100000:65536\n"), 0o600); err != nil { + t.Fatalf("writing the subuid fixture: %v", err) + } + if err := os.WriteFile(gidPath, []byte(owner+":165536:65536\n"), 0o600); err != nil { + t.Fatalf("writing the subgid fixture: %v", err) + } + + uidBase, gidBase, err := subordinateIDBases(uidPath, gidPath) + if err != nil { + t.Fatalf("subordinateIDBases over divergent fixtures = %v, want both bases", err) + } + if uidBase != 100000 { + t.Errorf("uid base = %d, want 100000 (from %s)", uidBase, uidPath) + } + if gidBase != 165536 { + t.Errorf("gid base = %d, want 165536 (from %s); reading it from /etc/subuid instead is the divergence bug", gidBase, gidPath) + } + if uidBase == gidBase { + t.Fatal("the two bases collapsed onto one value despite divergent fixture files; the gid axis is not read independently") + } +} + +// TestSubordinateIDBasesRefusesAMissingSubgidRange pins the OTHER half of the +// preflight fix: a host with a perfectly good /etc/subuid range and NO +// /etc/subgid range must fail at PREFLIGHT with both files and both usermod +// flags named — not at first boot, where newgidmap refuses the gid map AFTER +// virtiofsd has bound its socket and the operator sees only a vhost-user +// negotiation error. +func TestSubordinateIDBasesRefusesAMissingSubgidRange(t *testing.T) { + dir := t.TempDir() + owner := strconv.Itoa(os.Getuid()) + uidPath := filepath.Join(dir, "subuid") + gidPath := filepath.Join(dir, "subgid") + if err := os.WriteFile(uidPath, []byte(owner+":100000:65536\n"), 0o600); err != nil { + t.Fatalf("writing the subuid fixture: %v", err) + } + // A subgid file that exists but allocates the invoking user nothing — the + // `usermod --add-subuids`-only host. + if err := os.WriteFile(gidPath, []byte("root:100000:65536\n"), 0o600); err != nil { + t.Fatalf("writing the subgid fixture: %v", err) + } + + _, _, err := subordinateIDBases(uidPath, gidPath) + if err == nil { + t.Fatal("subordinateIDBases = nil error with no subgid range for the invoking user; " + + "a divergent host must fail PREFLIGHT, not at the first boot as an opaque virtiofsd death") + } + for _, part := range []string{ + gidPath, subordinateUIDPath, subordinateGIDPath, + "usermod --add-subuids", "usermod --add-subgids", "newgidmap", + } { + if !strings.Contains(err.Error(), part) { + t.Errorf("error %q does not name %q", err.Error(), part) + } + } +} + +// TestSubordinateIDBasesRefusesAnUnreadableFile pins the open-error arm on BOTH +// axes: an absent map file is a named refusal, never a fallback to the +// conventional base. The message must show its example range as an example +// (LOW-8) and name the matching --add-subgids alongside --add-subuids, since the +// two allocations are independent. +func TestSubordinateIDBasesRefusesAnUnreadableFile(t *testing.T) { + dir := t.TempDir() + owner := strconv.Itoa(os.Getuid()) + present := filepath.Join(dir, "subuid") + if err := os.WriteFile(present, []byte(owner+":100000:65536\n"), 0o600); err != nil { + t.Fatalf("writing the subuid fixture: %v", err) + } + absent := filepath.Join(dir, "does-not-exist") + + for _, tt := range []struct{ name, uidPath, gidPath string }{ + {"an unreadable subuid file", absent, present}, + {"an unreadable subgid file", present, absent}, + } { + t.Run(tt.name, func(t *testing.T) { + _, _, err := subordinateIDBases(tt.uidPath, tt.gidPath) + if err == nil { + t.Fatal("subordinateIDBases = nil error over an absent map file; an unreadable range must refuse, never fall back") + } + for _, part := range []string{absent, "usermod --add-subuids", "usermod --add-subgids", "only the shadow-utils convention"} { + if !strings.Contains(err.Error(), part) { + t.Errorf("error %q does not name %q", err.Error(), part) + } + } + }) + } +} diff --git a/go/internal/runtime/microvm/launch_teardown_test.go b/go/internal/runtime/microvm/launch_teardown_test.go index e9ee9236..35e7e824 100644 --- a/go/internal/runtime/microvm/launch_teardown_test.go +++ b/go/internal/runtime/microvm/launch_teardown_test.go @@ -85,9 +85,8 @@ sleep 30`) // that exits on its own (as the guest does on RB_POWER_OFF) is observed by // WaitVMMExit via the sole reaper WELL UNDER the grace window — it must NOT burn // the full timeout waiting on a zombie. It assembles a minimal VM with just a -// fake vmm child plus a manually-started reaper mirroring launch's (the full -// launch needs passt/virtiofsd), since the assertion is purely about the -// reaper→vmmExited→WaitVMMExit path. +// fake vmm child (the full launch needs passt/virtiofsd), since the assertion is +// purely about the startChild-reaper→exited→WaitVMMExit path. func TestWaitVMMExitObservesPromptSelfExit(t *testing.T) { dir := t.TempDir() vm := &VM{ @@ -98,17 +97,12 @@ func TestWaitVMMExitObservesPromptSelfExit(t *testing.T) { cmd: exec.CommandContext(t.Context(), "/bin/sh", "-c", "sleep 0.1; exit 0"), }, } + // startChild installs the sole reaper itself, exactly as launch relies on; + // the VM's channel is that child's, so there is no second Wait anywhere. if err := startChild(vm.vmm); err != nil { t.Fatalf("startChild(vmm fake): %v", err) } - // The sole reaper, mirroring launch: it owns the single Wait and closes - // vmmExited once the process has exited. - vm.vmmExited = make(chan struct{}) - go func() { - _ = vm.vmm.cmd.Wait() // reaper mirror: a fake VMM's exit is the expected outcome - vm.vmm.waited.Store(true) - close(vm.vmmExited) - }() + vm.vmmExited = vm.vmm.exited // A generous grace window: the fake exits in ~100ms, so WaitVMMExit must // return true well before this elapses. Measure to prove it did not burn @@ -123,6 +117,98 @@ func TestWaitVMMExitObservesPromptSelfExit(t *testing.T) { } } +// TestWaitForSocketsFailsFastOnADeadDaemon is HIGH-2's regression lock: the +// readiness wait must be LIVENESS-aware, not path-existence-only. +// +// The fake reproduces virtiofsd's real, verified ordering — it BINDS its socket +// and only then hits the id-map step that can fail, exiting non-zero with its +// diagnostic on stderr. (Measured on virtiofsd 1.14.0: a bad gid base left +// `srwx------ .../sb.sock` on disk with the daemon exited 1.) A path-only poll +// therefore Stats a socket that exists, returns nil, and launch starts +// cloud-hypervisor against a corpse; the operator then sees a vhost-user +// negotiation error instead of "couldn't setup id mappings". +// +// Three properties, each of which the pre-fix implementation fails: +// 1. an ERROR rather than nil, even though the socket path exists; +// 2. the error NAMES the daemon and carries its log tail, so virtiofsd's own +// line reaches the operator; +// 3. it returns FAST — well inside socketReadyTimeout, i.e. it short-circuits +// on the exit rather than polling out the full window. +func TestWaitForSocketsFailsFastOnADeadDaemon(t *testing.T) { + dir := t.TempDir() + socket := filepath.Join(dir, "sb.sock") + // virtiofsd's own line, apostrophe included. It travels as an ENV VAR, not + // inline in the -c script: embedded in single quotes the apostrophe would + // terminate the quoting and the fake would die before binding its socket, + // silently defeating the whole reproduction. + const diagnostic = "Couldn't setup id mappings: newgidmap failed" + + c := &child{ + name: "virtiofsd", + logPath: filepath.Join(dir, "virtiofsd.log"), + // Bind first, THEN fail — virtiofsd's actual ordering. + cmd: exec.CommandContext(t.Context(), "/bin/sh", "-c", + ": > \"$SOCKET\"; printf '%s\\n' \"$DIAGNOSTIC\" >&2; exit 1"), + } + c.cmd.Env = append(os.Environ(), "SOCKET="+socket, "DIAGNOSTIC="+diagnostic) + if err := startChild(c); err != nil { + t.Fatalf("startChild(dead-daemon fake): %v", err) + } + // Await the reaper so the poll enters with the exit already observable; + // otherwise this test would race the fake's own exit. + <-c.exited + + // The socket the dead daemon left behind: the exact condition that made the + // old path-existence poll return nil. + if _, err := os.Stat(socket); err != nil { + t.Fatalf("the fake did not leave its socket on disk (%v); the test would not reproduce the defect", err) + } + + start := time.Now() + err := waitForSockets(t.Context(), []string{socket}, []*child{c}, socketReadyTimeout) + elapsed := time.Since(start) + if err == nil { + t.Fatal("waitForSockets = nil for a daemon that exited after binding its socket; " + + "launch would start cloud-hypervisor against a DEAD virtiofsd") + } + if !strings.Contains(err.Error(), "virtiofsd") { + t.Errorf("error %q does not name the daemon", err.Error()) + } + if !strings.Contains(err.Error(), diagnostic) { + t.Errorf("error %q does not carry the daemon's own log tail (%q); the real cause would be lost", err.Error(), diagnostic) + } + // Fast: the exit short-circuits the poll instead of burning the window. + if elapsed > socketReadyTimeout/2 { + t.Errorf("waitForSockets took %v of a %v budget; a dead child must short-circuit the poll", elapsed, socketReadyTimeout) + } +} + +// TestWaitForSocketsSucceedsForALiveDaemon is the non-vacuity control for the +// liveness check: a child that binds its socket and STAYS UP must still be +// accepted. Without it the assertion above would pass on a waitForSockets that +// had simply started erroring unconditionally. +func TestWaitForSocketsSucceedsForALiveDaemon(t *testing.T) { + dir := t.TempDir() + socket := filepath.Join(dir, "live.sock") + c := &child{ + name: "virtiofsd", + logPath: filepath.Join(dir, "virtiofsd.log"), + cmd: exec.CommandContext(t.Context(), "/bin/sh", "-c", ": > "+socket+"; sleep 30"), + } + if err := startChild(c); err != nil { + t.Fatalf("startChild(live fake): %v", err) + } + t.Cleanup(func() { + if err := reap(c); err != nil { + t.Errorf("reaping the live fake: %v", err) + } + }) + + if err := waitForSockets(t.Context(), []string{socket}, []*child{c}, socketReadyTimeout); err != nil { + t.Fatalf("waitForSockets over a LIVE daemon that bound its socket = %v, want nil", err) + } +} + // readPidFile reads a pid a fake wrote, retrying briefly since the fake writes // it asynchronously after exec. func readPidFile(t *testing.T, path string) int { diff --git a/go/internal/runtime/microvm/subuid.go b/go/internal/runtime/microvm/subuid.go index 8ebd1f5f..baefb0b6 100644 --- a/go/internal/runtime/microvm/subuid.go +++ b/go/internal/runtime/microvm/subuid.go @@ -2,21 +2,24 @@ package microvm -// The /etc/subuid read behind virtiofsd's id mapping. virtiofsd shells out to -// newuidmap(1) for a non-trivial --uid-map, and newuidmap VALIDATES the -// requested host range against subuid(5) ("the range of subordinate user IDs -// must have been set up via subuid(5)", virtiofsd README §--uid-map). So the -// base mapped to namespace-uid 0 is a per-host allocation that must be READ, not -// assumed: shadow-utils happens to allocate 100000 to the first user, but a -// second user on the same box gets 165536, and LDAP/AD-backed or -// container-image-provisioned hosts routinely differ. Assuming it fails in the -// worst way — newuidmap refuses, virtiofsd dies before binding its socket, and -// the boot surfaces as an opaque "waiting for daemon sockets" timeout with the -// real cause only in virtiofsd.log. +// The subuid(5)/subgid(5) reads behind virtiofsd's id mapping. virtiofsd shells +// out to newuidmap(1)/newgidmap(1) for a non-trivial --uid-map/--gid-map, and +// those setuid helpers VALIDATE the requested host range against subuid(5) / +// subgid(5) respectively ("the range of subordinate user IDs must have been set +// up via subuid(5)", virtiofsd README §--uid-map). So each base mapped to a +// namespace id 0 is a per-host allocation that must be READ, not assumed: +// shadow-utils happens to allocate 100000 to the first user, but a second user +// on the same box gets 165536, and LDAP/AD-backed or container-image-provisioned +// hosts routinely differ. Assuming it fails in the worst way — the map helper +// refuses, virtiofsd dies, and the boot surfaces as an opaque "waiting for +// daemon sockets" timeout (or worse: virtiofsd binds its socket BEFORE the +// id-map step, so a mapping failure can leave a live-looking socket behind a +// dead daemon — see waitForSockets' liveness check) with the real cause only in +// virtiofsd.log. // // Split into a pure parse over an io.Reader plus a thin file-reading wrapper so -// the argv tests can pin the exact mapping against a fixed base with no -// dependence on the test box's own /etc/subuid. +// the argv tests can pin the exact mapping against fixed bases with no +// dependence on the test box's own /etc/subuid or /etc/subgid. import ( "bufio" @@ -28,58 +31,105 @@ import ( "strings" ) -// subordinateIDPath is the subuid(5) map newuidmap validates a requested host -// range against. Its sibling /etc/subgid is deliberately NOT read: the gid map -// reuses this base, matching how shadow-utils allocates the two ranges in -// lockstep and how rootless podman consumes them. -const subordinateIDPath = "/etc/subuid" +// The two subordinate-id maps the id-mapping depends on. They are SEPARATE, +// INDEPENDENT allocations and each is read on its own axis: newuidmap validates +// the --uid-map host range against /etc/subuid, newgidmap validates the +// --gid-map host range against /etc/subgid. shadow-utils' useradd default +// happens to allocate the same base in both, but that is a DEFAULT, NOT AN +// INVARIANT — `usermod --add-subuids` and `--add-subgids` are separate flags, +// and LDAP/AD-backed or image-provisioned hosts routinely allocate the two +// ranges independently. Reusing the subuid base for the gid map therefore boots +// fine on a lockstep box and dies on a divergent one, which is why both files +// are resolved here and at preflight (VerifySubordinateIDRange). +const ( + subordinateUIDPath = "/etc/subuid" + subordinateGIDPath = "/etc/subgid" +) + +// subordinateProvisionHint is the one remediation string every failure on either +// axis carries. The range it shows is an EXAMPLE, not a canonical value: the +// whole point of reading these files is that the conventional 100000 base must +// never be assumed. Both usermod flags are named because the two allocations are +// independent — provisioning only one leaves the other axis broken. +const subordinateProvisionHint = "provision both ranges with " + + "`usermod --add-subuids - ` and `usermod --add-subgids - ` " + + "(the base is an operator choice — 100000-65536 is only the shadow-utils convention, not a required value); " + + "rootless podman consumes the same two files" + +// SubordinateIDBases returns the first subordinate uid AND the first subordinate +// gid allocated to the invoking user — the host ids virtiofsd's mapping makes +// namespace-uid 0 and namespace-gid 0 so the daemon can chown guest-created +// inodes (see virtiofsdIDMapArgs). The two are read independently because +// /etc/subuid and /etc/subgid are independent allocations (see the consts +// above). It fails with a named error, never a fallback, when either range is +// missing: silently mapping an id the user does not own is exactly the opaque +// boot failure these reads exist to prevent. +func SubordinateIDBases() (uidBase, gidBase int, err error) { + return subordinateIDBases(subordinateUIDPath, subordinateGIDPath) +} + +// subordinateIDBases is the path-injected core, so the divergent-host and +// missing-subgid axes are unit-testable against fixture files rather than +// against the test box's own /etc — which on a shadow-utils default box carries +// IDENTICAL bases in both files and therefore cannot detect a gid map that +// wrongly reuses the uid base. +func subordinateIDBases(uidPath, gidPath string) (uidBase, gidBase int, err error) { + uidBase, err = subordinateBase(uidPath) + if err != nil { + return 0, 0, err + } + gidBase, err = subordinateBase(gidPath) + if err != nil { + return 0, 0, err + } + return uidBase, gidBase, nil +} + +// VerifySubordinateIDRange is the startup axis over the same reads: it resolves +// BOTH subordinate bases and discards them, so a host missing EITHER range — +// including the host that has a subuid range but no subgid range, which boots +// past every uid-only check and then dies in virtiofsd's gid map — fails +// preflight with the fix named rather than at the first session boot. +func VerifySubordinateIDRange() error { + if _, _, err := SubordinateIDBases(); err != nil { + return err + } + return nil +} -// SubordinateIDBase returns the first subordinate uid allocated to the invoking -// user — the host id virtiofsd's mapping makes namespace-uid 0 so the daemon can -// chown guest-created inodes (see virtiofsdIDMapArgs). It fails with a named -// error, never a fallback, when the user has no subordinate range: silently -// mapping an id the user does not own is exactly the opaque boot failure this -// read exists to prevent. -func SubordinateIDBase() (int, error) { - f, err := os.Open(subordinateIDPath) +// subordinateBase reads one subordinate-id map file and returns the invoking +// user's base within it. Path-taking rather than hardcoded so the uid and gid +// axes share one implementation and neither can silently borrow the other's +// allocation. +func subordinateBase(path string) (int, error) { + f, err := os.Open(path) //nolint:gosec // G304: path is one of this package's two subordinateUIDPath/subordinateGIDPath consts (or a test fixture), never caller input if err != nil { return 0, fmt.Errorf( - "microvm: virtiofsd id-mapping requires a subordinate uid range for the invoking user, "+ - "but %s is unreadable: %w (rootless podman requires the same file; "+ - "provision it with `usermod --add-subuids 100000-165535 `)", - subordinateIDPath, err) + "microvm: virtiofsd id-mapping requires a subordinate id range for the invoking user in both "+ + "%s and %s, but %s is unreadable: %w (%s)", + subordinateUIDPath, subordinateGIDPath, path, err, subordinateProvisionHint) } // Read-only handle over a small text file; a close error after a completed // parse cannot affect the already-returned base and is not actionable. - defer func() { _ = f.Close() }() + defer func() { _ = f.Close() }() // deliberate: read-only handle, nothing to flush, and the parse result is already in hand uid := os.Getuid() - // The invoking user's NAME as well as its uid: subuid(5) entries key on - // either, and shadow-utils writes the name. + // The invoking user's NAME as well as its uid: subuid(5)/subgid(5) entries + // key on either, and shadow-utils writes the name. name := "" if u, lookupErr := user.LookupId(strconv.Itoa(uid)); lookupErr == nil { name = u.Username } - return parseSubordinateIDBase(f, uid, name) -} - -// VerifySubordinateIDRange is the startup axis over the same read: it resolves -// the invoking user's subordinate base and discards it, so a host with no -// subordinate range fails preflight with the fix named rather than at the first -// session boot as a daemon-socket timeout. -func VerifySubordinateIDRange() error { - if _, err := SubordinateIDBase(); err != nil { - return err - } - return nil + return parseSubordinateIDBase(f, uid, name, path) } -// parseSubordinateIDBase is the pure half: the first subuid(5) entry whose owner -// field matches the invoking user by name or by uid, returning its base. The -// format is `::` with `#` comments; a malformed or +// parseSubordinateIDBase is the pure half: the first subuid(5)/subgid(5) entry +// whose owner field matches the invoking user by name or by uid, returning its +// base. The format is `::` with `#` comments; a malformed or // zero-count entry is skipped rather than trusted, since a range that grants no -// id cannot back a mapping. -func parseSubordinateIDBase(r io.Reader, uid int, username string) (int, error) { +// id cannot back a mapping. path names the file being parsed, so a failure on +// the gid axis is not misreported as a subuid problem. +func parseSubordinateIDBase(r io.Reader, uid int, username, path string) (int, error) { uidStr := strconv.Itoa(uid) scanner := bufio.NewScanner(r) for scanner.Scan() { @@ -106,16 +156,16 @@ func parseSubordinateIDBase(r io.Reader, uid int, username string) (int, error) return base, nil } if err := scanner.Err(); err != nil { - return 0, fmt.Errorf("microvm: reading %s: %w", subordinateIDPath, err) + return 0, fmt.Errorf("microvm: reading %s: %w", path, err) } who := username if who == "" { who = "uid " + uidStr } return 0, fmt.Errorf( - "microvm: virtiofsd id-mapping requires a subordinate uid range for %s in %s, but none is allocated "+ - "(newuidmap validates the mapped host range against subuid(5), so virtiofsd would die before binding "+ - "its socket); rootless podman requires the same — provision one with "+ - "`usermod --add-subuids 100000-165535 %s` and the matching --add-subgids", - who, subordinateIDPath, who) + "microvm: virtiofsd id-mapping requires a subordinate id range for %s in %s, but none is allocated "+ + "(newuidmap validates the mapped host uid range against %s and newgidmap validates the gid range "+ + "against %s — the two are INDEPENDENT allocations, so one being present does not cover the other, "+ + "and virtiofsd would die after binding its socket); %s", + who, path, subordinateUIDPath, subordinateGIDPath, subordinateProvisionHint) } diff --git a/go/internal/runtime/microvm_isolation_microvm_test.go b/go/internal/runtime/microvm_isolation_microvm_test.go index 056c09be..d265e644 100644 --- a/go/internal/runtime/microvm_isolation_microvm_test.go +++ b/go/internal/runtime/microvm_isolation_microvm_test.go @@ -45,6 +45,7 @@ import ( "strings" "syscall" "testing" + "time" "github.com/RigelBuild/compass/go/internal/agentuid" "github.com/RigelBuild/compass/go/internal/microvmtest" @@ -55,6 +56,27 @@ import ( // subtree, so it is a distinctive sentinel rather than a generic word. const outsideCanaryBody = "HOST-ONLY-CANARY-8f3ac1d0-must-never-be-readable-from-a-guest" +// The bounds on the quota-enforcement leg's guest write. It must cross the +// project byte bound to observe EDQUOT, but the cost of crossing it is the +// operator's quota size, which the test does not control — and the 120s +// per-exec cap (execDefaultTimeout) is hard, so an unbounded fill fails as a +// transport timeout rather than with the verdict the leg exists to assert. +// +// quotaFillMarginMiB is how far past the remaining headroom to write: enough to +// be unambiguously over the bound even if usage shifts between the pre-read and +// the write, small enough to be free. +// +// quotaFillCeilingMiB is the largest fill this leg will attempt. 1 GiB over +// virtio-fs is comfortably inside the cap on any box that can run this suite at +// all, while a 10 GiB production-sized quota is not — so a volume with more +// headroom than this SKIPS with the reason named rather than timing out. The +// operator's quota volume for this leg is expected to be purpose-sized (a few +// hundred MiB), not a production project. +const ( + quotaFillMarginMiB = 64 + quotaFillCeilingMiB = 1024 +) + // isolationSession boots one session against a fresh volume dir and returns the // runtime, its id, and the host-side volume path. Teardown is registered so a // failed assertion still tears the VM down. The volume is a t.TempDir() child so @@ -110,6 +132,23 @@ type crossSessionAttempt struct { forbid []string } +// sweepBatchSize is how many paths one awk invocation is handed. The scan's +// dominant cost is process creation, against the HARD 120s per-exec cap +// (execDefaultTimeout, microvm_lifecycle.go). One-spawn-per-file over roots +// including "/" ran the cross-tenant row at ~72s — ~60% of the cap, i.e. a flake +// waiting for a loaded box. Batching collapses that fork cost to per-batch, +// since awk takes many FILENAMEs per run and reports which file each match came +// from itself. +// +// Measured in-guest on this box over an identical 425-file tree, needle in the +// last file: 644ms at batch size 1 versus 47ms at 200 — a 13.7x reduction, i.e. +// batching removes essentially all of the ~1.5ms/file spawn overhead. +// +// 200 is bounded by the guest's ARG_MAX rather than by taste: paths average well +// under 128 bytes, so a 200-path argv stays far inside the limit while cutting +// spawns by two orders of magnitude. +const sweepBatchSize = 200 + // sweepScript builds a recursive content search the guest can actually run. The // guest image ships bash 5.3 and awk but NO grep and NO find, so a `grep -r` // row exits 127 without searching anything — a vacuous pass that looks like @@ -122,19 +161,33 @@ type crossSessionAttempt struct { // and the path appears if a file under another tenant's volume was reachable at // all. // +// BATCHED, one awk per sweepBatchSize files rather than one per file: the +// per-file variant spent nearly all its time forking (see sweepBatchSize) and +// sat at ~60% of the 120s exec cap. awk is handed many FILENAMEs at once and +// reports the matching one itself, so the output contract is unchanged. +// // Two things stop the sweep from finding ITS OWN needle, which would be a false // escape report rather than a real one: // // - The needle travels in an EXPORTED ENV VAR, never in argv. Passed as -// `awk -v`, it lands in the searcher's own /proc/self/cmdline, so the sweep -// matches the string it is looking for in its own command line. +// `awk -v`, it would land in the searcher's own /proc/self/cmdline, so the +// sweep would match the string it is looking for in its own command line. // - /proc, /sys and /dev are skipped. They are synthetic kernel interfaces // that cannot hold another tenant's volume, so excluding them removes the // self-match surface (the environ/cmdline of the running searcher) without // narrowing what the row actually probes. func sweepScript(needle, roots string) string { + // The awk program: scan every FILENAME handed to this invocation, print + // `:` per match, and exit non-zero when the batch had none — so + // the caller's `found` accumulator keeps grep's semantics across batches. + const awkProg = `index($0, ENVIRON["SWEEP_NEEDLE"]) { print FILENAME ":" $0; hit=1 } END { exit !hit }` return "export SWEEP_NEEDLE='" + needle + "'; " + - "shopt -s globstar nullglob dotglob; found=1; " + + "shopt -s globstar nullglob dotglob; found=1; batch=(); " + + // scan() runs one awk over the accumulated batch and clears it. Guarded + // on a non-empty batch so a trailing flush with nothing pending does not + // invoke awk on zero files (which would read stdin and hang). + "scan() { ((${#batch[@]})) || return 0; " + + "if awk '" + awkProg + "' \"${batch[@]}\" 2>/dev/null; then found=0; fi; batch=(); }; " + "for root in " + roots + "; do " + "for f in \"$root\"/**/*; do " + // Collapse repeated slashes before matching: a "/" root globs to @@ -144,8 +197,9 @@ func sweepScript(needle, roots string) string { "n=$f; while [[ $n == //* ]]; do n=${n#/}; done; " + "case $n in /proc/*|/sys/*|/dev/*) continue;; esac; " + "[[ -f $f && -r $f ]] || continue; " + - "if awk 'index($0, ENVIRON[\"SWEEP_NEEDLE\"]) { print FILENAME \":\" $0; hit=1 } END { exit !hit }' \"$f\" 2>/dev/null; then found=0; fi; " + - "done; done 2>/dev/null; exit $found" + "batch+=(\"$f\"); " + + "((${#batch[@]} >= " + strconv.Itoa(sweepBatchSize) + ")) && scan; " + + "done; done 2>/dev/null; scan; exit $found" } // TestMicroVMSweepScriptFindsItsNeedle is the non-vacuity control for @@ -175,6 +229,73 @@ func TestMicroVMSweepScriptFindsItsNeedle(t *testing.T) { t.Logf("sweep control: found the planted canary -> exit %d, %q", code, strings.TrimSpace(truncate(out))) } +// TestMicroVMSweepScriptFindsANeedleAcrossBatches is the control for the +// BATCHING specifically, which the single-file control above cannot reach: with +// only a handful of files the sweep never fills a batch, so the mid-loop scan +// and the accumulator that carries a hit across batches are both dead code in +// that run — and the cross-tenant row now completes in ~26ms over the targeted +// roots, so it does not exercise them either. +// +// Planting well over sweepBatchSize files and putting the needle ONLY in the +// last one forces two things the batched form must get right: +// +// 1. the mid-loop `scan` flushes full batches instead of accumulating an argv +// past ARG_MAX; +// 2. the trailing flush runs, and `found` survives the batch that matched — +// a batched sweep whose exit status came from the LAST awk alone would +// report "not found" whenever the needle sat in any earlier batch, so the +// early-needle case below pins the other side of the same accumulator. +func TestMicroVMSweepScriptFindsANeedleAcrossBatches(t *testing.T) { + env := microvmtest.Require(t) + m, id, _ := isolationSession(t, env, "iso-sweep-batch") + + // Comfortably more than two full batches, so at least two mid-loop flushes + // happen before the trailing one. + fileCount := sweepBatchSize*2 + 25 + plant := "mkdir -p /workspace/many && for i in $(seq 1 " + strconv.Itoa(fileCount) + "); do " + + "printf 'filler line %s\\n' \"$i\" > /workspace/many/f$i.txt; done && ls /workspace/many | wc -l" + out, code := guestSh(t, m, id, plant) + if code != 0 { + t.Fatalf("planting %d filler files: exit %d, %q", fileCount, code, truncate(out)) + } + if got := strings.TrimSpace(out); got != strconv.Itoa(fileCount) { + t.Fatalf("planted %q files, want %d; the batch boundary would not be crossed", got, fileCount) + } + + for _, tt := range []struct{ name, file, needle string }{ + // Last file: only the TRAILING flush can find it. + {"in the final batch", "/workspace/many/f" + strconv.Itoa(fileCount) + ".txt", "SWEEP-BATCH-LAST-6c1e8f30"}, + // First file: found by a MID-LOOP flush, so `found` must survive every + // later batch that matched nothing. + {"in the first batch", "/workspace/many/f1.txt", "SWEEP-BATCH-FIRST-91ad47b2"}, + } { + t.Run(tt.name, func(t *testing.T) { + if out, code := guestSh(t, m, id, + "printf '%s\\n' '"+tt.needle+"' >> "+tt.file); code != 0 { + t.Fatalf("planting the needle in %s: exit %d, %q", tt.file, code, truncate(out)) + } + start := time.Now() + out, code := guestSh(t, m, id, sweepScript(tt.needle, "/workspace")) + elapsed := time.Since(start) + if code != 0 { + t.Fatalf("the batched sweep did not find a needle planted %s of %d files (exit %d, %q); "+ + "the cross-tenant sweep row would pass vacuously", tt.name, fileCount, code, truncate(out)) + } + if !strings.Contains(out, tt.needle) { + t.Fatalf("the batched sweep exited 0 but its output %q does not carry the needle; "+ + "the content check would be vacuous", truncate(out)) + } + if !strings.Contains(out, filepath.Base(tt.file)) { + t.Errorf("the batched sweep output %q does not name the matching file %s; awk must report the "+ + "FILENAME each match came from, which is what makes a batched scan as diagnostic as a per-file one", + truncate(out), tt.file) + } + t.Logf("batched sweep over %d files (batch size %d) found the needle %s in %v: %q", + fileCount, sweepBatchSize, tt.name, elapsed.Round(time.Millisecond), strings.TrimSpace(truncate(out))) + }) + } +} + // TestMicroVMVolumeTraversalConfined is the path-traversal leg: the guest tries // to escape /workspace three ways and is confined every time, proven from both // sides of the boundary. @@ -329,13 +450,30 @@ func TestMicroVMCrossSessionVolumeUnreachable(t *testing.T) { // running. This is the same sweep in what the guest DOES have // (bash globstar + awk), and sweepScript keeps grep's exit // semantics: non-zero when nothing matched. - script: sweepScript(tenantBSecret, "/ /tmp /mnt /media /run /var /home /workspace"), + // + // "/" is deliberately NOT a root. The discriminating question is + // whether B's volume path is REACHABLE from A, not whether the + // read-only nix store holds the needle — and sweeping "/" pulled + // the guest's entire ~21.8k-file rootfs through the scan, which + // (even batched) buys nothing this targeted list plus B's own + // volume parent does not already answer. The parent of volume B is + // included explicitly so the one tree that COULD hold the secret is + // definitely walked; a sweep that skipped it would be the vacuous + // pass this row exists to avoid. + script: sweepScript(tenantBSecret, + "/tmp /mnt /media /run /var /home /workspace "+filepath.Dir(volumeB)), forbid: []string{volumeB}, }, } for name, attempt := range attempts { t.Run("A cannot reach "+name, func(t *testing.T) { + // Timed: the sweep row is the expensive one, and its cost is only + // bounded relative to the HARD 120s per-exec cap (execDefaultTimeout) + // — a row creeping back toward that cap is a flake on a loaded box, + // so the margin is reported rather than left to be rediscovered. + start := time.Now() out, code := guestSh(t, mA, idA, attempt.script) + elapsed := time.Since(start) if strings.Contains(out, tenantBSecret) { t.Fatalf("tenant A READ tenant B's secret via %s — CROSS-TENANT ESCAPE.\noutput: %q", name, out) } @@ -349,7 +487,15 @@ func TestMicroVMCrossSessionVolumeUnreachable(t *testing.T) { t.Errorf("cross-tenant attempt %q exited 0 (output %q); a confined command must fail", name, truncate(out)) } - t.Logf("unreachable: %s -> exit %d, %q", name, code, strings.TrimSpace(truncate(out))) + // Half the cap is the flake line: past it, a slower box turns this + // confinement assertion into a transport TimeoutError, which guestSh + // treats as fatal — a failure that says nothing about isolation. + if elapsed > execDefaultTimeout/2 { + t.Errorf("attempt %q took %v, over half the %v per-exec cap; it is a flake on a loaded box — "+ + "narrow its roots or increase the scan batch size", name, elapsed, execDefaultTimeout) + } + t.Logf("unreachable: %s -> exit %d in %v (cap %v), %q", + name, code, elapsed.Round(time.Millisecond), execDefaultTimeout, strings.TrimSpace(truncate(out))) }) } @@ -553,9 +699,31 @@ func TestMicroVMVolumeQuotaEnforcedInGuest(t *testing.T) { // Write past the byte bound: dd until it fails. The guest MUST hit // ENOSPC/EDQUOT rather than consuming the whole host filesystem. - fill := "dd if=/dev/zero of=/workspace/fill bs=1M count=" + - strconv.FormatInt(before.LimitBytes/(1<<20)+64, 10) + " 2>&1" + // + // Only the REMAINING HEADROOM plus a margin, never the whole limit. The old + // `LimitBytes/MiB + 64` wrote the entire project limit again on top of + // whatever was already used, so against a realistically-sized operator quota + // (10GiB) it was a 10GiB guest write over virtio-fs — past the 120s per-exec + // cap, which surfaces as a transport TimeoutError (fatal at guestSh) instead + // of the ENOSPC/EDQUOT verdict this leg exists to prove. Headroom+margin + // crosses the bound by exactly the same amount while writing only what is + // actually needed to cross it. + fillMiB := (before.LimitBytes-before.UsedBytes)/(1<<20) + quotaFillMarginMiB + if fillMiB > quotaFillCeilingMiB { + t.Skipf("the quota'd volume %s has %d MiB of headroom, over this leg's %d MiB ceiling: crossing the "+ + "bound would be a %d MiB guest write over virtio-fs, past the %v per-exec cap (execDefaultTimeout), "+ + "so it would fail as a transport timeout rather than with the ENOSPC/EDQUOT verdict it exists to "+ + "assert. Point $COMPASS_TEST_QUOTA_VOLUME at a purpose-sized test project (a few hundred MiB); the "+ + "operator's quota volume for this leg is expected to be small. Observed: %s", + volume, fillMiB-quotaFillMarginMiB, quotaFillCeilingMiB, fillMiB, execDefaultTimeout, before) + } + t.Logf("filling %d MiB (headroom %d MiB + %d MiB margin) against a %v exec cap; limit %d B, used %d B", + fillMiB, fillMiB-quotaFillMarginMiB, quotaFillMarginMiB, execDefaultTimeout, before.LimitBytes, before.UsedBytes) + + fill := "dd if=/dev/zero of=/workspace/fill bs=1M count=" + strconv.FormatInt(fillMiB, 10) + " 2>&1" + start := time.Now() out, code := guestSh(t, m, id, fill) + elapsed := time.Since(start) if code == 0 { t.Fatalf("the guest wrote past the project byte bound (%d B) without failing — the quota is not enforced.\noutput: %q", before.LimitBytes, out) @@ -563,7 +731,14 @@ func TestMicroVMVolumeQuotaEnforcedInGuest(t *testing.T) { if !strings.Contains(out, "No space left") && !strings.Contains(out, "Disk quota exceeded") { t.Errorf("the over-bound write failed with %q, want an ENOSPC/EDQUOT diagnostic", strings.TrimSpace(out)) } - t.Logf("over-bound write confined: exit %d, %q", code, strings.TrimSpace(truncate(out))) + // The ceiling above is a static estimate; this is the measured check that + // the chosen byte count really fit the budget rather than nearly missing it. + if elapsed > execDefaultTimeout/2 { + t.Errorf("the %d MiB fill took %v, over half the %v per-exec cap; lower quotaFillCeilingMiB or use a "+ + "smaller test project", fillMiB, elapsed, execDefaultTimeout) + } + t.Logf("over-bound write confined: exit %d in %v (cap %v), %q", + code, elapsed.Round(time.Millisecond), execDefaultTimeout, strings.TrimSpace(truncate(out))) // The HOST filesystem must stay healthy: the mount root's free space is // still ample, i.e. the guest exhausted its project, not the filesystem. diff --git a/go/internal/runtime/microvm_quota.go b/go/internal/runtime/microvm_quota.go index 3412de7c..bbd6b8bd 100644 --- a/go/internal/runtime/microvm_quota.go +++ b/go/internal/runtime/microvm_quota.go @@ -86,10 +86,12 @@ type QuotaReading struct { } // Active reports whether an ENFORCED project quota is scoping Path: the path's -// own statfs totals are strictly smaller than the mount root's, which only the -// kernel's project-quota projection produces (see the file header). Either axis -// suffices — an operator may bound bytes, inodes, or both — but the inode arm is -// only consulted on a filesystem that reports inode counts at all. +// own statfs totals are smaller than the mount root's, which only the kernel's +// project-quota projection produces (see the file header). Either axis suffices +// — an operator may bound bytes, inodes, or both — but the two axes are +// ASYMMETRIC: the byte arm accepts any strict inequality, while the inode arm +// requires a 1/16 jitter margin (the reason is two paragraphs down). The inode +// arm is also only consulted on a filesystem that reports inode counts at all. // // Deliberately conservative in one direction: a project quota whose byte limit // happens to equal the whole filesystem's size projects no observable difference @@ -179,7 +181,9 @@ func verifyVolumeQuota(path string, want VolumeQuota, read quotaReadFn) (QuotaRe if path == "" { return QuotaReading{}, fmt.Errorf( "microvm preflight: session-volume quota: no volume path to verify: %s", - "set --microvm-runroot or $COMPASS_MICROVM_RUNROOT") + "set --microvm-volume-root or $COMPASS_MICROVM_VOLUME_ROOT to the parent dir session volumes "+ + "are minted under (NOT the run-root: that is the socket dir, held to a short /tmp path by the "+ + "AF_UNIX sun_path budget, and routinely a different filesystem, so it is not a valid proxy)") } reading, err := read(path) if err != nil { diff --git a/go/internal/runtime/microvm_quota_linux.go b/go/internal/runtime/microvm_quota_linux.go index e6b0484c..a155816e 100644 --- a/go/internal/runtime/microvm_quota_linux.go +++ b/go/internal/runtime/microvm_quota_linux.go @@ -28,10 +28,27 @@ func readVolumeQuota(path string) (QuotaReading, error) { if err := syscall.Statfs(path, &at); err != nil { return QuotaReading{}, fmt.Errorf("statfs %q: %w", path, err) } - root, err := mountRoot(path) + root, distinct, err := mountRoot(path) if err != nil { return QuotaReading{}, err } + if !distinct { + // The volume root IS the mount point, so the comparison has no + // UNPROJECTED reference: both statfs calls would target the same path, + // LimitBytes would equal FilesystemBytes identically, and Active() would + // be false BY CONSTRUCTION rather than by observation. Reporting that as + // "no quota" refuses a QuotaRequired startup on a correctly provisioned + // host, so it is an INCONCLUSIVE probe with the fix named — the same + // posture as an unreadable ancestor (mountRoot's doc). + return QuotaReading{}, fmt.Errorf( + "locating an unprojected reference for %q: that path IS the mount point of its filesystem (%q), "+ + "so there is no unquota'd ancestor to compare its statfs totals against and whether a project "+ + "quota scopes it is INDETERMINATE; point --microvm-volume-root (or "+ + "$COMPASS_MICROVM_VOLUME_ROOT) at a SUBDIRECTORY of the quota'd filesystem rather than at its "+ + "mount point — that subdirectory is also where per-session volumes are actually minted, and it "+ + "is the dir the project id and FS_XFLAG_PROJINHERIT belong on", + path, root) + } var atRoot syscall.Statfs_t if err := syscall.Statfs(root, &atRoot); err != nil { return QuotaReading{}, fmt.Errorf("statfs mount root %q: %w", root, err) @@ -68,7 +85,8 @@ func blocksToBytes(blocks uint64, bsize int64) int64 { // mountRoot walks path's ancestors until the device number changes, returning // the deepest ancestor still on the same filesystem — the mount point path -// belongs to. A project quota does NOT change st_dev (it is an accounting scope +// belongs to — plus whether the walk actually CROSSED a device boundary to get +// there. A project quota does NOT change st_dev (it is an accounting scope // inside one filesystem, not a separate device), so this reliably reaches the // unprojected reference point the comparison needs. // @@ -76,36 +94,47 @@ func blocksToBytes(blocks uint64, bsize int64) int64 { // otherwise walk the link's lexical parents, which may live on a different // filesystem entirely and make the comparison meaningless. // -// An UNREADABLE ancestor is an INCONCLUSIVE probe, not a mount root. The -// comparison is only sound against an UNPROJECTED reference, and same-device is -// not the same as unprojected: FS_XFLAG_PROJINHERIT propagates a project id down -// the tree, so a walk halted by an EACCES may stop at an ancestor inside the -// SAME project (or inside a larger enclosing one), whose totals are themselves -// rewritten. Comparing against that yields a bogus verdict — reporting a quota -// active because the reference happened to be a bigger project, which is the -// fail-OPEN direction on a security-relevant preflight. So the error propagates -// through readVolumeQuota, and a QuotaRequired startup fails CLOSED naming the -// unreadable ancestor (the posture TestReadVolumeQuotaAbsentPath pins for a -// missing path). -func mountRoot(path string) (string, error) { +// distinct=false means the resolved path IS ITS OWN mount point, so there is NO +// unprojected reference to compare against — statfs'ing it twice yields +// identical totals and Active() is false BY CONSTRUCTION, not by observation. +// Verified by instrumented run: mountRoot("/tmp") and mountRoot("/") each return +// their own argument. That is the most natural production layout (a dedicated +// XFS mounted at, say, /srv/compass/volumes), so treating it as a negative +// verdict would refuse startup on a CORRECTLY provisioned host. The caller turns +// it into a distinct INCONCLUSIVE error instead — the same fail-closed-but-named +// posture as the unreadable-ancestor case below. +// +// An UNREADABLE ancestor is likewise an INCONCLUSIVE probe, not a mount root. +// The comparison is only sound against an UNPROJECTED reference, and same-device +// is not the same as unprojected: FS_XFLAG_PROJINHERIT propagates a project id +// down the tree, so a walk halted by an EACCES may stop at an ancestor inside +// the SAME project (or inside a larger enclosing one), whose totals are +// themselves rewritten. Comparing against that yields a bogus verdict — +// reporting a quota active because the reference happened to be a bigger +// project, which is the fail-OPEN direction on a security-relevant preflight. So +// the error propagates through readVolumeQuota, and a QuotaRequired startup +// fails CLOSED naming the unreadable ancestor (the posture +// TestReadVolumeQuotaAbsentPath pins for a missing path). +func mountRoot(path string) (root string, distinct bool, err error) { resolved, err := filepath.EvalSymlinks(path) if err != nil { - return "", fmt.Errorf("resolving %q: %w", path, err) + return "", false, fmt.Errorf("resolving %q: %w", path, err) } dev, err := deviceOf(resolved) if err != nil { - return "", err + return "", false, err } current := resolved for { parent := filepath.Dir(current) if parent == current { - // Reached "/" — the filesystem root is the mount root. - return current, nil + // Reached "/" — the filesystem root is the mount root. It is a + // distinct reference only if the walk moved off the given path. + return current, current != resolved, nil } parentDev, err := deviceOf(parent) if err != nil { - return "", fmt.Errorf( + return "", false, fmt.Errorf( "locating the mount root of %q: ancestor %q is not statable (%w), so no unprojected "+ "reference point could be reached and whether a project quota scopes the volume is "+ "INDETERMINATE; make the ancestor path traversable by the Runner uid (chmod o+x) "+ @@ -113,7 +142,7 @@ func mountRoot(path string) (string, error) { path, parent, err) } if parentDev != dev { - return current, nil + return current, current != resolved, nil } current = parent } diff --git a/go/internal/runtime/microvm_quota_linux_test.go b/go/internal/runtime/microvm_quota_linux_test.go index 16c8eeee..a75edaa7 100644 --- a/go/internal/runtime/microvm_quota_linux_test.go +++ b/go/internal/runtime/microvm_quota_linux_test.go @@ -45,10 +45,10 @@ func TestMountRootUnreadableAncestorIsInconclusive(t *testing.T) { // Restore the mode so t.TempDir's cleanup can remove the tree. t.Cleanup(func() { _ = os.Chmod(blocked, 0o700) }) - root, err := mountRoot(volume) + root, distinct, err := mountRoot(volume) if err == nil { - t.Fatalf("mountRoot(%q) = %q with no error; a stat-blocked ancestor must be an INCONCLUSIVE probe, "+ - "not a mount root — comparing against a possibly-projected ancestor can report a bogus active quota", volume, root) + t.Fatalf("mountRoot(%q) = %q (distinct=%v) with no error; a stat-blocked ancestor must be an INCONCLUSIVE probe, "+ + "not a mount root — comparing against a possibly-projected ancestor can report a bogus active quota", volume, root, distinct) } if root != "" { t.Errorf("mountRoot returned the reference %q alongside its error; an inconclusive probe must yield no reference", root) @@ -60,23 +60,127 @@ func TestMountRootUnreadableAncestorIsInconclusive(t *testing.T) { } // TestMountRootResolvesAnUnblockedPath is the positive control for the walk: on -// a path the Runner can traverse to its mount point, mountRoot still resolves a -// real reference. Without it the fail-closed assertion above could pass on a -// mountRoot that had simply stopped working. +// a path the Runner can traverse to its mount point, mountRoot resolves a real +// reference AND reports that it crossed a device boundary to get there. // -// The ancestor-walk's own error branch is not reachable through file modes: an -// EACCES that blocks stat(parent) necessarily blocks resolving the leaf through -// that same parent, so EvalSymlinks refuses first (the case above). The branch -// stays because a non-permission stat failure — an ancestor unlinked mid-walk, -// an EIO — must fail closed rather than return a same-device guess. +// It asserts the walk did real WORK, not merely that it returned something: the +// root must be a strict prefix ANCESTOR of the input, and a nested subdirectory +// must resolve to the SAME root. Both fail on an identity-returning mountRoot — +// which is exactly what the real one does for a path that IS a mount point, the +// degeneracy the old non-empty-and-no-error assertion could not catch. func TestMountRootResolvesAnUnblockedPath(t *testing.T) { - root, err := mountRoot(t.TempDir()) + base := t.TempDir() + nested := filepath.Join(base, "a", "b", "c") + if err := os.MkdirAll(nested, 0o700); err != nil { + t.Fatalf("creating the nested tree: %v", err) + } + // t.TempDir resolves through symlinks the same way mountRoot does, so the + // prefix comparison below is against the resolved form on a box where + // $TMPDIR is a symlink. + resolvedBase, err := filepath.EvalSymlinks(base) + if err != nil { + t.Fatalf("resolving the temp dir: %v", err) + } + + root, distinct, err := mountRoot(base) if err != nil { t.Fatalf("mountRoot on an unblocked temp dir = %v, want a resolved mount root", err) } - if root == "" { - t.Fatal("mountRoot resolved an empty mount root on an unblocked path") + if !distinct { + t.Fatalf("mountRoot(%q) reported distinct=false; a temp dir is never its own mount point, so the "+ + "device-boundary walk did not run", base) + } + // STRICT ancestor: an identity-returning implementation returns the input + // itself, which this rejects. + if root == resolvedBase { + t.Fatalf("mountRoot(%q) = %q — the input itself. The walk resolved no unprojected reference; an "+ + "identity implementation would pass a mere non-empty check", base, root) + } + if !strings.HasPrefix(resolvedBase, strings.TrimSuffix(root, "/")+"/") { + t.Fatalf("mountRoot(%q) = %q, which is not a prefix ancestor of the input", base, root) + } + // A NESTED path must land on the SAME mount root: the walk climbs to a + // device boundary, not to some depth-relative ancestor. + nestedRoot, nestedDistinct, err := mountRoot(nested) + if err != nil { + t.Fatalf("mountRoot(%q) = %v, want the same mount root as its ancestor", nested, err) + } + if !nestedDistinct { + t.Errorf("mountRoot(%q) reported distinct=false for a deeply nested path", nested) + } + if nestedRoot != root { + t.Fatalf("mountRoot(%q) = %q but mountRoot(%q) = %q; a device-boundary walk must reach the same "+ + "mount root from both (an identity implementation returns each input instead)", nested, nestedRoot, base, root) + } + t.Logf("mount-root walk: %q and %q both resolve to %q", base, nested, root) +} + +// TestMountRootRecognizesASelfReferentialPath is MED-3's core: a path that IS +// its own mount point must be reported as NON-distinct, because there is then no +// unprojected reference to compare statfs totals against. +// +// /tmp is a real mount on this box (a separate st_dev from /), so mountRoot +// returns /tmp itself — verified by instrumented run. Accepting that as a +// reference makes LimitBytes == FilesystemBytes identically, Active() false BY +// CONSTRUCTION, and a QuotaRequired startup a refusal on a host whose volume +// root is (as production layouts naturally do) the mount point of a dedicated +// quota'd filesystem. +func TestMountRootRecognizesASelfReferentialPath(t *testing.T) { + const mountPoint = "/tmp" + if _, err := os.Stat(mountPoint); err != nil { + t.Skipf("%s is not present: %v", mountPoint, err) + } + resolved, err := filepath.EvalSymlinks(mountPoint) + if err != nil { + t.Skipf("resolving %s: %v", mountPoint, err) + } + + root, distinct, err := mountRoot(mountPoint) + if err != nil { + t.Fatalf("mountRoot(%q) = %v", mountPoint, err) + } + if root != resolved { + t.Skipf("%s is not its own mount point on this box (mountRoot = %q); the self-referential case "+ + "cannot be exercised here", mountPoint, root) + } + if distinct { + t.Fatalf("mountRoot(%q) = %q with distinct=true, but the resolved path IS the returned root: no "+ + "device boundary was crossed, so there is NO unprojected reference and the comparison would be "+ + "self-referential (LimitBytes == FilesystemBytes identically, Active() false by construction)", + mountPoint, root) + } +} + +// TestReadVolumeQuotaRefusesASelfReferentialVolumeRoot is MED-3 one layer up: +// the self-referential case must propagate as a DISTINCT inconclusive error +// naming the volume-root knob and the subdirectory fix, not collapse into the +// generic "no enforced project quota" verdict — which would refuse startup on a +// correctly provisioned host and send the operator chasing a quota that is +// already there. +func TestReadVolumeQuotaRefusesASelfReferentialVolumeRoot(t *testing.T) { + const mountPoint = "/tmp" + root, distinct, err := mountRoot(mountPoint) + if err != nil { + t.Skipf("mountRoot(%q) = %v", mountPoint, err) + } + if distinct { + t.Skipf("%s is not its own mount point on this box (root %q); the self-referential case cannot be "+ + "exercised here", mountPoint, root) + } + + reading, readErr := readVolumeQuota(mountPoint) + if readErr == nil { + t.Fatalf("readVolumeQuota(%q) = %s with no error; a volume root that IS its own mount point has no "+ + "unprojected reference, so it must be INCONCLUSIVE rather than a verdict", mountPoint, reading) + } + for _, part := range []string{ + mountPoint, "IS the mount point", "INDETERMINATE", "--microvm-volume-root", "SUBDIRECTORY", + } { + if !strings.Contains(readErr.Error(), part) { + t.Errorf("error %q does not name %q", readErr.Error(), part) + } } + t.Logf("self-referential volume root refused: %v", readErr) } // TestReadVolumeQuotaPropagatesInconclusiveMountRoot is the same posture one diff --git a/go/internal/runtime/microvm_quota_test.go b/go/internal/runtime/microvm_quota_test.go index 422b88a4..e01726c8 100644 --- a/go/internal/runtime/microvm_quota_test.go +++ b/go/internal/runtime/microvm_quota_test.go @@ -245,11 +245,15 @@ func TestVerifyVolumeQuota(t *testing.T) { wantParts: []string{volume, "permission denied"}, }, { - name: "an empty volume path names the run-root knob", + // The knob must be the VOLUME root, never the retired run-root: the + // run-root is the socket dir on a routinely different filesystem, so + // setting it cannot make this check pass. Pinning the run-root text + // here is what regression-locked that stale advice. + name: "an empty volume path names the volume-root knob, not the retired run-root", path: "", want: VolumeQuota{}, read: func(string) (QuotaReading, error) { return quotaReading(10<<30, 0, 0, 0), nil }, - wantParts: []string{"--microvm-runroot", "COMPASS_MICROVM_RUNROOT"}, + wantParts: []string{"--microvm-volume-root", "COMPASS_MICROVM_VOLUME_ROOT"}, }, } for _, tt := range tests {