[PM-28263] Clear PIN on key rotation#2241
Conversation
|
Claude finished @matt-livefront's task in 6m 11s —— View job Review Summary - PR #2241: Clear PIN on Key RotationChanges OverviewThis PR addresses PIN handling when key rotation occurs by:
Evaluation of Previous Review Concerns1. Switch Statement Unification (Behavioral Change)
2. Error Handling Breadth // Note: We handle all errors broadly here because the SDK doesn't provide specific
// error types to distinguish key rotation failures from other errors. Clearing the
// PIN keys on any error is the safest approach to maintain data consistency.Analysis:
Recommendation: This is acceptable as-is. If the SDK team later provides specific error types for key rotation, this logic could be refined. 3. Test Coverage
These tests verify:
Minor Observations:
Code QualityStrengths:
Security: Correctness: Breaking Changes: RecommendationAPPROVED ✅ This PR correctly implements PIN clearing on key rotation with well-reasoned design decisions. The refactoring improves code maintainability by consolidating duplicate logic, and the test coverage adequately verifies the new behavior. The broad error handling is an acceptable pragmatic trade-off given SDK limitations, and is properly documented in the code. Optional Enhancement (non-blocking): Consider adding |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| try await stateService.setPinProtectedUserKeyToMemory(enrollPinResponse.pinProtectedUserKeyEnvelope) | ||
| await flightRecorder.log("[Auth] Set PIN-protected user key in memory") | ||
| } | ||
| } |
There was a problem hiding this comment.
CRITICAL: Unintended behavioral change for non-password unlock methods
The refactoring removed the switch method statement, which changes behavior for .authRequest, .deviceKey, .keyConnector, and .pin unlock methods.
Old behavior (for authRequest/deviceKey/keyConnector/pin):
- If
encryptedPinexists → always migrate to PIN-protected user key envelope - No conditional checks on
pinProtectedUserKeyorpinUnlockRequiresPasswordAfterRestart
New behavior (unified for all methods):
- If
encryptedPinexists → only migrate ifpinProtectedUserKey != nilAND!pinUnlockRequiresPasswordAfterRestart - Otherwise, if
pinProtectedUserKey == nilANDpinUnlockRequiresPasswordAfterRestart→ set in memory - Otherwise → do nothing (no migration, no logging)
Impact:
Users who unlock with device key/key connector/auth request and have a legacy PIN may not get their PIN migrated to the new format, potentially breaking PIN unlock after app restart.
Recommendation:
If this behavioral change is intentional, it needs:
- Explicit documentation explaining why the logic is now unified
- Test coverage for other unlock methods (device key, biometrics, etc.) to verify correct behavior
- A log message for the else case (when neither migration nor in-memory setting occurs)
If unintended, consider restoring the switch statement and only adding error handling, not changing the migration logic.
There was a problem hiding this comment.
If encryptedPin exists → always migrate to PIN-protected user key envelope
This was originally set up just for the pin case. Once the PIN was migrated, the unlock method would change to pinEnvelope. So at that time, the migration was just done once and then you'd be in a different branch of the switch statement. After adding the authRequest/deviceKey/keyConnector cases, we should have added a check to see if the migration had already been done to prevent unnecessary work.
To support the key rotation fix, I needed to add the enrollPinWithErrorHandling call to all switch cases. There was already similarities between the two branches in the switch statement, so unifying them made the most sense to me and I think is clearer overall without separate branches based on the unlock method.
| /// unlock process to continue without erroring). | ||
| /// - Throws: An error if clearing the PIN keys fails. | ||
| /// | ||
| private func enrollPinWithErrorHandling(encryptedPin: String) async throws -> EnrollPinResponse? { |
There was a problem hiding this comment.
IMPORTANT: Error handling may be too broad
This catches all errors and clears the PIN, including:
- Network failures
- Temporary SDK errors
- Memory/system issues
- Actual key rotation (the intended case)
Security consideration:
Clearing a valid PIN due to a transient error could frustrate users and reduce security (they might disable PIN entirely).
Recommendation:
Consider checking the error type to only clear PIN for specific errors indicating key rotation/invalid cryptographic material:
catch let error as CryptoError where error.isKeyRotationError {
// Clear PIN only for key rotation errors
}If the SDK doesn't provide specific error types, document why broad error handling is necessary and acceptable for this security-critical operation.
|
|
||
| // `unlockVaultWithPassword(_:)` unlocks the vault with the user's password and clears an | ||
| // existing PIN if `enrollPinWithEncryptedPin(encryptedPin:)` fails. | ||
| func test_unlockVaultWithPassword_enrollPinWithEncryptedPinError() async throws { |
There was a problem hiding this comment.
IMPORTANT: Missing test coverage for other unlock methods
This test only covers unlockVaultWithPassword, but the change affects all unlock methods that call unlockVault(method:):
unlockVaultWithDeviceKey()unlockVaultWithBiometrics()unlockVaultFromLoginWithDevice()unlockVaultWithKeyConnectorKey()unlockVaultWithNeverlockKey()
Why this matters:
If a user's key is rotated and they unlock with device key/biometrics/etc., the PIN should still be cleared. Without tests, we can't verify this works correctly for all unlock paths.
Recommendation:
Add at least one test for a non-password unlock method (e.g., test_unlockVaultWithDeviceKey_enrollPinWithEncryptedPinError) to verify the PIN clearing works universally.
|
Great job! No new security vulnerabilities introduced in this pull request |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #2241 +/- ##
==========================================
+ Coverage 85.57% 85.58% +0.01%
==========================================
Files 1748 1748
Lines 148565 148672 +107
==========================================
+ Hits 127137 127244 +107
Misses 21428 21428 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
| do { | ||
| return try await clientService.crypto().enrollPinWithEncryptedPin(encryptedPin: encryptedPin) | ||
| } catch { | ||
| await flightRecorder.log("[Auth] enrollPinWithEncryptedPin failed: \(error), clearing existing PIN keys") |
There was a problem hiding this comment.
🤔 Should we also log the error to the errorReporter for the case it's not a key rotation scenario and something else is making this throw an error? Is the SDK not going to throw a specific error to avoid some sort of attack like enumeration attacks or could they provide the specific error? That would be useful to know when to log it in the error reporter and when just to use this specific logic.
There was a problem hiding this comment.
If we log any error here to the errorReporter, will we be able to notice or catch if the error starts changing? My concern would be that unexpected errors would get lost among the expected errors. Unless maybe the volume of errors changes, but we'd still be reporting errors that aren't true errors (at least in the sense of errors that need to be resolved).
The SDK throws the following error if enrollPinWithEncryptedPin fails:
(lldb) po error
▿ BitwardenError
▿ MobileCrypto : CryptoClientError
▿ Crypto : 1 element
- message : "The cipher\'s MAC doesn\'t match the expected value"
I didn't feel like this was specific enough to key off of. Do you think we can be confident enough to know that this error isn't going to change in the SDK? We could look for either this error type or this error + the error message if you think that's better.
There was a problem hiding this comment.
Talked with KM team, and for now it's ok to handle it like this as they will introduce changes for the crypto crate regarding error handling.
2 participants
🎟️ Tracking
PM-28263
📔 Objective
If the user's key is rotated they will be logged out of the device. Upon logging back in, if the user has a previous PIN configured, that PIN should be cleared.
We can detect that a PIN is invalid if
enrollPinWithEncryptedPin(encryptedPin:)fails at login. And then the existing PIN can be cleared.⏰ Reminders before review
🦮 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