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

Move to cryptography 2.5 and stop using deprecated APIs. #1379

Merged
merged 4 commits into from May 31, 2019

Conversation

@rectalogic
Copy link
Contributor

@rectalogic rectalogic commented Feb 6, 2019

Fixes #1369

.travis.yml Outdated Show resolved Hide resolved
paramiko/ecdsakey.py Outdated Show resolved Hide resolved
paramiko/kex_ecdh_nist.py Show resolved Hide resolved
paramiko/kex_ecdh_nist.py Outdated Show resolved Hide resolved
paramiko/kex_ecdh_nist.py Show resolved Hide resolved
paramiko/kex_ecdh_nist.py Outdated Show resolved Hide resolved
paramiko/ecdsakey.py Outdated Show resolved Hide resolved
alex
alex approved these changes Feb 9, 2019
.travis.yml Show resolved Hide resolved
@bitprophet
Copy link
Member

@bitprophet bitprophet commented Feb 9, 2019

Thanks for this (& to @alex for the review. Also, huh, TIL people who aren't set up as explicit collaborators can still file a review?! whoa!).

I am playing catchup right now but now that I've seen this it'll be on my mind and I'll make sure we solidify it for next release cycle. (Also, milestones.)

At a glance my main worry is the usual dependency upgrade problem of users happily chugging away with some pinned (in any sense) Cryptography >=1.5,<2.5, who blindly upgrade Paramiko and start getting pip errors. Obviously, hard to gauge who has it worse, them or all the folks reporting on the base ticket who blindly upgraded/installed latest Crypto and got runtime warnings instead.

(Arguably, runtime deprecation warnings are not worse than hard pip install failures, but OTOH, in this day & age anybody pinning their sub-dependencies is forced to deal with this kind of situation frequently, no matter what us maintainers do to try and shield them.)

Copy link

@nzjc nzjc left a comment

I don't know much about anything, but I manually threw in the changes shown here into my 2.4.2 version of Paramiko and the dreaded warning messages went away. Beautiful work.

jaywcarman referenced this issue in jaywcarman/power-up Feb 19, 2019
The 'paramiko' python module depends on the 'cryptography' module. There
is a known 'CryptographyDeprecationWarning' problem (see
paramiko/paramiko#1369) that can be resolved
by installing 'cryptography==2.4.2'.
@vkobel
Copy link

@vkobel vkobel commented Feb 27, 2019

Until this gets merged/published, I created a patch for the regular package here: https://gist.github.com/vkobel/52d795920aacd74afb4db15a89c39b8b

@alex-aire
Copy link

@alex-aire alex-aire commented Mar 11, 2019

What is holding up this PR?

@MrMino
Copy link
Contributor

@MrMino MrMino commented Mar 11, 2019

@alex-aire I wouldn't be surprised if it was a result of what's in @bitprophet's github status.

@AInteriorB
Copy link

@AInteriorB AInteriorB commented Apr 12, 2019

Nothing new on this? Would be great to get rid of these warnings fired by duplicity in Debian Buster repo.

@iTaybb
Copy link

@iTaybb iTaybb commented Apr 20, 2019

Would like to see this merged.

ploxiln added a commit to ploxiln/paramiko-ng that referenced this issue May 1, 2019
ploxiln added a commit to ploxiln/paramiko-ng that referenced this issue May 1, 2019
@kabirbaidhya
Copy link

@kabirbaidhya kabirbaidhya commented May 9, 2019

Is this PR going to be merged any sooner? Thanks.

@ploxiln
Copy link
Contributor

@ploxiln ploxiln commented May 9, 2019

btw, I have a modification of this which adds back compatibility with cryptography < 2.5 here: ploxiln@17f2e17

(and made a release of my fork with some other stuff as well: https://github.com/ploxiln/paramiko-ng/releases/tag/2.5.0)

@AInteriorB
Copy link

@AInteriorB AInteriorB commented May 20, 2019

Until this gets merged/published, I created a patch for the regular package here: https://gist.github.com/vkobel/52d795920aacd74afb4db15a89c39b8b

I now went this way :-(

@DannyCork
Copy link

@DannyCork DannyCork commented May 22, 2019

let's get this merged @rectalogic @bitprophet @vkobel @alex-aire @MrMino

@ssbarnea
Copy link

@ssbarnea ssbarnea commented May 23, 2019

Seems like something to include in next release: #1386

@riptusk331
Copy link

@riptusk331 riptusk331 commented May 30, 2019

how many reviewers have to approve for it to merge?

@iTaybb
Copy link

@iTaybb iTaybb commented May 30, 2019

@bitprophet
Copy link
Member

@bitprophet bitprophet commented May 31, 2019

Oh hi. I'm back now. Not sure I appreciate the snark though :P heaven forbid we have lives! or upend those lives in pursuit of a job that lets us support these projects. Granted, I wasn't expecting it to take 6 @#$%! months to get back to vaguely normal...


My todo for this (mostly rubber ducking myself with too many words):

  • Confirm again that I can replicate it locally
    • Yup, as soon as I nuke and recreate my virtualenv and get crypto >2.5 (2.7 is latest rn), pytest surfaces a brazillion warnings of the type reported
  • See if there's a quick easy way to add a test cell for "latest Crypto version" so this is even more visible in the future, though visibility wasn't exactly my problem here...but it's irksome anyways
    • Counterpoint: this style of warning won't break any builds so it doesn't help visibility, though conversely that also means we can continue to test bugfixes for our <2.5 release lines even though they'll have this issue on newer Crypto versions.
  • Decide whether to adapt it into something that works either/or for pre-crypto-2.5 versions (to support folks wanting anything else in Paramiko 2.5+ on an older Cryptography, and/or in order to backport to our own stable lines).
    • Gut says to not do so, and we can always iterate if enough people complain. Especially if the complainers do the work of crafting a safe, sane PR.
    • Note to self: may be wise to make/document an explicit policy around Cryptography dependency upgrades, I'm very sick of having to wonder every single time if "breaking" folks' stable installs w/ dependency updates in minor releases is bad, and/or whether it should mean we cut a major instead. Take a position and stick with it?

Took a quick look at previous Crypto requirement updates:

  • From 2.0, when we first added it, we required Cryptography >=1.1
  • From 2.3, we required Cryptography >=1.5, and updated 2.0-2.2 to be compatible with Cryptography 1.1-1.6 (which IIRC was the latest at time of that Paramiko release).
  • We're now at Paramiko 2.4.x, and will be releasing 2.5.x with this change in it.
  • As above, I'm thinking of NOT extending either-or support back to anything prior to Paramiko 2.5 (meaning anyone on Paramiko <2.5 and Crypto >=2.5 would still get warnings) but if we get interest and patches, that may change.

@bitprophet
Copy link
Member

@bitprophet bitprophet commented May 31, 2019

Re: Crypto test cell: right, that's what that CRYPTO_BEFORE (gonna change that name as its' hella confusing) was for - it meant that we always test:

  • Oldest supported Crypto
  • Latest Crypto indicated by setup.py, at time of test run.

Part of my confusion was, as noted, no tests (on my branches) ran since the start of 2019 (😩) and Crypto 2.5 was not out then.

@bitprophet
Copy link
Member

@bitprophet bitprophet commented May 31, 2019

Yea gonna just keep this in master->2.5 for the time being. Testing/etc now.

@bitprophet bitprophet merged commit e5f47b5 into paramiko:master May 31, 2019
1 check passed
bitprophet added a commit that referenced this issue May 31, 2019
auneri added a commit to auneri/ipyslurm that referenced this issue Jun 3, 2019
@svoop
Copy link

@svoop svoop commented Jun 4, 2019

@bitprophet Thanks a bunch for the merge! As soon as a new release is cut, the package maintainers can up the different distros and get rid of the warnings.

@Rocking80
Copy link

@Rocking80 Rocking80 commented Jun 6, 2019

Anyone know when Paramiko 2.5 will be released?

@ssbarnea
Copy link

@ssbarnea ssbarnea commented Jun 6, 2019

@Rocking80 follow #1446

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

Successfully merging this pull request may close these issues.