[PM-27230] Introduce Account Cryptographic State #563
Conversation
|
Great job! No new security vulnerabilities introduced in this pull request |
🔍 SDK Breaking Change Detection ResultsSDK Version:
Breaking change detection completed. View SDK workflow |
quexten
changed the title
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #563 +/- ##
==========================================
+ Coverage 79.71% 79.86% +0.15%
==========================================
Files 300 303 +3
Lines 32238 32543 +305
==========================================
+ Hits 25697 25990 +293
- Misses 6541 6553 +12 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
| ) -> Result<Self, AccountCryptographyInitializationError> { | ||
| let mut ctx = store.context_mut(); | ||
|
|
||
| let user_key = ctx.make_symmetric_key(SymmetricKeyAlgorithm::XChaCha20Poly1305); |
There was a problem hiding this comment.
(Not for this PR)
@dani-garcia Any thoughts on moving these functions out to the domain objects? I.e something like:
let private_key = PrivateKey::make(PublicKeyEncryptionAlgorithm::RsaOaepSha1, &mut ctx)?;similarly, the operations such as wrap would be moved to the respective structs.
so that we only have:
- Delete
- Move
left on the context?
There was a problem hiding this comment.
That sounds reasonable, should also help reduce the amount of functions we expose in a context too
Co-authored-by: Daniel García <[email protected]>
…/sdk-internal into km/account-cryptographic-state
…/sdk-internal into km/account-cryptographic-state
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
🎟️ Tracking
https://bitwarden.atlassian.net/browse/PM-27230
https://bitwarden.atlassian.net/browse/PM-27313
Client PR, fixing breaking changes: bitwarden/clients#17488
📔 Objective
Account cryptographic state describes the core cryptographic objects making up a user. For a V1 encryption user these are: The RSA Keypair. For a V2 user these are:
Not included are:
which are not part of the user's cryptography, but do interact with it.
Provided is a function to generate such a cryptographic state for v2 users, and conversion to API request models. Further, this changes SDK initialization to be based on the account cryptographic state.
🚨 Breaking Changes
Please note that the public API for initializing the user's account cryptography is updated to instead take an enum. The variants for the enum contain the private key for V1 users, and the private key, signing key, signed public key, signed security state for V2 users.
Aside from re-packaging into an enum variant, no other changes should be needed on the consuming side.
This both helps prevent inconsistent states from being passed in by enforcing consistent state via type safetey, but also cleans up the primitive obsession anti-pattern (https://contributing.bitwarden.com/architecture/server/#avoid-primitive-obsession) that the crypto initialization was facing and will make future changes much easier.
Note that since so far clients did not store the signed public key, it is optional for now, but will be made mandatory later after clients save it for a sufficient time.
To migrate, simply repack the existing values into the corresponding enum. Note that previously the signed public key was not passed in, now it is passed in (optional for now, but required later on). Please ensure it is saved to state on sync / login.
⏰ Reminders before review
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 confirmedissue 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