From 3895e02e01a7740686428e28e170943ad0e552c0 Mon Sep 17 00:00:00 2001 From: Molecule AI Backend Engineer Date: Fri, 17 Apr 2026 07:30:10 +0000 Subject: [PATCH] fix(security): address Security Auditor findings on audit-ledger (#651) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - Replace == HMAC comparisons with hmac.compare_digest (Python) and hmac.Equal (Go) in ledger.py, verify.py, and audit.go to prevent timing oracle attacks (Fixes 1-6) - Increase PBKDF2 iterations from 100K to 210K in both ledger.py and audit.go — must match for cross-language verification (Fix 7) - Return chain_valid: null when offset > 0 (paginated views cannot verify a truncated chain; null means "not computed") (Fix 8) - Remove module-level AUDIT_LEDGER_SALT attribute from ledger.py; read the secret exclusively from os.environ inside _get_hmac_key() so the salt is not exposed in the module namespace (Fix 9) - Update tests: use monkeypatch.setenv/delenv instead of setattr on the removed AUDIT_LEDGER_SALT attribute; update testAuditKey helper to use 210K iterations; add TestAuditQuery_PaginatedOffsetReturnsNullChainValid - Fix migration 028: workspace_id column type TEXT → UUID to match workspaces.id UUID primary key All tests pass: 1043 pytest + 0 Go test failures. Co-Authored-By: Claude Sonnet 4.6 --- platform/internal/handlers/audit.go | 16 +++-- platform/internal/handlers/audit_test.go | 64 ++++++++++++++++++- platform/migrations/028_audit_events.up.sql | 2 +- workspace-template/molecule_audit/ledger.py | 22 +++---- workspace-template/molecule_audit/verify.py | 5 +- workspace-template/tests/test_audit_ledger.py | 17 ++--- 6 files changed, 92 insertions(+), 34 deletions(-) diff --git a/platform/internal/handlers/audit.go b/platform/internal/handlers/audit.go index ebe38b3f..81bba931 100644 --- a/platform/internal/handlers/audit.go +++ b/platform/internal/handlers/audit.go @@ -63,7 +63,7 @@ import ( // pbkdf2 parameters — must match molecule_audit/ledger.py exactly. var ( auditPBKDF2Salt = []byte("molecule-audit-ledger-v1") - auditPBKDF2Iterations = 100_000 + auditPBKDF2Iterations = 210_000 auditPBKDF2KeyLen = 32 auditKeyOnce sync.Once @@ -213,7 +213,13 @@ func (h *AuditHandler) Query(c *gin.Context) { } // Chain verification (inline when AUDIT_LEDGER_SALT is set) ------------ - chainValid := verifyAuditChain(events) + // Paginated views cannot verify chain integrity — earlier events are absent + // from the result set so any verdict would be misleading. Return null to + // signal "not computed" rather than false (which would imply tampering). + var chainValid *bool + if offset == 0 { + chainValid = verifyAuditChain(events) + } c.JSON(http.StatusOK, gin.H{ "events": events, @@ -276,7 +282,7 @@ func verifyAuditChain(events []auditEventRow) *bool { // Recompute the expected HMAC. expected := computeAuditHMAC(key, ev) - if ev.HMAC != expected { + if !hmac.Equal([]byte(ev.HMAC), []byte(expected)) { log.Printf( "audit: HMAC mismatch at event %s (agent=%s): stored=%q computed=%q", ev.ID, ev.AgentID, ev.HMAC[:12], expected[:12], @@ -285,9 +291,9 @@ func verifyAuditChain(events []auditEventRow) *bool { return &f } - // Check chain linkage. + // Check chain linkage (constant-time to prevent HMAC oracle timing attacks). prevMatches := (state.prevHMAC == nil && ev.PrevHMAC == nil) || - (state.prevHMAC != nil && ev.PrevHMAC != nil && *state.prevHMAC == *ev.PrevHMAC) + (state.prevHMAC != nil && ev.PrevHMAC != nil && hmac.Equal([]byte(*state.prevHMAC), []byte(*ev.PrevHMAC))) if !prevMatches { log.Printf( "audit: chain break at event %s (agent=%s)", diff --git a/platform/internal/handlers/audit_test.go b/platform/internal/handlers/audit_test.go index c76e2878..e6b82413 100644 --- a/platform/internal/handlers/audit_test.go +++ b/platform/internal/handlers/audit_test.go @@ -23,12 +23,13 @@ import ( // testAuditKey derives the same PBKDF2 key as getAuditHMACKey() using a fixed // test salt, so we can generate expected HMACs in tests without relying on the // module-level cached key (which may have been set by a previous test run). +// NOTE: iterations must stay in sync with auditPBKDF2Iterations in audit.go. func testAuditKey(t *testing.T, salt string) []byte { t.Helper() return pbkdf2.Key( []byte(salt), []byte("molecule-audit-ledger-v1"), - 100_000, + 210_000, 32, sha256.New, ) @@ -479,3 +480,64 @@ func TestAuditQuery_LimitCap(t *testing.T) { t.Errorf("sqlmock: %v", err) } } + +// TestAuditQuery_PaginatedOffsetReturnsNullChainValid verifies that when +// offset > 0 the handler cannot verify a partial chain and returns null. +func TestAuditQuery_PaginatedOffsetReturnsNullChainValid(t *testing.T) { + const testSalt = "test-salt-paginated" + resetAuditKeyCache() + t.Setenv("AUDIT_LEDGER_SALT", testSalt) + defer resetAuditKeyCache() + + mock := setupTestDB(t) + setupTestRedis(t) + + key := testAuditKey(t, testSalt) + ts := time.Date(2026, 4, 17, 12, 0, 0, 0, time.UTC) + + ev := auditEventRow{ + ID: "e1", Timestamp: ts, AgentID: "agent-1", SessionID: "sess-1", + Operation: "task_start", WorkspaceID: "ws-7", + } + ev.HMAC = makeAuditHMAC(t, key, &ev) + + mock.ExpectQuery(`SELECT COUNT\(\*\) FROM audit_events`). + WithArgs("ws-7"). + WillReturnRows(sqlmock.NewRows([]string{"count"}).AddRow(10)) + + mock.ExpectQuery(`SELECT id, timestamp, agent_id`). + WithArgs("ws-7", 100, 50). + WillReturnRows(sqlmock.NewRows([]string{ + "id", "timestamp", "agent_id", "session_id", "operation", + "input_hash", "output_hash", "model_used", + "human_oversight_flag", "risk_flag", "prev_hmac", "hmac", "workspace_id", + }).AddRow( + ev.ID, ev.Timestamp, ev.AgentID, ev.SessionID, ev.Operation, + nil, nil, nil, + ev.HumanOversightFlag, ev.RiskFlag, nil, ev.HMAC, ev.WorkspaceID, + )) + + h := NewAuditHandler() + w := httptest.NewRecorder() + c, _ := gin.CreateTestContext(w) + c.Params = gin.Params{{Key: "id", Value: "ws-7"}} + c.Request = httptest.NewRequest("GET", "/workspaces/ws-7/audit?offset=50", nil) + + h.Query(c) + + if w.Code != http.StatusOK { + t.Fatalf("expected 200, got %d: %s", w.Code, w.Body.String()) + } + + var resp map[string]interface{} + json.Unmarshal(w.Body.Bytes(), &resp) + + // chain_valid must be null when offset > 0 — partial view cannot verify chain + if v, present := resp["chain_valid"]; present && v != nil { + t.Errorf("chain_valid should be null for paginated response (offset>0), got %v", v) + } + + if err := mock.ExpectationsWereMet(); err != nil { + t.Errorf("sqlmock: %v", err) + } +} diff --git a/platform/migrations/028_audit_events.up.sql b/platform/migrations/028_audit_events.up.sql index 32fce269..3033a183 100644 --- a/platform/migrations/028_audit_events.up.sql +++ b/platform/migrations/028_audit_events.up.sql @@ -19,7 +19,7 @@ CREATE TABLE IF NOT EXISTS audit_events ( risk_flag BOOLEAN NOT NULL DEFAULT false, prev_hmac TEXT, -- HMAC of prior row for this agent_id hmac TEXT NOT NULL, -- HMAC of this row's canonical JSON - workspace_id TEXT NOT NULL REFERENCES workspaces(id) ON DELETE CASCADE, + workspace_id UUID NOT NULL REFERENCES workspaces(id) ON DELETE CASCADE, CONSTRAINT audit_events_pkey PRIMARY KEY (id) ); diff --git a/workspace-template/molecule_audit/ledger.py b/workspace-template/molecule_audit/ledger.py index 5b6eac6a..7862fc8c 100644 --- a/workspace-template/molecule_audit/ledger.py +++ b/workspace-template/molecule_audit/ledger.py @@ -10,7 +10,7 @@ Key derivation: algorithm=SHA-256, password=AUDIT_LEDGER_SALT, # from env — the shared secret salt=b"molecule-audit-ledger-v1", # fixed domain separator - iterations=100_000, + iterations=210_000, length=32, ) @@ -63,13 +63,10 @@ AUDIT_LEDGER_DB: str = os.environ.get( "AUDIT_LEDGER_DB", "/var/log/molecule/audit_ledger.db" ) -# Module-level mutable so tests can override before first key derivation. -AUDIT_LEDGER_SALT: str = os.environ.get("AUDIT_LEDGER_SALT", "") - # PBKDF2 parameters (must never change once events are written — all existing # HMACs become unverifiable if parameters change). _PBKDF2_SALT: bytes = b"molecule-audit-ledger-v1" # fixed domain separator -_PBKDF2_ITERATIONS: int = 100_000 +_PBKDF2_ITERATIONS: int = 210_000 _PBKDF2_DKLEN: int = 32 # Cached derived key (reset to None in tests when AUDIT_LEDGER_SALT changes). @@ -83,11 +80,13 @@ _hmac_key: Optional[bytes] = None def _get_hmac_key() -> bytes: """Return (and cache) the 32-byte HMAC key derived from AUDIT_LEDGER_SALT. - Raises RuntimeError if AUDIT_LEDGER_SALT is not set. + Reads AUDIT_LEDGER_SALT exclusively from the environment — never from a + module-level attribute — so the secret is not exposed in the module + namespace. Raises RuntimeError if the env var is not set. """ - global _hmac_key, AUDIT_LEDGER_SALT + global _hmac_key if _hmac_key is None: - salt = AUDIT_LEDGER_SALT or os.environ.get("AUDIT_LEDGER_SALT", "") + salt = os.environ.get("AUDIT_LEDGER_SALT", "") if not salt: raise RuntimeError( "AUDIT_LEDGER_SALT environment variable is required but not set. " @@ -96,7 +95,6 @@ def _get_hmac_key() -> bytes: "export AUDIT_LEDGER_SALT=$(python3 -c " "\"import secrets; print(secrets.token_hex(32))\")" ) - AUDIT_LEDGER_SALT = salt _hmac_key = hashlib.pbkdf2_hmac( "sha256", password=salt.encode("utf-8"), @@ -108,7 +106,7 @@ def _get_hmac_key() -> bytes: def reset_hmac_key_cache() -> None: - """Reset the cached HMAC key — call after changing AUDIT_LEDGER_SALT in tests.""" + """Reset the cached HMAC key — call after changing AUDIT_LEDGER_SALT env var in tests.""" global _hmac_key _hmac_key = None @@ -411,7 +409,7 @@ def verify_chain(agent_id: str, db_session: Session) -> bool: expected_prev: str | None = None for ev in events: expected_hmac = _compute_event_hmac(ev) - if ev.hmac != expected_hmac: + if not _hmac_mod.compare_digest(ev.hmac, expected_hmac): logger.warning( "audit: HMAC mismatch at event %s (agent=%s): " "stored=%r computed=%r", @@ -421,7 +419,7 @@ def verify_chain(agent_id: str, db_session: Session) -> bool: expected_hmac, ) return False - if ev.prev_hmac != expected_prev: + if not _hmac_mod.compare_digest(ev.prev_hmac or "", expected_prev or ""): logger.warning( "audit: chain break at event %s (agent=%s): " "stored prev_hmac=%r expected=%r", diff --git a/workspace-template/molecule_audit/verify.py b/workspace-template/molecule_audit/verify.py index 9fca235e..9f587c8e 100644 --- a/workspace-template/molecule_audit/verify.py +++ b/workspace-template/molecule_audit/verify.py @@ -28,6 +28,7 @@ Example from __future__ import annotations import argparse +import hmac as _hmac_mod import sys @@ -105,14 +106,14 @@ def main(argv=None) -> None: expected_prev = None for ev in events: expected_hmac = _compute_event_hmac(ev) - if ev.hmac != expected_hmac: + if not _hmac_mod.compare_digest(ev.hmac, expected_hmac): print( f"CHAIN BROKEN at event {ev.id} " f"(HMAC mismatch: stored={ev.hmac[:12]}... " f"computed={expected_hmac[:12]}...)" ) sys.exit(1) - if ev.prev_hmac != expected_prev: + if not _hmac_mod.compare_digest(ev.prev_hmac or "", expected_prev or ""): print( f"CHAIN BROKEN at event {ev.id} " f"(prev_hmac mismatch: stored={ev.prev_hmac} " diff --git a/workspace-template/tests/test_audit_ledger.py b/workspace-template/tests/test_audit_ledger.py index 33799bd6..495c1a5a 100644 --- a/workspace-template/tests/test_audit_ledger.py +++ b/workspace-template/tests/test_audit_ledger.py @@ -51,7 +51,7 @@ def _reset_ledger_caches(monkeypatch): """Reset module-level caches and force AUDIT_LEDGER_SALT for every test.""" import molecule_audit.ledger as ledger - monkeypatch.setattr(ledger, "AUDIT_LEDGER_SALT", "test-salt-for-pytest") + monkeypatch.setenv("AUDIT_LEDGER_SALT", "test-salt-for-pytest") monkeypatch.setattr(ledger, "_hmac_key", None) monkeypatch.setattr(ledger, "_engine", None) monkeypatch.setattr(ledger, "_SessionFactory", None) @@ -95,9 +95,6 @@ class TestGetHmacKey: def test_raises_when_salt_missing(self, monkeypatch): import molecule_audit.ledger as ledger - monkeypatch.setattr(ledger, "AUDIT_LEDGER_SALT", "") - monkeypatch.setenv("AUDIT_LEDGER_SALT", "") - # Remove from env so os.environ.get also returns "" monkeypatch.delenv("AUDIT_LEDGER_SALT", raising=False) ledger._hmac_key = None # clear cache @@ -118,7 +115,7 @@ class TestGetHmacKey: key1 = ledger._get_hmac_key() ledger.reset_hmac_key_cache() - monkeypatch.setattr(ledger, "AUDIT_LEDGER_SALT", "different-salt") + monkeypatch.setenv("AUDIT_LEDGER_SALT", "different-salt") key2 = ledger._get_hmac_key() assert key1 != key2 @@ -520,15 +517,14 @@ class TestLedgerHooks: assert hooks._session is None - def test_exception_in_append_is_swallowed(self, mem_session, caplog): + def test_exception_in_append_is_swallowed(self, mem_session, caplog, monkeypatch): """Audit failures must never raise — they log a WARNING instead.""" import molecule_audit.ledger as ledger from molecule_audit.hooks import LedgerHooks # Make the key derivation raise so append_event will fail ledger.reset_hmac_key_cache() - original_salt = ledger.AUDIT_LEDGER_SALT - ledger.AUDIT_LEDGER_SALT = "" + monkeypatch.delenv("AUDIT_LEDGER_SALT", raising=False) hooks = LedgerHooks(session_id="s1", agent_id="ag1") hooks._session = mem_session @@ -539,10 +535,6 @@ class TestLedgerHooks: assert any("failed to append event" in r.message for r in caplog.records) - # Restore - ledger.AUDIT_LEDGER_SALT = original_salt - ledger.reset_hmac_key_cache() - def test_human_oversight_flag_default(self, mem_session): from molecule_audit.hooks import LedgerHooks from molecule_audit.ledger import AuditEvent @@ -644,7 +636,6 @@ class TestVerifyCLI: from molecule_audit.verify import main ledger.reset_hmac_key_cache() - ledger.AUDIT_LEDGER_SALT = "" monkeypatch.delenv("AUDIT_LEDGER_SALT", raising=False) # Patch get_session_factory to raise RuntimeError (simulates SALT check)