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

Switched everything to use cryptography instead of pyCrypto #394

Merged
merged 64 commits into from Apr 25, 2016

Conversation

Projects
None yet
@alex
Contributor

alex commented Sep 15, 2014

Motivation:

  • Adds PyPy support
  • Performance improvement
  • OpenSSL and friends are better audited than PyCrypto
  • Easier windows install flow (Cryptography provides statically linked wheels
    on Windows)

This PR is basically complete on the code side, of course it can always use
more review :-)

Tests all pass locally (tested with PyPy!)

Still needs some docs work, and to figure out how to do this with the version
numbers so people's stuff doesn't suddenly get broken.

@alex

This comment has been minimized.

Show comment
Hide comment
@alex

alex Sep 15, 2014

Contributor

Oh, this also requires cryptography master right now, we need to get a release out before this is usable; that's no problem though.

Contributor

alex commented Sep 15, 2014

Oh, this also requires cryptography master right now, we need to get a release out before this is usable; that's no problem though.

@alex

This comment has been minimized.

Show comment
Hide comment
@alex

alex Sep 15, 2014

Contributor

Oh -- this also adds pyasn1 as a dependency, it's a pretty small thing, and could be used to replace some of the existing BER stuff in the future. (We can also remove the ecdsa requirement and just use cryptography as well)

Contributor

alex commented Sep 15, 2014

Oh -- this also adds pyasn1 as a dependency, it's a pretty small thing, and could be used to replace some of the existing BER stuff in the future. (We can also remove the ecdsa requirement and just use cryptography as well)

Show outdated Hide outdated paramiko/rsakey.py Outdated
Show outdated Hide outdated tests/test_client.py Outdated
@lndbrg

This comment has been minimized.

Show comment
Hide comment
@lndbrg

lndbrg Sep 16, 2014

Contributor

Some tests seems to be failing on CPython. Is that due to requiring master of cryptography?

If you'd like to add pypy and pypy3 to the .travis.yml that would be good, so we'll get those tested as well.

Contributor

lndbrg commented Sep 16, 2014

Some tests seems to be failing on CPython. Is that due to requiring master of cryptography?

If you'd like to add pypy and pypy3 to the .travis.yml that would be good, so we'll get those tested as well.

@lndbrg

This comment has been minimized.

Show comment
Hide comment
@lndbrg

lndbrg Sep 16, 2014

Contributor

Also, is there any significant speed difference going from PyCrypto to cryptography?

Contributor

lndbrg commented Sep 16, 2014

Also, is there any significant speed difference going from PyCrypto to cryptography?

@lndbrg

This comment has been minimized.

Show comment
Hide comment
@lndbrg

lndbrg Sep 16, 2014

Contributor

Most of it looks good though. :) I will look at it a couple of times more to see if i discover something else.

Contributor

lndbrg commented Sep 16, 2014

Most of it looks good though. :) I will look at it a couple of times more to see if i discover something else.

@reaperhulk

This comment has been minimized.

Show comment
Hide comment
@reaperhulk

reaperhulk Sep 16, 2014

Contributor

@lndbrg I don't believe any of the core developers have conducted a serious benchmark of pyca/cryptography against pycrypto, but I'd expect them to be broadly similar.

The CPython failures are due to a function required for this PR not being present in the currently released version of pyca/cryptography (0.5.4). We'll be releasing a new version of the library soon to correct this (and add some new features).

Contributor

reaperhulk commented Sep 16, 2014

@lndbrg I don't believe any of the core developers have conducted a serious benchmark of pyca/cryptography against pycrypto, but I'd expect them to be broadly similar.

The CPython failures are due to a function required for this PR not being present in the currently released version of pyca/cryptography (0.5.4). We'll be releasing a new version of the library soon to correct this (and add some new features).

@lndbrg

This comment has been minimized.

Show comment
Hide comment
@lndbrg

lndbrg Sep 16, 2014

Contributor

Would it be okay to ask you to try to transfer, say a 100M file, one with this branch and one with pycrypto? :)

Contributor

lndbrg commented Sep 16, 2014

Would it be okay to ask you to try to transfer, say a 100M file, one with this branch and one with pycrypto? :)

@alex

This comment has been minimized.

Show comment
Hide comment
@alex

alex Sep 16, 2014

Contributor

I haven't benchmarked it with paramiko, but the last time I benchmarked PyCrypto vs Cryptography (this was for DKIM, which is all RSA) it was many many times faster, 6x IIRC.

If you can show me how to benchmark transfering a 100MB file with paramiko, I'm happy to do so :-)

Contributor

alex commented Sep 16, 2014

I haven't benchmarked it with paramiko, but the last time I benchmarked PyCrypto vs Cryptography (this was for DKIM, which is all RSA) it was many many times faster, 6x IIRC.

If you can show me how to benchmark transfering a 100MB file with paramiko, I'm happy to do so :-)

@lndbrg

This comment has been minimized.

Show comment
Hide comment
@lndbrg

lndbrg Sep 17, 2014

Contributor

Cool. There is a test called test_sftp_big.py that transfers a 1MB file, running that should probably suffice. :)

Contributor

lndbrg commented Sep 17, 2014

Cool. There is a test called test_sftp_big.py that transfers a 1MB file, running that should probably suffice. :)

@lndbrg

This comment has been minimized.

Show comment
Hide comment
@lndbrg

lndbrg Sep 17, 2014

Contributor

@reaperhulk when the new version is released and we get a green run in travis i'll have another look. 👍

Contributor

lndbrg commented Sep 17, 2014

@reaperhulk when the new version is released and we get a green run in travis i'll have another look. 👍

@lndbrg

This comment has been minimized.

Show comment
Hide comment
@lndbrg

lndbrg Sep 17, 2014

This should probably go into tox.ini as well.

lndbrg commented on .travis.yml in a46ea81 Sep 17, 2014

This should probably go into tox.ini as well.

@bitprophet

This comment has been minimized.

Show comment
Hide comment
@bitprophet

bitprophet Sep 18, 2014

Member

SO EXCITED

Member

bitprophet commented Sep 18, 2014

SO EXCITED

@bitprophet

This comment has been minimized.

Show comment
Hide comment
@bitprophet

bitprophet Sep 18, 2014

Member

Actual comments, without reading the diffs yet:

  • Re: versions: if this only added pure-Python deps, it would be a minor release (eg we added ecdsa support in 1.13 or so) as it adds no "real" dependency - users able to install paramiko beforehand can still install after.
    • However, unless I'm missing something, Cryptography requires OpenSSL & PyCrypto doesn't (PyCrypto only needs regular Python dev headers and I'm reasonably sure those don't themselves require OpenSSL?)
    • If that's accurate I may just say "ok this is now Paramiko 2.0, but there are no API changes, only dependency changes" - perhaps unusual, but honoring semver nonetheless.
  • Re: ecdsa being subsumed, that's fine, and is also more fodder for making it a 2.0 level change (though not a hard requirement).
  • Re: pyasn1, that's already one of the optional requirements for the merged-but-unreleased Kerberos/GSSAPI support, so this just makes it non-optional.
Member

bitprophet commented Sep 18, 2014

Actual comments, without reading the diffs yet:

  • Re: versions: if this only added pure-Python deps, it would be a minor release (eg we added ecdsa support in 1.13 or so) as it adds no "real" dependency - users able to install paramiko beforehand can still install after.
    • However, unless I'm missing something, Cryptography requires OpenSSL & PyCrypto doesn't (PyCrypto only needs regular Python dev headers and I'm reasonably sure those don't themselves require OpenSSL?)
    • If that's accurate I may just say "ok this is now Paramiko 2.0, but there are no API changes, only dependency changes" - perhaps unusual, but honoring semver nonetheless.
  • Re: ecdsa being subsumed, that's fine, and is also more fodder for making it a 2.0 level change (though not a hard requirement).
  • Re: pyasn1, that's already one of the optional requirements for the merged-but-unreleased Kerberos/GSSAPI support, so this just makes it non-optional.
Show outdated Hide outdated paramiko/ecdsakey.py Outdated
Show outdated Hide outdated setup.py Outdated
Merge branch 'master' into switch-to-cryptography
Conflicts:
	paramiko/ecdsakey.py
	tests/test_client.py
Show outdated Hide outdated sites/www/installing.rst Outdated
Show outdated Hide outdated sites/www/installing.rst Outdated
@bitprophet

This comment has been minimized.

Show comment
Hide comment
@bitprophet

bitprophet Sep 19, 2014

Member

Added some line notes (see above), otherwise this looks good to me at a glance. I haven't yet installed master Cryptography to test it out though.

Member

bitprophet commented Sep 19, 2014

Added some line notes (see above), otherwise this looks good to me at a glance. I haven't yet installed master Cryptography to test it out though.

@alex

This comment has been minimized.

Show comment
Hide comment
@alex

alex Sep 19, 2014

Contributor

Pushed a fix for the merge conflict and the docs issue you noted.

On Thu, Sep 18, 2014 at 6:04 PM, Jeff Forcier notifications@github.com
wrote:

Added some line notes (see above), otherwise this looks good to me at a
glance. I haven't yet installed master Cryptography to test it out though.


Reply to this email directly or view it on GitHub
#394 (comment).

"I disapprove of what you say, but I will defend to the death your right to
say it." -- Evelyn Beatrice Hall (summarizing Voltaire)
"The people's good is the highest law." -- Cicero
GPG Key fingerprint: 125F 5C67 DFE9 4084

Contributor

alex commented Sep 19, 2014

Pushed a fix for the merge conflict and the docs issue you noted.

On Thu, Sep 18, 2014 at 6:04 PM, Jeff Forcier notifications@github.com
wrote:

Added some line notes (see above), otherwise this looks good to me at a
glance. I haven't yet installed master Cryptography to test it out though.


Reply to this email directly or view it on GitHub
#394 (comment).

"I disapprove of what you say, but I will defend to the death your right to
say it." -- Evelyn Beatrice Hall (summarizing Voltaire)
"The people's good is the highest law." -- Cicero
GPG Key fingerprint: 125F 5C67 DFE9 4084

@bitprophet bitprophet referenced this pull request Sep 19, 2014

Closed

Support pypy #1189

@bitprophet bitprophet added this to the 2.0 milestone Sep 19, 2014

@bitprophet

This comment has been minimized.

Show comment
Hide comment
@bitprophet

bitprophet Apr 23, 2016

Member

Neglected to tag this on another commit re: docs...after some rebasing and force pushing to my integration branch, here's a SHA-pinned comparison, cc @alex @reaperhulk: alex/paramiko@alex:d1e43c1...paramiko:0ae4d44diff-d3b5c8a3c496304a374ce4a27034e686

Member

bitprophet commented Apr 23, 2016

Neglected to tag this on another commit re: docs...after some rebasing and force pushing to my integration branch, here's a SHA-pinned comparison, cc @alex @reaperhulk: alex/paramiko@alex:d1e43c1...paramiko:0ae4d44diff-d3b5c8a3c496304a374ce4a27034e686

@reaperhulk

This comment has been minimized.

Show comment
Hide comment
@reaperhulk

reaperhulk Apr 23, 2016

Contributor

Mostly looks good. Only comment I have is that "plus development headers for Python, OpenSSL and CFFI" should probably read "plus development headers for Python, OpenSSL and libffi", as libffi is cffi's C dependency.

Contributor

reaperhulk commented Apr 23, 2016

Mostly looks good. Only comment I have is that "plus development headers for Python, OpenSSL and CFFI" should probably read "plus development headers for Python, OpenSSL and libffi", as libffi is cffi's C dependency.

@bitprophet

This comment has been minimized.

Show comment
Hide comment
@bitprophet

bitprophet Apr 23, 2016

Member

Updated, thanks!

Member

bitprophet commented Apr 23, 2016

Updated, thanks!

@bitprophet

This comment has been minimized.

Show comment
Hide comment
@bitprophet

bitprophet Apr 24, 2016

Member

Posted http://bitprophet.org/blog/2016/04/23/paramiko-2.0-is-coming/ so this doesn't catch everyone 100% flat-footed when it comes out :3

I'm off to bang on 1.16.1; some of its tickets may get bumped to 1.17.0; once those are both squared away I am probably going to declare master "2.0" and merge this.

Member

bitprophet commented Apr 24, 2016

Posted http://bitprophet.org/blog/2016/04/23/paramiko-2.0-is-coming/ so this doesn't catch everyone 100% flat-footed when it comes out :3

I'm off to bang on 1.16.1; some of its tickets may get bumped to 1.17.0; once those are both squared away I am probably going to declare master "2.0" and merge this.

@coveralls

This comment has been minimized.

Show comment
Hide comment
@coveralls

coveralls Apr 25, 2016

Coverage Status

Coverage decreased (-0.3%) to 72.326% when pulling 69b995a on alex:switch-to-cryptography into 1cda0eb on paramiko:master.

coveralls commented Apr 25, 2016

Coverage Status

Coverage decreased (-0.3%) to 72.326% when pulling 69b995a on alex:switch-to-cryptography into 1cda0eb on paramiko:master.

@coveralls

This comment has been minimized.

Show comment
Hide comment
@coveralls

coveralls Apr 25, 2016

Coverage Status

Coverage decreased (-0.3%) to 72.339% when pulling 69b995a on alex:switch-to-cryptography into 1cda0eb on paramiko:master.

coveralls commented Apr 25, 2016

Coverage Status

Coverage decreased (-0.3%) to 72.339% when pulling 69b995a on alex:switch-to-cryptography into 1cda0eb on paramiko:master.

@bitprophet bitprophet merged commit 69b995a into paramiko:master Apr 25, 2016

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@alex alex deleted the alex:switch-to-cryptography branch Apr 25, 2016

@alex

This comment has been minimized.

Show comment
Hide comment
@alex

alex Apr 25, 2016

Contributor

\o/

Thanks! Feel free to CC me on any issues should there be follow up necessary.

Contributor

alex commented Apr 25, 2016

\o/

Thanks! Feel free to CC me on any issues should there be follow up necessary.

@bitprophet

This comment has been minimized.

Show comment
Hide comment
@bitprophet

bitprophet Apr 25, 2016

Member

Did initial prep work for release:

  • Branched old master as 1.17 so that can be released (it's tiny, 1.16.1 will be much bigger changelog-wise)
  • Updated my integration branch (which has some of its own changes, as above, mostly docs) with alex's branch and master (been working on bugfixes a lot today). Looks ok
  • Merged that to master and pushed, which is why this is merged now :3

I plan to release 1.16.1, 1.17.0 and 2.0.0 tomorrow if all goes well. Thanks a billion, Alex, you have the patience of a saint.

Member

bitprophet commented Apr 25, 2016

Did initial prep work for release:

  • Branched old master as 1.17 so that can be released (it's tiny, 1.16.1 will be much bigger changelog-wise)
  • Updated my integration branch (which has some of its own changes, as above, mostly docs) with alex's branch and master (been working on bugfixes a lot today). Looks ok
  • Merged that to master and pushed, which is why this is merged now :3

I plan to release 1.16.1, 1.17.0 and 2.0.0 tomorrow if all goes well. Thanks a billion, Alex, you have the patience of a saint.

@bitprophet

This comment has been minimized.

Show comment
Hide comment
@bitprophet

bitprophet Apr 25, 2016

Member

Feel free to CC me on any issues should there be follow up necessary.

You know I will 😆

Member

bitprophet commented Apr 25, 2016

Feel free to CC me on any issues should there be follow up necessary.

You know I will 😆

@drewfisher314

This comment has been minimized.

Show comment
Hide comment
@drewfisher314

drewfisher314 Apr 25, 2016

Outstanding!

On Apr 24, 2016, at 9:21 PM, Jeff Forcier notifications@github.com wrote:

Merged #394.


You are receiving this because you were mentioned.
Reply to this email directly or view it on GitHub

drewfisher314 commented Apr 25, 2016

Outstanding!

On Apr 24, 2016, at 9:21 PM, Jeff Forcier notifications@github.com wrote:

Merged #394.


You are receiving this because you were mentioned.
Reply to this email directly or view it on GitHub

bitprophet added a commit that referenced this pull request Apr 25, 2016

@techtonik

This comment has been minimized.

Show comment
Hide comment
@techtonik

techtonik commented Apr 25, 2016

👍

@Julian

This comment has been minimized.

Show comment
Hide comment
@Julian

Julian Apr 25, 2016

@alex @bitprophet and others, all y'all are great. Thanks!

Julian commented Apr 25, 2016

@alex @bitprophet and others, all y'all are great. Thanks!

@bitprophet

This comment has been minimized.

Show comment
Hide comment
@bitprophet

bitprophet Apr 27, 2016

Member

Quick note, still planning to get this out ASAP, been banging on an update to my changelog library so it can actually support 1.0 -> 2.0 transitions gracefully. Mostly done with that now. Computers.

Member

bitprophet commented Apr 27, 2016

Quick note, still planning to get this out ASAP, been banging on an update to my changelog library so it can actually support 1.0 -> 2.0 transitions gracefully. Mostly done with that now. Computers.

@alex

This comment has been minimized.

Show comment
Hide comment
@alex

alex Apr 27, 2016

Contributor

@bitprophet Sounds good, let me know if you need anything.

Contributor

alex commented Apr 27, 2016

@bitprophet Sounds good, let me know if you need anything.

florianluediger added a commit to instantshare/instantshare that referenced this pull request May 16, 2016

Update installation instructions for Windows.
Paramiko has switched from PyCrypto to Cryptography as backend.
This makes the installation in Windows much easier.
See paramiko/paramiko#394 for more information.

@conradlink conradlink referenced this pull request Jan 26, 2017

Open

Remove pycrypto #819

@fakuivan fakuivan referenced this pull request Oct 26, 2017

Merged

Switch to "cryptography" #6

asfgit pushed a commit to apache/impala that referenced this pull request Aug 24, 2018

IMPALA-7460 part 2: upgrade Paramiko and Fabric in extended test env
Upgrade Paramiko to the latest, 2.4.1. Paramiko drastically changed its
dependencies in Paramiko 2, dropping defunct Pycrypto and using Cryptography
instead.

paramiko/paramiko#637
paramiko/paramiko#394

This change implicitly removes the dependency on Pycrypto.

Also upgrade Fabric to the latest 1.x version, 1.14.0.

Testing:
- This works in my development environment.
- This works in my downstream stress and query gen environments.
- This works when doing a full data load.
- Impala still builds on a variety of OSs.

Change-Id: I0636d8113be449953420e1d5773f63d7c91943e3
Reviewed-on: http://gerrit.cloudera.org:8080/11308
Reviewed-by: Philip Zeyliger <philip@cloudera.com>
Tested-by: Impala Public Jenkins <impala-public-jenkins@cloudera.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment