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

Crypto import for non-node envs #1307

Closed
wants to merge 5 commits into from
Closed

Crypto import for non-node envs #1307

wants to merge 5 commits into from

Conversation

mvid
Copy link
Contributor

@mvid mvid commented Oct 25, 2022

crypto import:
change import pattern of crypto, to allow import error to be caught and acted on. the current model fails in react native, not exactly sure why, but using require instead avoids the issue.

Changed the lookup for subtle. Currently, if subtle doesn't exist on the crypto library, cosmjs will attempt to reimport node crypto, even if a proper crypto was already shimmed in. The changeset will pull subtle if it exists, otherwise return undefined. getCryptoModule will only be called if crypto is not already set on window/this

authz queries:
add more authz grant queries to catch up to current authz types

…nd acted on. add more authz grant queries to catch up to current authz module
@mvid mvid changed the title change import pattern of crypto, to allow import error to be caught a… Crypto import for non-node envs, and added authz queries Oct 25, 2022
@webmaster128
Copy link
Member

Thank you. This looks both promising at first glance.

Could you split the additional queries and the crypto changes in 2 PRs please? This way we can keep the discussion specific and not block one change on the other change.

@mvid
Copy link
Contributor Author

mvid commented Oct 25, 2022

Thank you. This looks both promising at first glance.

Could you split the additional queries and the crypto changes in 2 PRs please? This way we can keep the discussion specific and not block one change on the other change.

Done, the authz queries PR is now in #1308

@mvid mvid changed the title Crypto import for non-node envs, and added authz queries Crypto import for non-node envs Oct 26, 2022
@@ -11,9 +11,9 @@ import { sha512 as nobleSha512 } from "@noble/hashes/sha512";
*/
export async function getCryptoModule(): Promise<any | undefined> {
try {
const crypto = await import("crypto");
const crypto = await require("crypto");
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you try this instead?

    // HACK: Use a variable to get webpack to ignore this and cause a
    // runtime error instead of build system error or fallback implementation.
    const nodeCryptoPackageName = "crypto";
    const crypto = await import(nodeCryptoPackageName);

We use this in a different place already. This should avoid build systems trying to bundle a crypto implementation when it is not available natively.

Copy link
Contributor Author

@mvid mvid Nov 26, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@webmaster128 I introduced this, however the problem is the same. It attempts to resolve crypto when bundling. In my current project, using Expo/React-Native/Metro, it seems to parse/resolve any import call, hence why I switched it to a require. I believe this is specific to the Metro bundler.

Any ideas on how to solve this?

@webmaster128
Copy link
Member

The change LGTM. Good point regarding the subtle import.

I'd just like to get rid of the Node.js specific require call.

@webmaster128 webmaster128 added this to the 0.29.x milestone Nov 24, 2022
@webmaster128
Copy link
Member

Sorry for letting you wait so long. I'm now trying to catch up with CosmJS.

It seems like metro allows us to use an "empty" crypto module. See

I never worked with metro though. Do you have a test project where I can reproduce the issues you are facing?

@webmaster128
Copy link
Member

webmaster128 commented Dec 7, 2022

Changed the lookup for subtle. Currently, if subtle doesn't exist on the crypto library, cosmjs will attempt to reimport node crypto, even if a proper crypto was already shimmed in. The changeset will pull subtle if it exists, otherwise return undefined. getCryptoModule will only be called if crypto is not already set on window/this

I think the motivation for the current codebase was this: subtle is either

  1. in globalThis.crypto.subtle (https://www.w3.org/TR/WebCryptoAPI/) which works in browsers and node.js 18+, or
  2. in (await import("crypto")).webcrypto.subtle (node.js 15+ versions)

But turns out there is a 3rd option in between:

  • in globalThis.crypto.webcrypto.subtle (node.js 16+)

I'll update the code to better reflect that.

@webmaster128
Copy link
Member

I looked even more into this and have two changes in the pipeline:

  1. getSubtle does not need to call getCryptoModule at all since I am not aware of any environment where subtle is only available in the crypto module
  2. getCryptoModule can be removed entirely if we are willing to use a pure-JS pbkdf2 implementation for Node.js 14. Given that Node.js 14 EOLs in 4 months and 3 weeks (30 Apr 2023), I think this is fine for CosmJS 0.30.0

@webmaster128
Copy link
Member

Thank you for bringing up this problem and proposing a solution. After looking into it I gained cnfidence that #1340 (for 0.29.x) and #1341 (for 0.30.0) are the two necessary steps to fix this nicely. Let's continue the discussion in those more specific tickets.

@webmaster128
Copy link
Member

okay, this is far from solved. See #1354 for a follow-up ticket.

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.

2 participants