midonet / tssrp6a

This library is a dependency free TypeScript implementation of Secure Remote Password SRP6a.
MIT License
36 stars 8 forks source link

Get rid of jsbn and crypto-js? #47

Closed jbis9051 closed 3 years ago

jbis9051 commented 3 years ago

With Node support for JavaScript's native big integer type, BigInt, could we eliminate the jsbn dependency?

Node also has a bunch of crypto functions in the crypto module. Could we eliminate the crypto-js dependency as well?

I would favor native capabilities rather than third part solutions when possible, unless of course there is something missing from those native solutions.

bgrosse-midokura commented 3 years ago

tssrp6a must be able to run in the browser, BigInt compatibility looks quite good. jsbn and BigInt might not behave in completely the same way, though. But I think the current test suite would catch any deviations.

For crypto, node's crypto isn't available on browsers, but there is https://developer.mozilla.org/en-US/docs/Web/API/Crypto which might as well work.

I would definitely favour using BigInt. For crypto, it looks a bit more complicated. Perhaps there is some ready-made module that offers the relevant crypto functions with fallbacks for node.js and browser?

jbis9051 commented 3 years ago

@bgrosse-midokura As for the crypto issue, we could utilize browserify to polyfill node's crypto library (or webpack, but due to webpack 5 drop for native node module polyfilling and the simplicity of this project browserify is probably better).

bgrosse-midokura commented 3 years ago

I would be reluctant to change the current simple tsc build. I think simply using a polyfill module (that has auto-fallback to native) would be sufficient, no?

jbis9051 commented 3 years ago

My concern with that approach is that the polyfill would be included in the build regardless of if it's needed (I think). Node's build would include the polyfill for no reason.

Based on TypeScript's docs, the build command complexity would only increase slightly.

Something like:

browserify src/index.ts -p [ tsify --noImplicitAny ] > bundle.js
bgrosse-midokura commented 3 years ago

It's either included, or there have to be two build artifacts/targets between which a user then also has to chose. This sounds like too much difficulty for some size improvement.

Could this https://github.com/PeculiarVentures/webcrypto maybe just work?

On Tue, May 11, 2021, 01:09 Josh Brown @.***> wrote:

My concern with that approach is that the polyfill would be included in the build regardless of it's needed (I think). Node's build size would include the polyfill for no reason.

Based on TypeScript's docs https://www.typescriptlang.org/docs/handbook/integrating-with-build-tools.html#browserify, the build command complexity would only increase slightly.

Something like:

browserify src/index.ts -p [ tsify --noImplicitAny ] > bundle.js

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/midonet/tssrp6a/issues/47#issuecomment-837473034, or unsubscribe https://github.com/notifications/unsubscribe-auth/ALPE2LWOD4B2FQDRMWLMB7TTNBRRPANCNFSM44SQ5W3A .

jbis9051 commented 3 years ago

It's either included, or there have to be two build artifacts/targets between which a user then also has to chose. This sounds like too much difficulty for some size improvement.

I agree. Although maybe two builds isn't a terrible idea?

Could this https://github.com/PeculiarVentures/webcrypto maybe just work?

I'd be concerned about the lack of support for many algorithms as well as the warning that it shouldn't be used in production.

How about something like https://nodejs.org/api/webcrypto.html#webcrypto_web_crypto_api? It's experimental but implements the same API available in the browser.

We could do something like

const subtle = crypto.subtle || require('crypto').webcrypto.subtle;
bgrosse-midokura commented 3 years ago

Yes, that node.js webcrypto looks much better, and is official, although experimental. Sounds like the best solution.

bgrosse-midokura commented 3 years ago

@jbis9051 I'm giving native BigInt a try

bufistov commented 3 years ago

If you guys can get rid off jsbn that would be awesome! :)

I recall to try native JS big int but they did not provide an access to bytes array at that time. I hope things improved!

bgrosse-midokura commented 3 years ago

@jbis9051 native BigInt fails with RangeError: Maximum BigInt size exceeded. I've opened #51 , you can take a look if you see a way to fix it. I think it boils down to V8.

@bufistov I think for that I just used .toString(16) in that PR.

bufistov commented 3 years ago

I see, nice! :)

jbis9051 commented 3 years ago

jsbn removed by https://github.com/midonet/tssrp6a/pull/51

jbis9051 commented 3 years ago

Looking more into webcrypto. There are less hash functions available (SHA-1, SHA-256, SHA-384 SHA-512), but that will give us an opportunity to implement #53. I'll try to work on a PR.

jbis9051 commented 3 years ago

Seems webcrypto was added in Node v15 If we want to support <15.0, we could add compatibility with the normal crypto library. Looking into that.