[PM-40801] Add option for alternative DataProtection settings on FIPS nodes

[eligrubb]

🎟️ Tracking

https://bitwarden.atlassian.net/browse/PM-40801

📔 Objective

PKCS#1 v1.5 certificate encryption silently fails when running in a FIPS environment. To support running services in FIPS environments we want the option to adjust DataProtection encryption settings. This PR introduces a new GlobalSettings:DataProtection:KeyProtectionPolicy enum. If the enum is unset or set to Certificate, the server will continue to use DataProtection via PKCS#1 v1.5 certificate encryption when storing the key ring. If the KeyProtectionPolicy is set to StorageManaged, then certificate-based DataProtection is skipped, in favor of storage-level protection at rest.

💭 Implementation decisions

KeyProtectionPolicy should only be set to StorageManaged for new or self-hosted regions. So, this PR does not handle migrating key protection from Certificate to StorageManaged. When KeyProtectionPolicy = StorageManaged, adding certificate handling to the builder is skipped completely, for both Protecting and Unprotecting the key ring.

This setting does not decrease user security. Currently, both the key ring and encrypting certificate are stored using the same connection string. With the StorageManaged setting on, the key ring is still stored using the connection string. No matter the setting, the key ring data is singularly protected by the connection string secret. Using the Certificate policy adds an additional non-FIPS compliant layer. See the related slack discussion for more.

Examples of setting new env var

Using a certificate thumbprint:

  {
    "GlobalSettings": {
      "Storage": {
        "ConnectionString": "<azure-storage-connection-string>"
      },
      "DataProtection": {
        "KeyProtectionPolicy": "Certificate",
        "CertificateThumbprint": "<certificate-thumbprint>"
      }
    }
  }

Or using a certificate stored in Azure Blob Storage:

  {
    "GlobalSettings": {
      "Storage": {
        "ConnectionString": "<azure-storage-connection-string>"
      },
      "DataProtection": {
        "KeyProtectionPolicy": "Certificate",
        "BlobName": "dataprotection.pfx",
        "CertificatePassword": "<certificate-password>"
      }
    }
  }

Certificate is the default, so KeyProtectionPolicy may be omitted.

No application-level certificate wrapping:

  {
    "GlobalSettings": {
      "Storage": {
        "ConnectionString": "<azure-storage-connection-string>"
      },
      "DataProtection": {
        "KeyProtectionPolicy": "StorageManaged"
      }
    }
  }

[github-actions]

🤖 Bitwarden Claude Code Review

Overall Assessment: REQUEST CHANGES

Re-reviewed the GlobalSettings:DataProtection:KeyProtectionPolicy setting and the restructured AddCustomDataProtectionServices flow at commit 3c81610d; no new commits since the previous pass, and no new findings surfaced. The Certificate default remains behaviorally unchanged — blob persistence, certificate acquisition, and the fail-fast InvalidOperationException paths are all preserved — and the new Development early return is equivalent to the previous !env.IsDevelopment() guard because certificates were acquired but never applied there. The StorageManaged path is covered end to end by the new integration test, including cross-instance round-tripping and an assertion that the persisted key ring contains no encryptedSecret. The AppSec question from the prior pass is answered (sign-off confirmed by the author), leaving one open operational concern.

Code Review Details

• ⚠️ : Switching an existing deployment to StorageManaged silently regenerates the key ring rather than failing fast, breaking unprotection of existing User.MasterPassword/User.Key values (open — see existing thread)
  • src/Core/Settings/GlobalSettings.cs:586

Resolved since the previous pass:

• ❓ : Envelope encryption removal on the key ring — AppSec sign-off confirmed by the author.

[claude]

⚠️ IMPORTANT: Switching an existing deployment to StorageManaged silently regenerates the key ring instead of failing fast.

Details and fix

"Migration between types is not supported" is documented here, but nothing enforces it. If StorageManaged is set on a region whose aspnet-dataprotection/keys.xml already holds certificate-wrapped keys, no protect/unprotect certificate is registered, so keys wrapped with the blob-stored certificate can no longer be decrypted. ASP.NET then marks those keys ineligible, generates a fresh key, and the app starts successfully — every value protected with the old keys stops round-tripping, including User.MasterPassword and User.Key (DatabaseFieldProtection, see src/Infrastructure.Dapper/Repositories/UserRepository.cs), so logins break until the config is reverted.

This is the same silent-divergence failure mode that UnprotectCertificateMissingFromBlobStorage_Throws was written to prevent ("pods came up... and auto-generated their own keys — producing a divergent key ring across a rolling deploy").

Suggested guard: when KeyProtectionPolicy == StorageManaged, read keys.xml at startup and throw InvalidOperationException if it exists and contains encryptedSecret — the same signal the new StorageManaged_PersistsUnwrappedKeysWithoutAcquiringCertificates test asserts on.

[claude]

❓ QUESTION: Was the loss of envelope encryption on the key ring confirmed with AppSec?

Context behind the question

This key ring is the wrapping key for Constants.DatabaseFieldProtectorPurpose, which protects User.MasterPassword and User.Key at rest (src/Infrastructure.Dapper/Repositories/UserRepository.cs), plus all DataProtectorTokenFactory tokens.

Under Certificate, unwrapping requires a secret that does not live in the storage account: either CertificatePassword (supplied via configuration, not stored in blob storage) or a certificate installed in the machine store for the thumbprint path. Under StorageManaged, read access to the storage account alone yields directly usable keys — so the PR description's "the key ring data is singularly protected by the connection string secret" holds for the key ring blob but not for the certificate that wraps it.

Two things would help reviewers of this change:

1. Was a FIPS-compatible wrapping mechanism (for example Key Vault-based key wrapping) evaluated before removing application-level wrapping entirely, given Bitwarden's preference for keys at rest being wrapped by another key?
2. If StorageManaged is the accepted outcome, a reference to the AppSec/threat-model decision (rather than only the Slack thread) would make the tradeoff auditable later.

[eligrubb]

Confirmed with AppSec. See https://bitwarden.slack.com/archives/C0AR7RNFZ9D/p1784658228235779?thread_ts=1784319173.826949&cid=C0AR7RNFZ9D

[codecov]

Codecov Report

❌ Patch coverage is 96.77419% with 1 line in your changes missing coverage. Please review.
✅ Project coverage is 62.69%. Comparing base (89a87bc) to head (3c81610).

Files with missing lines	Patch %	Lines
...ities/DataProtectionServiceCollectionExtensions.cs	96.55%	0 Missing and 1 partial ⚠️

Additional details and impacted files

@@           Coverage Diff           @@
##             main    #8092   +/-   ##
=======================================
  Coverage   62.68%   62.69%           
=======================================
  Files        2296     2296           
  Lines      100051   100059    +8     
  Branches     9004     9005    +1     
=======================================
+ Hits        62721    62730    +9     
+ Misses      35147    35146    -1     
  Partials     2183     2183           

☔ 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.