AUDIT-WP-0004-T04, closing the workplan. Decision (Bernd): default to redaction, allow rejection per sender. Losing an audit record over one field is worse than storing it masked, but a higher-assurance channel must be able to refuse rather than mask. secret_policy is set per sender identity in AUDIT_CORE_SENDERS and defaults to redact. Detection now covers the whole payload at any depth, including lists, rather than only the top level of data. Under redaction the value is masked and the key is preserved: dropping the key would hide that the sender transmitted the field at all, which is exactly what an operator needs in order to stop it. The stored record carries details.redaction with policy and affected paths, so a reader never has to infer whether what they see is what was sent. Idempotency is unaffected - the payload hash is taken over the original request body, so redaction is deterministic and a resubmission still reconciles as a duplicate. Both outcomes are counted durably by sender, source, action and field path, exposed at GET /v1/secret-findings. Per-path aggregation is the point: the actionable unit is "stop emitting data.auth.token on membership.added", not "there were 47 redactions". Counters survive restart because the fix they drive lives in another service. Contract doc updated to match. Tests 46 -> 50. WP-0004 is finished. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
303 lines
13 KiB
Markdown
303 lines
13 KiB
Markdown
---
|
|
id: AUDIT-WP-0004
|
|
type: workplan
|
|
title: "Make the event receiver correct and operable under load"
|
|
domain: infotech
|
|
repo: audit-core
|
|
status: finished
|
|
owner: codex
|
|
topic_slug: netkingdom
|
|
created: "2026-08-10"
|
|
updated: "2026-08-10"
|
|
state_hub_workstream_id: "f3345e90-f466-4184-b149-9b0be92ffec8"
|
|
---
|
|
|
|
# AUDIT-WP-0004 - receiver correctness and hardening
|
|
|
|
## Goal
|
|
|
|
Close the gap between what the receiver claims and what it does, and make it
|
|
survive a real sender. This is the work that must land before audit-core is
|
|
worth deploying.
|
|
|
|
A pre-deploy review of the WP-0003 implementation found that the ingestion
|
|
path never calls the `AuditBackend` contract, that several failure modes
|
|
escape the error handler and drop the connection, and that two isolation
|
|
properties the workplan claims as done are not implemented. None of these
|
|
depend on the choice of storage engine, so this workplan runs in parallel
|
|
with RAPP-POSTGRES-WP-0002 and gates AUDIT-WP-0005.
|
|
|
|
## Boundaries
|
|
|
|
This workplan owns the receiver's correctness, error semantics, and operable
|
|
surface. It does not own deployment, the production storage engine, or the
|
|
live failure matrix — those are AUDIT-WP-0005.
|
|
|
|
Work here stays storage-agnostic: the existing SQLite store remains the
|
|
development and test backend, and the Postgres implementation lands in
|
|
WP-0005 against the interface this workplan fixes.
|
|
|
|
## T01 - Route ingestion through the audit backend contract
|
|
|
|
```task
|
|
id: AUDIT-WP-0004-T01
|
|
status: done
|
|
priority: high
|
|
state_hub_task_id: "9b194ebc-f853-48c5-a65b-88b0eae3d7a2"
|
|
```
|
|
|
|
`ingestion.py` writes records into SQLite directly and never calls
|
|
`AuditBackend.emit()`. WP-0002 built a pluggable backend contract with a
|
|
declared `RetentionPolicy`; WP-0003 built an ingestion path that bypasses it.
|
|
A 202 currently means "a row exists in a file", against a store that declares
|
|
no custody class at all.
|
|
|
|
Make the ingestion path write through the backend contract, so that accepting
|
|
an event means a backend with a declared retention policy acknowledged it.
|
|
Idempotency state and custody write must not be able to diverge — an event
|
|
recorded as seen but not durably stored is the failure this whole service
|
|
exists to prevent.
|
|
|
|
Add a readiness condition that refuses to serve if the configured backend
|
|
declares `durable=False`, so the development file backend can never silently
|
|
become the production sink.
|
|
|
|
Done when a successful response is backed by a durable backend
|
|
acknowledgment, and the mock backend cannot be used to serve production
|
|
traffic.
|
|
|
|
Done 2026-08-10: added `IdempotentAuditBackend` to the contract — idempotency
|
|
lives inside the backend so custody and duplicate-detection share one
|
|
transaction and cannot diverge. `SQLiteAuditBackend` implements it with WAL,
|
|
`synchronous=FULL`, and a busy timeout. Ingestion refuses any backend
|
|
declaring `durable=False`, so the mock file backend cannot serve.
|
|
|
|
The atomicity claim was tested rather than asserted, and the first
|
|
implementation failed: with one shared connection, 16 racing submissions of
|
|
one event told **two** callers they were first. Fixed with per-thread
|
|
connections and `BEGIN IMMEDIATE` around the insert/read pair. Now covered by
|
|
`test_concurrent_duplicates_produce_exactly_one_record`.
|
|
|
|
## T02 - Fix error semantics and failure handling
|
|
|
|
```task
|
|
id: AUDIT-WP-0004-T02
|
|
status: done
|
|
priority: high
|
|
state_hub_task_id: "b193acaf-9c0e-411a-921b-13282bee8325"
|
|
```
|
|
|
|
The handler catches `ValueError`, `TypeError`, `KeyError`, and
|
|
`JSONDecodeError`. Storage errors do not inherit from those:
|
|
`sqlite3.OperationalError` on lock contention and `sqlite3.IntegrityError` on
|
|
the race between the existence check and the insert both escape with
|
|
`start_response` never called. The auth check sits outside the try block, so a
|
|
non-ASCII `Authorization` header raises `TypeError` from
|
|
`hmac.compare_digest` and crashes the request before any handler runs.
|
|
|
|
Add a catch-all that always produces a response, map storage and unexpected
|
|
failures to 503 or 500 rather than letting them surface as dropped
|
|
connections, and move credential comparison inside the guarded path.
|
|
|
|
Correct the status codes: a duplicate event ID carrying a different payload is
|
|
a conflict and must return 409, not 400. This distinction drives the sender's
|
|
retry-versus-dead-letter decision, so the full set of responses — accepted,
|
|
duplicate, conflict, rejected, unavailable — needs to be documented as a
|
|
contract with the retry semantics each implies.
|
|
|
|
Done when no request path can terminate without a response, and every status
|
|
code the receiver returns is deliberate and documented.
|
|
|
|
Done 2026-08-10: catch-all wrapper guarantees a response on every path.
|
|
Conflict is now 409, backend unavailability 503, unexpected faults 500.
|
|
Credential comparison moved inside the guarded path and a non-ASCII
|
|
`Authorization` header is a 401 rather than a crash. The full response
|
|
contract with its retry semantics is documented in the `ingestion` module
|
|
docstring.
|
|
|
|
## T03 - Enforce the isolation properties already claimed
|
|
|
|
```task
|
|
id: AUDIT-WP-0004-T03
|
|
status: done
|
|
priority: high
|
|
state_hub_task_id: "c29be4e7-4c2e-47f0-9d37-7d72061274ee"
|
|
```
|
|
|
|
WP-0003 T01 and T02 both record tenant isolation and cross-tenant claim
|
|
rejection as done. `normalize()` accepts whatever `tenant` string the caller
|
|
sends and never checks it against the authenticated identity; no test covers
|
|
it. Separately, `source` is hardcoded to `"user-engine"`, so the allowlist is
|
|
unbound to the credential — any holder of the token can claim to be
|
|
user-engine, and a second sender would require a code change.
|
|
|
|
Bind the authenticated identity to the tenants and sources it may write for,
|
|
and reject claims outside that binding. Drive the binding from configuration
|
|
rather than literals.
|
|
|
|
Done when a credential scoped to one tenant is refused when it claims
|
|
another, and that refusal is covered by a test.
|
|
|
|
Done 2026-08-10: `audit_core.senders` binds each credential to the sources and
|
|
tenants it may assert, configured via `AUDIT_CORE_SENDERS` rather than
|
|
literals. Each identity holds a list of tokens, so rotation publishes the
|
|
replacement alongside the incumbent and needs no delivery gap. Authentication
|
|
compares every candidate token regardless of match position. Read is a
|
|
separate privilege from write. The legacy single-token env var still works but
|
|
grants every tenant and says so.
|
|
|
|
## T04 - Settle redaction policy
|
|
|
|
```task
|
|
id: AUDIT-WP-0004-T04
|
|
status: done
|
|
priority: medium
|
|
state_hub_task_id: "5cf5c412-965a-4a56-aeef-e965f2861c51"
|
|
```
|
|
|
|
`_contains_secret` rejects the entire event when any key name in `data`
|
|
contains a secret-shaped fragment, and never inspects the envelope. WP-0003
|
|
T01 describes "redacted data" and the failure matrix tests redaction, but the
|
|
implementation only rejects.
|
|
|
|
Decide whether a secret-shaped field is a rejection or a redaction, and note
|
|
the consequence: rejection means a legitimate outbox event containing a field
|
|
named `token_count` is dead-lettered at the sender, and the audit trail loses
|
|
the event entirely. Extend detection to the envelope, and consider value shape
|
|
rather than key name alone.
|
|
|
|
Whatever is chosen, the contract document and the sender's expectations must
|
|
agree with the code.
|
|
|
|
Done when the redaction rule is explicit, applied to the whole payload, and
|
|
consistent between the contract and the implementation.
|
|
|
|
Decided 2026-08-10 (Bernd): **default to redaction, allow rejection per
|
|
sender.** Losing an audit record over one field is worse than storing it
|
|
masked, but a higher-assurance channel must be able to refuse rather than
|
|
mask. `secret_policy` is set per sender identity in `AUDIT_CORE_SENDERS`,
|
|
defaulting to `redact`.
|
|
|
|
Done 2026-08-10: `audit_core.redaction` detects secret-shaped keys at any
|
|
depth across the whole payload, including lists. Under redaction the value is
|
|
masked and the **key is preserved** — dropping it would hide that the sender
|
|
transmitted the field at all, which is what the operator needs in order to
|
|
stop it. The stored record carries `details.redaction` with the policy and the
|
|
affected paths, so a reader never has to infer whether what they see is what
|
|
was sent. Idempotency is unaffected: the payload hash is over the original
|
|
body, so redaction is deterministic and replay still reconciles.
|
|
|
|
Both outcomes are counted durably by sender, source, action and **field
|
|
path**, exposed at `GET /v1/secret-findings`. Per-path aggregation is the
|
|
point: the actionable unit is "stop emitting `data.auth.token` on
|
|
`membership.added`", not "there were 47 redactions". Counters survive restart
|
|
because the fix they drive lives in another service. A non-empty list is a
|
|
backlog item for the sender, not a steady state.
|
|
|
|
Contract updated to match; tests 46 -> 50.
|
|
|
|
## T05 - Provide the operator read surface
|
|
|
|
```task
|
|
id: AUDIT-WP-0004-T05
|
|
status: done
|
|
priority: high
|
|
state_hub_task_id: "006bc4ca-de36-4152-afae-0eef2a402e73"
|
|
```
|
|
|
|
There is no way to read anything back. The failure matrix requires dead-letter
|
|
visibility, operator replay, and correlation lookup; none exist, so that
|
|
evidence cannot be produced regardless of how the deployment goes.
|
|
|
|
Build lookup by event ID and by correlation ID, a dead-letter view of rejected
|
|
events with their rejection reason, and an operator replay path. Replay must
|
|
be idempotent against the same durable store, so a replayed event reconciles
|
|
with the original rather than creating a second custody record.
|
|
|
|
Read access is a separate privilege from write access — a sender credential
|
|
must not be able to read the audit trail back.
|
|
|
|
Done when an operator can trace one correlation ID through the system and
|
|
replay a specific event without creating a duplicate.
|
|
|
|
Done 2026-08-10 (replay deferred, see below): `GET /v1/events/{id}`,
|
|
`GET /v1/events?correlation_id=`, and `GET /v1/dead-letters`, all gated on a
|
|
read privilege a sender credential does not hold. Rejections are now recorded
|
|
rather than silently dropped.
|
|
|
|
One design point worth keeping: an event rejected *for carrying secret-shaped
|
|
material* has its payload withheld from the dead-letter record. Storing it
|
|
would write that material into the audit store, which is what the rejection
|
|
exists to prevent. Reason and payload hash are retained so the event is still
|
|
traceable.
|
|
|
|
Replay is deliberately not implemented here. Idempotent replay is a property
|
|
of the durable store, and building it against SQLite would produce a second
|
|
implementation to discard — it lands with the Postgres backend in
|
|
AUDIT-WP-0005-T01, where `accept()` already gives it the semantics it needs.
|
|
|
|
## T06 - Serving layer and observability
|
|
|
|
```task
|
|
id: AUDIT-WP-0004-T06
|
|
status: done
|
|
priority: high
|
|
state_hub_task_id: "348c2f4c-3ab0-46c7-9ddd-b198122f58ed"
|
|
```
|
|
|
|
`wsgiref.simple_server` is single-threaded with no request timeout, no
|
|
graceful shutdown, and no access log. One slow client blocks every sender.
|
|
It also makes the failure matrix meaningless: "receiver correctly reported
|
|
unavailable" and "the server stalled" are indistinguishable.
|
|
|
|
Move to a production WSGI server with explicit worker and timeout
|
|
configuration and graceful shutdown on SIGTERM, so in-flight events are not
|
|
lost on rollout.
|
|
|
|
Add structured request logging with a request ID, propagate the correlation ID
|
|
into logs, and expose counters for accepted, duplicate, conflicted, rejected,
|
|
and failed events plus write latency. Logs and metrics must never carry event
|
|
payloads.
|
|
|
|
Done when the receiver serves concurrent senders under a bounded timeout,
|
|
sheds load predictably instead of stalling, and its behaviour is visible from
|
|
outside.
|
|
|
|
Done 2026-08-10: serving moves to waitress with configurable threads and a
|
|
channel timeout, installed in the image via the `serve` extra. Where waitress
|
|
is absent the entrypoint falls back to a threaded wsgiref server with a socket
|
|
timeout and SIGTERM/SIGINT shutdown — bounded rather than good, and it logs a
|
|
warning so a deployment cannot quietly end up on it. Logging is structured
|
|
JSON to stdout.
|
|
|
|
Counters are not yet exposed. Deferred to AUDIT-WP-0005-T03 so the metric
|
|
surface is designed against the deployment's scrape path rather than guessed
|
|
at now.
|
|
|
|
## T07 - Close the test gaps
|
|
|
|
```task
|
|
id: AUDIT-WP-0004-T07
|
|
status: done
|
|
priority: medium
|
|
state_hub_task_id: "b8141609-858d-41e1-9cc4-eb6f2723d561"
|
|
```
|
|
|
|
Ingestion has two tests. Uncovered: oversized and zero-length bodies, absent
|
|
`Content-Length`, malformed JSON, wrong path and method, the health
|
|
endpoints, event ID conflict, naive and out-of-range timestamps, storage
|
|
failure, and persistence across restart.
|
|
|
|
`accepted_at` is recorded in local time while every other timestamp is UTC;
|
|
`datetime.fromisoformat` accepts naive timestamps and arbitrary dates. Fix
|
|
both and cover them.
|
|
|
|
Done when each rejection reason and each failure mode above has a test that
|
|
asserts the documented status code.
|
|
|
|
Done 2026-08-10: ingestion tests went from 2 to 23 (suite 15 -> 36). Covers
|
|
oversized, empty and truncated bodies, absent/invalid `Content-Length`,
|
|
malformed JSON, wrong path and method, health endpoints, id conflict,
|
|
naive and unparseable timestamps, backend unavailability, unexpected backend
|
|
faults, durability across reopen, and the concurrency race. `accepted_at` is
|
|
UTC; naive timestamps are rejected rather than silently assumed.
|