Repository navigation
fix(eventing): make audit mode observable - #902
Conversation
`EB_VERIFY_KEYSET_PATH` set with `EB_REQUIRE_RESPONSE_SIGNATURE=false` is documented as the step where an operator watches the reject rate before enforcing. It verified, returned a verdict, and recorded nothing: the reason string was computed at the call site and discarded, so there was no reject rate to watch and the two-flag rollout in DESIGN_PHASE2.md could not be performed. `response_decision` now returns `(accept, reason, verified)`. Audit mode and a clean verification both return `accept=True`, so telling them apart was previously only possible by matching the reason prose -- which exists to be read by a human in a log line, not parsed. The unsigned non-terminal passthrough reports `verified=True` because it is accepted by the signing policy rather than in spite of a failure, so a counter does not log every streamed frame of every genuine run. The consumer counts those acceptances in `audit_failed`, alongside the existing `rejected`, and logs one line each. The two counters partition the population: with enforcement on, a forgery lands in `rejected` and `audit_failed` stays 0. A verifier that raises while enforcement is off now counts too -- it is still an unverified acceptance. No metrics endpoint is added: `/v0/selftest` is a TCP reachability prober, there is no `/metrics`, and `rejected` is already a property read by tests. Fixes #886 Assisted-By: Claude Code Signed-off-by: Mariusz Sabath <mrsabath@gmail.com>
aslom
left a comment
There was a problem hiding this comment.
Summary
This closes a real gap: response_decision computed a verdict that the caller
structurally could not act on in audit mode, so the two-flag rollout in
DESIGN_PHASE2.md §4.4 was unperformable. Returning verified as a third element
rather than having callers match the reason prose is the right call, and the
accept and not verified partition holds — I checked each branch, and there is no
input where enforcement would reject but audit mode fails to count it, or the
reverse.
I confirmed the counters partition rather than double-count: the only paths that
set verified=False are the final return (not require), why, False and the
except handler, and both yield ok=False under enforcement. The audit log line
mirrors the existing reject line's correlation-id fallback exactly, so operator
output stays consistent.
Two non-blocking notes, both the same root cause — the ks is None branch is the
one place verified=False means "not checked" rather than "a check failed", which
is what obliges the caller to carry a redundant guard — plus one stale row in
DESIGN_PHASE2.md §8.1 that this PR makes untrue.
Author: mrsabath (MEMBER — maintainer)
Areas reviewed: Python (eventbridge consumer, shared signing), tests, design docs
Agent/IDE config (.claude/.vscode): none
Commits: 1 commit, all signed-off: yes
CI status: passing (12/12 — DCO, CodeQL, Trivy, eventing-test, lint, test-startup)
Docs: DESIGN_PHASE2.md §8.1 now contradicts the code
Not inline, because the file is not in the diff.
eventing/agentdocs/DESIGN_PHASE2.md:479 still lists this as a live gap:
| "A keyset alone verifies and logs" | Verifies and returns a verdict. Nothing is logged or counted, so the observable reject rate the two-flag rollout depends on does not exist. | #886 |
That row is exactly what this PR fixes, and the PR body quotes §4.4 as the
justification — so leaving it reading as present-tense fact makes the document
argue against its own implementation. §8's own convention (lines 469–473) is not to
delete the record but to say so explicitly:
where a later phase supersedes an earlier one it says so explicitly rather than
editing in place, and a correction that deletes the original intent loses the more
useful half.
So the fix is to mark the row resolved by this PR, not to drop it.
Second, smaller: the "fifth" gap below that table quotes the call site verbatim as
kafka_in.pyhasok, why = True, "not checked"behindif self._keyset is not None
which this PR changes to ok, why, verified = True, "not checked", True. The gap
itself is untouched and out of scope here, but the quotation no longer matches the
file.
Review: #902 (approved), two suggestions — taken together, because the second is resolved by the first. `response_decision(ks=None)` returned `verified=False`, the one branch where False meant "nothing was checked" rather than "a check failed". Every other acceptance reports True — a clean verify, and the unsigned non-terminal passthrough ("accepted by policy, not in spite of a failure") — and by that same reading a keyset not being configured is also an acceptance by policy: there is nothing to enforce and nothing that could fail it, so the audit-mode population is genuinely empty. That asymmetry was what forced kafka_in.py to carry the `self._keyset is not None` clause on its audit condition — a caller-side guard that exists to be forgotten. With `ks is None` returning True, `accept and not verified` IS the audit population unconditionally, and the clause is gone. The docstring's "excluded by the caller's own check" sentence is replaced by the invariant it was working around. The no-keyset test now pins the contract (`ok and verified`), and the no-keyset consumer test still holds: audit_failed stays 0 and nothing logs, because the unconfigured branch reports verified=True — via the invariant now, not via a guard. eventing suite: 902 passed, 7 skipped. ruff clean. Assisted-By: Claude (Anthropic AI) <noreply@anthropic.com> Signed-off-by: Mariusz Sabath <mrsabath@gmail.com>
What
Audit mode now logs and counts what enforcement would have rejected.
Fixes #886.
Why
DESIGN_PHASE2.md:359promises:signing.response_decision(event, ks, require=False)returned(True, "<reason>")foran event that failed verification, and the caller discarded
why. Only theif not ok:branch logged, and audit mode takes the
ok=Truepath — so no log line, no counter, noreject rate. The documented rollout step could not be performed.
How
response_decisionreturns(accept, reason, verified).Audit mode and a clean pass both return
accept=True, so distinguishing them meantmatching the reason prose — which exists to be read by a human in a log line, not
parsed.
One subtlety worth a reviewer's eye: the unsigned non-terminal passthrough reports
verified=True, because it is accepted by the signing policy rather than in spiteof a failure.
emit()signs terminal events only, so counting those would log everystreamed frame of every genuine run and bury the signal.
Consumergainsaudit_failedbeside the existingrejected, and one log line perevent:
The two counters partition the population rather than double-counting it: with
enforcement on, a forgery lands in
rejectedandaudit_failedstays 0. A verifierthat raises while enforcement is off now counts as well — it is still an unverified
acceptance.
Startup already announces the mode (
response verification ON (audit only)), so it wasdiscoverable; it just produced no output afterwards.
Not in scope
No HTTP metrics surface. There is no
/metrics,/v0/selftestis a TCP reachabilityprober for the public base URL, and the existing
rejectedcounter is a property readby tests. Wiring a gauge through
selftestwould be off-topic.Verification
tests/test_kafka_in.py+tests/test_keyset.py— 74 passedeventing/suite on this base — 902 passed, 7 skipped (4 new tests)ruff checkcleanphase == "result", nosignature_rejected), which is the other half of §4.4's promisebridge with no keyset configured all leave
audit_failed == 0and log nothing