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

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

Open
nerdatmath opened this Issue Sep 25, 2016 · 17 comments

Comments

Projects
None yet
@nerdatmath
Copy link
Contributor

nerdatmath commented Sep 25, 2016

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

This comment has been minimized.

Copy link

gopherbot commented Sep 25, 2016

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

@quentinmit quentinmit changed the title ssh: support Diffie-Hellman Group Exchange from RFC 4419 x/crypto/ssh: support Diffie-Hellman Group Exchange from RFC 4419 Oct 4, 2016

@quentinmit quentinmit added the NeedsFix label Oct 4, 2016

@quentinmit quentinmit added this to the Unreleased milestone Oct 4, 2016

@quentinmit

This comment has been minimized.

Copy link
Contributor

quentinmit commented Oct 4, 2016

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

@baleksan

This comment has been minimized.

Copy link

baleksan commented Nov 7, 2016

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

This comment has been minimized.

Copy link
Contributor

agl commented Nov 7, 2016

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

This comment has been minimized.

Copy link
Contributor

hanwen commented Nov 28, 2016

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

This comment has been minimized.

Copy link
Contributor Author

nerdatmath commented Nov 29, 2016

@breml

This comment has been minimized.

Copy link

breml commented Jan 15, 2017

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:

@hanwen

This comment has been minimized.

Copy link
Contributor

hanwen commented Jan 16, 2017

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

This comment has been minimized.

Copy link
Contributor

agl commented Jan 16, 2017

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

This comment has been minimized.

Copy link
Contributor

hanwen commented Jan 16, 2017

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

This comment has been minimized.

Copy link
Contributor

davecheney commented Jan 16, 2017

@breml

This comment has been minimized.

Copy link

breml commented Jan 16, 2017

@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

This comment has been minimized.

Copy link

breml commented Feb 17, 2017

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

@grinsted

This comment has been minimized.

Copy link

grinsted commented Nov 8, 2017

@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: ncw/rclone#1810

@gm42

This comment has been minimized.

Copy link

gm42 commented Feb 2, 2018

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

This comment has been minimized.

Copy link

nojo commented Feb 22, 2018

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 curve25519-sha256@libssh.org in the near future unless we see significant demand for it."

@42wim

This comment has been minimized.

Copy link

42wim commented Mar 18, 2019

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 ?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.