Opened 4 years ago

Closed 4 years ago

#8100 enhancement closed fixed (fixed)

SSH client only uses the deprecated MSG_KEX_DH_GEX_REQUEST_OLD instead of using MSG_KEX_DH_GEX_REQUEST

Reported by: Colin Watson Owned by: Colin Watson
Priority: normal Milestone:
Component: conch Keywords:
Cc: z3p Branch: branches/ssh-client-kex-old-8100-2
branch-diff, diff-cov, branch-cov, buildbot
Author: adiroiban

Description

The diffie-hellman-group-exchange-sha256 support I added in #7672 interoperates as a client with OpenSSH servers up to 6.8, but not 6.9 or newer. This is because it uses MSG_KEX_DH_GEX_REQUEST_OLD to initiate group exchange. This message number is listed as "for backward compatibility" in RFC 4419 (March 2006), probably because the newer MSG_KEX_DH_GEX_REQUEST is more useful: it allows specifying a range of acceptable group sizes rather than simply a preferred size. OpenSSH dropped support for this in 6.9 with the commit message "deprecate ancient, pre-RFC4419 and undocumented SSH2_MSG_KEX_DH_GEX_REQUEST_OLD message".

The chances of running across a server implementation that supports DH group exchange but not MSG_KEX_DH_GEX_REQUEST are pretty slim. Support for MSG_KEX_DH_GEX_REQUEST was added in OpenSSH 2.9p2 at the latest (probably earlier - git history that far back is confusing to navigate), and OpenSSH's compat.c records no non-OpenSSH server implementations that use the old message type. I think that therefore it is entirely safe to switch to the new message type in the conch client.

Attachments (3)

drop-old-dhgex-request.patch (3.4 KB) - added by Colin Watson 4 years ago.
drop-old-dhgex-request-1-to-2.patch (3.6 KB) - added by Colin Watson 4 years ago.
Patch against 0406b559dd8432bdf355a30611f5f0d8056dee59 to make instance variables private and document them
drop-old-dhgex-request-2-to-3.patch (757 bytes) - added by Colin Watson 4 years ago.
Expand comment

Download all attachments as: .zip

Change History (17)

comment:1 Changed 4 years ago by DefaultCC Plugin

Cc: z3p added

Changed 4 years ago by Colin Watson

comment:2 Changed 4 years ago by Colin Watson

Keywords: review added

This patch switches to the new message for initiating DH group exchange. Server support for the old message is retained. I've interop-tested this against OpenSSH 6.7 and 6.9 servers.

Minimum and maximum DH group sizes are from the RFC, and OpenSSH uses the same fixed values for these. The preferred DH group size ought to be computed more dynamically as OpenSSH does, but this is no worse than the previous state; I've just made it clearer what's going on (by using struct.pack rather than a hardcoded byte string) and left an XXX comment.

comment:3 Changed 4 years ago by Adi Roiban

Summary: diffie-hellman-group-exchange-sha256 doesn't interoperate with OpenSSH >= 6.9SSH client only uses the deprecated MSG_KEX_DH_GEX_REQUEST_OLD instead of using MSG_KEX_DH_GEX_REQUEST

I agree that we should start with MSG_KEX_DH_GEX_REQUEST.

It looks like there is a bug in OpenSSH https://bugzilla.mindrot.org/show_bug.cgi?id=2494 ... so for OpenSSH we can not fallback or try other method

I don't know what to say about the backward compatibility.

I would say that this is a bug in conch.ssh.client. Based on RFC 4419 the client should use MSG_KEX_DH_GEX_REQUEST

I think that conchs.sshclient might use MSG_KEX_DH_GEX_REQUEST_OLD for backward compatiblity... in case SSH_MSG_UNIMPLEMENTED is returned from MSG_KEX_DH_GEX_REQUEST

again, the RFC 4419 is very brief here :(

I could not find any reference about pre-4419 support in client docs/docstring... so I don't know if this change qualifies for compatible or incompatible.

comment:4 Changed 4 years ago by Adi Roiban

Author: adiroiban
Branch: branches/ssh-client-kex-old-8100

(In [46139]) Branching to ssh-client-kex-old-8100.

comment:5 Changed 4 years ago by Adi Roiban

From IRC

(14:53:44) glyph: adiroiban: definitely compatible
(14:53:58) glyph: adiroiban: the most important compatibility to preserve is python-API level
(14:54:11) glyph: adiroiban: literally nobody in the world should be running openssh 2.9p2

we can do this without deprecation work.

Thanks!

comment:6 Changed 4 years ago by Adi Roiban

Keywords: review removed
Owner: set to Colin Watson

Hi Colin and many thanks for your patch.

all new code should be documented, this include instance/class variables ... even if they are private.

I tried to updated your patch to document them.

We can send the recommended OpenSSH values, but if our client only support 1024 primes, then we should not ask for 2048 or inform the server that we are ok with bigger numbers.

So I think that this patch should include handling for the mix/max/pref values.

I tried to update your patch to sketch one possible fix.

There are 2 commits... first of them is exposing a new public API to allow configuring the preferred size.

I think that we start without any public API change and just document the rule based on which the preferred method is chose... later we can see how to expose this as a public API.

With the preferred size we hit the configuration area... and this is delicate issue in Twisted :(

Please review my comments and commits.

Either submit a new patch or come back with feedback.

Thanks again and sorry for all the noise related to the compatibility policy.

comment:7 Changed 4 years ago by Colin Watson

A couple of typos in your recent changes:

+            # Maybe no commons protocols were agreed.

commons -> common

+            # We agreed on a fixed group key exchange algoright.

algoright -> algorithm

Your changes to document instance variables are fine, and of course it's better for them to be private.

Regarding configuration, I think you're on the wrong path. This doesn't actually need to be configurable: that would be overkill and the caller doesn't have easy access to the information needed to work out a sensible value anyway. What I meant by my comment that it should be computed more dynamically is that Twisted should work out a safer preferred group size for itself. The way that OpenSSH does this - and I expect this is from a cryptographic standard/paper somewhere, but I don't have the reference to that - is to take the maximum of the encryption cipher key length, the encryption block size, the encryption IV length, and the MAC key length, which gives it a number of bits; and then it estimates a DH group size with an attack complexity approximately O(2bits), which is a simple mapping that you can find in its dh_estimate function.

So, I think we should not expose a new public API to make this configurable, but rather we should have a new ticket for conch to work out a better preferred group size for itself, and then leave it at that. Don't make things configurable where Twisted can do a better job of working out good values for itself.

comment:8 Changed 4 years ago by Colin Watson

Owner: changed from Colin Watson to Adi Roiban

Changed 4 years ago by Colin Watson

Patch against 0406b559dd8432bdf355a30611f5f0d8056dee59 to make instance variables private and document them

Changed 4 years ago by Colin Watson

Expand comment

comment:9 Changed 4 years ago by Colin Watson

Keywords: review added

The two attachments apply in sequence on top of my initial patch to fix up instance variable handling, per adiroiban's review. I've filed #8103 for the preferred group size issue.

comment:10 Changed 4 years ago by Adi Roiban

Branch: branches/ssh-client-kex-old-8100branches/ssh-client-kex-old-8100-2

(In [46158]) Branching to ssh-client-kex-old-8100-2.

comment:11 Changed 4 years ago by Adi Roiban

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

Thanks. Changes look good.

I added the missing news file fragments to advertise the changes.

I don't think that this is a bug... but rather a feature which allows Conch SSH client to work with latest OpenSSH versions. Please correct me if I am wrong :)

I added the standard comment format for FIXME reference to #8103.

I moved var initialization in the class definition. If someone wants to go with different values they can overwrite those values... not recommended... also because those are private.

I have sent the changes to buildbot.

Please check my latest commits and make sure they make sense.

If all is OK, I will merge it.

Many, many thanks for your work on this ticket.

comment:12 Changed 4 years ago by Colin Watson

Owner: changed from Colin Watson to Adi Roiban

Your changes look fine, thank you, except that you have written "support" in twisted/conch/topfiles/8100.removal where it should be "supports" (the class is singular). I would suggest that 8100.feature should probably add "using MSG_KEX_DH_GEX_REQUEST" after "key exchange" too; your sentence is strictly correct if you know the intricacies of RFC 4419, but adding the message name makes it clearer and parallels 8100.removal.

comment:13 Changed 4 years ago by Adi Roiban

Owner: changed from Adi Roiban to Colin Watson

Many thanks for your comments.

Waiting for the builds and then will merge.

comment:14 Changed 4 years ago by Adi Roiban

Resolution: fixed
Status: newclosed

(In [46168]) Merge ssh-client-kex-old-8100-2: Add support in twisted.conch.ssh client for RFC 4419.

Author: cjwatson Reviewer: adiroiban Fixes: #8100

Note: See TracTickets for help on using tickets.