diff --git a/CHANGELOG.md b/CHANGELOG.md index 2616758..d8ee2d8 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,3 +1,51 @@ +## v0.123.0 — a tier the box cannot READ now says so (2026-08-03, R-185) + +**The missing permission is one command. The silence was the defect.** On demo-felhom the agent's PVE +token held `FelhomAgentStore` on `local`, `local-lvm` and `felhom-pbs` — and **not** on +`felhom-backup`, the storage the same installer had configured as `local_backup_target`. Asked for +that storage's content the API answers `{"data":[]}` while root sees three archives (6.1–6.3 GB, +08-01/02/03). + +**An empty listing is what a FORBIDDEN tier and a NEWBORN tier both return.** `pickForThisRun` skips +an empty tier — correctly, because a fresh offsite tier legitimately has nothing yet — and reports +*"no settled archive yet"*. So that tier was never restore-testable on that box and nothing ever +mentioned it. This project's own rule, in a new place: an empty answer is not evidence that there is +nothing there. + +**The permission question, unlike the listing, has a definite answer — so it is asked directly.** +`Client.Permissions` reads `GET /access/permissions?path=/storage/` **as the agent's own +token** (asking as root answers a different question and always says yes), and one +`capability.Status` per configured tier reports the result. It composes *around* the sudo prober, the +way the pool-read check already does — an API read does not belong inside a sudo-policy probe. + +**MEASURED FIRST, and the obvious reading is wrong.** The ungranted path does not answer empty and +does not 403: + +``` +/storage/felhom-pbs → {"Datastore.Allocate":1,"Datastore.AllocateSpace":1} +/storage/felhom-backup → {"Sys.Audit":1,"SDN.Use":1,"Datastore.Audit":1} +``` + +It answers with the privileges **inherited** from the box-wide `/` grant. A probe asking *"is the +path present?"* or *"does it have `Datastore.Audit`?"* would report the blinded storage healthy — so +the probe tests for `Datastore.AllocateSpace` specifically, and a red-proof pins that. + +**Decisions, each weighed once:** + +- **The probed set comes from the box's own config** (`BackupTiers()`), not a fixed list. A hardcoded + probe list is exactly the defect being fixed, reproduced inside the fix. +- **CRITICAL**, because the hub alerts only on critical and a non-critical entry would ride the + report and alert nobody — the same silence with extra steps. **Except** the `local` fallback + target, which host-install's own comment calls the DEGRADED configuration: it is still probed and + still reported, but it does not page, because turning an ordinary documented setup into an alert + is how a signal becomes something an operator archives unread. +- **It never looks at content**, so it cannot alarm on a newborn tier by construction. +- **It never reports ok when it could not ask.** An unreachable PVE is degraded: a self-check that + fails open converts *"I do not know"* into *"fine"*. + +The wire shape (`capability.Status`) is unchanged, so the hub's existing critical-degraded alert +applies with no hub change and no hub bump. + ## v0.122.0 — three ways the signals lied about themselves (2026-08-03, R-189 · R-188 · R-186) All three are the reporting and release path misreporting its own work. **No customer machine, no diff --git a/cmd/felhom-agent/main.go b/cmd/felhom-agent/main.go index 94fe5ac..281d6cf 100644 --- a/cmd/felhom-agent/main.go +++ b/cmd/felhom-agent/main.go @@ -411,6 +411,110 @@ func poolReadStatus(ctx context.Context, px *proxmox.Client) capability.Status { return s } +// storeGrantStatuses probes whether the agent's OWN TOKEN may read the storages this box depends +// on — one capability.Status per configured backup tier (R-185). +// +// ── WHY THIS EXISTS, AND WHY IT IS NOT A CONTENT LISTING ───────────────────────────────────── +// +// On demo-felhom the token had FelhomAgentStore on local, local-lvm and felhom-pbs — and NOT on +// `felhom-backup`, the storage the same installer had configured as `local_backup_target`. Asked +// for that storage's content the API answers `{"data":[]}` while root sees three archives. +// +// **An empty listing is what a FORBIDDEN tier and a NEWBORN tier both return**, and no care at that +// call site can separate them: `pickForThisRun` skips an empty tier (correctly — a fresh offsite +// tier legitimately has nothing) and says "no settled archive yet". So the host tier on that box was +// never restore-testable and nothing ever mentioned it. That is this project's own rule failing in a +// new place: an empty answer is not evidence that there is nothing there. +// +// The permission question, unlike the listing, has a DEFINITE answer — so it is asked directly. +// +// ── WHAT IS PROBED, AND WHY NOT A FIXED LIST ───────────────────────────────────────────────── +// +// The tiers come from this box's own config (`BackupTiers()`), because a hardcoded probe list is +// precisely the defect being fixed — the installer's hardcoded ACL set is what drifted from the +// target it went on to configure. Probing what the box says it depends on cannot drift from it. +// +// CRITICAL, deliberately: a tier the agent cannot read is a tier whose backups are invisible to it +// and which is never restore-tested. The hub alerts only on Critical, and a non-critical entry here +// would ride the report and alert nobody — the same silence with extra steps. +// +// One exception, so an ordinary configuration is not turned into an alarm: a box with no dedicated +// target (`local_backup_target: "local"`, which host-install's own comment calls the DEGRADED +// fallback) is not treated as critical for that tier — see storeGrantCritical. +func storeGrantStatuses(ctx context.Context, px *proxmox.Client, cfg config.Config) []capability.Status { + tiers, _ := cfg.Backup.BackupTiers() // warnings are logged where the tiers are armed + out := make([]capability.Status, 0, len(tiers)) + for _, t := range tiers { + out = append(out, storeGrantStatus(ctx, px, t.TargetID, storeGrantCritical(t.TargetID))) + } + return out +} + +// storeGrantRequiredPriv is the privilege whose ABSENCE was measured to blind the content listing. +// +// Measured on demo-felhom 2026-08-03: the two storages that list through the token hold +// Datastore.Allocate + Datastore.AllocateSpace (the FelhomAgentStore role); the one that answers +// empty holds only what the box-wide grant propagates (Sys.Audit, SDN.Use, Datastore.Audit). It is +// NOT Datastore.Audit that is missing — checking for that would report the blinded storage healthy. +const storeGrantRequiredPriv = "Datastore.AllocateSpace" + +// storeGrantCritical decides whether a missing grant on this target is Critical (operator-paged). +// +// "local" is host-install's DEGRADED fallback target — a box with no dedicated backup storage is a +// known, ordinary configuration, and turning it into a critical alert is how a signal becomes +// something an operator archives unread. It is still probed and still reported; only the paging +// differs. +func storeGrantCritical(targetID string) bool { return targetID != "local" } + +// storeGrantStatus is one tier's grant probe. It NEVER reports ok when it could not ask: a +// self-check that fails open is worse than none, because it converts "I do not know" into "fine". +func storeGrantStatus(ctx context.Context, px *proxmox.Client, targetID string, critical bool) capability.Status { + s := capability.Status{ + Name: "pve:store-grant:" + targetID, + Feature: "backup tier " + targetID + " readable by the agent (archive listing, restore-test candidacy)", + Critical: critical, + Status: capability.StatusOK, + } + if px == nil { + s.Status, s.Reason = capability.StatusDegraded, "not configured" + return s + } + if targetID == "" { + s.Status, s.Reason = capability.StatusDegraded, "tier has no target id" + return s + } + pctx, cancel := context.WithTimeout(ctx, 10*time.Second) + defer cancel() + privs, err := px.Permissions(pctx, "/storage/"+targetID) + return storeGrantVerdict(targetID, critical, privs, err) +} + +// storeGrantVerdict is the DECISION, split out from the API call so the tests exercise the real +// thing rather than a copy of it. A test that re-implements this branch would pass while production +// diverged — which is the hollow shape this project keeps finding in its own tests. +func storeGrantVerdict(targetID string, critical bool, privs map[string]int, err error) capability.Status { + s := capability.Status{ + Name: "pve:store-grant:" + targetID, + Feature: "backup tier " + targetID + " readable by the agent (archive listing, restore-test candidacy)", + Critical: critical, + Status: capability.StatusOK, + } + if err != nil { + // Unreachable PVE is UNKNOWN, and unknown is reported as degraded rather than ok: a + // self-check that fails open converts "I do not know" into "fine". + s.Status, s.Reason = capability.StatusDegraded, "could not read own permissions: "+err.Error() + return s + } + if privs[storeGrantRequiredPriv] != 1 { + // Name the storage AND the missing role: "a storage grant is missing" without saying which + // one costs a diagnosis at 07:00. + s.Status, s.Reason = capability.StatusDegraded, + "the agent token lacks "+storeGrantRequiredPriv+" on /storage/"+targetID+ + " (grant FelhomAgentStore there) — this tier's archives are INVISIBLE to the agent and it is never restore-tested" + } + 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). Inactive (config-gated off, plumbing healthy — v0.86.0) is counted in the @@ -492,8 +596,11 @@ func runDaemon(cfg config.Config, logger *slog.Logger, logRing *applog.Ring) int // 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. + // R-185: the store-grant probes compose around the sudo prober the same way the pool read does + // (an API read does not belong inside the sudo-policy probe — the v0.62.0 A1 precedent). probeAll := func(ctx context.Context) []capability.Status { - return append(capProber.Probe(ctx), poolReadStatus(ctx, px)) + out := append(capProber.Probe(ctx), poolReadStatus(ctx, px)) + return append(out, storeGrantStatuses(ctx, px, cfg)...) } // (The startup self-check log runs AFTER the pbsdr manager is wired below, so its snapshot // already carries the gated view — v0.86.0.) diff --git a/cmd/felhom-agent/storegrant_test.go b/cmd/felhom-agent/storegrant_test.go new file mode 100644 index 0000000..3cc2fff --- /dev/null +++ b/cmd/felhom-agent/storegrant_test.go @@ -0,0 +1,166 @@ +package main + +import ( + "context" + "go/ast" + "strings" + "testing" + + "gitea.dooplex.hu/admin/felhom-agent/internal/capability" +) + +// R-185 — a tier the box cannot READ must say so. +// +// THE OBSERVATION (demo-felhom, 2026-08-03, reproduced at the start of this session): root lists +// three archives on `felhom-backup`; the agent's own token gets `{"data":[]}` from the same +// endpoint; and `local`, which has the grant, lists through that same token. The token is the +// variable, not the storage. +// +// The defect is NOT the missing grant — that is one command. It is that an empty content listing is +// what a FORBIDDEN tier and a NEWBORN tier both return, so the box could not tell them apart and +// said nothing. These tests pin the distinction. + +// permAnswer is the shape /access/permissions really returns, taken from the live measurement: +// an UNGRANTED path answers with the privileges inherited from the box-wide grant — NOT empty, and +// NOT a 403. +var ( + permGranted = map[string]int{"Datastore.Allocate": 1, "Datastore.AllocateSpace": 1} + permUngranted = map[string]int{"Sys.Audit": 1, "SDN.Use": 1, "Datastore.Audit": 1} +) + +// probeWith calls the PRODUCTION decision with a permissions answer. **Naming the seam:** everything +// below is true up to `storeGrantVerdict`; that the live call feeds it the real API answer is what +// Part 0's measurement established and what the live run on the box demonstrates. An earlier draft +// of this file re-implemented the branch here — it passed, and would have kept passing while +// production diverged, which is the hollow shape this project keeps catching in its own tests. +func probeWith(privs map[string]int, targetID string, critical bool) capability.Status { + return storeGrantVerdict(targetID, critical, privs, nil) +} + +// ── SCENARIO A — a forbidden storage is REPORTED, not passed over ──────────────────────────── +// +// COMPANION RED-PROOF (observed 2026-08-03): delete the store-grant probes from `probeAll` in +// main.go — i.e. restore `append(capProber.Probe(ctx), poolReadStatus(ctx, px))` — and +// TestMainWiresTheStoreGrantProbe fails with "main.go never calls storeGrantStatuses". That is +// today's behaviour on the live box: complete silence about a tier it cannot read. +func TestStoreGrant_ForbiddenStorageIsDegradedAndNamed(t *testing.T) { + s := probeWith(permUngranted, "felhom-backup", true) + + if s.Status != capability.StatusDegraded { + t.Fatalf("a storage the agent may not read must be DEGRADED, not %q — silence is the defect", s.Status) + } + if !s.Critical { + t.Fatal("it must be CRITICAL: the hub alerts only on critical, so a non-critical entry is the same silence with extra steps") + } + if !strings.Contains(s.Reason, "felhom-backup") { + t.Fatalf("the reason must NAME the storage — 'a grant is missing' costs a diagnosis at 07:00; got %q", s.Reason) + } + if !strings.Contains(s.Reason, "FelhomAgentStore") { + t.Fatalf("the reason must name the ROLE to grant, so the fix is in the alert; got %q", s.Reason) + } +} + +// THE TRAP THE LIVE MEASUREMENT CAUGHT, pinned so it cannot be re-introduced: the ungranted answer +// is not empty and not a 403 — it carries the INHERITED box-wide privileges. A probe that asked +// "did the path come back?" or "does it have Datastore.Audit?" would report the blinded storage +// healthy. +func TestStoreGrant_InheritedPrivilegesAreNotAGrant(t *testing.T) { + if len(permUngranted) == 0 { + t.Fatal("fixture wrong: the ungranted answer is NOT empty — that is the whole trap") + } + if permUngranted["Datastore.Audit"] != 1 { + t.Fatal("fixture wrong: the ungranted path DOES carry Datastore.Audit, inherited box-wide") + } + if s := probeWith(permUngranted, "felhom-backup", true); s.Status != capability.StatusDegraded { + t.Fatalf("checking for the wrong privilege reports a blinded storage healthy; got %q", s.Status) + } + // ...and the privilege actually checked is the one whose absence was measured to blind listing. + if storeGrantRequiredPriv != "Datastore.AllocateSpace" { + t.Fatalf("the probed privilege changed to %q — re-measure before trusting it", storeGrantRequiredPriv) + } +} + +// ── SCENARIO B — a newborn tier is still silent ────────────────────────────────────────────── +// +// A storage the agent IS allowed to read but which simply holds no archives yet is HEALTHY. The +// probe must not look at content at all, or every freshly provisioned box alarms and the signal dies. +// +// COMPANION RED-PROOF (observed): make the probe degrade on an empty content listing instead of on +// the permission — a granted-but-empty storage then reports degraded, i.e. every newborn box alarms. +func TestStoreGrant_GrantedButEmptyIsHealthy(t *testing.T) { + s := probeWith(permGranted, "felhom-pbs", true) + if s.Status != capability.StatusOK { + t.Fatalf("a readable tier is healthy whether or not it holds archives yet; got %q (%s)", s.Status, s.Reason) + } + if s.Reason != "" { + t.Fatalf("a healthy probe carries no reason; got %q", s.Reason) + } +} + +// ── SCENARIO C — the two states are distinguishable at a glance ────────────────────────────── +func TestStoreGrant_ForbiddenAndNewbornAreDistinguishable(t *testing.T) { + forbidden := probeWith(permUngranted, "felhom-backup", true) + newborn := probeWith(permGranted, "felhom-pbs", true) + + if forbidden.Status == newborn.Status { + t.Fatalf("the two states must differ — today both read as 'no settled archive yet'; got %q for both", forbidden.Status) + } + if forbidden.Name == newborn.Name { + t.Fatalf("each tier needs its own capability id, or one tier's fault hides another's; got %q twice", forbidden.Name) + } +} + +// §8.3, weighed once and pinned: a box with NO dedicated target ("local" — host-install's own +// DEGRADED fallback) must not turn an ordinary configuration into an operator page. It is still +// probed and still reported; only the paging differs. +func TestStoreGrant_TheFallbackTargetIsNotCritical(t *testing.T) { + if storeGrantCritical("local") { + t.Fatal("a box whose backup target is the 'local' fallback must not page the operator about " + + "an ordinary, documented configuration") + } + for _, dedicated := range []string{"felhom-backup", "felhom-pbs", "some-nvme"} { + if !storeGrantCritical(dedicated) { + t.Fatalf("a DEDICATED target that cannot be read is user-facing and must be critical; %q was not", dedicated) + } + } + // The fallback is still reported — silence for it would be the original defect, scoped smaller. + if s := probeWith(permUngranted, "local", storeGrantCritical("local")); s.Status != capability.StatusDegraded { + t.Fatalf("the fallback target must still report degraded when unreadable; got %q", s.Status) + } +} + +// A probe that cannot ask must never answer "ok" — unknown reported as healthy is worse than no +// probe, because it looks like coverage. +func TestStoreGrant_UnreachablePVEIsDegradedNotOK(t *testing.T) { + s := storeGrantStatus(context.Background(), nil, "felhom-backup", true) + if s.Status != capability.StatusDegraded { + t.Fatalf("an unaskable probe must be DEGRADED, never ok; got %q", s.Status) + } + if s.Reason == "" { + t.Fatal("it must say why it could not ask") + } +} + +// ── SCENARIO H — the seam ──────────────────────────────────────────────────────────────────── +// +// This project's "built but never wired" count reached six last week. The fix for a SILENCE must not +// itself be silent. AST, not grep: a commented-out call still contains the string. +func TestMainWiresTheStoreGrantProbe(t *testing.T) { + f := parseMainForWiring(t) + + var wired bool + ast.Inspect(f, func(n ast.Node) bool { + call, ok := n.(*ast.CallExpr) + if !ok { + return true + } + if id, ok := call.Fun.(*ast.Ident); ok && id.Name == "storeGrantStatuses" { + wired = true + } + return true + }) + if !wired { + t.Error("main.go never calls storeGrantStatuses — the probe would exist and report to nobody, " + + "which is precisely the silence R-185 is about") + } +} diff --git a/internal/proxmox/query.go b/internal/proxmox/query.go index bcd9fd7..fdad272 100644 --- a/internal/proxmox/query.go +++ b/internal/proxmox/query.go @@ -45,6 +45,35 @@ func (c *Client) Pool(ctx context.Context, name string) (PoolInfo, error) { return p, c.get(ctx, "/pools/"+url.PathEscape(name), &p) } +// Permissions returns the privileges this API TOKEN holds at an ACL path, as +// GET /access/permissions?path= answers it: privilege name → 1. +// +// R-185. It asks about the CALLER — the agent's own token — which is the only useful form of the +// question. Asking as root answers a different question and always says yes. +// +// MEASURED SHAPE (demo-felhom, 2026-08-03), because the whole value of this call is reading the +// answer correctly and the obvious reading is wrong: +// +// /storage/felhom-pbs → {"Datastore.Allocate":1,"Datastore.AllocateSpace":1} +// /storage/felhom-backup → {"Sys.Audit":1,"SDN.Use":1,"Datastore.Audit":1} +// +// The ungranted path does NOT answer empty, and does NOT 403. It answers with the privileges +// INHERITED from the box-wide `/` grant — so "is this path present in the response" reports OK for a +// storage the agent demonstrably cannot list. The caller must test for the SPECIFIC privilege. +// +// The response is keyed by path; an absent path yields no privileges, which is the same answer as +// "none" and is treated as such by the caller. +func (c *Client) Permissions(ctx context.Context, aclPath string) (map[string]int, error) { + var raw map[string]map[string]int + if err := c.get(ctx, "/access/permissions?path="+url.QueryEscape(aclPath), &raw); err != nil { + return nil, err + } + if p, ok := raw[aclPath]; ok { + return p, nil + } + return map[string]int{}, nil +} + // 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) {