From 471779c2ff63d348f6294b65383f1b416de95baa Mon Sep 17 00:00:00 2001 From: Nguyen Manh <0xmanhnv@gmail.com> Date: Tue, 4 Aug 2026 08:27:34 +0000 Subject: [PATCH] fix(audit): bring authentication events under the tamper-evident chain MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The audit hash chain is keyed by tenant. Authentication events have no tenant — at login a user may belong to several tenants and has not chosen one yet — so appendChainEntry returned early for them: tenantPtr := log.TenantID() if tenantPtr == nil { return // system-level events bypass the per-tenant chain } On the live database that is 925 of 1075 audit rows, 86%: every auth.login (814), auth.register (87), auth.failed (20) and auth.logout (4). None of them carried any tamper evidence. An intruder with database access could delete the record of their own login, or the failed attempts that preceded it, and GET /audit-logs/verify would report the trail intact — because it only ever walked rows that were chained. Nothing documented this. It was a consequence of the per-tenant design, not a decision: migration 000154 describes the chain as per-tenant and says nothing about excluding authentication. Tenant-less events now extend a dedicated system chain. Why a sentinel tenant id rather than making audit_log_chain.tenant_id nullable: that column is a tenant-isolation boundary and loosening it is the more dangerous change. All-Fs is deliberate — its UUID version nibble is 'f', and uuid.NewV7 / uuid.New can only ever emit 7 or 4 there, so no generated id can collide with it. The all-ZEROS UUID was rejected for the opposite reason: it is the zero value of shared.ID, which several call sites already test with IsZero() to mean "unset". Three things had to change together, and any one of them alone would have been worse than the bug: 1. appendChainEntry appends tenant-less events to SystemChainTenantID. 2. The verifier walks that chain. ListActiveTenantIDs can never return it — it is not a tenant — so the controller adds it explicitly, and FIRST: a run cut short by its context deadline would otherwise skip whatever is last, and this is the chain an intruder has the most reason to edit. Writing hashes nobody checks is not tamper evidence. 3. VerifyChain resolves system entries with a new GetSystemByID (WHERE tenant_id IS NULL) instead of the tenant-scoped getter, which cannot see those rows and would have reported every single one as audit_log_missing — a fabricated tamper signal on the control that exists to detect real ones. GetSystemByID is a separate repository method rather than a relaxed GetByTenantAndID on purpose: that one is a tenant-isolation boundary, and widening it so a sentinel also matches NULL rows is exactly the kind of change that later leaks a real tenant's rows. GetSystemByID can only ever return rows with no tenant. Tests run through the real repository against a real database, because the question is not "does the Go branch take the right path" but "does a row land in audit_log_chain" — and the reason this gap survived is that each component was individually correct. Verified fail-before/pass-after by restoring the early return: "no chain row for a tenant-less auth event" and "the system chain verified 0 entries". Tradeoff worth naming: every login now takes chainMu and does one LatestChainHash read plus one insert, where before it did neither. All authentication auditing serialises on a single chain. Login rate bounds it and the existing per-tenant path already had the same shape, but on a high-login-rate deployment this is the thing to watch. --- internal/app/audit/service.go | 24 ++- internal/app/audit/system_chain_db_test.go | 182 ++++++++++++++++++ internal/app/ingest/audit_chain_test.go | 4 + .../controller/audit_chain_system_test.go | 92 +++++++++ .../infra/controller/audit_chain_verify.go | 9 + .../controller/audit_chain_verify_test.go | 22 ++- .../infra/http/handler/mcp_handler_test.go | 4 + internal/infra/postgres/audit_repository.go | 9 + pkg/domain/audit/repository.go | 36 ++++ tests/unit/audit_service_test.go | 4 + tests/unit/auth_service_test.go | 4 + tests/unit/module_service_test.go | 4 + tests/unit/rule_service_test.go | 8 + tests/unit/secretstore_service_test.go | 8 + 14 files changed, 395 insertions(+), 15 deletions(-) create mode 100644 internal/app/audit/system_chain_db_test.go create mode 100644 internal/infra/controller/audit_chain_system_test.go diff --git a/internal/app/audit/service.go b/internal/app/audit/service.go index a6f2c2b9..df6aedce 100644 --- a/internal/app/audit/service.go +++ b/internal/app/audit/service.go @@ -201,7 +201,17 @@ func (s *AuditService) VerifyChain(ctx context.Context, tenantID shared.ID, limi // 1. Fetch the original audit_log. If it's gone, flag it — a // deleted row is a tamper signal (FK ON DELETE RESTRICT // blocks it in production but not in every path). - log, err := s.auditRepo.GetByTenantAndID(ctx, tenantID, e.AuditLogID) + // System-chain entries point at audit_logs rows with tenant_id IS NULL, + // which the tenant-scoped getter cannot see — using it here would + // report every one of them as audit_log_missing, i.e. a fabricated + // tamper signal on the chain that exists to detect real ones. + var log *auditdom.AuditLog + var err error + if tenantID == auditdom.SystemChainTenantID { + log, err = s.auditRepo.GetSystemByID(ctx, e.AuditLogID) + } else { + log, err = s.auditRepo.GetByTenantAndID(ctx, tenantID, e.AuditLogID) + } if err != nil { res.Breaks = append(res.Breaks, ChainBreak{ AuditLogID: e.AuditLogID.String(), @@ -319,11 +329,15 @@ func (s *AuditService) RebaselineChain(ctx context.Context, tenantID shared.ID, // (pg_advisory_xact_lock) and is not wired here because the current // deployment is single-replica. func (s *AuditService) appendChainEntry(ctx context.Context, log *auditdom.AuditLog) { - tenantPtr := log.TenantID() - if tenantPtr == nil { - return // system-level events bypass the per-tenant chain + // Tenant-less events (every auth.login / auth.register / auth.failed — + // 86% of the trail on the live database) used to return here, which left + // them with no tamper evidence at all. They now extend a dedicated system + // chain instead. See auditdom.SystemChainTenantID for why a sentinel + // rather than a nullable column. + tid := auditdom.SystemChainTenantID + if tenantPtr := log.TenantID(); tenantPtr != nil { + tid = *tenantPtr } - tid := *tenantPtr s.chainMu.Lock() defer s.chainMu.Unlock() diff --git a/internal/app/audit/system_chain_db_test.go b/internal/app/audit/system_chain_db_test.go new file mode 100644 index 00000000..4cb2761d --- /dev/null +++ b/internal/app/audit/system_chain_db_test.go @@ -0,0 +1,182 @@ +package audit_test + +import ( + "context" + "database/sql" + "os" + "testing" + + _ "github.com/lib/pq" + + auditapp "github.com/openctemio/api/internal/app/audit" + "github.com/openctemio/api/internal/infra/postgres" + auditdom "github.com/openctemio/api/pkg/domain/audit" + "github.com/openctemio/api/pkg/logger" +) + +// package audit_test, not audit: this test needs the real postgres repository, +// and internal/infra/postgres -> internal/app -> internal/app/audit, so an +// in-package test would be an import cycle. An external test package can +// depend on packages that depend on the one under test. +// +// Driven through the real repository against a real database, because the +// question this answers is not "does the Go branch take the right path" but +// "does a row land in audit_log_chain". A mock would answer the first and +// prove nothing about the second — and the whole reason this gap existed for +// months is that the components were each individually correct. + +func openAuditDB(t *testing.T) *postgres.DB { + t.Helper() + + dbURL := os.Getenv("DATABASE_URL") + if dbURL == "" { + t.Skip("DATABASE_URL not set; skipping audit system-chain DB tests") + } + db, err := sql.Open("postgres", dbURL) + if err != nil { + t.Skipf("open db: %v", err) + } + t.Cleanup(func() { _ = db.Close() }) + if err := db.PingContext(context.Background()); err != nil { + t.Skipf("cannot reach DATABASE_URL: %v", err) + } + return &postgres.DB{DB: db} +} + +// logSystemAuthEvent writes one tenant-less auth event the way the production +// helper does, and returns its audit_log id. +func logSystemAuthEvent(ctx context.Context, t *testing.T, db *postgres.DB) string { + t.Helper() + + repo := postgres.NewAuditRepository(db) + svc := auditapp.NewAuditService(repo, logger.NewNop()) + + // No TenantID: this is what every auth.login carries. + if err := svc.LogUserLogin(ctx, auditapp.AuditContext{}, "", "chain-test@example.test"); err != nil { + t.Fatalf("log login event: %v", err) + } + + var id string + if err := db.QueryRowContext(ctx, ` + SELECT id FROM audit_logs + WHERE tenant_id IS NULL AND resource_name = $1 + ORDER BY logged_at DESC LIMIT 1`, + "chain-test@example.test").Scan(&id); err != nil { + t.Fatalf("read back the audit log: %v", err) + } + t.Cleanup(func() { + bg := context.Background() + _, _ = db.ExecContext(bg, `DELETE FROM audit_log_chain WHERE audit_log_id = $1`, id) + _, _ = db.ExecContext(bg, `DELETE FROM audit_logs WHERE id = $1`, id) + }) + return id +} + +// The defect: an auth event produced no chain row at all, so deleting or +// editing it left no evidence. +func TestSystemChain_AuthEventIsChained(t *testing.T) { + ctx := context.Background() + db := openAuditDB(t) + + id := logSystemAuthEvent(ctx, t, db) + + var tenantID, hash string + err := db.QueryRowContext(ctx, + `SELECT tenant_id, hash FROM audit_log_chain WHERE audit_log_id = $1`, id, + ).Scan(&tenantID, &hash) + if err != nil { + t.Fatalf("no chain row for a tenant-less auth event (%v). It is stored in "+ + "audit_logs with no tamper evidence: an intruder can delete the record "+ + "of their own login and the verifier will report the trail intact, "+ + "because it only walks rows that were chained", err) + } + + if tenantID != auditdom.SystemChainTenantID.String() { + t.Errorf("chain row tenant_id = %s, want the system chain sentinel %s", + tenantID, auditdom.SystemChainTenantID) + } + if len(hash) != 64 { + t.Errorf("hash = %q, want 64 hex chars", hash) + } +} + +// A chain is only evidence if verification agrees with what was written. This +// catches a write/verify payload mismatch — the failure mode that produced +// months of false "chain break" alerts once before. +func TestSystemChain_VerifiesClean(t *testing.T) { + ctx := context.Background() + db := openAuditDB(t) + + id := logSystemAuthEvent(ctx, t, db) + + repo := postgres.NewAuditRepository(db) + svc := auditapp.NewAuditService(repo, logger.NewNop()) + + res, err := svc.VerifyChain(ctx, auditdom.SystemChainTenantID, 10_000) + if err != nil { + t.Fatalf("VerifyChain: %v", err) + } + if res.Total == 0 { + t.Fatal("the system chain verified 0 entries: nothing is being checked") + } + + for _, b := range res.Breaks { + if b.AuditLogID == id { + t.Fatalf("the entry just written verifies as broken (%s). Note "+ + "audit_log_missing here means the verifier looked the row up with "+ + "the tenant-scoped getter, which cannot see tenant_id IS NULL rows "+ + "— a fabricated tamper signal on the chain that exists to detect "+ + "real ones", b.Reason) + } + } +} + +// Tenant-scoped events must keep going to their own chain. A fix that swept +// everything into the system chain would destroy per-tenant isolation of the +// audit trail. +func TestSystemChain_TenantEventsStillUseTheirOwnChain(t *testing.T) { + ctx := context.Background() + db := openAuditDB(t) + + var tenantID string + if err := db.QueryRowContext(ctx, + `INSERT INTO tenants (id, name, slug) + VALUES (gen_random_uuid(), 'chain test', 'chain-test-' || gen_random_uuid()) + RETURNING id`).Scan(&tenantID); err != nil { + t.Fatalf("seed tenant: %v", err) + } + t.Cleanup(func() { + bg := context.Background() + _, _ = db.ExecContext(bg, + `DELETE FROM audit_log_chain WHERE audit_log_id IN + (SELECT id FROM audit_logs WHERE tenant_id = $1)`, tenantID) + _, _ = db.ExecContext(bg, `DELETE FROM audit_logs WHERE tenant_id = $1`, tenantID) + _, _ = db.ExecContext(bg, `DELETE FROM tenants WHERE id = $1`, tenantID) + }) + + repo := postgres.NewAuditRepository(db) + svc := auditapp.NewAuditService(repo, logger.NewNop()) + + if err := svc.LogUserLogin(ctx, auditapp.AuditContext{TenantID: tenantID}, + "", "tenant-scoped@example.test"); err != nil { + t.Fatalf("log tenant event: %v", err) + } + + var chainTenant string + if err := db.QueryRowContext(ctx, ` + SELECT c.tenant_id + FROM audit_log_chain c + JOIN audit_logs l ON l.id = c.audit_log_id + WHERE l.tenant_id = $1 + ORDER BY c.chain_position DESC LIMIT 1`, tenantID).Scan(&chainTenant); err != nil { + t.Fatalf("no chain row for a tenant-scoped event: %v", err) + } + + if chainTenant == auditdom.SystemChainTenantID.String() { + t.Fatal("a tenant's audit event was appended to the SYSTEM chain, merging " + + "tenants' trails into one shared chain") + } + if chainTenant != tenantID { + t.Errorf("chain tenant = %s, want %s", chainTenant, tenantID) + } +} diff --git a/internal/app/ingest/audit_chain_test.go b/internal/app/ingest/audit_chain_test.go index 61484399..dd21babf 100644 --- a/internal/app/ingest/audit_chain_test.go +++ b/internal/app/ingest/audit_chain_test.go @@ -95,6 +95,10 @@ func (r *chainAuditRepo) GetByTenantAndID(_ context.Context, tenantID, id shared return log, nil } +func (r *chainAuditRepo) GetSystemByID(_ context.Context, id shared.ID) (*audit.AuditLog, error) { + return nil, audit.AuditLogNotFoundError(id) +} + // chainEntryFor returns the chain row covering an audit log, if any. func (r *chainAuditRepo) chainEntryFor(id shared.ID) (audit.ChainEntry, bool) { r.mu.Lock() diff --git a/internal/infra/controller/audit_chain_system_test.go b/internal/infra/controller/audit_chain_system_test.go new file mode 100644 index 00000000..f8ab4a71 --- /dev/null +++ b/internal/infra/controller/audit_chain_system_test.go @@ -0,0 +1,92 @@ +package controller + +import ( + "context" + "testing" + + auditdom "github.com/openctemio/api/pkg/domain/audit" + "github.com/openctemio/api/pkg/domain/shared" +) + +// The audit hash chain is keyed by tenant, and authentication events have no +// tenant — at login a user may belong to several tenants and has not chosen +// one. They were therefore skipped entirely. On the live database that left +// 925 of 1075 audit rows (86%) with no tamper evidence, including every +// auth.login, auth.register and auth.failed. +// +// They now extend a dedicated system chain. But writing hashes nobody checks is +// not tamper evidence, and ListActiveTenantIDs can never return the system +// chain because it is not a tenant. So the controller has to add it, and that +// is what these tests hold in place. + +func TestAuditChainVerify_WalksTheSystemChain(t *testing.T) { + ids := mkTenantIDs(2) + verifier := &chainVerifierMock{} + + c := newTestController(t, verifier, &tenantListerMock{ids: ids}) + if _, err := c.Reconcile(context.Background()); err != nil { + t.Fatalf("Reconcile: %v", err) + } + + var sawSystem bool + for _, got := range verifier.calls { + if got == auditdom.SystemChainTenantID.String() { + sawSystem = true + break + } + } + if !sawSystem { + t.Fatalf("the system chain was never verified. Every authentication event "+ + "lives on it, so its hashes are stored but unchecked — which is not "+ + "tamper evidence. Chains walked: %v", verifier.calls) + } +} + +// A partial run — the context deadline expires part-way — must not be able to +// skip the chain carrying the authentication records. Walking it first is the +// property; asserting "it is in the list somewhere" would not catch a change +// that appends it at the end. +func TestAuditChainVerify_SystemChainIsWalkedFirst(t *testing.T) { + verifier := &chainVerifierMock{} + + c := newTestController(t, verifier, &tenantListerMock{ids: mkTenantIDs(3)}) + if _, err := c.Reconcile(context.Background()); err != nil { + t.Fatalf("Reconcile: %v", err) + } + + if len(verifier.calls) == 0 { + t.Fatal("no chains were verified at all") + } + if verifier.calls[0] != auditdom.SystemChainTenantID.String() { + t.Fatalf("first chain walked = %v, want the system chain. A run cut short "+ + "by its context would skip whatever is last, and the authentication "+ + "trail is the part an intruder has the most reason to edit", + verifier.calls[0]) + } +} + +// The sentinel must never collide with a generated ID. Its version nibble is +// 'f'; uuid.NewV7 and uuid.New can only ever emit 7 or 4 there. This asserts +// the property rather than the constant, so it keeps holding if the value is +// ever changed. +func TestSystemChainTenantID_CannotCollideWithAGeneratedID(t *testing.T) { + sentinel := auditdom.SystemChainTenantID.String() + + if sentinel == (shared.ID{}).String() { + t.Fatal("the sentinel is the zero value of shared.ID, which call sites " + + "already use IsZero() to mean \"unset\"") + } + + // UUID version nibble: character 15 of 8-4-4-4-12. + if v := sentinel[14]; v == '4' || v == '7' { + t.Fatalf("sentinel version nibble is %q — a generated UUID could collide "+ + "with it, and then a real tenant's chain would merge with the system "+ + "chain", v) + } + + for i := 0; i < 2000; i++ { + if shared.NewID().String() == sentinel { + t.Fatal("NewID produced the sentinel") + } + } +} diff --git a/internal/infra/controller/audit_chain_verify.go b/internal/infra/controller/audit_chain_verify.go index 98ff7d75..76a83e26 100644 --- a/internal/infra/controller/audit_chain_verify.go +++ b/internal/infra/controller/audit_chain_verify.go @@ -7,6 +7,7 @@ import ( "github.com/openctemio/api/internal/app/audit" "github.com/openctemio/api/internal/metrics" + auditdom "github.com/openctemio/api/pkg/domain/audit" "github.com/openctemio/api/pkg/domain/shared" tenantdom "github.com/openctemio/api/pkg/domain/tenant" "github.com/openctemio/api/pkg/logger" @@ -131,6 +132,14 @@ func (c *AuditChainVerifyController) Reconcile(ctx context.Context) (int, error) return 0, fmt.Errorf("list active tenants: %w", err) } + // The system chain is not a tenant, so ListActiveTenantIDs will never + // return it — and a chain nobody walks is not tamper-evident, it is just + // stored hashes. It carries every authentication event, which is the part + // of the trail an intruder has the most reason to edit, so it is walked + // FIRST rather than appended at the end where a partial run (ctx deadline) + // could skip it. + tenantIDs = append([]shared.ID{auditdom.SystemChainTenantID}, tenantIDs...) + processed := 0 totalBreaks := 0 newBreaks := 0 diff --git a/internal/infra/controller/audit_chain_verify_test.go b/internal/infra/controller/audit_chain_verify_test.go index 1cd160a1..34b97a49 100644 --- a/internal/infra/controller/audit_chain_verify_test.go +++ b/internal/infra/controller/audit_chain_verify_test.go @@ -101,11 +101,13 @@ func TestAuditChainVerify_CleanChain_NoErrors(t *testing.T) { if err != nil { t.Fatalf("Reconcile: %v", err) } - if processed != 3 { - t.Errorf("processed: want 3, got %d", processed) + // 3 tenants + the system chain. The system chain is not a tenant, so + // ListActiveTenantIDs never returns it; the controller adds it. + if processed != 4 { + t.Errorf("processed: want 4 (3 tenants + system chain), got %d", processed) } - if len(verifier.calls) != 3 { - t.Errorf("verifier called %d times, want 3", len(verifier.calls)) + if len(verifier.calls) != 4 { + t.Errorf("verifier called %d times, want 4", len(verifier.calls)) } } @@ -137,8 +139,8 @@ func TestAuditChainVerify_BreaksStillCountAsProcessed(t *testing.T) { // VerifyChain. The break is emitted via the logger (tested via // absence of error below — visual SIEM alerting is out of scope // for unit tests). - if processed != 2 { - t.Errorf("processed: want 2 (both tenants visited), got %d", processed) + if processed != 3 { + t.Errorf("processed: want 3 (both tenants + system chain), got %d", processed) } } @@ -160,12 +162,12 @@ func TestAuditChainVerify_PerTenantErrorSkipsButContinues(t *testing.T) { t.Fatalf("per-tenant error should not fail the run, got %v", err) } // processed counts only successful verifications; tenant[1] failed. - if processed != 2 { - t.Errorf("processed: want 2 (one failure skipped), got %d", processed) + if processed != 3 { + t.Errorf("processed: want 3 (4 chains, one failure skipped), got %d", processed) } // All three were attempted though. - if len(verifier.calls) != 3 { - t.Errorf("verifier should have been called for all 3 tenants even after one errored; got %d", len(verifier.calls)) + if len(verifier.calls) != 4 { + t.Errorf("verifier should have been called for all 3 tenants + the system chain even after one errored; got %d", len(verifier.calls)) } } diff --git a/internal/infra/http/handler/mcp_handler_test.go b/internal/infra/http/handler/mcp_handler_test.go index ad7e0a10..217f8740 100644 --- a/internal/infra/http/handler/mcp_handler_test.go +++ b/internal/infra/http/handler/mcp_handler_test.go @@ -384,6 +384,10 @@ func (m *fakeAuditRepo) GetByID(_ context.Context, _ shared.ID) (*auditdom.Audit func (m *fakeAuditRepo) GetByTenantAndID(_ context.Context, _, _ shared.ID) (*auditdom.AuditLog, error) { return nil, nil } + +func (m *fakeAuditRepo) GetSystemByID(_ context.Context, id shared.ID) (*auditdom.AuditLog, error) { + return nil, auditdom.AuditLogNotFoundError(id) +} func (m *fakeAuditRepo) List(_ context.Context, _ auditdom.Filter, _ pagination.Pagination) (pagination.Result[*auditdom.AuditLog], error) { return pagination.Result[*auditdom.AuditLog]{}, nil } diff --git a/internal/infra/postgres/audit_repository.go b/internal/infra/postgres/audit_repository.go index e5c6aa10..29106538 100644 --- a/internal/infra/postgres/audit_repository.go +++ b/internal/infra/postgres/audit_repository.go @@ -166,6 +166,15 @@ func (r *AuditRepository) GetByTenantAndID(ctx context.Context, tenantID, id sha return r.scanAuditLog(row, audit.AuditLogNotFoundError(id)) } +// GetSystemByID returns a tenant-less audit log by id — the rows on the +// SystemChainTenantID chain. `tenant_id IS NULL` is part of the query, not a +// caller's responsibility, so this can never return a tenant's row. +func (r *AuditRepository) GetSystemByID(ctx context.Context, id shared.ID) (*audit.AuditLog, error) { + query := r.selectQuery() + " WHERE tenant_id IS NULL AND id = $1" + row := r.db.QueryRowContext(ctx, query, id.String()) + return r.scanAuditLog(row, audit.AuditLogNotFoundError(id)) +} + // List retrieves audit logs matching the filter with pagination. func (r *AuditRepository) List(ctx context.Context, filter audit.Filter, page pagination.Pagination) (pagination.Result[*audit.AuditLog], error) { baseQuery := r.selectQuery() diff --git a/pkg/domain/audit/repository.go b/pkg/domain/audit/repository.go index 92a9c22c..57325eaf 100644 --- a/pkg/domain/audit/repository.go +++ b/pkg/domain/audit/repository.go @@ -67,6 +67,16 @@ type Repository interface { // duplicate. AppendChainEntry(ctx context.Context, entry ChainEntry) error + // GetSystemByID returns a tenant-less audit log by id. It exists so the + // chain verifier can resolve entries on the SystemChainTenantID chain, + // whose audit_logs rows have tenant_id IS NULL. + // + // Deliberately a separate method rather than relaxing GetByTenantAndID: + // that one is a tenant-isolation boundary, and widening it so a sentinel + // matches NULL rows is exactly the kind of change that later leaks a real + // tenant's rows. This one can only ever return rows with no tenant. + GetSystemByID(ctx context.Context, id shared.ID) (*AuditLog, error) + // ListChainEntries returns chain rows for verification. Ordered by // chain_position ASC. ListChainEntries(ctx context.Context, tenantID shared.ID, limit int) ([]ChainEntry, error) @@ -78,6 +88,32 @@ type Repository interface { UpdateChainEntryHashes(ctx context.Context, auditLogID shared.ID, prevHash, hash string) error } +// SystemChainTenantID is the chain that tenant-less audit events are +// appended to. +// +// The hash chain is keyed by tenant, and authentication events genuinely +// have no tenant: at login a user may belong to several tenants and has +// not chosen one yet. So they were skipped — and on the live database +// that meant 925 of 1075 audit rows (86%), including EVERY auth.login, +// auth.register and auth.failed, carried no tamper evidence at all. An +// attacker with database access could delete the record of their own +// login, or of the failed attempts that preceded it, and the chain +// verifier would report the trail intact, because it only ever walked +// rows that were chained. +// +// Nothing documented that exclusion — it was a consequence of the +// per-tenant design, not a decision. +// +// A sentinel is used rather than making audit_log_chain.tenant_id +// nullable, because that column is a tenant-isolation boundary and +// loosening it is the more dangerous change. All-Fs is deliberate: its +// version nibble is 'f', and uuid.NewV7 / uuid.New can only ever emit 7 +// or 4 there, so no generated ID can collide with it. The all-ZEROS +// UUID was rejected for the opposite reason — it is the zero value of +// shared.ID, which several call sites already test with IsZero() to mean +// "unset". +var SystemChainTenantID = shared.MustIDFromString("ffffffff-ffff-ffff-ffff-ffffffffffff") + // ChainEntry is one row of the tamper-evident audit hash-chain. // Mirrors the audit_log_chain table (migration 000154). type ChainEntry struct { diff --git a/tests/unit/audit_service_test.go b/tests/unit/audit_service_test.go index 1524af95..beeb35a9 100644 --- a/tests/unit/audit_service_test.go +++ b/tests/unit/audit_service_test.go @@ -126,6 +126,10 @@ func (m *mockAuditRepo) GetByTenantAndID(_ context.Context, _, id shared.ID) (*a return log, nil } +func (m *mockAuditRepo) GetSystemByID(_ context.Context, id shared.ID) (*audit.AuditLog, error) { + return nil, audit.AuditLogNotFoundError(id) +} + func (m *mockAuditRepo) List(_ context.Context, filter audit.Filter, page pagination.Pagination) (pagination.Result[*audit.AuditLog], error) { m.mu.Lock() defer m.mu.Unlock() diff --git a/tests/unit/auth_service_test.go b/tests/unit/auth_service_test.go index 06da216c..4d45e366 100644 --- a/tests/unit/auth_service_test.go +++ b/tests/unit/auth_service_test.go @@ -714,6 +714,10 @@ func (m *mockAuthAuditRepo) GetByTenantAndID(_ context.Context, _, _ shared.ID) return nil, nil } +func (m *mockAuthAuditRepo) GetSystemByID(_ context.Context, id shared.ID) (*audit.AuditLog, error) { + return nil, audit.AuditLogNotFoundError(id) +} + func (m *mockAuthAuditRepo) List(_ context.Context, _ audit.Filter, _ pagination.Pagination) (pagination.Result[*audit.AuditLog], error) { return pagination.Result[*audit.AuditLog]{}, nil } diff --git a/tests/unit/module_service_test.go b/tests/unit/module_service_test.go index c7895f3b..1e160b0a 100644 --- a/tests/unit/module_service_test.go +++ b/tests/unit/module_service_test.go @@ -208,6 +208,10 @@ func (m *moduleAuditMockRepo) GetByTenantAndID(_ context.Context, _, _ shared.ID return nil, nil } +func (m *moduleAuditMockRepo) GetSystemByID(_ context.Context, id shared.ID) (*audit.AuditLog, error) { + return nil, audit.AuditLogNotFoundError(id) +} + func (m *moduleAuditMockRepo) List(_ context.Context, _ audit.Filter, _ pagination.Pagination) (pagination.Result[*audit.AuditLog], error) { return pagination.Result[*audit.AuditLog]{}, nil } diff --git a/tests/unit/rule_service_test.go b/tests/unit/rule_service_test.go index 0312440d..21cdd02b 100644 --- a/tests/unit/rule_service_test.go +++ b/tests/unit/rule_service_test.go @@ -561,10 +561,18 @@ func (m *ruleSvcMockAuditRepo) GetByID(_ context.Context, _ shared.ID) (*audit.A return nil, errors.New("not implemented") } +func (m *ruleSvcMockSourceRepo) GetSystemByID(_ context.Context, id shared.ID) (*audit.AuditLog, error) { + return nil, audit.AuditLogNotFoundError(id) +} + func (m *ruleSvcMockAuditRepo) GetByTenantAndID(_ context.Context, _, _ shared.ID) (*audit.AuditLog, error) { return nil, nil } +func (m *ruleSvcMockAuditRepo) GetSystemByID(_ context.Context, id shared.ID) (*audit.AuditLog, error) { + return nil, audit.AuditLogNotFoundError(id) +} + func (m *ruleSvcMockAuditRepo) List(_ context.Context, _ audit.Filter, _ pagination.Pagination) (pagination.Result[*audit.AuditLog], error) { return pagination.Result[*audit.AuditLog]{}, nil } diff --git a/tests/unit/secretstore_service_test.go b/tests/unit/secretstore_service_test.go index df6028a2..bf62e3b7 100644 --- a/tests/unit/secretstore_service_test.go +++ b/tests/unit/secretstore_service_test.go @@ -202,10 +202,18 @@ func (m *secretMockAuditRepo) GetByID(_ context.Context, _ shared.ID) (*audit.Au return nil, errors.New("not implemented") } +func (m *secretMockRepo) GetSystemByID(_ context.Context, id shared.ID) (*audit.AuditLog, error) { + return nil, audit.AuditLogNotFoundError(id) +} + func (m *secretMockAuditRepo) GetByTenantAndID(_ context.Context, _, _ shared.ID) (*audit.AuditLog, error) { return nil, nil } +func (m *secretMockAuditRepo) GetSystemByID(_ context.Context, id shared.ID) (*audit.AuditLog, error) { + return nil, audit.AuditLogNotFoundError(id) +} + func (m *secretMockAuditRepo) List(_ context.Context, _ audit.Filter, _ pagination.Pagination) (pagination.Result[*audit.AuditLog], error) { return pagination.Result[*audit.AuditLog]{}, nil }