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

prefer ecdsa host key types #911

Closed
wants to merge 2 commits into from
Closed

Conversation

@ploxiln
Copy link
Contributor

@ploxiln ploxiln commented Feb 23, 2017

fixes #794 and #900

second part of an alternative set of PRs to #899

@coveralls
Copy link

@coveralls coveralls commented Feb 23, 2017

Coverage Status

Coverage decreased (-0.05%) to 74.227% when pulling 1186105 on ploxiln:ecdsa_more_support into 5061ee6 on paramiko:master.

@ploxiln
Copy link
Contributor Author

@ploxiln ploxiln commented Jun 2, 2017

rebased, conflicts resolved

@ploxiln ploxiln force-pushed the ploxiln:ecdsa_more_support branch from df024e5 to 25771e9 Jun 5, 2017
@ploxiln
Copy link
Contributor Author

@ploxiln ploxiln commented Jun 5, 2017

I decided to change the style of this, and use the literal key type names.

There's only the three ECDSA key types, and that set won't change any time soon, so the balance of simplicity/robustness in this case favors just writing them out, in my humble opinion. I haven't seen an opinion either way from the maintainer, and I would be happy to change the style back if that ends up being preferred.

For reference, here's the previous version of this PR, which I thought was uglier and not really justified by the theoretical robustness:

--- a/paramiko/transport.py
+++ b/paramiko/transport.py
@@ -122,10 +122,10 @@ class Transport (threading.Thread, ClosingContextManager):
         'hmac-md5-96',
         'hmac-sha1',
     )
-    _preferred_keys = (
+    _preferred_keys = tuple(ECDSAKey.supported_key_format_identifiers()) + (
         'ssh-rsa',
         'ssh-dss',
-    ) + tuple(ECDSAKey.supported_key_format_identifiers())
+    )
     _preferred_kex = (
         'diffie-hellman-group1-sha1',
         'diffie-hellman-group14-sha1',
@@ -210,8 +210,11 @@ class Transport (threading.Thread, ClosingContextManager):
     _key_info = {
         'ssh-rsa': RSAKey,
         'ssh-dss': DSSKey,
-        'ecdsa-sha2-nistp256': ECDSAKey,
     }
+    _key_info.update(
+        (name, ECDSAKey)
+        for name in ECDSAKey.supported_key_format_identifiers()
+    )
 
     _kex_info = {
         'diffie-hellman-group1-sha1': KexGroup1,

You can see the new version by looking at this PR's Files Changed

@ploxiln ploxiln changed the title more ecdsa host key support prefer ecdsa host key types Jun 5, 2017
@ploxiln ploxiln force-pushed the ploxiln:ecdsa_more_support branch from 25771e9 to b4d97b3 Jun 5, 2017
@ploxiln
Copy link
Contributor Author

@ploxiln ploxiln commented Jun 5, 2017

... and then I split out half of this PR to #981 because it really is a bug fix that should be backported

@bitprophet
Copy link
Member

@bitprophet bitprophet commented Jun 6, 2017

Had a second thought about "won't this bite anyone currently relying on the selection of the older key types, and thus shouldn't this be a 3.0 change", but that's more of a worry for user keys and not host keys, I think.

@ploxiln ploxiln force-pushed the ploxiln:ecdsa_more_support branch from b4d97b3 to 674fd06 Jun 6, 2017
@ploxiln
Copy link
Contributor Author

@ploxiln ploxiln commented Jun 6, 2017

resolved conflicts, put ecdsa keys even in front of ed25519, because this is the order openssh on macOS seems to use:

debug2: kex_parse_kexinit: ecdsa-sha2-nistp256-cert-v01@openssh.com,ecdsa-sha2-nistp384-cert-v01@openssh.com,ecdsa-sha2-nistp521-cert-v01@openssh.com,ssh-ed25519-cert-v01@openssh.com,ssh-rsa-cert-v01@openssh.com,ssh-dss-cert-v01@openssh.com,ssh-rsa-cert-v00@openssh.com,ssh-dss-cert-v00@openssh.com,ecdsa-sha2-nistp256,ecdsa-sha2-nistp384,ecdsa-sha2-nistp521,ssh-ed25519,ssh-rsa,ssh-dss

EDIT: preference of more recent openssh on ubuntu-16.04 is similar:

debug2: host key algorithms: ecdsa-sha2-nistp256-cert-v01@openssh.com,ecdsa-sha2-nistp384-cert-v01@openssh.com,ecdsa-sha2-nistp521-cert-v01@openssh.com,ssh-ed25519-cert-v01@openssh.com,ssh-rsa-cert-v01@openssh.com,ecdsa-sha2-nistp256,ecdsa-sha2-nistp384,ecdsa-sha2-nistp521,ssh-ed25519,rsa-sha2-512,rsa-sha2-256,ssh-rsa

@bitprophet
Copy link
Member

@bitprophet bitprophet commented Jun 6, 2017

Seems reasonable. thanks!

@ploxiln
Copy link
Contributor Author

@ploxiln ploxiln commented Jun 6, 2017

re: selection of keys: that's what #910 is about, often the wrong host key is selected ...

@ploxiln
Copy link
Contributor Author

@ploxiln ploxiln commented Jun 6, 2017

... to clarify: and then after the wrong host key is selected, connection is silently allowed to continue under a MissingHostKeyPolicy variant which doesn't save. This happens for fabric and I think ansible as well

@bitprophet
Copy link
Member

@bitprophet bitprophet commented Jun 6, 2017

Seems #981 also needs the ed25519 related change, I am merging that now so I will take care of it.

Sounds like #910 is additive on top of these changes so I'll get to it when I get to it (it is open...I am trying to see how much of the 2.2 milestone I can chow down on for an actual 2.2 tonight)

@bitprophet
Copy link
Member

@bitprophet bitprophet commented Jun 6, 2017

Mildly amusing thought, shouldn't OpenSSH (and us) be preferring the larger-key-size variants of these algorithms over the smaller-key-size ones? (I.e. ecdsa-sha2-nistp521,ecdsa-sha2-nistp384,ecdsa-sha2-nistp256, so that the largest one supported by the host wins out.)

I kinda trust OpenSSH knows what it's doing so as always "do what they do" seems fine, but it made me think. Also presumably there's a cost to using larger keys.

@ploxiln
Copy link
Contributor Author

@ploxiln ploxiln commented Jun 6, 2017

The reasoning might be that the larger key sizes are needlessly/wastefully large for today, but available in case of need in the future.

@bitprophet
Copy link
Member

@bitprophet bitprophet commented Jun 6, 2017

Honestly, if this is backwards compatible & also just not very instability-causing, we might as well roll it back to 2.0 and treat it as an outright bugfix. Otherwise feels a little odd putting this in 2.2 but not 2.0.x/2.1.x. Can always revert it in those lines if somebody gets megafucked somehow.

@ploxiln
Copy link
Contributor Author

@ploxiln ploxiln commented Jun 6, 2017

Agree it should probably be in 2.0 as well, but the conflict resolution when up-merging to 2.1 will be a bit tricky. I do have a branch with this change applied to 2.0 already, I'll push it up ...

@ploxiln
Copy link
Contributor Author

@ploxiln ploxiln commented Jun 6, 2017

oh nvm I was thinking about #910 again

The openssh client prefers ecdsa server host keys.
@ploxiln ploxiln force-pushed the ploxiln:ecdsa_more_support branch from 674fd06 to bace976 Jun 6, 2017
@ploxiln ploxiln changed the base branch from master to 2.0 Jun 6, 2017
@ploxiln
Copy link
Contributor Author

@ploxiln ploxiln commented Jun 6, 2017

rebased on 2.0 branch

@bitprophet
Copy link
Member

@bitprophet bitprophet commented Jun 6, 2017

Ugh I thought I said I was in the middle of doing that, but, I did not say that. Sorry! Got hung up pushing it because of a stupid docs related rabbit hole.

@bitprophet bitprophet closed this in 691f619 Jun 6, 2017
@bitprophet
Copy link
Member

@bitprophet bitprophet commented Jun 6, 2017

Pushed now, includes a newer changelog line. Thanks again.

@ploxiln
Copy link
Contributor Author

@ploxiln ploxiln commented Jun 6, 2017

no problem, thank you :)

@ploxiln ploxiln deleted the ploxiln:ecdsa_more_support branch Sep 13, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants