golang / go

The Go programming language
https://go.dev
BSD 3-Clause "New" or "Revised" License
123.99k stars 17.67k forks source link

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

Closed nerdatmath closed 5 years ago

nerdatmath commented 8 years ago

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 commented 8 years ago

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

quentinmit commented 8 years ago

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

baleksan commented 8 years ago

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 commented 8 years ago

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 commented 7 years ago

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 commented 7 years ago

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 notifications@github.com 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 https://github.com/golang/go/issues/17230#issuecomment-263347206, or mute the thread https://github.com/notifications/unsubscribe-auth/AClAHqROkV9aw2bt7oCFla9zvwypaurKks5rCxkagaJpZM4KF_jM .

breml commented 7 years ago

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:

Addition resources

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

hanwen commented 7 years ago

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 commented 7 years ago

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 commented 7 years ago

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 commented 7 years ago

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 notifications@github.com 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 https://github.com/golang/go/issues/17230#issuecomment-272914110, or mute the thread https://github.com/notifications/unsubscribe-auth/AAAcAzqzEAcqm0Eaw3dFid36QY-CgFe-ks5rS6FFgaJpZM4KF_jM .

breml commented 7 years ago

@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 commented 7 years ago

Just for documentation purposes: in https://github.com/zmap/zgrab/commit/3a144e6e866b8c96b5717ce455ad5034630e918b my single file drop in solution was used to implement DH Group Exchange.

grinsted commented 7 years ago

@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: https://github.com/ncw/rclone/issues/1810

gm42 commented 6 years ago

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 commented 6 years ago

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 commented 5 years ago

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 commented 5 years ago

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

hanwen commented 5 years ago

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 commented 5 years ago

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

gopherbot commented 5 years ago

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

breml commented 5 years ago

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

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

breml commented 5 years ago

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

salmanwaseem007 commented 5 years ago

@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

Same issue for me.

hanwen commented 5 years ago

The primary use here is a Go client with a server that cannot be upgraded to newer algorithms, so

hanwen commented 5 years ago

I think RFC 8270 should not be considered, unless we can be sure this does not break backward compatibility.

If you care about security, you should use ECDH based on P256 or the kex based on Ed25519

breml commented 5 years ago

@hanwen I updated the PR according to your feedback.

The primary use here is a Go client with a server that cannot be upgraded to newer algorithms, so

  • we should preferably add the SHA1 method.

Done.

  • the algorithms should not be part of the preferred/recommended kex list. It should only be a supported kex.

I removed this kex from the list of prefered kex. The variable for the prefered kex is currently called supportedKexAlgos, which is missleading in my opinion, because the set of supported kex is bigger. The variable only reflects the prefered/suggested kex.

  • what the devices that insist on using the group kex want for min/max? in absense of further data, the openSSH defaults should be OK

I let the minimum key size untouched on a value of 2048 bit, because the current OpenSSH implementation uses this minimum according to their dh.h. They have this value since Oct 2015 (they also reference RFC 8270). I think this value is save also with the devices in question since the actual bit size is chosen by comparing min, prefered and max of both sides and I doubt there is a device, which has the max set to less than 2048 bits.

  • Let's not insert file parsing routines here. Instead, provide the OpenSSH data as Go types, and leave it to the server implementers to parse files if they want to provide server support too.

I am not yet sure how to proceed with this one. Questions:

  1. Do you want to include the full list of the OpenSSH data? The moduli file from OpenSSH contains more than 200 moduli, which then all would get compiled into every binary, parsed during initialization and put into memory for every application that uses the ssh package.
  2. With "as Go types" do you mean something like an exported var Moduli map[int][]Modulus and then export the type Modulus as well like this:
type Modulus struct {
    Generator int64
    Modulus   *big.Int
}

With this implementation proposed above, there is the potential danger of a data race for the Moduli variable.

An alternative would be to include the moduli as a config option in ServerConfig.

breml commented 5 years ago

PR on Github: https://github.com/golang/crypto/pull/87

FiloSottile commented 5 years ago

I understand the pain of balancing compatibility and feature creep, but I don't want inclusion in OpenSSH to be the bar for inclusion in x/crypto/ssh. Changes like this should encounter appropriate resistance, and an automatic criteria like "is it in OpenSSH" will open the flood gates to features we don't actually need.

Every addition should be judged on the basis of how important to compatibility it is, how it can be avoided or replaced with something more widely compatible, and how hard it is to work around its absence. I'm not saying this issue doesn't clear that bar, I'm saying that's where the bar should stay.

About this specific instance, a few thoughts:

hanwen commented 5 years ago

thanks Filippo:

Regarding further technical discussion: could we do them on the review instead? That's what code review is for after all.

hanwen commented 5 years ago

"..open the flood gates to features we don't actually need.."

who is 'we' in this sentence?

FiloSottile commented 5 years ago
  • re. SHA1: this bugreport opens with a request for SHA1. I expect it to be zero extra work, so if we are adding suboptimal algorithms for compatibility's sake, we might add SHA1 as well.

Finding a better solution that still addresses most of use case is what I want this process for. A single feature request should not be enough to clear the bar for inclusion, what arguably does it are the detailed reports that followed, and those all mention sha256.

  • Is constant-time a consideration for establishing ephemeral keys? (I admit to not having read the RFC yet.)

Good point, I might be wrong worrying about that here. I’ll leave it to you, if you are convinced the key is single use and there is no risk, I have no objection.

"..open the flood gates to features we don't actually need.."

who is 'we' in this sentence?

The Go community, which I admit is a hard target to keep track of.

hanwen commented 5 years ago

Since Filippo asked for using the crypto proposal, added the label.

The proposal is:

Support the group-exchange key exchange method for the benefit of clients connecting to older servers. The kex should be have a server-side implementation too for unittesting purposes. We don't support configurable moduli.

I added some comments on the change to reflect this.

FiloSottile commented 5 years ago

Since there is a CL in flight for this, and I'm on vacation next week, fast-tracking this without passing by the proposal committee.

Let's do only diffie-hellman-group-exchange-sha256, only on the client side, and make it opt-in.

I went through all the comments and issue references, and the only two that mention the SHA1 variant are the first one from 2016, and zgrab which is firmly out of scope.

If the server side implementation is needed for testing, it can live in a _test.go file.

(Does this allow us to drop diffie-hellman-group1-sha1 and diffie-hellman-group14-sha1 BTW?)

breml commented 5 years ago

@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 commented 5 years ago

@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 commented 5 years ago

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 commented 5 years ago

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 commented 4 years ago

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

ianlancetaylor commented 4 years ago

@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 commented 4 years ago

@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: [curve25519-sha256@libssh.org ecdh-sha2-nistp256 ecdh-sha2-nistp384 ecdh-sha2-nistp521 diffie-hellman-group14-sha1], server offered: [diffie-hellman-group-exchange-sha256

breml commented 4 years ago

@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 commented 4 years ago

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

sumiet commented 4 years ago

@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{
            "curve25519-sha256@libssh.org",
            "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 commented 4 years ago

@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 commented 4 years ago

Thanks @canni, That helped! :)

danfletcher1 commented 4 years ago

Thank you for adding that feature !! Works great.

KarthikNath commented 4 years ago

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 commented 4 years ago

@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 commented 4 years ago

@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.