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

fix(utils): convert JWK with curv Ed25519 to X25519 #1078

Merged
merged 1 commit into from
Dec 5, 2022

Conversation

nickreynolds
Copy link
Contributor

What is being changed

allow JsonWebKeys with crv = Ed25519 to be converted to crv = X25519

Quality

Check all that apply:

  • I want these changes to be integrated
  • I successfully ran yarn, yarn build, yarn test, yarn test:browser locally.
  • I allow my PR to be updated by the reviewers (to speed up the review process).
  • I added unit tests.
  • I added integration tests.
  • I did not add automated tests because _________, and I am aware that a PR without tests will likely get rejected.

@codecov
Copy link

codecov bot commented Nov 29, 2022

Codecov Report

Merging #1078 (9c5f2f5) into next (125bf42) will decrease coverage by 0.23%.
The diff coverage is 78.37%.

❗ Current head 9c5f2f5 differs from pull request most recent head 7c91ed3. Consider uploading reports for the commit 7c91ed3 to get more accurate results

Additional details and impacted files
@@            Coverage Diff             @@
##             next    #1078      +/-   ##
==========================================
- Coverage   80.25%   80.01%   -0.24%     
==========================================
  Files         118      127       +9     
  Lines        4056     4608     +552     
  Branches      875     1000     +125     
==========================================
+ Hits         3255     3687     +432     
- Misses        798      921     +123     
+ Partials        3        0       -3     

Copy link
Member

@mirceanis mirceanis left a comment

Choose a reason for hiding this comment

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

This looks great!
Do you plan to fill the todo test cases as well?

@nickreynolds
Copy link
Contributor Author

I would like to add tests for all the unpack cases as well, but at least for JWK, I wouldn't know how to do that. We don't currently support importing JWKs into KeyManager, do we?

@mirceanis
Copy link
Member

We don't currently support importing JWKs into KeyManager, do we?

No, we don't. Private key data has to be converted to hex. We should be able to do the conversion with existing utils/deps:
For JWK ec private keys this means converting the d property from base64 to base16.

@nklomp
Copy link
Member

nklomp commented Nov 29, 2022

In our SSI Wallet we have code that actually has support to covert DID jwk and thus JWKs to hex. We intent to bring that to Veramo, but maybe you can already use it

https://github.com/Sphereon-Opensource/ssi-mobile-wallet/blob/79b7cb56a3d009f741463fb0ca0fb7997c1a090f/src/services/identityService.ts#L132

Called from https://github.com/Sphereon-Opensource/ssi-mobile-wallet/blob/79b7cb56a3d009f741463fb0ca0fb7997c1a090f/src/services/identityService.ts#L165

The last 3 methods are basically delegating to Veramo and then have specific support for JWKs

@nklomp
Copy link
Member

nklomp commented Nov 29, 2022

Btw we also have a did jwk provider and resolver https://github.com/Sphereon-Opensource/ssi-sdk/tree/develop/packages/jwk-did-provider

@nickreynolds
Copy link
Contributor Author

In our SSI Wallet we have code that actually has support to covert DID jwk and thus JWKs to hex. We intent to bring that to Veramo, but maybe you can already use it
Yes what you have seems to be essentially what this PR provides, with the addition of the p256 curve support, correct? Would love to bring that into Veramo

The did-jwk-provider also seems like it would help a lot with unit tests

@nickreynolds nickreynolds merged commit deb546b into next Dec 5, 2022
@nickreynolds nickreynolds deleted the nickreynolds/2020-keys branch December 5, 2022 18:12
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.

3 participants