-
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-1320] Argon2 SIMD support #4921
Conversation
Thank you for your contribution! We've added this to our internal Community PR board for review. |
Discussed elsewhere as well, but the upstream solution would be preferable. |
As also discussed in the discussion, the upstream argon2-browser library seems currently unmaintained for the foreseeable future. In case that does not change, would it be better to apply the auto-loading of the SIMD WebAssembly change as a patch instead of switching to my fork? That way the original package is still used, instead of my fork, the changes are applied, and can be easily undone once / if upstream is merged. (https://www.npmjs.com/package/patch-package, a solution as recommended f.e here: https://stackoverflow.com/a/62567504). |
I've never seen patch-package before, but I like it a lot. It seems like a reasonable way to solve our issue without implicitly getting us in the business of maintaining an npm package. |
@MGibson1 we used patches to handle some issues with electron-builder a long time ago. bitwarden/desktop#803 Main annoyance is updating the patch when the package is updated, which doesn't seem likely in this case. |
d908071
to
39ec767
Compare
Finally got to revisit this PR. I have re-based to latest master and changed it to just use a small patch to add automatic detection and loading of the SIMD binary instead of the forked package. This also means that the Webassembly binaries are still the same ones from argon2-browser which are somewhat slower than ones newly compiled using the latest Emscripten version. But there is not much that can be done about this aside from getting a new release compiled upstream (or adding a post-install script that copies the binaries). I also tested the argon2-browser's SIMD implementation on Safari 16.4. It seems to run, but is not actually faster than the regular implementation though this was on arm (A12Z Bionic). Finally, since #5072 also applies a patch to argon2-browser, I will update it (or this PR depending on which is merged first) to apply both changes. |
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.
This looks ready to go to me, but I do have a (hopefully) quick question about your min simd module check.
I'll get QA started testing in any case, though.
+ try { | ||
+ if (typeof WebAssembly === "object" && typeof WebAssembly.instantiate === "function") { | ||
+ const module = new WebAssembly.Module( | ||
+ Uint8Array.of(0x00, 0x61, 0x73, 0x6d, 0x01, 0x00, 0x00, 0x00, 0x01, 0x05, 0x01, 0x60, 0x00, 0x01, 0x7b, 0x03, 0x02, 0x01, 0x00, 0x0a, 0x0a, 0x01, 0x08, 0x00, 0x41, 0x00, 0xfd, 0x0f, 0xfd, 0x62, 0x0b) |
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.
Going through wat2wasm, I end up with
(0x00, 0x61, 0x73, 0x6d, 0x01, 0x00, 0x00, 0x00, 0x01, 0x05, 0x01, 0x60, 0x00, 0x01, 0x7b, 0x03, 0x02, 0x01, 0x00, 0x0a, 0x0a, 0x01, 0x08, 0x00, 0x41, 0x00, 0xfd, 0x0f, 0xfd, 0x62, 0x0b, 0x00, 0x0a, 0x04, 0x6e, 0x61, 0x6d, 0x65, 0x02, 0x03, 0x01, 0x00, 0x00)
which differs from this with yours by appending 0x00, 0x0a, 0x04, 0x6e, 0x61, 0x6d, 0x65, 0x02, 0x03, 0x01, 0x00, 0x00
to the end of your hex string. Do you know what this difference is?
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.
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.
Running both your result and the one I get through wasm2wat, I end up with the same result.
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.
weird, whatever. Thanks for checking
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.
g2g, Passed QA and just merged up!
Thank you again for your continued support on the argon front, @quexten! I want to send you some swag for all your help here, is the email address you have listed on GitHub a good one to send a giftcard?
Thanks @MGibson1 for taking the time for reviewing it, getting it QA'd and merged :) No need for a giftcard, I already received one previously from Kyle, but thank you for the offer. I'll re-base the some-what related #5072 onto the current master some time later this week as it was in conflict with this branch. |
Type of change
Objective
This PR changes the argon2-browser dependency to argon2-browser-bw (a fork I created since argon2-browser does not seem to be under active development at the moment). The fork is different in that it includes wasm binaries compiled using a newer Emscripten compiler version, which leads to significantly faster binaries (exact differences can be found on this GitHub branch). Furthermore, it adds automatic detection for SIMD support, and loads the correct WASM module. This makes unlock times on high argon2 configurations a lot more bearable.
On my system the unlock times are as follows (10 iterations, 1 parallelism, 1024MiB memory)
Current implementation in Bitwarden (non-SIMD): 26 seconds
argon2-browser SIMD: 14 seconds
argon2-browser-bw (also SIMD): 10 seconds
The SIMD improvements work on all modern chromium and firefox based browsers and the Electron desktop client. Safari support should arrive in version 16.4.
Code changes