decentralized-identity / did-jwt

Create and verify DID verifiable JWT's in Javascript
Apache License 2.0
331 stars 70 forks source link

feat: Replace @stablelib/ with noble-crypto #280

Closed ukstv closed 1 year ago

ukstv commented 1 year ago

This PR replaces @stablelib dependencies with noble-crypto wherever it makes sense, i.e. only @stablelib/xchacha20poly1305 is still used.

Code difference:

  1. Jest now treats test files as ES modules
  2. @stablelib/sha256 -> @noble/hashes
  3. js-sha3 -> @noble/hashes
  4. @stablelib/random -> @noble/hashes as @noble/hashes/utils
  5. elliptic, @stablelib/ed25519, @stablelib/x25519 -> @noble/curves
  6. Inhouse Ripemd160 implementation -> @noble/hashes
  7. jwe-vectors.js turned into TS jwe-vectors.ts

Important note regarding different signatures previously created by elliptic package you could see in the test cases. Apparently, elliptic created signature with high value "s". @noble/curves signature gets lower "s" value, being the same fully valid signature.

Another issue: for EdDSASigner a private key is expected to contain 64 bytes. Really, private key is only 32 bytes. The second 32 bytes are a public key. Maybe at some point, a constraint should be relieved.

One benefit apart relying on a solid cryptography libraries, is speed. Tests in the upstream take 22s on my machine. After changes in PR, the same test suite takes 4s.

Re: @scure/base for text encoding/decoding: I am conflicted. uint8arrays and multiformats are quite often used alongside did-jwt, so adding @scure/base would add to the end application size, rather than reduce it. And, @paulmillr names you use like base64url and base64 are quite misleading if compared to multiformats analogs because of padding.

ukstv commented 1 year ago

Is impact on package size also positive?

The best way to gauge size is a bundle prepared by webpack. It contains all the dependencies included as far as I can see. According to the bundle size, current upstream is 236K, changes in the PR reduce it to 133K. Looks like ~40% reduction.

paulmillr commented 1 year ago

Re: @scure/base for text encoding/decoding: I am conflicted. uint8arrays and multiformats are quite often used alongside did-jwt

multiformat libraries are using unaudited garbage dependencies

so adding @scure/base would add to the end application size, rather than reduce it.

true, so what's needed is switching multiformats to noble - unfortunately they don't seem to care much, i've tried an issue one time and they mentioned it ain't really needed: https://github.com/multiformats/js-sha3/issues/48 but open to pull requests

And, @paulmillr names you use like base64url and base64 are quite misleading if compared to multiformats analogs because of padding.

padding is easily adjustable in code. we have an open issue for the native features https://github.com/paulmillr/scure-base/issues/4

ukstv commented 1 year ago

To minimize the number of updates to the yarn.lock file it's probably worth version locking those to see only the affected changes from replacing the dependencies.

Okay, I am going to try minimising changes to yarn.lock.

ukstv commented 1 year ago

padding is easily adjustable in code. we have an open issue for the native features

@paulmillr I am aware I can create proper base64url using combinators provided in @scure/base/utils. This seems far from optimal to me. Are you open to rename exported names according to multibase table ? As far as I know, the table is the most comprehensive list of various encoding combinations. It gives you no surprises with regards to padding and casing.

Anyway, encoding/decoding I believe is a slightly separate topic. Let's do with crypto libs first.

paulmillr commented 1 year ago

Are you open to rename

if it ain't breaking backwards compat, sure thing. breaking will need a new breaking release, not optimal rn

nklomp commented 1 year ago

Very nice work!

I haven't done a proper review, but one thing I did notice is that the e256kSigner test is producing different signatures for the same inputs/keys. What is the reason for those?

ukstv commented 1 year ago

@nklomp Copied from the PR description :)

Important note regarding different signatures previously created by elliptic package you could see in the test cases. Apparently, elliptic created signature with high value "s". @noble/curves signature gets lower "s" value, being the same fully valid signature.

If you look closer to the changed signatures in the tests, you'll see that they begin the same. The second part encoding "s" is different.

nklomp commented 1 year ago

Hehehe, thanks. Guess I was too excited about these changes, to read that ;)

paulmillr commented 1 year ago

There is a lowS: false option that will bring the old behavior if you need it.

However, the old behavior (your current) is not safe and introduces signature malleability.

ukstv commented 1 year ago

@kdenhartog Now yarn.lock should be fine. One could easily see scope of the changes there.

ukstv commented 1 year ago

@mirceanis Hey, is there anything I can do to move the PR forward?

The gains are pretty significant:

mirceanis commented 1 year ago

@mirceanis Hey, is there anything I can do to move the PR forward?

I'm reviewing it now. This looks great so far 🚀

uport-automation-bot commented 1 year ago

:tada: This PR is included in version 7.0.0 :tada:

The release is available on:

Your semantic-release bot :package::rocket:

ukstv commented 1 year ago

Yahoo!!