feat(datastore): enforce the namespace_encryption_keys tombstone state in the schema
What
Two CHECK constraints on namespace_encryption_keys, so a row carrying part of the crypto-shred shape cannot be stored:
CONSTRAINT check_namespace_encryption_keys_shredded_inactive
CHECK (shredded_at IS NULL OR active = FALSE)
CONSTRAINT check_namespace_encryption_keys_shredded_key_emptied
CHECK ((shredded_at IS NOT NULL) = (octet_length(wrapped_key) = 0))Why
Crypto-shredding writes one shape — wrapped_key emptied, active cleared, shredded_at stamped — in a single UPDATE in NamespaceEncryptionKeyStore.ShredNamespace, and insertTombstoneForEmptyNamespace writes the same shape for a namespace with no key rows yet. Until now nothing in the database said so. A statement that set some of those three columns and not the others stored a row that no read path classifies, and any direct SQL, future migration, or future store could produce one silently.
What this is and is not. These constraints bound partial and accidental writes — a statement that touches some of the three columns and not the rest. They do not make the erasure guarantee database-enforced, and nothing here constrains a writer who sets all three columns consistently. A consistent rollback by someone holding UPDATE on this table satisfies both CHECKs. See What these constraints do not do.
The second constraint is a biconditional rather than the one-directional "a tombstone carries no key" form, because the reverse half is the half nothing enforced. S04-A's Crypto-shredding section already rules that state out in prose:
Clearing
shredded_atin place is not the re-enable path — it would leave rows present, none active, and the marker gone, a state that matches neither read-path branch and that root-key rotation would try to re-wrap despite the emptywrapped_key.
That state is now unreachable rather than merely unsupported.
The reverse half also backstops a provider defect. RootKeyRotator.rewrapRow validated the URI a provider stamped — refusing "" because it would strand the row — but neither wrap site checked that the provider returned any ciphertext, and UpdateWrappedKey wrote wrapped.Ciphertext unguarded. How reachable that was is worth stating precisely, because an earlier draft of this description overstated it: verifyRewrapRoundTrip (added in c2ed1bf47) already refuses an empty wrapping from any provider whose UnwrapKey is honest, so reaching the write needed a provider broken in both directions at once — returning no ciphertext from WrapKey and returning the exact plaintext from UnwrapKey given that empty input, which subtle.ConstantTimeCompare then accepts. Such a write now fails with SQLSTATE 23514.
That double defect is narrow enough that the schema should not be the primary enforcement point, so this MR also adds the Go guard at both wrap sites (52018d491) — rewrapRow and generateWrapInsert, the latter having had no provider-output validation at all — with the obligation stated on RootKeyProvider.WrapKey and both guards mutation-verified. The CHECK is the backstop; the named errEmptyWrappedCiphertext is the primary refusal. That guard came out of the AppSec review thread on this MR.
What these constraints do not do
Stated here because half a list is worse than none, and the migration header carries the same enumeration:
- A deactivated live row (
active = false,shredded_at IS NULL, non-emptywrapped_key) stays legal and is required: every namespace-key rotation produces one throughdeactivatePriorActive, and rows wrapped under a superseded version must stay decryptable. - A mixed-state namespace — a tombstone beside a live sibling — is still admitted. A CHECK is per-row, so nothing here reaches it; the namespace-wide invariant (any tombstone shreds the whole namespace) stays enforced in Go, on the read paths and, per #432 (closed), the rotation paths.
- A deliberate, consistent rollback is untouched, and this is the limit worth being plain about. Anyone holding
UPDATEon this table can restorewrapped_keybytes and clearshredded_atandactivein one statement, and every CHECK here passes, because the resulting row is a well-formed live key. No row-level constraint can distinguish that from a legitimate write; it needs a control at a different layer (who may issue the statement, and whether the statement is recorded). What these two bound is accidental and partial drift — a code bug, a hand-written UPDATE that forgets a column, a future store that forgets an arm.
Naming
Both names spell the table out rather than following this table's existing check_ns_enc_keys_* names. docs/dev/database.md's three cases for abbreviating a table name are: the spelled-out form does not fit, it lands within a character or two of 63, or the table's other names had to abbreviate. None applies at 49 and 52 characters, and that section names this table's own fk_ns_enc_keys_namespace_id_namespaces as its counterexample — 52 spelled out, so never forced — closing with "Treat an existing short name as history, not as precedent for a new one."
Migration shape
ADD CONSTRAINT ... NOT VALID followed by VALIDATE CONSTRAINT, both in the one transaction goose wraps the section in, following 20260831103000_add_id_uuid_version_checks.sql. A bare validating ADD draws squawk's constraint-missing-not-valid, measured on the shredded_inactive predicate; splitting the pair across two migrations buys nothing, because the ADD holds ACCESS EXCLUSIVE on the parent to COMMIT anyway.
Lock budget, measured rather than derived: the Up holds 66 distinct lock objects — 65 relations under ACCESS EXCLUSIVE (the partitioned parent plus its 64 partitions) and one under ACCESS SHARE. The second ADD CONSTRAINT recurses into the same 65 relations, so it costs no new object, and the two VALIDATEs take SHARE UPDATE EXCLUSIVE on those same relations and share their entries — which is why counting pg_locks rows (131) overstates it. The Down adds an object lock per pg_constraint row dropped (2 × 65), for 196.
VALIDATE aborts the transaction on any existing violating row, which crash-loops pods. Every write this table has taken from the store satisfies both predicates — the insert path writes a non-empty wrapping with shredded_at unset, deactivatePriorActive touches active alone, and both tombstone writers set all three columns together — so the practical risk is low rather than absent. A row hand-written outside the store is what would surface.
Tests
Test-first: test(datastore) lands the assertions red, feat(datastore) makes them pass.
One table of six row shapes drives both statement kinds, in internal/datastore/migrations/namespace_encryption_keys_schema_integration_test.go. Each refused shape violates exactly one constraint, so asserting ConstraintName alongside SQLSTATE 23514 distinguishes them rather than passing on whichever fired:
shredded_at |
active |
wrapped_key |
Verdict |
|---|---|---|---|
| NULL | true |
non-empty | legal |
| NULL | false |
non-empty | legal |
| set | false |
empty | legal |
| set | true |
empty | ..._shredded_inactive |
| set | false |
non-empty | ..._shredded_key_emptied |
| NULL | false |
empty | ..._shredded_key_emptied |
TestNamespaceEncryptionKeysSchema_TombstoneStateCheckspins both whole rendered definitions (not substrings, which a widened predicate would pass) and assertsconvalidated, so dropping theVALIDATEstep fails here.TestNamespaceEncryptionKeysConstraints_TombstoneStateOnInsertand..._OnUpdate. The update arm is not redundant: the shred path is an UPDATE, and a bug that clears one column and not the others arrives as one.
The suite's header comment moves to namespace_encryption_keys_schema_notes.md, following npm_virtual_upstream_rules_schema_notes.md, because the two-line test-file comment cap cannot hold the partition-uniqueness reasoning it carried.
That file also carries an If VALIDATE refuses a row section, pairing the migration header's crash-loop warning with the way out, as id_uuid_version.md does for its own. It gives one row-finding SELECT per constraint and the options per refused shape. Two of the three shapes destroy something whichever way they are resolved, and the section says so rather than implying a clean fix: a live row with an emptied wrapped_key has no material left to recover, and stamping shredded_at to satisfy the constraint trades one dead key version for a dead namespace, because the read paths' tombstone check is namespace-wide rather than row-local.
One fixture was wrong about the tombstone shape rather than inconvenienced by the constraint. retire_integration_test.go's seedKeyRows filled wrapped_key for every fixture, so its {version: 2, active: false, shredded: true} row carried 32 bytes of key material a real tombstone never has. A shredded fixture now defaults to empty bytes. The assertLogsCarryNoKeyMaterial needle for that version goes vacuous in the one subtest that seeds it — an empty needle cannot be passed, since NotContains(x, "") always fails — and the two live rows in that fixture still carry recognizable bytes, so the assertion keeps its content.
Verification
| Check | Result |
|---|---|
go test -tags=integration ./internal/datastore/migrations |
ok 1582.140s |
go test -tags=integration ./internal/datastore |
ok 1988.108s |
go test -tags=integration -run Retire ./cmd/artifact-registry |
ok 84.782s |
goose up / down / up (PG 17, max_locks_per_transaction=1024) |
66.98 ms / 75.60 ms / 96.67 ms |
| goose up / down (PG 17, production-default 64) | 129.13 ms / 86.40 ms |
squawk |
0 issues |
pg_format --inplace |
no reflow |
golangci-lint 2.13.2 --build-tags=integration --new-from-rev=origin/main --max-same-issues=0 --max-issues-per-linter=0 --uniq-by-line=false |
0 issues |
check-comment-caps.sh --base origin/main |
OK |
check-migration-immutability.sh origin/main |
OK |
check-integration-test-wiring.sh |
OK |
vale |
0 errors |
The two new table-driven tests add 20.5 s to the migrations suite (11.80 s + 8.71 s measured with -v); TestMigrations_UpDownUp and the template materialization dominate the rest.
Both //nolint:wrapcheck tokens on the new helpers were confirmed by removing them and watching wrapcheck fire three times.
go test ./... has one pre-existing failure unrelated to this branch — TestWireEncryption_NilInfraConfigResolvesLabKitDefault — reproduced identically on a pristine origin/main tree.
The branch does touch production Go, in the two commits that answer the AppSec review: 16 insertions across internal/crypto/{namespace_key,provider,rotation}.go — two len(wrapped.Ciphertext) == 0 guards and the errEmptyWrappedCiphertext sentinel. Re-verified at ed8fa428c:
| Check | Result |
|---|---|
go test -race ./internal/crypto/... |
ok 2.037s |
golangci-lint 2.13 --build-tags=integration --new-from-rev=origin/main over internal/crypto, internal/datastore/migrations, cmd/artifact-registry |
0 issues |
squawk (after the header rewrite) |
0 issues |
pg_format (after the header rewrite) |
no reflow |
check-comment-caps.sh --base origin/main |
OK |
check-migration-immutability.sh origin/main |
OK |
vale docs/specs/S04-a-column-level-encryption.md |
0 errors |
Pipeline 2816544097 on ed8fa428c is green: 71 jobs succeeded, 2 manual, none failed — including test:integration, test:integration:datastore, and test:integration:migrations on PostgreSQL 16, 17, and 18, and test:crypto-fips.
Diff size
git diff --stat origin/main...HEAD reads 667 insertions across 14 files, over the 500-LOC line development-model.md draws. The reviewable total is 537, still over it, so this is the justification that rule asks for rather than a claim of being under.
| Group | Insertions | Deletions |
|---|---|---|
structure.sql (regenerated by mise run db:dump-structure) |
130 | 0 |
| Migration SQL | 139 | 0 |
| Tests | 276 | 15 |
| Production Go | 16 | 0 |
| Docs and schema notes | 106 | 3 |
| Total | 667 | 18 |
structure.sql is generated, and its 130 lines are the same two CONSTRAINT lines repeated across the partitioned parent and its 64 partitions. Excluding it leaves 537 reviewable insertions.
Why this is not split. The reviewable bulk is not a code body. Of the 139 migration lines, 10 are SQL and 129 are the header comment. Of the 276 test insertions, 186 are one table-driven suite in a single file and 81 are the guard tests that arrived with the AppSec response. Production Go is 16 lines — two len(wrapped.Ciphertext) == 0 guards, one sentinel, and their comments.
Splitting would have to cut between the schema constraints and the Go guards, and the two are coupled in both directions: the guards exist because the CHECK made the unguarded wrap path visible, and the migration header now names errEmptyWrappedCiphertext when it explains why the CHECK is a backstop rather than the primary enforcement. Landing them apart would either strand that reference or invert the order the reasoning depends on. The alternative cut — tests away from the code they pin — is the one this repository's test-first rule forbids.
Notes for the reviewer
- Drive-by truthfulness fix. S04-A's
CREATE TABLEblock was missingcheck_namespace_encryption_keys_id_uuid_version, which7ab2cec85added to this table on 2026-09-01. It is added alongside the two new constraints so the block matches the catalog. - Overlap.
git diff --name-only main...HEADoverlaps Draft !1011 (closed) onstructure.sqlandmigrations_checksum_test.go'sknownHeadVersion. Both are regenerated or mechanical, and whichever lands second regenerates them; no pipeline reports the conflict. - No e2e scenario is added or affected. Nothing consumes the encryption stack from a request path:
keyManagerandrowEncryptorare set on the wiring struct and read by no handler, and the only production consumers are the operator CLI and thecredential_sweepit drives. No credential table carrieswrapped_dekor anencrypted_*column until #417 (closed) lands them, anddocs/testing/contains no scenario touching this table.
Database Review Evidence
Collected against ed8fa428c (pipeline 2816544097), the head that carries the migration as it will merge.
Migrations
Note
Timings are from CI (db:migrate matrix, goose verbose) against an
empty database, in apply / rollback order per PG version.
Production-scale validation via Database Lab is not yet available. See
Database review evidence
for the matrix rationale and how to read the numbers.
| Migration | PG 16 | PG 17 | PG 18 |
|---|---|---|---|
20260902145931_add_ns_enc_keys_tombstone_state_checks.sql |
OK (53.29ms / 44.16ms) | OK (51.7ms / 44.57ms) | OK (73.92ms / 40.42ms) |
Migration notes: no anomalies on any of the four checks. The slowest apply (PG 18, 73.92ms) is 1.39x the second-slowest, under the 2x version-regression threshold; every apply is far under the 1s empty-database mark and the 5-minute boot budget; all six phases report OK; and the widest up/down spread is 1.83x on sub-100ms figures. The apply recurses ADD CONSTRAINT into the partitioned parent and all 64 partitions twice and validates both constraints, which is what the ~50-75ms covers. VALIDATE scans for violating rows, so these figures are an empty-database floor — the scan is one row per namespace key version at production scale, and no deployed environment holds any.
Query mode: did not run, and correctly so. The MR changes three non-test Go files — internal/crypto/{namespace_key,provider,rotation}.go — and none dispatches a statement through any signature on the skill's dispatch list (QueryContext, QueryRowContext, ExecContext, instrumentQuery, instrumentExec, execAffected). Package internal/crypto names no table and issues no SQL by design, so there is no statement to plan. Every other changed Go file is a *_test.go, and the regenerated structure.sql is neither a query nor a migration to time.
Related to https://gitlab.com/gitlab-org/ops/artifact-registry/-/work_items/1101