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

Remove dependencies #77

Closed
dholms opened this issue Apr 27, 2022 · 20 comments · Fixed by #82
Closed

Remove dependencies #77

dholms opened this issue Apr 27, 2022 · 20 comments · Fixed by #82

Comments

@dholms
Copy link
Collaborator

dholms commented Apr 27, 2022

Throwing this out as an idea. If we're using the UCAN library in tightly-secured environments, it may make sense to harden it against supply chain attacks. Cryptography libraries often have 0 dependencies. If we're managing keypairs in a service worker, for instance, I would love to only depend on ts-ucan & tweetnacl: two libraries I trust and be essentially protected from supply chain attacks.

Granted right now the lib has only 4 dependencies & 1 of them is in house, so this isn't a terrible risk. But on the flip side, it also means it's not a terrible task to get to ✨ 0 dependency land ✨

  • uint8arrays: utility functions for uint8arrays. These are mainly used for going from bytes <-> hex/b64 or vice versa. I think we could easily replicate these within the library
  • semver: used for version check on token. would also be easy to replicate within the lib
  • one-webcrypto: this is already in house so less of a security concern. but could still be nice to pull it over. unfortunately the trick for switching based on environment with the package.json wouldn't work then...
  • @stablelib/ed25519: we'll be adding more crypto libraries, and we may want to leave it up to the user to decide which one(s) they wish to support (& which implementation they prefer). For instance, we're considering using tweetnacl instead. Could necessary libraries be dynamically injected at runtime? Maybe WebCrypto functions could be injected as well?

Thoughts? I'd be happy to help.

@matheus23
Copy link
Member

Here's the issue that made us decide to use @stablelib/ed25519 instead of tweetnacl: #56

Given that both are from the same author, would you trust both of these libraries?


I'm generally in favor of keeping the dependency footprint low. E.g. vendoring semver totally makes sense to me!

I would argue uint8arrays being under protocol lab's control & them being invested in this project should be an acceptable dependency for similar reasons as one-webcrypto, IMO.

Also want to throw in that if we implement #73 we'll have another protocol labs dependency, dag-json.


It may make sense to look at the whole dependency tree (with transitive dependencies) for our non-dev dependencies. That could also be something informing our decisions.

@hugomrdias
Copy link
Member

my 2cents for crypto i would use https://paulmillr.com/noble/ as much as possible its adopted by eth libs, 0 deps, readable, audited, etc

as for uint8arrays unless we go IPLD theres no point in using it, this https://github.com/nftstorage/ucan.storage/blob/main/src/encoding.js should cover most of our needs.

@dholms
Copy link
Collaborator Author

dholms commented Apr 27, 2022

i actually just did some research on the popular Ed25519 libs.

My only problem with @stablelib/ed25519 is that I didn't find proof of an audit. I trust the author to not be doing anything malicious. But a second set of eyes reviewing for bugs would be nice.

I actually ended up deciding to use noble instead of tweetnacl after reviewing (I made this issue mid-review 😛). Noble wasn't audited last time I used it, but was audited in Feb of this year. It appears to be smaller & faster than both tweetnacl & stablelib.

@dholms
Copy link
Collaborator Author

dholms commented Apr 27, 2022

There's something special about a package with 0 dependencies. That being said, we shouldn't go to absurd ends to do that. If we start introducing a ton of repeat code or antipatterns, it's probably better to just pull the dependency in. If we do, I think it'd be reasonable to limit dependencies to Fission + PL & lock them in at a specific version.

I still think it might make sense to not package any crypto libs in the ucan library. Crypto libs are something that devs can be very particular about & they may want to bring whatever they're currently using to the table, instead of relying on the libraries choice. Also, eventually we'll want to support many more curves/DID systems, and will we want to pull in packages for secp256k1 + bls12-381 + ed25519 + ION resolution + 3id resolution, etc? It might make more sense to inject the dependencies instead of including them in the package.

Thoughts? I'm not 100% sold on the idea tbf, it does add some complexity for the user

@bgins
Copy link
Contributor

bgins commented Apr 27, 2022

One thing to note about the Noble libraries (or at least noble-ed25519) is they use big integer literals, which are an es2020 feature that cannot be transpiled to older targets. Not sure how much of an issue this is now, but it has been an issue in with some bundlers in the past: sveltejs/kit#2220

Of course, this is the kind of issue that fades with time, but it's worth considering.

@dholms
Copy link
Collaborator Author

dholms commented Apr 27, 2022

ah good point. well that's a shame. would be worth looking into if most bundlers handle it well now

@matheus23
Copy link
Member

as for uint8arrays unless we go IPLD theres no point in using it, this https://github.com/nftstorage/ucan.storage/blob/main/src/encoding.js should cover most of our needs.

Yeah that makes sense to me, we can also just vendor that code.

If we do, I think it'd be reasonable to limit dependencies to Fission + PL & lock them in at a specific version.

Yeah, I'd agree with that too.

I still think it might make sense to not package any crypto libs in the ucan library. Crypto libs are something that devs can be very particular about & they may want to bring whatever they're currently using to the table, instead of relying on the libraries choice. Also, eventually we'll want to support many more curves/DID systems, and will we want to pull in packages for secp256k1 + bls12-381 + ed25519 + ION resolution + 3id resolution, etc? It might make more sense to inject the dependencies instead of including them in the package.

Thoughts? I'm not 100% sold on the idea tbf, it does add some complexity for the user

The DID resolution esp makes sense to me, we don't want to implement various DID resolution methods, because they can be as heavy as "lookup something in this smart contract's state".
But maybe that's a step 2 that we do once we support the first non-did:key DID.

Step 1 then would be about abstracting away just the non-webcrypto crypto stuff? Today that's just ed25519?

Let's figure out what the interface for that dependency-injection would be.

Something like this?

interface CryptoFuncs {
  signEd25519 //...
  verifyEd25519 // ...
  // ...
}

Or do we find a way to abstract away whole Keypair classes, including the static methods?

The next thing to figure out would be how we inject these functions. I can think of two options:

  • a module-local globally mutable variable. Then "call this function before you do anything with ts-ucan & ed25519".
  • an optional parameter to all functions that may end up running signature validation/generation.

Thoughts?

@dholms
Copy link
Collaborator Author

dholms commented Apr 28, 2022

Step 1 then would be about abstracting away just the non-webcrypto crypto stuff? Today that's just ed25519?

Just Ed25519 for right now. Though we'll likely be needing secp256k1 soon as well. There are some references to bls12-381 in the code form when I was working on that filecoin-cosigner project last year. Altho i think we just parse it right now & say "we don't support bls12-381" 😛

Let's figure out what the interface for that dependency-injection would be.

So we parse a DID & know the keytype based on the magic bytes prefix. So maybe the interface is something like

interface CryptoFuncs {
  [alg: string]: {
    sign //...
    verify //...
  }
}

So this would allow you to easily bring crypto functions that aren't supported by the main lib.

The next thing to figure out would be how we inject these functions. I can think of two options:
a module-local globally mutable variable. Then "call this function before you do anything with ts-ucan & ed25519".
an optional parameter to all functions that may end up running signature validation/generation.

Hmmm I was about to say the former, but I'm not sure of the security ramifications of having it as a global variable & letting any old javascript switch out your crypto fns 🤔 Need to think through the attack vectors on that one, but it doesn't sound great lol

The DX of an optional param isn't the best but may make the most sense.


Tbh im torn between 0 dependencies & batteries included. It would suck to need 2 libs (like ucans & ucans-default-crypto to use ucans in your app, or even worse muddle about defining every crypto alg you may run across.

I guess we could have ucans-stripped-down with no crypto dependencies or anything & ucans use that package & include all the batteries-included fun.

The more I'm thinking through this, the more I'm thinking it may make more sense to stay the course & just include some sensible crypto packages that are version locked. And then let the dev override them. If you want tweetnacl & this uses stablelib, then just import it & it overwrites stablelib.

Also in terms of security-conscious packages, it doesn't get much better than a crypto lib so if those are our only dependencies, I suppose it's really not that bad.


The DID resolution esp makes sense to me, we don't want to implement various DID resolution methods, because they can be as heavy as "lookup something in this smart contract's state".

Yeah we can probably do this a similar way with something like

interface DidMethods {
  [didType: string]: (did: string) => Promise<DidDoc> // or maybe just returns the root key here?
}

And services can decide which DIDs they want to support. We only want to support two out of the box (not including did:key) for instance.

But maybe that's a step 2 that we do once we support the first non-did:key DID.

will wanna chat with you about this soon btw. because we're going to need ION resolution & resolution for a custom DID format we're proposing

@matheus23
Copy link
Member

matheus23 commented May 2, 2022

Okay. The minimum thing we all agree on at least is these things:

  • locking the dependencies
  • remove(/vendor) semver
  • vendor something to convert utf8 bytearrays <-> strings or base64 <-> bytes (i.e. what uint8arrays provides).

I'd love to merge something like that 😁


That's not to say that I want to end this discussion! Just trying to steer towards something.

@dholms
Copy link
Collaborator Author

dholms commented May 2, 2022

Sweet sounds good 👍

That seems like a reasonable first step & we can re-address the rest later if need be.

I likely won't get to this immediately, but hopefully in the next few weeks

@pfrazee
Copy link

pfrazee commented Jun 16, 2022

Hopefully this isn't too off topic --

I'm working on a React Native app, and there is (surprise!) absolutely no crypto API built in. I decided to polyfill WebCrypto with msr-javascript-crypto and the native ios/android CSRNGs. Unfortunately I don't know if it's possible to run the polyfills before the bundle gets run, so one-webcrypto presents a challenge for me.

My solution has been to configure my bundler to s/one-webcrypto/wecrypto-paulyfill/ during the compile, which is fine for me but not ideal for other folks.

AFAICT there are two decent options:

  1. Give ucans consumers the ability to pass in their crypto
  2. Have folks upstream their polyfills into one-webcrypto

I'd recommend against option 2 because maintaining polyfills across platforms is prone to maintenance rot. For option 1, I think it'd be fine to start with making WebCrypto the required API that's provided by consumers. You could accept a generalization too if people want to use other APIs but forcing WebCrypto might reduce the potential for mistakes.

@expede
Copy link
Member

expede commented Jun 17, 2022

Agreed with @pfrazee that injecting crypto APIs (defaulting to WebCrypto) is sensible. We've had a lot of success with this strategy in general 👍

@matheus23
Copy link
Member

Progress on the above: With #79 we've removed the semver library and replaced it with the minuscule amount of logic that we're using ourselves from that library. 😛


About dependency injection crypto: I'm still thinking about the level we may want to do this at.
At some point we want to have DID resolution pluggable, since some DID methods are very complex to resolve and require e.g. a network connection. At the same layer we may want to do the dependency injection for crypto.
So what we dependency-inject is something like a did-signature-checking function like (did: string, signature: Uint8Array, signal?: AbortSignal) => Promise<boolean>. I think all other crypto-utilizing functions in ts-ucan take a Keypair as an argument, which already encapsulates that dependency-injection.

@matheus23
Copy link
Member

matheus23 commented Jun 20, 2022

So what I would propose concretely would be a map of DID method names to such functions:

interface DIDImplementations {
  [method: string]: (did: string, signature: Uint8Array, signal?: AbortSignal) => Promise<boolean>
}

The next question is how we provide that to ts-ucan functions.

Here's a bunch of options:

  1. A global state record of supported DID implementations. Something like
    // module-global variable
    export let didImplementations: DIDImplementations
  2. Passing in another parameter to all ts-ucan functions:
    export async function verify(..., supportedDIDs: DIDImplementations)

Also, I'm pretty sure we want to provide a default DIDImplementations with the did:key stuff that uses one-webcrypto. I see two main ways of doing that:

  1. Just implement that in the ucans library (ts-ucan). The downside of this is that one-webcrypto is still in the dependencies and may(?) mess up something like react native or other environments.
  2. We split ts-ucan into ucans-core and ucans, where ucans-core doesn't call any crypto functions / doesn't have any further dependencies. On top of that we provide a "reasonable default for most cases" ucans library that just instantiates the whole public API of ucans-core with a default DIDImplementations record.

What do you think @dholms and @pfrazee?

@dholms
Copy link
Collaborator Author

dholms commented Jun 20, 2022

So what we dependency-inject is something like a did-signature-checking function like (did: string, signature: Uint8Array, signal?: AbortSignal) => Promise. I think all other crypto-utilizing functions in ts-ucan take a Keypair as an argument, which already encapsulates that dependency-injection.

That sounds right^^
The only down side here is that every did:key would get verified by the same function. So you couldn't, for instance, load in default UCAN key validation & just add one extra key method. I'm not sure there is a way around that though.

Also, I'm pretty sure we want to provide a default DIDImplementations with the did:key stuff that uses one-webcrypto. I see two main ways of doing that:

  1. Just implement that in the ucans library (ts-ucan). The downside of this is that one-webcrypto is still in the dependencies and may(?) mess up something like react native or other environments.
  2. We split ts-ucan into ucans-core and ucans, where ucans-core doesn't call any crypto functions / doesn't have any further dependencies. On top of that we provide a "reasonable default for most cases" ucans library that just instantiates the whole public API of ucans-core with a default DIDImplementations record.

I'm leaning towards 2 on this one. It's a bit more work to maintain two libraries & keep them in sync. But I think the benefits of the core lib working in any env and being fully in control of which crypto & DID implementations you use makes it worth it

  1. A global state record of supported DID implementations
  2. Passing in another parameter to all ts-ucan functions:

I'm a bit of a toss up on this one. I think I also lean towards 2 on this one as well. As you pointed out, this logic is only needed in validation and could be passed in similarly to how ValidateOptions is passed. If this was done in ucans-core, then ucans could re-export validation functions with the default functions added in so it's no worse DX.

Global state records can be a pain sometimes: not knowing if you've set it or not, wanting different options in different contexts, etc.

@matheus23
Copy link
Member

The only down side here is that every did:key would get verified by the same function. So you couldn't, for instance, load in default UCAN key validation & just add one extra key method. I'm not sure there is a way around that though.

We could split our package into three (yay more packages :P).
ucans = ucans-core + ucans-default-did-implementations.

So if you simply want to add support for a single did:key which isn't provided by ucans-default-did-implementations, you can just instantiate ucans-core yourself, and implement did:key and falling back on ucans-default-did-implementations.

Needs a better name though 🤔


My gut also agrees with you: I'm leaning towards the second options.

I think this is a plan 👍

@dholms
Copy link
Collaborator Author

dholms commented Jun 20, 2022

We could split our package into three (yay more packages :P).
ucans = ucans-core + ucans-default-did-implementations.

Yup that seems right to me^^

maybe just ucan-defaults? or ucans-validation, implying it's the default for validation, but can of course be superseded

@hugomrdias
Copy link
Member

Injecting did/crypto stuff in the params seems good, we can also make a wrapper class to simplify functions params and give user a different way to interact with the api. We have been doing this in multiple libs, define the api with functions for max flexibility with a companion wrapper class that aggregate params in the constructor and calls functions from methods.

I would not make multiple packages but one package with multiple entrypoints in the export map. It's simpler and easier to maintain and change.

@matheus23
Copy link
Member

We have been doing this in multiple libs, define the api with functions for max flexibility with a companion wrapper class that aggregate params in the constructor and calls functions from methods.

Sounds interesting. Can you link some example code @hugomrdias?

I would not make multiple packages but one package with multiple entrypoints in the export map. It's simpler and easier to maintain and change.

Yeah, that's another good option. One downside I can identify is that there's nothing preventing you from accidentally using a dependency that isn't meant to be used in one of the entrypoints.

@dholms
Copy link
Collaborator Author

dholms commented Jun 22, 2022

I would not make multiple packages but one package with multiple entrypoints in the export map. It's simpler and easier to maintain and change.

So unfortunately metro (the main bundler for react-native) doesn't support package.json "exports" 🙃
And any issues with dependencies in the bundling stage would still likely present themselves as they're listed in the package.json.

That being said, I understand if we don't want to rework 1 package into 2-3 for the sake of getting this into react-native. As Paul mentioned, he figured out a way to get around metro on this one.

But it also seems a shame to have to install dependencies to make use of the 0-dependency subsection of a library. Like if I want to only using ucans-core but have to install various crypto libraries to do so...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants