-
Notifications
You must be signed in to change notification settings - Fork 75
[PM-23277] Force KDF updates if below minimums #1880
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
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1880 +/- ##
==========================================
- Coverage 81.33% 75.79% -5.54%
==========================================
Files 825 1022 +197
Lines 52136 62088 +9952
==========================================
+ Hits 42403 47062 +4659
- Misses 9733 15026 +5293 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Great job! No new security vulnerabilities introduced in this pull request |
e81760c
to
15ffd67
Compare
15ffd67
to
bc8cf8b
Compare
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 actually sync at some point before checking if it needs to update KDF minimums? I was thinking in the scenario that the user has already updated KDF settings in other client but mobile has not synced yet (maybe missed the notification) and then by using the mobile app it tries to update it (again). Not sure if that's already handled by the server but raising it in case it produces any bad outcomes.
@fedemkr Good callout, I updated the logic to sync and check again that the account still needs a KDF update before proceeding. |
"DefaultX" = "Default (%1$@)"; | ||
"UpdateYourEncryptionSettings" = "Update your encryption settings"; | ||
"TheNewRecommendedEncryptionSettingsDescriptionLong" = "The new recommended encryption settings will improve your account security. Enter your master password to update now."; | ||
"Updating" = "Updating..."; |
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 be Updating…
Last time someone unify all ...
(3 dots) to …
character
🎟️ Tracking
PM-23576 - Validate local KDF settings are correct when updating server KDF
PM-23277 - Upgrade KDF settings to minimums
📔 Objective
Note
This depends on bitwarden/sdk-internal#383 and bitwarden/server#6121
This adds forced KDF updates if the PBKDF2 iterations is below the minimum (600k). KDF updates can be performed without user interaction when unlocking the vault with a master password. Otherwise, there's a check on the vault list after syncing. In this flow, the user will have to input their master password before the update can proceed.
📸 Screenshots
⏰ 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