fix(husk): template artifacts stay restorable by a non-root husk (mandatory CoW + group-read contract)#612
fix(husk): template artifacts stay restorable by a non-root husk (mandatory CoW + group-read contract)#612stubbi wants to merge 7 commits into
Conversation
…datory CoW + group-read contract)
The jailed template build leaves /var/lib/mitos/templates/<id>/{rootfs.ext4,
snapshot/mem,snapshot/vmstate} owned by the jailer build uid, and #587's
normalize chowned them to the builder euid at 0o644, which only worked because
forkd and the husk both run uid 0 today. A husk that is neither that uid nor
privileged fails EACCES and the warm pool crashloops while the pool still
reports Ready (#597).
Close two failure modes:
1. Mandatory per-activation rootfs CoW. internal/husk Stub.Activate now fails
closed when a shared template rootfs is configured but no per-activation
reflink clone was prepared, rather than opening the shared template in place
O_RDWR. Firecracker only ever binds to the husk-owned clone.
2. Group-read contract that survives a non-root husk. normalizeTemplateArtifacts
(internal/firecracker) and the read-side reuse gate
checkTemplateArtifactInvariants (internal/fork) now target root:SharedKVMGID
with group-readable, not world-writable modes (files 0o640, dirs 0o750).
Named constants JailerBuildUID and SharedKVMGID replace the bare 64000. The
husk pod carries SharedKVMGID as a supplemental group so a future non-root
husk (#585) reads the shared template through the group class; the current
uid-0 husk reads as owner and is unaffected.
Pool honesty (#578/#579): the existing crashloop-based driveTemplateHealth
already reflects a Prepare-time failure; surfacing per-activation failures on the
SandboxPool status is deferred to #578 with a TODO.
Threat model updated with the new file permission posture and the honest O_RDWR
-at-load caveat for the future non-root husk.
Tests: a root-gated in-process proof drops privileges to a non-root uid in the
shared kvm group and reads the normalized artifacts without EACCES (and confirms
a non-member is denied); root-gated normalize/invariant tests assert the new
gid+mode class; a husk unit test proves Activate fails closed without a clone and
never loads the shared template.
Closes #597
Related #585, #587, #578, #579
Security-sensitive paths (internal/fork, internal/firecracker, internal/husk):
needs a named human reviewer before merge.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Signed-off-by: Jannes Stubbemann <jannes@openclaw.rocks>
|
Orchestrator review before handing to @stubbi. Local verification: builds clean, both lints clean, dash scan clean; internal/husk + internal/firecracker + internal/fork tests pass (root-gated ownership tests skip non-root as designed). Holding for human review + a KVM run per the security-sensitive rule. One unresolved question the reviewer should settle on KVM, because it bears on whether this actually stops the prod recurrence: I checked prod (v1.13.1). forkd DOES have CAP_CHOWN + DAC_OVERRIDE + FOWNER and there are zero normalize/chown errors in its logs, yet a FRESH template build on v1.13.0 forkd still produced 64000:64000 artifacts (I had to chown 0:0 by hand twice today). Since #587's normalizeTemplateArtifacts chowns to the builder euid (0) and forkd can chown, normalize is either not the last writer or is being undone. The reuse gate rebuilds on a failed invariant, and snapshot/ is under workDir so it is covered, so those are not the gap. Most likely mechanism: the jailer hardlinks the artifacts between the chroot and the canonical templates/ tree (shared inode), and chownIntoJail flips the shared inode to 64000. If any post-normalize jailer op (a later fork or warm activation reusing the chroot) re-chowns that shared inode, the canonical path reads 64000 again AFTER normalize ran. #612 changes normalize's TARGET (root:SharedKVMGID, 0640) but does not change this ordering, so if the re-chown is the cause, the recurrence persists. The robust fix that survives a re-chown: make the husk's baked template rootfs drive is_read_only=true so /snapshot/load never opens the shared rootfs.ext4 O_RDWR (the PR's own Risks note flags that it currently does, is_read_only=false, and only the uid-0-as-owner case saves it). With a read-only baked drive + mandatory CoW clone + group/other READ, restore succeeds regardless of whether the file is owned 0 or 64000, as long as it stays readable. That closes both the current uid-0 case and the future non-root #585 case, and is immune to the shared-inode re-chown. Recommend: on KVM, (1) confirm whether template ownership survives a fork/second-activation after build (reproduce the 64000-after-normalize), and (2) consider flipping the baked husk template drive to read-only in this PR or an immediate follow-up. The mandatory-CoW + group-read changes here are correct and worth landing regardless; I just do not want us to believe #597 is closed until the re-chatown-or-O_RDWR leg is confirmed dead on real KVM. |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThe PR adds a shared KVM group contract for jailed template artifacts, updates normalization and reuse checks to enforce it, wires husk pods into the group, and makes activation fail closed when a per-activation CoW clone is missing. ChangesTemplate artifact permission model
Estimated code review effort: 4 (Complex) | ~60 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
internal/husk/stub.go (1)
700-815: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winMove the rootfs-CoW guard before network setup.
This check runs afterapplyEgressFilter, so a failedActivatecan leave the tap, nftables rules, and optional DNS proxy alive until pod shutdown while the stub stays dormant. Hoist the guard above the egress-filter block, and add a test that setsreq.Networkto cover this path.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/husk/stub.go` around lines 700 - 815, Move the rootfs CoW validation in Activate ahead of the network setup block so a missing clone fails before applyEgressFilter, tap creation, nftables changes, or DNS proxy startup can occur. Locate the guard in the Activate flow in husk/stub.go and hoist it above the section that builds NetfilterConfig and calls applyEgressFilter, keeping the same fail-closed error behavior. Add or update a test for Activate that sets req.Network and verifies the rootfs-CoW failure returns before any network side effects are started.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@internal/firecracker/jailer.go`:
- Around line 40-49: SharedKVMGID can collide with a custom per-VM jailer gid
because parseUIDRange currently allows any non-root range, so update the
UID-range validation in parseUIDRange (and any caller that accepts --uid-range)
to reject ranges that include SharedKVMGID 65000, or otherwise constrain the
accepted range to avoid that value. Keep the SharedKVMGID constant and related
jailer setup in internal/firecracker/jailer.go aligned with the validation so
the supplemental-group/read-path for the husk remains unique.
In `@internal/firecracker/template_owner_test.go`:
- Around line 158-166: The ancestor-permission widening loop in the test
currently mutates directory modes via os.Chmod and leaves them changed, so
update the logic around the filepath.Dir traversal to capture each directory’s
original permissions before widening and register a t.Cleanup to restore them
afterward. Keep the fix localized to the test helper in template_owner_test.go
so the root-only setup still works without permanently altering ancestor
directories.
- Around line 4-8: The child-process failure handling in template_owner_test.go
is converting the error to a string, which drops the original cause. Update the
error path in the test helper/assertion logic (the code that handles runErr) to
wrap the returned exec error with fmt.Errorf using %w instead of calling
runErr.Error(), so callers retain the underlying error details. Apply the same
change in the related duplicate logic referenced by the review, keeping the
existing context message but preserving the original error value.
In `@internal/firecracker/template.go`:
- Around line 582-590: The ownership normalization in WalkDir should fail closed
on symlinks before calling os.Chown or os.Chmod, since both follow links and can
affect targets outside the template tree. Update the WalkDir callback in
internal/firecracker/template.go to detect symlink entries via os.DirEntry
before any privileged mutation, and return an error (or otherwise stop
processing) when a symlink is encountered. Keep the existing normalization flow
in place for regular files and directories only.
In `@internal/fork/template_invariants.go`:
- Around line 30-39: The reuse invariant in checkTemplateArtifactInvariants only
validates snapshot files, but it must also enforce the directory ownership and
permissions contract for the template and snapshot directories. Extend the
read-side checks in checkTemplateArtifactInvariants to include template
directories (including snapshot) and verify they remain owned by the effective
uid, group-owned by firecracker.SharedKVMGID, and have group-executable mode
0o750 so reuse fails early instead of later with EACCES.
---
Outside diff comments:
In `@internal/husk/stub.go`:
- Around line 700-815: Move the rootfs CoW validation in Activate ahead of the
network setup block so a missing clone fails before applyEgressFilter, tap
creation, nftables changes, or DNS proxy startup can occur. Locate the guard in
the Activate flow in husk/stub.go and hoist it above the section that builds
NetfilterConfig and calls applyEgressFilter, keeping the same fail-closed error
behavior. Add or update a test for Activate that sets req.Network and verifies
the rootfs-CoW failure returns before any network side effects are started.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 9f194d62-7cea-495a-8607-c0fc972b639c
📒 Files selected for processing (12)
docs/threat-model.mdinternal/controller/huskpod.gointernal/controller/huskpod_test.gointernal/controller/pool_rebuild.gointernal/firecracker/jailer.gointernal/firecracker/template.gointernal/firecracker/template_owner_test.gointernal/fork/template_invariants.gointernal/fork/template_reuse_engine_test.gointernal/fork/template_reuse_test.gointernal/husk/stub.gointernal/husk/stub_test.go
|
Merge-sweep note: this PR has a deterministic firecracker-test failure, reproduced across two consecutive runs after a branch update to current main (runs 28642727338 first attempt and its rerun). Signature, repeating several times per run in the "SDK direct-mode example executed end to end against a real KVM sandbox-server" step: The guest agent fails init mounting /dev, then the step exits 1. The box itself is healthy (other PRs' firecracker-test runs pass on the same runner before and after, including #611 which just merged), so this looks specific to this branch's change interacting with guest boot, possibly the ownership normalization changing what the jailer pre-mounts or a devtmpfs double-mount. Leaving this PR out of the merge sweep; auto-merge stays armed so it lands once the failure is fixed. Happy to help debug if useful. |
|
Heads-up before this auto-merges: firecracker-test is failing deterministically (not the usual external flake), so the merge is correctly blocked. Root cause, from the direct-mode sandbox-server step: The new ownership-normalization Suggested direction (for you / the implementer, not pushed): make the normalization best-effort in a non-privileged context. The husk-readability contract only matters on the jailed husk restore path; direct mode has no husk. Options: skip normalization when there is no jailer/husk, or attempt the chown and fall back to the owner-safe |
|
Held for maintainer review (security-sensitive: internal/firecracker + husk restore). Auto-merge is armed but correctly blocked: Why the check is redThe direct-mode leg (
uid := os.Geteuid()
os.Chown(path, uid, SharedKVMGID)Setting the owner to your own euid is a no-op, but changing the GROUP to Fix direction (maintainer call, security path)Make the group normalization best-effort against EPERM rather than fatal, so the non-privileged create path still produces a usable template:
Either keeps the husk group-read contract intact while not hard-failing the unprivileged direct-mode path that |
|
Correction to my earlier comment: the mount /dev EBUSY signature has now also appeared on an unrelated billing-only branch (PR #666) while main's push runs with identical engine content pass, so it is a flaky guest-init race, not something deterministic to this branch; filed as #668 with the cross-branch evidence and a fix shape (tolerate EBUSY as already-mounted in guest/agent-rs init). Your three consecutive hits may still mean this change raises the race probability, worth a look while fixing, but the earlier deterministic framing was too strong. Rerunning checks here is reasonable once #668 is understood. |
|
Found the branch-specific failure, distinct from the flakes. The rerun (job 84973802316) shows two independent failures:
So: fix the unprivileged chown path here, rebase, and the branch should go green modulo the #535 flake. |
… follow symlinks Two fixes to normalizeTemplateArtifacts, surfaced by the tmpl-smoke KVM job (which runs the standalone build path as the non-root runner user) and by CodeRabbit review. 1. Non-root builder support. The normalize step chgrped every artifact to the shared kvm gid (65000) so a future non-root husk (#585) could read the template through the group class. Only a root builder can chgrp to a group it does not belong to, so the standalone sandbox-server and any non-root self-host aborted with EPERM: 'operation not permitted'. A non-root builder both builds AND restores the template as the same uid, so it needs no shared-group contract. Attempt the shared gid and fall back to the builder's own gid on the first permission error. The reuse invariant (internal/fork.checkTemplateArtifactInvariants) now accepts either the shared kvm gid or the process's own gid, keeping the write and read sides in lockstep so a non-root deployment reuses its templates instead of rebuilding every one. The root path is unchanged: euid 0 still tags root:SharedKVMGID at 0o640/0o750. 2. Symlink safety. os.Chown and os.Chmod dereference symlinks, so a link planted in the build output could redirect a privileged ownership or mode change onto a target outside the template tree. The walk now skips symlinks and uses Lchown, so a link inode is never followed. Tests: two new cases run in the ordinary non-root environment (the fallback succeeds and tags the builder's own uid:gid; a symlink out of the tree is left untouched), plus the CodeRabbit test-hygiene nits (wrap the child-process error, restore widened ancestor perms via t.Cleanup). Updated TestCheckTemplateArtifactInvariantsRejectsUnnormalized to assert the mode citation, since own-gid is now accepted. Signed-off-by: Jannes Stubbemann <jannes@openclaw.rocks>
…plate dirs Two more CodeRabbit review findings on the husk-readable template work: - parseUIDRange now rejects a --uid-range that covers the shared kvm gid (65000). A per-VM jailer uid/gid drawn from an overlapping range would collide with the group that owns the group-readable template artifacts, so a jailed build VM could carry the template-read group. The shipped default 64000-64999 is unaffected. - checkTemplateArtifactInvariants now also checks the template and snapshot directories, not just the files. normalizeTemplateArtifacts sets the dirs to 0o750, but a husk cannot traverse to the files (EACCES) if a dir loses group execute even when the files are compliant; the read-side gate must catch that. The uid/gid/mode check is factored into one helper shared by the dir (0o750) and file (0o640) checks, and the root-gated compliant test helper normalizes the dirs too. Signed-off-by: Jannes Stubbemann <jannes@openclaw.rocks>
Root cause of the firecracker-test failure + review passThe tmpl-smoke KVM step failed with Fixes pushed
Verification
This touches security-sensitive paths ( |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@internal/fork/template_invariants.go`:
- Line 49: The reuse gate is following symlinks when validating template
artifacts, allowing a symlinked rootfs.ext4, mem, or vmstate to pass as if it
were a regular file. Update normalizeTemplateArtifacts and the validation path
around os.Stat to use os.Lstat or explicitly reject ModeSymlink so only real
files are accepted and symlinked inodes fail fast.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: dc0ef7ff-bbca-44dc-935d-e626d659be5d
📒 Files selected for processing (4)
cmd/forkd/jailer.gocmd/forkd/jailer_test.gointernal/fork/template_invariants.gointernal/fork/template_reuse_test.go
| // "artifact" or "dir" for the message; wantMode is 0o640 for files and | ||
| // 0o750 for directories. | ||
| checkOwnership := func(kind, path string, wantMode os.FileMode) error { | ||
| info, err := os.Stat(path) |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Look for any symlink creation in build output / template artifact paths.
rg -nP '(os\.Symlink|Symlink\()' internal/firecracker internal/fork
# Confirm what templateSnapshotFiles resolves to.
ast-grep run --pattern 'func templateSnapshotFiles($$$) $_ { $$$ }' --lang go internal/forkRepository: mitos-run/mitos
Length of output: 1259
🏁 Script executed:
#!/bin/bash
sed -n '1,220p' internal/fork/template_invariants.go
printf '\n---\n'
sed -n '1,140p' internal/fork/verify.go
printf '\n---\n'
sed -n '1640,1705p' internal/fork/engine.goRepository: mitos-run/mitos
Length of output: 15076
🏁 Script executed:
#!/bin/bash
rg -n "normalizeTemplateArtifacts|checkTemplateArtifactInvariants|templateSnapshotFiles|writeVerifiedMarker|writeDigestFile|recordTemplateDigest|verifyTemplate|shouldReuseTemplate" internal/fork
printf '\n---\n'
sed -n '1,220p' internal/fork/template_owner.go
printf '\n---\n'
sed -n '1,220p' internal/fork/verify.goRepository: mitos-run/mitos
Length of output: 18833
🏁 Script executed:
#!/bin/bash
sed -n '2180,2275p' internal/fork/engine.go
printf '\n---\n'
sed -n '2388,2455p' internal/fork/engine.go
printf '\n---\n'
sed -n '1,240p' internal/fork/template_reuse_test.goRepository: mitos-run/mitos
Length of output: 17123
🏁 Script executed:
#!/bin/bash
rg -n "normalizeTemplateArtifacts|Lchown|Lstat|Symlink|templateSnapshotFiles|checkTemplateArtifactInvariants" internal/fork
printf '\n---\n'
sed -n '1,220p' internal/fork/* | sed -n '1,220p'Repository: mitos-run/mitos
Length of output: 11611
🏁 Script executed:
#!/bin/bash
rg -n "func normalizeTemplateArtifacts|normalizeTemplateArtifacts\(" internal/fork
printf '\n---\n'
fd -a "template*go" internal/forkRepository: mitos-run/mitos
Length of output: 158
🏁 Script executed:
#!/bin/bash
rg -n "normalizeTemplateArtifacts|Lchown|Lstat|os\.Symlink|Symlink\(" internal/fork internal/firecracker
printf '\n---\n'
sed -n '1,220p' internal/fork/engine.go | sed -n '2280,2365p'Repository: mitos-run/mitos
Length of output: 3194
🏁 Script executed:
#!/bin/bash
rg -n "normalizeTemplateArtifacts|Lchown|Lstat|os\.Symlink|Symlink\(" internal/fork internal/firecracker
printf '\n---\n'
sed -n '2280,2365p' internal/fork/engine.goRepository: mitos-run/mitos
Length of output: 7310
🏁 Script executed:
#!/bin/bash
sed -n '554,630p' internal/firecracker/template.go
printf '\n---\n'
sed -n '250,290p' internal/firecracker/template_owner_test.go
printf '\n---\n'
sed -n '1658,1692p' internal/fork/engine.goRepository: mitos-run/mitos
Length of output: 7092
Reject symlinks in the reuse gate. normalizeTemplateArtifacts already skips symlinks, but os.Stat here follows them, so a planted rootfs.ext4/mem/vmstate symlink is validated against its target. Use os.Lstat (or reject ModeSymlink) so regular templates fail fast instead of passing via a symlinked inode.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@internal/fork/template_invariants.go` at line 49, The reuse gate is following
symlinks when validating template artifacts, allowing a symlinked rootfs.ext4,
mem, or vmstate to pass as if it were a regular file. Update
normalizeTemplateArtifacts and the validation path around os.Stat to use
os.Lstat or explicitly reject ModeSymlink so only real files are accepted and
symlinked inodes fail fast.
|
Re-assessed against current main (v1.38 era). Still relevant, not superseded: issue #597 is open, nothing on main touched template_invariants.go or the jailer legs since, and a merge against main is textually conflict-free. Caveat: the branch predates the live-cow lazy-UFFD restore work, so a fresh firecracker-test and cluster-husk-e2e run after update-branch is mandatory before this merges. Two small code fixes still owed from the CodeRabbit round: (1) the read-side reuse gate checkTemplateArtifactInvariants still uses os.Stat (follows symlinks) while the write side got the symlink-safe Lchown treatment; make it Lstat plus a ModeSymlink reject for symmetry. (2) the fail-closed CoW guard in Stub.Activate sits after applyEgressFilter, so a fail-closed activation leaves tap/nftables/DNS-proxy alive until pod shutdown; hoist the guard above the network block. One open question to settle before treating this as "Closes #597": if the prod recurrence mechanism is a post-normalize chownIntoJail re-flipping hardlinked inodes, this PR changes normalize's target but not that ordering, so the recurrence could survive. The mandatory-CoW and group-read contract are worth landing regardless; just keep the close provisional pending a KVM repro. Held for the named human security review per the standing instruction on this PR. |
1 participant
Thinking Path
Linked Issues or Issue Description
Closes #597
Related #585, #587, #578
What Changed
internal/husk/stub.go):Stub.Activatenow fails closed when a shared template rootfs is configured but no
per-activation reflink clone was prepared, instead of silently loading the
shared template in place O_RDWR. Firecracker only ever binds to the husk-owned
clone; the shared
rootfs.ext4is never opened in place.internal/firecracker/template.gonormalizeTemplateArtifactsnow hands theartifacts back to
rootand the shared kvm group (SharedKVMGID), files0o640, dirs0o750(was world-readable0o644/0o755).internal/fork/template_invariants.gocheckTemplateArtifactInvariants(theread-side reuse gate) now verifies
euid:SharedKVMGIDand mode0o640.firecracker.JailerBuildUID(64000) andfirecracker.SharedKVMGID(65000, chosen OUTSIDE the per-VM jailer range soit can never collide with a per-VM jailer gid) replace the bare literal.
internal/controller/huskpod.go: the husk pod carriesSharedKVMGIDas aSupplementalGroupsentry so a future non-root husk (Harden husk pod: run the VMM as a non-root uid (kvm group) + evaluate userns remap #585) can read thegroup-owned template artifacts; a no-op for the current uid-0 husk (file owner).
docs/threat-model.md: documented the new file permission posture (group-ownedto a named kvm gid, group-readable, not world-writable, still a read-only
hostPath mount for the husk) and the honest O_RDWR-at-load caveat for Harden husk pod: run the VMM as a non-root uid (kvm group) + evaluate userns remap #585.
internal/controller/pool_rebuild.godriveTemplateHealth. The existing crashloop-basedtemplateRestoreFailingalready reflects a Prepare-time failure on the pool; surfacing per-activation
failures on
SandboxPoolstatus is Surface template-build failures (e.g. ENOSPC) on SandboxPool status, not just controller logs #578's scope.Verification
go build ./...go test ./internal/firecracker/... ./internal/fork/... ./internal/husk/...(non-root; root-gated ownership tests skip)
go test ./internal/controller/...with envtestCI job is non-root):
TestNormalizeTemplateArtifactsRestorableByNonRootGroupMemberDROPSprivileges to a non-root uid whose primary + supplemental group is
SharedKVMGIDand reads every normalized artifact without EACCES, andconfirms a non-member uid is DENIED (proving the group is load-bearing, not
world-read). This is the in-process (dropped-privilege subprocess) non-root
restore proof.
TestNormalizeTemplateArtifactsSetsGroupReadableContract,TestNormalizeTemplateArtifactsRestoresJailFlippedOwner, and theinternal/forkinvariant + reuse tests assert the newroot:SharedKVMGID,0o640class.internal/huskTestActivateFailsClosedWhenTemplateWithoutClone: Activaterefuses without a clone and never loads the shared template
(
loadCalls == 0);TestActivateRebindsRootfsDriveToClonestill proves theCoW path binds Firecracker to the per-vm clone, never the shared template.
golangci-lint run --timeout=5m <pkgs>andGOOS=linux golangci-lint run --timeout=5m <pkgs>.Risks
is_read_only=false, so/snapshot/loadopens theshared template O_RDWR BEFORE the PatchDrive rebind. The current uid-0 husk is
the file OWNER (owner rw on
0o640), so the load succeeds; this PR keeps prodcorrect. A FUTURE non-root husk (Harden husk pod: run the VMM as a non-root uid (kvm group) + evaluate userns remap #585) reads through the GROUP class (read
only), so the O_RDWR load leg still needs resolving there (group-write on the
template, or a read-only baked rootfs drive). Called out in the threat model
and the deferred-work note; not regressed by this PR.
SharedKVMGID = 65000is a fixed gid the forkd chown and the husk supplementalgroup agree on; it is outside the jailer
[64000, 64999]range to avoid anyper-VM jailer gid collision.
Security
This touches security-sensitive paths (
internal/fork,internal/firecracker,internal/husk, plus the husk pod securityContext ininternal/controller). Perrepo policy it needs a named human review before merge. Do NOT auto-merge. No
auto-merge is enabled.
Model Used
Claude Fable 5 (claude-fable-5), via Claude Code.
Checklist
husk fail-closed unit test, invariant + normalize gid/mode tests, husk pod
supplemental-group test)
Summary by CodeRabbit