From 40aa79a91a88d224a6b5274f152b94b325afaa60 Mon Sep 17 00:00:00 2001 From: henleda Date: Thu, 30 Jul 2026 11:15:03 -0500 Subject: [PATCH] feat(audit): J4 attribution backfill MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `vpcopilot audit-backfill` + `POST /api/audit-backfill`. `backfill.py` freezes the finding each audit entry belongs to into `/audit-backfill.json`, which `export.build_audit_events` reads beside the log and the evidence bundle ships. The reconciled note framed this as persisting what the exporter already derives at read time. Running it on real data showed the sharper reason: `policies.json` is rewritten by EVERY scan, so the derivation does not merely decay — it can go wrong. Re-scan and the mapping is gone; if the new scan generates a policy with the same name for a different finding, the live lookup still succeeds and attributes the old entry to the WRONG finding. That is why the frozen answer wins over the live lookup rather than only filling gaps in it. Both reproduced against the real out-claude log, whose four refine_apply entries record a policy but no finding. The log is never edited — it is append-only and `record()` strips caller-supplied identity — so this is a sidecar, and the sidecar carries no actor, run_id or host. Acceptance: nothing invented, the backfill writes its own audit record, a second run is a no-op. Also consolidates the duplicated policy→finding lookup into `ledger.policy_index`. That consolidation silently changed three behaviours before it was measured against the previous implementation across nine inputs — duplicate-name precedence, falsy finding_ids, and what happens to a corrupt file. The old semantics are restored exactly and pinned; the exporter keeps its own tolerance at its call site. 778 tests pass with no credentials and no .env, ruff clean, coverage 79%. Co-Authored-By: Claude Opus 5 (1M context) --- ROADMAP.md | 118 ++++++-- docs/AUDIT.md | 66 ++++- src/vpcopilot/backfill.py | 237 ++++++++++++++++ src/vpcopilot/cli.py | 39 +++ src/vpcopilot/console/app.py | 17 ++ src/vpcopilot/export.py | 52 +++- src/vpcopilot/ledger.py | 44 ++- tests/test_backfill.py | 532 +++++++++++++++++++++++++++++++++++ 8 files changed, 1074 insertions(+), 31 deletions(-) create mode 100644 src/vpcopilot/backfill.py create mode 100644 tests/test_backfill.py diff --git a/ROADMAP.md b/ROADMAP.md index 85d6e21..06f7325 100644 --- a/ROADMAP.md +++ b/ROADMAP.md @@ -723,23 +723,107 @@ sees the trail. J1–J4 are the open `BACKLOG.md` evidence entries, scheduled; * - **Reconciled:** there is no Admin tab — credential and `.env` editing lives on the ⚙ Setup page (`GET`/`POST /api/config`). -- [ ] **J4** Attribution backfill. (S, P2) - `vpcopilot audit-backfill` fills only what is provably derivable, policy name to finding - via `policies.json`, marks the rest `unknown`, and records the backfill itself. - - Acceptance: no entry gets an invented actor or run_id; the backfill writes its own audit - record; a second run is a no-op. - - **Reconciled:** `audit` is a flat `@app.command()` with no typer sub-app, so `audit backfill` - as two words is not addable as written — `audit-backfill` matches the existing kebab - convention (`bench-model`, `apply-maluser`, `xc-rm`, `lab-create`). Smaller than it looks: - `export.build_audit_events` **already** recovers a finding from the `policies.json` index for - legacy entries and already leaves blanks rather than inventing. J4 persists what the exporter - derives at read time. **It cannot rewrite `audit.log`**: the log is append-only - (`audit.py:26`) and `record()` strips caller-supplied identity (`_STAMPED`, `audit.py:17,21`), - enforced by `test_identity_cannot_be_overridden_by_a_caller`. The backfill must write a - sidecar (e.g. `/audit-backfill.json`) that `export.build_audit_events` consults beside - its existing `by_policy` lookup. Note a second copy of that lookup already exists — - `ledger.find_finding_for_policy` (`ledger.py:108`) — consolidate on one rather than adding a - third. Needs a `POST /api/audit-backfill` twin per the two-surfaces invariant. +- [x] **J4** Attribution backfill. (S, P2) — **DONE:** `vpcopilot audit-backfill` + + `POST /api/audit-backfill`. `backfill.py` freezes the finding each audit entry belongs to into + `/audit-backfill.json`, which `export.build_audit_events` reads beside the log and the + evidence bundle ships. Verified against the two real audit logs on disk. 21 tests. + - **Acceptance, as met:** no entry gets an invented actor or run_id ✅ — guaranteed by having + nowhere to put one: the sidecar carries attribution and nothing else, pinned by a test on its + keys; the backfill writes its own audit record ✅ (`audit_backfill`, with identity stamped + centrally like every other action); a second run is a no-op ✅ — nothing written, nothing + recorded. + - **The reconciled note undersold why this matters, and running it on real data showed it.** It + framed J4 as persisting what the exporter already derives at read time. But `policies.json` is + rewritten by **every** scan, so the derivation does not merely decay — it can go *wrong*. Two + outcomes, and the second is the one worth building for: + - Re-scan into the same out dir and the mapping is gone; the exporter silently stops attributing + those entries. + - If the later scan generates a policy with the **same name** for a **different** finding, the + live lookup still succeeds and attributes the old entry to the **wrong** finding — a confident + wrong answer in the artifact whose entire job is to be trustworthy. That is why the frozen + answer **wins over** the live lookup rather than merely filling gaps in it. + Both reproduced against the real `out-claude` log, whose four `refine_apply` entries record a + policy but no finding: attribution survived a wiped index, and a planted name collision failed to + move them. + - **`unknown` is sticky, deliberately.** An entry the backfill looked at and could not resolve stays + unresolved, so a later `policies.json` cannot supply an answer the backfill already declined to + give. "We looked and could not establish this" and "we have not looked" are different facts — + the same distinction H2 draws between an unpinned dependency and a clean one. + - **The no-op is load-bearing, not tidiness.** The command appends its own `audit_backfill` entry, + so without the check every run would see one more entry than the last, decide something had + changed, and append again: a log that grows by a line every time anyone asks whether it needs + backfilling. (The I1 precedent — a reconcile pass that changes nothing writes nothing.) + - **Keyed by index, verified by `(ts, action)`.** The index is stable only because the log is + append-only; if it is ever rebuilt or truncated the index still resolves — to a *different* + entry. Mismatched rows are dropped and counted rather than moved onto the wrong record, and the + count is surfaced, because a non-zero one means the log was rewritten and that is itself worth + knowing. + - **Verified against the real logs, and the no-op case is the interesting one.** `demo/out` is + already fully attributed — every entry carries its `finding_id` — so the backfill resolved + nothing and wrote nothing, which is the correct answer and the one a synthetic fixture would not + have exercised. `out-claude` had four genuinely unattributed `refine_apply` entries; all four + resolved, and a repeat run left `audit.log` byte-identical. + - **Found by adversarial review, before shipping** — and the first one nearly shipped a feature + that destroyed its own purpose. + - **A second run wiped the attribution it had just frozen.** `derive` recomputed every row from + the CURRENT `policies.json`, which is precisely what the sidecar is a defence against: run the + command, re-scan, run it again, and every resolved row collapsed to `unknown` — permanently, + because `unknown` is sticky. The second use of the feature undid the first. **Two reviewers + found it independently**, which is the signal worth noting. The sidecar is now monotonic: a row + already on disk is carried forward verbatim and never recomputed, exactly like the append-only + log it annotates. Forcing a fresh derivation means deleting the sidecar — an explicit act, not + a side effect of running a command twice. + - **The sidecar was written non-atomically.** `write_text` truncates first, so a crash mid-write + left a half-written evidence file — and `load` swallows the resulting `JSONDecodeError`, so the + failure mode was losing every frozen attribution without a word. Written to a temp file and + `os.replace`d now. The skeptic verifying it went further and **found a weakness in the test + that was supposed to cover this**: `test_a_corrupt_sidecar_is_ignored_rather_than_fatal` + pinned "does not crash" but not "does not mis-attribute", because its fixture left the live + index agreeing with the frozen answer. With a re-scan that reused a policy name, a torn + sidecar fell through and produced a confidently wrong attribution — reproduced at 372 wrong + exports across 1,887 concurrent reads. So *absent* and *unreadable* are now different answers: + no sidecar means nobody froze anything and the live index is the best available guess, while a + sidecar that exists and will not parse means the live index is by definition not the one those + entries belong to, and the cell is left blank. + - **The bundle's caveats said nothing about it.** A derived sidecar that can supply a + `finding_id` is exactly what the manifest's caveat list exists to disclose; it now says what + the sidecar is, that it carries no identity, and that where it and the log disagree the log is + the evidence. + - **A corrupt `policies.json` tracebacked out of the CLI and 500'd the console.** Keeping the + raise for the apply paths (above) is right; propagating it out of a read-mostly evidence + command is not. An unreadable index means nothing can be established, which is a decline with a + reason. + - **Rich silently ate the `[dry-run]` marker.** `rprint(f"[dim]{m}[/dim]")` reads `[dry-run]` as + a markup tag and drops it — so the one word telling an operator that nothing was written was + the one word that vanished. Escaped at this command's log sink. The same pattern exists in + other commands whose messages contain brackets; only the one whose output actually does was + changed here rather than sweeping the CLI. + - **The two removed copies disagreed about duplicate policy names** — the exporter's inline dict + was last-wins, `find_finding_for_policy` first-wins — so consolidation could not preserve both. + First-wins is the deliberate unification: it matches the apply path, the safety-critical + consumer. Pinned, with the caveat that `policies.json` should never contain duplicates anyway. + - **Found while verifying the reviewers: consolidating the lookup silently changed three + behaviours.** Compared against the previous implementation across nine inputs, `policy_index` + had altered duplicate-name precedence (last-wins instead of first), filtered falsy `finding_id`s + to `None`, and swallowed a corrupt `policies.json` that used to raise. All three reach four + apply-path callers, where the corrupt-file case is the sharp one: returning `None` instead of + raising means applying a band-aid with weaker probe validation and no explanation. The old + semantics are restored exactly and pinned by a parametrized comparison; the exporter keeps its + own tolerance at its call site, and the backfill does its own `or None`. A consolidation that + quietly changes behaviour is worse than the duplication it removes. + - **The limit is stated rather than left to be discovered** (the J1 precedent). A forged sidecar + can move `finding_id` and the columns joined *through* it — `title`, `vuln_class`, `severity`, + `ledger_state`, `pr_url` — and nothing else: everything the log stamped is unreachable. That adds + no trust assumption the export did not already make, because `findings.json` and `ledger.json` + already drive those same joins and are the same kind of file in the same directory. Verified end + to end: the sidecar is digested in the bundle manifest, a clean bundle verifies at 47 members, + and a forged one is caught as `MISMATCH audit-backfill.json`. + - **Reconciled, and confirmed accurate:** `audit` is a flat `@app.command()` with no typer sub-app, + so `audit-backfill` matches the kebab convention. `export.build_audit_events` already left blanks + rather than inventing. The log cannot be rewritten (`audit.py` `_STAMPED`, enforced by + `test_identity_cannot_be_overridden_by_a_caller`), hence the sidecar. The duplicate lookup was + real — `ledger.find_finding_for_policy` and an inline copy in the exporter — and J4 would have + been the third; both now go through `ledger.policy_index`, the one implementation. - [ ] **J5** Weakness and framework mapping. (M, P2) Stamp each finding with a CWE ID and an OWASP API or Web Top 10 category at triage, and diff --git a/docs/AUDIT.md b/docs/AUDIT.md index f8cc53b..74f3c6d 100644 --- a/docs/AUDIT.md +++ b/docs/AUDIT.md @@ -568,7 +568,7 @@ back-fills it. |---|---| | No `run_id` / `actor` / `host` / `tool_version` | Those cells export **blank**. They are not inferred from the current environment — a guess in an audit trail is worse than a gap. | | `open_pr` wrote `finding`, not `finding_id` | Resolved via the `finding` key. Current builds write **both** (`pr.py`), so old and new logs read the same way. | -| `apply_*` recorded no finding at all | Resolved through the `policies.json` index: `entry.policy` → `policy_name` → `finding_id`. Works only for entries that name a `policy`, and only while that scan's `policies.json` is still in the dir. Otherwise blank. | +| `apply_*` recorded no finding at all | Resolved through the `policies.json` index: `entry.policy` → `policy_name` → `finding_id`. Works only for entries that name a `policy`, and only while that scan's `policies.json` is still in the dir — **`vpcopilot audit-backfill` freezes it before that expires** (§9.1). Otherwise blank. | | No `namespace` | Blank. The `lb` is still there; the tenant/namespace is not recoverable after the fact. | | No `kept` | Older entries (and any action that omits it) fall through to `passed`/`failed` rather than `kept`. Absence of `kept` is not evidence of rollback. | | Mixed success keys (`passed` / `config_enabled` / `enabled`) | Coalesced into one `outcome` (§5) — including the seeded demo dataset, which uses its own mix. | @@ -582,6 +582,70 @@ cells come out blank: vpcopilot export --out demo/out --output /tmp/demo-evidence.zip ``` +### 9.1 Attribution backfill (J4) + +The `policies.json` fallback above has an expiry date, and it is easy to miss: **every scan rewrites +`policies.json`**. Re-scan into the same out dir and the mapping from an old entry's policy name back +to its finding is gone — the exporter quietly stops attributing those entries. Worse, if the new scan +generates a policy with the *same name* for a *different* finding, the lookup still succeeds and +attributes the old entry to the **wrong** finding: a confident wrong answer, in the artifact whose +entire job is to be trustworthy. + +```sh +vpcopilot audit-backfill --out out # freeze what is derivable right now +vpcopilot audit-backfill --out out --dry-run # report it, write nothing +``` + +It writes `/audit-backfill.json`, a sidecar the exporter reads beside the log, and ships it in +the evidence bundle so a reviewer receives both together. + +Four properties are the whole point: + +- **`audit.log` is never edited.** It is append-only and `record()` strips caller-supplied identity; + a backfill that could rewrite an entry would destroy the property that makes the log worth keeping. + The only thing appended is the backfill's own `audit_backfill` record. +- **Nothing is invented.** The sidecar carries **no `actor`, no `run_id`, no `host`** — there is + nowhere to put one. It maps an entry to a finding and nothing else; identity stays on the log. +- **The frozen answer wins over the live lookup**, because after a re-scan the live index describes a + different run. +- **`unknown` is sticky.** An entry the backfill looked at and could not resolve stays unresolved, + so a later `policies.json` cannot supply an answer the backfill already declined to give. "We + looked and could not establish this" is a fact worth keeping. + +A second run writes nothing and records nothing. That matters more than tidiness: the command appends +its own entry, so without the check each run would see one more entry than the last, decide something +had changed, and append again. + +The sidecar is keyed by the entry's index in the append-only log and verified against its +`(ts, action)`. If the log is ever rebuilt or truncated, mismatched rows are **dropped and counted** +rather than silently moved onto the wrong entry. It is written atomically (temp file plus rename), +because a half-written evidence file that will not parse is worse than none. + +**Absent and unreadable are different answers.** No sidecar means nobody has frozen anything, so the +exporter falls back to the live policy index as it always did. A sidecar that is present and will +*not* parse means somebody did freeze attribution and it is now unreadable — and the live index is, +by definition, not the one those entries belong to. The cell is left blank rather than guessed. + +#### What a forged sidecar could do — and what it could not + +The log is append-only; a sidecar is an ordinary file. So state the limit plainly rather than leave +a reviewer to work it out. + +A hand-edited `audit-backfill.json` can change an entry's `finding_id`, and through it the fields +joined *from* that id: `title`, `vuln_class`, `severity`, `ledger_state`, `pr_url`. It cannot change +anything the log itself stamped — `ts`, `actor`, `host`, `run_id`, `action`, `outcome`, `lb`, +`tool_version` all come straight from `audit.log` and are unreachable from the sidecar. + +**This adds no trust assumption that the export did not already make.** `findings.json` and +`ledger.json` already drive exactly those joined columns, and they are the same kind of file in the +same directory — anyone who can forge the sidecar can forge those and get the same result. The +sidecar is digested in the bundle manifest like every other member (§6), so tampering *after* export +is caught by `export --verify`; tampering *before* export is a question about who has write access to +the run directory, which no artifact in it can answer. + +The one thing that survives all of it is the log: append-only, identity stamped centrally, and +shipped verbatim in the bundle. If the sidecar and the log ever disagree, the log is the evidence. + --- ## See also diff --git a/src/vpcopilot/backfill.py b/src/vpcopilot/backfill.py new file mode 100644 index 0000000..7c61402 --- /dev/null +++ b/src/vpcopilot/backfill.py @@ -0,0 +1,237 @@ +"""J4 — attribution backfill. Freeze the finding an old audit entry belongs to, while it is still +derivable. + +**The audit log cannot be rewritten, and should not be.** `audit.log` is append-only, and +`audit.record` strips caller-supplied identity so no caller can claim to be someone else — pinned by +`test_identity_cannot_be_overridden_by_a_caller`. An entry that could not say who made a change is +not an audit record, and a backfill that could edit one would destroy the property that makes the +log worth keeping. So this writes a **sidecar**, `/audit-backfill.json`, that the exporter +consults beside the log. The log stays exactly as it was written. + +**Why persist a lookup the exporter already does at read time.** `export.build_audit_events` recovers +a finding from `policies.json` for entries that carry only a policy name. But `policies.json` is +rewritten by **every** scan (`pipeline._write_out`), so it describes the newest run, not the run the +entry belongs to. Two things follow, and the second is the one that matters: + +1. Re-scan into the same out dir and the mapping for older entries is simply **gone** — the exporter + silently stops attributing them. +2. Worse, if the new scan happens to generate a policy with the **same name** and a different + finding, the lookup still succeeds and attributes the old entry to the **wrong finding**. A + confident wrong answer, in the artifact whose entire job is to be trustworthy. + +The backfill captures the mapping while it is still true, and its answers take precedence over the +live lookup for the entries it covers. + +**"Unknown" is sticky, on purpose.** An entry the backfill looked at and could not resolve is +recorded as `unknown`, and that stops the exporter guessing from a later `policies.json`. Having +looked and failed is a fact worth keeping; it is the difference between "we could not establish +this" and "we have not tried", which is the distinction this codebase refuses to blur anywhere else. +""" +from __future__ import annotations + +import json +import os +from collections.abc import Callable +from pathlib import Path + +from . import audit, ledger, runmeta +from . import __version__ + +SIDECAR = "audit-backfill.json" + +# Our own bookkeeping entries. Skipped when backfilling: they name no policy and belong to no +# finding, and including them would make every run differ from the last one — see `backfill()`. +_OWN_ACTION = "audit_backfill" + + +def _key(e: dict) -> tuple[str, str]: + """What identifies an entry besides its position. Stored alongside the index so a log that was + truncated or rebuilt is DETECTED rather than silently mis-keyed: the index would still resolve, + but to a different entry.""" + return e.get("ts", ""), e.get("action", "") + + +def derive(out_dir: str, *, keep: dict[int, dict] | None = None) -> list[dict]: + """The attribution this run dir can still prove, one record per audit entry that lacks one. + + Nothing is invented: the only source is `policies.json`, via the single shared lookup. An entry + that already carries `finding_id` is not recorded at all — there is nothing to freeze. + + **`keep` makes the sidecar monotonic, and without it this function destroys what it exists to + protect.** Re-deriving every row from the CURRENT `policies.json` is exactly what the sidecar is + a defence against: run the command once, re-scan, run it again, and every frozen attribution is + recomputed against an index that no longer describes those entries — resolved rows collapse to + `unknown`, and because `unknown` is sticky the loss is permanent. The second use of the feature + undid the first. + + So a row already on disk is carried forward **verbatim** and never recomputed, resolved or not. + The sidecar is monotonic in the same way as the log it annotates: entries are added, never + revised. Genuinely wanting a fresh derivation means deleting the sidecar — an explicit, visible + act rather than a silent side effect of running a command twice.""" + keep = keep or {} + entries = audit.load(out_dir) + index = ledger.policy_index(out_dir) + rows: list[dict] = [] + for i, e in enumerate(entries): + if e.get("action") == _OWN_ACTION: + continue + if e.get("finding_id") or e.get("finding"): + continue # already attributed; the log is the better source + if i in keep: + rows.append(keep[i]) # frozen when it was true; never re-derived + continue + ts, action = _key(e) + policy = e.get("policy") or "" + # `or None` here, not in `ledger.policy_index`: an empty finding_id is + # unresolved for OUR purposes, but four apply-path callers share that lookup + # and expect it to hand back whatever the index holds. + fid = (index.get(policy) or None) if policy else None + rows.append({"i": i, "ts": ts, "action": action, "policy": policy, + "finding_id": fid, + "source": "policies.json" if fid else "unknown"}) + return rows + + +def load_state(out_dir: str) -> tuple[dict[int, dict], bool]: + """`(rows, trustworthy)`. `trustworthy` is False when a sidecar EXISTS but cannot be read. + + Absent and unreadable are different answers and the exporter must be able to tell them apart. A + sidecar that is not there means nobody has frozen anything, so falling back to the live policy + index is the best available guess. A sidecar that is there and will not parse means somebody DID + freeze attribution and it is now unreadable — and the live index is, by definition, not the one + those entries belong to. Guessing from it there can produce a confidently wrong attribution, as + an adversarial review demonstrated: a torn sidecar plus a re-scan that reused a policy name + attributed old entries to the wrong finding, in a bundle that then SHA-256s and signs the answer + as evidence. + + Rows are VERIFIED against the log they describe: one whose `(ts, action)` no longer matches the + entry at its index is dropped, because the index means something else now. Better to lose an + attribution than to move it onto the wrong entry.""" + p = Path(out_dir) / SIDECAR + if not p.is_file(): + return {}, True + try: + doc = json.loads(p.read_text()) + except (OSError, json.JSONDecodeError): + return {}, False + entries = audit.load(out_dir) + out: dict[int, dict] = {} + for row in doc.get("entries") or []: + i = row.get("i") + if not isinstance(i, int) or not (0 <= i < len(entries)): + continue + if _key(entries[i]) != (row.get("ts", ""), row.get("action", "")): + continue # the log moved under it + out[i] = row + return out, True + + +def load(out_dir: str) -> dict[int, dict]: + """The verified rows, for callers that do not need to distinguish absent from unreadable.""" + return load_state(out_dir)[0] + + +def stale_rows(out_dir: str) -> int: + """How many sidecar rows no longer match the log. Surfaced rather than swallowed — a non-zero + count means the log was rewritten, which is itself worth knowing about.""" + p = Path(out_dir) / SIDECAR + if not p.is_file(): + return 0 + try: + doc = json.loads(p.read_text()) + except (OSError, json.JSONDecodeError): + return 0 + return len(doc.get("entries") or []) - len(load(out_dir)) + + +def _same(rows: list[dict], existing: dict) -> bool: + """Would writing `rows` change anything? Compared on the fields that carry meaning, so a + re-serialisation or a new `generated` timestamp does not count as a change.""" + old = {r["i"]: r for r in (existing.get("entries") or [])} + if len(old) != len(rows): + return False + for r in rows: + o = old.get(r["i"]) + if not o or any(o.get(k) != r.get(k) for k in ("ts", "action", "policy", + "finding_id", "source")): + return False + return True + + +def backfill(out_dir: str = "out", *, dry_run: bool = False, log: Callable = print) -> dict: + """Persist what is provably derivable. Returns a summary; writes at most one sidecar and one + audit record. + + A second run is a no-op — nothing written, nothing recorded — which is not merely tidiness. This + function appends its own `audit_backfill` entry, so without that check every run would see one + more entry than the last, decide something had changed, and append again: a log that grows by one + line every time anybody asks whether it needs backfilling. (The I1 precedent: a reconcile pass + that changes nothing writes nothing.)""" + out = Path(out_dir) + if not (out / "audit.log").is_file(): + log(f"no audit.log in {out_dir!r} — nothing to backfill") + return {"out": out_dir, "entries": 0, "resolved": 0, "unknown": 0, "wrote": False, + "recorded": False, "reason": "no audit log"} + + # Carry forward whatever is already frozen and verified — see `derive`. Without this, + # running the command twice with a re-scan in between destroys the attribution. + # + # `ledger.policy_index` RAISES on a corrupt `policies.json`, deliberately: the apply paths use it + # to choose which probe validates a band-aid, and returning nothing there would mean applying + # with weaker validation and no explanation. This command is not an apply. An unreadable index + # means it cannot establish anything, which is a decline with a reason — not a traceback out of + # the CLI and an HTTP 500 out of the console. + try: + rows = derive(out_dir, keep=load(out_dir)) + except (OSError, json.JSONDecodeError) as e: + log(f"cannot read the policy index in {out_dir!r}: {e} — nothing can be attributed from it") + return {"out": out_dir, "entries": 0, "resolved": 0, "unknown": 0, "wrote": False, + "recorded": False, "reason": f"unreadable policies.json: {e}"} + resolved = sum(1 for r in rows if r["source"] == "policies.json") + unknown = len(rows) - resolved + stale = stale_rows(out_dir) + if stale: + log(f" ⚠ {stale} existing sidecar row(s) no longer match audit.log and were discarded — " + "the log was rebuilt or truncated since the last backfill") + + existing: dict = {} + p = out / SIDECAR + if p.is_file(): + try: + existing = json.loads(p.read_text()) + except (OSError, json.JSONDecodeError): + existing = {} + + res = {"out": out_dir, "entries": len(rows), "resolved": resolved, "unknown": unknown, + "stale_dropped": stale, "wrote": False, "recorded": False} + + if _same(rows, existing): + log(f"nothing new to attribute ({resolved} resolved, {unknown} unknown) — writing nothing") + res["reason"] = "no change" + return res + if dry_run: + log(f"[dry-run] would attribute {resolved} entry(ies) and mark {unknown} unknown") + res["reason"] = "dry run" + return res + + # NO actor, NO run_id, NO host anywhere in this file. The acceptance is that no entry gets an + # invented identity, and the way to guarantee that is to have nowhere to put one: the sidecar + # carries attribution only. Identity stays where `audit.record` stamps it — on the log. + doc = {"tool_version": __version__, "generated": runmeta.utc_now(), + "note": "J4 attribution sidecar. Maps audit.log entries to findings by the policy index " + "as it stood when this ran. Carries no actor, run_id or host: identity lives in " + "audit.log, which is append-only and never edited.", + "entries": rows} + # Atomic: write a temp file in the same directory, then rename over the target. `write_text` + # truncates first, so a crash or a full disk mid-write leaves a half-written sidecar — and this + # one is evidence that an exporter reads and a bundle ships. A partial file would be silently + # unparseable (`load` swallows a JSONDecodeError and returns nothing), so the failure mode is + # losing every frozen attribution without a word. `os.replace` is atomic within a filesystem. + tmp = p.with_suffix(".json.tmp") + tmp.write_text(json.dumps(doc, indent=2)) + os.replace(tmp, p) + log(f"wrote {p} — {resolved} attributed, {unknown} unknown") + audit.record(out_dir, _OWN_ACTION, entries=len(rows), resolved=resolved, unknown=unknown, + sidecar=SIDECAR) + res.update(wrote=True, recorded=True, path=str(p)) + return res diff --git a/src/vpcopilot/cli.py b/src/vpcopilot/cli.py index 2d19017..ac6fad0 100644 --- a/src/vpcopilot/cli.py +++ b/src/vpcopilot/cli.py @@ -669,6 +669,45 @@ def export( rprint(f"wrote [bold]{path}[/bold] · {len(events)} audit event(s)") +@app.command(name="audit-backfill") +def audit_backfill( + out: str = typer.Option("out", help="run directory to backfill"), + dry_run: bool = typer.Option(False, "--dry-run", help="report what would be attributed, write nothing"), +): + """J4: freeze the finding each audit entry belongs to, while it is still derivable. + + `audit.log` is append-only and is never edited — an entry that cannot say who made a change is + not an audit record. This writes a sidecar, `/audit-backfill.json`, that the exporter reads + beside the log. + + Worth running before a re-scan. `policies.json` is rewritten by every scan, so the mapping from + a policy name back to its finding decays — and if a later scan reuses a policy name for a + different finding, the exporter's live lookup would attribute an old entry to the WRONG one. + The sidecar records what is true now, and takes precedence afterwards. + + Nothing is invented: an entry whose finding cannot be established is marked `unknown`, which + then stops the exporter guessing. A second run changes nothing and writes nothing.""" + from rich.markup import escape + + from .backfill import backfill + + # `escape`, because a log line here legitimately starts with `[dry-run]` and Rich reads that as + # a markup tag and silently drops it — the marker telling you nothing was written is the one + # word that must not vanish. (The same pattern appears in other commands; this fixes the one + # whose messages actually contain brackets.) + res = backfill(out, dry_run=dry_run, log=lambda m: rprint(f"[dim]{escape(str(m))}[/dim]")) + body = (f"[bold]entries needing attribution[/bold]: {res['entries']}\n" + f"[bold]resolved[/bold]: {res['resolved']} " + f"[bold]unknown[/bold]: {res['unknown']}") + if res.get("stale_dropped"): + body += f"\n[yellow]{res['stale_dropped']} stale row(s) discarded[/yellow]" + if res.get("reason"): + body += f"\n[dim]{res['reason']} — nothing written[/dim]" + elif res["wrote"]: + body += f"\n[dim]wrote {res['path']}[/dim]" + rprint(Panel.fit(body, title="audit-backfill")) + + @app.command(name="patches-list") def patches_list_cmd( out: str = typer.Option("out", help="output directory"), diff --git a/src/vpcopilot/console/app.py b/src/vpcopilot/console/app.py index 4ad8fc4..5a857a1 100644 --- a/src/vpcopilot/console/app.py +++ b/src/vpcopilot/console/app.py @@ -348,6 +348,23 @@ def drift_ep(lb: str, control: str | None = None, policy: str | None = None, raise HTTPException(400, str(e)) +class BackfillReq(BaseModel): + dry_run: bool = False + + +@app.post("/api/audit-backfill") +def audit_backfill(body: BackfillReq | None = None): + """J4: freeze the finding each audit entry belongs to, into `/audit-backfill.json`. + + Operates on THIS run's out dir, never a caller-supplied path — the J2 precedent. POST rather + than GET because it writes, even though what it writes is derived rather than decided: a GET the + browser is free to prefetch should not append an audit record.""" + from ..backfill import backfill + log: list[str] = [] + res = backfill(str(OUT), dry_run=(body.dry_run if body else False), log=log.append) + return {**res, "log": log} + + @app.get("/api/audit-verify") def audit_verify(): """J2: check the bundle this run last wrote (`/audit-bundle.zip`) against its own manifest. diff --git a/src/vpcopilot/export.py b/src/vpcopilot/export.py index 20d10ff..ff2de0b 100644 --- a/src/vpcopilot/export.py +++ b/src/vpcopilot/export.py @@ -22,7 +22,7 @@ from typing import Callable -from . import __version__, audit, ledger, runmeta +from . import __version__, audit, backfill, ledger, runmeta from .sign import sign_bytes, verify_bytes @@ -48,6 +48,10 @@ def _quiet(_msg: str) -> None: # existing consumer of that action keeps working, and this one carries the extra proof (the # origin probe) that justified doing it without a human present. "escalation": "reconcile", "fix_ineffective": "reconcile", "reconcile_retire": "reconcile", + # J4 — bookkeeping about the trail itself rather than a change to a load balancer. Its own + # category, because a reviewer filtering by "what touched the tenant" should not see it, and + # falling through to "other" made it an unexplained row in the CSV. + "audit_backfill": "evidence", } # The XC control each action acted on, where the action name alone implies it. CONTROL = { @@ -73,7 +77,11 @@ def _quiet(_msg: str) -> None: # H2. Named `dependencies.json` and NOT `manifest.json`: `verify_bundle` locates # each run in an archive by every member whose name ends `manifest.json`, so an # artifact by that name would be read as a second evidence manifest. - "dependencies.json", "report.html"] + "dependencies.json", + # J4. The attribution sidecar ships WITH the log it annotates — a + # reviewer who gets one without the other cannot tell a frozen + # attribution from an invented one. + "audit-backfill.json", "report.html"] def _rj(out: Path, name: str, default): @@ -102,7 +110,8 @@ def _outcome(e: dict) -> str: # An escalation that fell through to "recorded" would read as a no-op — the exact # opposite of a band-aid that is now overdue and still live. "escalation": "escalated", "fix_ineffective": "fix_ineffective", - "reconcile_retire": "retired"}.get(e.get("action", "")) + "reconcile_retire": "retired", + "audit_backfill": "attributed"}.get(e.get("action", "")) if fixed: return fixed if e.get("unfixable"): @@ -147,13 +156,35 @@ def build_audit_events(out_dir: str = "out") -> list[dict]: led = ledger.load(out_dir) findings = {f.get("id"): f for f in _rj(out, "findings.json", [])} # a policy name is often the only link back to a finding on older entries - by_policy = {a.get("policy_name"): a.get("finding_id") for a in _rj(out, "policies.json", [])} + try: + by_policy = ledger.policy_index(out_dir) + except (OSError, json.JSONDecodeError): + # The inline copy this replaced went through `_rj`, which swallows a damaged file. A + # read-only report should not explode on one; the apply paths deliberately do. + by_policy = {} + # J4 — attribution frozen by `vpcopilot audit-backfill`, keyed by entry index and already + # verified against this log. It WINS over `by_policy` because `policies.json` is rewritten by + # every scan: after a re-scan it describes a different run, and a policy name that recurs with a + # different finding would attribute an old entry to the wrong one. Empty until a backfill runs, + # so behaviour without one is unchanged. + frozen, sidecar_ok = backfill.load_state(out_dir) events = [] - for e in entries: + for i, e in enumerate(entries): # `open_pr` wrote `finding` before every other action settled on `finding_id`; older logs - # attributed nothing at all, so fall back to the policy index. - fid = e.get("finding_id") or e.get("finding") or by_policy.get(e.get("policy")) + # attributed nothing at all, so fall back to the frozen attribution, then to the live index. + row = frozen.get(i) + fid = e.get("finding_id") or e.get("finding") + if not fid and row is not None: + # A row that says `unknown` is sticky: the backfill looked and could not establish it, + # so guessing from a NEWER policies.json would be inventing an answer it already + # declined to give. + fid = row.get("finding_id") + elif not fid and sidecar_ok: + fid = by_policy.get(e.get("policy")) + # `sidecar_ok` False means a sidecar EXISTS and will not parse: somebody froze attribution + # and it is now unreadable, so the live index is not the one these entries belong to. + # Leaving the cell blank is the honest answer; guessing produced a confidently wrong one. f, le = findings.get(fid, {}), (led.get(fid) or {}) before, after = _ba(e, "before"), _ba(e, "after") detail = {k: v for k, v in e.items() @@ -226,6 +257,13 @@ def build_manifest(out_dir: str = "out", *, members: dict | None = None) -> dict "`advisories` include entries dispositioned below_severity or capped: found and " "listed, but never sent to an agent, so no band-aid was considered for them. Read " "`funnel` for the counts, and note it reflects OSV.dev on the run date only.", + "audit-backfill.json, when present, is a DERIVED sidecar and not part of the log. It " + "maps audit entries to findings via the policy index as it stood when " + "`vpcopilot audit-backfill` ran, because that index is rewritten by every scan. It " + "carries no actor, run_id or host — identity comes only from audit.log, which is " + "append-only and shipped verbatim. A finding_id in audit.csv may therefore come from " + "this sidecar rather than from the entry itself; where the two ever disagree, the log " + "is the evidence.", ], "members": members or {}, } diff --git a/src/vpcopilot/ledger.py b/src/vpcopilot/ledger.py index 0d8e193..38d6606 100644 --- a/src/vpcopilot/ledger.py +++ b/src/vpcopilot/ledger.py @@ -165,12 +165,44 @@ def mark_retired(out_dir, finding_id: str) -> dict: return e -def find_finding_for_policy(out_dir, policy_name: str) -> str | None: - """Map a generated policy back to its finding via the scan's policies.json index.""" +def policy_index(out_dir) -> dict[str, str]: + """`{policy_name: finding_id}` from the scan's `policies.json`. + + THE one place this lookup lives. It had grown a second copy in `export.build_audit_events`, + which built the same dict inline; J4 would have made a third. Two copies of a mapping is one + copy that eventually stops matching. + + **This index is rewritten by every scan** (`pipeline._write_out`), so it describes the LATEST + run and not the run an old audit entry belongs to. That is what J4's sidecar exists to freeze — + see `backfill.py`. + + Semantics are the OLD `find_finding_for_policy`'s, exactly, because four apply-path callers + depend on them and a consolidation that quietly changes behaviour is worse than the duplication + it removes. Three differences were measured against the previous implementation and all three + are deliberately preserved here: + + - **First match wins.** A dict comprehension would let a later duplicate overwrite an earlier + one. `policies.json` should not contain duplicate names, but "should not" is not "does not", + and silently changing which finding an apply validates against is not a refactor. + - **A falsy `finding_id` is returned as-is**, not filtered out. Callers all write + `finding_id or find_finding_for_policy(...)`, so `""` and `None` behave identically to them — + but the *backfill* wants them treated as unresolved, and that belongs at its call site rather + than in a lookup four other paths share. + - **A corrupt file raises**, as it did before. The apply paths use this to decide which probe + validates a band-aid; proceeding with no finding because the index would not parse means + applying with weaker validation and no explanation. `export` catches it instead, because a + read-only report should not explode on a damaged artifact.""" p = Path(out_dir) / "policies.json" if not p.exists(): - return None + return {} + idx: dict[str, str] = {} for a in json.loads(p.read_text()): - if a.get("policy_name") == policy_name: - return a.get("finding_id") - return None + name = a.get("policy_name") + if name is not None and name not in idx: + idx[name] = a.get("finding_id") + return idx + + +def find_finding_for_policy(out_dir, policy_name: str) -> str | None: + """Map a generated policy back to its finding via the scan's policies.json index.""" + return policy_index(out_dir).get(policy_name) diff --git a/tests/test_backfill.py b/tests/test_backfill.py new file mode 100644 index 0000000..42128e7 --- /dev/null +++ b/tests/test_backfill.py @@ -0,0 +1,532 @@ +"""J4 — attribution backfill. Offline throughout: a run dir on disk, no network, no tenant.""" +from __future__ import annotations + +import json + +import pytest + +from vpcopilot import audit, backfill, export + + +def _run(tmp_path, entries, policies=None): + """A run dir with an audit log and a policy index, written the way the pipeline writes them.""" + out = tmp_path + for e in entries: + audit.record(str(out), e.pop("action"), **e) + (out / "policies.json").write_text(json.dumps(policies or [])) + return str(out) + + +POLICIES = [{"finding_id": "neg-pay-001", "control": "service_policy", + "policy_name": "deny-negative-pay"}, + {"finding_id": "sqli-login-001", "control": "waf", "policy_name": "waf-block-sqli"}] + + +# ============================================================ deriving +def test_only_what_the_policy_index_proves_is_attributed(tmp_path): + out = _run(tmp_path, [ + {"action": "create_service_policy", "policy": "deny-negative-pay"}, # derivable + {"action": "apply_waf", "policy": "waf-block-sqli"}, # derivable + {"action": "apply_rate_limit", "lb": "lab"}, # nothing to go on + {"action": "apply_bot_defense", "policy": "never-generated-here"}, # not in the index + ], POLICIES) + rows = backfill.derive(out) + got = {(r["action"], r["finding_id"], r["source"]) for r in rows} + assert ("create_service_policy", "neg-pay-001", "policies.json") in got + assert ("apply_waf", "sqli-login-001", "policies.json") in got + assert ("apply_rate_limit", None, "unknown") in got + assert ("apply_bot_defense", None, "unknown") in got + + +def test_an_entry_that_already_carries_its_finding_is_not_recorded(tmp_path): + """There is nothing to freeze: the log is the better source, and `build_audit_events` reads the + record's own copy first precisely because reconcile denormalizes it for that reason.""" + out = _run(tmp_path, [ + {"action": "apply_waf", "finding_id": "sqli-login-001", "policy": "waf-block-sqli"}, + {"action": "open_pr", "finding": "neg-pay-001"}, # the legacy field name + ], POLICIES) + assert backfill.derive(out) == [] + + +def test_the_sidecar_carries_no_identity_at_all(tmp_path): + """Acceptance: no entry gets an invented actor or run_id. Guaranteed by having nowhere to put + one — identity lives in audit.log, which is append-only and never edited.""" + out = _run(tmp_path, [{"action": "create_service_policy", "policy": "deny-negative-pay"}], + POLICIES) + backfill.backfill(out, log=lambda m: None) + doc = json.loads((tmp_path / backfill.SIDECAR).read_text()) + # keys, not a substring search — the document's own `note` explains that it carries no actor, + # and a raw-text check would trip on the explanation rather than on a field. + forbidden = {"actor", "run_id", "host"} + assert not (set(doc) & forbidden) + for row in doc["entries"]: + assert set(row) == {"i", "ts", "action", "policy", "finding_id", "source"} + assert not (set(row) & forbidden) + + +def test_the_audit_log_is_never_rewritten(tmp_path): + """The log is append-only and `record()` strips caller-supplied identity. A backfill that could + edit an entry would destroy the property that makes the log worth keeping.""" + out = _run(tmp_path, [{"action": "create_service_policy", "policy": "deny-negative-pay"}], + POLICIES) + before = (tmp_path / "audit.log").read_text() + backfill.backfill(out, log=lambda m: None) + after = (tmp_path / "audit.log").read_text() + assert after.startswith(before) # only appended to + assert len(after.splitlines()) == len(before.splitlines()) + 1 # by exactly its own record + + +# ============================================================ acceptance +def test_the_backfill_writes_its_own_audit_record(tmp_path): + out = _run(tmp_path, [{"action": "create_service_policy", "policy": "deny-negative-pay"}, + {"action": "apply_rate_limit", "lb": "lab"}], POLICIES) + res = backfill.backfill(out, log=lambda m: None) + assert res["wrote"] and res["recorded"] + own = [e for e in audit.load(out) if e["action"] == "audit_backfill"] + assert len(own) == 1 + assert own[0]["resolved"] == 1 and own[0]["unknown"] == 1 and own[0]["entries"] == 2 + assert own[0]["actor"] and own[0]["run_id"] is not None # stamped centrally, as ever + + +def test_a_second_run_is_a_no_op(tmp_path): + """Not tidiness. This function appends its OWN entry, so without the check every run would see + one more entry than the last, decide something changed, and append again — a log that grows by a + line every time anyone asks whether it needs backfilling.""" + out = _run(tmp_path, [{"action": "create_service_policy", "policy": "deny-negative-pay"}], + POLICIES) + backfill.backfill(out, log=lambda m: None) + log_after_first = (tmp_path / "audit.log").read_text() + side_after_first = (tmp_path / backfill.SIDECAR).read_text() + + second = backfill.backfill(out, log=lambda m: None) + assert second["wrote"] is False and second["recorded"] is False + assert second["reason"] == "no change" + assert (tmp_path / "audit.log").read_text() == log_after_first + assert (tmp_path / backfill.SIDECAR).read_text() == side_after_first + + third = backfill.backfill(out, log=lambda m: None) # and it stays a no-op + assert third["wrote"] is False + assert (tmp_path / "audit.log").read_text() == log_after_first + + +def test_a_new_entry_makes_the_next_run_write_again(tmp_path): + """A no-op must not mean "never runs again".""" + out = _run(tmp_path, [{"action": "create_service_policy", "policy": "deny-negative-pay"}], + POLICIES) + backfill.backfill(out, log=lambda m: None) + audit.record(out, "apply_waf", policy="waf-block-sqli") + res = backfill.backfill(out, log=lambda m: None) + assert res["wrote"] and res["resolved"] == 2 + + +def test_dry_run_writes_nothing(tmp_path): + out = _run(tmp_path, [{"action": "create_service_policy", "policy": "deny-negative-pay"}], + POLICIES) + before = (tmp_path / "audit.log").read_text() + res = backfill.backfill(out, dry_run=True, log=lambda m: None) + assert res["resolved"] == 1 and res["wrote"] is False and res["recorded"] is False + assert not (tmp_path / backfill.SIDECAR).exists() + assert (tmp_path / "audit.log").read_text() == before + + +def test_a_run_dir_with_no_audit_log_declines(tmp_path): + res = backfill.backfill(str(tmp_path), log=lambda m: None) + assert res["wrote"] is False and res["reason"] == "no audit log" + assert not (tmp_path / backfill.SIDECAR).exists() + + +# ============================================================ what it is FOR +def test_a_rescan_no_longer_erases_the_attribution(tmp_path): + """The reason this item exists. `policies.json` is rewritten by EVERY scan, so the exporter's + live lookup stops attributing older entries as soon as the run dir is scanned again.""" + out = _run(tmp_path, [{"action": "create_service_policy", "policy": "deny-negative-pay"}], + POLICIES) + assert export.build_audit_events(out)[0]["finding_id"] == "neg-pay-001" # derivable today + + backfill.backfill(out, log=lambda m: None) + (tmp_path / "policies.json").write_text(json.dumps([])) # a later scan + + ev = [e for e in export.build_audit_events(out) if e["action"] == "create_service_policy"] + assert ev[0]["finding_id"] == "neg-pay-001", "the frozen attribution did not survive the re-scan" + + +def test_a_reused_policy_name_cannot_reattribute_an_old_entry(tmp_path): + """The sharper half, and the reason the sidecar WINS over the live index rather than merely + filling gaps: a later scan that reuses a policy name for a DIFFERENT finding would make the live + lookup succeed and attribute the old entry to the wrong finding — a confident wrong answer in the + artifact whose whole job is to be trustworthy.""" + out = _run(tmp_path, [{"action": "create_service_policy", "policy": "deny-negative-pay"}], + POLICIES) + backfill.backfill(out, log=lambda m: None) + # a later scan of a different app generates the same policy name for another finding + (tmp_path / "policies.json").write_text(json.dumps( + [{"finding_id": "SOMETHING-ELSE-999", "control": "service_policy", + "policy_name": "deny-negative-pay"}])) + ev = [e for e in export.build_audit_events(out) if e["action"] == "create_service_policy"] + assert ev[0]["finding_id"] == "neg-pay-001" + assert ev[0]["finding_id"] != "SOMETHING-ELSE-999" + + +def test_unknown_is_sticky_so_a_later_index_cannot_invent_an_answer(tmp_path): + """"We looked and could not establish this" is a fact worth keeping. Falling through to a newer + `policies.json` would be inventing an answer the backfill already declined to give.""" + out = _run(tmp_path, [{"action": "apply_bot_defense", "policy": "not-in-any-index"}], POLICIES) + backfill.backfill(out, log=lambda m: None) + rows = json.loads((tmp_path / backfill.SIDECAR).read_text())["entries"] + assert rows[0]["source"] == "unknown" and rows[0]["finding_id"] is None + + (tmp_path / "policies.json").write_text(json.dumps( + [{"finding_id": "INVENTED-001", "control": "bot_defense", + "policy_name": "not-in-any-index"}])) + ev = [e for e in export.build_audit_events(out) if e["action"] == "apply_bot_defense"] + assert ev[0]["finding_id"] == "" + + +def test_without_a_backfill_the_exporter_behaves_exactly_as_before(tmp_path): + """No regression: the sidecar is absent until someone runs the command.""" + out = _run(tmp_path, [{"action": "create_service_policy", "policy": "deny-negative-pay"}, + {"action": "apply_rate_limit", "lb": "lab"}], POLICIES) + ev = {e["action"]: e for e in export.build_audit_events(out)} + assert ev["create_service_policy"]["finding_id"] == "neg-pay-001" # live lookup still used + assert ev["apply_rate_limit"]["finding_id"] == "" + + +# ============================================================ the log moving underneath it +def test_a_row_whose_entry_no_longer_matches_is_dropped_not_moved(tmp_path): + """The sidecar is keyed by index, which is stable only because the log is append-only. If the log + is rebuilt or truncated, the index still resolves — to a DIFFERENT entry. Verifying (ts, action) + means losing an attribution rather than moving it onto the wrong record.""" + out = _run(tmp_path, [{"action": "create_service_policy", "policy": "deny-negative-pay"}, + {"action": "apply_waf", "policy": "waf-block-sqli"}], POLICIES) + backfill.backfill(out, log=lambda m: None) + assert len(backfill.load(out)) == 2 + + lines = (tmp_path / "audit.log").read_text().splitlines() + (tmp_path / "audit.log").write_text("\n".join(lines[1:]) + "\n") # first entry removed + assert backfill.stale_rows(out) > 0 + for i, row in backfill.load(out).items(): + assert (audit.load(out)[i]["ts"], audit.load(out)[i]["action"]) == (row["ts"], row["action"]) + + +def test_a_corrupt_sidecar_does_not_fall_through_to_a_wrong_guess(tmp_path): + """This test used to pin only "does not crash", and an adversarial review caught why that was + not enough: its fixture left `policies.json` mapping the same policy to the same finding, so the + live fallback happened to agree. With a re-scan that reused the name, the same code path produced + a confidently WRONG attribution — in a bundle that then SHA-256s and signs it as evidence. + + Absent and unreadable are different answers. No sidecar means nobody froze anything, so the live + index is the best available guess. A sidecar that exists and will not parse means somebody DID + freeze attribution and it is now unreadable — and the live index is by definition not the one + those entries belong to.""" + out = _run(tmp_path, [{"action": "create_service_policy", "policy": "deny-negative-pay"}], + POLICIES) + backfill.backfill(out, log=lambda m: None) + # a later scan reuses the policy name for a DIFFERENT finding + (tmp_path / "policies.json").write_text(json.dumps( + [{"policy_name": "deny-negative-pay", "finding_id": "WRONG-999"}])) + assert export.build_audit_events(out)[-1]["finding_id"] == "neg-pay-001" # frozen wins + + (tmp_path / backfill.SIDECAR).write_text("{ not json") + rows, ok = backfill.load_state(out) + assert rows == {} and ok is False + got = export.build_audit_events(out)[-1]["finding_id"] + assert got == "", f"a torn sidecar guessed {got!r} from an index that is not these entries'" + + # …and with NO sidecar at all the live lookup is used exactly as it always was + (tmp_path / backfill.SIDECAR).unlink() + assert backfill.load_state(out) == ({}, True) + assert export.build_audit_events(out)[-1]["finding_id"] == "WRONG-999" + + (tmp_path / backfill.SIDECAR).write_text("{ not json") + assert backfill.backfill(out, log=lambda m: None)["wrote"] is True # and it recovers + + +# ============================================================ one lookup, not three +def test_the_policy_lookup_lives_in_exactly_one_place(): + """J4 would have been the third copy. `ledger.policy_index` is the one; `find_finding_for_policy` + and the exporter both go through it.""" + import inspect + + from vpcopilot import ledger + assert "policy_index(out_dir).get(policy_name)" in inspect.getsource( + ledger.find_finding_for_policy) + src = inspect.getsource(export.build_audit_events) + assert "ledger.policy_index(out_dir)" in src + assert 'a.get("policy_name"): a.get("finding_id")' not in src # the inline copy is gone + + +@pytest.mark.parametrize("label,doc,want", [ + ("normal", [{"policy_name": "p", "finding_id": "f-1"}], "f-1"), + # FIRST match wins. A dict comprehension lets a later duplicate overwrite an earlier one, which + # silently changes which finding an apply validates against. + ("duplicate names", [{"policy_name": "p", "finding_id": "f-1"}, + {"policy_name": "p", "finding_id": "f-2"}], "f-1"), + ("name, no finding", [{"policy_name": "p"}], None), + ("finding is null", [{"policy_name": "p", "finding_id": None}], None), + # A falsy finding_id comes back AS-IS rather than filtered to None: the backfill wants it + # treated as unresolved, but that belongs at its call site, not in a lookup four paths share. + ("finding is empty", [{"policy_name": "p", "finding_id": ""}], ""), + ("empty list", [], None), +]) +def test_the_shared_index_preserves_the_old_semantics_exactly(tmp_path, label, doc, want): + """No regression for the four production callers of `find_finding_for_policy` (refiner.py and + apply.py x3). Measured against the previous implementation: consolidating two copies into one + silently changed THREE behaviours — duplicate-name precedence, falsy finding_ids, and what + happens to a corrupt file — before this pinned them.""" + from vpcopilot import ledger + (tmp_path / "policies.json").write_text(json.dumps(doc)) + assert ledger.find_finding_for_policy(str(tmp_path), "p") == want + + +def test_a_corrupt_index_still_raises_for_the_apply_paths(tmp_path): + """The apply paths use this to decide which probe validates a band-aid. Returning None because + the index would not parse means applying with weaker validation and no explanation, so it raises + as it always did — while the read-only exporter catches it instead.""" + from vpcopilot import ledger + (tmp_path / "policies.json").write_text("{ corrupt") + with pytest.raises(json.JSONDecodeError): + ledger.find_finding_for_policy(str(tmp_path), "p") + # …and the exporter is unaffected by the same file + assert export.build_audit_events(str(tmp_path)) == [] + + +def test_a_missing_index_is_empty_for_everyone(tmp_path): + from vpcopilot import ledger + assert ledger.find_finding_for_policy(str(tmp_path / "nowhere"), "x") is None + assert ledger.policy_index(str(tmp_path / "nowhere")) == {} + + +# ============================================================ evidence + surfaces +def test_the_sidecar_ships_in_the_evidence_bundle(tmp_path): + """A reviewer who gets the log without the sidecar cannot tell a frozen attribution from an + invented one.""" + import zipfile + out = _run(tmp_path, [{"action": "create_service_policy", "policy": "deny-negative-pay"}], + POLICIES) + backfill.backfill(out, log=lambda m: None) + z = zipfile.ZipFile(tmp_path / "b.zip", "w") + export._add_run(z, out) + z.close() + with zipfile.ZipFile(tmp_path / "b.zip") as zz: + assert backfill.SIDECAR in zz.namelist() + manifest = json.loads(zz.read("manifest.json")) + assert backfill.SIDECAR in manifest["members"] # and it is digested like every member + + +def test_both_surfaces_reach_the_same_module_function(tmp_path, monkeypatch): + import inspect + + from fastapi.testclient import TestClient + + from vpcopilot import cli + from vpcopilot.console import app as A + assert "from .backfill import backfill" in inspect.getsource(cli.audit_backfill) + + out = _run(tmp_path, [{"action": "create_service_policy", "policy": "deny-negative-pay"}], + POLICIES) + monkeypatch.setattr(A, "OUT", tmp_path) + r = TestClient(A.app).post("/api/audit-backfill", json={"dry_run": True}) + assert r.status_code == 200 + assert r.json()["resolved"] == 1 and r.json()["wrote"] is False + assert not (tmp_path / backfill.SIDECAR).exists() + + r2 = TestClient(A.app).post("/api/audit-backfill", json={}) + assert r2.json()["wrote"] is True + assert (tmp_path / backfill.SIDECAR).is_file() + assert backfill.load(out) + + +def test_the_console_endpoint_takes_no_caller_supplied_path(): + """The J2 precedent: an endpoint that accepted a path would be an arbitrary-file writer, and + localhost is not a reason to ship one.""" + from vpcopilot.console.app import BackfillReq + assert set(BackfillReq.model_fields) == {"dry_run"} + + +@pytest.mark.parametrize("action", ["audit_backfill"]) +def test_the_backfills_own_entries_are_never_backfilled(tmp_path, action): + out = _run(tmp_path, [{"action": "create_service_policy", "policy": "deny-negative-pay"}], + POLICIES) + backfill.backfill(out, log=lambda m: None) + assert all(r["action"] != action for r in backfill.derive(out)) + + +def test_the_new_action_is_categorised_rather_than_falling_through_to_other(tmp_path): + """A reviewer filters the CSV by category. `audit_backfill` is bookkeeping about the trail, not a + change to a load balancer, so it gets its own category — falling through to "other"/"recorded" + made it an unexplained row.""" + out = _run(tmp_path, [{"action": "create_service_policy", "policy": "deny-negative-pay"}], + POLICIES) + backfill.backfill(out, log=lambda m: None) + ev = [e for e in export.build_audit_events(out) if e["action"] == "audit_backfill"] + assert ev[0]["category"] == "evidence" and ev[0]["outcome"] == "attributed" + + +def test_a_forged_sidecar_cannot_move_anything_the_log_stamped(tmp_path): + """The limit, stated because a sidecar is an ordinary file while the log is append-only. A forged + sidecar reaches `finding_id` and the columns joined THROUGH it — no further. Everything the log + stamped is unreachable, and `findings.json`/`ledger.json` already drive those same joins, so this + adds no trust assumption the export did not already make.""" + out = _run(tmp_path, [{"action": "refine_apply", "policy": "deny-negative-pay", "lb": "lab", + "passed": True}], POLICIES) + backfill.backfill(out, log=lambda m: None) + before = {e["action"]: e for e in export.build_audit_events(out)}["refine_apply"] + + doc = json.loads((tmp_path / backfill.SIDECAR).read_text()) + doc["entries"][0]["finding_id"] = "sqli-login-001" + (tmp_path / backfill.SIDECAR).write_text(json.dumps(doc)) + after = {e["action"]: e for e in export.build_audit_events(out)}["refine_apply"] + + assert after["finding_id"] == "sqli-login-001" # it CAN move the attribution… + for stamped in ("ts", "actor", "host", "run_id", "action", "outcome", "lb", "tool_version"): + assert after[stamped] == before[stamped], stamped # …and nothing the log stamped + + +def test_the_documented_limit_is_actually_written_down(): + """This project states its limits rather than leaving them to be discovered — the J1 precedent + ("the signature attests who exported this bundle, not that the log is truthful").""" + from pathlib import Path + txt = (Path(__file__).resolve().parents[1] / "docs/AUDIT.md").read_text() + assert "forged sidecar" in txt + assert "no trust assumption that the export did not already make" in txt + assert "the log is the evidence" in txt + + +def test_a_second_run_after_a_rescan_does_not_destroy_the_frozen_attribution(tmp_path): + """Found by adversarial review — two reviewers independently, and it defeated the entire item. + `derive` recomputed every row from the CURRENT policies.json, which is precisely what the sidecar + is a defence against: run once, re-scan, run again, and every frozen attribution collapses to + `unknown`. Because `unknown` is sticky, the loss was permanent — the second use of the feature + undid the first.""" + out = _run(tmp_path, [{"action": "create_service_policy", "policy": "deny-negative-pay"}], + POLICIES) + backfill.backfill(out, log=lambda m: None) + (tmp_path / "policies.json").write_text(json.dumps([])) # a later scan + + second = backfill.backfill(out, log=lambda m: None) + assert second["wrote"] is False and second["reason"] == "no change" + rows = json.loads((tmp_path / backfill.SIDECAR).read_text())["entries"] + assert rows[0]["finding_id"] == "neg-pay-001" + ev = [e for e in export.build_audit_events(out) if e["action"] == "create_service_policy"] + assert ev[0]["finding_id"] == "neg-pay-001" + + +def test_a_frozen_row_is_never_recomputed_even_when_the_index_would_disagree(tmp_path): + """Monotonic in the same way as the log it annotates: rows are added, never revised. A later + index that would map the same policy to a DIFFERENT finding must not rewrite history.""" + out = _run(tmp_path, [{"action": "create_service_policy", "policy": "deny-negative-pay"}], + POLICIES) + backfill.backfill(out, log=lambda m: None) + (tmp_path / "policies.json").write_text(json.dumps( + [{"policy_name": "deny-negative-pay", "finding_id": "SOMETHING-ELSE-999"}])) + res = backfill.backfill(out, log=lambda m: None) + assert res["wrote"] is False + rows = json.loads((tmp_path / backfill.SIDECAR).read_text())["entries"] + assert rows[0]["finding_id"] == "neg-pay-001" + + +def test_new_entries_are_still_derived_after_a_rescan(tmp_path): + """Monotonic must not mean frozen shut: an entry with no row yet is derived normally.""" + out = _run(tmp_path, [{"action": "create_service_policy", "policy": "deny-negative-pay"}], + POLICIES) + backfill.backfill(out, log=lambda m: None) + audit.record(out, "apply_waf", policy="waf-block-sqli") + res = backfill.backfill(out, log=lambda m: None) + assert res["wrote"] and res["resolved"] == 2 + got = {r["policy"]: r["finding_id"] + for r in json.loads((tmp_path / backfill.SIDECAR).read_text())["entries"]} + assert got == {"deny-negative-pay": "neg-pay-001", "waf-block-sqli": "sqli-login-001"} + + +def test_deleting_the_sidecar_is_the_way_to_force_a_fresh_derivation(tmp_path): + """The escape hatch, and it is deliberately an explicit act rather than a side effect of running + the command twice.""" + out = _run(tmp_path, [{"action": "create_service_policy", "policy": "deny-negative-pay"}], + POLICIES) + backfill.backfill(out, log=lambda m: None) + (tmp_path / backfill.SIDECAR).unlink() + (tmp_path / "policies.json").write_text(json.dumps([])) + res = backfill.backfill(out, log=lambda m: None) + assert res["wrote"] and res["resolved"] == 0 and res["unknown"] == 1 + + +def test_the_sidecar_is_written_atomically(tmp_path, monkeypatch): + """It is evidence an exporter reads and a bundle ships. `write_text` truncates first, so a crash + mid-write leaves a half-written file — and `load` swallows the resulting JSONDecodeError, so the + failure mode is losing every frozen attribution without a word.""" + import inspect + assert "os.replace" in inspect.getsource(backfill.backfill) + + out = _run(tmp_path, [{"action": "create_service_policy", "policy": "deny-negative-pay"}], + POLICIES) + backfill.backfill(out, log=lambda m: None) + assert (tmp_path / backfill.SIDECAR).is_file() + assert not list(tmp_path.glob("*.tmp")) # no debris left behind + assert json.loads((tmp_path / backfill.SIDECAR).read_text())["entries"] + + +def test_the_bundle_caveats_describe_the_sidecar(tmp_path): + """The bundle states what it does NOT prove. A derived sidecar that can supply a finding_id is + exactly the kind of thing a reviewer must be told about.""" + out = _run(tmp_path, [{"action": "create_service_policy", "policy": "deny-negative-pay"}], + POLICIES) + backfill.backfill(out, log=lambda m: None) + cav = " ".join(export.build_manifest(out)["caveats"]) + assert "audit-backfill.json" in cav + assert "no actor, run_id or host" in cav + assert "the log is the evidence" in cav + + +def test_a_corrupt_policy_index_declines_rather_than_tracebacking(tmp_path): + """`ledger.policy_index` raises on a corrupt file DELIBERATELY, so the apply paths fail loudly + rather than validating a band-aid against no probe. This command is not an apply: an unreadable + index means it cannot establish anything, which is a decline — not a CLI traceback and an HTTP + 500 out of the console.""" + out = _run(tmp_path, [{"action": "create_service_policy", "policy": "deny-negative-pay"}]) + (tmp_path / "policies.json").write_text("{ corrupt") + res = backfill.backfill(out, log=lambda m: None) + assert res["wrote"] is False and res["recorded"] is False + assert "unreadable policies.json" in res["reason"] + assert not (tmp_path / backfill.SIDECAR).exists() + + from fastapi.testclient import TestClient + from vpcopilot.console import app as A + import vpcopilot.console.app as mod + old = mod.OUT + try: + mod.OUT = tmp_path + r = TestClient(A.app).post("/api/audit-backfill", json={}) + assert r.status_code == 200 and r.json()["wrote"] is False + finally: + mod.OUT = old + + +def test_the_dry_run_marker_survives_rich_markup(): + """Rich reads `[dry-run]` as a markup tag and silently drops it — the one word telling you + nothing was written is exactly the one that must not vanish.""" + import inspect + + from vpcopilot import cli + src = inspect.getsource(cli.audit_backfill) + assert "escape(" in src, "log lines reach rprint unescaped" + + from rich.console import Console + import io as _io + buf = _io.StringIO() + Console(file=buf, width=200, no_color=True).print( + "[dim]" + __import__("rich.markup", fromlist=["escape"]).escape( + "[dry-run] would attribute 3 entry(ies)") + "[/dim]") + assert "[dry-run]" in buf.getvalue() + + +def test_duplicate_policy_names_resolve_first_wins_everywhere(tmp_path): + """The two copies this consolidation removed DISAGREED: the exporter's inline dict was last-wins, + `find_finding_for_policy` was first-wins. Preserving both is impossible, so first-wins is the + deliberate unification — it matches the apply path, which is the safety-critical consumer. + `policies.json` should never contain duplicates; this pins what happens when it does.""" + out = _run(tmp_path, [{"action": "create_service_policy", "policy": "dup"}], + [{"policy_name": "dup", "finding_id": "FIRST"}, + {"policy_name": "dup", "finding_id": "LAST"}]) + from vpcopilot import ledger + assert ledger.find_finding_for_policy(out, "dup") == "FIRST" + assert export.build_audit_events(out)[0]["finding_id"] == "FIRST"