From 3f37c5fc2349faac7e3f68ef3ec08df741a4612c Mon Sep 17 00:00:00 2001 From: kisfenyo Date: Fri, 3 Jul 2026 13:25:05 +0200 Subject: [PATCH] =?UTF-8?q?v0.62.0=20=E2=80=94=20A1:=20pool-membership=20o?= =?UTF-8?q?wnership=20check=20for=20the=20stale-lock=20reaper?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 Claude-Session: https://claude.ai/code/session_01PSK5g6qYLknKj8u3QAFEr6 --- CHANGELOG.md | 31 ++++ CONTEXT.md | 11 +- REUSE.md | 7 +- cmd/felhom-agent/main.go | 66 +++++-- internal/localapi/stalelock.go | 49 ++++- internal/localapi/stalelock_pool_test.go | 224 +++++++++++++++++++++++ internal/proxmox/query.go | 9 + internal/proxmox/types.go | 15 ++ 8 files changed, 386 insertions(+), 26 deletions(-) create mode 100644 internal/localapi/stalelock_pool_test.go diff --git a/CHANGELOG.md b/CHANGELOG.md index 3a4d02e..fb4cabc 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,3 +1,34 @@ +## v0.62.0 — A1: pool-membership ownership check for the stale-lock reaper (2026-07-03) + +Implements audit finding A1 (`AUDIT-blast-radius-hostroot-localapi-2026-07-02` §A) per the spike +verdict (`SPIKE-a1-pool-membership-read-2026-07-03` — enumeration is pool-filtered under the scoped +token, so this is defense-in-depth: a future broad-token deployment can no longer re-arm the reaper +against co-tenant guests). Companion: host-install **v1.9.0** (`Pool.Audit` added to +`FelhomAgentGuest`) — **rescope BEFORE deploying this agent**, else the reaper fail-safes (skips) +until the ACL catches up. + +- **`Client.Pool`** (proxmox/query.go): `GET /pools/{name}` → `PoolInfo{PoolID, Members[]{VMID,Type}}`. + Requires `Pool.Audit` at `/pool/{name}`; `Pool.Allocate` does NOT satisfy the read (spike T2). +- **`staleLockController.Guests()`** (localapi/stalelock.go): now returns `ListLXC ∩ pool members` + (nonzero-vmid, non-storage entries only). Ownership is PROVEN via the pool registry, never assumed + from enumeration scope. A pool-read failure returns a wrapped error ("pool membership read + (pool=felhom): …") that rides the existing "guest list unavailable — skipping recovery" guard — + fail-safe: NO unlock/snapshot-delete/start on ANY guest, never a fallback to the unfiltered list. + One new INFO line per scan: `stale-lock: scanning pool guests` (pool, listed, scanned) — emitted + by the controller (the unchanged `StaleLockController` seam can't carry the pre-intersect count). +- **`NewStaleLockController`** gains `(pool string, logger *slog.Logger)`; main.go threads + `reconcile.DefaultPool`. +- **Capability surfacing**: the hub-report prober is now a composed closure — the sudo manifest + probe + one `pve:pool-read` status (non-critical; degraded ⇒ reaper is fail-safed, visible on the + report, no operator page). Composed in main.go; `internal/capability/` untouched. +- **`--selftest`**: new "pool read" line (pool id + member count + guest members). +- **Tests** (stalelock_pool_test.go, driving the REAL controller over a broad-token-shaped fake): + `TestStaleLock_ForeignGuestNotReaped` (red-proved: intersect removed ⇒ FAILS with + `pct unlock 5000` recorded), `TestStaleLock_PoolGuestStillReaped` (anti-over-filter), + `TestStaleLock_PoolReadFails_SkipsAll` (red-proved: fallback-to-unfiltered ⇒ FAILS with mutations + recorded), `TestStaleLockController_GuestsIntersect` (storage-member + empty-pool edges). The 9 + existing Server-level stalelock tests pass unmodified. + ## docs — CLAUDE.md refresh: stable orientation, complete layout (2026-07-03) No code change, no version bump. Deleted the version-pinned "Current: v0.31.0" narrative and the diff --git a/CONTEXT.md b/CONTEXT.md index 3e04ef2..69eea97 100644 --- a/CONTEXT.md +++ b/CONTEXT.md @@ -5,6 +5,12 @@ ## Current +- **v0.62.0** (2026-07-03) — **audit A1 RESOLVED**: the stale-lock reaper's scan is now + pool-intersected (`staleLockController.Guests()` = `ListLXC` ∩ `Client.Pool("felhom")` members), + fail-safe skip on pool-read failure; `pve:pool-read` capability (non-critical) + `--selftest` + "pool read" line. Companion host-install **v1.9.0** adds `Pool.Audit` to `FelhomAgentGuest` — + **deploy order on any box: rescope ACL first, then this agent.** Per + `SPIKE-a1-pool-membership-read-2026-07-03.md`; red-proofed tests in stalelock_pool_test.go. - **2026-07-03 — CLAUDE.md refreshed**: version narrative removed (state lives HERE + CHANGELOG top), layout completed (all 17 internal packages + cmd/felhom-opsign); deploy runbook now in the `felhom-build-deploy` skill (`felhom.eu/skills/`). - **2026-07-03 — `REUSE.md` exists at the repo root** (canonical helpers / format-safety guards / traps / seams, code-verified); maintenance rule active: update it in the same commit that changes a shared helper. - **v0.61.0** (2026-07-03) — blast-radius audit fixes **B1 + D1 + D2 + D3** from @@ -17,11 +23,6 @@ ## Open threads -- **A1 (LOW, pending SPIKE — not fixed in v0.61.0):** stale-lock recovery - (`internal/localapi/stalelock.go`) reaps guests by a pool-blind `ListLXC` scan, not ownership. - Contained by the pool-scoped token today; **must be fixed before any broad/root-token deployment.** - It is a spike, not a patch: the role lacks `Pool.Audit`, so there is no cheap authorized - pool-membership read — which read to use is the open design question. - Deferred audit items (housekeeping/design, all INFO): C1 (controller-swap version floor), C2 (NAS server allowlist), A2 (gate journal cross-check), B2–B5, E1/E2. - Drive-enrollment leftovers: (a) `runStorageInit` slow-device detached-format polling; (b) Impl-3 diff --git a/REUSE.md b/REUSE.md index 55e358d..5d6f302 100644 --- a/REUSE.md +++ b/REUSE.md @@ -68,7 +68,7 @@ | `Server.scopedFromBody` | internal/localapi/server.go | `scopedFromBody(w, bodyVMID, tokenVMID, path) bool` | POST-body vmid self-scope check | Call right after decode; false = already 403'd | | `decodeBody` / `writeOK` / `writeErr` / `writeStatus` | internal/localapi/server.go | `writeStatus(w, code, ok, data, errMsg)` | ALL local-API JSON I/O | Do not hand-roll response envelopes | | `EnsureLeaf` | internal/localapi/cert.go | `EnsureLeaf(certPath, keyPath, host) (cert, fingerprint, generated, err)` | pinned self-signed leaf | `generated=true` invalidates every issued bootstrap pin — log LOUD (B.1) | -| `Server.RecoverStaleLockedGuests` | internal/localapi/stalelock.go | `RecoverStaleLockedGuests(ctx)` | startup stale vzdump-lock heal (F2-b) | Clears ONLY `backup`/`snapshot-delete`, only when no vzdump in-flight; A1 OPEN: pool-blind `ListLXC` scan | +| `Server.RecoverStaleLockedGuests` | internal/localapi/stalelock.go | `RecoverStaleLockedGuests(ctx)` | startup stale vzdump-lock heal (F2-b) | Clears ONLY `backup`/`snapshot-delete`, only when no vzdump in-flight; A1 RESOLVED (v0.62.0): scan is pool-intersected (`ListLXC` ∩ `Client.Pool`), fail-safe skip on pool-read failure | | `ControllerSwapper.Swap` + `ValidControllerImage` | internal/localapi/controllerswap.go | `Swap(ctx, vmid, target) *ControllerSwapState` | agent-owned controller image swap + rollback | Strict image regex (repo + 3-part semver); state file written BEFORE swap; no-healthcheck images need `verifyDwell` | ### Proxmox client / hub / PBS / provisioning @@ -76,6 +76,7 @@ | Symbol | File | Short signature | Use for | Gotchas | |---|---|---|---|---| | `Client.WaitTask` | internal/proxmox/task.go | `WaitTask(ctx, upid, opts) (TaskStatus, error)` | asserting EVERY mutating op | POST 200 ≠ success; authz can fail at task exec; `AllowWarnings` opt-in | +| `Client.Pool` | internal/proxmox/query.go | `Pool(ctx, name) (PoolInfo, error)` | felhom-pool membership (the ownership registry, A1) | Needs `Pool.Audit` at `/pool/` (host-install v1.9.0+); `Pool.Allocate` does NOT satisfy the read; members can be storages (type `storage`, vmid 0) — filter them | | `Client` mutate wrappers (`RestoreLXC/Vzdump/DestroyLXC/Snapshot/Rollback/SetConfig/ResizeLXC/Start/Stop`) | internal/proxmox/mutate.go | return `(upid, error)` | all API mutations | Async → always pair with WaitTask; route via gate/queue, not ad-hoc | | `TLSConfig.build` / `normalizeFingerprint` | internal/proxmox/tls.go | `build() (*tls.Config, error)` | PVE leaf-cert SHA-256 pinning | No insecure default | | `pinnedTLS` | internal/pbs/pin.go | `pinnedTLS(fingerprint) (*tls.Config, error)` | PBS leaf pinning | Same model as PVE; 64-hex fingerprint normalized | @@ -116,7 +117,7 @@ | Calling `Client.DestroyLXC`/`Vzdump`/`SetConfig` outside the gate/queue/journal | skips classification, signature, per-guest serialization, crash recovery | `reconcile.Engine` paths / `RunSignedJob`; queue via `Queue.Submit` | | `GuestBinder.AttachBind`/`DetachBind` (per-drive `pct set -mpN`) | legacy model; a missing bind source can brick guest boot (C1) | `AttachDrive`/`DetachDrive` (intermediary model) | | `isHostMountpoint` to reconcile bind state | boolean can't converge stacked double-binds (the /mnt doubling bug) | `countHostMounts` normalization inside `AttachDrive` | -| Copying `RecoverStaleLockedGuests`' pool-blind `ListLXC` scan | audit A1: acts on guests the agent doesn't own — contained ONLY by today's pool-scoped token | don't extend it; A1 spike must land before any broad-token feature builds on that scan | +| Acting on a raw `ListLXC` list as if it were "guests the agent owns" | audit A1 (pre-v0.62.0 the stale-lock reaper did exactly this — contained only by the pool-scoped token) | ownership must be PROVEN: intersect with `Client.Pool` membership like `staleLockController.Guests()` (v0.62.0), fail-safe on read failure | ## 4. Seams & interfaces (testing + cross-repo) @@ -127,7 +128,7 @@ | `storage.HostReader` | internal/storage/hostread.go | `*ProcHostReader` | `fakeHostReader` internal/localapi/disks_test.go; internal/storage/role_test.go | | `localapi.DiskOps` / `StorageGate` / `GuestAttacher` / `GuestLister` | internal/localapi/disks.go | `*storage.SudoHostOps`; `storageGateAdapter` (cmd/felhom-agent/main.go); `*GuestBinder`; `*proxmox.Client` | `fakeDiskOps`/`fakeGate`/`fakeGuestAttacher`/`fakeGuestList` internal/localapi/disks_test.go | | `localapi.GuestAPI` / `BackupService` / `BackupStore` / `TokenAuthority` | internal/localapi/server.go | `*proxmox.Client`, `*backup.BackupRunner`, `*backup.Store`, `*TokenStore` | `fakeGuests`/`fakeBackups`/`fakeStore` internal/localapi/server_test.go | -| `localapi.StaleLockController` | internal/localapi/stalelock.go | `*staleLockController` (Client + Runner) | `fakeStaleLock` internal/localapi/stalelock_test.go | +| `localapi.StaleLockController` | internal/localapi/stalelock.go | `*staleLockController` (Client + Runner + pool) | `fakeStaleLock` (Server-level) stalelock_test.go; `fakeStaleLockAPI` (controller-level, tests the A1 pool intersect) stalelock_pool_test.go | | `localapi.GuestExecutor` | internal/localapi/controllerswap.go | `*GuestBinder` (pct exec) | `fakeGuestExec` internal/localapi/controllerswap_test.go | | `reconcile.OpVerifier` | internal/reconcile/gate.go | `*authz.Verifier` | fake verifier in internal/reconcile gate tests | | `signedjobs.WipeOps` / `Executor` (`ExecutorChain`) | internal/signedjobs/wipe.go + runner.go | `*storage.SudoHostOps`; `WipeExecutor`+`DecommissionExecutor` | internal/signedjobs wipe/runner/decommission tests | diff --git a/cmd/felhom-agent/main.go b/cmd/felhom-agent/main.go index ca2e508..54324e3 100644 --- a/cmd/felhom-agent/main.go +++ b/cmd/felhom-agent/main.go @@ -45,7 +45,7 @@ import ( // version is the agent version. Overridable at build time with // -ldflags "-X main.version="; defaults to the in-repo CHANGELOG version. -var version = "0.61.0" +var version = "0.62.0" // runGuestHook is the PVE pre-start hook body (`felhom-agent guest-hook `). On the // pre-start phase it creates placeholder dirs for any absent bind-mount source so the guest always boots @@ -274,6 +274,28 @@ func (r *gateRemounter) Remount(ctx context.Context, t storage.KnownTarget) { r.logger.Info("storage: re-mounted returned target", "target", t.Name, "where", t.MountPath) } +// poolReadStatus probes the felhom-pool membership read (GET /pools/felhom) the stale-lock reaper's +// ownership scoping depends on (v0.62.0, audit A1). Needs `Pool.Audit` at `/pool/felhom` — granted +// by host-install v1.9.0; on an older ACL this reports degraded and the reaper fail-safes (skips). +func poolReadStatus(ctx context.Context, px *proxmox.Client) capability.Status { + s := capability.Status{ + Name: "pve:pool-read", + Feature: "stale-lock recovery scoping (pool ownership check)", + Critical: false, + Status: capability.StatusOK, + } + if px == nil { + s.Status, s.Reason = capability.StatusDegraded, "not configured" + return s + } + pctx, cancel := context.WithTimeout(ctx, 10*time.Second) + defer cancel() + if _, err := px.Pool(pctx, reconcile.DefaultPool); err != nil { + s.Status, s.Reason = capability.StatusDegraded, err.Error() + } + return s +} + // logCapabilities logs the privileged-capability self-check at startup: one INFO summary, plus an // ERROR per degraded capability naming the gated feature (so a missing grant is loud at cutover, // not days later). It never exits — serve-degraded. @@ -336,8 +358,14 @@ func runDaemon(cfg config.Config, logger *slog.Logger) int { // startup (loud on any denial) and attach the snapshot to every hub report; the hub owns the // ok→degraded alert. Serve-degraded — a missing grant never blocks startup. capProber := capability.Prober{Runner: &proxmox.ExecRunner{Mode: proxmox.RunnerDirect}} - logCapabilities(capProber.Probe(context.Background()), logger) - collector.SetCapabilityProber(capProber.Probe) + // A1 (v0.62.0): compose the PVE pool-read check AROUND the sudo prober (an API read does not + // belong inside the sudo-policy probe). Non-critical: a degraded pool read means the stale-lock + // reaper fail-safes (locks stay uncleared) — visible on the hub report, no operator page. + probeAll := func(ctx context.Context) []capability.Status { + return append(capProber.Probe(ctx), poolReadStatus(ctx, px)) + } + logCapabilities(probeAll(context.Background()), logger) + collector.SetCapabilityProber(probeAll) loop := hub.NewLoop(collector, client, time.Duration(hcfg.PollSeconds)*time.Second, logger) interval := time.Duration(hcfg.PollSeconds) * time.Second @@ -766,25 +794,26 @@ func buildLocalAPIServer(cfg config.Config, px *proxmox.Client, store *backup.St Backups: runner, Store: store, Storage: observer, - DriveTargets: driveTargets, // Impl-2a: registry+units drives for the /disks view (union w/ Observe storages) + DriveTargets: driveTargets, // Impl-2a: registry+units drives for the /disks view (union w/ Observe storages) HostReader: storage.NewProcHostReader(), // Impl-2b: durableIDForMount raw-mount fallback + role gate Tokens: tokens, BackupCadence: cfg.Backup.BackupCadence(), // Disk management (slice 8C): the privileged host surface + the data-bearing wipe gate. - Disks: hostOps, - DiskGate: storageGateAdapter{gate: gate, hostID: cfg.Hub.HostID}, - Guests2: px, - GuestAttach: guestBinder, // slice 10 P2: bind enrolled data drives into the guest + Disks: hostOps, + DiskGate: storageGateAdapter{gate: gate, hostID: cfg.Hub.HostID}, + Guests2: px, + GuestAttach: guestBinder, // slice 10 P2: bind enrolled data drives into the guest // Network storage (NAS) — Part A1: the privileged host network-mount surface (NFS/SMB automount). - NetStorage: hostOps, - SmbCredsDir: cfg.Privileged.SmbCredsDir, + NetStorage: hostOps, + SmbCredsDir: cfg.Privileged.SmbCredsDir, ControllerSwap: guestBinder, // Phase 1: agentic controller update — in-guest image swap // F2-b: recover a guest left with a stale vzdump lock by a reboot-during-backup. Reads + start // go through the API client; the `pct unlock` is the one fenced root-CLI op (no API equivalent). - StaleLock: localapi.NewStaleLockController(px, &proxmox.ExecRunner{Mode: gaMode, SudoPath: cfg.Privileged.SudoPath}), - Intent: intent, // slice 10 P3: record enroll/eject intent for self-heal - GuestBinds: guestBinds, // F9: per-guest bind record for the startup re-assert - FormatJobs: formatJobs, // F20-BUG3: detached-format job record + restart recovery + // A1 (v0.62.0): the scan is restricted to felhom-pool members (ownership proven, not assumed). + StaleLock: localapi.NewStaleLockController(px, &proxmox.ExecRunner{Mode: gaMode, SudoPath: cfg.Privileged.SudoPath}, reconcile.DefaultPool, logger), + Intent: intent, // slice 10 P3: record enroll/eject intent for self-heal + GuestBinds: guestBinds, // F9: per-guest bind record for the startup re-assert + FormatJobs: formatJobs, // F20-BUG3: detached-format job record + restart recovery // Host metrics (slice 9): the shared collector serves GET /host/metrics — a fresh host + // per-storage view to the customer's monitoring page (reuses the slice-4 collector). @@ -1756,6 +1785,15 @@ func runSelftestRead(ctx context.Context, cfg config.Config, logger *slog.Logger fmt.Printf(" - %d %q status=%s\n", g.VMID, g.Name, g.Status) } } + // A1 (v0.62.0): the stale-lock reaper's ownership registry — needs Pool.Audit (host-install v1.9.0+). + if p, err := client.Pool(ctx, reconcile.DefaultPool); report("pool read", err) { + fmt.Printf(" [ ok ] %-14s pool %q, %d member(s)\n", "pool read", p.PoolID, len(p.Members)) + for _, m := range p.Members { + if m.VMID != 0 && m.Type != "storage" { + fmt.Printf(" - %d type=%s\n", m.VMID, m.Type) + } + } + } if ss, err := client.NodeStorage(ctx); report("storage", err) { fmt.Printf(" [ ok ] %-14s %d store(s)\n", "storage", len(ss)) for _, s := range ss { diff --git a/internal/localapi/stalelock.go b/internal/localapi/stalelock.go index 1006a2f..64ed6b1 100644 --- a/internal/localapi/stalelock.go +++ b/internal/localapi/stalelock.go @@ -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) { diff --git a/internal/localapi/stalelock_pool_test.go b/internal/localapi/stalelock_pool_test.go new file mode 100644 index 0000000..4ed8096 --- /dev/null +++ b/internal/localapi/stalelock_pool_test.go @@ -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) + } +} diff --git a/internal/proxmox/query.go b/internal/proxmox/query.go index 0286893..432f20b 100644 --- a/internal/proxmox/query.go +++ b/internal/proxmox/query.go @@ -35,6 +35,15 @@ func (c *Client) ListLXC(ctx context.Context) ([]Guest, error) { return gs, c.get(ctx, "/nodes/"+c.node+"/lxc", &gs) } +// Pool returns GET /pools/{name} (the pool's membership — the stale-lock reaper's ownership +// registry, audit A1). Requires `Pool.Audit` at `/pool/{name}` — NOTE: `Pool.Allocate` does NOT +// satisfy the read (spike SPIKE-a1-pool-membership-read T2: the live 403 named Pool.Audit with +// Allocate already granted). Host-install v1.9.0+ grants it in the FelhomAgentGuest role. +func (c *Client) Pool(ctx context.Context, name string) (PoolInfo, error) { + var p PoolInfo + return p, c.get(ctx, "/pools/"+url.PathEscape(name), &p) +} + // GuestStatus returns GET /nodes/{node}/lxc/{vmid}/status/current. The API body // has no vmid field (it is in the path), so it is set from the argument. func (c *Client) GuestStatus(ctx context.Context, vmid int) (Guest, error) { diff --git a/internal/proxmox/types.go b/internal/proxmox/types.go index 6914acf..cd7b187 100644 --- a/internal/proxmox/types.go +++ b/internal/proxmox/types.go @@ -76,6 +76,21 @@ type Guest struct { Uptime int64 `json:"uptime"` } +// PoolInfo is GET /pools/{poolid} — the pool's identity + membership. The stale-lock recovery +// uses it as the ownership registry: only pool members are ever scanned (audit A1). +type PoolInfo struct { + PoolID string `json:"poolid"` + Members []PoolMember `json:"members"` +} + +// PoolMember is one entry of PoolInfo.Members. A pool can hold guests AND storages; storage +// entries carry type "storage" and no vmid, so membership checks must filter on both (spike +// SPIKE-a1-pool-membership-read §8). +type PoolMember struct { + VMID int `json:"vmid"` + Type string `json:"type"` // "lxc" | "qemu" | "storage" +} + // GuestConfig is GET /nodes/{node}/lxc/{vmid}/config. The config surface is // dynamic (net0..netN, mp0..mpN, unusedN), so known fields are typed and the full // raw map is preserved in Extra for the dynamic ones.