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

Add an exportable option to default-plugins #109

Merged
merged 11 commits into from
Mar 15, 2024
Merged

Conversation

kshinn
Copy link
Contributor

@kshinn kshinn commented Mar 4, 2024

Addresses issue #108

This simply adds and exportable parameter to the EcdsaKeyPair class to allow the key to be exported.

@matheus23
Copy link
Member

With the provided fix, I can successfully export the key, however it exports it in pkcs8 format. There doesn't seem to be a symmetrical way to import this key material. The import function uses the jwk format instead of a raw format. In order to make this change useful, we would need to also add an importFromRaw type of method to this class. This may start to diverge from the original intent of these classes.

The export function hard-codes the pkcs8 format:

const arrayBuffer = await webcrypto.subtle.exportKey(
"pkcs8",
this.keypair.privateKey
)

Perhaps it makes sense to change it to jwk. It's not really a breaking change, because you weren't able to export P-256 keys previously at all, anyways.

@matheus23
Copy link
Member

It is weird that it wouldn't match the RSA export format though (which is also pkcs8).

@kshinn
Copy link
Contributor Author

kshinn commented Mar 5, 2024

To me, it looks like ed25519 is the only plugin with a full / symmetric import-export support. The p256 looks asymmetric (import is jwk, export is pkcs8). RSA only looks to export and doesn't have an import. As part of this PR, I could revamp both p256 and RSA to have an import and export based on pkcs8. What do you think?

@matheus23
Copy link
Member

Sounds good. I'll leave it up to you to whether it's going to be JWK or PCKS8 :) ✌️

@kshinn
Copy link
Contributor Author

kshinn commented Mar 7, 2024

Ok, I did a pass and got symmetric import / export functionality for all of the plugins. I tried to reuse where I could, but realized a lot of the import type functions are there for other purposes (usually in the process of signing or verifying a message). I went with JWK where it was available (p256 and rsa). Ed25519 is using a different library and doesn't appear to support JWK, but I it was the easiest to get working with the existing format.

@kshinn kshinn changed the title Add an exportable option to EcdsaKeyPair Add an exportable option to default-plugins Mar 7, 2024
@matheus23
Copy link
Member

@kshinn is there a reason why you removed the uint8arrays dependency?
Buffer is only available in nodejs, and this project also targets working in browsers, so relying on Buffer is a no-go for its goals, unfortunately.

@kshinn
Copy link
Contributor Author

kshinn commented Mar 7, 2024

Oops, that wasn't supposed to commit to this branch. I'll return the dependency. Long story short: I'm trying to make this library work in a constrained nodejs setting (total size needs to be as small as possible). I'm trying to eliminate some of the polyfills and work directly with node libs to do this.

But this is unrelated to the changes I'm making here and I didn't see that they leaked in. I will undo those changes in this branch.

Copy link
Member

@matheus23 matheus23 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Putting this on approve, because I trust you to fix the last nit before we merge :)

packages/default-plugins/src/ed25519/keypair.ts Outdated Show resolved Hide resolved
Copy link
Member

@matheus23 matheus23 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for your work, this looks good to me :)

@matheus23 matheus23 merged commit bf35b41 into ucan-wg:main Mar 15, 2024
1 check passed
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 this pull request may close these issues.

2 participants