stellar / js-stellar-base

The lowest-level stellar helper library. It consists of classes to read, write, hash, and sign Stellar xdr
https://stellar.github.io/js-stellar-base/
Apache License 2.0
108 stars 137 forks source link

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

Open orbitlens opened 1 year ago

orbitlens commented 1 year ago

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:

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

Shaptic commented 1 year ago

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 :thinking: 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 commented 1 year ago

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 commented 1 year ago

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