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

Consider converting hash, sign and verify to async functions #681

Open
3 of 4 tasks
orbitlens opened this issue Aug 15, 2023 · 3 comments
Open
3 of 4 tasks

Consider converting hash, sign and verify to async functions #681

orbitlens opened this issue Aug 15, 2023 · 3 comments

Comments

@orbitlens
Copy link
Contributor

Currently, stellar-base is using synchronous cryptography calls for transactions hashing, signing, and verification.

This is sub-optimal in terms of general JavaScript-based applications development because SHA256 and ED25519 CPU-intensive computations block the main thread, an obvious antipattern for JS runtimes, which are single-threaded by design. In case of server applications this can also potentially provide an exploitable surface for DoS attacks for NodeJS or resource exhaustion for cloud edge-computing runtimes like Cloudflare Workers and Vercel Functions.

Besides that, packages required for the cryptography, significantly increase size of the client-side bundle.

It makes sense to change API of all hashing/signing functions and methods exposed by stellar-base to return Promise, and therefore, make all these functions async.

Motivation:

  • Such CPU-intensive calls won't block the main thread.
  • This will allow us to drop sha.js and tweetnacl dependencies, replacing them with native built-in calls (NodeJS Crypto API and browser-based Web Crypto API). At this moment,
    • SHA256 is fully supported on all major platforms via Web Crypto API.
    • ED25519 is fully supported in NodeJS from v12.
    • ED25519 addition to the Web Crypto API is on the final standardization phase, and has been already implemented by all major platforms except Gecko. So at the moment it will require a polyfill for browsers. However, the polyfill will be automatically excluded from the bundle builds once ED25519 support is rolled in browsers.
    • NodeJS has full compatibility and support of both SHA256 and ED25519 via Web Crypto API since version v16.17.0 (LTS). So if we target only LTS Node versions, it's possible to utilize a unified interface for native API calls across NodeJS and browsers.
  • Since this is a breaking change, which will require some work from maintainers to update client-side code, it makes sense to align this change with other breaking changes caused by Soroban-related updates. Otherwise, we'll have to wait for the next opportunity because this breaking change by itself is not that significant to force ecosystem developers to do refactor on their side.

If this looks good, I'll prepare a pull-request.

@Shaptic
Copy link
Contributor

Shaptic commented Aug 15, 2023

Hmm, my main concern around touching crypto period is always around security. I don't see how/why we would be able to trust the ed25519 polyfill. Hashing seems fine, for sure, but I'm less confident mucking with the signing algorithms 🤔 Any thoughts in that regard? I do think we can move the code to be async, though, since that's a separate discussion from replacing the dependencies.

@orbitlens
Copy link
Contributor Author

Hmm, my main concern around touching crypto period is always around security. I don't see how/why we would be able to trust the ed25519 polyfill. Hashing seems fine, for sure, but I'm less confident mucking with the signing algorithms 🤔 Any thoughts in that regard?

But we can use the same good old tweetnacl as polyfill. I do not propose to replace tweetnacl with some noname library. On the contrary, a few lines of code will create a thin wrapper on top of it to match the Web Crypto API standard interface. And later on, once ED25519 makes it to all major browsers (which I hope to see by the end of the year), we'll just convert this thin wrapper to the dynamically loaded polyfill. This way, in the future, ecosystem developers will not have to deal with any breaking changes caused by migration from crypto libs to the native Web Crypto API.

@kalepail
Copy link
Contributor

I'm in favor of this change. Ideally we'd allow users to polyfill their own crypto where/as/if needed. Not sure how feasible that is in the near term but I am in favor of having sign be async

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

No branches or pull requests

3 participants