-
Notifications
You must be signed in to change notification settings - Fork 209
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
Update TLS Ciphers. #918
Update TLS Ciphers. #918
Conversation
Do you know which ones have been removed? Hard to see in the diff. |
Here is a side-by-side diff (unified was harder to read in my opinion due to offsets) -- the left side is the old settings and the right side is the new.
|
As a side note, we may want to consider enabling HSTS but it's a much bigger change to the config and may require changes in omnibus-software. |
Thanks for that split @rhass, let me dig into that - I wanted to verify that this isn't going to cause problems with our dated rest-client fork used by oc-chef-pedant. There's something in the back of my head from when we dropped SSLv3. I'm almost positive it was just for the protocol and not the cipher, but I wanted to double check. Will post back here in the a.m. |
Overall I'm 👍 on moving to a more modern list. Here are some random thoughts:
|
DES-CBC3-SHA needs to not be in the new list. |
i would honestly say that we prioritize mitigating attack surface over compatibility and ship the modern version of the ciphersuite, and then adjust downwards if anyone actually complains. we're currently cargo culting forwards my settings which just a wildassed guess at the time about browser compatibility, we've had zero customer input over what our settings should be. since the presence of DES-CBC3-SHA in this list makes us vulnerable to SWEET32 i think now is a good time to rethink that posture. |
aa6e64a
to
ec42b39
Compare
@lamont-granquist @stevendanna I updated the suite to the modern recommendations. |
Travis seems to be unhappy about something unrelated with installing ruby. |
That's been happening at intervals, rerunning should clear it. |
Now its failing to build gecode. =/ |
ec42b39
to
1862dfa
Compare
Update the recommended TLS protocol and ciphers suggested by Mozilla with support for modern browsers. Oldest compatible clients : Firefox 27, Chrome 30, IE 11 on Windows 7, Edge, Opera 17, Safari 9, Android 5.0, and Java 8 https://mozilla.github.io/server-side-tls/ssl-config-generator/?server=nginx-1.8.1&openssl=1.0.1u&hsts=no&profile=modern
1862dfa
to
ec8a5e2
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The overall opinion in chat was that this is overall the correct path forward. If we want to do this, let's not delay.
Users who may need to update their settings after an upgrade:
- Users on ancient old chef-10 omnibus builds or using chef installed from gems on old platforms
- "FIPS" users
- Users with clients connecting to Manage using very old web browsers.
👍 lets do it. document for users how to fix it, if anyone has compat issues. otherwise, lets be more secure by default. |
@chef/chef-server-maintainers Can another server maintainer review this? |
Just poking @chef/chef-server-maintainers one more time. |
👍 MWR |
FYI - this completely breaks all reporting pedant tests.
|
did we just discover that reporting has an ancient version of openssl or an ancient client-side configuration that we need to fix? or can you get the output of analyze.pl here against the chef-server: |
Thanks @lamont, that sets us down the right path - reporting pedant defaults to TLSv1. |
https://github.com/chef/oc_reporting/pull/297 should fix it. |
Update the recommended TLS protocol and ciphers suggested by Mozilla
with support for modern browsers.
Oldest compatible clients : Firefox 27, Chrome 30, IE 11 on Windows 7,
Edge, Opera 17, Safari 9, Android 5.0, and Java 8
https://mozilla.github.io/server-side-tls/ssl-config-generator/?server=nginx-1.8.1&openssl=1.0.1u&hsts=no&profile=modern