feat: stage host mount trees for virtiofsd exports - #7661
Conversation
Cloud Hypervisor v53 and virtiofsd v1.10 have no per-path readonly option, and a guest-side `ro` mount is not a security boundary. Add an optional, strongly typed enforcement input to `VirtiofsdManager.start()` that stages a private host mount tree per export: recursive bind, private propagation, recursive read-only enforcement, then nested writable bind overlays. When a tree is staged, virtiofsd is launched with `--announce-submounts`. The API is inert: with no enforcement argument the manager behaves exactly as before, including byte-identical virtiofsd arguments. Runtime wiring lands separately. libmount's `ro=recursive` option argument is deliberately not used. On util-linux 2.39.3 it exits 0 while leaving carried-in submounts read-write, so enforcement instead remounts each mount in the tree read-only deepest-first. Every tree is verified against `/proc/self/mountinfo` before virtiofsd starts and staging fails closed if the boundary cannot be proven. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
✅ Coverage Check PassedOverall Coverage
📁 Per-file Coverage Changes (2 files)
✨ New Files (1 files)
Coverage comparison generated by |
There was a problem hiding this comment.
Pull request overview
Adds inert host-side mount-tree enforcement for selective writable virtiofsd exports.
Changes:
- Stages recursively read-only private mount trees with writable overlays.
- Adds mount verification, rollback, and cleanup.
- Documents and extensively tests enforcement behavior.
Show a summary per file
| File | Description |
|---|---|
src/cloud-hypervisor/virtiofsd.ts |
Integrates optional staged mount plans. |
src/cloud-hypervisor/virtiofsd.test.ts |
Tests manager staging and cleanup. |
src/cloud-hypervisor/mount-tree.ts |
Implements mount-tree enforcement. |
src/cloud-hypervisor/mount-tree.test.ts |
Tests validation and lifecycle behavior. |
docs/cloud-hypervisor-foundation.md |
Documents the enforcement design. |
Review details
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
- Files reviewed: 5/5 changed files
- Comments generated: 2
- Review effort level: Balanced
| function assertPrivatePropagation(entry: MountInfoEntry): void { | ||
| const shared = entry.optionalFields.some((field) => field.startsWith('shared:')); | ||
| if (shared) { | ||
| throw new Error( | ||
| `Staged mount tree propagation would leak: ${entry.mountPoint} is a shared mount`, | ||
| ); | ||
| } | ||
| } |
| private async assertOverlayDestination(overlay: ResolvedOverlay): Promise<void> { | ||
| const stats = await this.options.dependencies.statPath(overlay.stagedDestination); | ||
| assertStatsMatchKind( | ||
| stats, | ||
| overlay.kind, | ||
| `writable overlay destination ${overlay.stagedDestination}`, | ||
| ); | ||
| } |
… tags Two issues found in review of the host mount-tree enforcement foundation. `lstat` on an overlay destination only reveals a symlink in the final component, while the kernel resolves every intermediate component when it binds. A `tools -> /etc` symlink carried in from the export made destination `tools/sudoers` lstat as an ordinary file and then bind over the host's `/etc/sudoers`, escaping the staged root before any post-bind verification ran. Destinations are now canonicalized and must satisfy realpath equality and containment under the staged root, mirroring source validation, and the staged root itself must be canonical for that comparison to mean anything. Verified on a live kernel: both intermediate and final symlink escapes are rejected before any bind reaches the kernel, and the host target is untouched. An enforcement plan naming an export tag that does not exist was silently discarded, so a renamed or mistyped tag would downgrade that export to unrestricted read-write. Plans are now reconciled against exports at start and an unknown tag throws. Exports without a plan still use the existing path, so partial enforcement stays supported. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
|
✅ Copilot review passed with no inline comments. @lpcox Add the |
lpcox
changed the title
|
@copilot address review feedback |
Addressed the propagation review feedback in |
|
📰 VERDICT: Smoke Copilot has concluded. All systems operational. This is a developing story. 🎤
|
|
📡 Smoke OTel Tracing completed. All tracing scenarios validated. ✅
|
|
✅ Smoke Copilot BYOK completed. Copilot BYOK mode operational. 🔓
|
|
❌ Contribution Check failed. Please review the logs for details.
|
|
✅ Smoke Claude passed
|
|
✅ Smoke Copilot BYOK AOAI (api-key) completed. Copilot AOAI BYOK (api-key) mode operational. 🔓
|
|
✨ The prophecy is fulfilled... Smoke Codex has completed its mystical journey. The stars align. 🌟 Warning Firewall blocked 3 domainsThe following domains were blocked by the firewall during workflow execution:
To allow these domains, add them to the network:
allowed:
- defaults
- "msfeed2.pkgs.visualstudio.com"
- "msfeed25.pkgs.visualstudio.com"
- "registry.npmjs.org"See Network Configuration for more information.
|
|
🛡️ Smoke Copilot Network Isolation confirmed the egress allowlist is enforced. ✅ Warning Firewall blocked 1 domainThe following domain was blocked by the firewall during workflow execution:
To allow these domains, add them to the network:
allowed:
- defaults
- "example.com"See Network Configuration for more information.
|
|
✅ Smoke Copilot BYOK AOAI (Entra) completed. Copilot AOAI BYOK (Entra) mode operational. 🔓
|
|
🔌 Smoke Services — All services reachable! ✅
|
|
Chroot tests passed! Smoke Chroot - All security and functionality tests succeeded.
|
|
✅ Build Test Suite completed successfully!
|
|
📰 VERDICT: Smoke Docker Sbx has concluded. All systems operational. This is a developing story. 🎤
|
|
❌ Smoke Gemini reports failed. Facets need polishing...
|
|
🛡️ Smoke Copilot Network Isolation confirmed the egress allowlist is enforced. ✅ Warning Firewall blocked 1 domainThe following domain was blocked by the firewall during workflow execution:
To allow these domains, add them to the network:
allowed:
- defaults
- "example.com"See Network Configuration for more information.
|
|
🚀 Security Guard has started processing this pull request |
|
✅ Smoke Copilot BYOK completed. Copilot BYOK mode operational. 🔓
|
|
📡 Smoke OTel Tracing completed. All tracing scenarios validated. ✅
|
|
✨ The prophecy is fulfilled... Smoke Codex has completed its mystical journey. The stars align. 🌟 Warning Firewall blocked 3 domainsThe following domains were blocked by the firewall during workflow execution:
To allow these domains, add them to the network:
allowed:
- defaults
- "msfeed2.pkgs.visualstudio.com"
- "msfeed25.pkgs.visualstudio.com"
- "registry.npmjs.org"See Network Configuration for more information.
|
Smoke Test: Claude Engine Validation
Overall result: PASS
|
|
Smoke Test: Copilot Engine — PASS ✅
Recent merged PRs:
Overall: PASS cc @lpcox
|
|
✅ Smoke Test: Copilot BYOK (Direct Mode) — PASS
Running in direct BYOK mode with
|
|
EGRESS_RESULT allow=pass deny=pass ✅ Allowed domain (api.github.com) reachable: Overall status: PASS @lpcox — network isolation egress enforcement verified for this PR. Warning Firewall blocked 1 domainThe following domain was blocked by the firewall during workflow execution:
To allow these domains, add them to the network:
allowed:
- defaults
- "example.com"See Network Configuration for more information.
|
|
Smoke Test: Docker Sbx — @lpcox
Overall: PASS
|
Smoke Test Results: Services Connectivity
Overall: FAIL —
|
Chroot Version Comparison Results
Overall: FAILED — Node.js version differs between host and chroot environment (v24.19.0 vs v22.23.2). The
|
Smoke Test: API Proxy OTel Tracing — Results✅ Scenario 1 — Module Loading: ✅ Scenario 2 — Test Suite: 3 suites / 68 tests passed, 0 failed ( ✅ Scenario 3 — Env Var Forwarding: ✅ Scenario 4 — Token Tracker Integration: ✅ Scenario 5 — OTEL Diagnostics: Spans were exported — Overall: All 5 scenarios pass. No regressions found.
|
🏗️ Build Test Suite Results
Overall: 8/8 ecosystems passed — PASS Note: Maven required a workaround —
|
Running in direct BYOK mode (COPILOT_PROVIDER_API_KEY + COPILOT_PROVIDER_BASE_URL) via api-proxy → Azure OpenAI (Foundry, o4-mini-aw) Overall PASS
|
|
Recent merged PRs:
Warning Firewall blocked 3 domainsThe following domains were blocked by the firewall during workflow execution:
To allow these domains, add them to the network:
allowed:
- defaults
- "msfeed2.pkgs.visualstudio.com"
- "msfeed25.pkgs.visualstudio.com"
- "registry.npmjs.org"See Network Configuration for more information.
|
|
GitHub MCP Testing: ✅
|
* feat: support filesystem allowWrite with Cloud Hypervisor Wires the write-policy planner (#7660) into the host mount-tree enforcement layer (#7661) so `filesystem.allowWrite` is actually enforced by the Cloud Hypervisor microVM runtime. Both foundations landed deliberately inert; this is the runtime integration. `filesystem-write-enforcement.ts` is the only place the two meet. It plans the policy in a dedicated startup stage before the boot loop, so an invalid allowlist aborts before virtiofsd or the guest launches, and translates each planner disposition into the merged mount-tree API: - policy absent: no enforcement argument at all, byte-identical legacy behaviour and original export objects; - unrestricted/fully writable: guest `rw`, no plan; - fully read-only: guest `ro`, staged host root `ro`, zero-overlay plan; - selectively writable: guest `rw`, staged host root `ro`, one overlay per allowed path. A zero-overlay read-only export still gets a plan rather than falling back to the legacy single bind plus remount, so a narrowed export always uses the recursively-verified staged tree. No `internalTags` are passed: Cloud Hypervisor has no analogue of Docker's always-writable log and session-state binds, and exempting `tmp-gh-aw` would defeat the narrowing a policy like `allowWrite: ["/tmp/gh-aw/agent"]` expresses. Because the host tree is the boundary, a selective export is never mounted read-only guest-side. `validateCloudHypervisorExports()` accepts a read-only `workspace` only when a plan for that tag exists, so a read-only workspace nothing enforces is still rejected. Unknown plan tags stay fail-closed and planner validation is not duplicated. Removes only the Cloud Hypervisor rejection from `filesystem-policy.ts`; sbx and Docker-in-Docker still fail closed. Adds live-KVM smoke cases proving an allowed directory and file write persists to the host while sibling, parent, create, truncate, rename, and delete outside the allowlist fail, plus empty-allowlist narrowing and a fail-closed abort. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> * Potential fix for pull request finding Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com> * fix: gate compose write policy by runtime and repair allowWrite smoke probes Two review blockers on the Cloud Hypervisor allowWrite integration. 1. CRITICAL — `filesystem.allowWrite` leaked into compose generation. `writeConfigs()` -> `generateDockerCompose()` -> `buildAgentVolumes()` called Docker's `applyFilesystemWritePolicy()` unconditionally. Compose still builds an agent service object for microVM runtimes (so infra containers can wire depends_on edges) even though it is omitted from the emitted file, so a Cloud Hypervisor policy was evaluated against compose bind mounts the agent never uses. Guest paths such as `/workspace/allowed` -- and the motivating `/tmp/gh-aw/agent` -- are not backed by any host bind mount, so they threw "not an existing path within a writable host mount" during writeConfigs(), long before the Cloud Hypervisor planner ran. Adds `resolveComposeFilesystemAllowWrite()`, which returns the policy only when `runtimeUsesComposeAgent()` is true, and routes both consumers through it: the `applyFilesystemWritePolicy()` call in volume-builder and the `dropUnbackedHostHomeOverlays()` toggle in optional-services. Docker and gVisor behaviour is unchanged; sbx still fails closed in filesystem-policy. Audited every other `filesystemAllowWrite` reader: the remaining ones are the type declaration, config mapping, the runtime compatibility gate, and the Cloud Hypervisor planner itself, all correct. 2. HIGH — the live smoke truncate probe killed the guest shell. The guest runs BusyBox ash, where a redirection failure on a POSIX *special* builtin is fatal. `! : > /workspace/input.txt` therefore exited the shell before the rename and delete probes ran, while the host post-checks still passed vacuously -- a write never attempted also never changes anything. Reproduced in busybox: the old chain exits 1 and never prints the rename/delete markers; `printf ''` continues correctly. Every write-denial probe is now a regular builtin or external command, wrapped in a subshell so a fatal error stays contained, and followed by a sentinel. A new `assert_sentinels` helper requires all of them after the case, so a shell that dies partway can no longer look like a pass. Regression coverage: top-level `generateDockerCompose` tests proving a Cloud Hypervisor config with `/workspace/allowed` neither throws nor rewrites any emitted compose volume, while Docker/gVisor still narrow mounts and still fail closed on an unbacked path; plus script-contract tests banning special-builtin write probes and requiring each sentinel to be both emitted and asserted. Full suite 319 suites / 5068 tests passing. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> --------- Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
Summary
Cloud Hypervisor v53 and virtiofsd v1.10 have no per-path readonly option, and a guest-side
romount is not a security boundary — a compromised guest can remount. This PR adds the host-side primitive that makes selective write access enforceable: a private, recursively read-only staged host mount tree with nested writable bind overlays, enforced by the host VFS.This is an inert foundation. With no enforcement argument,
VirtiofsdManagerbehaves exactly as before — including byte-identical virtiofsd arguments. Nothing calls the new API yet; runtime wiring lands separately.Independent of and not stacked on the policy planner PR (#7660). It does not touch
src/filesystem-policy.tsorsrc/cloud-hypervisor/exports.ts, and implements nofilesystem.allowWriteparsing or planning.API
VirtiofsdManager.start(exports, enforcement?)accepts an optionalVirtiofsdMountEnforcement, which maps an export to aVirtiofsdExportMountPlandescribing canonical writable overlay source/destination pairs. The shape is generic — it describes host paths, not policy — so the planner can target it without this PR knowing about policy at all.Exports without a plan keep the existing path, so partial enforcement is supported. A plan naming an export tag that does not exist is rejected at
start(): silently dropping it would leave that export unrestricted read-write, so a renamed or mistyped tag has to fail rather than downgrade.Staging sequence
mount --rbind <export source> <staged root>mount --make-rprivate <staged root>— propagation cannot leak back to the hostro,nosuid,nodev, deepest-first, before any overlay existsmount --bind(non-recursive, so a writable directory never exposes nested submounts) then an explicitmount -o remount,bind,rw,nosuid,nodev/proc/self/mountinfois parsed and the tree is verified; only then does virtiofsd start, with--announce-submountsOverlays support both directories and single files, and targets must already exist.
ro=recursiveis deliberately not usedlibmount's recursive option argument looked like the obvious mechanism. It is not safe here. Verified on a live kernel (util-linux 2.39.3, the version on GitHub-hosted Ubuntu 24.04, kernel 6.12):
Both succeed silently while leaving carried-in submounts writable — exactly the kind of failure that looks fine in CI and isn't. The per-mount remount loop was verified to work correctly on the same host. The mountinfo verifier is what caught this, which is the argument for keeping verification independent of tool exit codes.
Preflight now only requires util-linux >= 2.23 (for
--make-rprivate) and rejects a non-util-linuxmountwith a clear error.Security properties
MountTreeToolsabstraction; nothing is string-concatenated.make-rprivateplus a mountinfo assertion that no staged mount isshared:.realpathequality), contained in the export source, non-symlink, and of the declared kind. Destinations must exist, may not overlap, and are capped.realpathequality plus containment under the staged root.lstatalone is insufficient — it only reveals a symlink in the final component, while the kernel resolves every intermediate component when it binds. Atools -> /etcsymlink carried in from the export would otherwise let destinationtools/sudoerslstat as an ordinary file and then bind over the host's/etc/sudoers. The staged root is itself required to be canonical so that comparison is meaningful.Residual limitation (documented): overlay sources live in the still-writable export, so a setup-time TOCTOU window exists between validation and bind. Destinations have no window — they are canonicalized inside the already-read-only private tree. Mitigated by re-canonicalising immediately before each bind.
Guest semantics
Staged exports stay
rwin the guest on purpose. The host tree is the boundary; making the guest mount read-only would defeat the writable overlays and provides no security value anyway.--announce-submountsis added only when a tree is staged, so default output is unchanged.Cleanup
Overlay children unmount deepest-first, then the root with
umount -R(a plainumountof an rbind root carrying submounts failsEBUSY— found by the tests). Partial-start failures roll back what was staged and preserve the original error; anything that survives teardown is retained and retried on the next stop rather than being silently dropped.Validation
src/cloud-hypervisor, including 46 mount-tree tests and 8 manager-level tests: no-enforcement unchanged behaviour, partial enforcement leaving unplanned exports on the legacy path, unknown plan tag failing closed, read-only staged export, selective directory and file overlays, exact command ordering, explicit--announce-submounts, invalid/outside/symlink/overlapping paths, intermediate and final destination symlink escapes (asserting no--bindis ever invoked), non-canonical staged root, self-nesting rejection, partial setup failure, daemon startup failure, and reverse cleanup.tsc --noEmitclean; eslint clean (3 pre-existing execa warnings); markdownlint clean.ubuntu:24.04, 18/18 checks: staging + verifier pass; root, sibling, and file-overlay-parent writes deniedEROFS; carried-in submount recursively read-only; directory and single-file overlay writes persist to the export source; no shared propagation;nosuid/nodevapplied recursively; both symlink escapes rejected with the host target untouched and no mount created outside the tree; cleanup leaves zero mounts and removes the staging directory; export source untouched. This ran against the exact commands the implementation emits, and is what drove thero=recursiverewrite. No permanent test tooling was added — the harness was disposable, matching existing integration patterns.