feat(pii): per-user DEK envelope encryption for event-log PII (spec 19 stage A)#514
Merged
Conversation
…9 / ADR 0030, stage A)
…ge A, #502) - user_encryption_keys migration (010): one KEK-wrapped DEK per user; classified operational with the durable-non-recoverable justification (AC 5) — THE sole non-event-sourced non-recoverable state, per ADR 0030/0029. - crypto: GenerateWrappedDEK / UnwrapDEK (wrap AAD-bound to the owning user via RowAAD(user, user-dek)); DEK.SealField/OpenField seal PII values under a distinct pii:v1 tag with field-level AAD binding — no cross-user, cross-field, or cross-row relocation opens. Legacy plaintext passes through OpenField (documented beta no-backfill stance); a present-but-unwrappable DEK errors (AC 10 groundwork: unwrap-failure is a fault, never erasure). - Red-checked: field-AAD neutralized -> cross-field test fails.
… (spec 19 stage A, #502) - crypto: PIIFieldNames / SealPayloadPII / OpenPayloadPII walk a typed payload's pii:"true" string/*string fields by reflection, sealing under the subject's DEK with the field's json wire name as the AAD binding. Seal returns a copy (the caller's payload is never mutated); Open works in place for the projector decode path; an unsupported tagged field kind fails loudly. The PII set is self-discovered from the tags — no hand list. - store: UserEncryptionKeyRepo (Mint first-write-wins so a race can never REPLACE a DEK that already sealed PII; Get with ErrNotFound as the graceful erased state; idempotent Shred) + sqlc queries + wiring into the Repos facade.
…mpleteness guard (spec 19 stage A, #502) Tags (19 fields): UserCreatedWithRoles / UserProfileUpdated (email, display/given/family name, preferred_username, picture, linux_username), UserEmailChanged, UserLinuxUsernameChanged, IdentityLinked + IdentityLinkLoginUpdated (external_email, external_name — off-stream PII, owner = payload user_id), and TerminalAdminMembershipRevoked.linux_username (explicit spec decision). Deliberately untagged: password_hash (secret, not identity), role_ids/ linux_uid (non-string identifiers), ssh public keys + locale (revisit if a DPO requires it), UserLoggedIn.provider (an IdP slug). TestPIITagGuard_EveryTaggedFieldRoundTripsAndResolvesASubject (AC 3): AST-discovers every tagged field in the package (matches-zero floored at 10), requires a registry entry per tagged struct whose fixture (a) seals to pii:v1 ciphertext and opens back identically through a DEK for EVERY tagged field, and (b) resolves a subject — stream_id for user-stream payloads, a populated owner field otherwise. Registry entries without a tag in source fail as stale. Red-checked in the missing-entry direction.
…age A, #502) - store: PIISealer seam — AppendEvent/AppendEventWithVersion seal PII before marshalling, FAIL-CLOSED (AC 6): any sealing error aborts the append, plaintext PII never reaches the immutable log as a fallback. Nil-safe when unwired (bootstrap/tests that predate the wiring). - internal/pii: Sealer resolves the subject (user-stream → stream_id; off-stream → payload user_id field, erroring when unresolvable), loads + unwraps the DEK (missing AND unwrappable are both append faults), seals via the crypto walker. Opener mirrors it for the projector decode path with the AC 9/10 split: missing DEK row → ErrErased (graceful, caller sentinels), unwrappable → hard error (a KEK fault must never masquerade as erasure). - crypto.HasSealedPII: fast path so legacy plaintext events and factory-seeded map payloads never need a DEK lookup.
…er, test fixtures (spec 19)
…ng path, decrypt-on-project, AC 1/2/4/6 integration tests (spec 19 stage A, #502) - store: SetPIIMinter/MintUserDEK seam (fail-closed when unwired) so every provisioning path mints without constructor plumbing. Minting added to: API CreateUser, SCIM createUser, the SSO linker auto-create (EventAppender grows MintUserDEK; adapter forwards to the store), and the bootstrap admin seed — each BEFORE the creation event, which itself carries PII the sealer needs the key for. - projectors: decodePayloadPII/openSealedPII + SetPIIOpener; ctx threaded through the six PII-bearing FromEvent decoders; the identity link decoders switched from untagged inline structs to the TAGGED shared payloads structs (an untagged shape would have projected ciphertext silently). Refusing to project ciphertext when sealed PII appears with no opener wired. - testutil: SetupPostgres wires sealer/opener/minter under the fixed test KEK (WirePIIEnvelope, exported for the bootstrap-order test); CreateTestUser and the SCIM passwordless factory mint. - cmd/control: boot wires sealer+opener+minter after initEncryptor. - CI: internal/pii joins the store-projectors integration shard (the self-discovering CI-coverage guard caught the gap). - Terminal-admin revocation test updated to the new contract: the event's linux_username is sealed pii:v1 ciphertext, never plaintext (the spec's explicit decision); user_id stays plaintext (it addresses the DEK). Integration tests (real Postgres): user-stream round-trip (ciphertext in events, plaintext in projections, rebuild 1:1 — AC 2/4), off-stream identity-link round-trip via payload user_id subject (AC 2/4), fail-closed append for a subject without a key + nothing written (AC 6), CreateUser mints exactly one wrapped DEK and the creation event carries no plaintext email (AC 1/2), linker auto-create mints before appending (AC 1), re-mint never replaces an existing DEK.
|
Warning Review limit reached
Next review available in: 51 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughThis PR adds per-user DEK-based PII envelope encryption, wires sealing into event append paths, opens or redacts sealed PII during projection decoding, stores wrapped DEKs per user, and mints keys before user-creation events across the main creation flows. ChangesPII Envelope Encryption
Estimated code review effort: 4 (Complex) | ~75 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
internal/pii/envelope_integration_test.go (1)
21-28: 🎯 Functional Correctness | 🔵 Trivial | 💤 Low valueGlobal (non-stream-scoped) event lookup — fine given per-test DB isolation, but fragile if reused elsewhere.
eventFieldselects the most recent event of a givenevent_typeacross the wholeeventstable with nostream_id/stream_typefilter. This is safe today because each test gets its own database and only appends one event of a given type, but if this helper is reused in a test that appends multiple events of the same type, it will silently pick the wrong row instead of failing loudly.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/pii/envelope_integration_test.go` around lines 21 - 28, The helper eventField currently looks up the latest row by event_type across the entire events table, which is fragile outside isolated tests. Update eventField to scope the query by stream identifier as well (using the stream_id/stream_type available from the test context or helper inputs), or otherwise make it fail explicitly when multiple matches exist. Keep the change localized to eventField and any callers that need the additional filter parameters.internal/crypto/pii.go (1)
93-134: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winConsider guarding
SealFieldagainst re-sealing an already-sealed value.
SealFieldtreats any non-empty input as plaintext to encrypt. The shown caller (Sealer.SealEvent, internal/pii/pii.go:56-74) only skips sealing when there are no tagged fields at all — it never checksHasSealedPIIfirst. If an append is ever retried with the same in-memoryEvent.Data(e.g. after a transient store error), the field would be sealed twice: the outerOpenFieldon read would only unwrap once, yielding the still-sealed inner string as the "plaintext" instead of failing loudly the way the rest of this file is designed to (e.g.UnwrapDEK's AC 10 fail-closed stance).Given this file explicitly favors "fail loudly rather than silently corrupt" elsewhere, the same posture would help here.
🔒 Proposed guard against double-sealing
func (d *DEK) SealField(plaintext, field string) (string, error) { if plaintext == "" { return "", nil } + if strings.HasPrefix(plaintext, piiPrefix) { + return "", fmt.Errorf("crypto: refusing to re-seal an already-sealed PII field %s", field) + } if field == "" { return "", errors.New("crypto: refusing to seal PII without a field binding") }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/crypto/pii.go` around lines 93 - 134, SealField currently encrypts any non-empty input, so already-sealed values can be sealed again if Sealer.SealEvent retries with the same Event.Data. Add a guard in SealField to detect and reject or skip inputs already marked as sealed (using the piiPrefix/HasSealedPII check), and update Sealer.SealEvent to avoid calling SealField on values that are already sealed. Keep the behavior fail-closed like UnwrapDEK by ensuring double-sealing does not silently produce nested ciphertext.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@internal/projectors/decode.go`:
- Around line 89-103: Handle the redaction sentinel in the PII decode path
before it reaches listener dispatch or rebuild flow: in `decodePayloadPII` and
the call sites that feed `RebuildAll`/listeners, detect `pii.ErrErased` and
route it to the sentinel projection path instead of returning it as a hard
failure. Use the existing `openSealedPII`, `decodePayloadPII`, and any event
dispatch helpers around `RebuildAll` to ensure erased-user events still project
the erased state rather than aborting the projection pipeline.
---
Nitpick comments:
In `@internal/crypto/pii.go`:
- Around line 93-134: SealField currently encrypts any non-empty input, so
already-sealed values can be sealed again if Sealer.SealEvent retries with the
same Event.Data. Add a guard in SealField to detect and reject or skip inputs
already marked as sealed (using the piiPrefix/HasSealedPII check), and update
Sealer.SealEvent to avoid calling SealField on values that are already sealed.
Keep the behavior fail-closed like UnwrapDEK by ensuring double-sealing does not
silently produce nested ciphertext.
In `@internal/pii/envelope_integration_test.go`:
- Around line 21-28: The helper eventField currently looks up the latest row by
event_type across the entire events table, which is fragile outside isolated
tests. Update eventField to scope the query by stream identifier as well (using
the stream_id/stream_type available from the test context or helper inputs), or
otherwise make it fail explicitly when multiple matches exist. Keep the change
localized to eventField and any callers that need the additional filter
parameters.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 1f866c94-e13f-45a1-91d2-b6885cacc342
⛔ Files ignored due to path filters (2)
internal/store/generated/models.gois excluded by!**/generated/**internal/store/generated/user_encryption_keys.sql.gois excluded by!**/generated/**
📒 Files selected for processing (40)
.github/workflows/test.ymlcmd/control/admin_user.gocmd/control/bootstrap_test.gocmd/control/main.gointernal/api/sso_handler.gointernal/api/system_actions_terminal_admin_test.gointernal/api/user_dek_mint_test.gointernal/api/user_handler.gointernal/crypto/pii.gointernal/crypto/pii_test.gointernal/crypto/pii_walk.gointernal/crypto/pii_walk_test.gointernal/eventtypes/payloads/identity_link.gointernal/eventtypes/payloads/pii_guard_test.gointernal/eventtypes/payloads/terminal.gointernal/eventtypes/payloads/user.gointernal/idp/linker.gointernal/idp/linker_test.gointernal/pii/envelope_integration_test.gointernal/pii/opener.gointernal/pii/pii.gointernal/projectors/decode.gointernal/projectors/identity_provider.gointernal/projectors/identity_provider_listener.gointernal/projectors/identity_provider_test.gointernal/projectors/user.gointernal/projectors/user_listener.gointernal/projectors/user_test.gointernal/scim/users_create.gointernal/scim/users_link_test.gointernal/store/migrations/010_v2026_08.sqlinternal/store/postgres/user_encryption_key.gointernal/store/postgres/wire.gointernal/store/queries/user_encryption_keys.sqlinternal/store/repos.gointernal/store/schema_classification_test.gointernal/store/store.gointernal/store/user_encryption_key.gointernal/testutil/factories_user.gointernal/testutil/postgres.go
…ts (spec 19 AC 7/9)
…ts (spec 19 AC 7/9)
…t (CR finding, #502) CodeRabbit (major): decodePayloadPII documented an ErrErased → sentinel path but the listeners/RebuildAll bubbled it as a hard failure, so replaying a PII event for a crypto-shredded user would abort a live projection or a full rebuild instead of projecting the erased state (AC 9). Handle it at the decode seam once, for all callers: on pii.ErrErased, crypto.RedactPayloadPII collapses every tagged field to the shared RedactionSentinel ('[redacted]', never null/ciphertext) and decode SUCCEEDS. Any OTHER open failure (unwrappable DEK, tampered ciphertext) still aborts (AC 10 — a KEK fault must never masquerade as erasure). Integration test: shred a user's DEK, rebuild completes and reproduces the sentinel (red-checked by neutralizing the catch); crypto unit tests cover the redaction walker. projectors now imports internal/pii for the ErrErased sentinel — not circular (pii imports only store+crypto); the earlier decoupling comment is corrected.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
internal/pii/envelope_integration_test.go (1)
176-177: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valuePrefer the exported
crypto.RedactionSentinelover the hardcoded literal.The production redaction value is defined as an exported constant (
crypto.RedactionSentinel == "[redacted]"ininternal/crypto/pii_walk.go). Asserting against the literal here decouples the test from the source of truth, so a future change to the sentinel would silently pass in production but require a manual test edit.♻️ Use the exported constant
- assert.Equal(t, "[redacted]", u2.DisplayName, - "an erased subject's PII must reproduce as the redaction sentinel") + assert.Equal(t, crypto.RedactionSentinel, u2.DisplayName, + "an erased subject's PII must reproduce as the redaction sentinel")🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/pii/envelope_integration_test.go` around lines 176 - 177, The assertion in the envelope integration test is hardcoded to the redaction string literal instead of using the shared source of truth. Update the test that checks the erased subject’s DisplayName to reference crypto.RedactionSentinel so it stays aligned with the production value defined in the redaction logic, and keep the assertion message unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@internal/crypto/pii_walk.go`:
- Around line 44-64: RedactPayloadPII currently skips any pii:"true" field that
is not a string or *string, so unsupported tagged fields are left unchanged
instead of failing closed. Update the redaction logic in the PII walk code to
mirror walkPII by detecting any tagged field with an unsupported kind and
returning an error rather than continuing, while keeping the existing string and
*string redaction behavior for supported fields.
---
Nitpick comments:
In `@internal/pii/envelope_integration_test.go`:
- Around line 176-177: The assertion in the envelope integration test is
hardcoded to the redaction string literal instead of using the shared source of
truth. Update the test that checks the erased subject’s DisplayName to reference
crypto.RedactionSentinel so it stays aligned with the production value defined
in the redaction logic, and keep the assertion message unchanged.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 9038d32e-a1c4-4c70-9b3e-14561fcccc87
📒 Files selected for processing (4)
internal/crypto/pii_walk.gointernal/crypto/pii_walk_test.gointernal/pii/envelope_integration_test.gointernal/projectors/decode.go
🚧 Files skipped from review as they are similar to previous changes (1)
- internal/projectors/decode.go
… (CR finding, #502) CodeRabbit (minor): RedactPayloadPII silently skipped a pii:"true" field that is neither string nor *string, while walkPII (seal/open) fails loudly on the same. A future non-string PII field would then be left un-redacted in an 'erased' row. Add the matching default-case error so the two walkers agree. The AC 3 guard already blocks such a field from merging (its seal/open round-trip fails), so this is defense in depth. Test pins the fail-closed behavior.
This was referenced Jul 5, 2026
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
1 participant
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.
Part 1 of 4 for #502 (spec 19 — audit retention & crypto-shred erasure). Covers ACs 1–6: the PII envelope. The crypto-shred delete (PR B), retention/prune (PR C), and doctor checks (PR D) follow.
Design (per spec 19)
Every user gets one random 32-byte DEK, stored KEK-wrapped (under the spec-20 single
enc:v1AAD format, bound to the owning user) in the newuser_encryption_keystable — classifiedoperationalwith the durable-non-recoverable justification (AC 5): THE sole non-event-sourced, non-recoverable Postgres state (ADR 0030 amends 0029). PII fields carrypii:"true"on the typed payload structs; the sealer walks them by reflection at append (fail-closed — AC 6), the opener decrypts at projection build (AC 4), so the immutable log holds only ciphertext while projections stay queryable plaintext.stream_id; off-stream PII (identity links, terminal-admin membership) → the payload'suser_idfield; unresolvable → refuse the append.external_email/external_name,TerminalAdminMembershipRevoked.linux_username(the spec's explicit decision). Untagged with reasons in the commit: password_hash (secret, not identity), ssh keys + locale (revisit), provider slugs, non-string ids.pii_guard_test.go): AST-discovers every tagged field (matches-zero ≥10), forces a registry entry per tagged struct whose fixture round-trips every field through a DEK and resolves a subject; stale entries fail. Red-checked.pii.ErrErased(graceful; PR B projects the sentinel); present-but-unwrappable → hard abort, so a KEK fault can never masquerade as erasure.store.MintUserDEKseam; all four provisioning paths mint BEFORE the creation event (API CreateUser, SCIM createUser, SSO linker auto-create, bootstrap admin). First-write-wins: a re-mint can never replace a key that already sealed PII.Tests (real Postgres)
events, plaintext in projections, rebuild reproduces 1:1 (AC 2/4).enc:v1DEK and its creation event carries no plaintext email; linker auto-create asserts mint-before-append; SCIM covered via the (now-minting) factories through the full SCIM suite.Notes
pii:v1:ciphertext for tagged fields — the privacy-correct default; audit UX/export is audit-log: richer web UI + filtering + export #501 (out of scope here).internal/piijoined the store-projectors CI shard (the self-discovering CI-coverage guard caught the gap).coderabbit reviewwas started but has been rate-limit-stalled >30 min; the PR-side review is the gate.Summary by CodeRabbit
New Features
Bug Fixes
Tests