Add use-forwarded-headers configmap option. #2616

Dirbaio · 2018-06-10T22:17:09Z

This PR adds a configmap option use-forwarded-headers to enable/disable trust of incoming X-Forwarded-* headers.

Setting it to true corresponds to the current ingress behavior.

It is set to false by default. this is a breaking change, but IMO it's the right thing to do if we follow the "secure by default" principle:

If the default is true, it might lead to vulnerable installations: If it's set to true in a scenario when it shouldn't (direct port forwarding, L3 load balancers) the config is vulnerable to source IP spoofing. However, since everything appears to work correctly, the user probably won't notice, and will end up vulnerable in production.
If the default is false, it'll just cause incorrect behavior if it's wrong. The user is likely to notice he's getting incorrect source IPs and will set it to true. At no point the user is vulnerable to spoofing.

Fixes #1815 #1309 #1668
Rebase/update of #1851

codecov-io · 2018-06-10T22:38:34Z

Codecov Report

Merging #2616 into master will decrease coverage by <.01%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master    #2616      +/-   ##
==========================================
- Coverage   47.57%   47.56%   -0.01%     
==========================================
  Files          77       77              
  Lines        5532     5533       +1     
==========================================
  Hits         2632     2632              
- Misses       2560     2562       +2     
+ Partials      340      339       -1

Impacted Files	Coverage Δ
internal/ingress/controller/config/config.go	`98.3% <100%> (+0.01%)`	⬆️
internal/watch/file_watcher.go	`80.76% <0%> (-3.85%)`	⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 082325b...b5bcb93. Read the comment docs.

antoineco

Looks good but requires at least a unit test.

antoineco · 2018-06-11T11:29:37Z

This is indeed a breaking change, and not the behaviour most people will expect (why would my web proxy decide for me whether a header is secure or not?). If this gets approved we should first leave this to true and log a verbose message notifying users this default will change in future versions.

Dirbaio · 2018-06-11T12:46:20Z

why would my web proxy decide for me whether a header is secure or not?

It has to decide something if the user doesn't. IMO it's better to go with the most conservative option, which is assuming XFF headers aren't secure and not trusting them.

If doing breaking changes is not OK then I guess logging a warning is the best we can do, though..

I'll add unit tests and docs soon.

antoineco · 2018-06-11T13:19:33Z

I'm perfectly with you @Dirbaio, just trying to think about a safe path so that users can be warned in a timely manner before we do change the default (not everybody reads release notes carefully 😉).
The change to false by default can be done in 2 versions instead of 1.

Dirbaio · 2018-06-12T19:51:32Z

Changed setting to be true by default.
Added e2e test that checks for both behaviors
Added docs.

I haven't added the warning when the option is unset though because I'm unsure where to add it :) Help would be appreciated (or we can just not add it?)

Dirbaio · 2018-06-13T13:23:08Z

CI failed due to something unrelated (I think). How do I retest?

also ping @antoineco PTAL

antoineco · 2018-06-13T17:15:23Z

I restarted it.

antoineco · 2018-06-13T18:35:33Z

docs/user-guide/nginx-configuration/configmap.md

@@ -596,6 +597,12 @@ _References:_

 Sets the addresses on which the server will accept requests instead of *. It should be noted that these addresses must exist in the runtime environment or the controller will crash loop.

+## use-forwarded-headers
+
+If true, nginx passes the incoming `X-Forwarded-*` headers to upstreams. Use this option when nginx is behind another L7 proxy / load balancer that is setting these headers.


(nitpick) NGINX is commonly written in capital letters.

antoineco · 2018-06-13T18:41:35Z

rootfs/etc/nginx/template/nginx.tmpl

@@ -222,7 +226,9 @@ http {
        ''               close;
    }

-    map {{ buildForwardedFor $cfg.ForwardedForHeader }} $the_real_ip {
+    # The following is a sneaky way to do "set $the_real_ip $remote_addr"


I'd rather use a Go template comment to avoid writing this message to the config file.

IMO this could be useful to someone cating the rendered config from a running container, to help understand what's going on.

The rendered template already contains other similar comments, and this one is rendered just once so it's not like it'd cause much bloat.

You're right, no strong opinion on that one.

antoineco · 2018-06-13T18:57:53Z

test/e2e/settings/forwarded_headers.go

+		Expect(err).NotTo(HaveOccurred())
+		defer conn.Close()
+
+		conn.Write([]byte("GET / HTTP/1.1\r\nHost: forwarded-headers\r\nX-Forwarded-Port: 1234\r\nX-Forwarded-Proto: myproto\r\nX-Forwarded-For: 1.2.3.4\r\nX-Forwarded-Host: myhost\r\n\r\n"))


Using the gorequest package may make this easier to read.

antoineco · 2018-06-13T18:59:39Z

Overall LGTM, however I have a feeling I'm not experienced enough with the NGINX map directive to express a final statement regarding all this map-fu 🙂

antoineco · 2018-06-15T07:16:03Z

@aledbf I need your opinion on 2 things:

Is this reasonable to you to change the default in a future version?
If yes @Dirbaio please add the warning ~~similarly to https://github.com/kubernetes/ingress-nginx/blob/master/cmd/nginx/flags.go#L213~~
The map logic added to the template.

Dirbaio · 2018-06-18T20:15:09Z

@antoineco the code you linked is for cmdline flags, but this is a configmap flag, which is pulled dynamically from the Kubernetes API.

antoineco · 2018-06-20T08:37:27Z

Yeah stupid me, ignore that comment.

The next best place would be inside the OnUpdate() function then.

Dirbaio · 2018-06-21T23:43:26Z

ping @aledbf . Does the PR look OK? Should I add the future change warning?

aledbf · 2018-06-21T23:45:35Z

@Dirbaio I will take a look at this PR after we release 0.16. Apologies for the delay

albertvaka · 2018-08-16T13:28:42Z

Seems this got ignored again like it happened with #1851

Not great for a security issue :/

antoineco · 2018-08-16T14:16:15Z

/assign aledbf
#2616 (comment)

Like I said, the PR looks good to me but I'm unable to tell whether these maps are idiomatic.

Also pinging @ElvinEfendi

aledbf · 2018-08-16T23:29:11Z

/lgtm
/approve

k8s-ci-robot · 2018-08-16T23:29:16Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: aledbf, Dirbaio

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [aledbf]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

aledbf · 2018-08-16T23:29:33Z

@Dirbaio again, apologies for the delay with the review and merge of this PR

aledbf · 2018-08-16T23:29:39Z

@Dirbaio thanks!

aledbf · 2018-08-16T23:29:59Z

/hold

ElvinEfendi · 2018-08-17T00:20:36Z

rootfs/etc/nginx/template/nginx.tmpl

@@ -1047,7 +1069,7 @@ stream {

            {{ $proxySetHeader }} X-Request-ID           $req_id;
            {{ $proxySetHeader }} X-Real-IP              $the_real_ip;
-            {{ if $all.Cfg.ComputeFullForwardedFor }}
+            {{ if and $all.Cfg.UseForwardedHeaders $all.Cfg.ComputeFullForwardedFor }}


I think we need another else if here, something like

else if $all.Cfg.ComputeFullForwardedFor { $proxySetHeader }} X-Forwarded-For $proxy_add_x_forwarded_for;

since this is still not using forwarded headers, rather just passing it to upstream

$full_x_forwarded_for is the same as $proxy_add_x_forwarded_for but done "by hand" in a map above.

Maybe this is so it appends the proxy protocol addr instead of $remote_addr if using proxy protocol, and nginx's $proxy_add_x_forwarded_for doesn't?

yeah that makes sense @Dirbaio

Why do you require $all.Cfg.UseForwardedHeaders to be true here though?

ComputeFullForwardedFor has no effect when UseForwardedHeaders = false.

There are 3 cases to consider:

UseForwardedHeaders = false: incoming X-Forwarded-For value is discarded and replaced with the source IP nginx-ingress sees.

UseForwardedHeaders = true, ComputeFullForwardedFor = false: incoming X-Forwarded-For value is passed through if present, otherwise it's filled with the source IP nginx-ingress sees.

UseForwardedHeaders = true, ComputeFullForwardedFor = true: incoming X-Forwarded-For value is passed through, appended with the source IP nginx-ingress sees.

For case 3 we use $full_x_forwarded_for, which has been computed earlier in the template.
For cases 1, 2 we use $the_real_ip, which earlier in the template has been set to be the appropriate source IP depending on UseForwardedHeaders

ElvinEfendi · 2018-08-17T00:31:59Z

Thanks for the PR @Dirbaio!

I wonder if we can get rid of ComputeFullForwardedFor and X-Original-Forwarded-For at all. There's a case where we do

{{ $proxySetHeader }} X-Forwarded-For        $the_real_ip;

Why would a reverse proxy do this? This seems like a hacky way of satisfying some lazy upstream so that they can get the correct client IP.

schweikert · 2018-10-31T10:59:16Z

We noticed by chance that the default behavior of ingress-nginx is to trust the X-Forwarded-For header, which caused a security vulnerability for our setup. Is there an issue tracking the change of the default to false for this? I think that it would be much preferable to follow a "secure by default" approach. How many users of ingress-nginx are affected without knowing?

ElvinEfendi · 2018-10-31T12:11:43Z

@schweikert unfortunately that's the case. I think default value forproxy-real-ip-cidr should be "0.0.0.0/32", so that it trusts no client.

#3333 aims to do so.

Add use-forwarded-headers configmap option.

67b253a

k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Jun 10, 2018

antoineco reviewed Jun 11, 2018

View reviewed changes

Dirbaio added 2 commits June 12, 2018 21:33

Set use-forwarded-headers to true by default.

35a6d50

Add use-forwarded-headers e2e test.

fa626a6

k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Jun 12, 2018

Document use-forwarded-headers configmap option.

94266ff

antoineco suggested changes Jun 13, 2018

View reviewed changes

Dirbaio added 2 commits June 13, 2018 21:10

Switch forwarded-headers e2e to use gorequest.

04d24e1

Capitalize NGINX in docs.

4dfc83a

ElvinEfendi mentioned this pull request Jun 18, 2018

do not ignore $http_host and $http_x_forwarded_host #2030

Merged

antoineco mentioned this pull request Aug 2, 2018

Use X-forwarded-for for whitelisting (enabled per ingress via annotation) #2881

Closed

k8s-ci-robot assigned aledbf Aug 16, 2018

Merge branch 'master' into xff

b5bcb93

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Aug 16, 2018

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Aug 16, 2018

k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Aug 16, 2018

k8s-ci-robot merged commit b4942cc into kubernetes:master Aug 16, 2018

ElvinEfendi reviewed Aug 17, 2018

View reviewed changes

timdorr mentioned this pull request Feb 8, 2019

HTTPS via ELB termination broken in 0.22 #3690

Closed

Add use-forwarded-headers configmap option. #2616

Add use-forwarded-headers configmap option. #2616

Conversation

Dirbaio commented Jun 10, 2018

codecov-io commented Jun 10, 2018 • edited Loading

Codecov Report

antoineco left a comment

Choose a reason for hiding this comment

antoineco commented Jun 11, 2018

Dirbaio commented Jun 11, 2018

antoineco commented Jun 11, 2018 • edited Loading

Dirbaio commented Jun 12, 2018

Dirbaio commented Jun 13, 2018

antoineco commented Jun 13, 2018

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

antoineco commented Jun 13, 2018 • edited Loading

antoineco commented Jun 15, 2018 • edited Loading

Dirbaio commented Jun 18, 2018

antoineco commented Jun 20, 2018

Dirbaio commented Jun 21, 2018

aledbf commented Jun 21, 2018

albertvaka commented Aug 16, 2018

antoineco commented Aug 16, 2018 • edited Loading

aledbf commented Aug 16, 2018

k8s-ci-robot commented Aug 16, 2018

aledbf commented Aug 16, 2018

aledbf commented Aug 16, 2018

aledbf commented Aug 16, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Dirbaio Aug 18, 2018 • edited Loading

Choose a reason for hiding this comment

ElvinEfendi commented Aug 17, 2018

schweikert commented Oct 31, 2018

ElvinEfendi commented Oct 31, 2018

codecov-io commented Jun 10, 2018 •

edited

Loading

antoineco commented Jun 11, 2018 •

edited

Loading

antoineco commented Jun 13, 2018 •

edited

Loading

antoineco commented Jun 15, 2018 •

edited

Loading

antoineco commented Aug 16, 2018 •

edited

Loading

Dirbaio Aug 18, 2018 •

edited

Loading