-
Notifications
You must be signed in to change notification settings - Fork 1.3k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[PM-16603] Implement userkey rotation v2 #12646
base: main
Are you sure you want to change the base?
Conversation
New Issues (4)Checkmarx found the following issues in this Pull Request
|
Codecov ReportAttention: Patch coverage is
✅ All tests successful. No failed tests found. Additional details and impacted files@@ Coverage Diff @@
## main #12646 +/- ##
==========================================
+ Coverage 35.29% 35.33% +0.04%
==========================================
Files 2997 3002 +5
Lines 90882 90984 +102
Branches 16969 16982 +13
==========================================
+ Hits 32080 32153 +73
- Misses 56314 56335 +21
- Partials 2488 2496 +8 ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice work! After a quick pass through, I've got a few questions / comments below:
apps/web/src/app/key-management/key-rotation/user-key-rotation.service.ts
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
@@ -31,7 +31,7 @@ export enum FeatureFlag { | |||
AccountDeprovisioning = "pm-10308-account-deprovisioning", | |||
SSHKeyVaultItem = "ssh-key-vault-item", | |||
SSHAgent = "ssh-agent", | |||
UserKeyRotation = "userkey-rotation-refactor", | |||
UserKeyRotationV2 = "userkey-rotation-refactor", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should we rename the feature flag to userkey-rotation-v2
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, makes sense, this is the flag we had in launchdarkly, but i'll get that changed, because it does not match the actual change that is not a refactor.
c5ed61f
294fa4e
to
d4caf6f
Compare
Cleaned up the userkey rotation v2 api in the server PR and subsequently adjusted this PR to match. Added some more tests aswell. |
🎟️ Tracking
https://bitwarden.atlassian.net/browse/PM-16603
Server PR: bitwarden/server#5204
📔 Objective
Userkey rotation previously consisted of two steps: Update masterpassword first, then update the userkey. Between these two, the clients relied on the local state to track the updated masterkey, but also to track kdf settings. If any of these were wrong, the userkey rotation request could corrupt vault data.
This combines both into one request, such that the kdf parameters are also always sent together with the userkey rotation request so that the corruption cannot occur. Further, this makes it so that cancelling during key rotation, does not force logout the user (where previously it had to, since the masterpassword was changed, and thus the security stamp was changed).
The old key rotation is still kept around for migration of legacy users.
📸 Screenshots
Screen.Recording.2025-01-03.at.13.51.35.mov
⏰ 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