Refactor X-Forwarded-* headers #1381

aledbf · 2017-09-17T18:03:44Z

Test images: quay.io/aledbf/nginx-ingress-controller:0.223

Replaces #1222
Fixes #1000

k8s-reviewable · 2017-09-17T18:03:48Z

This change is

aledbf · 2017-09-17T18:04:27Z

coveralls · 2017-09-17T19:47:52Z

Coverage increased (+0.01%) to 43.89% when pulling f38f49e on aledbf:refactor-template-headers into f478084 on kubernetes:master.

…eaders

coveralls · 2017-09-17T20:15:57Z

Coverage decreased (-0.02%) to 43.89% when pulling 669428e on aledbf:refactor-template-headers into 1a68536 on kubernetes:master.

boazy · 2017-09-18T17:25:38Z

@aledbf Thanks for the implementation! It looks good
I'll try to test that as soon as possible.

jammm · 2017-09-19T05:45:41Z

controllers/nginx/rootfs/etc/nginx/template/nginx.tmpl

+        default          $scheme;
+    }
+    map $http_x_forwarded_port $pass_server_port {
+    default           $server_port;


Indentation could be used here.

jammm · 2017-09-19T10:00:02Z

controllers/nginx/rootfs/etc/nginx/template/nginx.tmpl

@@ -143,27 +143,59 @@ http {
        ''               close;
    }

+    {{ if (trustHTTPHeaders $cfg) }}


For some reason, trustHTTPHeaders is always returning true for me. This block here is always being created regardless of whether I set real-client-from: "http-proxy" or real-client-from: "tcp-proxy".

I checked to make sure whether it's loading real-client-from properly by setting an invalid value, and it does seem to show up on the logs and sets to "auto" so I'm almost certain this value is being read up to trustHTTPHeaders.

jammm · 2017-09-19T10:00:58Z

controllers/nginx/pkg/template/template.go

+func trustHTTPHeaders(input interface{}) bool {
+	conf, ok := input.(config.TemplateConfig)
+	if !ok {
+		return true


(Continuing from my comment on nginx.tmpl:146)
Maybe !ok always evaluates to true here.
Could you check if input.(config.TemplateConfig) is returning conf correctly? Or maybe I'm missing something..

jammm · 2017-09-19T10:12:11Z

controllers/nginx/rootfs/etc/nginx/template/nginx.tmpl

-    {{ if $cfg.UseProxyProtocol }}
-        ''               $proxy_protocol_addr;
+    {{ if (trustProxyProtocol $cfg) }}
+        # Get IP address from Proxy Protocol


This wasn't created for me after setting real-client-from: "tcp-proxy"

jammm · 2017-09-20T02:10:41Z

controllers/nginx/pkg/template/template.go

+	}
+
+	return conf.Cfg.RealClientFrom == "tcp-proxy" ||
+		(conf.Cfg.RealClientFrom == "auto" && !conf.Cfg.UseProxyProtocol)


I could be wrong, but shouldn't this be (conf.Cfg.RealClientFrom == "auto" && conf.Cfg.UseProxyProtocol) ?

…eaders Merge

aledbf · 2017-09-26T17:22:45Z

@jammm image updated quay.io/aledbf/nginx-ingress-controller:0.233

coveralls · 2017-09-26T17:44:16Z

Coverage increased (+0.06%) to 43.828% when pulling 6ee2b72 on aledbf:refactor-template-headers into 47ea2d7 on kubernetes:master.

coveralls · 2017-09-26T18:36:20Z

Coverage increased (+0.05%) to 43.816% when pulling 78e166f on aledbf:refactor-template-headers into 47ea2d7 on kubernetes:master.

aledbf · 2017-09-28T21:39:03Z

ping @rikatz @jammm @boazy please check the content of the PR
Temporal image quay.io/aledbf/nginx-ingress-controller:0.233

…eaders

coveralls · 2017-09-29T12:26:06Z

Coverage increased (+0.01%) to 43.52% when pulling db12b51 on aledbf:refactor-template-headers into ba6c896 on kubernetes:master.

coveralls · 2017-09-29T12:38:13Z

Coverage increased (+0.04%) to 43.545% when pulling db12b51 on aledbf:refactor-template-headers into ba6c896 on kubernetes:master.

coveralls · 2017-09-29T13:18:39Z

Coverage increased (+0.05%) to 43.557% when pulling b1b75f9 on aledbf:refactor-template-headers into ba6c896 on kubernetes:master.

coveralls · 2017-09-29T13:20:15Z

Coverage increased (+0.04%) to 43.545% when pulling b1b75f9 on aledbf:refactor-template-headers into ba6c896 on kubernetes:master.

rikatz · 2017-09-29T14:04:02Z

I'm going to test this afternoon and will return you.

aledbf · 2017-09-29T15:11:10Z

@rikatz please use quay.io/aledbf/nginx-ingress-controller:0.242

coveralls · 2017-09-29T15:43:58Z

Coverage increased (+0.01%) to 43.52% when pulling f549e03 on aledbf:refactor-template-headers into ba6c896 on kubernetes:master.

rikatz · 2017-09-29T19:33:45Z

@aledbf It's not working :(

I'm running ingress as a baremetal container, and it's not fetching correctly the IP address.

The mapping in nginx.conf is as the following:

   map $http_x_forwarded_for $the_real_ip {
        # Get IP address from Proxy Protocol
        default          $proxy_protocol_addr;
    }

    map $http_x_forwarded_host $best_http_host {
        default          $this_host;

I'm not using proxy protocol, and neither this is enabled in ConfigMap.

Probably this was auto detected (I hadn't changed anything).

Thanks

rikatz · 2017-09-29T20:52:04Z

controllers/nginx/rootfs/etc/nginx/template/nginx.tmpl

-        "~*(?<ip>[0-9\.]+).*"     $ip;
-    {{ if $cfg.UseProxyProtocol }}
-        ''               $proxy_protocol_addr;
+    {{ if (trustProxyProtocol $cfg) }}


Shouldn't this be $all instead of $cfg, as defined also in trustHTTPHeaders?

I'm facing an issue here, that even when UseProxyProtocol is false, it's returning a 'true' value here, using $proxy_protocol_addr instead of $realip_remote_addr.

Edit: After changing this to $all in my template, it works fine. I'm now going to test this with other options to see what's the behaviour.

aledbf · 2017-09-29T21:01:43Z

@rikatz please use quay.io/aledbf/nginx-ingress-controller:0.243

aledbf · 2017-09-29T21:02:48Z

@rikatz this image works fine.

telnet $(minikube ip) 80

Default configuration

Tests:

case 1

GET / HTTP/1.0

case 2

GET / HTTP/1.0
X-Forwarded-For: 10.0.0.1, fake.ip, 198.51.100.1

proxy-protocol

Configmap:
use-proxy-protocol: true

Tests:

case 1

PROXY TCP4 1.1.1.1 1.1.1.2 1 2 
GET / HTTP/1.0

case 2

PROXY TCP4 1.1.1.1 1.1.1.2 1 2 
GET / HTTP/1.0
X-Forwarded-For: 10.0.0.1, fake.ip, 198.51.100.1

proxy-protocol

Configmap:
use-proxy-protocol: true

Tests:

case 1

PROXY TCP4 1.1.1.1 1.1.1.2 1 2 
GET / HTTP/1.0

case 2

PROXY TCP4 1.1.1.1 1.1.1.2 1 2 
GET / HTTP/1.0
X-Forwarded-For: 10.0.0.1, fake.ip, 198.51.100.1

coveralls · 2017-09-29T21:21:06Z

Coverage increased (+0.04%) to 43.553% when pulling f253d24 on aledbf:refactor-template-headers into ba6c896 on kubernetes:master.

rikatz · 2017-09-29T21:54:16Z

@aledbf Now I'm entering the other 'if', that accepts X-Forwarded-For as the realip.

I'm trying to bypass this and see if the vulnerability still happens, but it seems fine now (I'll try to understand what changed and where later).

The only thing that's still wrong here is the mapping for http_x_forwarded_proto, that accepts whatever the user passes.

This allows the user to bypass SSL redirections as defined above:

 if ($pass_access_scheme = http) {
                return 301 https://$best_http_host$request_uri;
            }

An example: create a site, force the SSL redirection, and then set the Header X-Forwarded-Proto: https but call the HTTP Port/VHost and You'll not be redirected, bypassing this directive.

Thanks!

aledbf · 2017-09-29T22:42:34Z

@rikatz enabling HSTS solves this issue. Also your service should use secure cookies.
I am not sure we can do more than this. We need to allow the header to be able to run behind a load balancer.

k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Sep 17, 2017

aledbf force-pushed the refactor-template-headers branch from 2e9f4b7 to bc92b77 Compare September 17, 2017 18:47

Refactor X-Forwarded-* headers

f38f49e

aledbf force-pushed the refactor-template-headers branch from bc92b77 to f38f49e Compare September 17, 2017 19:17

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

aledbf added 2 commits September 17, 2017 16:48

Merge remote-tracking branch 'master/master' into refactor-template-h…

669428e

…eaders

1

df57b8b

jammm reviewed Sep 19, 2017

View reviewed changes

jammm reviewed Sep 20, 2017

View reviewed changes

aledbf added 2 commits September 26, 2017 14:08

Merge remote-tracking branch 'master/master' into refactor-template-h…

d73edb8

…eaders Merge

Fix template mappings

6ee2b72

Fix cast error

78e166f

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 Sep 26, 2017

abh mentioned this pull request Sep 29, 2017

X-Forwarded-For is not updated from proxy_protocol when terminating TLS #1443

Closed

Merge remote-tracking branch 'master/master' into refactor-template-h…

3ed6019

…eaders

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

Fix identation

db12b51

aledbf added 2 commits September 29, 2017 09:57

Cleanup

fe2386b

Rollback change in docker image

b1b75f9

Fix remote address

f549e03

rikatz reviewed Sep 29, 2017

View reviewed changes

Cleanup

f253d24

aledbf merged commit 1c67da5 into kubernetes:master Sep 30, 2017

aledbf deleted the refactor-template-headers branch September 30, 2017 20:08

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor X-Forwarded-* headers #1381

Refactor X-Forwarded-* headers #1381

aledbf commented Sep 17, 2017 •

edited

Loading

k8s-reviewable commented Sep 17, 2017

aledbf commented Sep 17, 2017

coveralls commented Sep 17, 2017

coveralls commented Sep 17, 2017

boazy commented Sep 18, 2017

jammm Sep 19, 2017

aledbf Sep 29, 2017

jammm Sep 19, 2017 •

edited

Loading

jammm Sep 19, 2017 •

edited

Loading

aledbf Sep 26, 2017

jammm Sep 19, 2017

jammm Sep 20, 2017

aledbf commented Sep 26, 2017

coveralls commented Sep 26, 2017

coveralls commented Sep 26, 2017

aledbf commented Sep 28, 2017 •

edited

Loading

coveralls commented Sep 29, 2017

coveralls commented Sep 29, 2017

coveralls commented Sep 29, 2017

coveralls commented Sep 29, 2017

rikatz commented Sep 29, 2017

aledbf commented Sep 29, 2017

coveralls commented Sep 29, 2017

rikatz commented Sep 29, 2017 •

edited

Loading

rikatz Sep 29, 2017 •

edited

Loading

aledbf commented Sep 29, 2017

aledbf commented Sep 29, 2017

coveralls commented Sep 29, 2017

rikatz commented Sep 29, 2017

aledbf commented Sep 29, 2017

Refactor X-Forwarded-* headers #1381

Refactor X-Forwarded-* headers #1381

Conversation

aledbf commented Sep 17, 2017 • edited Loading

k8s-reviewable commented Sep 17, 2017

aledbf commented Sep 17, 2017

coveralls commented Sep 17, 2017

coveralls commented Sep 17, 2017

boazy commented Sep 18, 2017

jammm Sep 19, 2017

Choose a reason for hiding this comment

aledbf Sep 29, 2017

Choose a reason for hiding this comment

jammm Sep 19, 2017 • edited Loading

Choose a reason for hiding this comment

jammm Sep 19, 2017 • edited Loading

Choose a reason for hiding this comment

aledbf Sep 26, 2017

Choose a reason for hiding this comment

jammm Sep 19, 2017

Choose a reason for hiding this comment

jammm Sep 20, 2017

Choose a reason for hiding this comment

aledbf commented Sep 26, 2017

coveralls commented Sep 26, 2017

coveralls commented Sep 26, 2017

aledbf commented Sep 28, 2017 • edited Loading

coveralls commented Sep 29, 2017

coveralls commented Sep 29, 2017

coveralls commented Sep 29, 2017

coveralls commented Sep 29, 2017

rikatz commented Sep 29, 2017

aledbf commented Sep 29, 2017

coveralls commented Sep 29, 2017

rikatz commented Sep 29, 2017 • edited Loading

rikatz Sep 29, 2017 • edited Loading

Choose a reason for hiding this comment

aledbf commented Sep 29, 2017

aledbf commented Sep 29, 2017

Default configuration

proxy-protocol

proxy-protocol

coveralls commented Sep 29, 2017

rikatz commented Sep 29, 2017

aledbf commented Sep 29, 2017

aledbf commented Sep 17, 2017 •

edited

Loading

jammm Sep 19, 2017 •

edited

Loading

jammm Sep 19, 2017 •

edited

Loading

aledbf commented Sep 28, 2017 •

edited

Loading

rikatz commented Sep 29, 2017 •

edited

Loading

rikatz Sep 29, 2017 •

edited

Loading