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

Remove pycrypto/dome dependency on python-rsa #121

Merged
merged 17 commits into from Apr 7, 2019

Conversation

mattsb42-aws
Copy link
Contributor

This removes the cross-dependency of the pycrypto/dome backend on the python-rsa backend by moving ASN1 parsing to translate between PKCS1 and PKCS8 to a separate module that is now used by both pycrypto/dome and python-rsa backends.

This makes pyasn1 a direct dependency of the pycrypto/dome backend (previously transient through python-rsa), but removes its dependency on python-rsa.

CI also now tests the pycryto/dome backends after uninstalling python-rsa to make sure that this dependency is actually severed.

@codecov
Copy link

codecov bot commented Dec 28, 2018

Codecov Report

Merging #121 into backend-explicit-tests will decrease coverage by 0.32%.
The diff coverage is 91.22%.

Impacted file tree graph

@@                    Coverage Diff                     @@
##           backend-explicit-tests     #121      +/-   ##
==========================================================
- Coverage                   96.86%   96.53%   -0.33%     
==========================================================
  Files                          13       14       +1     
  Lines                        1051     1068      +17     
==========================================================
+ Hits                         1018     1031      +13     
- Misses                         33       37       +4
Impacted Files Coverage Δ
jose/backends/rsa_backend.py 96.5% <100%> (-0.59%) ⬇️
jose/backends/_asn1.py 89.47% <89.47%> (ø)
jose/backends/pycrypto_backend.py 95.76% <92.3%> (+0.3%) ⬆️

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 c28f15a...b169959. Read the comment docs.

@mattsb42-aws
Copy link
Contributor Author

Checking in, @mpdavis. Looking to wrap this up; is there anything you need on my end for this?

tox.ini Outdated
only: pip uninstall -y ecdsa rsa
# Remove just the python-rsa backend
nosra: pip uninstall -y rsa
Copy link
Collaborator

Choose a reason for hiding this comment

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

It says nosra here when it should be norsa.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Doh. So it does. Thanks for the catch; looks like I also missed a few points where the pycrypto path is importing rsa.

@mattsb42-aws
Copy link
Contributor Author

Ok, this is weird...digging through this, it looks like the Crypto.IO module made it into the pycrypto code[1] but it's not resolving in the 2.6.1 build on PyPI[2]. I verified this in stand-alone virtualenv installs for both 2.7[3] and 3.7[4].

This actually means that the pycrypto backend can never work by itself as written (though pycryptodome can). The backend switching was masking this before.

Looking through the pycrypto_backend module, it looks like the only thing the Crypto.IO module is being used for is to do a basic DER-to-PEM encoding. This is trivial to recreate[5], but I'm conflicted as to whether it would be better to do this or turn the pycrypto extras install into the legacy backend (ie: ecdsa and rsa).

Aside from a desire to kill off a library that hasn't been supported for six years (pycrypto), I'm inclined to think that doing this redirect is a better option anyway, because it is less likely to break consumers. Since the pycrypto_backend module cannot load with pycrypto, any consumers who are currently using that extras install are unknowingly already using the legacy backend anyway. Making the pycrypto extras install a redirect to the legacy backend is simply making that behavior intentional. If, instead, I add a DER-to-PEM encoding function to the pycrypto_backend module, consumers of the pycrypto extras install will suddenly actually be using pycrypto for their crypto backend.

@mpdavis @zejn Any opinion on which direction this should take?

[1] https://github.com/dlitz/pycrypto/tree/master/lib/Crypto/IO
[2] https://www.dlitz.net/software/pycrypto/api/current/
[3]

Python 2.7

186590df9307:python-jose bullocm$ virtualenv ~/tmp/why -p python2.7
Running virtualenv with interpreter /usr/bin/python2.7
New python executable in /Users/bullocm/tmp/why/bin/python
Installing setuptools, pip, wheel...done.
186590df9307:python-jose bullocm$ source ~/tmp/why/bin/activate
(why) 186590df9307:python-jose bullocm$ pip install pycrypto
DEPRECATION: Python 2.7 will reach the end of its life on January 1st, 2020. Please upgrade your Python as Python 2.7 won't be maintained after that date. A future version of pip will drop support for Python 2.7.
Collecting pycrypto
Installing collected packages: pycrypto
Successfully installed pycrypto-2.6.1
(why) 186590df9307:python-jose bullocm$ python
Python 2.7.10 (default, Feb  7 2017, 00:08:15) 
[GCC 4.2.1 Compatible Apple LLVM 8.0.0 (clang-800.0.34)] on darwin
Type "help", "copyright", "credits" or "license" for more information.
>>> import Crypto
>>> import Crypto.IO
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
ImportError: No module named IO

[4]
Python 3.7

186590df9307:python-jose bullocm$ virtualenv ~/tmp/why -p python3.7
Running virtualenv with interpreter /usr/local/bin/python3.7
Using base prefix '/usr/local/Cellar/python/3.7.2_2/Frameworks/Python.framework/Versions/3.7'
/usr/local/lib/python3.7/site-packages/virtualenv.py:1041: DeprecationWarning: the imp module is deprecated in favour of importlib; see the module's documentation for alternative uses
  import imp
New python executable in /Users/bullocm/tmp/why/bin/python3.7
Also creating executable in /Users/bullocm/tmp/why/bin/python
Installing setuptools, pip, wheel...done.
186590df9307:python-jose bullocm$ source ~/tmp/why/bin/activate
(why) 186590df9307:python-jose bullocm$ pip install pycrypto
Collecting pycrypto
Installing collected packages: pycrypto
Successfully installed pycrypto-2.6.1
(why) 186590df9307:python-jose bullocm$ python
Python 3.7.2 (default, Feb 12 2019, 08:16:11) 
[Clang 9.0.0 (clang-900.0.39.2)] on darwin
Type "help", "copyright", "credits" or "license" for more information.
im>>> import Crypto
>>> import Crypto.IO
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
ModuleNotFoundError: No module named 'Crypto.IO'

[5] [part 1](https://github.com/Legrandin/pycryptodome/blob/master/lib/Crypto/IO/PEM.py#L74) [part 2](https://github.com/Legrandin/pycryptodome/blob/master/lib/Crypto/IO/PEM.py#L88-L94)

@mattsb42-aws
Copy link
Contributor Author

mattsb42-aws commented Apr 2, 2019

...nvm, looks like I added that Crypto.IO dependency in this PR. :/

DER-to-PEM function it is!

@mattsb42-aws
Copy link
Contributor Author

mattsb42-aws commented Apr 2, 2019

Added Travis testing for pypy-5.7.1 to make sure we're covering both sides of the pypy cryptography issue. While I was in there, I went ahead and added CPython 3.6 and 3.7 too.

@mattsb42-aws
Copy link
Contributor Author

@zejn @mpdavis Probably better for a different PR, but I see the flake8 tests commented out everywhere. The codebase is flake8 compliant; is there a reason to not have those turned on?

@zejn
Copy link
Collaborator

zejn commented Apr 3, 2019

If flake8 works then I agree it makes sense to actually run it.

@mattsb42-aws
Copy link
Contributor Author

Ok, I went ahead and enabled flake8 and extended it to check setup.py as well as the library source.

@zejn
Copy link
Collaborator

zejn commented Apr 5, 2019

This looks ok.

Just to confirm, the "compatibility" tox environment installs every extra as an emulation of previous, non-split backend tests, right?

@mattsb42-aws
Copy link
Contributor Author

Just to confirm, the "compatibility" tox environment installs every extra as an emulation of previous, non-split backend tests, right?

Correct. Those are to verify that a) the correct backend is selected when all are present, and b) that the various backends do not conflict with each other.

@zejn zejn merged commit dd5d551 into mpdavis:backend-explicit-tests Apr 7, 2019
@mattsb42-aws mattsb42-aws deleted the asn-rsa branch April 7, 2019 17:04
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.

None yet

2 participants