-
Notifications
You must be signed in to change notification settings - Fork 4.7k
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
feat: reduce user storage encryption iteration count #24523
feat: reduce user storage encryption iteration count #24523
Conversation
Balance between security and UX/speed. We perform many Encryption/Decryption transactions for notifications and speed really matters! The content we are saving in user storage does not need to be so secure that it compromises UX.
CLA Signature Action: All authors have signed the CLA. You may need to manually re-run the blocking PR check if it doesn't pass in a few minutes. |
Related thread from previous PR: #23353 (comment)
@Prithpal-Sooriya How was the exact number 32767 determined here? Noting that:
So any new production value will effectively "wipe" all past storage using the old value, so we should expect it to be sticky. Migration will be needed if this is to be seamless for users. So if reducing, we should still go as high as we can tolerate, which is the general recommendation. There was also some conversation in the linked previous PR about considering a different KDF - is this still on the table?
You may think that about the current usage but does the assumption hold for all users, and future use of the same derived key? |
Yeah this PR was mostly a hotfix from the notification team due to high iteration count compromising UX speed (we encrypt and decrypt a few times in a row).
Yeah this is magic number-y, its 2**15 - 1 - a number we are using on Portfolio. TBH we can definitely go higher, just that 900_000 was tremendously slow.
We store this iteration count with the payload, so when decrypting we can use iteration count from here. This allows us to increase the iteration count if we need to.
Yep 100%, I did a little playing around and got Please correct me if wrong, but these also have "levers"/fields which we can up the work factor, but still need to balance UX 😄 Update: Here is a draft/temp PR to demo using scrypt instead of pbkdf2 |
Closing this since this is not a solution we will be implementing. |
Description
Balance between security and UX/speed.
We perform many Encryption/Decryption transactions for notifications and speed really matters! The content we are saving in user storage does not need to be so secure that it compromises UX.
Related issues
Fixes: https://consensyssoftware.atlassian.net/browse/NOTIFY-616
Manual testing steps
N/A, this is testable once the entire Notifications Feature has been merged.
Screenshots/Recordings
Before
After
Pre-merge author checklist
Pre-merge reviewer checklist