From 8788e8f83b2c6b313c12e3cd5798a2283e3b71e7 Mon Sep 17 00:00:00 2001 From: Manish Kumar Date: Fri, 14 Aug 2026 14:09:14 -0500 Subject: [PATCH 1/2] security(audit): sign this service's own native audit:events payloads HIPAA PR3b: closes producer-side signing for the sixth and final real audit:events producer the HIPAA audit-integrity assessment found unsigned -- this repo's own AuditLogger.log(), used by api/routes_audit.py for platform-admin API actions. Unlike every other producer (api-gateway, tes, workflow-bundles, control-center, rag -- all separate deployables that hand-port sign_audit_event()), AuditLogger.log() lives in the same repo as audit.signing itself and imports sign_audit_event directly -- no drift risk, no parallel copy to keep in sync. data = json.dumps(payload) is computed exactly once, from the already- model_dump()'d payload, and that exact string is both signed and published as {"data": data, "sig": ...}. service is read from payload.get("service") -- the identity actually present in the signed bytes -- not event.service, so a hypothetical future divergence between the AuditEvent object and its own serialization can never desync the two. Secret reused unchanged: AuditConfig.EVENT_SIGNING_SECRET (audit/config.py), defined by PR2 specifically for this eventual purpose -- no new secret, no new config path. Existing event schema, event types, event IDs, timestamps, and stream name unchanged -- purely additive sig field. All 10 pre-existing tests/test_logger.py tests pass unmodified. 7 new tests (tests/test_logger_signing.py): signs the exact wire string (verified against the real verify_audit_event, not a re-derived value), wrong-secret and tampered-data both correctly fail verification, missing- service still publishes unsigned rather than dropping the event, and secret non-leakage on a forced xadd exception. Full suite: 249 passed (242 baseline + 7 new), 0 regressions. This closes producer-side signing across all six real audit:events producers on the platform (api-gateway, tes, workflow-bundles, control-center, rag, and this repo's own native producer) -- each in its own separate PR per repo, this being the sixth and last. Co-Authored-By: Claude Sonnet 5 --- audit/logger.py | 21 +++++- tests/test_logger_signing.py | 126 +++++++++++++++++++++++++++++++++++ 2 files changed, 146 insertions(+), 1 deletion(-) create mode 100644 tests/test_logger_signing.py diff --git a/audit/logger.py b/audit/logger.py index e9453e8..29d3b1b 100644 --- a/audit/logger.py +++ b/audit/logger.py @@ -3,6 +3,7 @@ from typing import Optional from audit.config import AuditConfig from audit.models import AuditEvent +from audit.signing import sign_audit_event class AuditLogger: @@ -16,9 +17,27 @@ async def log(self, event: AuditEvent): # so json.dumps below never sees a raw datetime. payload = event.model_dump(mode="json") + # HIPAA PR3b: this is the one producer that lives in the same + # repo as the consumer/audit.signing itself, so unlike every + # other producer (api-gateway, tes, workflow-bundles, + # control-center, rag -- all separate deployables that + # hand-port sign_audit_event), this one imports it directly -- + # no drift risk, no parallel copy to keep in sync. + # + # data is computed exactly once and that exact string is both + # signed and published, matching every other producer's + # contract. service comes from the already-serialized payload + # (not event.service) so a signature always covers the + # identity actually present in the signed bytes. + data = json.dumps(payload) + fields = {"data": data} + service = payload.get("service") + if service: + fields["sig"] = sign_audit_event(service, data, AuditConfig.EVENT_SIGNING_SECRET) + await self.redis.xadd( AuditConfig.STREAM_NAME, - {"data": json.dumps(payload)}, + fields, maxlen=AuditConfig.MAX_STREAM_LENGTH, approximate=True, ) diff --git a/tests/test_logger_signing.py b/tests/test_logger_signing.py new file mode 100644 index 0000000..75d58f6 --- /dev/null +++ b/tests/test_logger_signing.py @@ -0,0 +1,126 @@ +"""HIPAA PR3b: producer-side signing for AuditLogger.log() -- this +repo's own native audit:events producer, used by api/routes_audit.py. + +Unlike every other producer in the platform (api-gateway, tes, +workflow-bundles, control-center, rag -- all separate deployables that +hand-port sign_audit_event()), this one lives in the same repo as +audit.signing itself and imports it directly: no drift risk, no parallel +copy to keep in sync. These tests exercise AuditLogger.log()'s new +signing behavior specifically; tests/test_signing.py already covers +sign_audit_event/verify_audit_event in isolation and is untouched. +""" +import json +from unittest.mock import AsyncMock + +import pytest + +from audit.config import AuditConfig +from audit.models import AuditEvent +from audit.signing import verify_audit_event + + +@pytest.mark.asyncio +async def test_log_signs_the_exact_data_string_it_publishes(audit_logger, monkeypatch): + logger, mock_redis = audit_logger + mock_redis.xadd = AsyncMock() + monkeypatch.setattr(AuditConfig, "EVENT_SIGNING_SECRET", "s3cr3t") + + event = AuditEvent(service="security-audit", event_type="platform_admin_action") + await logger.log(event) + + fields = mock_redis.xadd.call_args[0][1] + data = fields["data"] + sig = fields["sig"] + assert verify_audit_event("security-audit", data, sig, "s3cr3t") is True + assert json.loads(data)["event_id"] == event.event_id + + +@pytest.mark.asyncio +async def test_log_includes_both_data_and_sig_fields(audit_logger, monkeypatch): + logger, mock_redis = audit_logger + mock_redis.xadd = AsyncMock() + monkeypatch.setattr(AuditConfig, "EVENT_SIGNING_SECRET", "s3cr3t") + + await logger.log(AuditEvent(service="security-audit", event_type="platform_admin_action")) + + fields = mock_redis.xadd.call_args[0][1] + assert "data" in fields + assert fields["sig"].startswith("v1:") + + +@pytest.mark.asyncio +async def test_log_signature_does_not_verify_under_a_different_secret(audit_logger, monkeypatch): + logger, mock_redis = audit_logger + mock_redis.xadd = AsyncMock() + monkeypatch.setattr(AuditConfig, "EVENT_SIGNING_SECRET", "s3cr3t") + + await logger.log(AuditEvent(service="security-audit", event_type="platform_admin_action")) + + fields = mock_redis.xadd.call_args[0][1] + assert verify_audit_event("security-audit", fields["data"], fields["sig"], "wrong-secret") is False + + +@pytest.mark.asyncio +async def test_log_tampered_data_fails_verification(audit_logger, monkeypatch): + """Proves the signature is bound to this exact payload -- modifying + even one field after signing must invalidate it.""" + logger, mock_redis = audit_logger + mock_redis.xadd = AsyncMock() + monkeypatch.setattr(AuditConfig, "EVENT_SIGNING_SECRET", "s3cr3t") + + await logger.log(AuditEvent(service="security-audit", event_type="platform_admin_action", decision="allow")) + + fields = mock_redis.xadd.call_args[0][1] + tampered = fields["data"].replace('"allow"', '"deny"') + assert verify_audit_event("security-audit", tampered, fields["sig"], "s3cr3t") is False + # the untampered original still verifies -- proves the failure above + # is specifically about the tampering, not a broken signature. + assert verify_audit_event("security-audit", fields["data"], fields["sig"], "s3cr3t") is True + + +@pytest.mark.asyncio +async def test_log_without_a_service_still_publishes_unsigned(audit_logger, monkeypatch): + """AuditEvent.service is a required, non-empty field on the real + model, so this can only happen via a MagicMock stand-in whose + model_dump() omits it -- still must not silently drop the event.""" + from unittest.mock import MagicMock + + logger, mock_redis = audit_logger + mock_redis.xadd = AsyncMock() + monkeypatch.setattr(AuditConfig, "EVENT_SIGNING_SECRET", "s3cr3t") + + fake_event = MagicMock() + fake_event.model_dump.return_value = { + "event_id": "e1", "timestamp": "2024-01-01T00:00:00", "event_type": "x", + "user_id": None, "action": "", "resource": None, "decision": None, + "reason": None, "trace_id": None, "context": {}, + } # no "service" key + await logger.log(fake_event) + + fields = mock_redis.xadd.call_args[0][1] + assert "data" in fields + assert "sig" not in fields + + +@pytest.mark.asyncio +async def test_log_exception_never_leaks_the_secret(audit_logger, monkeypatch, capsys): + logger, mock_redis = audit_logger + mock_redis.xadd = AsyncMock(side_effect=RuntimeError("boom")) + monkeypatch.setattr(AuditConfig, "EVENT_SIGNING_SECRET", "super-secret-value") + + await logger.log( + AuditEvent(service="security-audit", event_type="platform_admin_action") + ) # must not raise + + captured = capsys.readouterr() + assert "super-secret-value" not in captured.out + assert "super-secret-value" not in captured.err + + +def test_config_signing_secret_is_the_one_source_of_truth(): + """AuditConfig.EVENT_SIGNING_SECRET (audit/config.py) was already + defined by PR2 specifically for this eventual purpose -- signing + imports it directly rather than reading JWT_SECRET a second time.""" + import os + + assert AuditConfig.EVENT_SIGNING_SECRET == os.getenv("JWT_SECRET", "change-me") From 8153d384fd4aa44fcd9c38095499ad53feace63e Mon Sep 17 00:00:00 2001 From: Manish Kumar Date: Fri, 14 Aug 2026 14:40:52 -0500 Subject: [PATCH 2/2] ci: fix ruff findings surfaced by PR3b's audit/logger.py changes Lint-only, no behavior change. Fixes the 3 ruff errors this PR's own CI run reported once audit/logger.py entered the changed-file set (ruff lints changed files whole, not just changed lines, so this also caught two pre-existing issues the file already had before PR3b's signing change touched it): - I001: import block un-sorted -- auto-fixed via `ruff check --fix`, grouping stdlib/third-party/first-party with blank lines between, matching this repo's implicit isort convention elsewhere. - F401: `typing.Optional` imported but unused -- pre-existing, never referenced anywhere in this file even before PR3b; removed. - BLE001: blind `except Exception` -- pre-existing, and deliberate: this is the same "NEVER break core system" fire-and-forget contract every audit-write call site in this platform already documents for itself (worker/main.py's three identical except-Exception-print blocks, audit_service.log_event elsewhere). Suppressed with a minimal inline `# noqa: BLE001` plus a one-line reason, matching this repo's existing noqa style (alembic/env.py, tests/conftest.py's `# noqa: CODE -- reason` comments) -- not a broad/file-level suppression, and no repo-wide BLE001 precedent existed to follow, so this establishes the narrowest one for this exact line only. Zero signing-behavior change: data/sig construction, secret source, event schema, and control flow are byte-for-byte identical to before this commit -- confirmed by re-running tests/test_logger_signing.py (7 tests) and tests/test_logger.py (10 tests) unmodified, all still passing. Full suite: 249 passed, 0 regressions (unchanged from the commit this fixes). Co-Authored-By: Claude Sonnet 5 --- audit/logger.py | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/audit/logger.py b/audit/logger.py index 29d3b1b..d433c06 100644 --- a/audit/logger.py +++ b/audit/logger.py @@ -1,6 +1,7 @@ import json + import redis.asyncio as redis -from typing import Optional + from audit.config import AuditConfig from audit.models import AuditEvent from audit.signing import sign_audit_event @@ -41,6 +42,6 @@ async def log(self, event: AuditEvent): maxlen=AuditConfig.MAX_STREAM_LENGTH, approximate=True, ) - except Exception as e: + except Exception as e: # noqa: BLE001 -- intentional fire-and-forget: audit logging must NEVER break core system # NEVER break core system print(f"[AUDIT ERROR] {e}") \ No newline at end of file