In KEXINIT packet, put EC algorithms first in list of host key algorithms #642

rodrigc · 2016-12-22T01:39:13Z

http://twistedmatrix.com/trac/ticket/8958

…ore ssh-rsa. This matches behavior with OpenSSH during algorithm negotation.

…y have in the MockServer

codecov-io · 2016-12-22T18:20:16Z

Current coverage is 91.20% (diff: 93.75%)

Merging #642 into trunk will increase coverage by <.01%

@@              trunk       #642   diff @@
==========================================
  Files           836        836          
  Lines        146496     146523    +27   
  Methods           0          0          
  Messages          0          0          
  Branches      12988      12992     +4   
==========================================
+ Hits         133609     133642    +33   
+ Misses        10652      10648     -4   
+ Partials       2235       2233     -2

Powered by Codecov. Last update 98a3df2...7ac3d5f

rodrigc · 2016-12-24T00:24:57Z

@the0id please review this

….ssh/known_hosts. This will be used to change the order of supported host key algorithms when the client sends out the MSG_KEXINIT packet.

If we have an ecdsa-nistp-256 key in known_hosts, but the server offers an ssh-rsa key, this will cause conch to skip the entry instead of raising a bad host key error.

but the hostname + keyType is the same.

rodrigc · 2016-12-27T12:27:01Z

@acabhishek942 @the0id please review this

acabhishek942

Please address my inline comment if you think thats important, otherwise this change looks ready to merge

acabhishek942 · 2016-12-29T06:37:26Z

src/twisted/conch/client/default.py

+    in the KEXINIT packet.
+
+    @param host: the host to check in known_hosts
+    @param options: L{twisted.conch.client.options.ConchOptions}


please include the type of host and options here

glyph · 2016-12-30T09:10:24Z

@acabhishek942 In the future (at least until we are fully moved to github) please take care to remove the review keyword on the Trac ticket to indicate this officially counts as a review. Thanks for taking the time to do a code review though!

glyph

I know this has already been reviewed, and I don't want to propose a revert, but I think we may not be properly propagating our attention to details in reviews to new reviewers - the number of "+1" reviews on relatively complex changes and new public APIs concerns me a little. Just wanted to leave my own take on the review here as an example :).

glyph · 2016-12-30T09:12:23Z

src/twisted/conch/client/default.py

+    This can be used to change the order of supported key types
+    in the KEXINIT packet.
+
+    @type host: L{str}


While this isn't wrong exactly, convention is usually to put @param first, rather than @type.

There seem to be other places in Twisted that put @type before @param such as https://github.com/twisted/twisted/blob/trunk/src/twisted/words/xish/domish.py#L158 so I tried following that, but I'll keep that in mind next time around.

Thanks for the reference to the counterexample; I wasn't aware that was there. I'll have to go through and fix it at some point :)

glyph · 2016-12-30T09:12:25Z

src/twisted/conch/client/default.py

@@ -130,6 +130,31 @@ def isInKnownHosts(host, pubKey, options):



+def getHostKeyAlgorithms(host, options):


Was it really necessary to expose a new public API to address this?

Maybe this could have been left as an internal API. Maybe there could be other clients outside of the conch client that could guess at a preferred host key type based on what is in known_hosts

if that is the case, perhaps it makes sense to move it to a method on KnownHostsFile? Perhaps good to do this before the next release?

glyph · 2016-12-30T09:14:54Z

src/twisted/conch/client/default.py

+
+    @type host: L{str}
+    @param host: the host to check in known_hosts
+    @type options: L{twisted.conch.client.options.ConchOptions}


Why does this take a ConchOptions rather than a KnownHostsFile? It seems that this is duplicating the logic involved in determining the proper location of the known_hosts file.

glyph · 2016-12-30T09:15:15Z

src/twisted/conch/client/default.py

+    @param host: the host to check in known_hosts
+    @type options: L{twisted.conch.client.options.ConchOptions}
+    @param options: options passed to client
+    @return: L{list} of L{str} representing key types or L{None}.


Should this really be native str or is it more properly unicode or bytes?

After going through things and testing it out, str seems to be the most workable.

glyph · 2016-12-30T09:17:18Z

src/twisted/conch/client/default.py

+        or os.path.expanduser(_KNOWN_HOSTS)
+        ))
+    keyTypes = []
+    for entry in knownHosts.iterentries():


It seems like this should be a set rather than a list, since order isn't really significant. This would also be easier to spell / implement, as simply return set([entry.keyType for entry in knownHosts.iterentries() if entry.matchesHost(host)])

During my testing, I found that in the OpenSSH command-line client that the order that entries are traversed in known_hosts seems to matter. I tried to keep the behavior of the conch client the same as OpenSSH in this area, so kept it as a list.

Aah, interesting. I guess that's why it talks about "order" in the docstring? That raises the question though: how can this be "used" to do that?

glyph · 2016-12-30T09:19:56Z

src/twisted/conch/client/knownhosts.py

@@ -431,7 +431,7 @@ def hasHostKey(self, hostname, key):
            does not match the given key.
        """
        for lineidx, entry in enumerate(self.iterentries(), -len(self._added)):
-            if entry.matchesHost(hostname):
+            if entry.matchesHost(hostname) and entry.keyType == key.sshType():


This feels like a separate fix that should have been addressed in a different ticket?

I had it in a separate ticket, but it was easier to combine in one, because the fixes are interrelated, having to do with deducing the host key algorithm from the known_hosts file,
and using that to specify the order of algorithms in the client's kexinit packet.

glyph · 2016-12-30T09:22:50Z

src/twisted/conch/test/test_knownhosts.py

@@ -53,9 +53,14 @@
    b'343Hd2QHiIE0KPZJEgCynKeWoKz8v6eTSK8n4rBnaqWdp8MnGZK1WGy05MguXbyCDuTC8AmJXQ'
    b'==')

+ecdsaSampleEncodedKey = (


Just for future reference, we should try to generate keys for tests rather than having a bunch of hard-coded ones hanging around. This is the sort of thing automated security scanners pick up and complain about. There's a case to be made that they're technically wrong for complaining, but they're also the sort of thing that people blindly copy/paste from tests or examples into production systems, so in practice I think they're right.

This is just repeating a pretty well-established pattern in this file, so I wouldn't blame you for doing it here, but it's something to keep in mind for the future.

rodrigc added 4 commits December 21, 2016 13:48

In KEXINIT, in the host key algorithms, list the ecdsa algorithms bef…

0cc7532

…ore ssh-rsa. This matches behavior with OpenSSH during algorithm negotation.

topfile

5bfc36c

In MockServer, specify the supportedPublicKeys to the ones we actuall…

7a0f871

…y have in the MockServer

Shorten line to appease twistedchecker

fa20ebc

rodrigc added 7 commits December 27, 2016 04:22

For a given hostname, get a list of host key algorithms by reading ~/…

0f99059

….ssh/known_hosts. This will be used to change the order of supported host key algorithms when the client sends out the MSG_KEXINIT packet.

Add test

d5dacf3

In hasHostKey(), check the host key type when parsing ~/.ssh/known_hosts

061ef64

If we have an ecdsa-nistp-256 key in known_hosts, but the server offers an ssh-rsa key, this will cause conch to skip the entry instead of raising a bad host key error.

Test that known_hosts does not have a ecdsa key

2c9073e

topfile

5e7d114

Fix test_mismatchedHostKey test to look at two different keys

b7b3be8

but the hostname + keyType is the same.

Fix line length

9f1f397

rodrigc mentioned this pull request Dec 27, 2016

In hasHostKey(), check the host key type when parsing ~/.ssh/known_hosts #640

Closed

rodrigc added 3 commits December 27, 2016 04:59

Merge branch 'trunk' into 8958-rodrigc-ssh

7ac3d5f

Add test

b50e616

Fix whitespace

6f3440c

acabhishek942 approved these changes Dec 29, 2016

View reviewed changes

rodrigc added 2 commits December 29, 2016 02:30

Clarify documentation

d2ca170

Merge branch 'trunk' into 8958-rodrigc-ssh

cd0929c

rodrigc merged commit ae2954c into trunk Dec 29, 2016

rodrigc deleted the 8958-rodrigc-ssh branch December 29, 2016 08:14

glyph reviewed Dec 30, 2016

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

In KEXINIT packet, put EC algorithms first in list of host key algorithms #642

In KEXINIT packet, put EC algorithms first in list of host key algorithms #642

rodrigc commented Dec 22, 2016

codecov-io commented Dec 22, 2016 •

edited

Loading

rodrigc commented Dec 24, 2016

rodrigc commented Dec 27, 2016

acabhishek942 left a comment

acabhishek942 Dec 29, 2016

glyph commented Dec 30, 2016

glyph left a comment

glyph Dec 30, 2016

rodrigc Dec 31, 2016

glyph Jan 1, 2017

glyph Dec 30, 2016

rodrigc Dec 31, 2016

glyph Jan 1, 2017

glyph Dec 30, 2016

glyph Dec 30, 2016

rodrigc Dec 31, 2016

glyph Dec 30, 2016

rodrigc Dec 31, 2016

glyph Jan 1, 2017

glyph Dec 30, 2016

rodrigc Dec 31, 2016

glyph Dec 30, 2016

		@@ -130,6 +130,31 @@ def isInKnownHosts(host, pubKey, options):



		def getHostKeyAlgorithms(host, options):

In KEXINIT packet, put EC algorithms first in list of host key algorithms #642

In KEXINIT packet, put EC algorithms first in list of host key algorithms #642

Conversation

rodrigc commented Dec 22, 2016

codecov-io commented Dec 22, 2016 • edited Loading

Current coverage is 91.20% (diff: 93.75%)

rodrigc commented Dec 24, 2016

rodrigc commented Dec 27, 2016

acabhishek942 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

glyph commented Dec 30, 2016

glyph left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov-io commented Dec 22, 2016 •

edited

Loading