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

crypto/tls: add DHE support #7758

Closed
gopherbot opened this issue Apr 10, 2014 · 20 comments
Closed

crypto/tls: add DHE support #7758

gopherbot opened this issue Apr 10, 2014 · 20 comments
Labels
FrozenDueToAge NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made.
Milestone

Comments

@gopherbot
Copy link

by stalkr:

ECDHE is supported but not DHE. It would be nice to support it so it could be added to
existing cipher suites.

Server https://developers.databox.com only support DHE ciphers:
DHE-RSA-AES256-GCM-SHA384
DHE-RSA-AES256-SHA256
DHE-RSA-AES256-SHA
DHE-RSA-CAMELLIA256-SHA
DHE-RSA-AES128-GCM-SHA256
DHE-RSA-AES128-SHA256
DHE-RSA-AES128-SHA
DHE-RSA-SEED-SHA

Which makes Go fail: Get https://developers.databox.com: remote error: handshake failure

Reference: https://groups.google.com/forum/#!topic/golang-nuts/hqm_ssUNUtI
@gopherbot
Copy link
Author

Comment 1 by frank@runscope.com:

I'm also seeing errors in the CA chain for 
Get https://api.moip.com.br/: x509: certificate signed by unknown authority (possibly
because of "x509: cannot verify signature: algorithm unimplemented" while trying to
verify candidate authority certificate "COMODO RSA Certification Authority")
It seems like everyone reissuing/updating certificates with the recent Heartbleed
announcement are getting new certificates that aren't fully supported by crypto/tls.

@gopherbot
Copy link
Author

Comment 2 by stalkr:

re #1: as replied on go-nuts this is a separate issue, crypto/sha512 is not imported by
default (see also https://golang.org/cl/84700045).

@gopherbot
Copy link
Author

Comment 4 by stalkr:

re #2: crypto/sha512 is now imported by default https://golang.org/cl/87670045

@ianlancetaylor
Copy link
Contributor

Comment 5:

Labels changed: added repo-main, release-none.

@bradfitz bradfitz removed the new label Dec 18, 2014
@rsc rsc added this to the Unplanned milestone Apr 10, 2015
@mstoykov
Copy link

mstoykov commented Feb 5, 2016

Is this being worked on?

Or does anyone know how to work around this?

@bradfitz
Copy link
Contributor

bradfitz commented Feb 5, 2016

@mstoykov, I would ask on the golang-nuts mailing list. But I don't know of anybody working on this.

@ghost
Copy link

ghost commented Jan 22, 2017

What's the status on this?

@bradfitz
Copy link
Contributor

@emansom, doesn't look like. Does this affect you? For which site?

@agl, is it your intention to ever implement this? If not, please close with a reason.

@bradfitz bradfitz modified the milestones: Go1.9Maybe, Unplanned Jan 22, 2017
@bradfitz bradfitz added the NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made. label Jan 22, 2017
@agl
Copy link
Contributor

agl commented Jan 23, 2017

DHE is slow, has compatibility issues over 1024 bits and is getting removed in browsers. No plans to support it.

@agl agl closed this as completed Jan 23, 2017
@mark-kubacki
Copy link

mark-kubacki commented Jan 24, 2017

For sites and networks where nistp* curves are banned, it's either DHE or implementing one of the alternative curves or curve families.

@mstoykov If you still need DHE support, drop me a line. I can point you to vendors that share (sell) customizations.

@mstoykov
Copy link

@wmark Thanks, but as this was an internal (apache) server we just reconfigured it to support modern algorithms :)

@mordyovits
Copy link

I have implemented the DHE ciphersuites (well, just the AES ones) in my fork of crypto/tls . It works well, and includes both client and server support for:
TLS_DHE_RSA_WITH_AES_256_GCM_SHA384
TLS_DHE_RSA_WITH_AES_128_GCM_SHA256
TLS_DHE_RSA_WITH_AES_256_CBC_SHA256
TLS_DHE_RSA_WITH_AES_128_CBC_SHA256
TLS_DHE_RSA_WITH_AES_256_CBC_SHA
TLS_DHE_RSA_WITH_AES_128_CBC_SHA

(Why only RSA? Because if you have an EC server cert you probably will just use ECDH. Right?)

I don't yet know how to go about getting it merged into mainline, but before I go through the effort to learn, is there any official interesting in including it?

Some arguments in its favor:

  1. It's needed by some people. Me, for example, which is why I wrote it and why tickets such as this one were opened.

  2. It's default off (flag suiteDefaultOff). Any client or server code that does not explicitly set Config.CipherSuites to include the DHE ciphers will never use it.

  3. It's low priority, just above the (also default off) RC4.

Thanks. I'd really love the chance to have my code reviewed and to contribute this functionality.

@bradfitz
Copy link
Contributor

@mordyovits, this issue was already closed with a decision by @agl.

@mordyovits
Copy link

mordyovits commented May 18, 2017

@bradfitz @agl Can that be reconsidered? The code is written and working. The added ciphers default off. That means there currently exists zero Go code in world that when compiled would cause DHE to be used, since no code could have explicitly configured it. To my mind, that's a good argument: only future code that goes out of its way to ask for it will ever use it.

The issues with DHE are compatibility and speed, not security (given acceptable key size). Therefore, I think default off DHE ciphersuites should be acceptable.

@mordyovits
Copy link

Also, I've added PSK and DHE_PSK ciphersuites. I'd like to have those considered for inclusion, but I think that should be its own issue, because it required deeper surgery to remove assumptions about there always being a server cert.

@bradfitz
Copy link
Contributor

There's also an ongoing maintenance cost (refactoring, security) and binary size you didn't include in your list of counterarguments.

@mordyovits
Copy link

Sure, but the point of crypto/tls is to implement TLS for people who use Go. People who want to use these ciphers in Go will have to go elsewhere. Not everything is Chrome or a Google webserver. Why not make Go usable for as many kinds of things as possible? Especially since the code is structured so that no one can expose it by accident.

As for maintenance & binary size, it's a small patch. The diffstat for everything including DHE, PSK and DHE_PSK, but not _test is:

alert.go | 2
cipher_suites.go | 87 ++++++-
common.go | 28 ++
handshake_client.go | 129 ++++++-----
handshake_server.go | 75 +++---
key_agreement.go | 598 +++++++++++++++++++++++++++++++++++++
tls.go | 71 +++++-

Thanks for considering it.

@mordyovits
Copy link

You can find my fork with DHE ciphersuite support here:
https://github.com/mordyovits/golang-crypto-tls

@mark-kubacki
Copy link

mark-kubacki commented Jun 6, 2017

@mordyovits Reading your code for literary one minute I have spotted two flaws, in key_agreement.go: Not mitigating the small subgroup attack; not checking if the agreed-upon value is large enough.

@mordyovits
Copy link

@wmark Thanks! I've emailed you to discuss it.

@golang golang locked and limited conversation to collaborators Jun 6, 2018
@rsc rsc unassigned agl Jun 23, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
FrozenDueToAge NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made.
Projects
None yet
Development

No branches or pull requests

8 participants