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

This PR stores the account security state on the client for v2 users. However, besides validating that the state is correctly signed, it is not yet used.

⏰ 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 – 259e40f1-ef16-40e7-b433-e2353dedc39b

Great job, no security vulnerabilities found in this Pull Request

[codecov]

Codecov Report

Attention: Patch coverage is 89.09657% with 70 lines in your changes missing coverage. Please review.

Project coverage is 72.72%. Comparing base (81493f3) to head (736f744).
Report is 2 commits behind head on main.

Files with missing lines	Patch %	Lines
...itwarden-core/src/key_management/security_state.rs	74.35%	20 Missing ⚠️
crates/bitwarden-core/src/key_management/crypto.rs	96.59%	9 Missing ⚠️
crates/bitwarden-core/src/uniffi_support.rs	0.00%	8 Missing ⚠️
crates/bitwarden-core/src/auth/tde.rs	0.00%	7 Missing ⚠️
crates/bitwarden-crypto/src/store/context.rs	93.87%	6 Missing ⚠️
crates/bitwarden-core/src/auth/login/api_key.rs	0.00%	5 Missing ⚠️
crates/bitwarden-core/src/auth/login/password.rs	0.00%	5 Missing ⚠️
...bitwarden-core/src/key_management/crypto_client.rs	0.00%	5 Missing ⚠️
crates/bitwarden-core/src/client/internal.rs	95.45%	2 Missing ⚠️
...ates/bitwarden-core/src/auth/login/auth_request.rs	0.00%	1 Missing ⚠️
... and 2 more

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #322      +/-   ##
==========================================
+ Coverage   71.86%   72.72%   +0.85%     
==========================================
  Files         240      241       +1     
  Lines       19635    20154     +519     
==========================================
+ Hits        14111    14656     +545     
+ Misses       5524     5498      -26     

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

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

[quexten]

Moving this to draft until the preceeding PR is merged.

[quexten]

This moves more to the SDK than had been previously in the SDK. Since this API is not used yet a breaking change seemed OK.

[sonarqubecloud]

Quality Gate passed

Issues
0 New issues
0 Accepted issues

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

See analysis details on SonarQube Cloud