fix ecdsa key generation #335

Merged
merged 1 commit into from Sep 6, 2014

Projects

None yet

4 participants

@solarw
solarw commented May 26, 2014

i sent pull request before.
this is better one

fix ecdsa key generation for current paramiko master

@coveralls

Coverage Status

Coverage decreased (-0.02%) when pulling 8455a8a on solarw:ecdsa_key_fix into e811e71 on paramiko:master.

@bitprophet
Member

This looks like a significant change (despite its size) - as far as I can tell folks have been using ECDSA support since it was implemented, and this feels like it could break that. Can you provide a concrete test case (real test, or even just an explanation of how to manually test) proving that the code is broken without this change? Thanks!

@lndbrg lndbrg commented on the diff Aug 12, 2014
paramiko/ecdsakey.py
@@ -24,7 +24,6 @@
from hashlib import sha256
from ecdsa import SigningKey, VerifyingKey, der, curves
-from ecdsa.test_pyecdsa import ECDSA
@lndbrg
lndbrg Aug 12, 2014 Contributor

Yes, this is good. I can't find the generate method on the ECDSA test class anymore. I think this should go in as a bugfix.

@bitprophet
Member

@lndbrg can you source the change being made here? Is this something that is breaking with newer ecdsa versions or something? Otherwise it's unclear to me why this isn't breaking code that used to work fine.

@bitprophet
Member

@bitprophet hello self, I just looked in detail and the line in question is within ECDSAKey.generate which Paramiko never appears to call itself, and which is also probably not used widely by third parties (given its young age and the infrequency of generating new keys among common Paramiko use cases.)

So yea, I'm on board with this now. Still need to test it out (& probably add tests - when I was grepping I saw tests of the other key types' generate methods but not this one) unless @lndbrg did already.

@bitprophet bitprophet added this to the 1.15 milestone Sep 5, 2014
@bitprophet bitprophet merged commit 8455a8a into paramiko:master Sep 6, 2014

1 check passed

continuous-integration/travis-ci The Travis CI build passed
Details
@bitprophet bitprophet added a commit that referenced this pull request Sep 6, 2014
@bitprophet bitprophet Changelog re #335 286b5fb
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment