[PM-22861] Account security state

[quexten]

🎟️ Tracking

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

📔 Objective

This adds the initial version of account security state to the sdk, on init of the sdk state, and for enrollment to v2 crypto.

Account security state is an attestation to a specific version and featureset that is enabled. The version is used to prevent cryptographic downgrades of the account state.

This will prevent downgrades in the following ways:

• Arbitrary old states cannot be created by the server, because the state is signed for a user by the user's signing key, the server can only record entire states
• A signed in session will not accept a sync with an older state
• Some sign in mechanisms will not accept a sync with an older state (login-with-device 2.0, PRF 2.0, both still to be specified)

A security version then is used to lock down specific features. If a feature is to be disabled, then a migrator is needed. After migrating the account not to use the retired feature, the version is incremented and any subsequent syncs with that format included are rejected.

⏰ Reminders before review

• Contributor guidelines followed
• All formatters and local linters executed and passed
• Written new unit and / or integration tests where applicable
• Protected functional changes with optionality (feature flags)
• Used internationalization (i18n) for all UI strings
• CI builds passed
• Communicated to DevOps any deployment requirements
• Updated any necessary documentation (Confluence, contributing docs) or informed the documentation
  team

🦮 Reviewer guidelines

• 👍 (:+1:) or similar for great changes
• 📝 (:memo:) or ℹ️ (:information_source:) for notes or general info
• ❓ (:question:) for questions
• 🤔 (:thinking:) or 💭 (:thought_balloon:) for more open inquiry that's not quite a confirmed
  issue and could potentially benefit from discussion
• 🎨 (:art:) for suggestions / improvements
• ❌ (:x:) or ⚠️ (:warning:) for more significant problems or concerns needing attention
• 🌱 (:seedling:) or ♻️ (:recycle:) for future improvements or indications of technical debt
• ⛏ (:pick:) for minor or nitpick changes

[github-actions]

Checkmarx One – Scan Summary & Details – bd2f361d-bd6e-4f52-a974-a704a905fb92

Great job, no security vulnerabilities found in this Pull Request

[codecov]

Codecov Report

Attention: Patch coverage is 79.67213% with 62 lines in your changes missing coverage. Please review.

Project coverage is 71.92%. Comparing base (a766ab9) to head (94a3983).

Files with missing lines	Patch %	Lines
crates/bitwarden-crypto/src/security_state.rs	59.30%	35 Missing ⚠️
crates/bitwarden-crypto/src/uniffi_support.rs	0.00%	8 Missing ⚠️
crates/bitwarden-crypto/src/store/context.rs	0.00%	6 Missing ⚠️
crates/bitwarden-core/src/key_management/crypto.rs	96.49%	4 Missing ⚠️
...s/bitwarden-core/src/client/encryption_settings.rs	95.65%	2 Missing ⚠️
...bitwarden-core/src/key_management/crypto_client.rs	0.00%	2 Missing ⚠️
crates/bitwarden-core/src/auth/login/api_key.rs	0.00%	1 Missing ⚠️
...ates/bitwarden-core/src/auth/login/auth_request.rs	0.00%	1 Missing ⚠️
crates/bitwarden-core/src/auth/login/password.rs	0.00%	1 Missing ⚠️
crates/bitwarden-core/src/auth/tde.rs	0.00%	1 Missing ⚠️
... and 1 more

Additional details and impacted files

@@                   Coverage Diff                    @@
##           km/cose-key-rotation     #322      +/-   ##
========================================================
+ Coverage                 71.61%   71.92%   +0.30%     
========================================================
  Files                       225      226       +1     
  Lines                     18426    18689     +263     
========================================================
+ Hits                      13196    13442     +246     
- Misses                     5230     5247      +17     

☔ View full report in Codecov by Sentry.
📢 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.

[Hinton]

Security state seems like it would fit better in the bitwarden-core crate than crypto, similar to EncryptionSettings.

[quexten]

Security state seems like it would fit better in the bitwarden-core crate than crypto, similar to EncryptionSettings.

The reason it lives in crypto is because the namespace definition lives in crypto, and we (so far) commented which specific struct a namespace is for. We can not do that if the struct lives somewhere else. We want to make sure the namespace is only ever used with one struct, to avoid any possibility for cross-protocol attacks where multiple structs are valid for the serialized payload that is signed.

That does make me wonder if namespace should really be a generic parameter, similar to how key context is done with the actual key ids being defined in core.

[sonarqubecloud]

Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.4% Duplication on New Code

See analysis details on SonarQube Cloud

[Hinton]

This seems to only be enforced by convention though, there's nothing in the type system providing compiler level safety?

That does make me wonder if namespace should really be a generic parameter, similar to how key context is done with the actual key ids being defined in core.

I think so, or leaving the code signing concepts in crypto but lifting out the application logic. Or splitting up the crypto crate into multiple more focused areas where some provide low level utilities but other mixing domain logic.

[quexten]

This seems to only be enforced by convention though, there's nothing in the type system providing compiler level safety?

Yeah, that is true, currently this relies on people reading the documentation.

I think we can change this in the long term to:

• Remove namespace as a separate parameter
• Make signable objects have a trait that forces them to provide a namespace
  • This is similar to what we do for content formats

This does not provide complete safety, but at least makes accidental misuse less likely.
I'm open to better suggestions. We may be able to use procmacros to provide a more safe solution that forces a namespace variant to always have one associated struct, but I'm not too familiar here.

[quexten]

I think so, or leaving the code signing concepts in crypto but lifting out the application logic. Or splitting up the crypto crate into multiple more focused areas where some provide low level utilities but other mixing domain logic.

I think resorting the crypto crate is something we wanted to do anyways after "user crypto v2" is done; I recall we had a DM about this with you and Dani, for how this would look. I'd like to defer resorting the crate for after the bigger changes here are done however. Are we OK with putting this in bitwarden-crypto for now, and then looking at this again after the resort is done? (Either moving it into a different place within crypto or moving it out to core?

[Hinton]

I think I'd prefer to just put it in core right now and not have a link in the documentation. It's weird that we have crypto errors that are only thrown by other crates. We can always lift it in later if we think it fits better.