feat(proxy) X-Forwarded-* upstream headers #2236

bungle · 2017-03-22T00:38:54Z

Summary

Implementation of #2202, taking care of all X-Forwarded-* upstream headers and introducing configuration options for the ngx_http_realip_module.

Full changelog

Move the real_ip directives to the Kong proxy location block
Introduce trusted_ips config property
Introduce real_ip_header config property
New lua-resty-mediador library for ip utils, supporting both IPv4 and IPv6 & CIDR blocks
Implement logic to validate client ips as trusted from the new trusted_ips config value
Implement Lua logic for the X-Forwarded-For upstream header
Related tests (renamed real_ip test suite to upstream_headers)

Issues resolved

Fix #2202, #2240

* 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

Partially fixes #2202

* Introduce trusted_ips config property * Introduce real_ip_header * New lua-resty-mediador * Implement logic to validate client ips as trusted * Implement Lua logic for the X-Forwarded-For upstream header * Related tests (renamed real_ip test suite to upstream_headers) Fix #2202

Fix #2240

…at/hop-to-hop-headers # Conflicts: # kong/core/handler.lua # spec/02-integration/05-proxy/02-upstream_headers_spec.lua

p0pr0ck5 · 2017-03-31T03:51:55Z

CHANGELOG.md

+
+- :fireworks: Configurable `X-Forwarded-*` and `X-Real-IP` upstream headers.
+  [#2236](https://github.com/Mashape/kong/pull/2236)
+- :fireworks: Support for The PROXY protocol.


pedantic: lowercase 'the'

#2236 (comment)

thibaultcha

Almost ready it seems! I think there are still 1 or 2 small leftovers or so it seems? Will squash and merge properly for 0.10.2 when ready :)

thibaultcha · 2017-04-11T08:39:38Z

CHANGELOG.md

+  read more about this change from our
+  [0.10.x Proxy Reference](https://getkong.org/docs/0.10.x/proxy/)
+  and
+  [0.10.x Configuration Reference](https://getkong.org/docs/0.10.x/configuration/)


note to self: slightly rephrase for merge

thibaultcha · 2017-04-11T08:44:09Z

kong/templates/nginx_kong.lua

+        proxy_set_header   Upgrade           $upstream_upgrade;
+        proxy_set_header   Connection        $upstream_connection;
+        proxy_pass_header  Server;
+        proxy_pass_header  Date;


note: We should also mention this in the changelog and add it to the proxy guide list of forwarded headers from upstream

thibaultcha · 2017-04-11T08:48:21Z

kong/tools/ip.lua

+    }
+  end
+
+  if trust_all_ipv4 and trust_all_ipv6 then


note: we need to make sure to document that one needs both ipv4 and ipv6 forms specified to "trust all"

(but tbh, I feel like we should respect the --with-ipv6 option of Nginx, and ignore all IPv6 settings/values if support is not enabled in Nginx)

Wasn't it so that --with-ipv6 was dropped already from Nginx mainline, or does OpenResty still continue having them somehow?

Yeah sorry I wasn't very clear here. Yes, --with-ipv6 was dropped. What I meant is split in 2 parts:

I think that users should be able to enable/disable ipv6 support through Kong

Currently doable via the --with-nginx option check in the current OpenResty (I think 1.11.2still has it? not sure?), but in the future, the need for disabling ipv6 will be even greater, because the OS might support ipv6, but not the network, and we cannot rely on the --with-ipv6 option anymore to assume what the user wanted, hence, it simply increased the need for this behavior to be configurable.

Do you think this is enough for now:
Kong/docs.konghq.com@2775f51#diff-9e30b1ff312829ea15c5647e63e8ba8aR468

thibaultcha · 2017-04-11T08:51:24Z

kong/templates/nginx_kong.lua

+map $http_x_forwarded_port $upstream_x_forwarded_port {
+    default $http_x_forwarded_port;
+    ''      $server_port;
+}


Any reason why we're not handling those in the Lua logic but need those maps again instead? I don't remember if there was a special case that they handle in lieu of an else in the Lua code?

We just initialize the vars with defaults. We can do it in Lua land as well.

That would avoid having pieces of the logic in two different places indeed

thibaultcha · 2017-04-11T08:55:35Z

spec/02-integration/05-proxy/02-upstream_headers_spec.lua

+      end)
+    end)
+
+    describe("with the downstream host preserved (pending: because the test fails to connect to API when preserve_host = true)", function()


Maybe we can remove this "pending" comment now? Assuming this was related to one of the recent fixes on master

thibaultcha · 2017-04-11T09:01:19Z

kong/core/handler.lua

+        var.upstream_x_forwarded_for = http_x_forwarded_for .. ", " .. realip_remote_addr
+
+      else
+        var.upstream_x_forwarded_for = var.remote_addr


I think some comments here would be very beneficial to avoid future confusion, regarding the usage of realip_remote_addr vs var.remove_addr, and why we need to build the X-Forwarded-For upstream header ourselves (due to the ngx_http_proxy_module limitation we discovered).

Comments were added.

Fix: #2236 (comment)

thibaultcha · 2017-04-13T22:58:11Z

kong/core/handler.lua

+      -- is okay for us to use it in case no X-Forwarded-For header was present.
+      -- But in case it was given, we will append the $realip_remote_addr that
+      -- contains the IP that was originally in $remote_addr before realip module
+      -- overrode that (aka the client that connected us).


👍 great! This will give future contributors a good reason as to why things are done like this

thibaultcha · 2017-04-13T23:12:43Z

kong/core/handler.lua

          balancer_address.hostname..":"..balancer_address.port

+      -- Keep-Alive and WebSocket Protocol Upgrade Headers
+      if var.http_upgrade == "websocket" then


Sorry if you thought I implied that we should move those as well in the Lua-land. I only meant to apply changes to the PR that apply to this scope, and did not mean to add extra work to it! It would have been perfectly if this had stayed in the Nginx configuration. Maybe it still should be moved back there to avoid introducing breaking changes? As of this stands, we perform a case-sensitive check on the content of those headers, and that is a breaking change since Nginx's map is case-insensitive.

Sorry for the misunderstanding!

While I was doing it, I thought, why not do them all similarly in Lua-land (they are somewhat related). But that case-insensitivity thing was something I didn't think about. Great catch! Basically two choices, case insensitive match on Lua or move them back to maps?

@thibaultcha, I pushed a patch to make case-insensitive comparison on Lua-land. I can always move it back to map, but I like this because it keeps our user facing nginx.conf cleaner.

I like this because it keeps our user facing nginx.conf cleaner.

Exactly, we should keep it on the Lua-land indeed.

thibaultcha · 2017-04-14T17:44:05Z

(In the future, please don't merge next or master in your branch. Rebasing is fine though. Thanks!)

thibaultcha · 2017-04-14T18:57:15Z

kong/core/handler.lua

      local http_x_forwarded_for = var.http_x_forwarded_for
+


You got it! 😉

Changes from #2236.

thibaultcha · 2017-04-15T00:37:59Z

Merged to next with a few modifications to the docs (changelog + conf), and a clean git history. Thank you!

subnetmarco · 2017-04-19T18:26:21Z

@thibaultcha can this be also merged into master so that it will be available in 0.10.2?

braedon · 2017-05-12T04:26:30Z

It doesn't look like this actually made it into 0.10.2 - any word on when it will be released?

thibaultcha · 2017-05-12T06:37:29Z

This is scheduled for 0.11 as it introduces some breaking changes.

headers Documents Kong/kong#2236

thibaultcha added 2 commits March 21, 2017 17:10

feat(proxy) proper X-Forwarded-* upstream headers

4193423

Partially fixes #2202

bungle added the pr/status/needs review label Mar 22, 2017

bungle force-pushed the feat/hop-to-hop-headers branch 7 times, most recently from aa9d19e to 475e653 Compare March 22, 2017 06:02

bungle mentioned this pull request Mar 23, 2017

IP Restriction and Rate-Limiting should make X-Forwarded-For parsing configurable #2020

Closed

bungle force-pushed the feat/hop-to-hop-headers branch 2 times, most recently from cf47c58 to 56c080c Compare March 23, 2017 17:57

bungle force-pushed the feat/hop-to-hop-headers branch from 56c080c to 8e2fcb1 Compare March 23, 2017 22:08

bungle added 2 commits March 23, 2017 17:47

Added related entries and information to the CHANGELOG.md.

f885be2

Added small notes to pending tests.

77b394b

bungle mentioned this pull request Mar 24, 2017

Kong nginx_kong.lua template overrides request proxy headers #1615

Closed

shamil mentioned this pull request Mar 26, 2017

Proxy protocol support #2240

Closed

This was referenced Mar 27, 2017

[plugin] Allow usage of the X-Forwarded-For header in the ip-restriction plugin #1995

Closed

[plugin] Allow usage of the X-Forwarded-For header in the rate-limiting plugin #2012

Closed

bungle added 7 commits March 28, 2017 02:23

Add support for proxy protocol

618c19b

Fix #2240

Add support for proxy protocol

ccc4781

Fix #2240

Merge remote-tracking branch 'origin/feat/hop-to-hop-headers' into fe…

d0c40c5

…at/hop-to-hop-headers # Conflicts: # kong/core/handler.lua # spec/02-integration/05-proxy/02-upstream_headers_spec.lua

Remove headers-inspect test plugin because it is not needed anymore.

514ff60

Add a line about the PROXY protocol support in changelog

89eaeea

Resolves merge conflicts with next.

bad20fc

Merge branch 'next' into feat/hop-to-hop-headers

600c4c5

p0pr0ck5 reviewed Mar 31, 2017

View reviewed changes

bungle added this to the 0.10.2 milestone Apr 3, 2017

Fix review comment:

1f8b67b

#2236 (comment)

bungle mentioned this pull request Apr 10, 2017

Fix X-Forwarded-For when use ngx_http_realip_module #2361

Closed

thibaultcha reviewed Apr 11, 2017

View reviewed changes

Fix: #2236 (comment)

d966e41

Fix: #2236 (comment)

79c2743

Fix: #2236 (comment)

bungle force-pushed the feat/hop-to-hop-headers branch from 75be12b to 79c2743 Compare April 11, 2017 21:07

Fixed a typo in WebSocket.

f4d7859

thibaultcha reviewed Apr 13, 2017

View reviewed changes

thibaultcha requested changes Apr 13, 2017

View reviewed changes

fix: #2236 (comment)

5045b3d

thibaultcha reviewed Apr 14, 2017

View reviewed changes

kong/core/handler.lua Outdated

local http_x_forwarded_for = var.http_x_forwarded_for

Copy link

Member

thibaultcha Apr 14, 2017

Choose a reason for hiding this comment

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

You got it! 😉

thibaultcha added a commit that referenced this pull request Apr 15, 2017

docs(changelog) add recent upstream headers changes

a9cb664

Changes from #2236.

thibaultcha closed this Apr 15, 2017

thibaultcha deleted the feat/hop-to-hop-headers branch April 15, 2017 00:36

thibaultcha mentioned this pull request Apr 15, 2017

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

Closed

13 tasks

bungle modified the milestones: 0.11.0, 0.10.2 May 12, 2017

thibaultcha added a commit to Kong/docs.konghq.com that referenced this pull request Aug 11, 2017

docs(*) new realip_module conf directives and proxied hop-to-hop

4f13ce3

headers Documents Kong/kong#2236

thibaultcha added a commit to Kong/docs.konghq.com that referenced this pull request Aug 11, 2017

docs(*) new realip_module conf directives and proxied hop-to-hop

fa33fbc

headers Documents Kong/kong#2236

thibaultcha added a commit to Kong/docs.konghq.com that referenced this pull request Aug 11, 2017

docs(*) new realip_module conf directives and proxied hop-to-hop

185ff42

headers Documents Kong/kong#2236

thibaultcha added a commit to Kong/docs.konghq.com that referenced this pull request Aug 16, 2017

docs(*) new realip_module conf directives and proxied hop-to-hop

9b856be

headers Documents Kong/kong#2236

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(proxy) X-Forwarded-* upstream headers #2236

feat(proxy) X-Forwarded-* upstream headers #2236

bungle commented Mar 22, 2017 •

edited

Loading

p0pr0ck5 Mar 31, 2017

thibaultcha left a comment

thibaultcha Apr 11, 2017

thibaultcha Apr 11, 2017

thibaultcha Apr 11, 2017

bungle Apr 13, 2017

thibaultcha Apr 13, 2017

bungle Apr 14, 2017

thibaultcha Apr 11, 2017

bungle Apr 11, 2017

thibaultcha Apr 11, 2017

thibaultcha Apr 11, 2017

thibaultcha Apr 11, 2017

bungle Apr 11, 2017

thibaultcha Apr 13, 2017 •

edited

Loading

thibaultcha Apr 13, 2017

bungle Apr 13, 2017 •

edited

Loading

bungle Apr 14, 2017 •

edited

Loading

thibaultcha Apr 14, 2017

thibaultcha commented Apr 14, 2017

thibaultcha Apr 14, 2017

thibaultcha commented Apr 15, 2017

subnetmarco commented Apr 19, 2017

braedon commented May 12, 2017

thibaultcha commented May 12, 2017

feat(proxy) X-Forwarded-* upstream headers #2236

feat(proxy) X-Forwarded-* upstream headers #2236

Conversation

bungle commented Mar 22, 2017 • edited Loading

Summary

Full changelog

Issues resolved

Choose a reason for hiding this comment

thibaultcha left a 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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

thibaultcha Apr 13, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bungle Apr 13, 2017 • edited Loading

Choose a reason for hiding this comment

bungle Apr 14, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

thibaultcha commented Apr 14, 2017

Choose a reason for hiding this comment

thibaultcha commented Apr 15, 2017

subnetmarco commented Apr 19, 2017

braedon commented May 12, 2017

thibaultcha commented May 12, 2017

bungle commented Mar 22, 2017 •

edited

Loading

thibaultcha Apr 13, 2017 •

edited

Loading

bungle Apr 13, 2017 •

edited

Loading

bungle Apr 14, 2017 •

edited

Loading