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 Parameter should be configurable outside of vhosts #670

Closed
omcnet opened this issue Aug 18, 2015 · 4 comments · Fixed by #1382
Closed

SSL Parameter should be configurable outside of vhosts #670

omcnet opened this issue Aug 18, 2015 · 4 comments · Fixed by #1382
Labels
enhancement New feature or request

Comments

@omcnet
Copy link

omcnet commented Aug 18, 2015

It may be common practice, but there is no point in configuring the ssl parameters in every vhosts.
There should be a global configuration that sets secure defaults and only if a vhosts really needs a custom config, should the vhost parameters be used.
This would make all vhosts more readable, mass updates more stable and analysis of the configuration by admins much easier.

This seems even more important since the default value is a very verbose way of naming the currently mostly used ciphers.

What does @jfryman think about this? If you can confirm that this would be helpful, i could work on a PR to implement this. But if this is not desired I will stick with a fork.

Another alternative would to be to handle an undef or empty value of ciphers and handling that case by not adding a configuration line.

@b4ldr
Copy link
Member

b4ldr commented Sep 11, 2015

I think its good to have a set of defaults but i would also like to be able to override this on the vhost

@omcnet
Copy link
Author

omcnet commented Sep 14, 2015

@b4ldr That should not be an issue.
For our production deployments we currently include a modules.d/*.conf, (where our ssl.conf is located ) within the http{ } section.
Below that modules.d include we simply do an include of vhosts.d ( or sites-enabled/available, depending on the os ).
Thus any setting can be overwritten by the vhosts.

Thus using a wildcard include of conf.d or modules.d or simply placing a global ciphersuite within the http section before including virtual hosts one should have the best of both worlds.

@3flex
Copy link
Contributor

3flex commented Sep 15, 2015

@omcnet I also think it makes a lot of sense to have SSL defaults on the http level. But as @b4ldr said any PR must still allow for SSL directives to be overridden at the server level too.

Please have directives added directly to the nginx.conf file, don't put them into a separate file to include, so the generated configuration is easier to understand.

@wyardley wyardley added enhancement New feature or request needs discussion labels Oct 11, 2016
@wyardley
Copy link
Collaborator

I think this is sensible as well, but as best I know, a PR hasn't come up for this, and it's 1 year later.

Other than keeping things shorter and more readable, I'd argue that it's more or less a cosmetic thing, since the current solution should give the same results. But I'm going to leave this open, since I would love to see this get implemented if it's not too difficult. We'd probably need to move some of the default parameters out of the 'vhost' resource and into params or config?

bastelfreak added a commit that referenced this issue May 5, 2020
…x_conf

Add ability to set ssl-settings globally - fixes #670
ceonizm pushed a commit to ceonizm/puppet-nginx that referenced this issue May 31, 2020
ceonizm pushed a commit to ceonizm/puppet-nginx that referenced this issue Jun 1, 2020
jradmacher pushed a commit to emetriq/puppet-nginx that referenced this issue Jun 22, 2020
Rubueno pushed a commit to Rubueno/puppet-nginx that referenced this issue Oct 19, 2020
Rubueno pushed a commit to Rubueno/puppet-nginx that referenced this issue Oct 19, 2020
…s_to_nginx_conf

Add ability to set ssl-settings globally - fixes voxpupuli#670
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants