From 73b6dbc27d7d4b99e697bf6eb690759232fc0508 Mon Sep 17 00:00:00 2001 From: kisfenyo Date: Wed, 5 Aug 2026 07:17:13 +0200 Subject: [PATCH] R-204 item 1: a freshly minted reset code works without a restart (v0.198.0) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --print-reset-code runs as a separate process and persists the new code; the running server's cache was never told, so the code the customer was told to type was refused until the controller restarted. Nothing said so — during the 2026-08-04 drill that cost two attempts with an operator present. effectiveClaimCode now reads through to the persisted state before applying the settings-vs-config precedence, which is itself unchanged. Read-through, not a TTL: a TTL would leave a window in which a superseded code still works, which is worse than the bug. Fails closed on an unreadable state; an absent file is not an error. --- controller/internal/settings/settings.go | 52 +++++ controller/internal/web/claim.go | 51 ++++- .../internal/web/claim_readthrough_test.go | 211 ++++++++++++++++++ 3 files changed, 307 insertions(+), 7 deletions(-) create mode 100644 controller/internal/web/claim_readthrough_test.go diff --git a/controller/internal/settings/settings.go b/controller/internal/settings/settings.go index 5ea3108..17a1f5d 100644 --- a/controller/internal/settings/settings.go +++ b/controller/internal/settings/settings.go @@ -628,6 +628,58 @@ func (s *Settings) GetClaimCode() (hash string, generation int, issuedAt string) return s.ClaimCodeHash, s.ClaimCodeGeneration, s.ClaimCodeIssuedAt } +// ReloadClaimCode re-reads the PERSISTED claim-code state from settings.json into the in-process +// cache, so a code minted by ANOTHER PROCESS is visible without restarting this one. +// +// WHY THIS EXISTS (R-204 item 1, v0.198.0). `--print-reset-code` runs as a separate process +// (`docker exec`): it loads settings itself, mints a code, persists it and exits. The running +// server's cache never heard, so it kept validating against the previous hash and the code the +// customer was told to type was refused until the controller restarted. Nothing said so. During the +// 2026-08-04 recovery drill that cost two failed attempts with an operator present; a customer alone +// stops there. THE READ IS THE FIX — it is not a cache refresh for tidiness. +// +// READ-THROUGH, NOT A WATCHER AND NOT A TTL, deliberately (R-204 §8.1). A watcher/signal/background +// reloader is a new failure mode for one stale read. A TTL is worse than the bug: it opens a window +// in which a SUPERSEDED code still works. Pinned by +// web.TestClaimCode_SupersededByASecondMint_RefusedImmediately, whose stated red-proof is exactly +// that TTL. The read happens on the claim path only — see web.effectiveClaimCode — and only while the +// box carries no password, i.e. exactly the gate window. +// +// WHAT IT REFRESHES AND WHAT IT DELIBERATELY DOES NOT: hash/generation/issuedAt only. +// ClaimConsumedGeneration is NOT re-read. This process is its only writer and its in-memory value is +// monotonic; re-reading it could move it BACKWARDS if a save had failed, which would resurrect an +// already-consumed code — the exact widening this fix is not allowed to introduce. +// +// An ABSENT file is not an error: a box before its first save legitimately has no persisted state and +// falls back to the controller.yaml bake. A present-but-unreadable or corrupt file IS an error, and +// the caller fails closed on it. +func (s *Settings) ReloadClaimCode() error { + if s.path == "" { + return nil // no persistence configured (tests) — the cache is all there is + } + data, err := os.ReadFile(s.path) + if err != nil { + if os.IsNotExist(err) { + return nil + } + return fmt.Errorf("reading persisted claim state: %w", err) + } + var onDisk struct { + ClaimCodeHash string `json:"claim_code_hash"` + ClaimCodeGeneration int `json:"claim_code_generation"` + ClaimCodeIssuedAt string `json:"claim_code_issued_at"` + } + if err := json.Unmarshal(data, &onDisk); err != nil { + return fmt.Errorf("parsing persisted claim state: %w", err) + } + s.mu.Lock() + defer s.mu.Unlock() + s.ClaimCodeHash = onDisk.ClaimCodeHash + s.ClaimCodeGeneration = onDisk.ClaimCodeGeneration + s.ClaimCodeIssuedAt = onDisk.ClaimCodeIssuedAt + return nil +} + // SetClaimCode caches a hub-delivered code state (idempotent by generation — the caller guards). func (s *Settings) SetClaimCode(hash string, generation int, issuedAt string) error { s.mu.Lock() diff --git a/controller/internal/web/claim.go b/controller/internal/web/claim.go index d4ac4c7..45374cf 100644 --- a/controller/internal/web/claim.go +++ b/controller/internal/web/claim.go @@ -43,19 +43,32 @@ type claimAttempt struct { // effectiveClaimCode returns the freshest hub-delivered claim-code state: the ACK-cached // settings value when its generation is at least the config-baked one (fresher), else the // controller.yaml bake. Returns ("", 0, "") when neither carries a code. -func (s *Server) effectiveClaimCode() (hash string, generation int, issuedAt string) { +// +// R-204 item 1 (v0.198.0): it now READS THROUGH to the persisted settings first, because +// `--print-reset-code` mints its code in a SEPARATE PROCESS and this one's cache never heard — so a +// freshly minted code was refused until the controller restarted, and nothing said so. The +// PRECEDENCE RULE BELOW IS UNCHANGED and deliberate (settings wins only at an equal-or-newer +// generation); the defect was the freshness of the settings value, not which source wins. +// +// The read-through is why the ERROR RETURN exists: a persisted state that cannot be read must FAIL +// CLOSED at every caller (an absent file is not an error — see settings.ReloadClaimCode). A gate that +// opens because it could not read its own state is the shape this project has removed four times. +func (s *Server) effectiveClaimCode() (hash string, generation int, issuedAt string, err error) { var sHash, sIssued string var sGen int if s.settings != nil { + if rerr := s.settings.ReloadClaimCode(); rerr != nil { + return "", 0, "", rerr + } sHash, sGen, sIssued = s.settings.GetClaimCode() } cHash := s.cfg.Web.ClaimCodeHash cGen := s.cfg.Web.ClaimCodeGeneration cIssued := s.cfg.Web.ClaimCodeIssuedAt if sHash != "" && sGen >= cGen { - return sHash, sGen, sIssued + return sHash, sGen, sIssued, nil } - return cHash, cGen, cIssued + return cHash, cGen, cIssued, nil } // claimGateActive reports whether the unclaimed-gate applies: no password set anywhere, a claim @@ -65,7 +78,13 @@ func (s *Server) claimGateActive() bool { if s.authEnabled() { return false // a password beats the gate (claimed boxes, or an operator-set one) } - hash, _, _ := s.effectiveClaimCode() + hash, _, _, err := s.effectiveClaimCode() + if err != nil { + // FAIL CLOSED. Unreadable claim state must not open the dashboard — keep the gate up. The + // claim page itself stays reachable (claimPageAllowedPath), so this is recoverable, not a brick. + s.logger.Printf("[ERROR] [web] claim: cannot read the persisted claim state — keeping the gate CLOSED: %v", err) + return true + } if hash == "" { return false // legacy-open (transition state) — no code to gate on } @@ -81,7 +100,10 @@ func (s *Server) claimLegacyOpen() bool { if s.authEnabled() { return false } - hash, _, _ := s.effectiveClaimCode() + hash, _, _, err := s.effectiveClaimCode() + if err != nil { + return false // FAIL CLOSED: an unreadable state is not evidence the box is legacy-open + } return hash == "" } @@ -244,7 +266,15 @@ func (s *Server) serveClaimGate(w http.ResponseWriter, r *http.Request) { // the strong factor. For a claimed box (password set) it doubles as the reset-code entry. func (s *Server) handleClaimPage(w http.ResponseWriter, r *http.Request, errorMsg, flashMsg string) { csrf := s.setClaimCSRFCookie(w, r) - hash, _, _ := s.effectiveClaimCode() + hash, _, _, cerr := s.effectiveClaimCode() + if cerr != nil { + // FAIL CLOSED on the page too: never invite a code we could not read the state for. + s.logger.Printf("[ERROR] [web] claim: cannot read the persisted claim state while rendering the claim page: %v", cerr) + hash = "" + if errorMsg == "" { + errorMsg = "A beállító állapot most nem olvasható — próbáld újra néhány perc múlva." + } + } reset := s.authEnabled() // a set password means this is the reset flow, not first-claim data := map[string]interface{}{ "Title": "A szerver beállítása", @@ -290,7 +320,14 @@ func (s *Server) handleClaimSubmit(w http.ResponseWriter, r *http.Request) { newPassword := r.FormValue("new_password") confirm := r.FormValue("confirm_password") - hash, generation, issuedAt := s.effectiveClaimCode() + hash, generation, issuedAt, cerr := s.effectiveClaimCode() + if cerr != nil { + // FAIL CLOSED: refuse the claim rather than validate against a possibly-superseded cache. + // NOT counted as a failed attempt — the customer typed nothing wrong. + s.logger.Printf("[ERROR] [web] claim: refusing the submission — the persisted claim state is unreadable: %v", cerr) + s.handleClaimPage(w, r, "A beállító állapot most nem olvasható — próbáld újra néhány perc múlva.", "") + return + } if hash == "" { s.handleClaimPage(w, r, "Nincs aktív kód — kérj újat az alábbi gombbal.", "") return diff --git a/controller/internal/web/claim_readthrough_test.go b/controller/internal/web/claim_readthrough_test.go new file mode 100644 index 0000000..3be38b8 --- /dev/null +++ b/controller/internal/web/claim_readthrough_test.go @@ -0,0 +1,211 @@ +package web + +import ( + "io" + "log" + "net/http" + "net/http/httptest" + "net/url" + "os" + "path/filepath" + "regexp" + "strings" + "testing" + + "gitea.dooplex.hu/admin/felhom-controller/internal/config" + "gitea.dooplex.hu/admin/felhom-controller/internal/settings" + "gitea.dooplex.hu/admin/felhom-controller/internal/stacks" +) + +// R-204 item 1 — a freshly minted reset code must work on the FIRST attempt, with nothing restarted. +// +// WHAT THESE TESTS REPRODUCE, AND WHY THE SEAM IS WHERE IT IS. `--print-reset-code` is a SEPARATE +// PROCESS (`docker exec`): it calls settings.Load on the same file, mints, persists and exits. The +// running server never hears. So the mint below runs against a SECOND settings.Settings loaded from +// the SAME path — that is the whole defect, and a test that reused the server's own *Settings would +// mint straight into the cache and prove nothing. The submission then goes through the FULL MUX +// (RequireAuth + CsrfProtect + ServeHTTP), i.e. the production path, not handleClaimSubmit directly. + +// claimReadThroughServer is claimTestServer's sibling: same wiring, but it installs NO code and +// returns the settings PATH, because these tests need a second process's view of that file. +func claimReadThroughServer(t *testing.T) (*Server, *config.Config, string) { + t.Helper() + lg := log.New(io.Discard, "", 0) + dir := t.TempDir() + cfg := &config.Config{} + cfg.Customer.ID = "c1" + cfg.Customer.Name = "Teszt" + cfg.Customer.Domain = "example.hu" + cfg.Paths.StacksDir = filepath.Join(dir, "stacks") + cfg.Paths.DataDir = filepath.Join(dir, "data") + cfg.Stacks.ComposeCommand = "docker compose" + cfg.Web.SessionSecret = "test-session-secret-abcdef" + + path := filepath.Join(dir, "settings.json") + sett, err := settings.Load(path, lg) + if err != nil { + t.Fatalf("settings: %v", err) + } + mgr, err := stacks.NewManager(cfg, lg) + if err != nil { + t.Fatalf("stacks: %v", err) + } + s := &Server{cfg: cfg, settings: sett, stackMgr: mgr, logger: lg, version: "test"} + s.loadTemplates() + return s, cfg, path +} + +var hatchCodeRe = regexp.MustCompile(`(?m)^\s{4}([a-z0-9]{4}-[a-z0-9]{4}-[a-z0-9]{4})\s*$`) + +// mintCodeViaHatch runs the REAL escape hatch exactly as `docker exec ... --print-reset-code` does: +// a fresh settings.Load on the same file (a separate process's cache), PrintLocalResetCode, exit. +// Returns the plaintext the operator would read off the terminal. +func mintCodeViaHatch(t *testing.T, path string, cfg *config.Config) string { + t.Helper() + other, err := settings.Load(path, log.New(io.Discard, "", 0)) + if err != nil { + t.Fatalf("hatch: settings.Load: %v", err) + } + r, w, perr := os.Pipe() + if perr != nil { + t.Fatalf("hatch: pipe: %v", perr) + } + saved := os.Stdout + os.Stdout = w + rc := PrintLocalResetCode(other, cfg) + os.Stdout = saved + w.Close() + out, _ := io.ReadAll(r) + r.Close() + if rc != 0 { + t.Fatalf("hatch: PrintLocalResetCode rc=%d, out=%q", rc, string(out)) + } + m := hatchCodeRe.FindStringSubmatch(string(out)) + if m == nil { + t.Fatalf("hatch: no code found in output %q", string(out)) + } + return m[1] +} + +// submitClaim POSTs the code + a new password through the FULL MUX. Returns the recorder. +func submitClaim(t *testing.T, s *Server, code, pw string) *httptest.ResponseRecorder { + t.Helper() + form := url.Values{"_csrf": {s.claimCSRFToken()}, "code": {code}, "new_password": {pw}, "confirm_password": {pw}} + req := httptest.NewRequest(http.MethodPost, "/claim", strings.NewReader(form.Encode())) + req.Header.Set("Content-Type", "application/x-www-form-urlencoded") + req.AddCookie(&http.Cookie{Name: claimCSRFCookie, Value: s.claimCSRFToken()}) + rr := httptest.NewRecorder() + s.fullMux().ServeHTTP(rr, req) + return rr +} + +// SCENARIO A — a code minted by the hatch is accepted immediately, with NOTHING restarted. +// +// RED-PROOF (task §10): delete the `s.settings.ReloadClaimCode()` call in effectiveClaimCode. The +// server then validates against its startup cache (empty here), the submission is refused with +// „Nincs aktív kód" and this test fails. That mutation removes the ONLY guard this test covers. +func TestClaimCode_FreshlyMintedByHatch_AcceptedWithoutRestart(t *testing.T) { + s, cfg, path := claimReadThroughServer(t) + + // Precondition: the server's cache is empty — this is the running process before the hatch. + if h, _, _, err := s.effectiveClaimCode(); err != nil || h != "" { + t.Fatalf("precondition: want no code and no error, got hash=%q err=%v", h, err) + } + + code := mintCodeViaHatch(t, path, cfg) + + // NOTHING is restarted and no reload is invoked by the test — the server must see it by itself. + rr := submitClaim(t, s, code, "a-strong-passphrase-12") + if rr.Code != http.StatusFound || rr.Header().Get("Location") != "/" { + t.Fatalf("freshly minted code refused: got %d loc=%q body=%q — the running server did not see the persisted code", + rr.Code, rr.Header().Get("Location"), claimFirstLine(rr.Body.String())) + } + // Assert the EFFECT, not the absence of an error: the box is claimed and the password is live. + if !s.settings.GetClaimed() { + t.Fatal("box not marked claimed after accepting the freshly minted code") + } + if !s.authEnabled() { + t.Fatal("password not set after accepting the freshly minted code") + } +} + +// SCENARIO B — the MOMENT a second code is minted, the first one stops working. +// +// This is the guard that matters: the plausible wrong fix is a TTL/interval cache, which would make +// the new code visible but leave a window in which the SUPERSEDED one still works — worse than the +// bug being fixed (task §8.1). +// +// RED-PROOF: substitute a TTL cache for the read-through (reload only when a stored deadline has +// passed, e.g. 30s). Within the window the server keeps the first hash AND the first generation, so +// the first code VERIFIES and „the SUPERSEDED code was accepted" fires. +// +// THE SETUP IS LOAD-BEARING, not ceremony: the server must have ALREADY SEEN the first code before +// the second is minted. Without that step the TTL mutation leaves the cache empty, the first code is +// refused for having no code at all, and the test would fail for the wrong reason — which is the +// red-proof-that-passes-for-the-wrong-reason trap (task §10). The seeing is done through +// claimGateActive(), the production path a customer's page load takes, not a test-only reload. +func TestClaimCode_SupersededByASecondMint_RefusedImmediately(t *testing.T) { + s, cfg, path := claimReadThroughServer(t) + + first := mintCodeViaHatch(t, path, cfg) + if !s.claimGateActive() { + t.Fatal("the gate is not active after the first mint — the server never saw the first code") + } + if h, _, _, _ := s.effectiveClaimCode(); h == "" { + t.Fatal("precondition: the server must hold the FIRST code before the second is minted") + } + second := mintCodeViaHatch(t, path, cfg) + if first == second { + t.Fatal("the two mints produced the same code — the test cannot distinguish them") + } + + // The FIRST code must be dead the instant the second exists — no window, no TTL. + rr := submitClaim(t, s, first, "a-strong-passphrase-12") + if rr.Code == http.StatusFound { + t.Fatal("the SUPERSEDED code was accepted — a stale/TTL read left an old code alive") + } + if !strings.Contains(rr.Body.String(), "Hib") { // „Hibás vagy lejárt kód" (ASCII-safe substring) + t.Fatalf("superseded code: want the wrong-code error, body=%q", claimFirstLine(rr.Body.String())) + } + if s.settings.GetClaimed() { + t.Fatal("box was claimed by a superseded code") + } + if s.authEnabled() { + t.Fatal("a password was set by a superseded code") + } + + // ...and the SECOND code — the one the customer was actually told — works on the first attempt. + rr = submitClaim(t, s, second, "a-strong-passphrase-12") + if rr.Code != http.StatusFound || rr.Header().Get("Location") != "/" { + t.Fatalf("the current code was refused: got %d body=%q", rr.Code, claimFirstLine(rr.Body.String())) + } +} + +// FAIL CLOSED (§8.1) — an unreadable persisted state must never open the gate or accept a claim. +// The mutation that breaks it: return the cached value instead of the error from effectiveClaimCode. +func TestClaimCode_UnreadablePersistedState_FailsClosed(t *testing.T) { + s, cfg, path := claimReadThroughServer(t) + code := mintCodeViaHatch(t, path, cfg) + + // Corrupt the persisted state the way a truncated write would (present but unparseable). + if err := os.WriteFile(path, []byte("{not json"), 0o644); err != nil { + t.Fatalf("corrupting settings: %v", err) + } + + if _, _, _, err := s.effectiveClaimCode(); err == nil { + t.Fatal("an unparseable settings file did not surface an error") + } + if !s.claimGateActive() { + t.Fatal("the gate OPENED on an unreadable claim state — must fail closed") + } + if s.claimLegacyOpen() { + t.Fatal("an unreadable claim state was reported as legacy-open — must fail closed") + } + rr := submitClaim(t, s, code, "a-strong-passphrase-12") + if rr.Code == http.StatusFound { + t.Fatal("a claim was accepted while the persisted state was unreadable") + } + if s.authEnabled() { + t.Fatal("a password was set while the persisted state was unreadable") + } +}