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

SSL cipher changes (issue 859) #909

Merged
merged 1 commit into from
Oct 16, 2016
Merged

SSL cipher changes (issue 859) #909

merged 1 commit into from
Oct 16, 2016

Conversation

wyardley
Copy link
Collaborator

@wyardley wyardley commented Oct 8, 2016

This is my first stab at updating the SSL cipher list (re: #859)
I suppose we can't / shouldn't disable TLS 1.0 yet?

@wyardley wyardley mentioned this pull request Oct 8, 2016
@ekohl
Copy link
Member

ekohl commented Oct 8, 2016

https://mozilla.github.io/server-side-tls/ssl-config-generator/ still enables TLSv1 on the intermediate profile which I think is a good default for the module.

Have you checked this against https://www.ssllabs.com/ as well? Did it give an A rating?

@wyardley
Copy link
Collaborator Author

wyardley commented Oct 8, 2016

@ekohl: I didn't have an easy and quick way to expose a test box using the Puppet generated config to the network... Would love it if someone has time to test, but if not, I can come up with something early next week.

@wyardley
Copy link
Collaborator Author

TODO: once cipher list is finalized, also need to update vhost.pp with new defaults in docs.

@wyardley
Copy link
Collaborator Author

https://mozilla.github.io/server-side-tls/ssl-config-generator/ does look reasonable (though not sure what OpenSSL version we should specify as highest, I'm reasonably sure we don't have 1.0.1e everywhere).

Someone on IRC suggested that site as well. I am taking that recommendation; updated docs and also had mailhost use nginx::config::ssl_ciphers as default rather than hard-coding it in the template.

@wyardley wyardley changed the title [WIP] SSL cipher changes (#859) SSL cipher changes (#859) Oct 11, 2016
@wyardley wyardley changed the title SSL cipher changes (#859) SSL cipher changes (issue 859) Oct 12, 2016
@jyaworski jyaworski merged commit ee5e963 into voxpupuli:master Oct 16, 2016
@@ -28,14 +28,9 @@ server {
ssl on;
ssl_certificate <%= @ssl_cert %>;
ssl_certificate_key <%= @ssl_key %>;

ssl_session_timeout 5m;

ssl_protocols TLSv1;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like the re-use of the $nginx::config::ssl_ciphers. Should we do the same for ssl_protocols here?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Absolutely.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See #930

ekohl added a commit to ekohl/puppet-nginx that referenced this pull request Oct 17, 2016
Based on voxpupuli#909. It also
adds a test for the $ssl_ciphers parameter.
# Suggested from https://wiki.mozilla.org/Security/Server_Side_TLS#Nginx
ssl_ciphers 'ECDHE-RSA-AES128-GCM-SHA256:ECDHE-ECDSA-AES128-GCM-SHA256:ECDHE-RSA-AES256-GCM-SHA384:ECDHE-ECDSA-AES256-GCM-SHA384:DHE-RSA-AES128-GCM-SHA256:DHE-DSS-AES128-GCM-SHA256:kEDH+AESGCM:ECDHE-RSA-AES128-SHA256:ECDHE-ECDSA-AES128-SHA256:ECDHE-RSA-AES128-SHA:ECDHE-ECDSA-AES128-SHA:ECDHE-RSA-AES256-SHA384:ECDHE-ECDSA-AES256-SHA384:ECDHE-RSA-AES256-SHA:ECDHE-ECDSA-AES256-SHA:DHE-RSA-AES128-SHA256:DHE-RSA-AES128-SHA:DHE-DSS-AES128-SHA256:DHE-RSA-AES256-SHA256:DHE-DSS-AES256-SHA:DHE-RSA-AES256-SHA:AES128-GCM-SHA256:AES256-GCM-SHA384:AES128-SHA:AES256-SHA:AES:CAMELLIA:DES-CBC3-SHA:!aNULL:!eNULL:!EXPORT:!DES:!RC4:!MD5:!PSK:!aECDH:!EDH-DSS-DES-CBC3-SHA:!EDH-RSA-DES-CBC3-SHA:!KRB5-DES-CBC3-SHA';

ssl_ciphers <%= @ssl_ciphers %>;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The same change applies to the puppet-nginx/templates/mailhost/mailhost.erb file. Because the SSL part of the two templates are equal I used to combine the two into one file that gets included. I'll fix the missing variable in my PR #769.

@wyardley wyardley deleted the issues_859_ssl_configs branch October 27, 2016 17:16
cegeka-jenkins pushed a commit to cegeka/puppet-nginx that referenced this pull request Sep 13, 2019
cegeka-jenkins pushed a commit to cegeka/puppet-nginx that referenced this pull request Sep 13, 2019
Based on voxpupuli#909. It also
adds a test for the $ssl_ciphers parameter.
Rubueno pushed a commit to Rubueno/puppet-nginx that referenced this pull request Oct 19, 2020
Rubueno pushed a commit to Rubueno/puppet-nginx that referenced this pull request Oct 19, 2020
Based on voxpupuli#909. It also
adds a test for the $ssl_ciphers parameter.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants