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
Assignees
Milestone

Comments

@gopherbot
Copy link

@gopherbot gopherbot commented Apr 10, 2014

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

This comment has been minimized.

Copy link
Author

@gopherbot gopherbot commented Apr 14, 2014

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

This comment has been minimized.

Copy link
Author

@gopherbot gopherbot commented Apr 14, 2014

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

This comment has been minimized.

Copy link
Author

@gopherbot gopherbot commented Apr 14, 2014

Comment 4 by stalkr:

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

This comment has been minimized.

Copy link
Contributor

@ianlancetaylor ianlancetaylor commented May 9, 2014

Comment 5:

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

@gopherbot gopherbot added new labels May 9, 2014
@bradfitz bradfitz removed the new label Dec 18, 2014
@rsc rsc added this to the Unplanned milestone Apr 10, 2015
@rsc rsc removed release-none labels Apr 10, 2015
@tcz001 tcz001 mentioned this issue Jan 4, 2016
9 of 9 tasks complete
@MStoykov

This comment has been minimized.

Copy link

@MStoykov MStoykov commented Feb 5, 2016

Is this being worked on?

Or does anyone know how to work around this?

@bradfitz

This comment has been minimized.

Copy link
Contributor

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

This comment has been minimized.

Copy link

@ghost ghost commented Jan 22, 2017

What's the status on this?

@bradfitz

This comment has been minimized.

Copy link
Contributor

@bradfitz bradfitz commented Jan 22, 2017

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

@agl

This comment has been minimized.

Copy link
Contributor

@agl 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 Jan 23, 2017
@wmark

This comment has been minimized.

Copy link

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

This comment has been minimized.

Copy link

@MStoykov MStoykov commented Jan 24, 2017

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

@mordyovits

This comment has been minimized.

Copy link

@mordyovits mordyovits commented May 11, 2017

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

This comment has been minimized.

Copy link
Contributor

@bradfitz bradfitz commented May 13, 2017

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

@mordyovits

This comment has been minimized.

Copy link

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

This comment has been minimized.

Copy link

@mordyovits mordyovits commented May 18, 2017

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

This comment has been minimized.

Copy link
Contributor

@bradfitz bradfitz commented May 18, 2017

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

@mordyovits

This comment has been minimized.

Copy link

@mordyovits mordyovits commented May 18, 2017

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

This comment has been minimized.

Copy link

@mordyovits mordyovits commented Jun 6, 2017

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

@wmark

This comment has been minimized.

Copy link

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

This comment has been minimized.

Copy link

@mordyovits mordyovits commented Jun 6, 2017

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

@golang golang locked and limited conversation to collaborators Jun 6, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
8 participants
You can’t perform that action at this time.