diff --git a/CHANGELOG.md b/CHANGELOG.md index d8e2d5b..b2088fc 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,3 +1,40 @@ +## v0.110.0 — F-LEAK, third attempt: the fourth root-fenced exception (2026-07-28) + +**The band-scoped ACL fix (v1.21.0) is durable for exactly ONE use per slot, and the live check caught +it.** After the first successful teardown of `990000`, `pveum acl list` showed **0 grants** at +`/vms/990000`. Cause, confirmed in PVE's own source rather than inferred: the destroy path calls +`PVE::AccessControl::remove_vm_access($vmid)` (`API2/LXC.pm:906`), which deletes +`acl_root->children->vms->children->{$vmid}` — **every ACL at `/vms/`** (`AccessControl.pm:1898`). + +**The grant is consumed by the very operation it authorises.** After ten teardowns the whole band is +ungranted and F-LEAK returns. A per-VM ACL is structurally the wrong tool here; the pool grant is +durable only because it lives on a *pool* path, which `remove_vm_access` does not touch (it removes the +guest's pool *membership*, not the pool's ACL). + +**So this is attempt three, and it is the route the task offered as the alternative:** route the destroy +through the same authority that already performs the create. `Privileged.DestroyScratchLXC` is the +**fourth** root-fenced exception (previously exactly three: keyctl `pct create`, USB mount/fstab, +SMART/sensors), and it is fenced harder than any of them: + +| layer | enforcement | +|---|---| +| **sudoers** | `/usr/sbin/pct destroy 99000[0-9] --purge` — sudo matches the vmid **literally**. Even a compromised agent asking for `pct destroy 9201` is refused by sudo itself. | +| **`DestroyScratchLXC`** | re-checks the band before exec; refuses an unconfigured or inverted band rather than defaulting | +| **`teardownScratch`** | acts only on this journal entry's own scratch provenance | + +None of the three is consumed by use, which is the property the ACL lacked. The API destroy is still +tried **first** and remains the normal path; this is the fallback that makes teardown deterministic +instead of once-per-slot. The v1.21.0 band ACLs stay provisioned — they make the common case need no +privileged call at all. + +**Ships with a sudoers change**, so `configs/felhom-agent.sudoers` must be deployed with the binary +(`visudo -cf`: parsed OK). + +Red-proofs 9 and 10, both observed failing: removing the band check → +`REFUSAL FAILED: executed [pct destroy 9201 --purge] for out-of-band vmid 9201`; removing the +unconfigured-band check → `an unconfigured band admitted vmid 0 and EXECUTED [pct destroy 0 --purge]`. +The out-of-band table deliberately includes **9201**, the live customer guest on both demo boxes. + ## v0.109.0 — the guest-power watchdog gets the observable it was shipped without (2026-07-28) **Self-correction to v0.107.0, found by running the very check this session added elsewhere.** The new diff --git a/configs/felhom-agent.sudoers b/configs/felhom-agent.sudoers index 29dcf11..65b6e1b 100644 --- a/configs/felhom-agent.sudoers +++ b/configs/felhom-agent.sudoers @@ -133,6 +133,28 @@ Cmnd_Alias FELHOM_CONTROLLERSWAP = \ Cmnd_Alias FELHOM_STALELOCK = \ /usr/sbin/pct unlock [0-9]* +# Restore-test scratch teardown (F-LEAK, Campaign 8, v0.110.0). A restore-test whose restore FAILS +# leaves a scratch guest the API token CANNOT destroy: `FelhomAgentGuest` is granted at /pool/felhom and +# a guest joins that pool only when its restore COMPLETES, so a failed restore leaves a pool-less guest +# out of reach (403 VM.Allocate) holding its disks until a human removes it. +# +# TWO API-SIDE FIXES WERE TRIED AND BOTH REFUTED LIVE on 2026-07-28, which is why this grant exists: +# 1. Adopt the stranded guest into the pool, then retry. `PUT /pools/{pool}` ALSO requires +# VM.Allocate on the VM being added — pool membership cannot bootstrap its own authority. +# 2. Grant FelhomAgentGuest per-path at /vms/990000..990009. Durable for exactly one use per slot: +# PVE's own destroy path calls `AccessControl::remove_vm_access($vmid)` (LXC.pm:906), which DELETES +# every ACL at /vms/ (AccessControl.pm:1898). The grant is consumed by the operation it +# authorises, so after ten teardowns the band is ungranted and the defect returns. +# +# WHY THIS IS THE TIGHTEST AVAILABLE FENCE, not a widening: sudo matches the vmid LITERALLY, so +# `99000[0-9]` is exactly the ten-slot scratch band the restore-test picks from — nothing else. There is +# no `[0-9]*` coarse allowlist here on purpose: unlike `pct unlock`, this op DESTROYS, so the band must +# be in the policy and not merely validated in the agent. Even a compromised agent asking for +# `pct destroy 9201` is refused by sudo itself. Unlike an ACL, a sudoers rule is not consumed by use. +# The agent re-checks the band in code before exec (defence in depth); this is the outer fence. +Cmnd_Alias FELHOM_SCRATCH_TEARDOWN = \ + /usr/sbin/pct destroy 99000[0-9] --purge + # Network storage / NAS (Part A1, SPIKE-nas-storage-2026-06-29). The agent mounts a customer NAS share # HOST-SIDE under /mnt/felhom-drives/ via a systemd .automount (+ .mount) pair so it propagates # into the guest through the existing shared bind (an unprivileged LXC cannot mount NFS/CIFS itself). @@ -274,4 +296,4 @@ Cmnd_Alias FELHOM_GUESTNET = \ /usr/sbin/pct exec [0-9]* -- pgrep -x dhclient, \ /usr/sbin/pct exec [0-9]* -- dhclient -pf /run/dhclient.eth0.pid -lf /var/lib/dhcp/dhclient.eth0.leases eth0 -felhom-agent ALL=(root) NOPASSWD: FELHOM_MOUNT, FELHOM_DISK, FELHOM_PROVISION, FELHOM_FORMAT, FELHOM_DNSMASQ, FELHOM_GUESTHOOK, FELHOM_INTERMEDIARY, FELHOM_CONTROLLERSWAP, FELHOM_STALELOCK, FELHOM_NETMOUNT, FELHOM_WG, FELHOM_SELFUPDATE, FELHOM_SSHD, FELHOM_OOB, FELHOM_PBSDR, FELHOM_SELFHEAL, FELHOM_ESCROW, FELHOM_GUESTNET +felhom-agent ALL=(root) NOPASSWD: FELHOM_MOUNT, FELHOM_DISK, FELHOM_PROVISION, FELHOM_FORMAT, FELHOM_DNSMASQ, FELHOM_GUESTHOOK, FELHOM_INTERMEDIARY, FELHOM_CONTROLLERSWAP, FELHOM_STALELOCK, FELHOM_NETMOUNT, FELHOM_WG, FELHOM_SELFUPDATE, FELHOM_SSHD, FELHOM_OOB, FELHOM_PBSDR, FELHOM_SELFHEAL, FELHOM_ESCROW, FELHOM_GUESTNET, FELHOM_SCRATCH_TEARDOWN diff --git a/internal/proxmox/privileged.go b/internal/proxmox/privileged.go index 7992069..4cb0997 100644 --- a/internal/proxmox/privileged.go +++ b/internal/proxmox/privileged.go @@ -136,6 +136,39 @@ func (p *Privileged) CreateGoldenLXC(ctx context.Context, spec GoldenLXCSpec) er return p.run(ctx, "pct", args...) } +// DestroyScratchLXC destroys a restore-test scratch guest through the fenced root path, refusing any +// vmid outside the caller-supplied scratch band. +// +// WHY THIS CANNOT BE THE API — and this is the fourth fenced exception, so the reasoning is recorded +// in full. A restore-test whose restore FAILS leaves a scratch guest the API token cannot destroy: +// `FelhomAgentGuest` is granted at /pool/felhom, and a guest joins that pool only when its restore +// COMPLETES. A failed restore therefore leaves a guest that exists, is in no pool, and is out of +// reach (403 VM.Allocate) while holding its disks. +// +// TWO API-SIDE FIXES WERE BUILT AND BOTH REFUTED BY LIVE TEST on 2026-07-28: +// - Adopt the stranded guest into the pool, then retry. `PUT /pools/{pool}` ALSO requires +// VM.Allocate on the VM being added, so pool membership cannot bootstrap its own authority. +// - Grant the role per-path at /vms/990000..990009. Durable for exactly ONE use per slot: PVE's own +// destroy calls AccessControl::remove_vm_access (LXC.pm:906), deleting every ACL at /vms/ +// (AccessControl.pm:1898). The grant is consumed by the operation it authorises. +// +// The band ACLs are still provisioned (host-install v1.21.0) and the API path is still tried FIRST — +// this is the fallback that makes teardown deterministic rather than once-per-slot. +// +// THE FENCE. The band is enforced in THREE places, deliberately: sudoers matches the vmid literally +// (`pct destroy 99000[0-9] --purge` — even a compromised agent asking for 9201 is refused by sudo +// itself), this method re-checks it before exec, and the caller checks its own journal provenance. +// Unlike an ACL, none of these is consumed by use. +func (p *Privileged) DestroyScratchLXC(ctx context.Context, vmid, bandMin, bandMax int) error { + if bandMin <= 0 || bandMax < bandMin { + return fmt.Errorf("proxmox: DestroyScratchLXC needs a configured scratch band, got [%d,%d]", bandMin, bandMax) + } + if vmid < bandMin || vmid > bandMax { + return fmt.Errorf("proxmox: refusing to destroy vmid %d — outside the scratch band [%d,%d]", vmid, bandMin, bandMax) + } + return p.run(ctx, "pct", "destroy", strconv.Itoa(vmid), "--purge") +} + // MountUSBByUUID mounts a filesystem by UUID at target (creating the mountpoint). // // WHY THIS CANNOT BE THE API: a physical host mount is not a Proxmox API op; it is diff --git a/internal/proxmox/scratch_destroy_test.go b/internal/proxmox/scratch_destroy_test.go new file mode 100644 index 0000000..9621123 --- /dev/null +++ b/internal/proxmox/scratch_destroy_test.go @@ -0,0 +1,101 @@ +package proxmox + +import ( + "context" + "io" + "strings" + "testing" +) + +// F-LEAK (Campaign 8): the fourth root-fenced exception. Its whole justification is that the band is +// enforced rather than assumed, so these tests are about the REFUSALS, not the happy path. +// +// The band is checked in three independent places on purpose: sudoers matches the vmid literally +// (`pct destroy 99000[0-9] --purge`), this method re-checks before exec, and the caller checks journal +// provenance. These tests pin the middle one; the sudoers glob is proven live. + +type recordingRunner struct { + calls [][]string + err error +} + +func (r *recordingRunner) Run(_ context.Context, name string, args ...string) ([]byte, []byte, error) { + r.calls = append(r.calls, append([]string{name}, args...)) + return nil, nil, r.err +} + +func (r *recordingRunner) RunStdin(_ context.Context, _ io.Reader, name string, args ...string) ([]byte, []byte, error) { + r.calls = append(r.calls, append([]string{name}, args...)) + return nil, nil, r.err +} + +// A vmid inside the band is destroyed, with --purge so no config/ACL/firewall residue survives. +// +// RED-PROOF: drop "--purge" from the args → this fails with "destroy is not --purge", and the live +// sudoers rule (which matches the FULL vector including --purge) would refuse the call outright. +func TestDestroyScratchLXC_InBandDestroysWithPurge(t *testing.T) { + r := &recordingRunner{} + if err := NewPrivileged(r, "").DestroyScratchLXC(context.Background(), 990003, 990000, 990009); err != nil { + t.Fatalf("in-band destroy failed: %v", err) + } + if len(r.calls) != 1 { + t.Fatalf("want exactly 1 exec, got %d: %v", len(r.calls), r.calls) + } + got := strings.Join(r.calls[0], " ") + if got != "pct destroy 990003 --purge" { + t.Errorf("exec vector = %q, want %q (it must match the sudoers rule byte for byte)", + got, "pct destroy 990003 --purge") + } +} + +// THE ONE THAT MATTERS. A vmid outside the band must be refused WITHOUT EXECUTING ANYTHING — a real +// customer guest, the golden image, a co-tenant's VM. +// +// RED-PROOF: remove the `vmid < bandMin || vmid > bandMax` check → this fails with +// "REFUSAL FAILED: executed [pct destroy 9201 --purge] for out-of-band vmid 9201". +func TestDestroyScratchLXC_RefusesOutOfBandWithoutExecuting(t *testing.T) { + for _, vmid := range []int{ + 1, // arbitrary + 9201, // the LIVE customer guest on both demo boxes + 9100, // golden image + 9999, // reserved + 989999, // one below the band + 990010, // one ABOVE the band — the off-by-one + } { + r := &recordingRunner{} + err := NewPrivileged(r, "").DestroyScratchLXC(context.Background(), vmid, 990000, 990009) + if err == nil { + t.Errorf("vmid %d was NOT refused — the fence is open", vmid) + } + if len(r.calls) != 0 { + t.Errorf("REFUSAL FAILED: executed %v for out-of-band vmid %d", r.calls, vmid) + } + if err != nil && !strings.Contains(err.Error(), "outside the scratch band") { + t.Errorf("vmid %d refused with an unhelpful error: %v", vmid, err) + } + } +} + +// An unconfigured or inverted band must refuse everything rather than defaulting to something. A zero +// band is what a mis-wired caller looks like, and "destroy vmid 0" must never become reachable. +// +// RED-PROOF: drop the `bandMin <= 0 || bandMax < bandMin` check → the [0,0] case admits vmid 0 and +// this fails with "an unconfigured band admitted vmid 0". +func TestDestroyScratchLXC_RefusesUnconfiguredBand(t *testing.T) { + cases := []struct{ vmid, min, max int }{ + {0, 0, 0}, // nothing configured at all + {990000, 0, 0}, // band absent, real scratch vmid + {990000, 0, 990009}, // min unset + {990005, 990009, 990000}, // inverted + {990000, -1, 990009}, // negative + } + for _, c := range cases { + r := &recordingRunner{} + if err := NewPrivileged(r, "").DestroyScratchLXC(context.Background(), c.vmid, c.min, c.max); err == nil { + t.Errorf("band [%d,%d] admitted vmid %d — an unconfigured band must refuse", c.min, c.max, c.vmid) + } + if len(r.calls) != 0 { + t.Errorf("an unconfigured band admitted vmid %d and EXECUTED %v", c.vmid, r.calls) + } + } +} diff --git a/internal/reconcile/restoretest.go b/internal/reconcile/restoretest.go index b27bd90..2f707f7 100644 --- a/internal/reconcile/restoretest.go +++ b/internal/reconcile/restoretest.go @@ -198,7 +198,7 @@ func (e *Engine) runScratchTest(ctx context.Context, vmid int, spec RestoreTestS launched := false defer func() { if launched { - e.teardownScratch(ctx, base) + e.teardownScratch(ctx, base, spec.ScratchMin, spec.ScratchMax) return } e.append(withState(base, OpFailed)) @@ -446,7 +446,12 @@ func sizeToGB(s string) int { // teardownScratch destroys the scratch guest (benign, gated) and records the entry terminal. // On any teardown failure it leaves the entry in-flight so Recover reaps the guest later. -func (e *Engine) teardownScratch(ctx context.Context, base JournalEntry) { +// +// F-LEAK (Campaign 8): the API destroy is tried FIRST and is the normal path. It fails on a scratch +// left by a FAILED restore, because such a guest never joined /pool/felhom and the token's +// VM.Allocate lives there — so a band-scoped fallback through the fenced root path follows. See +// proxmox.DestroyScratchLXC for the two API-side fixes that were built and refuted live. +func (e *Engine) teardownScratch(ctx context.Context, base JournalEntry, scratchMin, scratchMax int) { // Cancel-immune + bounded, so a shutdown mid-test still tears down. tctx, cancel := context.WithTimeout(context.WithoutCancel(ctx), 2*time.Minute) defer cancel() @@ -459,13 +464,14 @@ func (e *Engine) teardownScratch(ctx context.Context, base JournalEntry) { } upid, err := e.api.DestroyLXC(tctx, base.VMID) if err != nil { - // F-LEAK (Campaign 8): a 403 "missing privilege VM.Allocate" here means this scratch is not a - // felhom-pool member — a FAILED restore never completes the `--pool` association, and the - // token's VM.Allocate is granted at /pool/felhom, never at /. Fixed by GRANTING VM.Allocate on - // the scratch VMID band itself (host-install v1.21.0), path-scoped so the agent still cannot - // reach a non-scratch guest. NOT fixable from here: adopting the guest into the pool was tried - // and refused — `PUT /pools/{pool}` ALSO requires VM.Allocate on the VM being added, so - // Pool.Allocate alone cannot bootstrap membership (proven live 2026-07-28). + // A 403 "missing privilege VM.Allocate" here means this scratch is not a felhom-pool member: a + // FAILED restore never completes the `--pool` association, and the token's VM.Allocate is + // granted at /pool/felhom. Fall back to the band-scoped fenced destroy — WITHOUT it the guest + // leaks and holds its disks until a human removes it. + if e.destroyScratchPrivileged(tctx, base.VMID, scratchMin, scratchMax, err) { + e.append(withState(base, OpSucceeded)) + return + } e.logger.Error("restore-test: scratch teardown failed; left for Recover", "vmid", base.VMID, "err", err) return } @@ -568,3 +574,37 @@ func withUPID(base JournalEntry, upid string, state OpState) JournalEntry { base.At = time.Now().UTC() return base } + +// destroyScratchPrivileged is the F-LEAK fallback: destroy a stranded scratch through the fenced root +// path when the API token cannot. Reports whether the guest is gone. +// +// It refuses unless the guest is BOTH agent-created scratch provenance (this journal entry) and inside +// the configured band. That is the innermost of three checks — sudoers matches the vmid literally and +// proxmox.DestroyScratchLXC re-checks the band — because this op DESTROYS and the band must not rest +// on a single guard. +func (e *Engine) destroyScratchPrivileged(ctx context.Context, vmid, bandMin, bandMax int, apiErr error) bool { + if e.hostRun == nil { + e.logger.Warn("restore-test: no host-root runner wired — cannot reclaim the stranded scratch", + "vmid", vmid, "api_err", apiErr) + return false + } + if bandMin <= 0 || bandMax < bandMin { + e.logger.Error("restore-test: scratch band is not configured — refusing the privileged teardown", + "vmid", vmid, "min", bandMin, "max", bandMax) + return false + } + if vmid < bandMin || vmid > bandMax { + e.logger.Error("restore-test: refusing the privileged teardown — vmid is outside the scratch band", + "vmid", vmid, "min", bandMin, "max", bandMax) + return false + } + e.logger.Warn("restore-test: API teardown failed (stranded scratch is in no pool) — reclaiming via the fenced root path", + "vmid", vmid, "api_err", apiErr) + if err := proxmox.NewPrivileged(e.hostRun, "").DestroyScratchLXC(ctx, vmid, bandMin, bandMax); err != nil { + e.logger.Error("restore-test: privileged scratch teardown ALSO failed; left for Recover", + "vmid", vmid, "err", err) + return false + } + e.logger.Warn("restore-test: stranded scratch guest reclaimed via the fenced root path", "vmid", vmid) + return true +}