Closed dholms closed 2 years ago
Here's the issue that made us decide to use @stablelib/ed25519
instead of tweetnacl
: https://github.com/ucan-wg/ts-ucan/issues/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.
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.
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.
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
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: https://github.com/sveltejs/kit/issues/2220
Of course, this is the kind of issue that fades with time, but it's worth considering.
ah good point. well that's a shame. would be worth looking into if most bundlers handle it well now
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:
Thoughts?
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
Okay. The minimum thing we all agree on at least is these things:
I'd love to merge something like that :grin:
That's not to say that I want to end this discussion! Just trying to steer towards something.
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
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:
ucans
consumers the ability to pass in their cryptoone-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.
Agreed with @pfrazee that injecting crypto APIs (defaulting to WebCrypto) is sensible. We've had a lot of success with this strategy in general 👍
Progress on the above: With https://github.com/ucan-wg/ts-ucan/pull/79 we've removed the semver
library and replaced it with the minuscule amount of logic that we're using ourselves from that library. :stuck_out_tongue:
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.
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:
// module-global variable
export let didImplementations: DIDImplementations
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:
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.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?
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:
- 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.
- 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
- A global state record of supported DID implementations
- 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.
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 :thinking:
My gut also agrees with you: I'm leaning towards the second options.
I think this is a plan :+1:
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
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.
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.
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...
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 ✨
Thoughts? I'd be happy to help.