v0.62.0 — A1: pool-membership ownership check for the stale-lock reaper

staleLockController.Guests() = ListLXC ∩ GET /pools/felhom members (ownership PROVEN via the pool registry, never assumed from enumeration scope); pool-read failure fail-safes the whole recovery through the existing guest-list guard. New Client.Pool read (needs Pool.Audit — host-install v1.9.0; Pool.Allocate does NOT satisfy it, spike T2). Composed pve:pool-read capability (non-critical) + --selftest pool-read line. Red-proofed negative tests drive the REAL controller over a broad-token-shaped fake. Per SPIKE-a1-pool-membership-read-2026-07-03.md; audit A1 (AUDIT-blast-radius-hostroot-localapi-2026-07-02). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01PSK5g6qYLknKj8u3QAFEr6
2026-07-03 13:25:05 +02:00
parent 3e760a559a
commit 3f37c5fc23
8 changed files with 386 additions and 26 deletions
@@ -2,6 +2,8 @@ package localapi

 import (
 	"context"
+	"fmt"
+	"log/slog"
 	"strconv"

 	"gitea.dooplex.hu/admin/felhom-agent/internal/proxmox"
@@ -22,6 +24,10 @@ import (
 // Scope is deliberately narrow: only the two vzdump-related locks are ever touched. migrate/disk/create/
 // rollback/mounted/snapshot locks are left alone (they belong to a different operation, mid-flight or
 // needing different handling). The recovery is idempotent and never touches a guest without a stale lock.
+//
+// Ownership (audit A1, v0.62.0): the scan covers ONLY felhom-pool members — the production controller
+// intersects ListLXC with GET /pools/{pool} membership, so even under a broad token the reaper can
+// never unlock/start a co-tenant's guest. A failed pool read fails safe (whole recovery skipped).

 // staleBackupLocks are the lock values an interrupted vzdump can leave. ONLY these are cleared.
 var staleBackupLocks = map[string]bool{
@@ -134,11 +140,14 @@ func (s *Server) recoverOneStaleLock(ctx context.Context, g proxmox.Guest) {
 type staleLockController struct {
 	px     staleLockAPI
 	runner proxmox.Runner
+	pool   string       // ownership registry: only members of this PVE pool are ever scanned (A1)
+	logger *slog.Logger // the one success-path scan-summary line; nil = silent
 }

 // staleLockAPI is the subset of *proxmox.Client the controller uses (kept narrow for clarity/testing).
 type staleLockAPI interface {
 	ListLXC(ctx context.Context) ([]proxmox.Guest, error)
+	Pool(ctx context.Context, name string) (proxmox.PoolInfo, error)
 	GuestConfig(ctx context.Context, vmid int) (proxmox.GuestConfig, error)
 	ListSnapshots(ctx context.Context, vmid int) ([]proxmox.Snapshot, error)
 	ListRunningTasks(ctx context.Context) ([]proxmox.TaskStatus, error)
@@ -148,16 +157,48 @@ type staleLockAPI interface {
 }

 // NewStaleLockController builds the production controller. Returns nil if px or runner is nil (the
-// feature then stays unwired and RecoverStaleLockedGuests is a no-op).
-func NewStaleLockController(px staleLockAPI, runner proxmox.Runner) StaleLockController {
+// feature then stays unwired and RecoverStaleLockedGuests is a no-op). pool names the PVE pool the
+// scan is restricted to (reconcile.DefaultPool in production).
+func NewStaleLockController(px staleLockAPI, runner proxmox.Runner, pool string, logger *slog.Logger) StaleLockController {
 	if px == nil || runner == nil {
 		return nil
 	}
-	return &staleLockController{px: px, runner: runner}
+	return &staleLockController{px: px, runner: runner, pool: pool, logger: logger}
 }

+// Guests returns ListLXC ∩ the felhom pool's members (audit A1: ownership is PROVEN via the pool
+// registry, never assumed from enumeration scope). Under the pool-scoped token the intersect is a
+// no-op (ListLXC is already pool-filtered — spike T1); under a broad token it is the guard that
+// keeps the reaper off co-tenant guests. A pool-read failure returns an error — the caller's
+// existing "guest list unavailable — skipping recovery" guard then fail-safes the whole scan
+// (unknown ownership ⇒ don't act, mirroring reconcile/recover.go's proof-of-launch gate). NEVER
+// fall back to the unfiltered ListLXC list on error.
 func (c *staleLockController) Guests(ctx context.Context) ([]proxmox.Guest, error) {
-	return c.px.ListLXC(ctx)
+	lxc, err := c.px.ListLXC(ctx)
+	if err != nil {
+		return nil, err
+	}
+	pool, err := c.px.Pool(ctx, c.pool)
+	if err != nil {
+		return nil, fmt.Errorf("pool membership read (pool=%s): %w", c.pool, err)
+	}
+	// A pool can hold storages too (type "storage", no vmid) — membership is nonzero-vmid guests only.
+	members := make(map[int]bool, len(pool.Members))
+	for _, m := range pool.Members {
+		if m.VMID != 0 && m.Type != "storage" {
+			members[m.VMID] = true
+		}
+	}
+	owned := lxc[:0:0]
+	for _, g := range lxc {
+		if members[g.VMID] {
+			owned = append(owned, g)
+		}
+	}
+	if c.logger != nil {
+		c.logger.Info("stale-lock: scanning pool guests", "pool", c.pool, "listed", len(lxc), "scanned", len(owned))
+	}
+	return owned, nil
 }

 func (c *staleLockController) Lock(ctx context.Context, vmid int) (string, bool, error) {
@@ -0,0 +1,224 @@
+package localapi
+
+import (
+	"context"
+	"encoding/json"
+	"errors"
+	"io"
+	"log/slog"
+	"strings"
+	"testing"
+
+	"gitea.dooplex.hu/admin/felhom-agent/internal/proxmox"
+)
+
+// A1 (audit AUDIT-blast-radius-hostroot-localapi-2026-07-02, spike SPIKE-a1-pool-membership-read):
+// these tests drive the REAL staleLockController (not the Server-level fakeStaleLock) so the
+// pool-membership intersect in Guests() is the code under test. The staleLockAPI fake simulates a
+// BROAD token (ListLXC returns non-pool guests too — the exploit precondition); the recording
+// runner captures every `pct unlock` the controller would execute.
+
+// fakeStaleLockAPI is a scripted staleLockAPI: broad-token-shaped ListLXC + a scripted pool read,
+// with the mutating calls recorded.
+type fakeStaleLockAPI struct {
+	lxc     []proxmox.Guest
+	pool    proxmox.PoolInfo
+	poolErr error
+	cfg     map[int]proxmox.GuestConfig
+	snaps   map[int][]proxmox.Snapshot
+	tasks   []proxmox.TaskStatus
+
+	delsnap []int
+	started []int
+}
+
+func (f *fakeStaleLockAPI) ListLXC(context.Context) ([]proxmox.Guest, error) { return f.lxc, nil }
+
+func (f *fakeStaleLockAPI) Pool(_ context.Context, name string) (proxmox.PoolInfo, error) {
+	if f.poolErr != nil {
+		return proxmox.PoolInfo{}, f.poolErr
+	}
+	return f.pool, nil
+}
+
+func (f *fakeStaleLockAPI) GuestConfig(_ context.Context, vmid int) (proxmox.GuestConfig, error) {
+	return f.cfg[vmid], nil
+}
+
+func (f *fakeStaleLockAPI) ListSnapshots(_ context.Context, vmid int) ([]proxmox.Snapshot, error) {
+	return f.snaps[vmid], nil
+}
+
+func (f *fakeStaleLockAPI) ListRunningTasks(context.Context) ([]proxmox.TaskStatus, error) {
+	return f.tasks, nil
+}
+
+func (f *fakeStaleLockAPI) DeleteSnapshot(_ context.Context, vmid int, snapname string) (string, error) {
+	f.delsnap = append(f.delsnap, vmid)
+	return "", nil // synchronous — no WaitTask
+}
+
+func (f *fakeStaleLockAPI) Start(_ context.Context, vmid int) (string, error) {
+	f.started = append(f.started, vmid)
+	return "", nil
+}
+
+func (f *fakeStaleLockAPI) WaitTask(_ context.Context, upid string, _ proxmox.WaitOptions) (proxmox.TaskStatus, error) {
+	return proxmox.TaskStatus{Status: "stopped", ExitStatus: "OK"}, nil
+}
+
+// recordingRunner captures every fenced-CLI invocation (the controller's `pct unlock`).
+type recordingRunner struct {
+	calls [][]string
+}
+
+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, nil
+}
+
+func (r *recordingRunner) RunStdin(ctx context.Context, _ io.Reader, name string, args ...string) ([]byte, []byte, error) {
+	return r.Run(ctx, name, args...)
+}
+
+func (r *recordingRunner) unlocked(vmid string) bool {
+	for _, c := range r.calls {
+		if len(c) == 3 && c[0] == "pct" && c[1] == "unlock" && c[2] == vmid {
+			return true
+		}
+	}
+	return false
+}
+
+// staleCfg builds a GuestConfig whose Lock()/OnBoot() read the given values (both live in Extra).
+func staleCfg(lock string, onboot bool) proxmox.GuestConfig {
+	extra := map[string]json.RawMessage{}
+	if lock != "" {
+		extra["lock"] = json.RawMessage(`"` + lock + `"`)
+	}
+	if onboot {
+		extra["onboot"] = json.RawMessage(`1`)
+	}
+	return proxmox.GuestConfig{Extra: extra}
+}
+
+// poolScanServer wires the REAL production controller (fake API + recording runner) into a Server.
+func poolScanServer(api *fakeStaleLockAPI, runner *recordingRunner) *Server {
+	ctrl := NewStaleLockController(api, runner, "felhom", nil)
+	return &Server{staleLock: ctrl, logger: slog.New(slog.NewTextHandler(io.Discard, nil))}
+}
+
+// TestStaleLock_ForeignGuestNotReaped forecloses the A1 exploit: under a broad token, a co-tenant
+// (non-pool) guest with the exact stale-lock signature (snapshot-delete lock + dangling vzdump
+// snapshot + no vzdump task visible yet) must be EXCLUDED by the pool intersect — never unlocked,
+// never snapshot-deleted, never started.
+func TestStaleLock_ForeignGuestNotReaped(t *testing.T) {
+	api := &fakeStaleLockAPI{
+		lxc: []proxmox.Guest{ // broad-token-shaped: the foreign guest IS enumerated
+			{VMID: 9201, Status: "stopped"},
+			{VMID: 5000, Status: "stopped"},
+		},
+		pool: proxmox.PoolInfo{PoolID: "felhom", Members: []proxmox.PoolMember{{VMID: 9201, Type: "lxc"}}},
+		cfg: map[int]proxmox.GuestConfig{
+			9201: staleCfg("", false),               // healthy pool member
+			5000: staleCfg("snapshot-delete", true), // the co-tenant's interrupted backup
+		},
+		snaps: map[int][]proxmox.Snapshot{5000: {{Name: "vzdump"}}},
+	}
+	runner := &recordingRunner{}
+	poolScanServer(api, runner).RecoverStaleLockedGuests(context.Background())
+
+	if runner.unlocked("5000") {
+		t.Fatalf("A1 exploit: foreign guest 5000 was unlocked; runner calls=%v", runner.calls)
+	}
+	if contains(api.delsnap, 5000) {
+		t.Fatalf("A1 exploit: foreign guest 5000's vzdump snapshot was deleted; delsnap=%v", api.delsnap)
+	}
+	if contains(api.started, 5000) {
+		t.Fatalf("A1 exploit: foreign guest 5000 was force-started; started=%v", api.started)
+	}
+}
+
+// TestStaleLock_PoolGuestStillReaped is the over-filtering companion: an owned (pool-member) guest
+// with the same stale signature IS fully recovered — the filter must not be "reap nothing".
+func TestStaleLock_PoolGuestStillReaped(t *testing.T) {
+	api := &fakeStaleLockAPI{
+		lxc: []proxmox.Guest{
+			{VMID: 9201, Status: "stopped"},
+			{VMID: 5000, Status: "stopped"},
+		},
+		pool: proxmox.PoolInfo{PoolID: "felhom", Members: []proxmox.PoolMember{{VMID: 9201, Type: "lxc"}}},
+		cfg: map[int]proxmox.GuestConfig{
+			9201: staleCfg("snapshot-delete", true), // the F2-b recovery case, on the OWNED guest
+			5000: staleCfg("", false),
+		},
+		snaps: map[int][]proxmox.Snapshot{9201: {{Name: "vzdump"}}},
+	}
+	runner := &recordingRunner{}
+	poolScanServer(api, runner).RecoverStaleLockedGuests(context.Background())
+
+	if !runner.unlocked("9201") {
+		t.Fatalf("owned guest 9201 must still be unlocked; runner calls=%v", runner.calls)
+	}
+	if !contains(api.delsnap, 9201) {
+		t.Fatalf("owned guest 9201's dangling snapshot must be deleted; delsnap=%v", api.delsnap)
+	}
+	if !contains(api.started, 9201) {
+		t.Fatalf("owned guest 9201 (onboot, stopped) must be started; started=%v", api.started)
+	}
+}
+
+// TestStaleLock_PoolReadFails_SkipsAll: when ownership can't be PROVEN (pool read errors — 403 on a
+// pre-v1.9.0 ACL, timeout, parse failure), the whole recovery fail-safes: ZERO mutations on ANY
+// guest, never a fallback to the unfiltered list.
+func TestStaleLock_PoolReadFails_SkipsAll(t *testing.T) {
+	api := &fakeStaleLockAPI{
+		lxc:     []proxmox.Guest{{VMID: 9201, Status: "stopped"}},
+		poolErr: errors.New(`proxmox: GET /pools/felhom: 403 Permission check failed (/pool/felhom, Pool.Audit)`),
+		cfg:     map[int]proxmox.GuestConfig{9201: staleCfg("snapshot-delete", true)},
+		snaps:   map[int][]proxmox.Snapshot{9201: {{Name: "vzdump"}}},
+	}
+	runner := &recordingRunner{}
+	poolScanServer(api, runner).RecoverStaleLockedGuests(context.Background())
+
+	if len(runner.calls) != 0 || len(api.delsnap) != 0 || len(api.started) != 0 {
+		t.Fatalf("pool-read failure must skip ALL recovery; runner=%v delsnap=%v started=%v",
+			runner.calls, api.delsnap, api.started)
+	}
+
+	// The wrapped error the :64 guard logs must name the pool read (operator diagnosability).
+	_, err := NewStaleLockController(api, runner, "felhom", nil).Guests(context.Background())
+	if err == nil || !strings.Contains(err.Error(), "pool membership read (pool=felhom)") {
+		t.Fatalf("Guests() error must name the pool read; got %v", err)
+	}
+}
+
+// TestStaleLockController_GuestsIntersect covers the intersect edges (§8): storage-type pool members
+// and zero-vmid entries never grant membership; an EMPTY pool is a valid empty scan, not an error.
+func TestStaleLockController_GuestsIntersect(t *testing.T) {
+	api := &fakeStaleLockAPI{
+		lxc: []proxmox.Guest{{VMID: 9201}, {VMID: 5000}},
+		pool: proxmox.PoolInfo{PoolID: "felhom", Members: []proxmox.PoolMember{
+			{VMID: 9201, Type: "lxc"},
+			{VMID: 0, Type: "storage"}, // a pool-attached storage — must not grant vmid-0 membership
+		}},
+	}
+	ctrl := NewStaleLockController(api, &recordingRunner{}, "felhom", nil)
+
+	got, err := ctrl.Guests(context.Background())
+	if err != nil {
+		t.Fatalf("Guests: %v", err)
+	}
+	if len(got) != 1 || got[0].VMID != 9201 {
+		t.Fatalf("intersect must keep exactly the guest pool members; got %v", got)
+	}
+
+	api.pool = proxmox.PoolInfo{PoolID: "felhom"} // empty pool (a box before first provision)
+	got, err = ctrl.Guests(context.Background())
+	if err != nil {
+		t.Fatalf("empty pool must not error: %v", err)
+	}
+	if len(got) != 0 {
+		t.Fatalf("empty pool ⇒ empty scan; got %v", got)
+	}
+}