Opened 3 years ago

Closed 2 years ago

#7672 enhancement closed fixed (fixed)

Add diffie-hellman-group-exchange-sha256 key exchange algorithm to conch

Reported by: Colin Watson Owned by: Colin Watson
Priority: normal Milestone:
Component: conch Keywords:
Cc: Antoine Martin, mkuron Branch: branches/conch-ssh-dhgex-sha256-7672
branch-diff, diff-cov, branch-cov, buildbot
Author: adiroiban

Description

OpenSSH has recently removed support for pre-SHA2 key exchange algorithms, including diffie-hellman-{group-exchange,group1}-sha1. This patch adds support for diffie-hellman-group-exchange-sha256 per RFC4419, allowing interoperation with OpenSSH 6.7. It's mostly a straight cargo-cult from the -sha1 variant.

Unfortunately I haven't been able to get OpenSSH's interop tests working with conch as yet. Looking at the test server log confirms that it's completing the key exchange and initiating a session, but the transferred test file seems to be truncated to zero bytes. It would be good if somebody could look into this. However, things seem to work interactively and twisted.conch's own tests pass with my modifications, so I'm reasonably confident that I've got the basics right.

Attachments (6)

dhgex-sha256.patch (22.1 KB) - added by Colin Watson 3 years ago.
Add diffie-hellman-group-exchange-sha256
dhgex-sha256-2.patch (35.2 KB) - added by Colin Watson 2 years ago.
dhgex-sha256-3.patch (37.0 KB) - added by Colin Watson 2 years ago.
dhgex-sha256-2-to-3.patch (10.8 KB) - added by Colin Watson 2 years ago.
dhgex-sha256-4.patch (37.8 KB) - added by Colin Watson 2 years ago.
dhgex-sha256-3-to-4.patch (758 bytes) - added by Colin Watson 2 years ago.

Download all attachments as: .zip

Change History (21)

Changed 3 years ago by Colin Watson

Attachment: dhgex-sha256.patch added

Add diffie-hellman-group-exchange-sha256

comment:1 Changed 3 years ago by Colin Watson

Keywords: review added

comment:2 Changed 3 years ago by Adi Roiban

Keywords: review removed
Owner: set to Colin Watson

Many thanks for the patch!

Please note that to have this merged, it will need a news file. https://twistedmatrix.com/trac/wiki/ReviewProcess#Newsfiles

Also, to help apply the patch it would be nice to configure git, not to create diffs with prefix:

https://twistedmatrix.com/trac/wiki/GitMirror#CreatingaGitclone

When I applied the patch on trunk, one hunk was reject (don't know why)... but I can figure it out and do the review with this diff

$ patch -p1 < ~/Downloads/dhgex-sha256.patch 
patching file twisted/conch/ssh/factory.py
patching file twisted/conch/ssh/transport.py
patching file twisted/conch/test/test_ssh.py
patching file twisted/conch/test/test_transport.py
Hunk #6 FAILED at 1367.
1 out of 13 hunks FAILED -- saving rejects to file twisted/conch/test/test_transport.py.rej


Code from factory.py and test_ssh.py looks good.


For transport.py

I see this code in more places

if self.kexAlg == 'diffie-hellman-group-exchange-sha256':
  h = sha256()
else:
  h = sha1()

I suggest that in ssh_KEXINIT to also set a self.keyAlgHashMethod once the keyAlg is set and then just use this reference.


test_transport.py

  1. Rename test_getKey(self) to test_getKeySHA1(self)
  1. Rename test_getKey_sha256 to test_getKeySHA256 ... sorry for asking this, but it looks like this is the way Twisted developers think that they can improve test readability... maybe we can ask them to reconsider this.
  1. In test test_KEX_DH_GEX_REQUEST_sha256 and test_KEX_DH_GEX_INIT_after_REQUEST_sha256 (and similar) I see a lot of duplicate code, please extract it into a helper method.

Here are my steps:

  1. Got and compiled latest OpenSSH release openssh-6.7p1.tar.gz
  1. Start a modified version of sshsimpleserver.py in which it loads primes from '/etc/ssh/moduli' . Here is the code https://gist.github.com/adiroiban/c0e309b6d58494b8ebea
  1. Tried to connect using SSH client and forcing sha256 ./ssh -oKexAlgorithms=diffie-hellman-group-exchange-sha256 -p 5022 user@localhost

It works... but with a huge delay after debug1: expecting SSH2_MSG_KEX_DH_GEX_REPLY ... delay in which no new connections were accepted... so this is an CPU lock :(

Note that the delay was also present for sha1, so is not something new.

Did you also get the slow key exchange process? I got a i7-4500U CPU @ 1.80GHz

This is not a problem for this ticket, but maybe we should create a new ticket to use deferToThread for computing the key exchange values.


I have not tested a SFTP server / client as there is no simple example in Twisted source code... I will brink this over the email list and see if this deserve a ticket

Meanwhile I will try to get the code for a simple SFTP server so that we can share with other reviewers of this ticket.

Many thanks for this patch!

comment:3 Changed 3 years ago by Jean-Paul Calderone

Component: coreconch

comment:4 Changed 3 years ago by Antoine Martin

Cc: Antoine Martin added

comment:5 Changed 3 years ago by Adi Roiban

Please take a look at latest development from #7717

comment:6 Changed 3 years ago by mkuron

Cc: mkuron added

Changed 2 years ago by Colin Watson

Attachment: dhgex-sha256-2.patch added

comment:7 Changed 2 years ago by Colin Watson

Keywords: review added
Owner: changed from Colin Watson to Adi Roiban

I've attached an updated version of this patch, based on the work in #7717. This includes a fair amount of test refactoring to reduce duplication, which I'm afraid involved moving some methods around a bit, so that part of the diff is a little noisy.

Regarding your performance issue, this mostly goes away if you install gmpy as recommended in the conch documentation.

comment:8 Changed 2 years ago by Adi Roiban

Author: adiroiban
Branch: branches/conch-ssh-dhgex-sha256-7672

(In [45918]) Branching to conch-ssh-dhgex-sha256-7672.

comment:9 Changed 2 years ago by Adi Roiban

Keywords: review removed
Owner: changed from Adi Roiban to Colin Watson

Many thanks for you work!

Maybe we can ramame getHashAlgorithm into getHashProcessor or something to hint that this is a callable and not the hash algo name.

In twisted/conch/ssh/factory.py in buildProtocol can we have a dynamic lookup which removes all non-fixed algorithm names?

In twisted/conch/test/test_transport.py in getPrimes, can we update the docstring to inform about generic non-fixed key exchanges, rather than listing here each new method that we support?

In BaseSSHTransportTests there is a double space... I can also fix it before merge.

In ServerSSHTransportTests maybe we need to rename test_KEXINIT to test_KEXINITAllAlgorithms and update the docstrign that this is a kexinit for the case when all algorithms are supported.

Same for ClientSSHTransportTests.


Other than that, it looks look, and the test pass.

Please review my comments and resubmit.

Thanks!

Changed 2 years ago by Colin Watson

Attachment: dhgex-sha256-3.patch added

Changed 2 years ago by Colin Watson

Attachment: dhgex-sha256-2-to-3.patch added

comment:10 Changed 2 years ago by Colin Watson

Keywords: review added
Owner: changed from Colin Watson to Adi Roiban

I've addressed your review comments, thanks. It would be nice if the style guide documented whether to use dot-space or dot-space-space, since both are in widespread use in Twisted.

Changed 2 years ago by Colin Watson

Attachment: dhgex-sha256-4.patch added

Changed 2 years ago by Colin Watson

Attachment: dhgex-sha256-3-to-4.patch added

comment:11 Changed 2 years ago by Colin Watson

I missed an OpenSSH interoperability test (requires OpenSSH >= 4.4). Fortunately it's pretty trivial.

comment:12 Changed 2 years ago by Adi Roiban

Keywords: review removed
Owner: changed from Adi Roiban to Colin Watson

Thanks for the latest patches.

I did a few small changes while reviewing the tests and trying to check that the test docstring matches the test execution.

Please check my changes.

If all is ok I will merge.

Thanks!

comment:13 Changed 2 years ago by Colin Watson

Owner: changed from Colin Watson to Adi Roiban

Your changes look fine, thanks, except that in one place in a comment you say "diffie-hellman-group1-sha256" where you should say "diffie-hellman-group-exchange-sha256".

comment:14 Changed 2 years ago by Adi Roiban

Owner: changed from Adi Roiban to Colin Watson

Thanks. I have fixed it. Tests pass. Will merge.

I have also run a manual test using the docs/conch/examples/sshsimpleserver.py

ssh -p 5022 user@localhost -oKexAlgorithms=diffie-hellman-group-exchange-sha256

comment:15 Changed 2 years ago by Adi Roiban

Resolution: fixed
Status: newclosed

(In [46130]) Merge conch-ssh-dhgex-sha256-7672: Add diffie-hellman-group-exchange-sha256 to twisted.conch.ssh

Author: cjwatson Reviewer: adiroiban Fixes: #7672

Note: See TracTickets for help on using tickets.