feat(crypto): verify a re-wrap before it overwrites the row

What

RootKeyRotator.RotateAll now proves each new wrapping before it overwrites the row: unwrap it under the provider that produced it, and require the result to equal the key material that was wrapped.

Stacked on !1656 (merged), which adds re-encrypt --scope=root --phase=verify. This MR targets that branch, so the diff shown here is only the round-trip check. Review and merge !1656 (merged) first.

Why

Everything before this took WrapKey's return on trust. RootKeyProvider does not require WrapKey to stamp KeyURI(), so a provider that seals under one key while reporting another passes both existing URI guards. UpdateWrappedKey then replaces the only ciphertext that still opens, and the row's root_key_uri names a key that cannot read it — so nothing afterwards can even say which key it needs.

The sibling MR's --phase=verify finds that row. It finds it after the recoverable ciphertext is gone. This is the preventive half.

The two failure modes stay separate

Sentinel Means
errWrapDoesNotUnwrap The wrapping does not open under its own provider — a misconfigured or mid-rotation key set.
errWrapRoundTripMismatch It opens, but to different key material — a provider returning another row's ciphertext.

The second is the one worth spelling out: within a single namespace the namespace-bound AAD (from !1612 (merged)) matches, and the AEAD authenticates, so nothing else in the stack rejects it. The round-trip comparison is the only thing that does.

The comparison is constant-time. Neither operand is attacker-supplied and both already live in this process, so the leak closed is theoretical — but these are raw key bytes, and comparing key bytes in variable time is not a habit worth having.

Cost

One extra AEAD open per row — measured at ~731 ns per unwrap — against a rotation already making a database round trip per row.

Verification

Check Result
go build, go vet (both tag sets) 0
go test ./internal/... ./cmd/... 0
go test -race (crypto, datastore, cmd) 0
GODEBUG=fips140=only ./internal/crypto/... 0
golangci-lint (untagged + -tags=integration) 0 findings in any file this branch touches
mise run lint:docs 0 errors
internal/crypto integration on PostgreSQL 16 / 17 / 18 0 / 0 / 0
pre-commit scoped to the branch all hooks pass
git merge-tree vs origin/main clean

Non-vacuity. 8 mutations applied, all 8 killed: the check removed entirely; its error discarded; the check moved after the write (detect instead of prevent); the payload comparison dropped; the comparison weakened to a length check; the mismatch reported as unopenable; and the verified plaintext's Zeroize removed.

That last one survived the first pass — the second KEK copy the check obtains had no wipe assertion, since the pre-existing zeroize coverage watches only the OldProvider's buffer. TestRotateAll_ZeroizesTheRoundTripPlaintext closes it by putting the spy on the NewProvider.

The assertion that matters in every failure test is that the row is unchanged. A detected divergence is recoverable only while the old wrapping survives, so aborting after the write would be no better than not checking — which is exactly what the "check moved after the write" mutation confirms.

Database review evidence

None required, and checked rather than assumed. Neither changed file issues SQL — no .QueryContext(, .QueryRowContext( or .ExecContext( in internal/crypto/rotation.go or its test — and no migration is touched. The extra work this MR adds is one in-process AEAD open per row; the statements RotateAll issues are unchanged, and its write-back read path is covered by !1656 (merged)'s evidence.

Reviewable size

+329/−3 across 5 files: 55 lines of implementation, ~240 of tests, ~34 of docs.

e2e scenarios

No scenario in docs/testing/ is added or affected: the catalogs cover client-facing per-format flows, and this changes an operator-run rotation path with no client surface.

Found during the #608 data-consistency review of the merged S04 A/B/C tier.

Related to https://gitlab.com/gitlab-org/ops/artifact-registry/-/work_items/651

Edited by Suleimi Ahmed

Merge request reports

Loading
Loading