fix(security): address Security Auditor findings on audit-ledger (#651)
- 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 <noreply@anthropic.com>
This commit is contained in:
Molecule AI Backend Engineer
parent
fff063bd15
commit
3895e02e01
@ -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)",
|
||||
|
||||
@ -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)
|
||||
}
|
||||
}
|
||||
|
||||
@ -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)
|
||||
);
|
||||
|
||||
|
||||
@ -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",
|
||||
|
||||
@ -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} "
|
||||
|
||||
@ -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)
|
||||
|
||||
Loading…
Reference in New Issue
Block a user