Replace PyCrypto with cryptography. #46
Conversation
|
Unfortunately, there are many people that rely on the PyCrypto support in environments where cryptography is not supported, such as Google App Engine. That said, with the recent work around custom algorithms would make it pretty easy to add a implementation based on cryptography that ships with python-jose that could easily be registered and used. I would even be open to conversations about setting it as the default, but PyCrypto would still need to be supported. |
This is really Google's narrative; that there is "investment" and "community support" for these old libraries. I understand that from your perspective, PyCrypto support is necessary, but the only reason why is Google, frankly. And I'm not suggesting that this is an unreasonable perspective to have, considering that GAE standard is considerably cheaper than GAE flexible. What I think will be satisfactory is provide a backend that minimally emulates cryptography using pycrypto. I'm currently down to 3 test failures on pypy only (which tests the pycrypto backend), so that should be completed today. |
Codecov Report
@@ Coverage Diff @@
## master #46 +/- ##
=========================================
+ Coverage 95.57% 95.7% +0.12%
=========================================
Files 7 10 +3
Lines 542 628 +86
=========================================
+ Hits 518 601 +83
- Misses 24 27 +3
Continue to review full report at Codecov.
|
|
Should be done now. With patch applied, requirement is cryptography except on pypy, and runtime behavior is that fallback to pycrypto will occur if cryptography cannot be imported. |
ghost
changed the title
jose/jwk.py
Outdated
| sig, | ||
| msg, | ||
| padding.PKCS1v15(), | ||
| self.hash_alg(), |
There was a problem hiding this comment.
One thing to note is that I'm not completely sure what happens when the hash_alg is not one of the SHA* hashes. Maybe some feedback on that?
|
I'm not of fan of trying to fake the cryptography api with PyCrypto. There is no requirement that each hashing algorithm needs to be implemented in a single Key class. I would make a new class that implements the Key interface and is implemented with the cryptography library. That key class could then be registered over top of the PyCrypto class (or set as the default, allowing the PyCrypto class to overwrite the cryptography version). Doing so would keep all of that code separate and much easier to understand. |
Much of the code is shared between classes, which means that we would have code duplication with that option.
Not necessarily. Cryptography is the de facto cryptographic library in Python which means its functions are extensively documented. PyCrypto is significantly less documented which means that its functions are more unclear. I can add docstrings to the compat code to ease maintainability. |
|
We have a system in place for swapping out key implementations. I don't see how ignoring that system and doing it in a one-off manner is going to be more maintainable, despite some code duplication. Code that does what the maintainers expect it to is important. |
|
Okay, I'll fold. |
|
Alright, should be done now. I had some trouble with tox and some ancient testing requirements, so I took it out. |
| @@ -0,0 +1,112 @@ | |||
| import six | |||
|
|
|||
| from cryptography.hazmat.backends import default_backend | |||
There was a problem hiding this comment.
Are all the hazmat imports necessary because cryptography doesn't (yet?) expose these algorithms in their 'public' layer?
|
👍 I strongly endorse getting away from insecure, unmaintained |
|
Is it worth putting |
|
@bjmc To be perfectly honest, I am no longer going to maintain this PR because I have convinced other projects to remove python-jose as a dependency. Feel free to fork this if you wish, though. |
|
Fixed in #49 |
I know the cryptodome route is easier, but cryptography is the way to go for the long run because it is supported by the python software foundation. Feel free to give feedback and suggestions!