fix(audit): bring authentication events under the tamper-evident chain - #414
Merged
Conversation
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.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
86% of the audit trail — including every authentication record — has no tamper evidence.
The gap
The hash chain is keyed by tenant. Auth events have no tenant (at login a user may belong to several tenants and has not chosen one), so
appendChainEntryreturned early:Live database:
An intruder with database access could delete the record of their own login, or the failed attempts that preceded it, and
GET /audit-logs/verifywould report the trail intact — because it only ever walks rows that were chained.Nothing documented this. Migration 000154 describes the chain as per-tenant and says nothing about excluding authentication. It was a consequence of the design, not a decision.
How I got here (and what I nearly filed instead)
The live logs show
audit chain break80× per run, and my own notes record that I once wrongly declared this control healthy. So I checked the data rather than the code.Truncate→Roundfix from that earlier round is in place; the code is correct now.What the data did show is the real gap above.
The fix — three changes that only work together
1. Tenant-less events extend a dedicated system chain.
Sentinel rather than making
audit_log_chain.tenant_idnullable: that column is a tenant-isolation boundary and loosening it is the more dangerous change. All-Fs is deliberate — its UUID version nibble isf, anduuid.NewV7/uuid.Newcan only emit7or4there, so no generated id can collide with it. All-zeros was rejected for the opposite reason: it isshared.IDs zero value, which call sites already test withIsZero()to mean "unset".2. The verifier walks it — first.
ListActiveTenantIDscan never return the system chain, so the controller adds it explicitly. Writing hashes nobody checks is not tamper evidence. It goes first because a run cut short by its context deadline skips whatever is last, and this is the chain an intruder has the most reason to edit.3.
VerifyChainresolves system entries with a newGetSystemByID(WHERE tenant_id IS NULL). Without this, the tenant-scoped getter cannot see those rows and would report every one of them asaudit_log_missing— a fabricated tamper signal on the control that exists to detect real ones.GetSystemByIDis a separate repository method, not a relaxedGetByTenantAndID. That one is a tenant-isolation boundary; widening it so a sentinel also matches NULL rows is exactly the change that later leaks a real tenants rows. This one can only ever return tenant-less rows.Verification
Tests run through the real repository against a real database — the question is not "does the Go branch take the right path" but "does a row land in
audit_log_chain", and this gap survived precisely because each component was individually correct.Proven fail-before/pass-after by restoring the early return:
Also covered: tenant events still go to their own chain (a fix that swept everything into the system chain would merge tenants trails), the verifier walks the system chain and walks it first, and the sentinel cannot collide with a generated id (asserted as a property, not a constant, plus 2000
NewID()draws).GOWORK=off go build ./...— okGOWORK=off go test ./...againstapp_test— all green (9 test doubles gainedGetSystemByID)GOWORK=off make lint-ci— cleanTradeoff worth naming
Every login now takes
chainMuand does oneLatestChainHashread plus one insert, where before it did neither. All authentication auditing serialises on a single chain. Login rate bounds it and the per-tenant path already had this shape, but on a high-login-rate deployment this is the thing to watch.Not in scope
The 132 historical rows still carry pre-fix hash breaks. Clearing them needs
RebaselineChain, which is admin-gated and must stay a deliberate human action — it is not something a fix PR should trigger.