[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 at 5d4825c3. Since the last pass the branch was reduced to a single feature commit plus a fresh merge from main; the merge preserves main's PendingProtection staging flow, the Enabled filter on UnprotectCertificates, and every fail-fast InvalidOperationException path, so the default Certificate policy remains behaviorally identical apart from PersistKeysToAzureBlobStorage now being registered before certificate acquisition (an ordering change with no observable effect, since a throw during acquisition aborts startup either way). The hoisted env.IsDevelopment() early return is equivalent to the previous !env.IsDevelopment() guard because certificates were acquired but never applied in Development; it additionally stops Development startup from failing on unreachable certificate blobs, and existing tests default to Production so none regress. The new StorageManaged short-circuit is confined to the !SelfHosted + storage-connection-string branch, is only reachable via a valid enum value (unknown numeric values fall back to the stricter Certificate path), and is covered end to end by the new Azurite test including cross-instance round-tripping and an assertion that the persisted key ring contains no encryptedSecret.

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; no new comment posted)
  • src/Core/Settings/GlobalSettings.cs:597

Resolved in earlier passes:

• ❓ : Envelope encryption removal on the key ring — AppSec sign-off confirmed by the author.
• ❓ : Self-hosted applicability of the new setting — answered by the author (change targets Bitwarden-deployed FedRAMP regions).

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

[codecov]

Codecov Report

❌ Patch coverage is 96.87500% with 1 line in your changes missing coverage. Please review.
✅ Project coverage is 63.27%. Comparing base (bc45404) to head (5d4825c).

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

❗ There is a different number of reports uploaded between BASE (bc45404) and HEAD (5d4825c). Click for more details.

HEAD has 1 upload less than BASE

Flag	BASE (bc45404)	HEAD (5d4825c)
	2	1

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #8092      +/-   ##
==========================================
- Coverage   68.56%   63.27%   -5.30%     
==========================================
  Files        2381     2381              
  Lines      103902   103910       +8     
  Branches     9405     9406       +1     
==========================================
- Hits        71245    65744    -5501     
- Misses      30310    35916    +5606     
+ Partials     2347     2250      -97     

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

[quexten]

A question, this lives in the !globalSettings.selfhosted branch, so does that mean this setting is not available in self hosted environments? I understood the purpose here to be supporting self hosted fips installations, but maybe I'm missing something?

[eligrubb]

My understanding is that the failure affects the new GovMode cluster, not self hosted environments. I believe self-hosted currently skips persisting the keys and the failing dataprotection settings, this change should only affect BW-deployed FedRamp regions.

[quexten]

A question, other than that looks reasonable.