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

Use audited dependencies #270

Closed
paulmillr opened this issue Feb 16, 2023 · 12 comments
Closed

Use audited dependencies #270

paulmillr opened this issue Feb 16, 2023 · 12 comments
Labels
bug Something isn't working released

Comments

@paulmillr
Copy link
Contributor

paulmillr commented Feb 16, 2023

  • @stablelib/ed25519, @stablelib/x25519, elliptic - replace with noble-curves
  • @stablelib/random, @stablelib/sha256, js-sha3 - replace with noble-hashes
  • bech32 - replace with scure-base
  • @stablelib/xchacha20poly1305, canonicalize, did-resolver, multiformats, uint8arrays will remain

Wanna me send a PR?

@paulmillr paulmillr added the bug Something isn't working label Feb 16, 2023
@nklomp
Copy link
Member

nklomp commented Feb 16, 2023

Not a maintainer, but I think it would be really nice to use audited packages.

I have one question though. Do these libs work on React Native?

As there is overlap in maintainers between Veramo and this package and one of the requirements of Veramo is that it also has to run on RN, this package also needs to run on RN.

If it does not it would break certain apps, like our wallet.

@paulmillr
Copy link
Contributor Author

Yes they do

@nklomp
Copy link
Member

nklomp commented Feb 16, 2023

Cool

@mirceanis What do you think. If you ask me it would be great to have audited packages for these core functionalities needed in this lib

@inevolin
Copy link

inevolin commented Mar 4, 2023

@mirceanis
Copy link
Member

The discord link refers to this server: https://discord.gg/U5SCRnNFuS

@mirceanis
Copy link
Member

I'm in favor of a replacement.

I would have liked to move forward with #170, which aims to refactor this library to allow folks to bring their own crypto implementations instead of us bundling things in, but since I haven't had any time for that it makes more sense to do what you suggest.

@paulmillr
Copy link
Contributor Author

I see you have ES256K, ES256 signers. How should this rewrite behave? Should I simply replace elliptic with noble in these files, or should I create new signers?

@ukstv
Copy link
Contributor

ukstv commented Apr 16, 2023

Great minds think alike: #280

Have just noticed this thread, otherwise would ping you pals first before doing any code.

uport-automation-bot pushed a commit that referenced this issue Apr 19, 2023
# [7.0.0](6.11.6...7.0.0) (2023-04-19)

### Features

* **deps:** replace @stablelib/ with noble-crypto ([#280](#280)) ([0f6221a](0f6221a)), closes [#270](#270)

### BREAKING CHANGES

* **deps:** `ES256*` signers are now enforcing canonical signatures (s-value less than or equal to half the curve order). This will likely break some expectations for dependents that were using the previous versions.
@uport-automation-bot
Copy link
Collaborator

🎉 This issue has been resolved in version 7.0.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

@inevolin
Copy link

inevolin commented Apr 20, 2023

Hey @ukstv , this is wonderful, thank you for doing this!
I was wondering if you're able to do the same for the did-provider-key lib, which still has the @transmute/... dependenceis and they marked as unstable releases. There have been discussions of replacing those as well. That would be amazing!

@ukstv
Copy link
Contributor

ukstv commented Apr 20, 2023

@inevolin The package you mentioned is not directly used by Ceramic ecosystem AFAIK, so I would like to politely refuse the invitation.

What I could propose instead, is maybe you could depend on key-did-provider-ed25519 or key-did-provider-secp256k1 which are cared for. As soon as paulmillr/noble-curves#32 gets merged, we could fully migrate to noble-crypto and leverage its speed and dependability there. That would transitively apply to your package suite as well.

@inevolin
Copy link

interesting! thank you

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working released
Projects
None yet
Development

No branches or pull requests

6 participants