[PM-31056] Migrate PIN unlock after V2 upgrade#1035
Conversation
quexten
force-pushed
the
km/pin-lock-system
branch
from
7fa4035 to
83eee71
Compare
quexten
force-pushed
the
km/pin-migrate-tmp
branch
from
5a335a3 to
526b3df
Compare
quexten
changed the title
🔍 SDK Breaking Change DetectionSDK Version:
| typescript | ✅ No breaking changes detected | Compilation passed with new SDK version - View Details | | android | ✅ No breaking changes detected | Compilation passed with new SDK version - View Details |Results update as workflows complete. |
🤖 Bitwarden Claude Code ReviewOverall Assessment: APPROVE This PR adds a V1→V2 user-key migration path for the BFU (Before-First-Unlock) persistent PIN envelope, driven from Code Review DetailsNo new findings. The active review threads on this PR cover the open discussion points (AFU coverage, error-path centralization) and a follow-up is most useful in those existing threads rather than as new comments.
|
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #1035 +/- ##
==========================================
+ Coverage 84.56% 84.69% +0.12%
==========================================
Files 427 430 +3
Lines 52930 53459 +529
==========================================
+ Hits 44762 45277 +515
- Misses 8168 8182 +14 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| async fn migrate_pin_envelope_if_needed(&self) { | ||
| let Some(envelope) = self | ||
| .client | ||
| .km_state_bridge() | ||
| .get_persistent_pin_envelope() | ||
| .await | ||
| else { | ||
| return; | ||
| }; |
There was a problem hiding this comment.
❓ QUESTION: Is AFU (AfterFirstUnlock) intentionally out of scope for V1→V2 PIN migration?
Details
migrate_pin_envelope_if_needed returns early when no persistent envelope is present, which is the AFU case. However, in AFU mode encrypted_pin is still persisted (so on_unlock can rebuild the ephemeral envelope after each unlock) and that ciphertext is encrypted with the previous user key.
After a V1→V2 user-key upgrade, the AFU flow appears to be:
migrate_pin_envelope_if_neededreturns early (no persistent envelope).on_unlockcallsencrypted_pin.decrypt(&mut ctx, SymmetricKeySlotId::User)against the new V2 key.- Decryption fails, the warning
"Failed to create PIN envelope"is logged, and no ephemeral envelope is set. get_pin_lock_type()still returnsSome(AfterFirstUnlock)(becauseencrypted_pinis still present), andget_pin_status()returnsNeedsUnlockindefinitely — PIN unlock is silently broken until the user re-enrolls.
If this is intentional (e.g. AFU users are expected to be unaffected because of some flow I'm missing), a code comment in migrate_pin_envelope_if_needed explaining why the early-return branch is safe for AFU would help future readers. If it isn't, AFU users probably want the same V2-upgrade-token-driven recovery path — falling back to unset_pin() so the stale encrypted_pin is cleared and PIN status returns NotSet.
| /// PIN envelope is still encrypted with the V1 user key, this function migrates | ||
| /// the persistent PIN enrollment to be encrypted with the current user-key. | ||
| async fn migrate_pin_envelope_if_needed(&self) { | ||
| let Some(envelope) = self |
There was a problem hiding this comment.
Will be removed when the bridge is merged on all clients.
quexten
changed the title
| warn!("Failed to re-seal PIN envelope after migration, unenrolling PIN"); | ||
| self.unset_pin().await; | ||
| } | ||
| } |
There was a problem hiding this comment.
Seems like every error path is doing self.unset_pin().await; I wonder if it would be easier for the function to return the error and then just do:
if let Err(e) = self.migrate_pin_envelope_if_needed().await {
self.unset_pin().await;
}There was a problem hiding this comment.
Nice. I've made the pin migration return an error enum with the different error states, and have a central log on it (as we do want to have this in e.g. flight recorder), and unenrolling centrally as per your suggestion.
|
Macos build errors are unrelated and happening on other PRs. |
|
3 participants
🎟️ Tracking
https://bitwarden.atlassian.net/browse/PM-31056
📔 Objective
Implements PIN migration to v2 on unlock. Note: Since not all clients have the state bridge implemented yet, this bails out without error if the state bridge is not implemented.
🚨 Breaking Changes