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"