Skip to content

security: threat-model review, audit coverage, and key-rotation runbooks (#64) - #104

Merged
richardmhope merged 1 commit into
mainfrom
m4-security-review
Jul 31, 2026
Merged

security: threat-model review, audit coverage, and key-rotation runbooks (#64)#104
richardmhope merged 1 commit into
mainfrom
m4-security-review

Conversation

@richardmhope

Copy link
Copy Markdown
Contributor

Stacked on #103 (Helm), which is stacked on #102 and #101. Review the last commit.

Closes the hardening epic's last issue. Three parts, one of which turned out to need code.

1. The review — docs/security-review.md

Every mitigation in docs/security.md mapped to the test that makes it true, boundary by boundary.
The point is falsifiability: "we validate input" is not a control, a named test that fails when the
validation is removed is one.

Where a mitigation has no test it says so. It ends with five open items stated rather than left
to be discovered
— extraction is not out-of-process sandboxed, the Vault backend is a seam, no
mTLS, reads are not audited, and rate limiting fails open by design.

2. Audit coverage — a test, not a paragraph

test_audit_coverage.py fails when:

  • a declared AUDIT_* action has no call site (coverage on paper only);
  • the reviewed vocabulary drifts from the declared one, in either direction;
  • an audited route quietly stops writing its trail (its own tests would still pass — behaviour is
    unchanged, only the trail is gone);
  • a credential value reaches an audit row.

What is deliberately not audited — reads, finding triage (richer finding_event trail),
lease/heartbeat traffic, failed logins — is recorded with the reasoning rather than left as an
apparent gap.

3. Pepper rotation needed a mechanism

The problem. Identity is HMAC(pepper, locator ‖ rule ‖ HMAC(pepper, secret)) and the plaintext
secret is never stored (ADR 0004). A new pepper therefore cannot be applied to existing rows by
recomputation — the input is gone. No migration can do this; only an engine holding the secret
mid-scan can compute its identity under a different key.

Rotating without accounting for that is quietly destructive: every finding ingests as new and
reconciliation auto-resolves the originals, detaching every accepted risk and analyst note from
anything anyone will look at again.

The fix — a dual-pepper window. With ICEBERG_PREVIOUS_FINGERPRINT_PEPPER_REF set, leases carry
both peppers, engines report each finding under both, and ingest treats a hit on the old identity as
the same finding under a new key — re-keying in place, so the row keeps its id and therefore its
state, resolution, assignee, notes, suppression and entire event trail. Scans count rekeyed, so
watching it fall to zero is how an operator knows the rotation is done.

The previous pepper deliberately does not fail closed the way the current one does: without it a
scan still produces correct findings, they simply ingest as new, which the next scan of the window
can fix. Refusing to scan would be the worse trade.

Runbook + dry-run

docs/runbooks/key-rotation.md covers both keys and is explicit that they are different operations:
master key = a re-seal (mechanical, minutes, finding identities untouched); pepper = a re-scan (a
window of days). It includes the re-seal loop, the "which sources still hold old identities?" query,
and what to do if a source cannot be scanned during the window.

test_pepper_rotation.py is the dry-run those promises rest on — it builds a finding under an old
pepper with a real analyst decision, runs a scan's worth of ingest with both identities reported,
and asserts the decision is still on the same row afterwards. It also asserts the negative: that
without the window the same secret produces a duplicate.

make lint, make type, make helm-verify and the full suite pass.

Closes #64

🤖 Generated with Claude Code

…oks (#64)

**The review.** `docs/security-review.md` maps every mitigation in
docs/security.md to the test that makes it true, boundary by boundary. Where
there is no test it says so and says what stands in its place — and it ends with
five open items stated rather than left to be discovered, including that
extraction is not out-of-process sandboxed and that rate limiting fails open.

**Audit coverage** is a test rather than a paragraph that goes stale.
`test_audit_coverage.py` fails when a declared `AUDIT_*` action has no call site,
when the reviewed vocabulary drifts from the declared one, when an audited route
stops writing its trail, or when a credential value reaches an audit row. What is
deliberately *not* audited — reads, finding triage (it has the richer
`finding_event` trail), lease/heartbeat traffic, failed logins — is recorded with
the reasoning.

**Pepper rotation needed a mechanism, not just a document.** Finding identity is
`HMAC(pepper, locator ‖ rule ‖ HMAC(pepper, secret))` and the plaintext is never
stored (ADR 0004), so a new pepper cannot be applied to existing rows by
recomputation — the input is gone. Only an engine holding the secret mid-scan can
compute its identity under a different key.

So: a dual-pepper window. With `ICEBERG_PREVIOUS_FINGERPRINT_PEPPER_REF` set,
leases carry both peppers, engines report each finding under both, and ingest
treats a hit on the old identity as the same finding under a new key and re-keys
it **in place** — same row, so state, resolution, assignee, notes, suppression
and the whole event trail come with it. Scans count `rekeyed`, so watching it
fall to zero is how an operator knows the rotation is complete.

The previous pepper deliberately does not fail closed the way the current one
does: without it a scan still produces correct findings, they just ingest as new,
which the next scan of the window can fix. Refusing to scan would be the worse
trade.

`docs/runbooks/key-rotation.md` covers both keys and is explicit that they are
different operations — the master key is a re-seal (mechanical, minutes, finding
identities untouched), the pepper is a re-scan (a window of days).

`test_pepper_rotation.py` is the dry-run the runbook's promises rest on: it
builds a finding under an old pepper with a real analyst decision, runs a scan's
worth of ingest, and asserts the decision is still attached to the same row —
and separately demonstrates the duplicate-and-auto-resolve failure that happens
without the window.

Closes #64

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@richardmhope
richardmhope merged commit c43a3b8 into main Jul 31, 2026
4 checks passed
@richardmhope
richardmhope deleted the m4-security-review branch July 31, 2026 08:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Threat-model & audit-log review; key-rotation runbook

1 participant