Skip to content

Implement PyNaCl backend for Ed25519 keys (part of RFC 8037) - #100

Closed
blag wants to merge 5 commits into
mpdavis:masterfrom
blag:ed25519
Closed

Implement PyNaCl backend for Ed25519 keys (part of RFC 8037)#100
blag wants to merge 5 commits into
mpdavis:masterfrom
blag:ed25519

Conversation

@blag

@blagblag commented Jul 7, 2018

Copy link
Copy Markdown
Contributor

RFC8037 is an extension to JOSE that includes Ed25519 and Ed448 keys.

This PR adds support for Ed25519 keys in a new nacl backend using the wonderful PyNaCl package, and integrates it into the JWK portion of this project.

Unfortunately, while Ed448 keys are part of RFC8037, there are no good Python libraries for those keys yet, so support for them was left out. Implementation of that is a good candidate for future work, but will not be considered part of this PR.

This PR should be good to go (assuming it passes CI tests). 😃

@blag
blagforce-pushed the ed25519 branch 3 times, most recently from d011c89 to dd87583CompareJuly 7, 2018 04:39
@codecov-io

codecov-io commented Jul 7, 2018

Copy link
Copy Markdown

Codecov Report

Merging #100 into master will decrease coverage by 1.85%.
The diff coverage is 95.74%.

Impacted file tree graph

@@ Coverage Diff @@## master #100 +/- ##
==========================================
- Coverage 96.54% 94.68% -1.86% 
==========================================
Files 14 14 Lines 1071 1092 +21 ==========================================
Hits 1034 1034 - Misses 37 58 +21
Impacted FilesCoverage Δ
jose/backends/nacl_backend.py100% <100%> (ø)
jose/constants.py100% <100%> (ø)⬆️
jose/backends/__init__.py50% <71.42%> (-50%)⬇️
jose/jwk.py93.4% <81.81%> (-1.72%)⬇️
jose/utils.py63.15% <0%> (-24.57%)⬇️
jose/backends/cryptography_backend.py96.89% <0%> (-1.23%)⬇️
jose/jws.py96.63% <0%> (-0.09%)⬇️
jose/jwt.py100% <0%> (ø)⬆️
jose/backends/_asn1.py
... and 3 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 1c3c1a6...af5ed97. Read the comment docs.

@blag

blag commented Jul 7, 2018

Copy link
Copy Markdown
ContributorAuthor

Oh, and I forgot to mention: all of the code that I added has 100% test coverage. 👍

@pohutukawa

Copy link
Copy Markdown

I'd really like to see this land in python-jose, as this is straight on route towards also implementing ChaCha20/Poly1305 for JWE. I'd be willing to contribute, as I need both (JWT with Ed25519 and JWE with the above mentioned cipher).

@mpdavis

Copy link
Copy Markdown
Owner

First, my apologies for taking so long to take a look at this.

The only issue I see with this PR is that it introduces a C-based python dependency, which breaks the compatibility with Google App Engine. That said, GAE support has proven to be enough of a pain that I am likely to drop support for it entirely to avoid these complications trying to support further extensions of the JOSE framework.

I would like to officially drop support for GAE compatibility before merging this PR. I will see about getting that done.

@mpdavis

Copy link
Copy Markdown
Owner

@blag I have successfully talked myself about worrying too much about getting a pure-python route for now. If we get these merge conflicts resolved, I can get this merged and simply cut a 4.0.0 release and let anyone that needs a pure-python deploy pin to 3.0.1 for now.

@zejn

zejn commented Aug 30, 2018

Copy link
Copy Markdown
Collaborator

I think that with a few small changes pure-Python can still be usable. And not having to install software you're not using is always a bonus.

First the Ed25519Key imports need to be made so they pass on ImportError both in jose/backends/init.py and jose/jwk.py.
Then advertising ED support in jose/constants.py must be made dependant on successful import of Ed25519Key.
Then it makes sense to add a new entry into extras_require so one can say pip install python-jose[pynacl] to enable Ed25519 support.

@zejnzejn left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

If you can change this then pynacl will not be a hard requirement.

Comment threadjose/backends/__init__.py Outdated
Comment threadjose/constants.py Outdated
Comment threadjose/jwk.py Outdated
Comment threadsetup.py
@pohutukawa

Copy link
Copy Markdown

Are there any insights on whether this branch (after fixing the conflicts of course) will get merged?
It would mean that I can much more easily get a crack on stuff from #104, and thus discontinue my 'home brew' implementation of a very limited JOSE set.

@blag

blag commented Nov 27, 2018

Copy link
Copy Markdown
ContributorAuthor

Sorry, I lost track of this since my last comment. I'll try to implement the suggested changes and fix the merge conflicts once my life calms down a bit, which should be this or next week.

@blag

blag commented Nov 27, 2018

Copy link
Copy Markdown
ContributorAuthor

I rebased this on master and implemented the changes @zejn recommended. Ed25519 support is now optional. I'm not super happy with how I had to workaround the circular imports issue with jose.constants, but it works.

This is ready for review.

@pohutukawa

Copy link
Copy Markdown

@mpdavis, there's been a lot of work recently gone into sorting out crypto backend implementation details. Does this PR stand a chance of getting merged somewhere soonish?

@blag

blag commented Jan 4, 2019

Copy link
Copy Markdown
ContributorAuthor

@zejn and/or @mpdavis: Please review.

@Mischala

Copy link
Copy Markdown

Any idea when this is likely to go in?
Would be very useful for a project i am working on.
Thanks!

@blag

blag commented Mar 29, 2019

Copy link
Copy Markdown
ContributorAuthor

@zejn and @mpdavis - I have emailed both of you offering my assistance in maintaining this project. If you do not respond either privately via email or here by the end of next week, I will be forking this project and publishing my fork to PyPI.

Thank you both for your work so far!

@mpdavis

Copy link
Copy Markdown
Owner

I think it is valid to be frustrated with my level of attention.

It makes sense to me to simply accept the offer of help instead of continuing trying to find free time to work on it, only to continue pushing it off.

I have sent invites to both @zejn and @blag to collaborate on the project.

I do appreciate the offers of help, and I apologize for the frustrating lack of communication on my part.

@pohutukawa

Copy link
Copy Markdown

It would be awesome if development could pick up (again) for this. Especially as it's called python-jose, but basically just does JWS/JWT. I'd like to see if I can contribute a starting set of JWE to this ensemble as well (based on my private/limited stand-alone implementation).

@zejn

zejn commented Apr 2, 2019

Copy link
Copy Markdown
Collaborator

This looks ok to me.

@blag

blag commented Apr 3, 2019

Copy link
Copy Markdown
ContributorAuthor

In light of pyca/pynacl#507, I'm going to remove our dependence on encoders. I'll push a few more commits to this PR, get somebody to review the additional commits, then merge it.

@blag
blagforce-pushed the ed25519 branch 2 times, most recently from d386b77 to 867f8fbCompareApril 3, 2019 09:38
@blag

blag commented Apr 3, 2019

Copy link
Copy Markdown
ContributorAuthor

This is again ready for review. None of my previous commits were changed, only 867f8fb and 9bcae9e were added.

@blag

blag commented Apr 6, 2019

Copy link
Copy Markdown
ContributorAuthor

@mpdavis Thank you for adding me as a collaborator, I very much appreciate it. I've had to make the call to stop maintaining other Python packages, so I understand how weighty of a decision it can be.

@zejn Sorry to string you along, but do you mind reviewing this one last time? I removed the reliance on PyNaCl's decoders since they are being removed in pyca/pynacl#507.

Comment threadjose/constants.py
@zejnzejn mentioned this pull request Apr 8, 2019
@blag

blag commented Dec 5, 2019

Copy link
Copy Markdown
ContributorAuthor

As per this conversation, I'm closing this PR.

Sign up for freeto 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.

7 participants

@blag@codecov-io@pohutukawa@mpdavis@zejn@Mischala@mattsb42-aws