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

proposal: crypto/tls: support DHE #31933

Closed
swanandt opened this issue May 9, 2019 · 16 comments
Closed

proposal: crypto/tls: support DHE #31933

swanandt opened this issue May 9, 2019 · 16 comments
Labels
FrozenDueToAge Proposal Proposal-Crypto Proposal related to crypto packages or other security issues
Milestone

Comments

@swanandt
Copy link

swanandt commented May 9, 2019

What version of Go are you using (go version)?

$ go version
go1.9.2

Does this issue reproduce with the latest release?

Yes

What operating system and processor architecture are you using (go env)?

go env Output
$ go env

linux/amd64

What did you do?

Work for NOKIA and we are planning to develop API Gateway with go for Telecom Apps.

What did you expect to see?

We want that go-lang to support following cipher suites as well. I have read one reply that DHE is slow and support is dis-continued in browser but I think now go is not only about browser but because of cloud-native drive even telecom clouds will use REST based apps and I see that go plays important role as it has enriched eco-system. The traditional model use openssl and it supports all these ciphers already. I want to see go-lang also be in par with openssl as it shall not just about Browsers anymore :-)

For Telco Cloud Apps these are still good to have instead of just only ECDHE as to be feature-Parity. I assume K8S [ which also seems in go ] and creating foot-prints in telco cloud having such support in golang is not only good but also appears more like "shall have " :-)

TLS_DHE_RSA_WITH_AES_128_GCM_SHA256 (DH 2048)
TLS_DHE_RSA_WITH_AES_128_CBC_SHA256 (DH 2048)
TLS_DHE_RSA_WITH_AES_128_CBC_SHA (DH 2048)
TLS_DHE_RSA_WITH_AES_256_GCM_SHA384 (DH 2048)
TLS_DHE_RSA_WITH_AES_256_CBC_SHA256 (DH 2048)
TLS_DHE_RSA_WITH_AES_256_CBC_SHA (DH 2048)
TLS_RSA_WITH_AES_256_CBC_SHA256 (RSA 2048)

What did you see instead?

No Support for DHE

@dominikh
Copy link
Member

dominikh commented May 9, 2019

The traditional model use openssl and it supports all these ciphers already. I want to see go-lang also be in par with openssl as it shall not just about Browsers anymore

For Telco Cloud Apps these are still good to have instead of just only ECDHE as to be feature-Parity

Feature parity in itself isn't a goal. What stops "Telco Cloud" from using modern ciphers instead?

@swanandt
Copy link
Author

swanandt commented May 9, 2019 via email

@jamie-digital
Copy link

OpenSSL has historically implemented almost every available feature, which has resulted in a bloated and complex implementation with several vulnerabilities. Go is taking the approach of implementing the (slightly more than minimum) necessary functionality to be useful so that the codebase is reasonably easy to understand and thus much less likely to be vulnerable.

DHE as implemented in TLS is a massive footgun, as there's no enforced standardisation of groups. I think there would need to be a pretty compelling reason to add new functionality which is arguably less secure than what's already present.

@swanandt
Copy link
Author

swanandt commented May 10, 2019

Hi

The more compelling reason, I can think is ROI for each customer who has vast volumes of deployment. If the new solution completely makes them unusable then it is big business loss to them and cant compel upgrade if they dont wish to . For telco eco-system it is more a "business loss"

I have few questions now
Q: If no TLS Config is not passed to go-lang net/http Server which uses ListenAndServeTLS, will it accept the client connection with ciphers not mentioned in cipher_suites ?

Q: If we still wish to add support for
DHE-RSA-AES256-GCM-SHA384
DHE-RSA-AES256-SHA256
DHE-RSA-AES256-SHA
AES256-GCM-SHA384
AES256-SHA256
AES256-SHA
DHE-RSA-AES128-GCM-SHA256
DHE-RSA-AES128-SHA256
DHE-RSA-AES128-SHA
AES128-GCM-SHA256
AES128-SHA256
AES128-SHA

What does it mean ? only adding those constants and test stub for it OR
It means complex implementation for these apart from what you already have in crypto/tls ?

Q: As I mentioned about the business case, do you have any suggestion what is best way to do it if we still wish to lock on crypto/tls or move to other fork ?

Q: Also if we set following flag to false will the clients preferred list will be used and basically override default ciphers-suite of Server which is using crypto/tls package ? Can using this flag we can support what I have mentioned above ?
PreferServerCipherSuites

Thanks
Swan

@swanandt
Copy link
Author

Any Comments on above questions ?? Can we use external TLS stack with net/http ?

@jamie-digital
Copy link

Provided the external TLS stack has the same overall behaviour (transparently and automatically performing a handshake) and has a net.Listener interface then you can use it easily enough with a server:

srv := &http.Server{ /* ... */ }
ln, err := tls.Listen( /* ... */ )
if err != nil { /* handle error */ }
srv.Serve(ln)

Supporting an external TLS stack with an HTTP client would be a little more involved but you'd essentially create a new http.Transport and override DialTLS with a function dialing using the TLS package.

@swanandt
Copy link
Author

Our source Code is something like

 s.server = &http.Server{ Addr:         address, Handler:      handler,
	ReadTimeout:  s.globalConfig.RespondingTimeouts.ReadTimeout,
	WriteTimeout: s.globalConfig.RespondingTimeouts.WriteTimeout,
	IdleTimeout:  s.globalConfig.RespondingTimeouts.IdleTimeout,
}
listener, err := net.Listen("tcp4", address
....
...
s.server.ServeTLS(listener, s.globalConfig.TLS.CertFile, s.globalConfig.TLS.KeyFile)

We are not using tls.Config which is part of http Server Structure member.

Q: If in server instantiation if we dont use tls.Config will it still using go-lang specific crypto implementation in ServeTLS code ?
Q: In the crypto/tls package there is a note " Package tls partially implements TLS 1.2, as specified in RFC 5246" What are exceptions from RFC 5246 which is not supported by this package ??

Quick search results in:
https://godoc.org/github.com/spacemonkeygo/openssl#NewListener

Looks like this can be used as wrapper around if we wish to use openssl TLS stack.

@andybons andybons changed the title Support DHE in Crypto For Telco Cloud proposal: crypto/tls: support DHE May 13, 2019
@gopherbot gopherbot added this to the Proposal milestone May 13, 2019
@andybons andybons added the Proposal-Crypto Proposal related to crypto packages or other security issues label May 13, 2019
@andybons andybons modified the milestones: Proposal, Unplanned May 13, 2019
@andybons
Copy link
Member

@FiloSottile

@FiloSottile
Copy link
Contributor

We are very unlikely to implement finite field DH in crypto/tls for the reasons mentioned by @jamie-digital. It's a bad design, very hard to implement securely and in constant time, and superseded by ECDHE. I am also unaware of any clients that support DHE but not the plain RSA ciphers, which we carry along for compatibility (and so we can avoid adding the complexity of things like DHE instead). Feature parity is explicitly a non-goal of crypto/tls.

I am going to leave this open to collect feedback, but it's not at all planned.

@swanandt
Copy link
Author

What about my other question ?

"Q: In the crypto/tls package there is a note " Package tls partially implements TLS 1.2, as specified in RFC 5246" What are exceptions from RFC 5246 which is not supported by this package ??"

Can you please comment what are the exceptions this package does not support from RFC 5246 compliance !!!

@FiloSottile
Copy link
Contributor

As far as I know, we implement everything that is REQUIRED by RFC 5246, so we are in full compliance with it. There is no list of what's not implemented, but what is implemented is documented at https://golang.org/pkg/crypto/tls.

If you have other questions unrelated to this proposal, please open a new issue or see https://golang.org/wiki/Questions.

@swanandt
Copy link
Author

Thanks for quick response, I would then suggest it is better to change the documentation to reflect it else it's kind of misleading as it is first thing be read in the Overview Section of this package documentation itself

@jamie-digital
Copy link

I think it's fair to say that the package "partially" implements TLS 1.2 in that it doesn't implement the entirety of RFC 5246. There are optional features left unimplemented, such as the DHE key exchange (which is why we're having this discussion), server-initiated renegotiation, signatures using MD-5, and so on.

@ALTree
Copy link
Member

ALTree commented May 16, 2019

I would then suggest it is better to change the documentation to reflect it else it's kind of misleading as it is first thing be read in the Overview Section of this package documentation itself

Thanks for the suggestion. I've opened issue #32079 to track this.

@rsc
Copy link
Contributor

rsc commented Mar 25, 2020

Based on the discussion above and the lack of activity, this seems like a likely decline.

@rsc
Copy link
Contributor

rsc commented Apr 1, 2020

No change in consensus, so declining.

@rsc rsc closed this as completed Apr 1, 2020
@rsc rsc moved this from Likely Decline to Declined in Proposals (old) Apr 1, 2020
@golang golang locked and limited conversation to collaborators Apr 1, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
FrozenDueToAge Proposal Proposal-Crypto Proposal related to crypto packages or other security issues
Projects
No open projects
Development

No branches or pull requests

8 participants