Skip to content

feat(security_audit): append-only security audit log + redaction (#440) [stacked on #476] - #477

Open
olegbrok wants to merge 1 commit into
feat/boot-guards-stackedfrom
feat/security-audit-stacked
Open

feat(security_audit): append-only security audit log + redaction (#440) [stacked on #476]#477
olegbrok wants to merge 1 commit into
feat/boot-guards-stackedfrom
feat/security-audit-stacked

Conversation

@olegbrok

Copy link
Copy Markdown
Collaborator

Summary

PR-6 of the #433 web-exposed hardening track. Introduces a dedicated security-event audit log distinct from the per-tool-call audit in `hooks.AuditStore`.

Stacked on #476#475#474#473#472.

Closes part of #433. Refs #440.

Why a new store

`hooks.AuditStore` tracks tool execution (agent_name, session_id, tool_name, cost) — useful for cost/usage attribution. #440's audit log tracks security events (auth, admin, network reject, rate-limit, boot guards) with a different schema and retention story. Separate DB so a focused security-export job doesn't have to scan the bulky tool-call history.

Schema

`security_audit_log`:

  • `id`, `ts`, `event_type`
  • `actor_user_id`, `actor_token_id`, `actor_ip`
  • `via_proxy`, `forwarded_chain` (JSON, from Hardening 9/10: Canonical trusted-client-IP resolver (cross-cutting) #442 `ClientIdentity`)
  • `method`, `route_name`, `target`
  • `outcome` (success / failure / denied)
  • `user_agent`, `correlation_id`
  • `detail_json` (redacted)
  • `prev_hash` — reserved for future hash-chain integrity, not populated this PR

Indexes on `(ts)`, `(event_type, ts)`, `(actor_user_id, ts)`, `(correlation_id)`.

Murzik review followups (from #440) — addressed

  • Schema additions all present (actor_token_id, method, route_name, user_agent, correlation_id, via_proxy, forwarded_chain)
  • Actor identity is `(user_id, token_id)` tuple
  • `detail` renamed → `detail_json` (column); helper takes `Mapping`, JSON-serialises after redaction
  • Allowlist-shaped recursive redaction per event_type — define what's kept; future credential field names default to redacted
  • Best-effort writes on hot paths — `log_event` catches all exceptions, returns `None`; login can't fail because audit DB is locked
  • No update/delete API — retention pruning is the only deletion path
  • Reserved `prev_hash` column slot

Out of scope (deferred to owning PRs)

Tests (25 new)

  • `redact_detail`: None pass-through, per-event allowlist, default allowlist for unknown events, default doesn't include credential words, nested dict walked, lists walked, empty allowlist drops everything
  • `SecurityAuditStore`: log returns id, redacts on write, query filters (event_type / actor / outcome / time range / correlation_id), newest-first, pagination, count, via_proxy + forwarded_chain round-trip, method/route/user_agent round-trip, best-effort returns None on closed DB
  • `prune_older_than`: deletes older rows, no-op on zero/negative
  • Schema: table + all four indexes present, prev_hash column reserved
  • Wiring: `app.state.security_audit` is a `SecurityAuditStore` instance; smoke-write through it

Test plan

  • `pytest tests/test_security_audit.py -v` (25 passing)
  • `pytest tests/test_*.py` (no regression)
  • `ruff check src/pinky_daemon/security_audit.py src/pinky_daemon/api.py tests/test_security_audit.py` (clean)

🤖 Opened by Barsik

PR-6 of the #433 web-exposed hardening track. Introduces a dedicated
security-event audit log distinct from the per-tool-call audit in
hooks.AuditStore. Stacked on #476#475#474#473#472.

Why a new store
===============
hooks.AuditStore tracks tool execution (agent_name, session_id,
tool_name, cost) — useful for cost/usage attribution. #440's audit log
tracks security events (auth, admin, network reject, rate-limit, boot
guards) with a different schema and a different retention story:

* Security audit benefits from a focused export job that doesn't have
  to scan the bulky tool-call history.
* Append-only by convention; no UPDATE/DELETE API.
* Best-effort writes: a locked DB must NOT take down login.

Schema
======
security_audit_log:
  id, ts, event_type,
  actor_user_id, actor_token_id, actor_ip,
  via_proxy, forwarded_chain (JSON),
  method, route_name, target,
  outcome, user_agent, correlation_id,
  detail_json,
  prev_hash       -- reserved for future hash-chain integrity

Indexes on (ts), (event_type, ts), (actor_user_id, ts),
(correlation_id) — query patterns are time-windowed by event/actor
filters or correlation-id traces across multi-hop requests.

Redaction
=========
Allowlist-shaped per event_type. Define what's *kept*, not what's
stripped, so a future credential field name added to login flow
defaults to redacted instead of accidentally leaking.

EVENT_DETAIL_ALLOWLIST registers the allowed keys for known event
types (auth.login.success/fail, auth.token.create/revoke, admin.*,
rate_limit.exceeded, network.lan_filter.reject, boot.*). Unknown
event types fall back to DEFAULT_DETAIL_ALLOWLIST — a conservative set
({reason, duration_ms, status_code, error_class, count, limit, bucket}).

Best-effort writes
==================
log_event catches all exceptions, logs at WARNING via the standard
logger, returns None instead of raising. Boot-time refuse-to-boot
events that need fail-loud semantics call assert_web_mode_safe (#441)
which raises BootGuardError; the audit log there is informational
only.

Wiring
======
SecurityAuditStore is constructed in create_api alongside the existing
AuditStore and cached on app.state.security_audit. Routes will reach
it via Depends() once #435 / #438 / #439 / #443 land their consumers.

Out of scope (deferred)
=======================
* GET /audit endpoints (admin-only) — depend on #435 user accounts +
  #439 elevated-session protection. Land alongside those PRs.
* GET /audit/export (CSV/JSON) — same dependency chain.
* Retention cron wiring — store ships prune_older_than(); the
  scheduler wiring lands as a one-line follow-up once review confirms
  the desired default (365d).
* Hash-chain population — column is reserved for the integrity work
  but not populated in this PR.
* Override-emits-audit for boot guards (#441) — needs this PR + an
  obvious wiring point. Land in a follow-up that depends on both.

Tests (25 new)
==============
* redact_detail: None pass-through, per-event allowlist, default
  allowlist for unknown events, default doesn't include credential
  words, nested dict walked, lists walked, empty allowlist drops
  everything.
* SecurityAuditStore: log returns id, redacts on write, query filters
  (event_type, actor, outcome, time range, correlation_id), newest-first
  ordering, pagination, count, via_proxy + forwarded_chain round-trip,
  method/route/user_agent round-trip, best-effort returns None on
  closed DB.
* prune_older_than: deletes older rows, no-op on zero/negative.
* Schema: table + all four indexes present, prev_hash column reserved.
* Wiring: app.state.security_audit is a SecurityAuditStore instance;
  smoke-write through it.

Closes part of #433. Refs #440.

🤖 Authored by Barsik
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant