Add PendingProtection for safe protect-cert rotation when secrets land before non-secrets - #8104
Add PendingProtection for safe protect-cert rotation when secrets land before non-secrets#8104justindbaur wants to merge 2 commits into
Conversation
…s land before non-secrets Introduces GlobalSettings.DataProtection.PendingProtection (FileName, Password, Enabled) so operators can stage a new protection certificate's secret before the non-secret blob name is committed. When Enabled=false the entry is completely ignored; when Enabled=true the pending cert becomes the active protection cert and BlobName/CertificatePassword are ignored entirely, making them safe to update in any order during cleanup before deactivating PendingProtection. The old cert must be added to UnprotectCertificates explicitly before activation — there is no auto-demotion — keeping the config explicit and the lifecycle reversible. Tests cover the full rotation lifecycle (activation + cleanup) and carry a warning on the original UpgradePath test that its atomic-deploy assumption is unsafe in practice.
| if (pending is { Enabled: true }) | ||
| { | ||
| dataProtectionCert = DownloadRequiredCertFromBlobStorage( | ||
| globalSettings.Storage.ConnectionString, | ||
| "certificates", | ||
| pending.FileName, | ||
| pending.Password, | ||
| "pending protect" | ||
| ); | ||
| } | ||
| else | ||
| { | ||
| dataProtectionCert = DownloadRequiredCertFromBlobStorage( | ||
| globalSettings.Storage.ConnectionString, | ||
| "certificates", | ||
| globalSettings.DataProtection.BlobName, | ||
| globalSettings.DataProtection.CertificatePassword, | ||
| "protect" | ||
| ); | ||
| } |
justindbaur
left a comment
There was a problem hiding this comment.
Bitwarden Claude Code Review
Overall Assessment: APPROVE
Reviewed the PendingProtection staging mechanism and the Enabled flag on UnprotectCertificates entries. The change solves a real operational problem where secret and non-secret config values land at different times during certificate rotation, previously causing CryptographicException at startup. The design is sound: PendingProtection acts as a safe overlay that makes the standard BlobName/CertificatePassword pair irrelevant while active, and the Enabled=false staging pattern allows secrets to be deployed before their corresponding non-secrets without triggering downloads. Default values are well chosen (CertificateInfo.Enabled defaults to true for backward compatibility, PendingProtection.Enabled defaults to false for safe-by-default). The DownloadRequiredCertFromBlobStorage null-FileName handling provides clear fail-fast error messages with index context. Tests comprehensively walk the full rotation lifecycle across seven intermediate states and cover the fail-fast and disabled-entry edge cases.
Code Review Details
No findings.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #8104 +/- ##
=======================================
Coverage 62.83% 62.84%
=======================================
Files 2299 2299
Lines 100138 100160 +22
Branches 9012 9013 +1
=======================================
+ Hits 62923 62944 +21
- Misses 35030 35031 +1
Partials 2185 2185 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
🎟️ Tracking
📔 Objective
Rotating the data protection certificate in production requires coordinating secret changes (
CertificatePassword,UnprotectCertificates[i].Password) with non-secret changes (BlobName,UnprotectCertificates[i].FileName). Because secrets can land before non-secrets there are intermediate states where passwords and blob names are mismatched, causingCryptographicExceptionat startup and preventing pods from starting.This PR adds two mechanisms to make cert rotation safe regardless of deployment order.
UnprotectCertificates[i].EnabledEach entry in
UnprotectCertificatesnow has anEnabledflag (defaulttrue). SettingEnabled=falsecauses the entry to be skipped entirely at startup, so the password (secret) can be deployed first while the filename (non-secret) is not yet configured. Once both are in place, flipEnabled=truealongsideFileNamein a single non-secret deploy.The previous implementation swallowed errors when
FileNamewas null (returningnullwhich causedNullReferenceExceptiondownstream and led pods to auto-generate divergent key rings). The new implementation is fail-fast: a missingFileNamewithEnabled=truethrowsInvalidOperationExceptionat startup with the index in the message so operators can identify which entry is misconfigured from the log alone.PendingProtectionRotating the protection certificate has the same secret/non-secret ordering problem:
CertificatePasswordandBlobNamemust effectively change together.PendingProtection(FileName,Password,Enabled) solves this with the sameEnabled=falsestaging pattern:Enabled=false— entry is completely ignored; the password secret can be deployed safelyEnabled=true— the pending cert becomes the active protection certificate;BlobNameandCertificatePasswordare ignored entirely while pending is active, so they can be updated to match the new cert in any order without causing a startup failureThere is no auto-demotion of the old cert. The old cert must be added to
UnprotectCertificatesexplicitly (using theEnabled=falsestaging pattern) beforePendingProtectionis activated. OnceBlobName/CertificatePasswordhave been updated to the new cert's values,PendingProtectioncan be deactivated and the config returns to the standard path.Safe rotation procedure
UnprotectCertificates—Enabled=falsefirst, then secret (Password), thenFileName+Enabled=trueas non-secretsPendingProtection—Enabled=falsefirst, then secret (Password), thenFileName+Enabled=trueas non-secretsBlobName/CertificatePasswordin any order (both ignored while pending active), then setPendingProtection:Enabled=falseTests added
UnprotectCertificateCorrectPassword_NoFileName_Throws— documents the fail-fast behaviour whenEnabled=truebutFileNameis missingUnprotectCertificateCorrectPassword_NoFileName_EnabledFalse_Works— showsEnabled=falsesuppresses the error even with a missing filename or password, allowing progressive stagingUpgradePath_WithPendingProtection— walks the full rotation lifecycle (Steps A, B, C) verifying each intermediate state starts cleanly and existing protected data remains accessible throughout; the existingUpgradePathtest carries a warning that its atomic-deploy assumption is unsafe in practice📸 Screenshots
N/A