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 ES256K support #90

Merged
merged 1 commit into from
Feb 1, 2024
Merged

Add ES256K support #90

merged 1 commit into from
Feb 1, 2024

Conversation

imirkin
Copy link
Contributor

@imirkin imirkin commented Feb 18, 2021

Unfortunately it doesn't seem like RFC 8812 provides any "worked" examples, unlike some of the other RFCs. This roundtrips successfully with the implementation I recently added to github.com/lestrrat-go/jwx.

There's also a libsecp256k1 library from the bitcoin project (https://github.com/bitcoin-core/secp256k1). However it doesn't seem to have a stable release, and it's unclear that the extra dependency would be worth it over openssl for this usage.

If this is accepted, I'd be happy to add EdDSA and x25519/x448 curve support.

@sarroutbi
Copy link
Collaborator

Hello @imirkin. Could you please try to rebase master branch to this one? This would allow to retrieve changes with the newest CI strategy (based on Github action), so that it could be checked if checks pass on your PR?

@codecov-commenter
Copy link

codecov-commenter commented Oct 7, 2021

Codecov Report

Merging #90 (63d28ab) into master (4248b93) will decrease coverage by 1.16%.
The diff coverage is 94.11%.

❗ Current head 63d28ab differs from pull request most recent head c43d553. Consider uploading reports for the commit c43d553 to get more accurate results

@@            Coverage Diff             @@
##           master      #90      +/-   ##
==========================================
- Coverage   77.58%   76.41%   -1.17%     
==========================================
  Files          60       57       -3     
  Lines        5648     5322     -326     
==========================================
- Hits         4382     4067     -315     
+ Misses       1266     1255      -11     
Impacted Files Coverage Δ
lib/openssl/ecdsa.c 80.89% <91.66%> (-0.69%) ⬇️
lib/openssl/ec.c 80.64% <100.00%> (ø)
lib/openssl/jwk.c 69.10% <100.00%> (-1.10%) ⬇️
cmd/jws/jws.h 76.47% <0.00%> (-11.41%) ⬇️
tests/api_jwe.c 92.85% <0.00%> (-3.92%) ⬇️
cmd/jwe/enc.c 41.42% <0.00%> (-2.41%) ⬇️
cmd/jws/sig.c 65.85% <0.00%> (-2.33%) ⬇️
cmd/jwe/dec.c 51.85% <0.00%> (-2.27%) ⬇️
lib/openssl/ecmr.c 75.43% <0.00%> (-1.62%) ⬇️
lib/cfg.c 39.47% <0.00%> (-1.56%) ⬇️
... and 37 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update a091f56...c43d553. Read the comment docs.

Copy link
Collaborator

@sarroutbi sarroutbi left a comment

Choose a reason for hiding this comment

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

Changes look good to me. I will add @sergio-correia for further inspection, but code is pretty clean IMHO.

@sarroutbi
Copy link
Collaborator

@sergio-correia : In my opinion, this looks good. Please, provide your feedback when possible

@sergio-correia
Copy link
Collaborator

Looks good to me, thanks.

@sarroutbi sarroutbi merged commit e6a7ae7 into latchset:master Feb 1, 2024
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.

4 participants