Skip to content
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

Generate VUK with crypto.subtle #224

Conversation

leordev
Copy link
Contributor

@leordev leordev commented Sep 24, 2023

Problem

I'm experiencing a slow initialization of the Web5.connect() fn, to the point where the browser freezes for ~4-5 seconds.

It can be reproduced here: https://bucolic-cendol-324631.netlify.app/ -- just click in sign in and then press Use Browser In-App Agent. You will notice that the loading spinning animation even freezes while the connect is running. After being logged in you can also just refresh the page and you will notice how slow it takes to load the page, this is because if I recognize a session was initiated I just call Web5.connect() before rendering the page.

Fix

I was debugging the connect() fn stepping each line and I narrowed it down to the AppDataVault#generateVaultUnlockKey().

From the conversation (link below) it seems that @noble/hashes/pbkdf2 does not use the native browser crypto subtle which runs way faster. So I just did that.

I tested in my web5-music app and it reduces from 5 seconds to less than 2 seconds and it's non blocking now (the browser does not freeze).

Reference

The whole context about this issue can be found in our Discord #web5 in this conversation

@leordev leordev marked this pull request as draft September 24, 2023 12:23
@codecov
Copy link

codecov bot commented Sep 24, 2023

Codecov Report

Merging #224 (f400157) into main (f3c7184) will decrease coverage by 0.01%.
The diff coverage is 92.47%.

@@            Coverage Diff             @@
##             main     #224      +/-   ##
==========================================
- Coverage   90.99%   90.99%   -0.01%     
==========================================
  Files          67       68       +1     
  Lines       12645    12711      +66     
  Branches     1260     1268       +8     
==========================================
+ Hits        11506    11566      +60     
- Misses       1116     1122       +6     
  Partials       23       23              
Components Coverage Δ
api 94.32% <ø> (ø)
common 95.00% <ø> (ø)
credentials 92.77% <ø> (ø)
crypto 100.00% <ø> (ø)
dids 92.16% <ø> (ø)
agent 88.20% <92.47%> (+0.03%) ⬆️
identity-agent 59.05% <ø> (ø)
proxy-agent 58.59% <ø> (ø)
user-agent 57.36% <ø> (ø)

@shamilovtim
Copy link
Contributor

shamilovtim commented Oct 2, 2023

Hey @leordev I agree with making as much of our SDK use native functions as possible!

One thing, many of our crypto libs first check for node crypto, and then if node crypto isn't available, they fallback to web crypto (crypto.subtle). This approach is beneficial for runtimes like React Native or node versions less than node 16 which don't currently have great support for crypto.subtle. You can look through the insides of our crypto dependencies for how they handle this and follow their lead for how they handle using node crypto first and then falling back to subtle crypto.

@leordev
Copy link
Contributor Author

leordev commented Oct 3, 2023

Adding our discord thread for more context on the PR here.

Great suggestion. I found a few variations of how subtle is instantiated, and this was the closest one to our case:

browserify/pbkdf2 lib - which hints this in their readme:

For high performance, use the async variant (pbkdf2.pbkdf2), not pbkdf2.pbkdf2Sync, this variant has the oppurtunity to use window.crypto.subtle when browserified.

Check it out: https://github.com/browserify/pbkdf2/blob/master/lib/async.js#L113-L114

Which is exactly what we are trying to do here but falling back to noble, which is audited... I will add this change in my pr!

--

Oh, I just noticed you suggested using node:crypto > webcrypto > 3rd party. It seems like multiformats (dependency from dwn-sdk-js) is doing that. Although IIUC, during bundling, exporting sha2.js or sha2-browser.js... Let me what it would look like for us.

@leordev
Copy link
Contributor Author

leordev commented Oct 5, 2023

So as we aligned in our sync I'm checking for subtle crypto first and then falling back to noble here:

https://github.com/TBD54566975/web5-js/blob/1b641f99f624c9d9713ae61f3a5f1886929c3bf8/packages/agent/src/app-data-store.ts#L147-L151

I was going through the path of dynamically importing node:crypto and using pbkdf2() but that seemed unnecessary just because of the polyfill that we require for node <=18:

// node.js 18 and earlier,  needs globalThis.crypto polyfill
import { webcrypto } from "node:crypto";
// @ts-ignore
if (!globalThis.crypto) globalThis.crypto = webcrypto;

We will always have the subtle in node because of this polyfill...

@leordev leordev marked this pull request as ready for review October 5, 2023 21:20
@shamilovtim
Copy link
Contributor

I think we still want node crypto as an option (non subtle). Could it be included here?

@shamilovtim
Copy link
Contributor

What's the benefit of falling back to noble after node crypto and web crypto? Are there runtimes with no native crypto library at all?

@leordev
Copy link
Contributor Author

leordev commented Oct 10, 2023

I've added the fallback to node:crypto and removed the noble one. If it makes sense please let me know and I will work on the unit tests!

@leordev leordev force-pushed the leordev/vuk-with-cryptosubtle branch from 13bbe03 to f400157 Compare October 11, 2023 17:04
@frankhinek
Copy link
Contributor

Implemented as part of @web5/crypto in PR #262

@frankhinek frankhinek closed this Nov 7, 2023
@leordev leordev deleted the leordev/vuk-with-cryptosubtle branch February 15, 2024 21:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants