[PS-2251] Implement argon2 kdf #4468

quexten · 2023-01-13T17:05:12Z

Type of change

- [ ] Bug fix
- [x] New feature development
- [ ] Tech debt (refactoring, code cleanup, dependency upgrades, etc)
- [ ] Build/deploy pipeline (DevOps)
- [ ] Other

Objective

This pull request implements argon2id support via wasm (and native bindings on node js).
Some points of consideration: I tested on desktop (linux), web (chrome, firefox), manifest-v2 browser (chrome), manifest-v3 browser (chrome), cli (linux). All other platforms are untested by me, any help testing is appreciated, especially on safari and edge.

For now only memory is configurable, but in a future pull request me might introduce a kdfOptions object, to expose more configuration options (iterations, parallelism) to the user.

Code changes

manifestv3.json: csp should be "extension pages", and add wasm-unsafe-eval so we can load the wasm. (Is there a better way here? I tried configuring the CSP via the hash of the WASM file but wasn't able to get it to work. On the other hand manifest.json alread has "unsafe-eval" set which is way more loose (a superset of wasm-unsafe-eval), so it might not be an issue?
webpack.config.js: Add rules required for wasm, and fallbacks for fs, path required by the argon2 module
webpack.main.js: Add copy step for the wasm module for the desktop app
webpack.render.js: Add wasm loader configurations
package.json: Add argon2 browser module
change-kdf.component.html: Add the UI for argon2 configuration
change-kdf-component.ts: Add argon2 configuration code
messages.json: Add internationalized texts for agno2 configuration UI
cryptoFunction.service.ts: Add argon2 interface
kdfType.ts: Add argon2 as type with id 2 (id 1 is taken by scrypt in another pull request)
crypto.service.ts: Add the case for argon2, iterations and parallelization are fixed for now
webCryptoFunction.service.ts, node-crypto-function.service.ts: Implement argon2 calls, using WASM in the former service and bindings in the latter service. We could also use WASM in node-crypto-function.service.ts but this would likely be slower than the native bindings.

Screenshots

bitwarden-bot · 2023-01-13T17:06:50Z

Thank you for your contribution! We've added this to our internal Community PR board for review.
ID: PS-2251

quexten · 2023-01-15T14:36:42Z

Currently the pull request only stores memory (in the kdfIterations value).
More clean would be to directly store the memory in a dedicated field, and use the kdfIterations for argon2's iterations. Here is a pull request enabling that functionality on the back-end side. If it is accepted I will update this pull-request to take advantage of the new API..

bitwarden/server#2583

quexten · 2023-01-18T04:28:30Z

Currently the pull request only stores memory (in the kdfIterations value). More clean would be to directly store the memory in a dedicated field, and use the kdfIterations for argon2's iterations. Here is a pull request enabling that functionality on the back-end side. If it is accepted I will update this pull-request to take advantage of the new API..

bitwarden/server#2583

The server implementation still needs some work, but in the meantime the client has been updated to support more options.
It required replacing a lot of references to kdfIterations by a "KdfConfig" object. The kdfMemory and kdfParallelism fields are always optional.

Screenshot of the the configuration:

quexten · 2023-01-21T07:15:27Z

Since in the mobile pull request we are doing the kdf support first with fixed memory and parallelism, should I split this pull request into 2 requests, one doing the kdf support (same as on mobile), and one adding the kdf configuration (since that relies on server changes anyways)?

kspearrin · 2023-01-23T15:50:38Z

Since in the mobile pull request we are doing the kdf support first with fixed memory and parallelism, should I split this pull request into 2 requests, one doing the kdf support (same as on mobile), and one adding the kdf configuration (since that relies on server changes anyways)?

Yes, let's do that. I think we will probably want to deploy the client support ahead of the configuration anyways so that people can get client updates propagated out ahead of being able to enable it.

quexten · 2023-01-23T17:27:06Z

Since in the mobile pull request we are doing the kdf support first with fixed memory and parallelism, should I split this pull request into 2 requests, one doing the kdf support (same as on mobile), and one adding the kdf configuration (since that relies on server changes anyways)?

Yes, let's do that. I think we will probably want to deploy the client support ahead of the configuration anyways so that people can get client updates propagated out ahead of being able to enable it.

Okay, pulled out all the kdfConfiguration related changes and rebased onto latest master.

kspearrin · 2023-01-16T19:00:39Z

apps/web/src/app/settings/change-kdf.component.html

-          [(ngModel)]="kdfIterations"
-          required
-        />
+        <ng-container *ngIf="kdf == 0">


I think we need to expose the structured enum to the UI instead of using magic ints.

Fixed in e3a5808

kspearrin · 2023-01-16T19:00:57Z

apps/web/src/app/settings/change-kdf.component.html

+          />
+        </ng-container>
+        <ng-container *ngIf="kdf == 2">
+          <label for="kdfIterations">Memory (in KiB)</label>


Need i18n support

kspearrin · 2023-01-16T19:01:38Z

apps/web/src/app/settings/change-kdf.component.html

+            <p>
+              {{ "argon2MemoryDesc" | i18n: (recommendedMemory | number) }}
+            </p>
+            <bit-callout type="warning"> {{ "argon2Warning" | i18n }}} </bit-callout>


Not sure we need this warning. What browsers do not support WASM?

Yeah, fair. Only IE really, but I'm not even sure bitwarden supports IE anyways.

Removed in 2a937d0

Unsure if there are other configurations out there like this (i.e., first-party supported by a major OS/browser vendor), but

enabling Lockdown Mode on iOS disables WASM (among other browser features) on Safari and any WebView implementation in all apps (including other browser apps).

Attempting to access the web vault via Safari or any other browser is affected.

I don't believe the Bitwarden native iOS app is affected (AFAIK the only WebView is for webauthn)

Lockdown Mode is also available on macOS, but AFAIK only Safari is affected.

The Bitwarden browser extension within Safari is affected, and I have not found a way to whitelist it. But the extension is mostly functional. (The UI svg/webfont icons for "copy username", "copy password", etc, do not render and are simply shown as squares. Initial login is slow too, but reasonable as Lockdown also disables JIT. There's support for whitelisting domains from Lockdown Mode, but the safari-web-extension://${UUID} URIs can't seem to be whitelisted.)

kspearrin · 2023-01-23T17:52:56Z

apps/web/src/app/settings/change-kdf.component.html

@@ -32,41 +32,80 @@ <h1>{{ "encKeySettings" | i18n }}</h1>
        >
          <i class="bwi bwi-question-circle" aria-hidden="true"></i>
        </a>
-        <select id="kdf" name="Kdf" [(ngModel)]="kdf" class="form-control" required>
+        <select


Do we still need to remove this to a config PR?

kspearrin · 2023-01-23T17:53:53Z

@Hinton Can you look into the wasm implementation. Is this how we want to do it?

quexten · 2023-01-23T17:54:13Z

Had a wrong commit from another branch on my local master, so had to force push one more time.

quexten · 2023-01-23T17:57:06Z

@Hinton Can you look into the wasm implementation. Is this how we want to do it?

By the way about the wasm implementation: The argon2-browser project states that on node.js, native bindings should be used(https://www.npmjs.com/package/argon2), which I did. WASM would also work in node.js though. We could save an implementation/dependency by only using the WASM implmentation if we don't need the extra speed since as long as it is acceptable in WASM in the browser, it should be acceptable for the CLI too speed wise, right?

Hinton · 2023-01-23T19:24:33Z

@kspearrin @quexten there is webpack flag, asyncWebAssembly https://webpack.js.org/configuration/experiments/ which should let you remove all the webpack changes. (At least that's how I got webassembly working in the web vault a while back). Would be nice figuring out why we need the csp change as well.

quexten · 2023-01-23T20:36:14Z

@kspearrin @quexten there is webpack flag, asyncWebAssembly https://webpack.js.org/configuration/experiments/ which should let you remove all the webpack changes. (At least that's how I got webassembly working in the web vault a while back). Would be nice figuring out why we need the csp change as well.

I did see this flag during development, but didn't use it as it was experimental. But it seems it is enabled under future defaults? I'll switch to it at any rate.

quexten · 2023-01-23T21:48:36Z

@kspearrin @quexten there is webpack flag, asyncWebAssembly https://webpack.js.org/configuration/experiments/ which should let you remove all the webpack changes. (At least that's how I got webassembly working in the web vault a while back). Would be nice figuring out why we need the csp change as well.

Hmm, so I tried the flag and I was able to remove the line:
noParse: /\.wasm$/,

However, still required is:

  {
    test: /\.wasm$/,
    loader: "base64-loader",
    type: "javascript/auto",
  },

because of:

ERROR in ../../node_modules/argon2-browser/dist/argon2.wasm 
Module not found: Error: Can't resolve 'a' in '/home/quexten/projects/clients/node_modules/argon2-browser/dist'

(not exactly sure yet what the exact cause is)

About the CSP changes:
removing wasm-eval should be fine as wasm-unsafe-eval is preferred these days, but it seems (according to a 1 year old comment, so might have changed) chrome still requires this in extensions?

Ideally we could use SRI hashes, but for WASM, these are unsupported in chromium.

https://developer.mozilla.org/en-US/docs/Mozilla/Add-ons/WebExtensions/Content_Security_Policy#WebAssembly

https://bugs.chromium.org/p/chromium/issues/detail?id=945121

kspearrin · 2023-01-24T02:06:37Z

libs/node/package.json

@@ -18,6 +18,7 @@
    "build:watch": "npm run clean && tsc -watch"
  },
  "dependencies": {
-    "@bitwarden/common": "file:../common"
+    "@bitwarden/common": "file:../common",
+    "argon2": "^0.30.3"


@Hinton Is this where we want to include the node package?

No, packages should only be in the root package.json. All of these should be moved over.

For native packages in the CLI, they also need to be added to the package.json under CLI. And native node modules for desktop needs to be added to src/package.json under desktop to ensure they get cross compiled.

Anything we need to consider for packaging for the different app stores? I only tested running in dev mode.

I moved these to root.

Hinton · 2023-01-24T11:29:32Z

I toyed around with the wasm stuff on this branch. The Argon2 wasm dependency seems to work quite different from the rust one I built myself. It seems we do need the webpack changes, and the experimentals.asyncWebAssembly doesn't do anything for us.

I also read up on wasm, seems they don't support sha signatures which is a pitty. I guess our general sha CSP rules protectes against arbitrary wasm being loaded though. Do we have any warnings in case the wasm execution fails due to wasm-eval or wasm-unsafe-eval isn't supported? We would want to block the user from logging in + changing the KDF with a nice message vs silently logging errors in the console.

libs/common/package.json

kspearrin · 2023-01-24T13:54:19Z

@quexten I finished updating the server PR with the necessary changes. I think @justindbaur is adding some tests to it.

Did you start a branch somewhere else with the separate config changes yet?

quexten · 2023-01-24T15:59:16Z

Did you start a branch somewhere else with the separate config changes yet?

Not yet, I have the old branch, which I need to re-base onto this. I'll do that now.

quexten · 2023-01-24T17:13:04Z

@quexten I finished updating the server PR with the necessary changes. I think @justindbaur is adding some tests to it.

Did you start a branch somewhere else with the separate config changes yet?

quexten#1

I targeted it to this pull request's source branch for now. Let me know whether I should merge it into this PR's source or submit it separately against bitwarden's master branch.

kspearrin · 2023-01-24T17:17:09Z

@quexten Let's finish cleaning up this branch, merge it, and then we can target master and open a PR for your new config branch.

kspearrin · 2023-01-24T21:46:46Z

@quexten I did some work cleaning things up here. I think it is good to go after we remove the changes to web angular components and locale files.

micahblut · 2023-01-24T21:49:54Z

@quexten we will create a help document for users comparing the two algorithms. Can you update the kdf algorithms tooltip link to point to https://bitwarden.com/help/kdf-algorithms

kspearrin · 2023-01-24T21:51:12Z

@micahblut We can do that in the configuration branch.

danielleflinn · 2023-01-24T23:55:46Z

Feedback from Bitwarden Design:

Update the form so that there is not a large gap on the left side when Argon2id is selected
Use sentence case on field labels
Helper text: "Higher Argon 2 iterations, memory, and parallelism..." (update order to match field order and use the oxford comma)
In Callout, update sentence to "...WebAssembly, you cannot change your KDF algorithm to Argon2." Remove the “}” that is appearing at the end of the callout’s text in the previously posted screenshot
If after submit we know the submission failed from lack of WebAssembly support, show an error toast notification with the following: "Browser does not support WebAssembly. Try again with a different browser"
Update the PBKDF2 encryption text based warning to use the warning callout. This provider better UI consistency between the 2 encryption options.

apps/browser/webpack.config.js

apps/web/webpack.config.js

kspearrin · 2023-01-25T12:53:44Z

I reverted the config pages in web vault. Good to go for me. @Hinton for final approval.

libs/common/src/enums/kdfType.ts

Co-authored-by: Martin Weinelt <[email protected]>

Hinton

@kspearrin maybe I'm missing something, but there doesn't seem to be any way to configure the argon2 options?

We should also track and show a reasonable error if we can't load the argon2 wasm and block the login on those devices / changing kdf.

Other than that I think we're good. Do we want a feature flag for this until everything is wrapped up?

quexten · 2023-01-26T10:06:45Z

@kspearrin maybe I'm missing something, but there doesn't seem to be any way to configure the argon2 options?

We should also track and show a reasonable error if we can't load the argon2 wasm and block the login on those devices / changing kdf.

Other than that I think we're good. Do we want a feature flag for this until everything is wrapped up?

Config options got rolled back in 9537c5c.

So at the moment, the clients can log in if the account is configured for argon2 but there is no way to switch to argon2, or configure argon2's parameters.

Separate pull request for configuration that will be re-targeted to master once this is merged is here:
quexten#1

kspearrin · 2023-01-26T12:59:02Z

@Hinton I don't think we need a feature flag since everything is planned to be in this release.

Config options will follow after this is merged.

kspearrin · 2023-01-26T14:20:34Z

@quexten Please open your config PR when ready.

quexten · 2023-01-26T14:44:12Z

@kspearrin rebased onto latest master and opened #4578
I have not yet applied changes suggested by the Bitwarden design team though.

kspearrin · 2023-01-26T14:54:20Z

@quexten I can help with that. Will move to #4578

bitwarden-bot changed the title ~~Implement argon2 kdf~~ [PS-2251] Implement argon2 kdf Jan 13, 2023

bitwarden-bot added the community-pr label Jan 13, 2023

quexten force-pushed the feature/argon2-kdf branch 2 times, most recently from e9801ba to 9bae3d0 Compare January 23, 2023 17:26

quexten force-pushed the feature/argon2-kdf branch from 9bae3d0 to fcd9383 Compare January 23, 2023 17:38

Implement argon2

e05752c

kspearrin reviewed Jan 23, 2023

View reviewed changes

quexten force-pushed the feature/argon2-kdf branch from fcd9383 to e05752c Compare January 23, 2023 17:53

quexten added 2 commits January 23, 2023 19:05

Remove argon2 webassembly warning

2a937d0

Replace magic numbers by enum

e3a5808

kspearrin reviewed Jan 24, 2023

View reviewed changes

Hinton reviewed Jan 24, 2023

View reviewed changes

libs/common/package.json Outdated Show resolved Hide resolved

quexten mentioned this pull request Jan 24, 2023

Implement kdf configuration quexten/clients#1

Closed

Merge branch 'master' into feature/argon2-kdf

5395062

kspearrin reviewed Jan 25, 2023

View reviewed changes

apps/browser/webpack.config.js Show resolved Hide resolved

apps/web/webpack.config.js Outdated Show resolved Hide resolved

apps/web/webpack.config.js Show resolved Hide resolved

kspearrin added 2 commits January 25, 2023 07:42

don't need wasm-eval

96dd369

revert config changes

9537c5c

kspearrin requested a review from Hinton January 25, 2023 12:53

kspearrin previously approved these changes Jan 25, 2023

View reviewed changes

djsmith85 assigned kspearrin Jan 25, 2023

mweinelt reviewed Jan 25, 2023

View reviewed changes

libs/common/src/enums/kdfType.ts Outdated Show resolved Hide resolved

Update libs/common/src/enums/kdfType.ts

2f55f93

Co-authored-by: Martin Weinelt <[email protected]>

kspearrin dismissed their stale review via 2f55f93 January 26, 2023 00:56

Update kdfType.ts

edea0f4

Hinton reviewed Jan 26, 2023

View reviewed changes

Merge branch 'master' into feature/argon2-kdf

dbca1e5

kspearrin added 2 commits January 26, 2023 08:35

apply DEFAULT_PBKDF2_ITERATIONS

b2a22c2

checkIfWasmSupported

e3d92ea

kspearrin approved these changes Jan 26, 2023

View reviewed changes

Hinton approved these changes Jan 26, 2023

View reviewed changes

kspearrin merged commit e055e68 into bitwarden:master Jan 26, 2023

quexten mentioned this pull request Jan 26, 2023

[PS-2365] Kdf Configuration Options for Argon2 #4578

Merged

MGibson1 mentioned this pull request Jan 31, 2023

[CSA-28] Use path normalization in API requests #4580

Merged

[PS-2251] Implement argon2 kdf #4468

[PS-2251] Implement argon2 kdf #4468

Uh oh!

Conversation

quexten commented Jan 13, 2023

Type of change

Objective

Code changes

Screenshots

Uh oh!

bitwarden-bot commented Jan 13, 2023

Uh oh!

quexten commented Jan 15, 2023

Uh oh!

quexten commented Jan 18, 2023

Uh oh!

quexten commented Jan 21, 2023

Uh oh!

kspearrin commented Jan 23, 2023

Uh oh!

quexten commented Jan 23, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mtigas Jan 24, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kspearrin commented Jan 23, 2023

Uh oh!

quexten commented Jan 23, 2023

Uh oh!

quexten commented Jan 23, 2023

Uh oh!

Hinton commented Jan 23, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

quexten commented Jan 23, 2023

Uh oh!

quexten commented Jan 23, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Hinton Jan 24, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Hinton commented Jan 24, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

kspearrin commented Jan 24, 2023

Uh oh!

quexten commented Jan 24, 2023

Uh oh!

quexten commented Jan 24, 2023

Uh oh!

kspearrin commented Jan 24, 2023

Uh oh!

kspearrin commented Jan 24, 2023

Uh oh!

mtigas Jan 24, 2023 •

edited

Loading

Hinton commented Jan 23, 2023 •

edited

Loading

quexten commented Jan 23, 2023 •

edited

Loading

Hinton Jan 24, 2023 •

edited

Loading

Hinton commented Jan 24, 2023 •

edited

Loading