feat(conf) control the set_real_ip_from NGINX variable in kong.conf #1662

kylegato · 2016-09-20T21:04:11Z

Summary

Adding the ability to control the set_real_ip_from NGINX variable in the kong.conf file.

Full changelog

Added set_real_ip_from config option - See http://nginx.org/en/docs/http/ngx_http_realip_module.html#set_real_ip_from

Issues resolved

Fix #1661

…stent-pid hotfix(cli) prevent executing 'kill' on missing pids

kylegato · 2016-09-21T20:33:52Z

CC: @thefosk

subnetmarco · 2016-09-22T22:07:47Z

LGTM

subnetmarco · 2016-09-22T22:08:35Z

@kylegato can you also send a PR against the release/0.10 branch on to https://github.com/Mashape/getkong.org to add the relevant documentation?

thibaultcha · 2016-09-22T22:09:02Z

kong.conf.default

+				 # correct replacement addresses. 
+				 # If the special value unix: is specified, 
+				 # all UNIX-domain sockets will be trusted.
+				 # See: http://nginx.org/en/docs/http/ngx_http_realip_module.html#set_real_ip_from


Could you align those 4 to the same column as the comments of the other properties?

Also, please include # See [link] on the next line, like so: https://github.com/Mashape/kong/blob/master/kong.conf.default#L34-L35

# Note: See http://nginx.org/en/docs/http/ngx_http_realip_module.html#set_real_ip_from # for a list of accepted values.

I think the trailing ; would throw an error when starting nginx?

Finally: please move this property below the proxy_listen and proxy_listen_ssl properties. And it should probably be renamed to proxy_set_real_ip_from.

Yes that's right, I missed the ;. This will certainly cause an error because when it's being added with set_real_ip_from ${{SET_REAL_IP_FROM}}; it basically appends a double semicolon.

Updated PR, thanks guys.

thibaultcha · 2016-09-22T22:11:15Z

kong.conf.default

+				 # all UNIX-domain sockets will be trusted.
+				 # See: http://nginx.org/en/docs/http/ngx_http_realip_module.html#set_real_ip_from
+
+


Extraneous line jump

thibaultcha · 2016-09-22T22:12:04Z

kong.conf.default

+				 # correct replacement addresses. 
+				 # If the special value unix: is specified, 
+				 # all UNIX-domain sockets will be trusted.
+				 # See: http://nginx.org/en/docs/http/ngx_http_realip_module.html#set_real_ip_from


I think the trailing ; would throw an error when starting nginx?

thibaultcha · 2016-09-22T22:12:28Z

kong/templates/nginx_kong.lua

@@ -26,7 +26,7 @@ proxy_ssl_server_name on;
 underscores_in_headers on;

 real_ip_header X-Forwarded-For;
-set_real_ip_from 0.0.0.0/0;
+set_real_ip_from ${{SET_REAL_IP_FROM}};


See here, semicolon is already provided.

Whoops, sorry about that!

thibaultcha · 2016-09-22T22:14:13Z

kong.conf.default

+				 # correct replacement addresses. 
+				 # If the special value unix: is specified, 
+				 # all UNIX-domain sockets will be trusted.
+				 # See: http://nginx.org/en/docs/http/ngx_http_realip_module.html#set_real_ip_from


Finally: please move this property below the proxy_listen and proxy_listen_ssl properties. And it should probably be renamed to proxy_set_real_ip_from.

thibaultcha

See comments

thibaultcha · 2016-09-22T22:17:49Z

Ultimately, I wonder if we shouldn't also implement an on/off flag for this behavior in order to also solve #1615.

thibaultcha · 2016-09-22T22:19:08Z

Also: there should probably be a test for this in 02-real_ip_spec.lua?

… fixing some formatting

kylegato · 2016-09-22T23:06:11Z

@thibaultcha I'm having trouble coming up with my own original idea for modifying 02-real_ip_spec.lua after reviewing it and what it's doing.

Maybe we would have the test set the "set_real_ip_from" to something like 123.123.123.0/0 and then check that the sent header IP does not match?

thibaultcha · 2016-09-22T23:06:17Z

kong.conf.default

@@ -59,6 +59,13 @@
 #proxy_listen_ssl = 0.0.0.0:8443 # Address and port on which Kong will accept
                                 # HTTPS requests if `ssl` is enabled.

+#proxy_set_real_ip_from = 0.0.0.0/0    # Defines trusted addresses that are known to send


Please respect the 80 chars columns limit

Also: the # should be 2 white spaces after the last /0

thibaultcha · 2016-09-22T23:07:17Z

kong.conf.default

+#proxy_set_real_ip_from = 0.0.0.0/0    # Defines trusted addresses that are known to send
+                                 # correct replacement addresses.
+                                 # If the special value unix: is specified,
+                                 # all UNIX-domain sockets will be trusted.


Please align those comments with the hash from L62

@thibaultcha Please review my latest config update, I believe i did.

thibaultcha · 2016-09-22T23:10:02Z

Maybe we would have the test set the "set_real_ip_from" to something like 123.123.123.0/0 and then check that the sent header IP does not match?

I think that is reasonable enough, given another test exists which asserts the contrary when set_real_ip_from is 0.0.0.0/0.

we go over the 80 chars limit for the link but that's ok for now.

style(conf) correct formatting

thibaultcha · 2016-09-24T01:04:59Z

Latest update: this is just waiting on a test as discussed in this thread. @kylegato said he would write one in the next few days.

subnetmarco · 2016-10-03T21:37:27Z

@kylegato any updates?

thibaultcha · 2016-10-23T20:11:26Z

This would be a great addition to have in 0.9.4, alongside #1615. Would love to see some tests and docs updates, if not, we'll try to find time to do it ourselves.

subnetmarco · 2017-02-21T22:25:26Z

@kylegato would you be able to update this PR?

thibaultcha · 2017-03-01T19:52:28Z

It should be possible to provide a list of trusted addresses, as several occurrences of this directive can be applied.

thibaultcha · 2017-03-14T18:19:53Z

Moved to #2202. Closing this as the current implementation takes care of a few more things + has tests. Thanks!

* Add real_ip_recursive and set_real_ip_from Kong configuration fields to configure ngx_http_realip_module directives. * Move the real_ip directives to the Kong proxy location block. * Add configuration building unit tests for those 2 new directives. Fix #1661 Deprecates #1662

thibaultcha and others added 2 commits September 16, 2016 11:36

Merge pull request Kong#1640 from Mashape/hotfix/kill-against-non-exi…

9920679

…stent-pid hotfix(cli) prevent executing 'kill' on missing pids

Adding set_real_ip_from config option Kong#1661

d7751fb

thibaultcha added the pr/status/needs review label Sep 21, 2016

subnetmarco approved these changes Sep 22, 2016

View reviewed changes

subnetmarco added this to the 0.10 milestone Sep 22, 2016

thibaultcha reviewed Sep 22, 2016

View reviewed changes

thibaultcha requested changes Sep 22, 2016

View reviewed changes

thibaultcha added pr/changes requested Changes were requested to this PR by a maintainer. Please address them and ping back once done. and removed pr/status/needs review labels Sep 22, 2016

Changing set_real_ip_from config option to proxy_set_real_ip_from and…

6ca0067

… fixing some formatting

thibaultcha reviewed Sep 22, 2016

View reviewed changes

Kyle Gato and others added 3 commits September 22, 2016 16:10

Updating README to respect the 80 char line limit

f178591

style(conf) correct formatting

1b03019

we go over the 80 chars limit for the link but that's ok for now.

Merge pull request #1 from kylegato/style/conf

c13b675

style(conf) correct formatting

jussiarpalahti mentioned this pull request Sep 27, 2016

Kong nginx_kong.lua template overrides request proxy headers #1615

Closed

thibaultcha changed the title ~~Adding the ability to control the set_real_ip_from NGINX variable in the kong.conf file.~~ feat(conf) control the set_real_ip_from NGINX variable in kong.conf Oct 6, 2016

thibaultcha modified the milestone: 0.10 RC Mar 7, 2017

thibaultcha mentioned this pull request Mar 14, 2017

Implement proper hop-to-hop + forwarded headers #2202

Closed

13 tasks

thibaultcha closed this Mar 14, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(conf) control the set_real_ip_from NGINX variable in kong.conf #1662

feat(conf) control the set_real_ip_from NGINX variable in kong.conf #1662

kylegato commented Sep 20, 2016 •

edited

Loading

kylegato commented Sep 21, 2016

subnetmarco commented Sep 22, 2016

subnetmarco commented Sep 22, 2016

thibaultcha Sep 22, 2016

thibaultcha Sep 22, 2016

thibaultcha Sep 22, 2016

thibaultcha Sep 22, 2016 •

edited

Loading

subnetmarco Sep 22, 2016 •

edited

Loading

kylegato Sep 22, 2016

thibaultcha Sep 22, 2016

thibaultcha Sep 22, 2016

thibaultcha Sep 22, 2016

kylegato Sep 22, 2016

thibaultcha Sep 22, 2016 •

edited

Loading

thibaultcha left a comment

thibaultcha commented Sep 22, 2016

thibaultcha commented Sep 22, 2016

kylegato commented Sep 22, 2016

thibaultcha Sep 22, 2016

thibaultcha Sep 22, 2016

thibaultcha Sep 22, 2016

kylegato Sep 22, 2016

thibaultcha commented Sep 22, 2016

thibaultcha commented Sep 24, 2016

subnetmarco commented Oct 3, 2016

thibaultcha commented Oct 23, 2016

subnetmarco commented Feb 21, 2017

thibaultcha commented Mar 1, 2017

thibaultcha commented Mar 14, 2017 •

edited by bungle

Loading

		# all UNIX-domain sockets will be trusted.
		# See: http://nginx.org/en/docs/http/ngx_http_realip_module.html#set_real_ip_from

feat(conf) control the set_real_ip_from NGINX variable in kong.conf #1662

feat(conf) control the set_real_ip_from NGINX variable in kong.conf #1662

Conversation

kylegato commented Sep 20, 2016 • edited Loading

Summary

Full changelog

Issues resolved

kylegato commented Sep 21, 2016

subnetmarco commented Sep 22, 2016

subnetmarco commented Sep 22, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

thibaultcha Sep 22, 2016 • edited Loading

Choose a reason for hiding this comment

subnetmarco Sep 22, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

thibaultcha Sep 22, 2016 • edited Loading

Choose a reason for hiding this comment

thibaultcha left a comment

Choose a reason for hiding this comment

thibaultcha commented Sep 22, 2016

thibaultcha commented Sep 22, 2016

kylegato commented Sep 22, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

thibaultcha commented Sep 22, 2016

thibaultcha commented Sep 24, 2016

subnetmarco commented Oct 3, 2016

thibaultcha commented Oct 23, 2016

subnetmarco commented Feb 21, 2017

thibaultcha commented Mar 1, 2017

thibaultcha commented Mar 14, 2017 • edited by bungle Loading

kylegato commented Sep 20, 2016 •

edited

Loading

thibaultcha Sep 22, 2016 •

edited

Loading

subnetmarco Sep 22, 2016 •

edited

Loading

thibaultcha Sep 22, 2016 •

edited

Loading

thibaultcha commented Mar 14, 2017 •

edited by bungle

Loading