feat(crypto): add user-key id tracking - #1307
Conversation
![github-code-quality[bot]](https://avatars.githubusercontent.com/u/9919?s=60&v=4){kind=small}
| kdf, | ||
| master_key_wrapped_user_key: user_key.clone(), | ||
| salt: email.to_string(), | ||
| user_key_id: None, |
There was a problem hiding this comment.
Thought: Would be nice if we didn't explicitly define this everywhere. Currently any change to unlock methods requires approval from like 4 teams.
🔍 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. |
| } | ||
|
|
||
| #[cfg(feature = "wasm")] | ||
| impl TryFrom<wasm_bindgen::JsValue> for Kdf { |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## km/sync-handler #1307 +/- ##
===================================================
+ Coverage 85.76% 85.79% +0.02%
===================================================
Files 494 495 +1
Lines 71013 71253 +240
===================================================
+ Hits 60906 61130 +224
- Misses 10107 10123 +16 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
🤖 Bitwarden Claude Code ReviewOverall Assessment: APPROVE Reviewed the user-key-id tracking change against base Code Review DetailsNo new findings this round. Previously raised items were handled by the author: the backfill retry behaviour on Dependency ChangesNo third-party dependency changes. |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| async fn handle_user_key_id(client: &Client, data: &CryptoSyncData) { | ||
| let server_user_key_id = data | ||
| .user_decryption | ||
| .as_ref() | ||
| .and_then(|d| d.user_key_id.as_ref()); | ||
| if server_user_key_id.is_some() { | ||
| return; | ||
| } | ||
|
|
||
| info!("Server has no user key id; attempting to report the current one"); | ||
| match client.user_crypto_management().post_user_key_id().await { |
There was a problem hiding this comment.
Details and fix
Two paths repeat indefinitely because nothing records that the attempt already happened:
- V1 accounts (the majority today). An
Aes256CbcHmacuser key carries no key id, which the PR description calls out as unsupported for now. Every sync logsServer has no user key id; attempting to report the current onefollowed byNo user key id available to report. Two INFO lines per sync, per account, forever. - Servers without
POST /accounts/key-management/user-key-id. Self-hosted deployments lag client releases, souser_key_idstays absent in the sync response and each sync spends a round trip on a 404 plus awarn!.
Cheapest fix for the first path is to check whether the key can supply an id before announcing the attempt, so the log only fires when there is something to report:
// Nothing to report for key algorithms that carry no key id (V1 accounts).
match client.user_crypto_management().post_user_key_id().await {
Ok(()) => info!("Reported the user key id to the server"),
Err(PostUserKeyIdError::UserKeyNotAvailable | PostUserKeyIdError::NoKeyId) => {
debug!("No user key id available to report");
}
Err(e) => warn!("Failed to report the user key id: {e:?}"),
}The second path needs the attempt to be remembered (state bridge flag, or a once-per-session guard on the handler) or gated on server capability, otherwise every sync on an older server pays for a request that cannot succeed.
1 participant
https://bitwarden.atlassian.net/browse/PM-39454
bitwarden/server#8076
#1307
bitwarden/clients#22128
We want to track the current key ID of the user-key. This is required for encryption stability. With follow-up work this will allow us to:
The user key id is set as part of the registration for new users. For existing users an upgrade endpoint that can be used once is provided. Key rotations will also set the key id.
Password changes, and kdf changes may not change the key ID and will reject in case the user had a wrong user-key in their client.
SDK Specific notes
We do not want to add syncing logic in Typescript for this. A new crate is introduced "km sync handler" which from now on will handle receiving a sync, and setting data to state. Further, it may react to events -- in this case no key-id being present on the sync; in which case it will post up a new key id to the server.
Please note that AES256_CBC_HMAC_SHA256 keys do not currently support key ids. This will be added in a future ticket.