proposal: x/crypto/ssh: support Diffie-Hellman Group Exchange from RFC 4419

[nerdatmath]

A request for support of the diffie-hellman-group-exchange-sha1 key exchange algorithm was made on golang-nuts. This is supported by openssh 7.2 along with diffie-hellman-group-exchange-sha256, both of which are defined by RFC 4419.

[gopherbot]

CL https://golang.org/cl/29758 mentions this issue.

[quentinmit]

/cc @agl just to make sure there isn't a good reason we don't support these kex algorithms.

[baleksan]

Lack of support for diffie-hellman-group-exchange-sha256 is pretty limiting in our use-cases of SSH, and especially with compatibility of SFTP support over SSH.

[agl]

I've not looked at the linked CL, but the group-exchange protocol is SSH isn't great and goes against the direction that TLS is going. Technically it's probably ok, but it's mostly just excess complexity.

That's not to say that it's unthinkable, but it needs to be justified and there are so far only unsubstantiated requests here.

[hanwen]

it looks like this protocol needs an extra roundtrip to negotiate some parameter (the prime?). That makes it slow.

Is this because some SSH implementation stopped supporting the dh-group1 even though the SSH standards requires it? You'd expect a vendor that aggressively removed older algorithms to add newer algorithms (eg. ecdh-p256) to make up for that.

@baleksan - what does SFTP have to do with the key exchange algorithm?

[nerdatmath]

I would envision a case of competing requirements: an organization's internal policy may require they not use dh-group1 but for some other reason they are stuck using an older ssh implementation which doesn't support the latest algorithms but does support dh group exchange. That's just a guess though.

…

On Mon, Nov 28, 2016 at 10:10 AM Han-Wen Nienhuys ***@***.***> wrote: it looks like this protocol needs an extra roundtrip to negotiate some parameter (the prime?). That makes it slow. Is this because some SSH implementation stopped supporting the dh-group1 even though the SSH standards requires it? You'd expect a vendor that aggressively removed older algorithms to add newer algorithms (eg. ecdh-p256) to make up for that. @baleksan <https://github.com/baleksan> - what does SFTP have to do with the key exchange algorithm? — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#17230 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AClAHqROkV9aw2bt7oCFla9zvwypaurKks5rCxkagaJpZM4KF_jM> .

[breml]

TL;DR

There are real situations, where DH group key exchange is needed. We have a working solution for DH group exchange, where at least the client side is running in production. (Change to x/crypto/ssh, drop-in addion)
I realy would appreciate if DH group exchange would be added to x/crypto/ssh.

Background story

In our case, a large vendor of firewall appliances decided around a year ago to remove dh-group1 due to the security issues. On the other hand, they were not able/ready/willing to add new key exchange algorithms. This left us in a very unpleasant situation because our Go based tooling was no longer able to access these firewall via SSH. On the other hand, the vendor pointed out to OpenSSH and putty, as possible solutions to connect to the appliances. While this was a valid answer for manual tasks on these firewall appliances, it was still not a valid solution for us, because we lost our ability to automate tasks and to proper monitor these firewall appliances.

Implementation

My own implementations and the one provided in the CL by @nerdatmath are quite similar (ok, there is not much room for creativity).

I do offer to provide a new mergable CL.

Limitations

Both implementations currently lake a proper implementation to load additional moduli on the server side implementation. I did a first implementation but I would not consider it to be finished.

Alternative solution

Edited on 15.01.2017

The current way, how key exchange algorithms are registered forces users of x/crypto/ssh to fork the whole x/crypto repository to simply add an alternative key exchange algorithm. Maybe an interface could be provided, which would allow to provide an key exchange algorithm.

Remarks

Extra round trip

@hanwen mentioned, that there is an extra round trip involved for the key exchange. This is absolutely true, but is this a real argument against this algorithm? There are real world situations, like the mentioned situation above (removal of dh-group1 due to security reasons, without alternatives) where DH group exchange is the last possibility to still establish a SSH connection. In these situations as a user you are well willing to accept this additional round trip.

Unsustained requests

@agl mentioned in his comment, that currently there are only unsubstantiated requests. I would like to know, that would be needed, for this request to be enough sustained.
In my opinion, there is already some good justification in the following facts:

• DH group exchange is well defined in an widely accepted RFC
• Defacto standard implementations of SSH like OpenSSH (and putty on windows systems) do support this key exchange algorithm.

Addition resources

I would like to mention, that there existed already another requests to add DH group exchange to x/crypto/ssh:

• x/crypto/ssh: support [email protected] and diffie-hellman-group-exchange-sha256 #11004

[hanwen]

The larger issue is that the SSH started out as a package that offered a sensible set of algorithms and secure defaults, but interoperability with ancient hardware and strange decisions by vendors (such as the one breml alludes to) are forcing us to add everything and the kitchen sink.

There a couple of ways out:

1. hold to the original ideal (and incommodate people like breml).
2. declare defeat, and host everything and the kitchensink in the SSH package. Downside: we have to vet all the implementations.
3. declare defeat, and make algorithms pluggable. Downside: extra API surface, more likely for poor crypto implementations to proliferate.

Adam?

I slightly favor 2), as there is a finite amount of historical cruft we have to add, but you're the crypto expert who would have to review the code.

[agl]

In two cases recently where I've added things to crypto/tls that were motivated by this sort of thing, I've regretted it. It's always going to be the case that those who would benefit from adding more cruft will advocate for it strongly, but the disperse costs will be ignored.

If we're going to add things, I think issues like #17676 have much greater merit since they're asking for things that are better designed than what we currently have.

[hanwen]

I guess we'll continue deciding this on a case-by-case basis then?

I think the decision of said vendor to remove dh-group1 but not provide dh-group14 (which is basically the same code but with a larger and therefore more secure prime) is silly, and the alternative kex qualifies as cruft, so I don't see why we would want to add it. File a bugreport with said vendor?

As a response to @breml : OpenSSH is actually actually removing older algorithms from their code base, see https://www.openssh.com/txt/release-7.0 - the fact that OpenSSH implements something by itself is not a proof that it's a good idea.

(if we were to take OpenSSH as a standard, we would be removing DSA support from the Go package.)

[davecheney]

I vote for option 1, hold the line. The are companies like active state who may be comfortable managing the risk of providing a fork of this package that includes deprecated algos.

…

On Tue, 17 Jan 2017, 03:56 Han-Wen Nienhuys ***@***.***> wrote: I guess we'll continue deciding this on a case-by-case basis then? I think the decision of said vendor to remove dh-group1 but not provide dh-group14 (which is basically the same code but with a larger and therefore more secure prime) is silly, and the alternative kex qualifies as cruft, so I don't see why we would want to add it. File a bugreport with said vendor? As a response to @breml <https://github.com/breml> : OpenSSH is actually actually removing older algorithms from their code base, see https://www.openssh.com/txt/release-7.0 - the fact that OpenSSH implements something by itself is not a proof that it's a good idea. (if we were to take OpenSSH as a standard, we would be removing DSA support from the Go package.) — You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub <#17230 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAAcAzqzEAcqm0Eaw3dFid36QY-CgFe-ks5rS6FFgaJpZM4KF_jM> .

[breml]

@hanwen, @agl First of all I would like to thank you for looking at and commenting on this issue. I can assure you, that filling a bug with said firewall vendor was the first thing we did, even before we thought about implementing an additional kex for x/crypto/ssh. But as a "normal" customer (even with an installed base of several 100 devices) it is quite hard to get an open ear if you don't represent a Fortune 500 company (or something similar).

The solution I would hope for is that there is an API (proposal 3 by @hanwen), which allows to add missing algorithms. In fact this would also allow to test and use newer algorithms until they eventually become part of x/crypto.

I would like to know, how you define "cruft". As a non native English speaker, this translates for me to something like "unnecessary software". If this is an appropriate translation, this would lead me to the question, what is the definition of unnecessary. What would be for example the minimal required user base for a piece of x/crypto to not be considered unnecessary. In my opinion there are other parts within the Go ecosystem, which also only support a very small user base like exotic processor architectures and operating systems or not yet standardized network protocol proposals. Would this also be considered as cruft?

I can understand, if the decision is, that this kex is not integrated into x/crypto/ssh, because it is considered cruft. But in this case I would hope, that the same criteria would be applied to other parts of the Go ecosystem, as these parts also needed to be vet (as @hanwen puts it). Maybe there should even be some kind of guidelines, what requirements have to be fulfilled for a feature to be added. This is in my opinion especially true, if decisions are made on a case-by-case base.

[breml]

Just for documentation purposes: in zmap/zgrab@3a144e6 my single file drop in solution was used to implement DH Group Exchange.

[grinsted]

@hanwen & @baleksan : Lack of diffie-hellman-group-exchange-sha256 breaks SFTP in rclone for me. The cloud storage I connect to only offer diffie-hellman-group-exchange-sha256 and nothing else.

I also reported it here: rclone/rclone#1810

[gm42]

Let's not forget the cost/waste of having each developer use ugly workarounds or implementations (much worse than those that would be provided by approaches (2) or (3)).

[nojo]

Typical response from a vendor when I ask them to add new key exchange algorithms so that they're interoperable with Go:

"Thanks to ubiquitous support for the diffie-hellman-group-exchange-sha256 key exchange algorithm (with the unfortunate exception of golang), we have not seen demand for other algorithms to be supported aside from your request. Unfortunately, due to the effort that would be involved, it is unlikely that we will be able to add support for additional algorithms such as [email protected] in the near future unless we see significant demand for it."

[42wim]

Also got bitten by the diffie-hellman-group-exchange-sha256 issue.
Seems like the discussion has halted some time ago, are they're any plans to add support for this ?

[BennySitbon]

We also need support for diffie-hellman-group-exchange-sha256.... are there any news about supporting this kex algo?

[hanwen]

My stance has been that the SSH package should be a small, well curated subset of SSH, but I have to reconsider this stance.

Because SSH is the lingua franca to talk to network devices, and these devices often are not upgraded after the initial release, the SSH package has grown many extensions of that have questionable quality (eg. 3DES-CBC) but are indispensable for administrating devices in real life. The group-exchange kex methods also belong to this category.

My new stance will be that features available (and not deprecated) in OpenSSH should in principle be open to inclusion.

If someone would be so good to revive the outstanding change for this and send it for review, that would be great.

[breml]

@hanwen I have a "drop-in" implementation, that is already used by several projects. I can create a PR on that basis.

[gopherbot]

Change https://golang.org/cl/174257 mentions this issue: ssh: add diffie-hellman-group-exchange-sha256

[breml]

@hanwen I looked into my existing code and I sent a PR (golang/crypto#87). While doing so, the following questions have emerged:

• Should this PR include diffie-hellman-group-exchange-sha256 as well as the less secure diffie-hellman-group-exchange-sha1? The IETF draft Key Exchange (KEX) Method Updates and Recommendations for Secure Shell (SSH) dated of July, 30th 2017 does recommend to NOT include diffie-hellman-group-exchange-sha1. On contrary, if the purpose of this PR is to make the Go ssh package more similar to OpenSSH, then it should still be included (as it is still included in recent OpenSSH packages, e.g. Ubuntu 19.04: OpenSSH_7.9p1 Ubuntu-10, OpenSSL 1.1.1b 26 Feb 2019).
• The slice supportedKexAlgos does list the key exchange algorithms in the order of preference. What is the right position to insert diffie-hellman-group-exchange-sha256 (and maybe diffie-hellman-group-exchange-sha1)?
• In order to negotiate the key size, the client and the server provide a minimum, prefered and maximum key size (see). What key sizes should we use here (I think the maximum is OK, the question is more about the minimum and the prefered key size)
• For DH Group Exchange to work, the server side implementation uses moduli (with the agreed to key size) in order to generate the key.
  • I included the necessary code to load the moduli from the local OpenSSH (normally /etc/ssh/moduli or /usr/local/etc/moduli, see). Do you know other locations, we should try for moduli?
  • Are you aware about locations for moduli on e.g. Windows?
  • As fallback, the implementation in the PR includes some moduli from OpenSSH (see link in the source code). The original PR used some other moduli from RFC 4253 and RFC 3526 (see). Which route should we go here?

Update: The document Key Exchange (KEX) Method Updates and Recommendations for Secure Shell (SSH) is a IETF draft (and not an approved document)

[breml]

In regards to the minimum key size RFC 8270 should be considered.

[breml]

@FiloSottile When I hit this problem in 2017, the situation was, that the hardware appliances, we needed to connect to with SSH + DH Group Exchange, only supported the SHA1 variant. Since then, the situation has improved with newer models and newer firmware releases, such that SHA256 is supported as well, but I am pretty sure, that there are still devices in the field, where only SHA1 variant is supported.
My detailed explanation does not mention SHA1 explicitly, because the implementation I did, supported both variants. Also the piece of software, that uses my implementation is closed source, so it can not be linked in this issue directly.

[FiloSottile]

@breml Thanks for following up. I'm sure there will always be a long tail, but it sounds like the SHA-256 variant these days is well supported, and the amount of deployments supporting only the SHA-1 variant is getting smaller, meaning it will get less important to have as time passes, not more.

[hanwen]

we can start with sha256 and add sha1 if there is a need.

I don't see a cost to adding sha1 that we haven't already paid by adding 3DES-CBC, so it's largely an academic discussion.

[BennySitbon]

Hi all contributors!
I must say thank you so much for pushing this change! I can't wait for it to go out - right now we have to write some crazy, ugly, impossible to maintain code to support the algorithms you're about to support natively in the library. Can wait to delete all this ugly code.
Thank you! ❤️

[canni]

Any news on when this can be expected to be released?

[ianlancetaylor]

@canni As far as I know it has been released. If you update to the current version of golang.org/x/crypto you should see it.

[canni]

@iamoryanmoshe hmm... I looks like on go1.13.4 and latest x/crypto I'm still getting this error:
Failed to connect to *******:22. Error: ssh: handshake failed: ssh: no common algorithm for key exchange; client offered: [[email protected] ecdh-sha2-nistp256 ecdh-sha2-nistp384 ecdh-sha2-nistp521 diffie-hellman-group14-sha1], server offered: [diffie-hellman-group-exchange-sha256

[breml]

@canni This kex is disabled by default so you have to explicitly opt-in to use it. Please have a look at this test case for an example: https://github.com/golang/crypto/blob/57b3e21c3d5606066a87e63cfe07ec6b9f0db000/ssh/test/session_test.go#L420-L427

[canni]

@breml thanks, this works! I wasn't aware of this :)

[sumiet]

@canni can you share the config changes you made? It's still not working for me.

Here is what I am trying:

sshConfig := &ssh.ClientConfig{
		User:            s.params.User,
		Auth:            []ssh.AuthMethod{publicKeysFunc(signer)},
		HostKeyCallback: fixedHostKeyFunc(hostPublicKey),
		HostKeyAlgorithms: []string{
			"[email protected]",
			"ecdh-sha2-nistp256",
			"ecdh-sha2-nistp384",
			"ecdh-sha2-nistp521",
			"diffie-hellman-group14-sha1",
			"diffie-hellman-group1-sha1",
			"diffie-hellman-group-exchange-sha256"},
	}

	return dialFunc("tcp", s.params.Server, sshConfig)

[canni]

@sumiet This works for me:

sshConf := ssh.Config{}
sshConf.SetDefaults()
sshConf.KeyExchanges = append(
	sshConf.KeyExchanges,
	"diffie-hellman-group-exchange-sha256",
	"diffie-hellman-group-exchange-sha1",
)

cfg := ssh.ClientConfig{
	/* removed */
	Config: sshConf,
}

[sumiet]

Thanks @canni, That helped! :)

[danfletcher1]

Thank you for adding that feature !! Works great.

[KarthikNath]

This works ! but now i'm running into failures on concurrent ssh, a few gets through tho. Below is the error i'm seeing,

ssh: handshake failed: crypto/rsa: verification error

It has nothing to do with the keys as its random hosts failing always. Seems more like its running into race condition with diffiehelman addition. Works perfectly with default kex's. Below is the race condition errors i see.

WARNING: DATA RACE
Write at 0x00c00021ace8 by goroutine 85:
golang.org/x/crypto/ssh.(*dhGEXSHA).Client()
xxxxxxxxxxx/go/src/golang.org/x/crypto/ssh/kex.go:602 +0x43d
golang.org/x/crypto/ssh.(*handshakeTransport).client()
xxxxxxxxxxx/go/src/golang.org/x/crypto/ssh/handshake.go:627 +0x127
golang.org/x/crypto/ssh.(*handshakeTransport).enterKeyExchange()
xxxxxxxxxxx/go/src/golang.org/x/crypto/ssh/handshake.go:587 +0xaa3
golang.org/x/crypto/ssh.(*handshakeTransport).kexLoop()
xxxxxxxxxxx/go/src/golang.org/x/crypto/ssh/handshake.go:301 +0x296

Previous write at 0x00c00021ace8 by goroutine 203:
golang.org/x/crypto/ssh.(*dhGEXSHA).Client()
xxxxxxxxxxx/go/src/golang.org/x/crypto/ssh/kex.go:602 +0x43d
golang.org/x/crypto/ssh.(*handshakeTransport).client()
xxxxxxxxxxx/go/src/golang.org/x/crypto/ssh/handshake.go:627 +0x127
golang.org/x/crypto/ssh.(*handshakeTransport).enterKeyExchange()
xxxxxxxxxxx/go/src/golang.org/x/crypto/ssh/handshake.go:587 +0xaa3
golang.org/x/crypto/ssh.(*handshakeTransport).kexLoop()
xxxxxxxxxxx/go/src/golang.org/x/crypto/ssh/handshake.go:301 +0x296

Goroutine 85 (running) created at:
golang.org/x/crypto/ssh.newClientTransport()
xxxxxxxxxxx/go/src/golang.org/x/crypto/ssh/handshake.go:135 +0x311
golang.org/x/crypto/ssh.(*connection).clientHandshake()
xxxxxxxxxxx/go/src/golang.org/x/crypto/ssh/client.go:105 +0x47c
golang.org/x/crypto/ssh.NewClientConn()
xxxxxxxxxxx/go/src/golang.org/x/crypto/ssh/client.go:83 +0x1d1
golang.org/x/crypto/ssh.Dial()
xxxxxxxxxxx/go/src/golang.org/x/crypto/ssh/client.go:177 +0xe6
xxxxxxx/force.Sshconnect()
xxxxxxxxxxx/go/src/xxxxxxx/force/forcessh.go:226 +0xca
xxxxxxx/force.executeCmd()
xxxxxxxxxxx/go/src/xxxxxxx/force/forcessh.go:183 +0x7d
xxxxxxx/force.Input.func1()
xxxxxxxxxxx/go/src/xxxxxxx/force/forcessh.go:380 +0x87

Goroutine 203 (running) created at:
golang.org/x/crypto/ssh.newClientTransport()
xxxxxxxxxxx/go/src/golang.org/x/crypto/ssh/handshake.go:135 +0x311
golang.org/x/crypto/ssh.(*connection).clientHandshake()
xxxxxxxxxxx/go/src/golang.org/x/crypto/ssh/client.go:105 +0x47c
golang.org/x/crypto/ssh.NewClientConn()
xxxxxxxxxxx/go/src/golang.org/x/crypto/ssh/client.go:83 +0x1d1
golang.org/x/crypto/ssh.Dial()
xxxxxxxxxxx/go/src/golang.org/x/crypto/ssh/client.go:177 +0xe6
xxxxxxx/force.Sshconnect()
xxxxxxxxxxx/go/src/xxxxxxx/force/forcessh.go:226 +0xca
xxxxxxx/force.executeCmd()
xxxxxxxxxxx/go/src/xxxxxxx/force/forcessh.go:183 +0x7d
xxxxxxx/force.Input.func1()
xxxxxxxxxxx/go/src/xxxxxxx/force/forcessh.go:380 +0x87

Any help is appreciated.

[FiloSottile]

@KarthikNath Thanks for reporting this. It looks like this key exchange implementation is not concurrent safe, which I suspect is a requirement. Can you please open a new issue about this?

[breml]

@KarthikNath Please provide some more details on how you use this kex with some code example. And please mention me on the issue, such that I can have a look.

[hanwen]

the kex instance comes from kexAlgoMap, and is shared across invocations. This worked for previous algorithms that had constant parameters. For the group exchange, you have to return a fresh instance for every kex.