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_at in 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 empty wrapped_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-empty wrapped_key) stays legal and is required: every namespace-key rotation produces one through deactivatePriorActive, 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 UPDATE on this table can restore wrapped_key bytes and clear shredded_at and active in 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_TombstoneStateChecks pins both whole rendered definitions (not substrings, which a widened predicate would pass) and asserts convalidated, so dropping the VALIDATE step fails here.
  • TestNamespaceEncryptionKeysConstraints_TombstoneStateOnInsert and ..._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 TABLE block was missing check_namespace_encryption_keys_id_uuid_version, which 7ab2cec85 added 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...HEAD overlaps Draft !1011 (closed) on structure.sql and migrations_checksum_test.go's knownHeadVersion. 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: keyManager and rowEncryptor are set on the wiring struct and read by no handler, and the only production consumers are the operator CLI and the credential_sweep it drives. No credential table carries wrapped_dek or an encrypted_* column until #417 (closed) lands them, and docs/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

Edited by Suleimi Ahmed

Merge request reports

Loading
Loading