diff --git a/CHANGELOG.md b/CHANGELOG.md index 029f73d..d647b32 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,3 +1,40 @@ +## v0.128.0 — the escrow seed is asserted every tick, not remembered once (2026-08-08, R-221) + +**A rebuilt box could not run the escrow ceremony at all, and there was no way forward from inside +the product.** The preflight refuses on `escrow.pbs_storage_id`; the pbsdr bridge writes that key; +and it wrote it from exactly one place — `finishConverged`, reached only on the paths that actually +converge. + +**The two things live in different places and die at different times.** The convergence marker is +host-side (`/pbsdr/marker.json`). The key it seeds is in `agent.json`, which +`step_agent_config` renders from `base = {}` unless an explicit `--preserve-from` is passed +(`felhom.eu/scripts/felhom-host-install.sh:2396`, the render at `:2449`, the `O_TRUNC` write at +`:2579`; the flag at `:1246`, defaulting empty at `:256`) — **and the render never writes an +`escrow` section at all.** So a rebuild keeps the marker and takes the key: same descriptor, same +hash, early return, and the seed never runs again into a config that no longer has it. + +**Fixed by asserting rather than remembering.** `Apply` now re-asserts the seed *before* the +idempotent early return. `seedEscrowStorageID` is unchanged and still never clobbers a different +existing value — an operator's own choice outranks the descriptor's, with a warning naming both. + +**The early return is KEPT.** It exists so a converged box does not re-run Proxmox operations every +60 s, and `TestSeedReasserted_OnConvergedTick_WithZeroProxmoxCalls` asserts **zero** recorded runner +calls on that tick — so a "fix" that simply deleted the return would fail. Cost of the re-assert: one +small file read plus a JSON parse per tick, no exec, no network, and an early return once the value +matches. + +**A seed failure can never un-converge the box:** Warn plus a message on the published status, +exactly as `finishConverged` does it — no marker write, no state change. Pinned by +`TestSeedReassertFailure_DoesNotUnconverge`. + +Tests drive the real `Apply` with a real temp-dir `agent.json` and a call-recording runner; calling +`seedEscrowStorageID` directly cannot see the early return, which IS the defect. The production +wiring (`pbsdr.NewManager(..., cfg.SourcePath, ...)`) is asserted by walking `main.go`'s **AST**, not +by `strings.Contains`, which a commented-out call also satisfies. + +Red-proofs: removing the new call makes Scenario A fail on today's tree; removing the early return +makes the zero-Proxmox-calls assertion fail. + ## (no version bump) — a comment that claimed the hub reads a field it has no field for (2026-08-08, R-260) Comment-only; no behaviour, no wire change, nothing to rebuild. diff --git a/internal/pbsdr/manager.go b/internal/pbsdr/manager.go index 650b850..657966a 100644 --- a/internal/pbsdr/manager.go +++ b/internal/pbsdr/manager.go @@ -283,7 +283,36 @@ func (m *Manager) Apply(ctx context.Context, fetched bool, block *hub.WirePBSDR) h := descriptorHash(block) cf := m.loadConsumedFailed() if mk := m.loadMarker(); mk != nil && mk.Hash == h && (cf == nil || cf.Hash != h) { - m.setStatus(&hub.PBSDRStatus{State: mk.State, StorageID: block.StorageID, Namespace: block.Namespace, AppliedAt: mk.AppliedAt}) + // R-221: RE-ASSERT THE SEED, DO NOT REMEMBER IT. The marker records that this descriptor + // converged; it says nothing about whether the file the seed writes still exists. + // + // The two live in different places and die at different times. The marker is host-side + // (`/pbsdr/`, markerPath above); the seed's target is `agent.json`, and the + // installer's `step_agent_config` renders that file from `base = {}` unless an explicit + // `--preserve-from` is passed — it NEVER writes an `escrow` section — then replaces it with + // O_TRUNC (felhom-host-install.sh:2396, :2449, :2579; the flag is :1246, defaulting empty at + // :256). So a rebuild leaves the marker and takes the seed, the hash still matches, this + // branch returns, and `escrow.pbs_storage_id` is never written again. The customer then + // cannot run the escrow ceremony AT ALL: handleEscrowPreflight fails the `pbs_storage_id` + // row and the wizard refuses, with no way forward from inside the product. + // + // A rebuild is only the case that was measured. The same hole opens for a hand-edited or + // restored config, which is the honest reason this is a seam fix rather than an installer + // fix — the seed must be a thing the loop asserts, not a thing it did once. + // + // COST: this runs on the converged path, i.e. every tick (60 s) forever. It is one small + // file read plus a JSON parse — no exec, no network, no Proxmox call — and seedEscrowStorageID + // returns early once the value matches. That is the whole reason it is affordable here. + // + // IT MUST NEVER UN-CONVERGE THE BOX: a failure is a Warn plus a message on the published + // status, exactly as finishConverged does it. No marker write, no state change, no retry + // storm — the early return below still happens either way. + msg := "" + if err := m.seedEscrowStorageID(block.StorageID); err != nil { + msg = "escrow.pbs_storage_id seed failed: " + err.Error() + " (set it manually before the ceremony)" + m.logger.Warn("pbsdr: " + msg) + } + m.setStatus(&hub.PBSDRStatus{State: mk.State, StorageID: block.StorageID, Namespace: block.Namespace, AppliedAt: mk.AppliedAt, Message: msg}) return // idempotent: this exact descriptor already converged } diff --git a/internal/pbsdr/seed_reassert_test.go b/internal/pbsdr/seed_reassert_test.go new file mode 100644 index 0000000..b7bd4ba --- /dev/null +++ b/internal/pbsdr/seed_reassert_test.go @@ -0,0 +1,232 @@ +package pbsdr + +import ( + "context" + "encoding/json" + "go/ast" + "go/parser" + "go/printer" + "go/token" + "io" + "os" + "path/filepath" + "strings" + "testing" + + "gitea.dooplex.hu/admin/felhom-agent/internal/hub" +) + +// R-221 — the escrow seed must be ASSERTED on every converged tick, not remembered. +// +// These tests drive the real Apply() with a real temp-dir agent.json and a call-recording runner. +// Calling seedEscrowStorageID directly would prove nothing: the defect IS the early return in +// Apply, and a test that steps around it cannot see it. + +// convergedMarker writes a marker whose hash matches the block, i.e. puts the manager on exactly +// the idempotent path where the seed used to be skipped. +func convergedMarker(t *testing.T, m *Manager, block *hub.WirePBSDR, state string) { + t.Helper() + if err := m.writeState(m.markerPath(), marker{ + Hash: descriptorHash(block), State: state, AppliedAt: "2026-08-08T00:00:00Z", + }); err != nil { + t.Fatalf("write marker: %v", err) + } +} + +func escrowStorageID(t *testing.T, cfgPath string) string { + t.Helper() + raw, err := os.ReadFile(cfgPath) + if err != nil { + t.Fatalf("read config: %v", err) + } + var doc struct { + Escrow struct { + PBSStorageID string `json:"pbs_storage_id"` + } `json:"escrow"` + } + if err := json.Unmarshal(raw, &doc); err != nil { + t.Fatalf("parse config: %v", err) + } + return doc.Escrow.PBSStorageID +} + +// drBlock mirrors the existing valid fixture (manager_test.go descriptor()) so that validate() +// passes and Apply actually reaches the marker check — the branch these tests are about. +func drBlock(storageID string) *hub.WirePBSDR { + return &hub.WirePBSDR{ + Enabled: true, StorageID: storageID, PBSTunnelIP: "10.77.0.1", + Datastore: "felhom-offsite", Namespace: "peti", TokenID: "felhom@pbs!peti", + Fingerprint: testFP, + } +} + +// SCENARIO A — the seed is re-asserted on a converged box, and nothing else happens. +// +// THIS IS THE TEST THAT MATTERS. It must fail against the pre-R-221 tree; if it passes there, it is +// not testing the defect and that is the finding. +func TestSeedReasserted_OnConvergedTick_WithZeroProxmoxCalls(t *testing.T) { + r := &fakeRunner{} + st := &fakeStorage{found: true, active: []bool{true}} + c := &fakeConsumer{} + m, cfgPath := newTestManager(t, r, st, c) + + block := drBlock("felhom-pbs-dr") + convergedMarker(t, m, block, "applied") + + // the shape a rebuild leaves behind: escrow section present, pbs_storage_id GONE + if got := escrowStorageID(t, cfgPath); got != "" { + t.Fatalf("precondition: config already carries a storage id %q", got) + } + + m.Apply(context.Background(), true, block) + + if got := escrowStorageID(t, cfgPath); got != "felhom-pbs-dr" { + t.Errorf("the converged tick did not re-assert the seed: escrow.pbs_storage_id = %q, want %q.\n"+ + "This is R-221: the marker survives a rebuild, the descriptor hash still matches, the early "+ + "return fires and the seed never runs into the config that no longer has it — so the customer "+ + "cannot run the escrow ceremony at all.", got, "felhom-pbs-dr") + } + + // ...and the idempotent path is STILL idempotent. This assertion is not decorative: without it + // a "fix" that simply deletes the early return would pass the line above. + if calls := r.recorded(); len(calls) != 0 { + t.Errorf("a converged tick must execute ZERO Proxmox commands; got %d: %+v", len(calls), calls) + } +} + +// SCENARIO B — an operator's own different value survives, and the warning names both. +func TestSeedReasserted_NeverClobbersAnOperatorValue(t *testing.T) { + r := &fakeRunner{} + m, cfgPath := newTestManager(t, r, &fakeStorage{found: true, active: []bool{true}}, &fakeConsumer{}) + + if err := os.WriteFile(cfgPath, []byte( + `{"log_level":"info","escrow":{"posture":"zero_knowledge","pbs_storage_id":"operator-chosen"},`+ + `"custom_unknown":{"keep":1}}`), 0o600); err != nil { + t.Fatal(err) + } + block := drBlock("hub-chosen") + convergedMarker(t, m, block, "applied") + + m.Apply(context.Background(), true, block) + + if got := escrowStorageID(t, cfgPath); got != "operator-chosen" { + t.Errorf("a value a person put there was overwritten by the descriptor: got %q, want %q", got, "operator-chosen") + } + // unknown keys must still round-trip + raw, _ := os.ReadFile(cfgPath) + if !strings.Contains(string(raw), "custom_unknown") { + t.Error("an unknown config key was dropped by the re-assert") + } +} + +// SCENARIO C — the ceremony preflight's live read sees the re-asserted value with NO restart. +// +// The preflight itself lives in internal/localapi and reads the file through config.Load; what this +// asserts is the half that belongs to this package: after a converged tick, THE FILE ON DISK carries +// the id, so any live re-read is green. The daemon is never restarted in this test because it is +// never started — which is the point. +func TestSeedReasserted_IsVisibleOnDiskImmediately(t *testing.T) { + r := &fakeRunner{} + m, cfgPath := newTestManager(t, r, &fakeStorage{found: true, active: []bool{true}}, &fakeConsumer{}) + block := drBlock("felhom-pbs-dr") + convergedMarker(t, m, block, "applied") + + // preflight's predicate BEFORE: storageID == "" → the row is NOT OK + if escrowStorageID(t, cfgPath) != "" { + t.Fatal("precondition") + } + m.Apply(context.Background(), true, block) + // preflight's predicate AFTER, from the same file the ceremony subprocess loads + if id := escrowStorageID(t, cfgPath); id == "" { + t.Error("the preflight row would still be NOT OK after a converged tick") + } +} + +// A seed failure must NEVER un-converge the box: no marker rewrite, no state change, and the status +// still reports the marker's converged state — with the failure surfaced as a message. +func TestSeedReassertFailure_DoesNotUnconverge(t *testing.T) { + r := &fakeRunner{} + m, cfgPath := newTestManager(t, r, &fakeStorage{found: true, active: []bool{true}}, &fakeConsumer{}) + block := drBlock("felhom-pbs-dr") + convergedMarker(t, m, block, "applied") + markerBefore, err := os.ReadFile(m.markerPath()) + if err != nil { + t.Fatal(err) + } + + // make the seed fail in a way it cannot recover from: unparseable config + if err := os.WriteFile(cfgPath, []byte(`{ this is not json`), 0o600); err != nil { + t.Fatal(err) + } + + m.Apply(context.Background(), true, block) + + after, err := os.ReadFile(m.markerPath()) + if err != nil { + t.Fatalf("the marker was removed by a seed failure: %v", err) + } + if string(after) != string(markerBefore) { + t.Error("a seed failure rewrote the convergence marker — it must not touch state") + } + if calls := r.recorded(); len(calls) != 0 { + t.Errorf("a seed failure must not trigger Proxmox work; got %+v", calls) + } + st := m.Status() + if st == nil || st.State != "applied" { + t.Errorf("a seed failure must leave the box converged; status = %+v", st) + } + if st != nil && !strings.Contains(st.Message, "seed failed") { + t.Errorf("a seed failure must be surfaced on the status, got message %q", st.Message) + } +} + +// SEAM WIRING — production must construct the manager with the live config path, or the whole seed +// leg is inert. Three shipped defects in this project were fully green while their seam was never +// wired, so this walks main.go's AST for the actual call rather than grepping: a commented-out call +// satisfies strings.Contains, and an AST walk cannot see a comment. +func TestProductionWiring_NewManagerGetsTheLiveConfigPath(t *testing.T) { + path := filepath.Join("..", "..", "cmd", "felhom-agent", "main.go") + fset := token.NewFileSet() + f, err := parser.ParseFile(fset, path, nil, 0) // comments not even collected + if err != nil { + t.Fatalf("parse main.go: %v", err) + } + found := false + ast.Inspect(f, func(n ast.Node) bool { + call, ok := n.(*ast.CallExpr) + if !ok { + return true + } + sel, ok := call.Fun.(*ast.SelectorExpr) + if !ok || sel.Sel.Name != "NewManager" { + return true + } + if pkg, ok := sel.X.(*ast.Ident); !ok || pkg.Name != "pbsdr" { + return true + } + // signature: (runner, px, hubc, stateDir, secretDir, configPath, logger) + if len(call.Args) < 6 { + t.Errorf("pbsdr.NewManager called with %d args, expected >= 6", len(call.Args)) + return false + } + var buf strings.Builder + if err := printNode(&buf, fset, call.Args[5]); err != nil { + t.Fatalf("print arg: %v", err) + } + got := buf.String() + if !strings.Contains(got, "SourcePath") { + t.Errorf("pbsdr.NewManager's configPath argument is %q, which is not the live config path.\n"+ + "With an empty or wrong path seedEscrowStorageID returns nil immediately and the entire "+ + "R-221 fix is inert while every test above still passes.", got) + } + found = true + return false + }) + if !found { + t.Error("no pbsdr.NewManager call found in main.go — the manager is not constructed in production") + } +} + +func printNode(w io.Writer, fset *token.FileSet, n ast.Node) error { + return printer.Fprint(w, fset, n) +}