Correct nginx tmpl

[rikatz]

This is an improvement to NGINX template security, with the following logic:

• Is the 'UseProxyProtocol' true? Then accept headers sent by Proxy to NGINX, real_ip_header and others
• Is the 'UseProxyProtocol' false (default)? Then use the default variables as real_ip_header

Additionally, I've made a correction in template in line 262 putting this comment as inside template comment, instead of a repeat comment for each upstream.

This solves #1000

[k8s-reviewable]

This change is 

[rikatz]

@aledbf Please take a look at the CI, apparently the first check failed but passed anyway :)

[aledbf]

@rikatz this is not a solution. If you are running in aws using an elb in http mode we need all this headers
http://docs.aws.amazon.com/elasticloadbalancing/latest/classic/x-forwarded-headers.html
https://github.com/kubernetes/ingress/tree/master/examples/tls-termination/elb-nginx

[coveralls]

Coverage remained the same at 44.384% when pulling f49e756 on estaleiro:correct-nginx-tmpl into 3463ddb on kubernetes:master.

[rikatz]

@aledbf Is this the case to create a flag like 'notCloud deployment' to protect this?

[rikatz]

@aledbf I'm refactoring this, creating a new config in ConfigMap called cloud-provider-headers, defaulting it to false, and changing the nginx.tmpl as the following:

map "$cloudproviderheaders" $the_real_ip {
        "true"                 $http_x_forwarded_for;
        default                $realip_remote_addr;
    }
    {{ end }}

and then in SERVER template, doing this:

{{ if $cfg.CloudProviderHeaders }} set $cloudproviderheaders "true"; {{end}}

Does this make any sense? This way, if You're running Ingress Controller inside Amazon/GCE you set this config to 'true', otherwise it will use the remote_addr :)

Also, I'll probably open a different PR, as I've made some mess here in my repo :)

Thanks!

[aledbf]

Is this the case to create a flag like 'notCloud deployment' to protect this?

I think we should inspect the --publish-service and check if the service is type=LoadBalancer or add something like https://github.com/appscode/cloudid than adding another flag for this.

[rikatz]

OK, sounds good! I'll take a look in that and will let you know

[rikatz]

@aledbf I've been looking into the docs and it's not guaranted that GCE users will use the publish-service flag. Also It's not a must to AWS users.

So I'll try to achieve this by using the cloudid verification, is that a problem using a new external lib?

EDIT: cloudid seems to detect wrong cloudid here. I'm running a baremetal controller with docker in my notebook and it detects a GCE cloud.

Will change the approach to detecting if service type is LoadBalancer.

Thanks

[rikatz]

@aledbf sorry for taking so long, I'm pretty lost here. I'm trying to figure out where I put the right code. I'm trying to follow the same logic as 'default-backend-service' here, but I'm not able to use the config here.

I've seen there's a piece of code here that could be used, but don't know if launch.go is the best place to put my code.

Also, I'm trying to figure out how to use this information further, to set a config that could be used in nginx.tmpl.

Can you provide me some light? :)

Thanks!

[boazy]

The current and change configuration still poses a security risk in some (or most?) cases.
If a public load balancer is using the TCP proxy protocol (e.g. Google Cloud TCP Proxy configured to use the proxy protocol) to forward traffic to nginx, an attacker can just set the X-Forwarded-For header (or any of the other header of the X-Forwarded-* family) and spoof their IP address, host, etc.

It's very easy to override Ingress whitelisting this way. Let's say I whitelist only IPs in the range: 144.13.99.0/24

A smart adversary can just do this:
curl -H 'X-Forwarded-For: 144.13.99.1' https://my-host.com/very-secret-api

The best practice to prevent with the Forwarded headers is to always clear them or overwrite them unless specified otherwise in a configuration flag. In this case, we should probably have a different configuration flag than UseProxyProtocol, since the HTTP Forwarded headers and the TCP proxy protocol are orthogonal and more often than not mutually exclusive. I don't understand why they're bundled together here.

[rikatz]

You're right. But isn't this only the case now to flag if it is, or isn't a LoadBalancer service and then deal with that in nginx.tmpl?

I'll finish modifying the code here to support this kind of detection, and then we can use this flag in nginx.tmpl to take actions.

[rikatz]

Also, clarify me something (as I don't use GCE). When we have a LoadBalancer, can the ExternalIP from the LoadBalancer be used as a trustable source of IP? Or they're different (internal and external IPs)? Is there somewhere a spec/status in Service object that says which is the internal Load Balancer IP?

If this is the case, doesn't google/AWS loadbalancers sanitize this before passing them to the next hop (in our case, NGINX)?

[rikatz]

OK, I've made this commit

Which adds the support to detect the publish-service type.

@boazy take a look that I set here only as a comment the directive fetched from the Service Object.

Maybe, If we can grab also the internal IP address we could set this as the realip trusted source, otherwise we could respect the protocol and ignore any X-Forwarded-For sent header.

Also we have to deal with situations where the user uses an external proxy (not ELB or AWS), like a cache (Varnish as an example), and then we respect this if user forces the 'use proxy protocol' directive.

Thanks

[boazy]

In my particular case I don't even have a LoadBalancer service set in GCE (since a LoadBalancer service doesn't support global load balancing, and as far as I remember doesn't even support the proxy protocol).

I configured my ingress controller as DaemonSet listening on NodePort and manually set up GCE load balancers pointing there. So looking at the Service type won't solve it in my case. It's also not useful for people who want to set up an L4 load balancer themselves using HAProxy. So it looks like this would only solve the issue for ELB and some GCE LB configurations.

I would really prefer a separate setting, even as an override.

[rikatz]

OK, so maybe this is the case that we have a directive in ConfigMap for that (defaulting to false), but that's overriden by publish-service type?

Also, this config that I'm doing here enforces the usage of real ip address and only uses the X-Forwarded-For if the publish-service is LoadBalancer (didn't changed the nginx.tmpl here yet). There's also a config in ConfigMap with trusted ips that could be used.

[boazy]

Yes, but if the service is not a LoadBalancer you enforce the use of $remote_addr instead of $real_ip_remote_addr.

What I'm saying is that there are too many use-cases to rely on "smart" autodetection. The best approach would be to:

1. Detect LoadBalancer service type and depending on UseProxyProtocol use either ONLY $real_ip_remote_addr(from Proxy protocol) ifUseProxyProtocol = true` or ONLY the HTTP Forwarded family of header otherwise.
2. If no load balancer is set use $remote_addr by default and ignore all the forwarded headers - like you're doing now.
3. Have a new config setting real-client-from with 4 possible values:
   default Use automatically detected settings from 1 and 2.
   direct Use $remote_addr directly
   tcp-proxy Use Real IP from proxy protocol.
   http-proxy Use HTTP forwarded headers from proxy.

For clarification:
By HTTP Forwarded Family I mean X-Forwarded-For, X-Forwarded-Host, X-Forwarded-Port X-Real-IP, etc.

[boazy]

You can even just call default autodetect and then detect which behavior you want (direct, tcp-proxy, http-proxy) by looking at whether Service.Type == 'Balancer' and cfg.UseProxyProtocol == true.

[rikatz]

@boazy OK, so let's turn this into code.

1. Detect if the publish service is Type LoadBalancer. If this is the case, we should set a variable in nginx.tmpl that sets the remote_addr as the X-Forwarded-For

2. If this is not the case, then we should look into the 'trusted-ip' array to check if there's something set there. In this case, we set the trusted_ip from this array, and use the remote_addr as X-Forwarded-For

3. If you're using the Proxy Protocol, the same applies.

4. If none of the above is true, so we set the IP Address directly from remote_addr, and ignore anything else

Does this sounds good?

[boazy]

@rikatz Sorry for the late reply

I think you misunderstood me (or perhaps I misunderstood you?)

This should all depend on these variables:

cfg.RealClientFrom indicates the method for getting the client data.
It can be auto, in which case we'll use other information to guess the right method.

cfg.UseProxyProtocol if true, and cfg.RealClientFrom is set to auto, then in
99.999% of the times we want to get the IP address from the proxy protocol.
In the other times we can just explicitly set cfg.RealClientFrom to something else.

cfg.ServiceType if it's 'LoadBalancer' we and cfg.UseProxyProtocol is not set,
we can guess that we should use X-Forwarded-For headers (only if using auto mode
of course).

cfg.ProxyRealIPCIDR if this array is not empty we should use $realip_remote_addr instead of
$http_x_forwarded_for or $proxy_protocol_addr - in case we were planning on using them based
on other settings.
I'm not 100% sure about this logic but it seems reasonable to me.

Putting that into go template code it would look like this:

{{ if (or (eq $cfg.RealClientFrom "http-proxy") (and (eq $cfg.RealClientFrom "auto") (ne $cfg.UseProxyProtocol) (eq $cfg.ServiceType "LoadBalancer"))) }}
# Trust HTTP X-Forwarded-* Headers, but use direct values if they're missing.

map $http_x_forwarded_for $the_real_ip {
    # Get IP address from X-Forwarded-For HTTP header
{{ if (ne (len $cfg.ProxyRealIPCIDR) 0) }}
    # using trusted real IP CIDR
    default          $realip_remote_addr;
{{ else }}
    default          $http_x_forwarded_for;
{{ end }}
   ''                $remote_addr;
}

# trust http_x_forwarded_proto headers correctly indicate ssl offloading
map $http_x_forwarded_proto $pass_access_scheme {
    default          $http_x_forwarded_proto;
    ''               $scheme;
}

map $http_x_forwarded_port $pass_server_port {
    default           $http_x_forwarded_port;
    ''                $server_port;
}

map $http_x_forwarded_host $best_http_host {
    default          $http_x_forwarded_host;
    ''               $this_host;
}

{{ else}}
# Do not trust HTTP X-Forwarded-* Headers

map $http_x_forwarded_for $the_real_ip {
{{ if (or (eq $cfg.RealClientFrom "tcp-proxy") (and (eq $cfg.RealClientFrom "auto") ($cfg.UseProxyProtocol))) }}
    # Get IP address from Proxy Protocol
{{   if (ne (len $cfg.ProxyRealIPCIDR) 0) }}
    # using trusted real IP CIDR
    default          $realip_remote_addr;
{{   else }}
    default          $proxy_protocol_addr;
{{   end }}
{{ else }}
    # Get IP from direct remote address
    default          $remote_addr;
{{ end }}
}

map $http_x_forwarded_host $best_http_host {
    default          $this_host;
}
map $http_x_forwarded_proto $pass_access_scheme {
    default          $scheme;
}
map $http_x_forwarded_port $pass_server_port {
   default           $server_port;
}

{{ end }}

[aledbf]

@rikatz any update on this?

[rikatz]

I'm on vacation right now, so will be able to fully work on that next week :) Em 7 de set de 2017 1:59 PM, "Manuel Alejandro de Brito Fontes" < notifications@github.com> escreveu: @rikatz <https://github.com/rikatz> any update on this? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#1222 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AG2YBX5H6gz2fQnIL3AgvQQT_6lNV7vsks5sgCDBgaJpZM4O_Jy-> .

[rikatz]

@aledbf @boazy I've some personal problems here and will not be able to solve this PR until Sep, 21st. Should I leave this open for historic reasons, or do you prefer me to close this and we shall start working in another PR / issue?

Thanks!

[aledbf]

@rikatz do you mind if I use this code and continue with the work?

[rikatz]

@aledbf No problem at all! Please, and let me know if you need someone to test this later :)

I'm really sorry about not being able to continue this right now. Do you want access to our original repo, or are you going to work directly here?

Thanks!!