Skip to content
This repository has been archived by the owner on Jul 21, 2023. It is now read-only.

Proposal: reduce scope of modules #220

Open
achingbrain opened this issue Dec 1, 2021 · 3 comments
Open

Proposal: reduce scope of modules #220

achingbrain opened this issue Dec 1, 2021 · 3 comments
Labels
P3 Low: Not priority right now

Comments

@achingbrain
Copy link
Member

Looking through our use of this library in the ipfs/libp2p stack, it's mostly used for:

a) getting random bytes
b) marshalling/unmarshalling keys
c) signing/verifying
d) keypair generation

Very little actual encryption/decryption. I wonder if we couldn't split these functions out and/or refactor it so it takes a more bring-your-own-algorithm approach like js-multiformats.

Getting random bytes

This module just re-exports iso-random-stream - maybe consumers could just depend on that instead?

Marshalling/unmarshalling keys

This is mostly so we can create PeerId objects which have pubKey and privKey properties which are PublicKey and PrivateKey objects from this module.

This presents problems as the PublicKey/PrivateKey classes bring the encrypt/decrypt/sign/verify methods with them and the fallback crypto libraries to make them work which makes them impractical to use in the browser, in particular in projects like ipfs-http-client.

Instead what if we just had the pubKey/privKey props as a Uint8Array which we pass into libp2p-crypto when we needed to do some signing.

Signing/verifying & keypair generation

This is done in a few places - IPNS, pubsub, NOISE, etc. These modules don't do encryption so shouldn't need to pull in node-forge.

What if we split this module into capabilities, libp2p-crypto becomes libp2p-crypto-signing, libp2p-crypto-encryption, libp2p-crypto-keypairs, etc. Then you could just depend on the parts that you want. We can use npm workspaces to coordinate releases and ensure cross-compatibility as we do with libp2p-interfaces, ipfs-interfaces, etc. We may end up with multiple modules depending on node-forge but it should be the same version so will be deduped.

We could even go down the route we did with js-multiformats and make it a 'bring-your-own-algorithm' affair if there are requirements for really esoteric algorithms.


The modules we have in the ipfs stack that have a direct non-dev dependency on libp2p-crypto, and their uses of that module are:

ipns

  • Marshalling/unmarshalling keys

peer-id

  • Types
  • Marshalling/unmarshalling keys
  • Keypair generation

ipfs-core

  • Key export from libp2p-keychain

@chainsafe/libp2p-noise

  • Signing

libp2p-interfaces

  • Types

libp2p-kad-dht

  • Marshalling/unmarshalling keys
  • Random bytes

libp2p

  • Key import/export
  • Random bytes
  • Keypair generation
  • Marshalling/unmarshalling keys
  • Password based key derivation (requires node-forge)

Notably, nothing uses encryption/decryption, one of the main reasons we have node-forge here.

@hugomrdias
Copy link
Member

We could start with libp2p-crypto/sign with export maps in this repo and keep the main entry point exactly the same no breaking changes. This would allow to improve our libs to import just the thing it needs.

iso-random-stream can be swapped out now that streams it's not a thing, but there's still safety features in there that i can't find anywhere else.

Password based key derivation can be done with a noble lib no need to use forge, i tried it already it works i didn't push because we would still need forge for RSA and the PR was already big.

Where do we actually encrypt stuff to need forge ?

@hugomrdias
Copy link
Member

@aschmahmann where do we actually encrypt stuff with the old RSA algo that web crypto doesn't support?

@mpetrunic mpetrunic moved this to Needs Investigation in js-libp2p Nov 15, 2022
@achingbrain achingbrain added the P3 Low: Not priority right now label Nov 29, 2022
@jimmywarting
Copy link
Contributor

my recommendation is that we try to use web crypto as much as possible, Deno settled on implementing only web crypto and has it. NodeJS now also has web crypto and can be used instead. those we can remove most duplicated code / logic

for getting random bytes i suggest that u use crypto.getRandomValues

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
P3 Low: Not priority right now
Projects
None yet
Development

No branches or pull requests

3 participants