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

enable ES256K for ECDSA signing scheme #113

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

pohutukawa
Copy link

Ethereum, Bitcoin (and uPort) use the secp256k1 curve for their signature scheme. To allow for JWT (and other JOSE schemes) handling in uPort, the ES256K algorithm has been added.

Additionally, requirements-dev.txt contained a redundant entry ecdsa causing a pip install to fail due to it being already listed in the chained requirements.txt.

@codecov
Copy link

codecov bot commented Nov 27, 2018

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 96.56%. Comparing base (e31416a) to head (35fb5b8).
Report is 162 commits behind head on master.

Additional details and impacted files
@@           Coverage Diff           @@
##           master     #113   +/-   ##
=======================================
  Coverage   96.55%   96.56%           
=======================================
  Files          14       14           
  Lines        1075     1076    +1     
=======================================
+ Hits         1038     1039    +1     
  Misses         37       37           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@pohutukawa
Copy link
Author

Ping. Any insights on whether this might make it into python-jose or whether there's something I can do to make it suitable?

@wbobeirne
Copy link

I can confirm that this worked perfectly for my use-case (Decoding Blockstack JWTs, as they use ES256K encoding.) Would love to see this merged!

@pohutukawa
Copy link
Author

I've just fixed up a minor merge conflict that's crept in due to this thing being a bit stale.

Copy link
Collaborator

@zejn zejn 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 refreshing this PR!

I think this would be a welcome functionality to have in python-jose.

I did a quick review, the functionality generally looks good. There's an issue in the ecdsa backend with the hash function implicitly specifying the curve to be used, so current PR can only use P-256K when using ecdsa backend. This should probably be turned around, similarly to the way it's implemented in cryptography backend, so the hash function depends on the curve.

Please also add an entry to the change log now that we have it and reference this PR.

This will need more tests to be more maintainable over the long term. Current tests didn't find the issue with ecdsa backend, flake8 test did, so there's a blind spot somewhere. It would probably be best if the existing EC tests could be parametrized to run with both P-256 and P-256K keys, see how the pytest.mark.parametrize decorator works in other test cases. Tests will also have to check how the library behaves when supplying a P-256 key and choosing P-256K algorithm and reverse, this should probably throw an error.

If you are unsure on any approach, need help, or more time, say so. I'm here to help this make a good contribution and hopefully break as little existing usages as possible.

@@ -26,6 +26,7 @@ class ECDSAECKey(Key):
SHA256: ecdsa.curves.NIST256p,
SHA384: ecdsa.curves.NIST384p,
SHA512: ecdsa.curves.NIST521p,
SHA256: ecdsa.curves.SECP256k1,
Copy link
Collaborator

Choose a reason for hiding this comment

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

Key in line 29 is same as key in line 26, that's a bug.

Copy link
Contributor

Choose a reason for hiding this comment

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

@pohutukawa Please fix.

Copy link
Contributor

@blag blag left a comment

Choose a reason for hiding this comment

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

A few nitpicks to take care of, and one bug to fix.

@@ -36,7 +36,8 @@ def __init__(self, key, algorithm, cryptography_backend=default_backend):
self.hash_alg = {
ALGORITHMS.ES256: self.SHA256,
ALGORITHMS.ES384: self.SHA384,
ALGORITHMS.ES512: self.SHA512
ALGORITHMS.ES512: self.SHA512,
ALGORITHMS.ES256K: self.SHA256
Copy link
Contributor

Choose a reason for hiding this comment

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

Nitpick: I'm a huge fan of trailing commas in lists to minimize the size of diffs (and also makes git blames a lot more easier). Please add a trailing comma to the ALGORITHMS.E256K line.

@@ -26,6 +26,7 @@ class ECDSAECKey(Key):
SHA256: ecdsa.curves.NIST256p,
SHA384: ecdsa.curves.NIST384p,
SHA512: ecdsa.curves.NIST521p,
SHA256: ecdsa.curves.SECP256k1,
Copy link
Contributor

Choose a reason for hiding this comment

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

@pohutukawa Please fix.

@@ -35,7 +36,8 @@ def __init__(self, key, algorithm):
self.hash_alg = {
ALGORITHMS.ES256: self.SHA256,
ALGORITHMS.ES384: self.SHA384,
ALGORITHMS.ES512: self.SHA512
ALGORITHMS.ES512: self.SHA512,
ALGORITHMS.ES256K: self.SHA256
Copy link
Contributor

Choose a reason for hiding this comment

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

Trailing comma here as well.

@twwildey
Copy link
Collaborator

Can you rebase your changes onto the latest master branch and force-update your branch for this PR?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants