From 8db92947cd1fac305b083f049eb4b1cc714f0ea1 Mon Sep 17 00:00:00 2001 From: kisfenyo Date: Tue, 28 Jul 2026 11:05:37 +0200 Subject: [PATCH] =?UTF-8?q?F-LEAK:=20remove=20the=20pool-adoption=20fix=20?= =?UTF-8?q?=E2=80=94=20refuted=20live;=20the=20fix=20is=20a=20path-scoped?= =?UTF-8?q?=20ACL=20(v0.108.0)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit PUT /pools/{pool} ALSO requires VM.Allocate on the VM being added, so Pool.Allocate cannot bootstrap its own membership. Proven live on demo-hp 2026-07-28. The real fix is felhom-host-install v1.21.0 granting FelhomAgentGuest at /vms/990000..990009. --- CHANGELOG.md | 29 ++++++++++ internal/reconcile/restoretest.go | 70 ++++------------------- internal/reconcile/scratch_adopt_test.go | 71 ------------------------ 3 files changed, 40 insertions(+), 130 deletions(-) delete mode 100644 internal/reconcile/scratch_adopt_test.go diff --git a/CHANGELOG.md b/CHANGELOG.md index 0091047..cbde816 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,5 +1,34 @@ # felhom-agent — Changelog +## v0.108.0 — F-LEAK: the pool-adoption fix was WRONG; the fix is a path-scoped ACL (2026-07-28) + +**A correction, made because the live replay refuted the design.** v0.107.0 shipped a scratch-teardown +fallback that, on a 403, adopted the stranded guest into the `felhom` pool and retried the destroy — +reasoning that the token holds `Pool.Allocate` on `/pool/felhom`. The live replay on demo-hp fired +that path exactly as designed and PVE refused it: + +``` +WARN restore-test: scratch teardown failed — adopting the stranded scratch ... vmid=990000 pool=felhom +ERROR restore-test: pool adoption failed; left for Recover vmid=990000 + err="proxmox: PUT /pools/felhom -> HTTP 500: permission denied at /vms/990000 (missing privilege ...)" +``` + +`PUT /pools/{pool}` **also** requires `VM.Allocate` on the VM being added — the very privilege the +403 was about. **Pool membership cannot bootstrap its own authority.** The adoption code and its +guard (`scratchAdoptAllowed`) are removed; a path that provably cannot work should not ship, and +leaving it would have left a plausible-looking fix in place of a real one. + +**The actual fix lives in `felhom-host-install.sh` v1.21.0**: the `FelhomAgentGuest` role is now +granted at each `/vms/990000`…`/vms/990009` path — the restore-test's scratch band. PVE ACLs are +path-scoped, so this authorises the agent on exactly those ten IDs. It is **not** granted at `/vms`, +which would authorise destroying every guest on the box. + +Proven live on demo-hp: `/vms/990000` → **has** `VM.Allocate` (13 privs); `/vms/100`, `/vms/9999` and +`/vms/990010` (one past the band) → **no** `VM.Allocate` (3 privs, base only). + +Code side keeps only the corrected diagnostic on the teardown-failure branch, which now records what +the 403 means and that adoption was tried and refused — so the next reader does not re-derive it. + ## v0.107.0 — F-REBOOT + F-LEAK: the agent's authority over guest lifecycle (2026-07-28) Two Campaign 8 findings, both about the agent being unable to act on a guest it owns. diff --git a/internal/reconcile/restoretest.go b/internal/reconcile/restoretest.go index d1992a2..b27bd90 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, spec.ScratchMin, spec.ScratchMax) + e.teardownScratch(ctx, base) return } e.append(withState(base, OpFailed)) @@ -444,44 +444,9 @@ func sizeToGB(s string) int { return gb } -// scratchAdoptAllowed decides whether a stranded guest may be adopted into the felhom pool so the -// pool-scoped token can destroy it (F-LEAK). PURE, so the refusal is unit-testable without PVE. -// -// TWO INDEPENDENT GUARDS, both required. This function is the only thing standing between "clean up -// my own scratch" and "co-opt an arbitrary guest into the pool and delete it", so it does not rely -// on either check alone: -// - PROVENANCE: the journal entry must be one the agent itself created as a restore-test scratch. -// - NUMERIC BAND: the VMID must be inside the configured scratch band (scratch_vmid_min..max). -// -// A guest failing either is refused, loudly. Adopting a customer guest into the pool would hand the -// token destroy rights over it, which is a far worse outcome than a leaked scratch. -func scratchAdoptAllowed(vmid, min, max int, scratch bool) (bool, string) { - if !scratch { - return false, "journal entry is not agent-created scratch provenance" - } - if min <= 0 || max < min { - return false, fmt.Sprintf("scratch band [%d,%d] is not configured", min, max) - } - if vmid < min || vmid > max { - return false, fmt.Sprintf("vmid %d is outside the scratch band [%d,%d]", vmid, min, max) - } - return true, "" -} - // 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. -// -// F-LEAK (Campaign 8): a restore-test whose RESTORE FAILED left a scratch guest the agent could not -// destroy — `DELETE /nodes/x/lxc/990000` returned 403 "missing privilege VM.Allocate". The cause is -// pool membership, not privsep: VM.Allocate is granted at /pool/felhom ONLY (never at /), and a -// failed restore never completes the `--pool felhom` association, so the guest's own path resolves -// to / where the token holds nothing. Verified live: /vms/ grants only -// Datastore.Audit+SDN.Use+Sys.Audit, while /pool/felhom grants VM.Allocate AND Pool.Allocate. -// -// So the recovery needs NO new privilege: Pool.Allocate is already held, so we adopt the stranded -// scratch into the pool and retry the destroy, which then authorizes via /pool/felhom. Guarded by -// scratchAdoptAllowed — see there for why two guards rather than one. -func (e *Engine) teardownScratch(ctx context.Context, base JournalEntry, scratchMin, scratchMax int) { +func (e *Engine) teardownScratch(ctx context.Context, base JournalEntry) { // Cancel-immune + bounded, so a shutdown mid-test still tears down. tctx, cancel := context.WithTimeout(context.WithoutCancel(ctx), 2*time.Minute) defer cancel() @@ -494,28 +459,15 @@ func (e *Engine) teardownScratch(ctx context.Context, base JournalEntry, scratch } upid, err := e.api.DestroyLXC(tctx, base.VMID) if err != nil { - // F-LEAK: the destroy may have 403'd because a FAILED restore never completed pool - // membership, leaving the guest outside /pool/felhom where the token's VM.Allocate lives. - // Adopt it into the pool (Pool.Allocate, already granted) and retry ONCE. Any other error - // falls through to the original behaviour. - if ok, why := scratchAdoptAllowed(base.VMID, scratchMin, scratchMax, base.Scratch); !ok { - e.logger.Error("restore-test: scratch teardown failed and adoption REFUSED; left for Recover", - "vmid", base.VMID, "refused_because", why, "err", err) - return - } - e.logger.Warn("restore-test: scratch teardown failed — adopting the stranded scratch into the pool and retrying once", - "vmid", base.VMID, "pool", DefaultPool, "err", err) - if perr := e.api.PoolAddVMID(tctx, DefaultPool, base.VMID); perr != nil { - e.logger.Error("restore-test: pool adoption failed; left for Recover", "vmid", base.VMID, "err", perr) - return - } - upid, err = e.api.DestroyLXC(tctx, base.VMID) - if err != nil { - e.logger.Error("restore-test: scratch teardown failed even after pool adoption; left for Recover", - "vmid", base.VMID, "err", err) - return - } - e.logger.Info("restore-test: stranded scratch adopted into the pool and destroyed", "vmid", base.VMID) + // 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). + e.logger.Error("restore-test: scratch teardown failed; left for Recover", "vmid", base.VMID, "err", err) + return } if upid != "" { if _, err := e.api.WaitTask(tctx, upid, proxmox.WaitOptions{}); err != nil { diff --git a/internal/reconcile/scratch_adopt_test.go b/internal/reconcile/scratch_adopt_test.go deleted file mode 100644 index d53461f..0000000 --- a/internal/reconcile/scratch_adopt_test.go +++ /dev/null @@ -1,71 +0,0 @@ -package reconcile - -import "testing" - -// F-LEAK (Campaign 8): a restore-test whose RESTORE FAILED could not destroy its own scratch guest — -// `DELETE /nodes/x/lxc/990000` → 403 "missing privilege VM.Allocate". The cause is pool membership, -// not privsep: VM.Allocate is granted at /pool/felhom only, and a failed restore never completes the -// `--pool felhom` association, so the guest's own path resolves to / where the token holds nothing. -// -// The fix adopts the stranded scratch into the pool (Pool.Allocate — already granted) and retries the -// destroy. scratchAdoptAllowed is the guard that keeps that from becoming "co-opt any guest into the -// pool and delete it", and this file is its mirror test. -// -// Scenario D (the scratch is destroyed) is the live replay; Scenario E — the agent still cannot reach -// a non-scratch guest this way — is HERE, because it must hold as a pure property and not depend on -// what PVE happens to refuse. - -// Scenario E — the adoption path REFUSES anything outside the scratch band. -// -// RED-PROOF: replace the band check with `return true, ""` → every case below reports allowed and the -// test fails with "adoption ALLOWED for vmid 9201 — that would let the agent co-opt a customer guest -// into the pool and destroy it". -func TestScratchAdoptAllowed_RefusesNonScratch(t *testing.T) { - const min, max = 990000, 990009 - for _, tc := range []struct { - name string - vmid int - scratch bool - }{ - {"a live customer guest", 9201, true}, // scratch-flagged but OUTSIDE the band - {"the golden image", 9100, true}, // ditto - {"just below the band", min - 1, true}, // off-by-one - {"just above the band", max + 1, true}, // off-by-one - {"in-band but NOT scratch provenance", min, false}, - {"neither", 100, false}, - } { - t.Run(tc.name, func(t *testing.T) { - ok, why := scratchAdoptAllowed(tc.vmid, min, max, tc.scratch) - if ok { - t.Errorf("adoption ALLOWED for vmid %d — that would let the agent co-opt a non-scratch guest into the pool and destroy it", tc.vmid) - } - if why == "" { - t.Error("refusal carried no reason — a silent refusal is unreviewable") - } - }) - } -} - -// The in-band, scratch-provenanced case IS allowed — otherwise the fix does nothing and the leak stays. -// -// RED-PROOF: make scratchAdoptAllowed always return false → this fails with "adoption refused for a -// genuine in-band scratch guest", i.e. F-LEAK is not fixed at all. -func TestScratchAdoptAllowed_AllowsGenuineScratch(t *testing.T) { - const min, max = 990000, 990009 - for _, vmid := range []int{min, min + 5, max} { - ok, why := scratchAdoptAllowed(vmid, min, max, true) - if !ok { - t.Errorf("adoption refused for a genuine in-band scratch guest %d: %s", vmid, why) - } - } -} - -// An unconfigured band must refuse everything rather than defaulting to something permissive — a -// zero-valued band is a wiring bug, and the safe reading of a wiring bug is "do nothing". -func TestScratchAdoptAllowed_UnconfiguredBandRefuses(t *testing.T) { - for _, tc := range []struct{ min, max int }{{0, 0}, {0, 990009}, {990009, 990000}, {-1, 5}} { - if ok, _ := scratchAdoptAllowed(990000, tc.min, tc.max, true); ok { - t.Errorf("adoption allowed with an unconfigured band [%d,%d]", tc.min, tc.max) - } - } -}