Skip to content

Add PendingProtection for safe protect-cert rotation when secrets land before non-secrets - #8104

Draft
justindbaur wants to merge 2 commits into
mainfrom
allow-partial-unprotect-certificate-config
Draft

Add PendingProtection for safe protect-cert rotation when secrets land before non-secrets#8104
justindbaur wants to merge 2 commits into
mainfrom
allow-partial-unprotect-certificate-config

Conversation

@justindbaur

Copy link
Copy Markdown
Member

🎟️ 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, causing CryptographicException at startup and preventing pods from starting.

This PR adds two mechanisms to make cert rotation safe regardless of deployment order.

UnprotectCertificates[i].Enabled

Each entry in UnprotectCertificates now has an Enabled flag (default true). Setting Enabled=false causes 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, flip Enabled=true alongside FileName in a single non-secret deploy.

The previous implementation swallowed errors when FileName was null (returning null which caused NullReferenceException downstream and led pods to auto-generate divergent key rings). The new implementation is fail-fast: a missing FileName with Enabled=true throws InvalidOperationException at startup with the index in the message so operators can identify which entry is misconfigured from the log alone.

PendingProtection

Rotating the protection certificate has the same secret/non-secret ordering problem: CertificatePassword and BlobName must effectively change together. PendingProtection (FileName, Password, Enabled) solves this with the same Enabled=false staging pattern:

  • Enabled=false — entry is completely ignored; the password secret can be deployed safely
  • Enabled=true — the pending cert becomes the active protection certificate; BlobName and CertificatePassword are ignored entirely while pending is active, so they can be updated to match the new cert in any order without causing a startup failure

There is no auto-demotion of the old cert. The old cert must be added to UnprotectCertificates explicitly (using the Enabled=false staging pattern) before PendingProtection is activated. Once BlobName/CertificatePassword have been updated to the new cert's values, PendingProtection can be deactivated and the config returns to the standard path.

Safe rotation procedure

  1. Stage old cert into UnprotectCertificatesEnabled=false first, then secret (Password), then FileName + Enabled=true as non-secrets
  2. Stage PendingProtectionEnabled=false first, then secret (Password), then FileName + Enabled=true as non-secrets
  3. Cleanup — update BlobName/CertificatePassword in any order (both ignored while pending active), then set PendingProtection:Enabled=false

Tests added

  • UnprotectCertificateCorrectPassword_NoFileName_Throws — documents the fail-fast behaviour when Enabled=true but FileName is missing
  • UnprotectCertificateCorrectPassword_NoFileName_EnabledFalse_Works — shows Enabled=false suppresses the error even with a missing filename or password, allowing progressive staging
  • UpgradePath_WithPendingProtection — walks the full rotation lifecycle (Steps A, B, C) verifying each intermediate state starts cleanly and existing protected data remains accessible throughout; the existing UpgradePath test carries a warning that its atomic-deploy assumption is unsafe in practice

📸 Screenshots

N/A

…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.
Comment on lines +38 to +57
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 justindbaur left a comment

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

codecov Bot commented Jul 31, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 62.84%. Comparing base (8589031) to head (f968222).

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.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant