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