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

reconcile the UCAN keypair API with keystore-idb #2

Closed
nichoth opened this issue Oct 16, 2021 · 13 comments
Closed

reconcile the UCAN keypair API with keystore-idb #2

nichoth opened this issue Oct 16, 2021 · 13 comments

Comments

@nichoth
Copy link
Contributor

nichoth commented Oct 16, 2021

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
Copy link
Collaborator

dholms commented Oct 19, 2021

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 nichoth changed the title reconcile the keypair API with keystore-idb reconcile the UCN keypair API with keystore-idb Oct 19, 2021
@nichoth nichoth changed the title reconcile the UCN keypair API with keystore-idb reconcile the UCAN keypair API with keystore-idb Oct 19, 2021
@nichoth
Copy link
Contributor Author

nichoth commented Oct 19, 2021

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
Copy link
Member

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

  • it doesn't require an exportable key (e.g. no privateKey: Uint8Array property or similar)
  • it doesn't depend on the WebCrypto API.

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
Copy link
Contributor Author

nichoth commented Oct 20, 2021

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
Copy link
Collaborator

dholms commented Oct 20, 2021

I like it @matheus23 👌

@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
Copy link
Collaborator

dholms commented Oct 20, 2021

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
Copy link
Contributor Author

nichoth commented Oct 21, 2021

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
Copy link
Member

matheus23 commented Oct 21, 2021

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
Copy link
Collaborator

dholms commented Oct 22, 2021

Good call @matheus23 I like it 👍

I might hack on this a bit today 👀

@dholms
Copy link
Collaborator

dholms commented Oct 22, 2021

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

Thoughts?

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

@matheus23
Copy link
Member

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

@dholms
Copy link
Collaborator

dholms commented Oct 22, 2021

Alrighty I decided to just go for it real quick:
#4

@matheus23
Copy link
Member

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

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

No branches or pull requests

3 participants