diff --git a/CHANGELOG.md b/CHANGELOG.md index ee999b3cc..3ae96e8e7 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -15,6 +15,19 @@ All notable changes to this project are documented in this file. The format is b ## [Unreleased] ### Security +- **Audit hardening:** audit entries no longer record credential values. + Redacted fields retain change indicators using `[redacted]`; credential + primary-key identities are masked too. Migration `b8d4f2a6c901` redacts + historical credential entries and, conservatively, all historical settings + values. Downgrading does not restore these values. Operators should rotate + credentials saved through the settings UI on earlier versions, and handle + existing backups and audit exports according to their retention policies. +- Entity update/delete log events mask primary keys that are themselves + credentials (session and refresh tokens), as the audit trail does. + +### Removed +- `settings.constants.SENSITIVE_KEYS`; the set now lives in + `simple_module_core.secret_fields.SENSITIVE_KEYS`. - **Behaviour change (#687):** delegated permission edits are bounded by platform-level access. Global user and role permission writes require platform-level `permissions.manage`; tenant-role mappings do not contribute diff --git a/docs/modules/audit_log.md b/docs/modules/audit_log.md index 79314a927..cb2ddf669 100644 --- a/docs/modules/audit_log.md +++ b/docs/modules/audit_log.md @@ -221,3 +221,33 @@ pip install simple_module_audit_log ``` It depends on no other module. Actor names, the Actor search and actor links come from the app's [user directory](/framework/user-directory), so they work under `users`, `keycloak` (users who have signed in) or no provider at all (raw ids). The module also installs the core [activity feed](/framework/user-directory#activity-feed) the users edit page reads. + +## Credential redaction + +Set `__audit_redact_fields__: ClassVar[set[str]] = {"credential"}` on a +model to keep a field in the audit diff while replacing every recorded `old` +and `new` value with `[redacted]`. Unlike `__audit_exclude_fields__`, which +omits a field entirely, redaction preserves the fact that it changed. +Exclusion takes precedence. A redacted primary key also masks `entity_id`. + +For row-dependent rules, define a static `__audit_redact_row__(row)` method +returning a set of field names. ORM capture evaluates the current and pre-edit +column dictionaries and redacts fields identified by either. Bulk and manual +capture pass `None`; the predicate must conservatively return every field +that could hold a credential when row context is unavailable. Manual capture +accepts the mapped class or its registered class name to apply its policy. + +Built-in policies cover stored secret settings, user password hashes, OAuth +credentials, tenant invitation hashes, bearer tokens, lease ownership tokens +and auth refresh outcomes. +Settings use the same secret rule as the settings UI. Non-secret ORM settings +retain their values; bulk settings values are always redacted because the +statement may affect rows with different keys or prior values. API token +hashes remain excluded from automatic diffs. + +Migration `b8d4f2a6c901` hardens existing entries, conservatively redacting all +historical settings values because old diffs may lack the key or type needed +to classify them. It also masks historical credential primary-key identities. +Its downgrade is a no-op: credential values cannot be recovered. Operators +should rotate credentials saved through the settings UI on earlier versions; +the migration does not rewrite backups or previously exported audit data. diff --git a/framework/core/simple_module_core/secret_fields.py b/framework/core/simple_module_core/secret_fields.py index b635d3d1b..20801be24 100644 --- a/framework/core/simple_module_core/secret_fields.py +++ b/framework/core/simple_module_core/secret_fields.py @@ -9,7 +9,7 @@ from __future__ import annotations import re -from typing import Any, get_args, get_origin +from typing import Any, Final, get_args, get_origin from urllib.parse import urlsplit from pydantic import BaseModel, SecretBytes, SecretStr @@ -20,7 +20,15 @@ _SECRET_PATTERNS = re.compile( r"(password|secret|api[_-]?key|private[_-]?key|token[_-]?secret)", re.I ) + SECRET_MASK = "••••••••" +# Keys whose stored value must never leave the settings API, the raw +# key/value browse screen or the audit trail. ``host.secret_key`` is the session-signing key: the +# hosting layer generates and persists it here when SM_SECRET_KEY is unset, and +# anyone who can read it can forge a session cookie for any account. Masked on +# the way out only — the boot-time reader goes straight to SQL, so hiding it +# here costs nothing. +SENSITIVE_KEYS: Final = frozenset({"host.secret_key"}) # Value types that cannot carry credential material, so a secret-ish *name* # on one of them is a false positive rather than something to hide. @@ -92,7 +100,10 @@ def conceals_secret(name: str, value: Any, value_type: str | None = None) -> boo ``broker_url``, while that field's password-free default is still shown. """ return ( - name in _TYPED_SECRET_KEYS or is_named_secret(name, value_type) or embeds_credential(value) + name in SENSITIVE_KEYS + or name in _TYPED_SECRET_KEYS + or is_named_secret(name, value_type) + or embeds_credential(value) ) diff --git a/framework/db/simple_module_db/_bulk_audit_records.py b/framework/db/simple_module_db/_bulk_audit_records.py index b4286f7aa..3b44ccd6c 100644 --- a/framework/db/simple_module_db/_bulk_audit_records.py +++ b/framework/db/simple_module_db/_bulk_audit_records.py @@ -10,7 +10,8 @@ from sqlalchemy import inspect as sa_inspect from sqlalchemy.orm import ORMExecuteState, Session -from simple_module_db.audit import AuditRecord, _serialize, format_pk +from simple_module_db.audit import AuditRecord, _excluded_fields, _serialize, format_pk +from simple_module_db.audit_redaction import model_for_audit, redact_changes, redact_entity_id from simple_module_db.query_filter import engine_policy logger = logging.getLogger("simple_module_db.bulk_audit") @@ -29,8 +30,16 @@ class _Ctx: """What one audited statement records, built before it runs.""" - def __init__(self, entity_type: str, action: str, changes: list[dict[str, Any]]) -> None: + def __init__( + self, + entity_type: str, + action: str, + changes: list[dict[str, Any]], + model: type | None = None, + ) -> None: self.entity_type = entity_type + # The statement's own class when known: a name lookup scans the registry. + self._model = model or model_for_audit(entity_type) self.action = action self.row_actions = [action] self.changes = changes @@ -45,7 +54,11 @@ def record( ) -> AuditRecord: return AuditRecord( entity_type=self.entity_type, - entity_id=entity_id, + entity_id=( + entity_id + if entity_id == SUMMARY_ENTITY_ID + else redact_entity_id(self._model, entity_id) + ), action=action or self.action, changes=self.changes if changes is None else changes, user_id=self.user_id, @@ -113,6 +126,11 @@ def record_bulk_audit( return name = entity_type if isinstance(entity_type, str) else entity_type.__name__ diff = [{"field": k, "new": _serialize(v)} for k, v in (changes or {}).items()] - ctx = _Ctx(name, action, diff) + ctx = _Ctx(name, action, diff, entity_type if isinstance(entity_type, type) else None) + model = ctx._model + if model is not None: + ctx.changes = redact_changes( + model, [c for c in diff if c["field"] not in _excluded_fields(model)] + ) keys = [SUMMARY_ENTITY_ID] if ids is None else [str(i) for i in ids] _deliver(sync, callback, [ctx.record(k) for k in keys]) diff --git a/framework/db/simple_module_db/audit.py b/framework/db/simple_module_db/audit.py index a52eaa10b..419315228 100644 --- a/framework/db/simple_module_db/audit.py +++ b/framework/db/simple_module_db/audit.py @@ -23,6 +23,7 @@ from sqlalchemy import inspect as sa_inspect from sqlalchemy.orm import Session +from simple_module_db.audit_redaction import redact_changes, redact_entity_id from simple_module_db.mixins import SoftDeleteMixin # Fields injected by AuditMixin — always excluded from change diffs @@ -97,8 +98,10 @@ def _entity_pk_str(obj: object) -> str: if identity is None: # Pre-flush: fall back to the mapper-level key attrs mapper = inspector.mapper - return format_pk([getattr(obj, col.key) for col in mapper.primary_key]) - return format_pk(identity) + pk = format_pk([getattr(obj, col.key) for col in mapper.primary_key]) + else: + pk = format_pk(identity) + return redact_entity_id(type(obj), pk) def _column_names(obj: object) -> list[str]: @@ -128,7 +131,7 @@ def _column_diffs(obj: object, inspector: Any, excl: frozenset[str]) -> list[dic old_val = hist.deleted[0] if hist.deleted else None new_val = hist.added[0] if hist.added else None changes.append({"field": col_name, "old": _serialize(old_val), "new": _serialize(new_val)}) - return changes + return redact_changes(type(obj), changes, obj) # --------------------------------------------------------------------------- @@ -182,7 +185,7 @@ def _get_pk_cols(obj: object) -> set[str]: obj_ref=obj, entity_type=type(obj).__name__, action="created", - changes=changes, + changes=redact_changes(type(obj), changes, obj), user_id=user_id, correlation_id=correlation_id, ) diff --git a/framework/db/simple_module_db/audit_redaction.py b/framework/db/simple_module_db/audit_redaction.py new file mode 100644 index 000000000..794339f20 --- /dev/null +++ b/framework/db/simple_module_db/audit_redaction.py @@ -0,0 +1,71 @@ +"""Shared credential redaction for flush, statement and manual audit capture.""" + +from __future__ import annotations + +from typing import Any, Final + +from sqlalchemy import inspect + +AUDIT_REDACTED: Final = "[redacted]" + + +def model_for_audit(model: type | str) -> type | None: + """Accept a mapped class or its registered class name; None for unknown names.""" + if isinstance(model, type): + return model + from simple_module_db.model_registry import classes_by_table + + return next((cls for cls in classes_by_table.values() if cls.__name__ == model), None) + + +def redact_changes( + model: type, changes: list[dict[str, Any]], obj: object | None = None +) -> list[dict[str, Any]]: + """Keep fields and diff shape, replacing credential values before delivery. + + A row predicate receives both the current and pre-edit column values. With + no row (statement/manual capture) it receives None and must fail closed. + """ + if not changes: + return changes + fields = set(getattr(model, "__audit_redact_fields__", ())) + predicate = getattr(model, "__audit_redact_row__", None) + if predicate is not None: + if obj is None: + fields.update(predicate(None)) + else: + state = inspect(obj) + current: dict[str, Any] = {} + previous: dict[str, Any] = {} + for attr in state.mapper.column_attrs: + key = attr.key + value = current[key] = state.dict.get(key) + history = state.attrs[key].history + if history.has_changes(): + previous[key] = history.deleted[0] if history.deleted else None + else: + previous[key] = value + fields.update(predicate(current)) + if state.persistent: + fields.update(predicate(previous)) + if not fields: + return changes + return [ + { + k: AUDIT_REDACTED if c["field"] in fields and k in {"old", "new"} else v + for k, v in c.items() + } + for c in changes + ] + + +def pk_is_redacted(model: type | None) -> bool: + """True when a primary-key column of ``model`` is itself a redacted credential.""" + fields = getattr(model, "__audit_redact_fields__", None) + if model is None or not fields: + return False + return any(col.key in fields for col in inspect(model).primary_key) + + +def redact_entity_id(model: type | None, entity_id: str) -> str: + return AUDIT_REDACTED if pk_is_redacted(model) else entity_id diff --git a/framework/db/simple_module_db/bulk_audit.py b/framework/db/simple_module_db/bulk_audit.py index d23955661..c9c6a16af 100644 --- a/framework/db/simple_module_db/bulk_audit.py +++ b/framework/db/simple_module_db/bulk_audit.py @@ -52,6 +52,7 @@ record_bulk_audit, ) from simple_module_db.audit import _excluded_fields, format_pk +from simple_module_db.audit_redaction import redact_changes from simple_module_db.bulk_audit_targets import _is_set, _shown, _targets from simple_module_db.query_filter import engine_policy from simple_module_db.tenancy import ALL_TENANTS_OPTION @@ -85,7 +86,7 @@ def _plan(execute_state: ORMExecuteState, cls: type) -> _Ctx | None: """The record shape for this statement, or ``None`` when nothing is audited.""" name = cls.__name__ if execute_state.is_delete: - return _Ctx(name, ACTION_BULK_DELETED, []) + return _Ctx(name, ACTION_BULK_DELETED, [], cls) rows, many = _targets(execute_state, cls) default = ACTION_BULK_INSERTED if execute_state.is_insert else ACTION_BULK_UPDATED # The soft-delete flag picks a row's action; it is never a change, and @@ -112,7 +113,7 @@ def _plan(execute_state: ORMExecuteState, cls: type) -> _Ctx | None: changes = [{"field": k} for k in kept] else: changes = [{"field": k, "new": _shown(rows[0][k])} for k in kept] - ctx = _Ctx(name, actions[0] if actions else default, changes) + ctx = _Ctx(name, actions[0] if actions else default, redact_changes(cls, changes), cls) ctx.row_actions = actions or [default] return ctx diff --git a/framework/db/simple_module_db/leases.py b/framework/db/simple_module_db/leases.py index 51d2ba664..2724c096c 100644 --- a/framework/db/simple_module_db/leases.py +++ b/framework/db/simple_module_db/leases.py @@ -19,7 +19,7 @@ import contextlib from collections.abc import AsyncGenerator from datetime import UTC, datetime, timedelta -from typing import Any, Final +from typing import Any, ClassVar, Final from sqlalchemy import and_, func, select, text, update from sqlalchemy.dialects import postgresql, sqlite @@ -48,6 +48,8 @@ class LeaseRow(Base, table=True): # ty: ignore[unsupported-base] """One lease slot. ``holder`` is the owner's token, ``""`` when never held.""" + __audit_redact_fields__: ClassVar[set[str]] = {"holder"} + __tablename__ = TABLE_LEASE name: str = Field(primary_key=True, max_length=NAME_MAX_LENGTH) diff --git a/framework/db/simple_module_db/listeners.py b/framework/db/simple_module_db/listeners.py index e0be095ac..9a840abd0 100644 --- a/framework/db/simple_module_db/listeners.py +++ b/framework/db/simple_module_db/listeners.py @@ -13,6 +13,7 @@ from sqlalchemy.orm import Session from simple_module_db._persisted_owner import mark_unverified, owner_mark, restore_owner_mark +from simple_module_db.audit_redaction import AUDIT_REDACTED, pk_is_redacted from simple_module_db.bulk_audit import audit_statement from simple_module_db.bulk_guard import install_bulk_guard from simple_module_db.flush_guard import guard_flush @@ -76,8 +77,14 @@ def _entity_label(obj: object) -> str: def _entity_pk(obj: object) -> object: - """Return the primary key value(s) if available, else None.""" + """Return the primary key value(s) if available, else None. + + A primary key that is itself a credential (a session or refresh token) is + masked: this value goes to the application log. + """ try: + if pk_is_redacted(type(obj)): + return AUDIT_REDACTED inspector = sa_inspect(obj) if inspector is None: return None diff --git a/framework/db/simple_module_db/stored_settings.py b/framework/db/simple_module_db/stored_settings.py index bb314e2d1..94fff4887 100644 --- a/framework/db/simple_module_db/stored_settings.py +++ b/framework/db/simple_module_db/stored_settings.py @@ -16,6 +16,7 @@ from typing import Final from pydantic_settings import BaseSettings +from simple_module_core.secret_fields import conceals_secret from simple_module_core.settings_hydrate import build_settings from sqlalchemy import Select, UniqueConstraint, inspect, select from sqlalchemy.ext.asyncio import AsyncConnection, AsyncSession @@ -84,6 +85,17 @@ class Setting(Base, AuditMixin, table=True): # ty: ignore[unsupported-base] inputs that don't parse. """ + @staticmethod + def __audit_redact_row__(row: dict | None) -> set[str]: + # Fail closed: no row (bulk/manual capture) or a partly loaded one hides the value. + if ( + row is None + or any(row.get(k) is None for k in (COL_KEY, COL_VALUE, COL_VALUE_TYPE)) + or conceals_secret(row[COL_KEY], row[COL_VALUE], row[COL_VALUE_TYPE]) + ): + return {COL_VALUE} + return set() + __tablename__ = TABLE_SETTING __table_args__ = (UniqueConstraint(COL_SCOPE, COL_SCOPE_ID, COL_KEY, name=UQ_SCOPE_KEY),) diff --git a/framework/db/tests/test_db_credential_audit.py b/framework/db/tests/test_db_credential_audit.py new file mode 100644 index 000000000..c429cb380 --- /dev/null +++ b/framework/db/tests/test_db_credential_audit.py @@ -0,0 +1,168 @@ +"""Credential changes remain visible without recording their values.""" + +from datetime import UTC, datetime + +import pytest +from simple_module_db.audit import collect_audit_records +from simple_module_db.leases import LeaseRow +from simple_module_db.stored_settings import Setting +from sqlalchemy.orm import Session + + +@pytest.mark.parametrize( + "key,value,value_type,expected", + [ + ("mail.smtp_password", "private", "string", "[redacted]"), + ("host.secret_key", "private", "int", "[redacted]"), + ("mail.url", "smtp://user:private@localhost", "string", "[redacted]"), + ("mail.port", "587", "int", "587"), + ("users.reset_password_token_lifetime_seconds", "3600", "int", "3600"), + ], +) +def test_setting_create_redaction(key, value, value_type, expected): + with Session() as session: + session.add(Setting(key=key, value=value, value_type=value_type)) + record = collect_audit_records(session)[0] + assert next(c["new"] for c in record.changes if c["field"] == "value") == expected + + +@pytest.fixture +def credential_session(): + from simple_module_db.listeners import attach_session_listeners + from simple_module_db.query_filter import EngineTenancy, bind_engine_policy + from sqlalchemy import create_engine + + engine = create_engine("sqlite://") + records = [] + policy = EngineTenancy( + tenant_strict=False, audit_callback=lambda session, batch: records.extend(batch) + ) + bind_engine_policy(engine, policy) + + class AuditSession(Session): + pass + + attach_session_listeners(AuditSession) + try: + Setting.metadata.create_all(engine) + with AuditSession(engine) as session: + yield session, records + finally: + engine.dispose() + + +@pytest.mark.parametrize("kind", ["setting", "renamed", "url"]) +def test_credential_update_redacts_both_sides(credential_session, kind): + session, records = credential_session + entity = Setting( + key="mail.url" if kind == "url" else "mail.smtp_password", + value="smtp://u:private@localhost" if kind == "url" else "old-private", + ) + field = "value" + session.add(entity) + session.flush() + records.clear() + setattr(entity, field, "public" if kind == "url" else "new-private") + if kind == "renamed": + entity.key = "mail.label" + session.flush() + assert next(c for c in records[0].changes if c["field"] == field) == { + "field": field, + "old": "[redacted]", + "new": "[redacted]", + } + + +@pytest.mark.parametrize("core", [False, True]) +def test_bulk_credentials_redacted(credential_session, core): + from sqlalchemy import update + + session, records = credential_session + session.add(Setting(key="mail.smtp_password", value="old-private")) + session.flush() + records.clear() + session.execute(update(Setting.__table__ if core else Setting).values(value="new-private")) + assert records[0].changes == [{"field": "value", "new": "[redacted]"}] + + +@pytest.mark.parametrize("by_name", [False, True]) +def test_manual_bulk_redaction(credential_session, by_name): + from simple_module_db import record_bulk_audit + + session, records = credential_session + record_bulk_audit( + session, "Setting" if by_name else Setting, "bulk_updated", ["1"], {"value": "private"} + ) + assert records[0].changes == [{"field": "value", "new": "[redacted]"}] + + +def test_nonsecret_setting_update_retains_values(credential_session): + session, records = credential_session + setting = Setting(key="mail.port", value="587", value_type="int") + session.add(setting) + session.flush() + records.clear() + setting.value = "465" + session.flush() + assert records[0].changes == [{"field": "value", "old": "587", "new": "465"}] + + +@pytest.mark.parametrize("core", [False, True]) +def test_bulk_insert_and_summary_redaction(credential_session, monkeypatch, core): + import simple_module_db.bulk_audit as bulk + from sqlalchemy import insert, update + + session, records = credential_session + target = Setting.__table__ if core else Setting + session.execute( + insert(target).values( + key="mail.smtp_password", + value="private", + value_type="string", + scope="system", + scope_id="", + ) + ) + assert next(c["new"] for c in records[0].changes if c["field"] == "value") == "[redacted]" + records.clear() + monkeypatch.setattr(bulk, "BULK_AUDIT_ID_CAP", 0) + session.execute(update(target).values(value="new-private")) + assert records[0].entity_id == "*" + assert records[0].changes[0] == {"field": "value", "new": "[redacted]"} + + +def test_typed_setting_redaction(): + from pydantic import SecretStr + from simple_module_core.secret_fields import register_typed_secrets + from sqlmodel import SQLModel + + class CredentialSettings(SQLModel): + credential: SecretStr + + register_typed_secrets("audit_test", CredentialSettings) + with Session() as session: + session.add(Setting(key="audit_test.credential", value="private")) + changes = collect_audit_records(session)[0].changes + assert next(c["new"] for c in changes if c["field"] == "value") == "[redacted]" + + +def test_manual_bulk_honors_field_exclusions(credential_session): + from _audit_models import PartialExcludeModel + from simple_module_db import record_bulk_audit + + session, records = credential_session + record_bulk_audit( + session, + PartialExcludeModel, + "bulk_updated", + ["1"], + {"password_hash": "private", "username": "visible"}, + ) + assert records[0].changes == [{"field": "username", "new": "visible"}] + + +def test_lease_ownership_value_is_redacted(): + with Session() as session: + session.add(LeaseRow(name="audit", slot=0, holder="private", expires_at=datetime.now(UTC))) + changes = collect_audit_records(session)[0].changes + assert next(c["new"] for c in changes if c["field"] == "holder") == "[redacted]" diff --git a/framework/db/tests/test_db_credential_log_masking.py b/framework/db/tests/test_db_credential_log_masking.py new file mode 100644 index 000000000..edd5133f7 --- /dev/null +++ b/framework/db/tests/test_db_credential_log_masking.py @@ -0,0 +1,16 @@ +"""A credential used as a primary key never reaches the entity-change log.""" + +import uuid + +from simple_module_db.listeners import _entity_pk +from users.models import RefreshToken, User + + +def test_token_primary_key_is_masked_in_log_identity(): + token = RefreshToken(token="private", user_id=uuid.uuid4()) + assert _entity_pk(token) == "[redacted]" + + +def test_ordinary_primary_key_is_kept(): + user = User(email="log@test") + assert _entity_pk(user) != "[redacted]" diff --git a/host/migrations/versions/b8d4f2a6c901_redact_audit_credentials.py b/host/migrations/versions/b8d4f2a6c901_redact_audit_credentials.py new file mode 100644 index 000000000..076fe3251 --- /dev/null +++ b/host/migrations/versions/b8d4f2a6c901_redact_audit_credentials.py @@ -0,0 +1,73 @@ +"""Harden historical audit credential values. + +Revision ID: b8d4f2a6c901 +Revises: a7c2e91d4b58 +""" + +import sqlalchemy as sa +from alembic import op + +revision = "b8d4f2a6c901" +down_revision = "a7c2e91d4b58" +branch_labels = None +depends_on = None + +# Frozen policy: historical settings entries may lack their key or declared +# type, so all historical setting values are redacted conservatively. +_REDACTED = "[redacted]" +_FIELDS = { + "Setting": {"value"}, + "LeaseRow": {"holder"}, + "User": {"hashed_password"}, + "OAuthAccount": {"access_token", "refresh_token"}, + "Invitation": {"token_hash"}, + "ApiToken": {"token_hash"}, + "UserAccessToken": {"token"}, + "RefreshToken": {"token"}, + "RefreshOutcome": {"session", "outcome_key", "lock_key"}, +} +_REDACT_IDS = {"UserAccessToken", "RefreshToken", "RefreshOutcome"} + + +def upgrade() -> None: + conn = op.get_bind() + audit = sa.Table("audit_log_audit_entry", sa.MetaData(), autoload_with=conn) + # Keyset batches bound memory and close each cursor before updating rows. + last_id = None + while True: + query = ( + sa.select(audit).where(audit.c.entity_type.in_(_FIELDS)).order_by(audit.c.id).limit(500) + ) + if last_id is not None: + query = query.where(audit.c.id > last_id) + rows = conn.execute(query).mappings().all() + if not rows: + break + for row in rows: + changes = row["changes"] + fields = _FIELDS[row["entity_type"]] + if isinstance(changes, list): + changes = [ + { + k: _REDACTED if c.get("field") in fields and k in {"old", "new"} else v + for k, v in c.items() + } + if isinstance(c, dict) + else c + for c in changes + ] + elif isinstance(changes, dict): + changes = {k: _REDACTED if k in fields else v for k, v in changes.items()} + identity = _REDACTED if row["entity_type"] in _REDACT_IDS else row["entity_id"] + if changes != row["changes"] or identity != row["entity_id"]: + conn.execute( + audit.update() + .where(audit.c.id == row["id"]) + .values(changes=changes, entity_id=identity) + ) + last_id = rows[-1]["id"] + + +def downgrade() -> None: + # Credential values are intentionally unrecoverable; retain the hardened trail. + pass diff --git a/host/tests/test_host_audit_redaction_migration.py b/host/tests/test_host_audit_redaction_migration.py new file mode 100644 index 000000000..bb8de6ff6 --- /dev/null +++ b/host/tests/test_host_audit_redaction_migration.py @@ -0,0 +1,120 @@ +"""Historical audit hardening keeps the trail and removes credential values.""" + +import importlib.util +from pathlib import Path + +import sqlalchemy as sa +from alembic.migration import MigrationContext +from alembic.operations import Operations + + +def test_upgrade_scrubs_credentials_and_preserves_other_changes(): + path = ( + Path(__file__).parents[1] / "migrations/versions/b8d4f2a6c901_redact_audit_credentials.py" + ) + spec = importlib.util.spec_from_file_location("audit_hardening_revision", path) + revision = importlib.util.module_from_spec(spec) + spec.loader.exec_module(revision) + engine = sa.create_engine("sqlite://") + metadata = sa.MetaData() + audit = sa.Table( + "audit_log_audit_entry", + metadata, + sa.Column("id", sa.String, primary_key=True), + sa.Column("entity_type", sa.String), + sa.Column("entity_id", sa.String), + sa.Column("changes", sa.JSON), + ) + metadata.create_all(engine) + cases = [ + ("Setting", "1", "value"), + ("LeaseRow", "lease", "holder"), + ("User", "2", "hashed_password"), + ("OAuthAccount", "3", "access_token"), + ("OAuthAccount", "4", "refresh_token"), + ("Invitation", "5", "token_hash"), + ("RefreshOutcome", "private-id", "session"), + ("UserAccessToken", "private-id", "token"), + ("RefreshToken", "private-id", "token"), + ("ApiToken", "9", "token_hash"), + ] + with engine.begin() as conn: + for i, (entity, identity, field) in enumerate(cases): + conn.execute( + audit.insert().values( + id=str(i), + entity_type=entity, + entity_id=identity, + changes=[ + {"field": field, "old": "old-private", "new": "private"}, + {"field": "label", "new": "visible"}, + ], + ) + ) + conn.execute( + audit.insert().values( + id="ordinary", + entity_type="User", + entity_id="10", + changes=[{"field": "email", "new": "visible"}], + ) + ) + conn.execute( + audit.insert(), + [ + { + "id": f"batch-{i:04}", + "entity_type": "Setting", + "entity_id": str(i), + "changes": [{"field": "value", "new": "private"}], + } + for i in range(501) + ], + ) + conn.execute( + audit.insert().values( + id="legacy", + entity_type="User", + entity_id="11", + changes={"hashed_password": {"old": "private"}, "label": "visible"}, + ) + ) + conn.execute( + audit.insert().values( + id="sparse", + entity_type="User", + entity_id="12", + changes=[{"field": "hashed_password"}, "note"], + ) + ) + conn.execute( + audit.insert().values(id="empty", entity_type="User", entity_id="13", changes=None) + ) + with Operations.context(MigrationContext.configure(conn)): + revision.upgrade() + revision.upgrade() # idempotent data rewrite + revision.downgrade() + rows = conn.execute(sa.select(audit).order_by(audit.c.id)).mappings().all() + assert len(rows) == len(cases) + 505 + for row in rows: + if row["id"].startswith("batch-"): + assert row["changes"] == [{"field": "value", "new": "[redacted]"}] + continue + if row["id"] == "legacy": + assert row["changes"] == {"hashed_password": "[redacted]", "label": "visible"} + continue + if row["id"] == "sparse": + assert row["changes"] == [{"field": "hashed_password"}, "note"] + continue + if row["id"] == "empty": + assert row["changes"] is None + continue + if row["id"] == "ordinary": + assert row["changes"] == [{"field": "email", "new": "visible"}] + continue + assert row["changes"][0]["old"] == "[redacted]" + assert row["changes"][0]["new"] == "[redacted]" + assert row["changes"][1] == {"field": "label", "new": "visible"} + if row["entity_type"] in {"UserAccessToken", "RefreshToken", "RefreshOutcome"}: + assert row["entity_id"] == "[redacted]" + engine.dispose() diff --git a/host/tests/test_migration_graph.py b/host/tests/test_migration_graph.py index 6749026ac..fc8e9e4fc 100644 --- a/host/tests/test_migration_graph.py +++ b/host/tests/test_migration_graph.py @@ -52,6 +52,7 @@ "f1a9c3d7b2e4": "tenants", "70786227af4c": "audit_log", "a7c2e91d4b58": "audit_log", + "b8d4f2a6c901": "audit_log", "f733c7938fd1": "sharing", "a738c2e4f601": "auth", "d263ea727663": "leases", # framework-owned sm_lease (#539) @@ -68,7 +69,7 @@ # head revision -> module owning the tables it creates or alters. _HEADS = { - "a7c2e91d4b58": "audit_log", + "b8d4f2a6c901": "audit_log", "e731a1b2c3d4": "users", # also carries ``api_tokens``, ``background_tasks``, ``file_storage`` "f1a9c3d7b2e4": "tenants", "f6a3b9d2e8c4": "keycloak", diff --git a/modules/audit_log/README.md b/modules/audit_log/README.md index e573be54a..483158b85 100644 --- a/modules/audit_log/README.md +++ b/modules/audit_log/README.md @@ -17,6 +17,7 @@ Add `simple_module_audit_log` to your host's dependencies and the entry point wi - `GET /api/audit_log/` REST endpoint with `entity_type`, `entity_id`, `action`, `user_id`, `from_date`, `to_date`, paging filters. - `/audit_log` Inertia browse page with the same filter set rendered as a table, gated by the `audit_log.view` permission. - Opt-outs: set `__audit_exclude__ = True` on a model to skip it entirely, or `__audit_exclude_fields__ = {"password_hash"}` to skip specific columns. `AuditMixin` / `SoftDeleteMixin` bookkeeping fields are excluded automatically. +- Credential redaction: set `__audit_redact_fields__` (or a `__audit_redact_row__` predicate) to keep a field in the diff with its values replaced by `[redacted]`; see the [audit_log docs](https://github.com/antosubash/simple_module_python/blob/main/docs/modules/audit_log.md) for details. ## Usage diff --git a/modules/auth/auth/models.py b/modules/auth/auth/models.py index 85021a6ec..7e0847dbf 100644 --- a/modules/auth/auth/models.py +++ b/modules/auth/auth/models.py @@ -1,6 +1,6 @@ """Short-lived cross-worker outcomes for browser token renewal.""" -from typing import Any +from typing import Any, ClassVar from simple_module_db.base import create_module_base from sqlalchemy import JSON, BigInteger @@ -10,6 +10,8 @@ class RefreshOutcome(Base, table=True): # ty: ignore[unsupported-base] + __audit_redact_fields__: ClassVar[set[str]] = {"outcome_key", "lock_key", "session"} + __tablename__ = "auth_refresh_outcome" # sha256(refresh key : identity : spent sealed token); not a session id. diff --git a/modules/auth/tests/test_auth_audit_redaction.py b/modules/auth/tests/test_auth_audit_redaction.py new file mode 100644 index 000000000..88e65889b --- /dev/null +++ b/modules/auth/tests/test_auth_audit_redaction.py @@ -0,0 +1,16 @@ +"""Refresh-outcome credentials are redacted from audit diffs.""" + +from auth.models import RefreshOutcome +from simple_module_db.audit import collect_audit_records +from sqlalchemy.orm import Session + + +def test_refresh_outcome_credentials_are_redacted(): + with Session() as session: + session.add( + RefreshOutcome( + outcome_key="private", lock_key="private", expires_at=1, session={"token": "x"} + ) + ) + changes = {c["field"]: c["new"] for c in collect_audit_records(session)[0].changes} + assert changes["lock_key"] == changes["session"] == "[redacted]" diff --git a/modules/settings/settings/_row_masking.py b/modules/settings/settings/_row_masking.py index 9ea41c241..497778981 100644 --- a/modules/settings/settings/_row_masking.py +++ b/modules/settings/settings/_row_masking.py @@ -12,24 +12,22 @@ from simple_module_core.secret_fields import conceals_secret from simple_module_db.stored_settings import Setting -from settings.constants import SENSITIVE_KEYS, SENSITIVE_PLACEHOLDER +from settings.constants import SENSITIVE_PLACEHOLDER from settings.contracts.schemas import SettingOut def is_masked(entity: Setting) -> bool: """Whether this row's stored value must not leave the service. - :data:`SENSITIVE_KEYS` names the one key the hosting layer owns - (``host.secret_key``). Everything else is judged by the same name/value + ``conceals_secret`` covers the one key the hosting layer owns + (``host.secret_key``) and judges everything else by the same name/value rule the module editor uses, because the store is the *same data* seen through a different screen: an override named ``mail.smtp_password``, or any override holding a ``postgresql://user:pw@host/db``, was rendered in clear text in the browse table and pre-filled into the edit form purely because the allowlist had a single entry in it. """ - return entity.key in SENSITIVE_KEYS or conceals_secret( - entity.key, entity.value, entity.value_type - ) + return conceals_secret(entity.key, entity.value, entity.value_type) def out(entity: Setting) -> SettingOut: diff --git a/modules/settings/settings/constants.py b/modules/settings/settings/constants.py index bcce41550..d299cbf80 100644 --- a/modules/settings/settings/constants.py +++ b/modules/settings/settings/constants.py @@ -45,13 +45,7 @@ ALL_SCOPES: Final = (SCOPE_SYSTEM, SCOPE_TENANT, SCOPE_USER) # ── Secrets ────────────────────────────────────────────────────────── -# Keys whose stored value must never leave the settings API or the raw -# key/value browse screen. ``host.secret_key`` is the session-signing key: the -# hosting layer generates and persists it here when SM_SECRET_KEY is unset, and -# anyone who can read it can forge a session cookie for any account. Masked on -# the way out only — the boot-time reader goes straight to SQL, so hiding it -# here costs nothing. -SENSITIVE_KEYS: Final = frozenset({"host.secret_key"}) +# The sensitive-key allowlist lives in ``simple_module_core.secret_fields``. SENSITIVE_PLACEHOLDER: Final = "********" # ── Routing ────────────────────────────────────────────────────────── diff --git a/modules/tenants/tenants/models.py b/modules/tenants/tenants/models.py index c135e3eef..6c2fe2484 100644 --- a/modules/tenants/tenants/models.py +++ b/modules/tenants/tenants/models.py @@ -10,6 +10,7 @@ import uuid from datetime import datetime +from typing import ClassVar from simple_module_db import is_valid_tenant_id from simple_module_db.base import create_module_base @@ -81,6 +82,8 @@ class Membership(Base, AuditMixin, table=True): # ty: ignore[unsupported-base] class Invitation(Base, AuditMixin, table=True): # ty: ignore[unsupported-base] """A pending invitation. Only the token's SHA-256 is stored.""" + __audit_redact_fields__: ClassVar[set[str]] = {"token_hash"} + __tablename__ = "tenants_invitation" __table_args__ = ( # One open invitation per address: the service's duplicate check is a diff --git a/modules/tenants/tests/test_tenants_audit_redaction.py b/modules/tenants/tests/test_tenants_audit_redaction.py new file mode 100644 index 000000000..001c280ec --- /dev/null +++ b/modules/tenants/tests/test_tenants_audit_redaction.py @@ -0,0 +1,21 @@ +"""Invitation token hashes are redacted from audit diffs.""" + +from datetime import UTC, datetime + +from simple_module_db.audit import collect_audit_records +from sqlalchemy.orm import Session +from tenants.models import Invitation + + +def test_invitation_token_hash_is_redacted(): + with Session() as session: + session.add( + Invitation( + tenant_id="default", + email="invitee@example.test", + token_hash="private", + expires_at=datetime.now(UTC), + ) + ) + changes = {c["field"]: c["new"] for c in collect_audit_records(session)[0].changes} + assert changes["token_hash"] == "[redacted]" diff --git a/modules/users/tests/test_users_audit_redaction.py b/modules/users/tests/test_users_audit_redaction.py new file mode 100644 index 000000000..32ccde63e --- /dev/null +++ b/modules/users/tests/test_users_audit_redaction.py @@ -0,0 +1,118 @@ +"""User credentials and token identities are redacted from audit capture.""" + +import uuid +from datetime import UTC, datetime + +import pytest +from simple_module_db import record_bulk_audit +from simple_module_db.audit import collect_audit_records +from simple_module_db.listeners import attach_session_listeners +from simple_module_db.query_filter import EngineTenancy, bind_engine_policy +from sqlalchemy import create_engine, delete, update +from sqlalchemy.orm import Session +from users.models import OAuthAccount, RefreshToken, User, UserAccessToken + +TOKENS = [UserAccessToken, RefreshToken] + + +@pytest.fixture +def credential_session(): + engine = create_engine("sqlite://") + records = [] + bind_engine_policy( + engine, + EngineTenancy( + tenant_strict=False, audit_callback=lambda session, batch: records.extend(batch) + ), + ) + + class AuditSession(Session): + pass + + attach_session_listeners(AuditSession) + try: + User.metadata.create_all(engine) + with AuditSession(engine) as session: + yield session, records + finally: + engine.dispose() + + +def _created(entity): + with Session() as session: + session.add(entity) + return collect_audit_records(session)[0] + + +def test_user_and_oauth_credentials_are_redacted(): + user = { + c["field"]: c["new"] for c in _created(User(email="a@test", hashed_password="x")).changes + } + assert user["hashed_password"] == "[redacted]" + oauth = OAuthAccount( + user_id=uuid.uuid4(), + access_token="x", + refresh_token="x", + oauth_name="test", + account_id="1", + account_email="a@test", + ) + changes = {c["field"]: c["new"] for c in _created(oauth).changes} + assert changes["access_token"] == changes["refresh_token"] == "[redacted]" + + +@pytest.mark.parametrize("cls", TOKENS) +def test_token_primary_key_is_redacted(cls): + token = cls(token="private", user_id=uuid.uuid4(), expires_at=datetime.now(UTC)) + assert _created(token).entity_id == "[redacted]" + + +def test_password_update_redacts_both_sides(credential_session): + session, records = credential_session + user = User(email="update@test", hashed_password="old-private") + session.add(user) + session.flush() + records.clear() + user.hashed_password = "new-private" + session.flush() + assert records[0].changes == [ + {"field": "hashed_password", "old": "[redacted]", "new": "[redacted]"} + ] + + +def test_bulk_password_update_redacted(credential_session): + session, records = credential_session + session.add(User(email="bulk@test", hashed_password="old-private")) + session.flush() + records.clear() + session.execute(update(User).values(hashed_password="new-private")) + session.execute(update(User.__table__).values(hashed_password="newer-private")) + assert [r.changes for r in records] == [[{"field": "hashed_password", "new": "[redacted]"}]] * 2 + + +@pytest.mark.parametrize("by_name", [False, True]) +def test_manual_bulk_redaction(credential_session, by_name): + session, records = credential_session + record_bulk_audit( + session, "User" if by_name else User, "bulk_updated", ["1"], {"hashed_password": "private"} + ) + assert records[0].changes == [{"field": "hashed_password", "new": "[redacted]"}] + + +@pytest.mark.parametrize("cls", TOKENS) +def test_token_identity_redacted_on_update_delete_and_bulk(credential_session, cls): + session, records = credential_session + user = User(email="token-owner@test") + session.add(user) + session.flush() + token = cls(token="private", user_id=user.id, expires_at=datetime.now(UTC)) + session.add(token) + session.flush() + records.clear() + token.expires_at = datetime(2030, 1, 1, tzinfo=UTC) + session.flush() + session.execute(update(cls).values(expires_at=datetime(2031, 1, 1, tzinfo=UTC))) + record_bulk_audit(session, cls.__name__, "bulk_updated", ["private"]) + session.execute(delete(cls)) + assert len(records) == 4 + assert all(r.entity_id == "[redacted]" for r in records) diff --git a/modules/users/users/models/access_token.py b/modules/users/users/models/access_token.py index 2161234a0..2d7caa181 100644 --- a/modules/users/users/models/access_token.py +++ b/modules/users/users/models/access_token.py @@ -7,6 +7,7 @@ import uuid from datetime import datetime, timedelta +from typing import ClassVar from fastapi_users_db_sqlalchemy.generics import GUID, TIMESTAMPAware, now_utc from sqlmodel import Field @@ -30,6 +31,8 @@ def _default_expires_at() -> datetime: class UserAccessToken(Base, table=True): # ty: ignore[unsupported-base] """fastapi-users DatabaseStrategy-backed access tokens.""" + __audit_redact_fields__: ClassVar[set[str]] = {"token"} + __tablename__ = "users_access_token" token: str = Field(max_length=64, primary_key=True) diff --git a/modules/users/users/models/oauth_account.py b/modules/users/users/models/oauth_account.py index 26f26b036..775caa82b 100644 --- a/modules/users/users/models/oauth_account.py +++ b/modules/users/users/models/oauth_account.py @@ -10,6 +10,7 @@ # Relationship resolution requires runtime annotations. import uuid +from typing import ClassVar from fastapi_users_db_sqlalchemy.generics import GUID from sqlmodel import Field @@ -20,6 +21,8 @@ class OAuthAccount(Base, table=True): # ty: ignore[unsupported-base] """One row per (provider, account_id) link to a local User.""" + __audit_redact_fields__: ClassVar[set[str]] = {"refresh_token", "access_token"} + __tablename__ = "users_oauth_account" id: uuid.UUID = Field( diff --git a/modules/users/users/models/refresh_token.py b/modules/users/users/models/refresh_token.py index 34f77f774..ffdb2c19f 100644 --- a/modules/users/users/models/refresh_token.py +++ b/modules/users/users/models/refresh_token.py @@ -4,6 +4,7 @@ import uuid as uuid_mod from datetime import UTC, datetime +from typing import ClassVar from fastapi_users_db_sqlalchemy.generics import GUID from sqlmodel import Field @@ -14,6 +15,8 @@ class RefreshToken(Base, table=True): # ty: ignore[unsupported-base] """Opaque refresh token exchanged for a new access + refresh pair.""" + __audit_redact_fields__: ClassVar[set[str]] = {"token"} + __tablename__ = "users_refresh_token" token: str = Field(primary_key=True, max_length=64) diff --git a/modules/users/users/models/user.py b/modules/users/users/models/user.py index 405426044..2de3b3b57 100644 --- a/modules/users/users/models/user.py +++ b/modules/users/users/models/user.py @@ -33,6 +33,8 @@ class User(Base, AuditMixin, table=True): # ty: ignore[unsupported-base] """Local user. Column surface mirrors fastapi-users' SQLAlchemyBaseUserTableUUID.""" + __audit_redact_fields__: ClassVar[set[str]] = {"hashed_password"} + __tablename__ = "users_user" id: uuid.UUID = Field(