Closed felixbuenemann closed 8 years ago
Thanks for the contribution! Please ensure your commits follow our style guide. This code will be tested once a Deis maintainer reviews it.
@krancour, @kmala and @helgi are potential reviewers of this pull request based on my analysis of git blame
information. Thanks @felixbuenemann!
@krancour should probably look at this.
Regarding the breaking change: Java 6 clients might actually still work, even when strong DH params are used. According to SSL-Labs checks Java6 connects fine, but without forward secrecy. The same cipher suite on a server with openssl 1.0.1 leads to a connection error for Java6, so this might have been fixed in openssl 1.0.2.
IE6 on XP was already unable to connect with the default openssl ciphers, so there's no change regarding that.
Btw. my local build environment is currently broken and I can't figure out why, so I wasn't able to verify that the above change actually ends up in the nginx.conf
, so it would be great if one of the reviewers could check that.
@sstarcher You could try this out, if you like.
@felixbuenemann I'll give it a try tomorrow
Great, I'm probably off for today as well, it's pretty late here.
@@ master #231 diff @@
==========================================
Files 3 3
Lines 303 304 +1
Methods 0 0
Messages 0 0
Branches 0 0
==========================================
+ Hits 139 140 +1
Misses 160 160
Partials 4 4
Powered by Codecov. Last update 90c8bd1...c00da10
@felixbuenemann the documented default list of ciphers should be updated in the annotations table in the readme.
@krancour Updated the default value in the README.
@felixbuenemann knowing now that we did ship with a 2048 bit dhparam by default... can you update the comment about the breaking change in the OP accordingly? The breakage would be more widespread in light of this knowledge. Also, do you know how that impacts older Android clients?
I am less concerned about IE6 on XP-- mainly because that combo doesn't support SNI anyway, which I think made it incompatible with HTTPS on our router to begin with.
I'm going to run this branch through Qualys SSL Labs, just for my own edification.
@krancour Well, I can't quite make sense of the Java6 handshake results.
According to the SSL-Labs checks it is connecting with TLS_RSA_WITH_AES_128_CBC_SHA
on the deis router with the given cipher list, but the expected behavior would be for it to fail with Client does not support DH parameters > 1024 bits
when trying to negotiate TLS_DHE_RSA_WITH_AES_128_CBC_SHA
, which it does on both an Ubuntu Trusty Server with Nginx 1.10.1 (openssl 1.0.1f) and on an Ubuntu Xenial Server with Nginx 1.10.1 (openssl 1.0.2g-fips) with the same cipher suite.
There is another difference in that on both of the Ubuntu systems Andoird 2.3.7 connects using TLS_DHE_RSA_WITH_AES_128_CBC_SHA
with Forward Secrecy, but on the deis router it uses TLS_RSA_WITH_AES_128_CBC_SHA
instead.
Do you know which exact OS the deis baseimage is based on?
Maybe they are using a patch to fix the Java6 incompatibility, that has the side-effect of breaking FS for Android 2.3.
@felixbuenemann this is what we start from:
https://github.com/kubernetes/contrib/tree/master/images/ubuntu-slim
That is the source of the gcr.io/google_containers/ubuntu-slim:0.3
image that the deis/docker-base builds upon.
Also note that there may be some unintended side-effects to this PR. Comparing the results from Qualys SSL Labs with and without this PR, I notice that 128 bit encryption is now used in many places where 256 bit encryption was used previously.
That is intended, the cipher list explicitly prefers 128-Bit and there are some good explanations in the Mozilla Wiki why 128-Bit should be preferred for now:
ChaCha20 is prefered as the fastest and safest in-software cipher, followed but AES128. Unlike the modern configuration, we do not assume clients support AESNI and thus do not prioritize AES256 above 128 and ChaCha20. There has been discussions (1, 2) on whether AES256 extra security was worth its computing cost in software (without AESNI), and the results are far from obvious. At the moment, AES128 is preferred, because it provides good security, is really fast, and seems to be more resistant to timing attacks. DES-CBC3-SHA and EDH-RSA-DES-CBC3-SHA are maintained for backward compatibility with clients that do not support AES.
Ignore the above mentions of ChaChaPoly, it is not supported by openssl 1.0.2 without custom patches and not included in our cipher list. It is also more expensive for servers, because most servers have AESNI instructions to compute AES128 and AES256 in hardware.
I have updated the commit message and description to specify the preferred order and dropped the "BREAKING CHANGES" footer, because the previous cipher list already excluded Java6 clients (specifically it preferred EDH over RSA auth and used a 2048-Bit DH param and Java6 only supports DH params up to 1024-Bit).
@felixbuenemann we're getting closer on this.
I am noticing a lot of CBC, even combined with TLS 1.0. I'm under the impression that TLS 1.0's use of MAC-then-encrypt combined with CBC creates a vulnerability to attacks like Lucky12 or BEAST.
To be fair on two points, this was the case before your PR as well and SSL Labs isn't dinging us on it. I'm wondering though if it makes sense to exclude the use of CBC.
@felixbuenemann while my curiosity re: the previous question has finally compelled me to buy Bulletproof SSL for my own edification, there's been some internal discussion on this PR that's led us to the following...
Your OP says you're using Mozilla's intermediate compatibility recommendations "with small modifications to make the list more compact."
router.deis.io/nginx.ssl.ciphers
annotation. Each of those should be expanded within the model. All other values should be treated as they currently are-- a custom list of ciphers. The documentation should be amended to convey this behavior and link to Mozilla's recommendations.Would you care to amend this PR to address no. 1 and no. 2 above? Alternatively, I can open a new PR if you haven't got the bandwidth for those changes.
old, intermediate, modern are from - https://wiki.mozilla.org/Security/Server_Side_TLS#Recommended_configurations magic - is ssl_prefer_server_ciphers on?
With Deis SNI issues will still cause problems with old correct?
@sstarcher You can only use SSL virtual hosts with SNI, non SNI clients will connect to the vhost that is marked as default_server
in the listen
directive.
@felixbuenemann yep, I understand. We run several different apps over several different wildcard SSL certs. In my case SNI is required so Java 6 and XP will fail for me even in the old
configuration.
@krancour I don't think the profiles are that useful, if only applied to the cipher list.
The Mozilla profiles define a whole set of SSL configuration parameters, like the ciphersuite, protocol versions, TLS curves, certificate types, HSTS and so on.
So using only the ciphersuite while ignoring all the other related parameters doesn't make much sense to me. If you go with profiles, there should be an option like SSLProfile
which switches between three pre-baked SSLConfigs
named old, intermediate and modern that set all the parameters dictated by the mozilla profiles.
IMHO this is overkill and the ciphersuite from the intermediate profile is good for the vast majority of use cases and those that do need something different should be able to figure out the parameters to set. Unless you have very specific requirements, neither the old nor the modern profile is currently very useful. The old profile includes insecure protocols and ciphers and the modern profile excludes a lot of common clients including some search engines without any particular gain in security.
If you do want to go the profile way, the mozilla recommended configurations are available as a JSON document on github, so they could be consumed as a build step
https://github.com/mozilla/server-side-tls/blob/gh-pages/json/server-side-tls-conf.json
@krancour Maybe we could go with a single set of defaults for now, I don't care is my slimmed down version or the verbatim version from the Mozilla intermediate profile and keep the profiles feature as an enhancedment for a separate PR, given that it is much larger in scope.
What do you think?
The Mozilla profiles define a whole set of SSL configuration parameters, like the ciphersuite, protocol versions, TLS curves, certificate types, HSTS and so on.
That's true, but if you use this tool and diff the generated examples, the only differences are isolated to just two fields: ssl_protocols
and ssl_ciphers
. That's not too thorny to deal with.
I do believe the profiles would be a valuable feature. The big advantage would be to operators who lack sufficient technical knowledge to curate their own custom cipher list. They would still be able to choose from three reasonable, pre-established, and documented points on he compatibility v security spectrum.
But you are right that this could or should be a separate PR.
If we can just get the cipher suite to be exactly as Mozilla currently defines it for "intermediate," then I think this will be satisfactory.
I have updated the ciphers to match the mozilla list exactly. Note that the list suggests we can use ChaChaPoly but it won't be used because it's not supported by OpenSSL 1.0.2.
@felixbuenemann we should probably link to https://wiki.mozilla.org/Security/Server_Side_TLS in the docs.
I have added a link to the Wiki.
Note that the table becomes absolutely unreadable due to the long unwrapped string.
Any idea how to fix that? That is one of the reasons I used an abbreviated form previously.
I wouldn't worry about it. The table was already wider than the surrounding viewport and this doesn't look like it makes that situation any worse.
Yeah, the information is not well suited to tables.
Anything else needed for this to get it merged?
Yes, LGTM2 label :-)
@felixbuenemann haha I guess I can't help you on that
@felixbuenemann if you or someone else is willing to keep this cipher list up to date then I'm ok with this. I have the same reservations as @krancour has in https://github.com/deis/router/issues/230#issuecomment-239021455. This is a burden we must now maintain, should we choose to accept this PR over the default cipher list.
Someone told me that this is already going to be handled through a check-in calendar event every 3 months or so. Leaving it to @krancour to confirm/merge
@bacongobbler Yes, that's why we switched to the exact list from the Mozilla Wiki. As mentioned before it would also be possible to update the list during build from the JSON resource linked above.
This is on our team calendar for the 1st of every month.
This specifies a default cipher list instead of relying on the defaults used by openssl, which are not tailored to https and cause issues with HTTP/2 in Firefox due to poor cipher order.
The defaults are chosen to be secure, fast and compatible with current clients, but exclude IE6 on XP and Java6 from connecting.
The list was built using current recommendation from the Mozilla Wiki at https://wiki.mozilla.org/Security/Server_Side_TLS for intermediate compatibility
with small modifications to make the list more compact.Ciphers are preferred in this order:
Fixes #229 #230