diff --git a/hub/internal/monitor/offsite.go b/hub/internal/monitor/offsite.go index a6e88c50..10469eaf 100644 --- a/hub/internal/monitor/offsite.go +++ b/hub/internal/monitor/offsite.go @@ -46,6 +46,9 @@ type OffsiteChecker struct { // R-435: when the hub RECEIVED the report that set lastCounts — the start of the interval whose // clean-up windows may explain the next fall (pinned tiers only, see snapshotDropped). lastCountAt map[string]time.Time + // R-435: clean-up windows already spent explaining a fall, per customer — each window explains one + // fall only, so the slack before the interval cannot let it excuse a second one (security review). + usedWindows map[string]map[int64]bool } const defaultOffsiteStaleAfter = 48 * time.Hour @@ -84,6 +87,7 @@ func NewOffsiteChecker(s *store.Store, staleAfter time.Duration, onEvent EventNo store: s, logger: logger, onEvent: onEvent, staleAfter: staleAfter, now: time.Now, fillStates: make(map[string]string), staleStates: make(map[string]string), dropStates: make(map[string]string), lastCounts: make(map[string]int), lastCountAt: make(map[string]time.Time), + usedWindows: make(map[string]map[int64]bool), } customers, err := s.GetCustomers() if err != nil { @@ -221,22 +225,22 @@ func (oc *OffsiteChecker) isStale(customerID string, off *offsiteReport) bool { // THE THRESHOLD, AND THE MEASUREMENT IT CAME FROM. Measured over the hub's own `reports` table on // 2026-09-01: 12 898 reports, 4 customers, 2026-06-05 → 2026-09-01. // -// * In the whole history there are NINE decreases, and EVERY ONE of them lands exactly on ZERO +// - In the whole history there are NINE decreases, and EVERY ONE of them lands exactly on ZERO // (36→0, 18→0 ×2, 15→0, 12→0, 8→0, 3→0). There is not one gradual retention decrease anywhere. -// * Every one of those nine predates `stats_known`, i.e. they are the R-331 shape — a zero that +// - Every one of those nine predates `stats_known`, i.e. they are the R-331 shape — a zero that // means "could not measure", not "nothing is there". Several carry a declared State // (`needs_credential`, `awaiting_recovery_key`), which says so outright. -// * In the window where `stats_known` is TRUE (380 reports across both live boxes) there are ZERO +// - In the window where `stats_known` is TRUE (380 reports across both live boxes) there are ZERO // decreases: demo-felhom sat flat at 10, demo-hp moved 67→68→69. Only rises. // // So observed retention churn gives NOTHING to calibrate against, and saying so is the honest answer // rather than inventing a number (R-401's lesson). The threshold is therefore reasoned from what // retention CAN do, not from what it was seen to do: // -// the box runs `forget --keep-daily 7 --keep-weekly 4 --keep-monthly 6 --group-by host,tags` -// over ~8 apps. On a boundary day several groups can expire at once, so a legitimate pass can -// plausibly remove low double digits. **What it can NEVER do is halve the total**: keeping 7 daily -// + 4 weekly + 6 monthly per group is a floor, and a mass deletion goes to ~0. +// the box runs `forget --keep-daily 7 --keep-weekly 4 --keep-monthly 6 --group-by host,tags` +// over ~8 apps. On a boundary day several groups can expire at once, so a legitimate pass can +// plausibly remove low double digits. **What it can NEVER do is halve the total**: keeping 7 daily +// + 4 weekly + 6 monthly per group is a floor, and a mass deletion goes to ~0. // // Hence: **a fall of MORE THAN HALF the previous count, and at least 5 snapshots.** The 50% cannot be // reached by retention; the floor of 5 stops a tiny-count box alarming on ordinary ageing. It is @@ -318,7 +322,19 @@ func (oc *OffsiteChecker) snapshotDropped(customerID string, off *offsiteReport, drop := prev - off.SnapshotCount if drop > 0 && oc.pinnedTier(customerID) { from := oc.lastCountAt[customerID].Add(-windowSlack) - removed, unknown := oc.store.RemovedByWindowsBetween(customerID, from, reportAt) + credits, unknown := oc.store.WindowCreditsBetween(customerID, from, reportAt) + used := oc.usedWindows[customerID] + if used == nil { + used = make(map[int64]bool) + oc.usedWindows[customerID] = used + } + removed := 0 + for _, c := range credits { + if !used[c.ID] { + removed += c.Explains + used[c.ID] = true // spent on this fall, explained or not + } + } if !unknown { if drop > removed { return true, prev, off.SnapshotCount, true @@ -449,6 +465,7 @@ func (oc *OffsiteChecker) Check() { delete(oc.dropStates, c.CustomerID) delete(oc.lastCounts, c.CustomerID) // no baseline survives a vanished object delete(oc.lastCountAt, c.CustomerID) + delete(oc.usedWindows, c.CustomerID) continue } if oc.store.IsCustomerBlocked(c.CustomerID) { @@ -457,6 +474,7 @@ func (oc *OffsiteChecker) Check() { delete(oc.dropStates, c.CustomerID) delete(oc.lastCounts, c.CustomerID) delete(oc.lastCountAt, c.CustomerID) + delete(oc.usedWindows, c.CustomerID) continue } @@ -524,6 +542,7 @@ func (oc *OffsiteChecker) Check() { delete(oc.dropStates, k) delete(oc.lastCounts, k) delete(oc.lastCountAt, k) + delete(oc.usedWindows, k) } } } diff --git a/hub/internal/monitor/r435_pinned_drop_test.go b/hub/internal/monitor/r435_pinned_drop_test.go index efc1bd70..aff50970 100644 --- a/hub/internal/monitor/r435_pinned_drop_test.go +++ b/hub/internal/monitor/r435_pinned_drop_test.go @@ -117,6 +117,38 @@ func TestR435_LyingWindowExplainsOnlyItsCap(t *testing.T) { } } +// Security review 2026-10-08: one window explains ONE fall. A window that removed 9 explains 69 → 60; a +// second fall 60 → 55 in the next report (inside the 2-h slack, no new window) alarms. +// RED-PROOF: stop marking windows as spent (usedWindows) → the second fall is explained again → 0 alarms → FAILS. +func TestR435_OneWindowExplainsOneFall(t *testing.T) { + st := newDiskStore(t) + cust := "p10" + if err := st.RecordOffsiteKeyInstalled(cust, "fp-1"); err != nil { + t.Fatal(err) + } + if ok, err := st.RecordOffsiteKeyConfirmed(cust, "fp-1"); err != nil || !ok { + t.Fatalf("confirm: %v %v", ok, err) + } + var msgs []string + saveOffsiteReport(t, st, cust, dropJSON(69, true, "", "ok")) + oc := NewOffsiteChecker(st, 48*time.Hour, func(_, et, _, msg, _, _ string) { + if et == "offsite_snapshots_dropped" { + msgs = append(msgs, msg) + } + }, quietLog()) + closedWindow(t, cust, 69, 60)(st) + saveOffsiteReport(t, st, cust, dropJSON(60, true, "", "ok")) + oc.Check() + if len(msgs) != 0 { + t.Fatalf("the window explains 69 -> 60; got %d alarm(s)", len(msgs)) + } + saveOffsiteReport(t, st, cust, dropJSON(55, true, "", "ok")) + oc.Check() + if len(msgs) != 1 { + t.Fatalf("a spent window must not explain the next fall 60 -> 55; got %d alarm(s)", len(msgs)) + } +} + // Controls: the half-rule still governs a non-pinned tier, and an installed-but-unconfirmed key. func TestR435_NotPinnedKeepsHalfRule(t *testing.T) { if msgs := r435Run(t, "n1", r435Setup{next: dropJSON(60, true, "", "ok")}); len(msgs) != 0 { diff --git a/hub/internal/store/offsite_keys.go b/hub/internal/store/offsite_keys.go index c3ae8895..3f00ae06 100644 --- a/hub/internal/store/offsite_keys.go +++ b/hub/internal/store/offsite_keys.go @@ -325,36 +325,44 @@ func (s *Store) ForceOffsiteAbandonDueForTest(id int64) error { return err } -// RemovedByWindowsBetween — R-435 (D7, `09` §3 decision 191). How many snapshots the clean-up windows -// the hub opened for this customer EXPLAIN between two reports, over windows closed in (from, to] or -// still open. On a pinned tier these windows are the only legitimate way the count can fall, so -// anything beyond the sum is unexplained. +// WindowCredit is how many snapshots one clean-up window may explain (R-435). +type WindowCredit struct { + ID int64 + Explains int +} + +// WindowCreditsBetween — R-435 (D7, `09` §3 decision 191). The clean-up windows the hub opened for this +// customer that may EXPLAIN a fall between two reports: windows closed in (from, to], and windows still +// open at `to` whose closes_by has not passed before `from` (a window stuck open past its deadline +// explains nothing). On a pinned tier these windows are the only legitimate way the count can fall. // // Each window explains AT MOST its hub-set max_remove (security review 2026-10-08): count_after is the // box's own word, so a box that lies about it — or a window closed by timeout or still open, which has // no count_after — can never explain more than the cap the hub itself granted. A closed window with a -// count_after explains min(count_before − count_after, max_remove); a window with no usable count_after -// explains max_remove. +// count_after explains min(count_before − count_after, max_remove); one with no usable count_after +// explains max_remove. The CALLER spends each window once (OffsiteChecker.usedWindows), so the slack +// before `from` cannot let one window explain several falls. // // unknown = true only when the store cannot answer (a query error, or a window with no usable cap). The -// caller then falls back to the half-rule — never to „explained" (fail closed, the review's other -// finding). Pinned by r435_windows_between_test.go and r435_pinned_drop_test.go. -func (s *Store) RemovedByWindowsBetween(customerID string, from, to time.Time) (removed int, unknown bool) { +// caller then falls back to the half-rule — never to „explained". Pinned by r435_windows_between_test.go +// and r435_pinned_drop_test.go. +func (s *Store) WindowCreditsBetween(customerID string, from, to time.Time) (credits []WindowCredit, unknown bool) { const f = "2006-01-02 15:04:05" rows, err := s.db.Query(` - SELECT count_before, count_after, max_remove, closed_at IS NULL FROM offsite_windows + SELECT id, count_before, count_after, max_remove, closed_at IS NULL FROM offsite_windows WHERE customer_id = ? - AND ((closed_at IS NULL AND opened_at <= ?) OR (closed_at > ? AND closed_at <= ?))`, - customerID, to.UTC().Format(f), from.UTC().Format(f), to.UTC().Format(f)) + AND ((closed_at IS NULL AND opened_at <= ? AND closes_by > ?) OR (closed_at > ? AND closed_at <= ?))`, + customerID, to.UTC().Format(f), from.UTC().Format(f), from.UTC().Format(f), to.UTC().Format(f)) if err != nil { - return 0, true + return nil, true } defer rows.Close() for rows.Next() { + var id int64 var before, after, maxRemove sql.NullInt64 var open bool - if err := rows.Scan(&before, &after, &maxRemove, &open); err != nil { - return 0, true + if err := rows.Scan(&id, &before, &after, &maxRemove, &open); err != nil { + return nil, true } if !maxRemove.Valid || maxRemove.Int64 <= 0 { unknown = true @@ -367,11 +375,11 @@ func (s *Store) RemovedByWindowsBetween(customerID string, from, to time.Time) ( } } if explains > 0 { - removed += explains + credits = append(credits, WindowCredit{ID: id, Explains: explains}) } } if rows.Err() != nil { - return 0, true + return nil, true } - return removed, unknown + return credits, unknown } diff --git a/hub/internal/store/r435_windows_between_test.go b/hub/internal/store/r435_windows_between_test.go index bf7afee2..e19715f5 100644 --- a/hub/internal/store/r435_windows_between_test.go +++ b/hub/internal/store/r435_windows_between_test.go @@ -5,12 +5,20 @@ import ( "time" ) -// R-435: RemovedByWindowsBetween sums only windows closed inside the interval (or still open), and each +// R-435: WindowCreditsBetween lists only windows closed inside the interval (or still open), and each // window explains at most its hub-set max_remove — a box's count_after cannot widen it, and a window with // no usable count_after explains exactly its cap. Only a window with no cap makes the answer unknown. // RED-PROOF (security review 2026-10-08): drop the max_remove cap → „lying box" returns 69, not 34 → FAILS. func TestR435_RemovedByWindowsBetween(t *testing.T) { s := newTestStore(t) + sum := func(cust string, from, to time.Time) (int, bool) { + cs, unk := s.WindowCreditsBetween(cust, from, to) + n := 0 + for _, c := range cs { + n += c.Explains + } + return n, unk + } at := func(ts string) time.Time { v, _ := time.Parse("2006-01-02 15:04:05", ts); return v } ins := func(cust, opened, closed string, before, after, maxRemove any) { t.Helper() @@ -18,8 +26,9 @@ func TestR435_RemovedByWindowsBetween(t *testing.T) { if closed != "" { c = closed } + closesBy := "2026-10-01 23:00:00" // a window's deadline; an open one past it before `from` is stale if _, err := s.db.Exec(`INSERT INTO offsite_windows (customer_id, opened_at, closes_by, closed_at, count_before, count_after, max_remove) VALUES (?, ?, ?, ?, ?, ?, ?)`, - cust, opened, opened, c, before, after, maxRemove); err != nil { + cust, opened, closesBy, c, before, after, maxRemove); err != nil { t.Fatal(err) } } @@ -29,30 +38,38 @@ func TestR435_RemovedByWindowsBetween(t *testing.T) { ins("b", "2026-10-01 03:00:00", "2026-10-01 03:10:00", 50, 40, 25) // another customer from, to := at("2026-09-30 00:00:00"), at("2026-10-02 00:00:00") - if n, unk := s.RemovedByWindowsBetween("a", from, to); n != 9 || unk { + if n, unk := sum("a", from, to); n != 9 || unk { t.Fatalf("a: want 9 known, got %d unknown=%v", n, unk) } - if n, unk := s.RemovedByWindowsBetween("c", from, to); n != 0 || unk { + if n, unk := sum("c", from, to); n != 0 || unk { t.Fatalf("no window: want 0 known, got %d unknown=%v", n, unk) } // a box that claims it removed everything explains only the cap the hub granted ins("l", "2026-10-01 03:00:00", "2026-10-01 03:10:00", 69, 0, 34) - if n, unk := s.RemovedByWindowsBetween("l", from, to); n != 34 || unk { + if n, unk := sum("l", from, to); n != 34 || unk { t.Fatalf("lying box: want the cap 34, got %d unknown=%v", n, unk) } // a timeout close (count_after −1) inside the interval → explains its cap ins("d", "2026-10-01 03:00:00", "2026-10-01 03:40:00", 69, -1, 10) - if n, unk := s.RemovedByWindowsBetween("d", from, to); n != 10 || unk { + if n, unk := sum("d", from, to); n != 10 || unk { t.Fatalf("timeout-closed window: want its cap 10, got %d unknown=%v", n, unk) } // a window still open → explains its cap ins("e", "2026-10-01 03:00:00", "", 69, nil, 10) - if n, unk := s.RemovedByWindowsBetween("e", from, to); n != 10 || unk { + if n, unk := sum("e", from, to); n != 10 || unk { t.Fatalf("open window: want its cap 10, got %d unknown=%v", n, unk) } + // a window stuck open past its deadline before the interval explains nothing + ins("g", "2026-09-01 03:00:00", "", 69, nil, 10) + if _, err := s.db.Exec(`UPDATE offsite_windows SET closes_by = '2026-09-01 04:00:00' WHERE customer_id = 'g'`); err != nil { + t.Fatal(err) + } + if n, unk := sum("g", from, to); n != 0 || unk { + t.Fatalf("stale open window: want 0, got %d unknown=%v", n, unk) + } // a window with no cap → unknown (the caller falls back to the half-rule) ins("f", "2026-10-01 03:00:00", "2026-10-01 03:10:00", 69, 60, nil) - if _, unk := s.RemovedByWindowsBetween("f", from, to); !unk { + if _, unk := sum("f", from, to); !unk { t.Fatal("a window with no cap must make the interval unknown") } }