ucan-wg / ts-ucan

Auth tokens for a distributed, user-controlled world
https://www.npmjs.com/package/ucans
Apache License 2.0
98 stars 12 forks source link

reconcile the UCAN keypair API with `keystore-idb` #2

Closed nichoth closed 2 years ago

nichoth commented 2 years ago

The ucans library has a method ucan.keypair.create, which returns a keypair object with a .did method. In real life you would want to use the keystore-idb library though to manage persistent keys over time. However, the keystore-idb library returns a different API for a keypair. I started by forking the keystore-idb repo here; this gives a method getKeypair that should return the same API that is returned by ucans.

It uses a different test library and bundler though, so not ideal for merging at this point

dholms commented 2 years ago

Hey @nichoth thanks for the issue.

I tried to make this library as compatible as possible. You're right that keystore-idb uses a different interface (centered around keystores instead of keypairs). Since ucan.token.build takes a Keypair interace, you would be able to make a class on top of keystore-idb that matches that interface and can be passed to build. Alternatively, you could use ucan.token.buildParts to just returns the formatted header & payload for the ucan, then ucan.token.addSignature can add the signature (and takes a callback that keystore-idb can handle).

I don't think we want to pull keystore-idb in a dependency on this library. But I do like the idea of making the ks.getKeypair interface match the Keypair interface in this library. I will note in your code that Ed25519 keys and webcrypto elliptic curve keys (NIST 256 in this case) are not the same curve and so do not translate to one another. Webcrypto does not implement Ed25519 yet.

I'm also not sure that keystore-idb should rely on the ucans library. I think we'll want matching interfaces but maintained separately. Another option (to prevent repeat code) is that we introduce another library that both of these rely on to handle keypairs & dids.

Thanks for bringing this up. Good food for thought, and something I'd like to address soon. Feel free to post any questions/issues you run into along the way.

nichoth commented 2 years ago

I think we'll want matching interfaces but maintained separately. Another option (to prevent repeat code) is that we introduce another library that both of these rely on to handle keypairs & dids.

I like that idea. There could be another dependency, @fission/keypair or something, that both these would depend on.

matheus23 commented 2 years ago

I think the Keypair type in this library is very elegant.

That's why I think it's a great candidate for an interface between webnative and the ucans library.

A @fission/keypair library might be redundant. I think we'd rather support the Keypair type (or a conversion into it) in either webnative or keystore-idb.

nichoth commented 2 years ago

A @fission/keypair library might be redundant. I think we'd rather support the Keypair type (or a conversion into it) in either webnative or keystore-idb.

True it seems unnecessary. Would keystore-idb then need to depend on the ucan library in this case, to get access to the Keypair type?

dholms commented 2 years ago

I like it @matheus23 :ok_hand:

@nichoth It does not. Since it's an interface, we can just use a matching interface in keystore-idb and it should work just fine.

dholms commented 2 years ago

Side note on this:

I will note in your code that Ed25519 keys and webcrypto elliptic curve keys (NIST 256 in this case) are not the same curve and so do not translate to one another. Webcrypto does not implement Ed25519 yet.

I am planning to implement both secp256k1 & the NIST curves in this library in the relatively short term, so that will give even better compatibility with keystore-idb (which uses the NIST curves)

nichoth commented 2 years ago

I see; thanks @dholms. So these remain completely decoupled — ucan & keystore-idb. I suppose that works out because of typescript? meaning if the keypair in, say ucan changes, it would be caught at compile time by the interface in the keystore-idb repo.

matheus23 commented 2 years ago

I suppose that works out because of typescript?

Yes. We would duplicate the type in ucan and keystore-idb. Typescript uses "structural typing". So whether some value of type A is a valid input to a function that takes a type B depends on whether A's structure fits into B's structure. That's unlike "nominal typing" used in e.g. java or haskell (mostly), where A fits B only if A is defined in the same place as B (or a subtype etc.).


Also, I'd actually like to kick off a small discussion about the Keypair type. Can we simplify it a little bit, so its definition is more concise and eliminates impossible states?

I'd propose this:

export interface Keypair {
  publicKey: Uint8Array
  keyType: KeyType
  sign: (msg: Uint8Array) => Promise<Uint8Array>
}

Thus, removing publicKeyStr and did. These could be additional functions (Keypair => string) outside of this interface.

If they're inside the interface, then the interface can be in an impossible state where did() or publicKeyStr returns something that doesn't match the publicKey property, if some implementation were faulty (e.g. I accidentally mess up this definition in keystore-idb).

It would also be possible to only require this reduced Keypair definition as inputs to functions in the ucan package, but provide the user with a Keypair type extended with did and publicKeyStr in the outputs of functions for ease of use.

dholms commented 2 years ago

Good call @matheus23 I like it :+1:

I might hack on this a bit today :eyes:

dholms commented 2 years ago

@matheus23 I also might change KeyType to be type KeyType = 'rsa' | 'ed25519' | 'bls12-381'

Thoughts?

enums in TS kinda suck :stuck_out_tongue: and I'm not sure how well they play in terms of structural typing

matheus23 commented 2 years ago

Oh yeah good point. And I haven't had issues with not using enums so far :D

dholms commented 2 years ago

Alrighty I decided to just go for it real quick: https://github.com/fission-suite/ucan/pull/4

matheus23 commented 2 years ago

I think we can close this issue and continue tracking this in keystore-idb instead.