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

Backport a version-agnostic form of crypto 1.5 compatibility #1292

Closed
bitprophet opened this issue Sep 18, 2018 · 6 comments
Closed

Backport a version-agnostic form of crypto 1.5 compatibility #1292

bitprophet opened this issue Sep 18, 2018 · 6 comments
Labels

Comments

@bitprophet
Copy link
Member

@bitprophet bitprophet commented Sep 18, 2018

Increasingly vexed by the enormous reams of warnings spat out by Paramiko <2.3 during test runs, caused by a lack of #979.

However, it's not nice to backport that ticket itself (because then dependencies will change in tertiary releases - I'm more flexible these days but that's beyond the pale) so I want to see if it's feasible to rewrite it for 2.0+ in a "if X else Y" fashion.

Ought to be straightforward? Famous last words...

Remember to make sure that the merge from 2.2 to 2.3 uses -s ours or similar, so that 2.3+ goes back to assuming what it's able.

@bitprophet bitprophet added the Support label Sep 18, 2018
@bitprophet
Copy link
Member Author

@bitprophet bitprophet commented Sep 18, 2018

Belatedly realized a side effect of this is that 2.3+ could ostensibly relax its version requirement for Cryptography if we preserve the either-or code during the merge.

However that cat's out of the bag - users of Paramiko 2.3+ are already going to be on Cryptography 1.5+ - so it doesn't really seem worthwhile. And esp re: crypto libraries we want to be pushing users to get used to updating periodically instead of staying on old versions, when we have a choice...

@bitprophet
Copy link
Member Author

@bitprophet bitprophet commented Sep 18, 2018

Updating travis-ci at this point too so it tests some older cryptography versions (for 2.0-2.2, crypto 1.1 and 1.5 as well as implicit latest; then when I merge to 2.3, I should preserve some of this so that it tests 1.5 and latest only.)

@bitprophet
Copy link
Member Author

@bitprophet bitprophet commented Sep 18, 2018

FWIW I am unable to even return to older cryptography versions on macOS 10.12, so I'm basically relying on Travis here (tho I can test on a container locally if I gotta).

Also of note, I'm very slightly modifying the logic in the code, pulling the setup of the signatures/etc inside the try/except/else. Otherwise, the test for whether the keys have the newer API is split up in two places and it just looks messy/is hard to follow. This should be safe because the try/except/else only catches InvalidSignature; so any exceptions within the verifier/signature/etc setup should still get raised as they were previously by being outside the block.

bitprophet added a commit that referenced this issue Sep 18, 2018
bitprophet added a commit that referenced this issue Sep 18, 2018
Rest of merge was resolved with '-s ours' to keep
those changes back on the old branches only.
bitprophet added a commit that referenced this issue Sep 18, 2018
@bitprophet
Copy link
Member Author

@bitprophet bitprophet commented Sep 18, 2018

Should be all done, including making sure not to nuke the improved Travis "test some older Cryptos" additions (which...I almost nuked in the merge. heh.)

@bitprophet bitprophet closed this Sep 18, 2018
@twirrim
Copy link

@twirrim twirrim commented Apr 18, 2019

Is there any rough ETA on this getting released? So far as I can see, these changes never landed in master, nor are on the 2.4.2 tag? https://github.com/paramiko/paramiko/commits/2.4.2/paramiko/dsskey.py, for example, lacks 35b1f57, and I don't see related changes in master either? https://github.com/paramiko/paramiko/blob/master/paramiko/dsskey.py

The underlying warning messages are polluting the output from some of our tools :)

@bitprophet
Copy link
Member Author

@bitprophet bitprophet commented May 31, 2019

@twirrim Are you sure you're seeing this exact issue (and not something like #1379 which is open but I'll be merging it RSN)? As near as I can tell the changes from this ticket made it into 2.0 and on up through master; in fact master even did away with the if/else part of this and just uses key.sign, there's no signer at all.

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

Successfully merging a pull request may close this issue.

None yet
2 participants
You can’t perform that action at this time.