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

Support for NIST Secp256r1 keys and ES256 signatures #1039

Conversation

nklomp
Copy link
Member

@nklomp nklomp commented Oct 23, 2022

What issue is this PR fixing

Add support for NIST Secp256r1 keys and ES256 signatures

What is being changed

This PR adds support for Secp256r1/ES256. Reason is a need to sign a Proof of Possession JWT in the OpenID 4 Verifiable Credentials track of the W3C EDU Jobs for the Future Plugfest 2. Most OID4VCI issuer support ES256 and only some support ES256k. Since Veramo already support ES256k and has DID-JWT as a dep, which supports Secp256r1/ES256, this PR does not add any additional deps that weren't already transitive dependencies for KMS-Local anyway (elliptic package).

Quality

Check all that apply:

  • I want these changes to be integrated
  • I successfully ran yarn, yarn build, yarn test, yarn test:browser locally. (Yarn test fails on Ethereum tests, but that has nothinig todo with this PR)
  • 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 Oct 23, 2022

Codecov Report

Merging #1039 (f6216fa) into next (125bf42) will increase coverage by 0.68%.
The diff coverage is 83.56%.

Additional details and impacted files
@@            Coverage Diff             @@
##             next    #1039      +/-   ##
==========================================
+ Coverage   80.25%   80.93%   +0.68%     
==========================================
  Files         118      124       +6     
  Lines        4056     4533     +477     
  Branches      875      971      +96     
==========================================
+ Hits         3255     3669     +414     
- Misses        798      861      +63     
  Partials        3        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.

Thank you for adding this!

There are a couple of things to resolve before this can be released successfully:

First one is the line endings being changed in some of the plugin-schema.json files. I think it will fail in the release process otherwise. This can also be fixed in another PR, if your machine doesn't allow it.

Second one is a clarification about ES256-R. Is it defined somewhere as a JWA? If it isn't we should remove it.

We've been criticized before for adding what looks like standards but are not, and I somewhat understand that position, so we shouldn't be creating extra confusion about algorithms.

@nklomp
Copy link
Member Author

nklomp commented Oct 24, 2022

Thanks for the quick feedback @mirceanis. You are correct that ES256-R is not standard, so removed it altogether.

Reverted the EOL changes in the plugin.schema.json. I did notice them, but hoped they wouldn't pose a problem. My IDE and the editor in Github does automatically add a newline at the end of the file though.

Another thing I noticed is that after running a yarn build-clean my plugin.json contained several 'description' lines that aren't present in the repo

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.

looks great, thanks for adding this!

@mirceanis mirceanis merged commit 61eb369 into decentralized-identity:next Oct 24, 2022
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