restrict TLS cipher suites of the server #5245
Conversation
pkg/http/server.go
Outdated
| @@ -155,8 +155,18 @@ func NewServer(addrs []string, handler http.Handler, certificate *tls.Certificat | |||
| if certificate != nil { | |||
| tlsConfig = &tls.Config{ | |||
| PreferServerCipherSuites: true, | |||
| MinVersion: tls.VersionTLS12, | |||
| NextProtos: []string{"http/1.1", "h2"}, | |||
| CipherSuites: []uint16{ | |||
There was a problem hiding this comment.
We should take this list as perhaps an argument to NewServer() function, in future we might need to change these as well. We can take this as an input from top level and not default/restrict this package to the following ciphers.
Codecov Report
@@ Coverage Diff @@
## master #5245 +/- ##
=========================================
+ Coverage 61.12% 61.43% +0.3%
=========================================
Files 201 201
Lines 29509 29303 -206
=========================================
- Hits 18038 18002 -36
+ Misses 10083 9908 -175
- Partials 1388 1393 +5
Continue to review full report at Codecov.
|
aead
force-pushed
the
tls-ciphersuite
branch
from
d60da39
to
2d6d9b0
Compare
Some counter measures implemented golang/go@f28cf83 Refer here golang/go#13385 Disabling CBC for 128bit and above can have real issues with clients like CloudBerry which was using CBC. Historically we had this in the past https://github.com/minio/minio/pull/3608/files#diff-e6845802830d0358dfc98e01152842c2R379 Thus we had to bring back CBC to support it, so we should validate those clients before accepting this patch and also have a thorough review such that we are not too strict about this. |
|
Oh okay, wasn't aware that Filippo fixed some CBC timing issues. So we can "safely" enable CBC-*-SHA suites. However SHA256 suites are still vulnerable. Agree that we have to check clients. |
deekoder
requested review from
donatello and
harshavardhana
and removed request for
balamurugana
|
Disabling RSA PKCS v1.5 ciphers seems to make no difference. However it's theoretically possible that there are clients which does not support ECC... As far as I can see disabling RSA-only ciphers doesn't break anything I'm aware of... |
cmd/globals.go
Outdated
| @@ -167,6 +167,16 @@ var ( | |||
| colorYellow = color.New(color.FgYellow).SprintfFunc() | |||
| ) | |||
|
|
|||
| // Secure Go implementations of TLS ciphers | |||
| var tlsCipherSuites = []uint16{ | |||
There was a problem hiding this comment.
If this is more secure TLS configuration, how about making this default cipher suite for http server in case of TLS enabled? You always has an option to override this default behavior.
There was a problem hiding this comment.
Don't understand - what do you mean with "making this the default cipher suites". These ciphers are used as the "default" cipher suites for the server and the gateway. Or do I miss something?
See https://github.com/minio/minio/pull/5245/files#diff-736572b438aa05056f334265beb9507bR158
There was a problem hiding this comment.
My suggestion is below
- move
tlsCipherSuitestopkg/http/server.goitself i.e. no definition incmd/*.go - there is no
cipherSuitesargument toNewServer()i.e.tlsCipherSuitesis set always for TLS. - if user wants to override this, he could directly set
Server.TLSConfigaccordingly to his need. - this way there is no change in
cmd/*.go
There was a problem hiding this comment.
That's exactly how the patch looked before but harsha mentioned:
We should take this list as perhaps an argument to NewServer() function, in future we might need to change these as well. We can take this as an input from top level and not default/restrict this package to the following ciphers.
There was a problem hiding this comment.
@harshavardhana As the approach provides support to the use case you mentioned, do you have any other concern to do the change?
There was a problem hiding this comment.
That won't be a problem, since the TLSConfig can be overriden. I guess having a default is fine for now.
aead
force-pushed
the
tls-ciphersuite
branch
from
b027bf4
to
f4a77a7
Compare
|
Okay, reverted changes according to @balamurugana comment. |
aead
force-pushed
the
tls-ciphersuite
branch
from
f4a77a7
to
7a73f63
Compare
There was a problem hiding this comment.
GIt commit message here still says we are allowing following ciphers but code is changed can you amend your commit as well @aead ?
- tls.TLS_RSA_WITH_AES_128_GCM_SHA256
- tls.TLS_RSA_WITH_AES_256_GCM_SHA384
There was a problem hiding this comment.
A typo s/restircts/restricts/
because the Go implementation of CBC does not contain effective countermeasures against timing attacks.
Do we still need to mention this? @aead
Further disable RSA PKCSv1.5 cipher suites because of timing issues as shown with the ROBOT attack.
This sentence is not needed perhaps. Unless you want to really say why we even removed RSA_AES support.
| // NewServer - creates new HTTP server using given arguments. | ||
| func NewServer(addrs []string, handler http.Handler, certificate *tls.Certificate) *Server { | ||
| var tlsConfig *tls.Config | ||
| if certificate != nil { | ||
| tlsConfig = &tls.Config{ | ||
| PreferServerCipherSuites: true, | ||
| CipherSuites: defaultCipherSuites, |
There was a problem hiding this comment.
could you add a unit tests to address below cases for default http server and TLSConfig'ed http server?
- http client connecting with
defaultCipherSuites - http client connecting without
defaultCipherSuites - http client connecting with unsupported cipher
aead
force-pushed
the
tls-ciphersuite
branch
from
7a73f63
to
828eb61
Compare
|
@harshavardhana BTW: I disabled RSA-PKCS1-v1.5 not because of ROBOT itself (Go stack is not vulnerable to ROBOT) |
aead
force-pushed
the
tls-ciphersuite
branch
2 times, most recently
from
7da2406
to
9940668
Compare
|
@aead You go ahead and remove unit tests added in this PR. I will send a separate PR to address unit tests. |
|
@balamurugana Have added tests - Can you take a look and if something is missing from your point of view can you address? |
|
@aead My previous comment Let me know I can send a PR with the fix to your fork |
…-tests Fix unit tests to follow style used in minio
|
@harshavardhana @donatello |
Mint Automation
5245-ce07746/mint-gateway-azure.sh.log:5245-ce07746/mint-fs.sh.log:5245-ce07746/mint-xl.sh.log:5245-ce07746/mint-gateway-s3.sh.log:5245-ce07746/mint-dist-xl.sh.log: |
|
This patch has broken CloudBerry support for Minio since CloudBerry clients are now unable to initialise a TLS connection. [2018-02-13T10:25:09.295333193Z] [ERROR] TLS handshake failed with new connection CLIENTIP:PORT at server SERVERIP:PORT (tls: no cipher suite supported by both client and server) |
|
@gowithplanb |
|
I'm using the latest stable version 5.8 for MBS customers (similar to the one publically available). I've captured traffic between client and server, and this is what the client offers: Secure Sockets Layer Minio then responds with: Secure Sockets Layer |
|
Thanks @gowithplanb I guess you are using a RSA certificate and the client does not support The problem is that the Go TLS stack only contains timing attack countermeasures for |
|
@aead I have contact with CloudBerry about this too now. They're informed about it and are looking into it as well. I will not upgrade Minio in production until this is resolved. |
Great, this is actually a Cloudberry configuration issue. The correct way to fix that is:
You can point them also to this PR thread so they can reach out if the need more information. |
|
Thanks @aead! PR pointed out to them. |
|
Is it just me or does this PR also prevent Windows 8.1 / Windows Server 2012 R2 from connecting to Minio? IE on Windows Server 2012 R2 just reports
when all of these TLS versions are enabled in IE. |
|
@topeju
So my guess from the reported msg would be that TLS 1.2 is not enabled on your Windows 8.1 / Windows Server 2012 R2 machine. Further I don't think that this PR causes the problem because according to the ms doc the ms TLS 1.2 stack supports the named cipher suites. So can you try to enable TLS 1.2 and check if the issue still exists? (on all participating machines) |
|
No, TLS 1.2 is (and was) already enabled. When I check it with |
|
The linked ms doc lists:
However it seems Windows 8.1 does not support AES-GCM with ECDHE_RSA. Since your BTW: You were right - this is exactly the same issue reported using Cloudberry above. |
|
Yes, I'm using an RSA private key. I tried to generate an ECDSA key using |
|
Oh you are right - I'm sorry about that. I will fix the documentation. In the meantime you can use the following openssl command: |
|
OK, so I just wasted a couple of days trying to get a Let's Encrypt ECC key generated using acme.sh to work with Minio, but I kept getting the message "ERROR Unable to load the TLS configuration: The private key contains additional data. Please check your certificate.". Turns out that for some reason I need to translate the private key by adapting the latter part of the call described above, i.e. if I call "openssl -in .acme.sh/domain/private.key -out .minio/certs/private.key" it'll work. Now, is there any logic behind the fact that I can use a 4096 bit RSA key straight off, but I need to do some kind of translation in order to use a prime256v1 ECC key? What am I missing here...? |
|
@mhjartstrom I am opening a new issue with your comment, please add additional details there. Also in future open new issues rather can opening older threads. It makes it easier for us to get to the bottom of this with more details and context. |
Description
This change restircts the supported cipher suites of the minio server.
The server only supports AEAD ciphers (Chacha20Poly1305 and AES-GCM)
The supported cipher suites are:
Motivation and Context
Fixes #5244
Fixes #5291
How Has This Been Tested?
manually
Types of changes
Checklist:
mintPR # here: )