nginx: Option to not use neither incoming X-Forwarded headers nor Proxy Protocol.

[Dirbaio]

Follow up to #1309 #1668

nginx-ingress with GCE network load balancer allows spoofing source IP via X-Forwarded-For header, without any way to disable it.

Steps to reproduce:

• Create a k8s cluster on GKE or GCE.
• Deploy nginx-ingress deployment, configmap, etc
• Create the nginx-ingress service like this:

apiVersion: v1
kind: Service
metadata:
  name: ingress-nginx
  namespace: ingress-nginx
  labels:
    app: ingress-nginx
spec:
  externalTrafficPolicy: Local
  type: LoadBalancer
  loadBalancerIP: 1.2.3.4
  selector:
    app: ingress-nginx
  ports:
  - name: http
    port: 80
    targetPort: http
  - name: https
    port: 443
    targetPort: https

• This works, BUT end-users from the WAN can spoof their IP by sending an X-Forwarded-For header.

This setup creates a GCE "Network Load Balancer", which basically hashes TCP packets based on src/dest IP/ports and sends them to a random node unmodified, including the source IP addr. (it's NOT an "active" TCP proxy nor a L7 load balancer)

Note the externalTrafficPolicy: Local in the Service. This makes it so the source IP of the incoming TCP connections is preserved: it doesn't go through any NAT in kube-proxy. More info: https://kubernetes.io/docs/tutorials/services/source-ip/#source-ip-for-services-with-typeloadbalancer

Using Proxy Protocol is NOT a solution here: GCE NLB cannot do ProxyProtocol because it doesn't modify packets. The GCE TCP LB can, but I'd rather stick with NLB because it's much simpler (supported by k8s out of the box, externalTrafficPolicy: Local saves an extra network hop, and not having an active TCP proxy in the middle another one probably too.)

Looking through the template confirms that nginx-ingress must either use incoming X-Forwarded-For OR Proxy Protocol.

Ideally there should be an option to use neither (just use the source IP in the actual raw TCP connection).

[aledbf]

Ideally there should be an option to use neither (just use the source IP in the actual raw TCP connection).

even if you get the IP address of one of the nodes? (not using the externalTrafficPolicy field)

[Dirbaio]

I'd say yes.

It's up to you as the cluster manager to configure it correctly, or you get incorrect behavior. You're already getting incorrect behavior in this environment right now by trusting XFF, so adding this option wouldn't make it "more incorrect".

The docs here could be expanded to clarify it.

[maxlaverse]

Hi @Dirbaio
I hope you don't mind me jumping into the conversation. I'd like to understand the issue.

How can users spoof their IPs if you don't set the proxy-real-ip-cidr ?
My understanding it that with the GCE NLB and the service externalTrafficPolicy set, clients arrive with their true IP on the Ingress.

If proxy-real-ip-cidr is not set, the nginx realIP module wouldn't accept anything from the xff header and use the original remote_addr, isn't that right ?

[Dirbaio]

My understanding it that with the GCE NLB and the service externalTrafficPolicy set, clients arrive with their true IP on the Ingress.

Yes, correct.

If proxy-real-ip-cidr is not set, the nginx realIP module wouldn't accept anything from the xff header and use the original remote_addr, isn't that right ?

No. Not setting proxy-real-ip-cidr makes it accept xff from any IP. The nginx.conf looks like this:

    real_ip_header      X-Forwarded-For;
    real_ip_recursive   on;
    set_real_ip_from    0.0.0.0/0;
    ...
    map $http_x_forwarded_for $the_real_ip {
        default          $remote_addr;
    }
    server {
        ...
        location / {
            ...
            proxy_set_header X-Real-IP              $the_real_ip;
            proxy_set_header X-Forwarded-For        $the_real_ip;
            ...
        }
    }

and I can indeed spoof my IP. Making requests from outside the cluster to a test service that echos the received headers back:

$ curl https://example.com/
HEADERS RECEIVED:
...
X-Forwarded-For=x.x.x.x   <- My real IP

$ curl https://example.com/ -H 'X-Forwarded-For: 1.2.3.4'
HEADERS RECEIVED:
...
X-Forwarded-For=1.2.3.4   <-- SPOOFED

[Dirbaio]

Setting proxy-real-ip-cidr: 0.0.0.0/32 is a workaround, however partial. It only works for XFF, you can still spoof the other X-Forwarded-* headers when making requests from WAN.

curl https://example.com/ -H 'X-Forwarded-For: 1.2.3.4' -H 'X-Forwarded-Port: 1337' -H 'X-Forwarded-Host: spoof' -H 'X-Forwarded-Proto: spoof'
HEADERS RECEIVED:
Connection=close
Host=spoof
X-Forwarded-For=x.x.x.x   (my real IP)
X-Forwarded-Host=spoof
X-Forwarded-Port=1337
X-Forwarded-Proto=spoof
X-Original-URI=/
X-Real-IP= x.x.x.x (my real IP)
X-Scheme=spoof

[maxlaverse]

Good to know!
This default behavior should be written in the documentation (I'll open a PR if no one is quicker).
And maybe we could think of making it the default value. In practice, the situation where you blindly trust the header no matter the source are probably rare.

However, I'm not sure what to do about those additional headers X-Forwarded-Host/Port/Proto. They are not standard if I'm not mistaken and some contain single values (as opposite to X-Forwarded-For). They should probably contain the values as set by the last trusted proxy in the chain. If they had been standard, it would have been the task of the Nginx RealIP module to ensure that and eventually drop them.

One workaround to this issue is to move the logic checking those headers on the server side, activate compute-full-forwarded-for and ignore those headers if remote_addr doesn't match a trusted source.

[Dirbaio]

IMO the nginx-ingress config needs a bit of refactor to increase flexibility, make it less error-prone and fix the inconsistencies in the XF* headers.

Here's a bit of brainstorming:

We'd like the user to be able to configure nginx-ingress to handle these 3 "operation modes", depending on where it's serving requests from.

1. direct from WAN
2. from a LB speaking X-Forwarded-For headers
3. from a LB speaking Proxy Protocol.

Mode 1 - direct from WAN

Designed for: single hosts, Amazon NLB, GCE NLB, other L4 packet-based LBs doing direct return

Doesn't trust incoming X-Forwarded-* headers at all.
Sends the following headers to upstream:

• X-Forwarded-For, X-Real-IP: real source IP
• X-Forwarded-Port: real TCP listen port.
• X-Forwarded-Proto, X-Scheme: real http or https
• Host, X-Forwarded-Host: incoming Host header.
• X-Original-Forwarded-* contain copy of the original X-Forwarded-* from the client.

Mode 2 - from a LB speaking X-Forwarded-For headers

Designed for: Amazon ALB, GCE L7 LB, other L7 LBs.

Fully trusts incoming X-Forwarded-* headers.
Sends the following headers to upstream:

• X-Forwarded-For, X-Real-IP: Pass through
• X-Forwarded-Port: Pass through
• X-Forwarded-Proto, X-Scheme: Pass through.
• Host, X-Forwarded-Host: incoming Host header.
• X-Original-Forwarded-* Pass through (in case the frontmost LB has filled them)

Mode 3. from a LB speaking Proxy Protocol.

Designed for: Amazon ELB in TCP/SSL modes (?), GCE TCP LB, GCE SSL LB, haproxy with Proxy Protocol, other TCP proxy LBs.

Only trusts Proxy Protocol. Doesn't trust incoming X-Forwarded-* headers.
Sends the following headers to upstream:

• X-Forwarded-For, X-Real-IP: source IP from Proxy Protocol.
• X-Forwarded-Port: listen port from Proxy Protocol.
• X-Forwarded-Proto, X-Scheme: real http or https
• Host, X-Forwarded-Host: incoming Host header.
• X-Original-Forwarded-* contain copy of the original X-Forwarded-* from the client.

My proposal for changes

• Add a config option: use-incoming-x-forwarded-headers. IMO it should be default false, but if you want to keep backwards compatibility, set it to default true.
• use-incoming-x-forwarded-headers triggers mode 2. use-proxy-protocol triggers mode 3. None enabled triggers mode 1. Both enabled is not supported (?)
• Document exactly what each mode does to the headers, like I did above.

proxy-real-ip-cidr

Currently the behavior of proxy-real-ip-cidr is a bit broken:

Using headers: If the IP is whitelisted, it trusts XFF (mode 2), otherwise it falls back to real IP (mode 1). That's only for XFF. It still blindly trusts other XF headers no matter the IP...

Using proxy protocol: If the IP is whitelisted, it uses the source IP in the ProxyProtocol header. If not, it will still expect a ProxyProtocol header, but ignore the info in it, which is weird.

I really can't think of a case where you would actually want a "mixed" mode like this. Your traffic either comes from another LB or directly. Also, if nginx-ingress is behind an LB it shouldn'be exposed to WAN to begin with. Maybe proxy-real-ip-cidr should be deprecated/discouraged/removed?

[jpalomaki]

@Dirbaio See also #1779

[maxlaverse]

My opinion on your proposition @Dirbaio

Mode 1 - direct from WAN

Are X-Original-Forwarded-* really needed/used in that case ? We already choose not to trust any proxy incoming header and we expect the Ingress to be directly in front of the clients.

Mode 2 - from a LB speaking X-Forwarded-For headers

Since I wrote the compute-full-forwarded-for option, I'm of course in favor of having it active in that case, meaning that we append the $remote_addr to x-forwarded-for to preserve the proxy chain (as other L7 proxies do).

Mode 3 - from a LB speaking Proxy Protocol

We can't trust X-Forwarded-Host. Also, does it make sense to set X-Forwarded-Proto ? It's not part of the information sent in the proxy protocol right ? The only close info we have is the port the proxy was reached on.

My proposal for changes

• Add a config option: use-incoming-x-forwarded-headers. IMO it should be default false, but if you want to keep backwards compatibility, set it to default true.
• use-incoming-x-forwarded-headers triggers mode 2. use-proxy-protocol triggers mode 3. None enabled triggers mode 1. Both enabled is not supported (?)
• Document exactly what each mode does to the headers, like I did above.

Sounds good

proxy-real-ip-cidr

RealIP doesn't work with headers other than X-Forwarded-For because they are not standard. I can't see a way to treat the X-Forwarded-* headers as a whole without patching the module.

Deprecating/dropping proxy-real-ip-cidr sounds extrem, but it would definitely make the Nginx Ingress behavior much more easier to configure and to understand. The default value is to trust everything anyway. At least the documentation should clearly state the limitation that the headers other than xff won't be properly set when using proxy-real-ip-cidr

[Dirbaio]

Are X-Original-Forwarded-* really needed/used in that case ? We already choose not to trust any proxy incoming header and we expect the Ingress to be directly in front of the clients.

I added it because the current template does that. It was added in 0755231 without much explanation. @aledbf what was the rationale behind it? is it something we should keep?

Since I wrote the compute-full-forwarded-for option, I'm of course in favor of having it active in that case,

I think it's a safer choice to keep it off by default. I'm afraid some backend may break with multiple IPs in the header. Having it off works for any backend, and will show the end-user real IP, which is all you need most of the time. If the user cares about the middle proxy IPs he can always enable it after making sure his backend handles it correctly.

[in proxy protocol] We can't trust X-Forwarded-Host

I meant the real http Host header, not the X-Forwarded-Host header. The value in X-Forwarded-Host is discarded.

Also, does it make sense to set X-Forwarded-Proto ? It's not part of the information sent in the proxy protocol right ? The only close info we have is the port the proxy was reached on.

Yes, it is needed. Imagine a backend receives X-Forwarded-Port: 1234 and wants to build an absolute link back to itself. It can't know whether it's http://example.com:1234 or https://example.com:1234.

The value would come from whether nginx is accepting that request with SSL or not. This works fine as long as there are only TCP proxies. This would break with Google SSL Proxy load balancer, for example.

Deprecating/dropping proxy-real-ip-cidr sounds extrem, but it would definitely make the Nginx Ingress behavior much more easier to configure and to understand

+10000

[edevil]

In Azure the load balancer is L4 and so clients arrive with their real IP address. Therefore the X-Forwarder-X headers should not be trusted/used.

From reading this issue I gather that the only way to achieve this is to set proxy-real-ip-cidr: 0.0.0.0/32 and this only works for XFF?

[Dirbaio]

the only way to achieve this is to set proxy-real-ip-cidr: 0.0.0.0/32 and this only works for XFF?

Yes, you're correct.

What's the status on this @aledbf @maxlaverse ? If we get consensus on how it should work, I can go ahead and update my PR to move this forward.

[mrparkers]

@Dirbaio Thanks for posting the workaround. I was just bit by this, and I am also in favor of a more explicit configmap setting to override this behavior. I'm happy to help with this wherever I can.

[sschwenker]

IMO. What about having 4 cases then?

Mode 1: Direct from Wan
No proxy or other headers added. This implementation should probably strip any proxy headers.

Mode 2: Proxy from Wan
Implements Proxy headers for back end.

Mode 3: from a LB speaking X-Forwarded-For headers
Same as above

Mode 4: from a LB speaking Proxy Protocol
Same as above.

For configuration, I would recommend having a single key configuration for setting the mode in which you would like it to act. Having multiple key value pairs that control the mode could confuse things.

[chriswessels]

+1. We're after Mode 1.

[albertvaka]

For the record, in case anyone wants to fix this:

There was a patch for this security issue, but it went ignored: #1851

[jpalomaki]

FWIW, running in mode 1 (behind GCE network LB), specifying:

use-proxy-protocol: "false"
proxy-real-ip-cidr: "<LB external IP>/32"

and using a custom nginx.tmpl with these parts minimally customized:

...
# Do not trust X-Forwarded-Proto
map $http_x_forwarded_proto $pass_access_scheme {
    default          $scheme;
}

# Redirect to https if scheme is http
map $pass_access_scheme $redirect_to_https {
    default          0;
    http             1;
}

# Do not trust X-Forwarded-Port
map $http_x_forwarded_port $pass_server_port {
    default          $server_port;
}
...
# Do not trust X-Forwarded-Host
map $http_x_forwarded_host $best_http_host {
     default         $this_host;
}

seems to do the trick for me (prevent spoofing via X-Forwarded-*).

See https://github.com/kubernetes/ingress-nginx/blob/master/docs/user-guide/nginx-configuration/custom-template.md and https://github.com/kubernetes/ingress-nginx/blob/master/rootfs/etc/nginx/template/nginx.tmpl