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

Add config option for ssl_protocols #3010

Closed
wants to merge 1 commit into from

Conversation

francois-maillard
Copy link
Contributor

That's useful for those poor people with client that still
support TLSv1 only.

Summary

Add config option in kong.conf for ssl_protocols

Full changelog

  • Nginx's ssl_protocols option can now be configured from kong.conf
  • Add related tests

That's useful for those poor people with client that still
support TLSv1 only.
@thibaultcha
Copy link
Member

Hi!

Thanks for the patch. I do have mixed feelings about it though, because we try more and more to shy away from adding more NGINX-specific directive mapping to the Kong configuration file. It is hard to draw a line in the sand, but if we keep adding such configuration values for any directive, the model simply won't scale, and we will end up with a very cumberstone configuration file. Instead, we'd rather enforce custom NGINX templates if possible...

We have ideas to alleviate that, especially #2355, if you care to give it a look.

I'm not sure how "popular" the tweaking of the ssl_protocols directive would be among users, but your own comment seems to underline the fact that this is rarely needed, and very much of an edge-case in order to support legacy clients? If so, then this only reinforces the mixed feelings I am having for this addition...

Exceptions are fine, but should only occur when the added value seems significant enough :)

@p0pr0ck5 As you implemented the SSL ciphers configuration properties (for legacy clients support as well iirc, but also best SSL practices), what do you think of this?

@francois-maillard
Copy link
Contributor Author

#2355 seems like a good option to the overall issue of dealing with nginx options.

Regarding the occuracy of tweaking the ssl_protocols directive, thing is, for those with legacy clients, that directive may stay for a while (I personnally have no way to ask my clients to upgrade, even though I'm talking about people working in the same company as me).

Another usecase for this directive is to deactivate bogus implementations following vulnerability being released.

@p0pr0ck5
Copy link
Contributor

p0pr0ck5 commented Nov 8, 2017

I do not believe this should be merged. While I'm all for allowing flexibility in configuration, I think this adds cognitive load and maintenance overhead with little real value, which is why I didn't implement it in the first place. Users with legacy clients should, IMO, resolve to using a custom configuration, as it's more possible that in such a case that there are other options in our template that may need to be adjusted as well. Ultimately, what's being address in #2355 is the appropriate long-term solution.

One other approach I could see would be abstracting away from individual config components, and instead providing a single tunable knob that adjusts protocol selection, ciphers, DH params, etc, all in one go. This, of course, doesn't help with edge cases that need further adjustment, and seems like more work than is warranted here.

Of course, I'm willing to have more of a discussion here, but I suspect the best approach for installations that need to support legacy (read: dangerous) TLS settings should probably stick to a custom template environment for now.

@Darren-wh
Copy link

@francois-maillard I download your source code and modify it follow your changelist. but kong cannot work in TLS1.1.

@thibaultcha
Copy link
Member

The decision here seems to have been reached: considering our reluctance to introduce too many NGINX directives in our configuration, combined with the "edge-case" aspect of this change (supporting legacy versions of TLS), our position is to not accept this and ask of users in need of such a configuration to implement a custom NGINX configuration template instead.

@francois-maillard Thank you for your contribution regardless! We are very much happy to see Kong spreading in various organizations around the world, please do join our community at: https://discuss.konghq.com
There, we hope we can have deeper conversations about use-cases, feature requests, ideas, etc... I'd personally be really interested to hear more about your use-case if you happen to use Kong professionally at Orange :)

Best,

hbagdi added a commit that referenced this pull request Jun 11, 2018
Problem
-------
Kong ships with an NGINX template which is rendered when Kong starts.
There exists no mechanisms to add/update any NGINX directive to the
`nginx.conf` used to run Kong. To change or add any directive, user has
to use a custom NGINX template which has to be synced with Kong for a
release which introduces changes to Kong's template.
Including options in `kong.conf` to configure NGINX directives is not a
good solution since the list will be endless.
This problem can be seen in #3010, #3323 and #3382.

Solution
--------
There needs to be a flexible  way to specify any NGINX directive
via Kong's config file without Kong needing to maintain a list of all
NGINX directives.
While a clean and ideal solution would be #2355, this commit
adopts a simpler as discussed like the one proposed in #2675.

NGINX directives can be specified using config variables with prefixes,
which help determine the block in which to place a directive.
eg:

`nginx_proxy_add_header=Header-Name header-value` will add a `add_header
Header-Name header-value;` directive in the proxy server block of Kong.

`nginx_http_lua_shared_dict=custom_cache 2k` will add a a
`lua_shared_dict custom_cache 2k;` directive to HTTP block of Kong.s
thibaultcha pushed a commit that referenced this pull request Jun 13, 2018
Problem
-------

Kong ships with an NGINX template which is rendered when Kong starts.
There exists no mechanisms to add/update arbitrary NGINX directives to
the `nginx.conf` used to run Kong. To change or add any directive, user
has to use a custom NGINX template which has to be synced with Kong for
a release which introduces changes to Kong's template.  Including
options in `kong.conf` to configure NGINX directives is not a good
solution since the list will be endless. This problem can be seen in
 #3010, #3323, and #3382.

Proposed Solution
-----------------

Proposed in #3382:

There needs to be a flexible  way to specify any NGINX directive via
Kong's config file without Kong needing to maintain a list of all NGINX
directives. While a clean and ideal solution would be #2355, this commit
adopts a simpler approach as described in #3382, and keeps solutions
similar to the one proposed in #2675 possible (by way of injecting an
`include` directive).

NGINX directives can be specified using config variables with prefixes,
which helps determine the block in which to place a directive. E.g.:

* `nginx_proxy_add_header=Header-Name header-value` will add a `add_header
  Header-Name header-value;` directive in the proxy `server` block of
  Kong.

* `nginx_http_lua_shared_dict=custom_cache 2k` will add a
  `lua_shared_dict custom_cache 2k;` directive to `http` block of Kong.
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