[PM-31128] Add reinit_user_crypto for mobile#1148
Conversation
🔍 SDK Breaking Change DetectionSDK Version:
Breaking change detection uses the build of the SDK from this branch, including any incompatibities pre-existing on or merged into this branch. Check the workflow logs to confirm. |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #1148 +/- ##
==========================================
+ Coverage 83.86% 84.17% +0.30%
==========================================
Files 438 447 +9
Lines 56923 59104 +2181
==========================================
+ Hits 47738 49749 +2011
- Misses 9185 9355 +170 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
| // The key store should not already have any keys initialized | ||
| if ctx.has_symmetric_key(SymmetricKeySlotId::User) | ||
| || ctx.has_private_key(PrivateKeySlotId::UserPrivateKey) | ||
| || ctx.has_signing_key(SigningKeySlotId::UserSigningKey) | ||
| { | ||
| return Err(EncryptionSettingsError::CryptoInitialization); | ||
| } |
There was a problem hiding this comment.
Moving this check upstream seem like the best approach. Open to other suggestions.
quexten
left a comment
quexten
left a comment
There was a problem hiding this comment.
A few smaller nits, but more importantly the PIN migration is missing.
| .internal | ||
| .get_user_id() | ||
| .ok_or(ReinitUserCryptoError::LocalUserDataMigrationFailed)?; | ||
| migrate_local_user_data_key_for_user_key_upgrade(client, user_id) |
There was a problem hiding this comment.
We are migrating the local user data key but are not migrating the PIN key here, which will leave the PIN encrypted with the old key. Ideally we'd extract these migrations so that we cannot get them inconsistently, between init and reinit.
PinLockSystem::on_unlock(&PinLockSystem::with_client(client)).await;
There was a problem hiding this comment.
I consolidated them into a shared on_unlock_handler.
Something that I would like to hear your opinion on. Looking at this, all our local migrations need and assume the v2_upgrade_token is set in state. Should we change reinit_user_crypto to also require the upgrade token set in state instead of passed in via the request? I would think having one source of truth would be desirable.
There was a problem hiding this comment.
Something that I would like to hear your opinion on. Looking at this, all our local migrations need and assume the v2_upgrade_token is set in state. Should we change reinit_user_crypto to also require the upgrade token set in state instead of passed in via the request? I would think having one source of truth would be desirable.
IMO, in the long term:
- All public functions should have only legitimate variables (that vary) passed in, never state
- I.e "unlock({pin, pin envelope, cryptographic state})" -> BAD
- "unlock(pin)" -> GOOD
State should be KM internal concern, not public API.
Subsequently, we should not take the v2 token as public API if we do not have to, we also should not take in the account cryptographic state as public API if we not have to.
In the long term, the vision for how this would work is:
- SDK receives "on sync" handler, which gets triggered by a sync notification, and passes in a response model
- KM has a sync handler registered, that takes the data and sets the data to state
- The KM handler dispatches a call to reinit crypto, and reinit crypto is not a public API
However in this case we give the reinit API precisely because the above described desired state does not yet exist. Further, it is easy for mobile to forget to set the pre-conditions (setting the tokens, etc). So for this API, given that it is the bridge until we implement the above sdk consolidation, we should make it as safe for mobile to not be able to mis-use it. Subsequently, having the parameters required in the parameter set seems safer that accessing through state, as in the latter case mobile could forget to set the state, even though it is a worse public API.
There was a problem hiding this comment.
I do want to simplify the unlock APIs as much as possible however, though, it is possible to mis-use it. Maybe we should make a ticket to implement a simplified version of "set data to state after sync in SDK" so that we can take that risk / burden off of mobile.
There was a problem hiding this comment.
@quexten Going to recap here to make sure we are both on the same page.
For the local user data key migration and the PIN on_unlock, both require the getting the upgrade token from state.
So for a time mobile will pass in the upgrade token and not have the state bridge implemented and this will just upgrade the userKey? Then when mobile implements the state bridge we will need them to set the upgrade token into state and pass it in here for things to work. The other option is to have the upgrade token pass into reinit_user_crypto and us set the upgrade_token into state there. The local data migrations don't work without the upgrade token set to state.
There was a problem hiding this comment.
Okay I dug into this deeper and I think I've covered all of our concerns with c1620a2.
First the idea that that we can upgrade just the userKey without the state bridge is incorrect. This would corrupt the localUserDataKey with how we have things setup. To solve for this we can just guard the entire reinit_user_crypto to only run when a state bridge is registered.
The concern I had with different sources for the upgrade token and your concern with mobile having to set state prior to calling can be solved as follows. Have the request model require the upgrade token and have reinit_user_crypto responsible for writing that to state bridge state prior to calling local migrations.
coroiu
left a comment
coroiu
left a comment
There was a problem hiding this comment.
Ping me when this is ready for merge :)
|
3 participants
🎟️ Tracking
https://bitwarden.atlassian.net/browse/PM-31128
📔 Objective
Add functionality to
CryptoClientto allow mobile applications that receive a newaccountCryptographicStateandV2UpgradeTokenfrom a userKey rotation upgrade sync to be able to reinit SDK's cryptography state.