Merge pull request #1485 from aledbf/rd

Fix source IP address

controllers/nginx/README.md

@@ -14,6 +14,7 @@ This is an nginx Ingress controller that uses [ConfigMap](https://kubernetes.io/
   * [HTTPS enforcement](#server-side-https-enforcement)
   * [HSTS](#http-strict-transport-security)
   * [Kube-Lego](#automated-certificate-management-with-kube-lego)
+* [Source IP address](#source-ip-address)
 * [TCP Services](#exposing-tcp-services)
 * [UDP Services](#exposing-udp-services)
 * [Proxy Protocol](#proxy-protocol)
@@ -333,6 +334,13 @@ version to fully support Kube-Lego is nginx Ingress controller 0.8.
 [Kube-Lego]:https://github.com/jetstack/kube-lego
 [Let's Encrypt]:https://letsencrypt.org
 
+## Source IP address
+
+By default NGINX uses the content of the header `X-Forwarded-For` as the source of truth to get information about the client IP address. This works without issues in L7 **if we configure the setting `proxy-real-ip-cidr`** with the correct information of the IP/network address of the external load balancer. 
+If the ingress controller is running in AWS we need to use the VPC IPv4 CIDR. This allows NGINX to avoid the spoofing of the header.
+Another option is to enable proxy protocol using `use-proxy-protocol: "true"`.
+In this mode NGINX do not uses the content of the header to get the source IP address of the connection.
+
 ## Exposing TCP services
 
 Ingress does not support TCP services (yet). For this reason this Ingress controller uses the flag `--tcp-services-configmap` to point to an existing config map where the key is the external port to use and the value is `<namespace/service name>:<service port>:[PROXY]:[PROXY]`

controllers/nginx/pkg/config/config.go

@@ -261,11 +261,6 @@ type Configuration struct {
 	// https://nginx.org/en/docs/http/ngx_http_proxy_module.html#proxy_headers_hash_bucket_size
 	ProxyHeadersHashBucketSize int `json:"proxy-headers-hash-bucket-size,omitempty"`
 
-	// RealClientFrom defines the trusted source of the client source IP address
-	// The valid values are "auto", "http-proxy" and "tcp-proxy"
-	// Default: auto
-	RealClientFrom string `json:"real-client-from,omitempty"`
-
 	// Enables or disables emitting nginx version in error messages and in the “Server” response header field.
 	// http://nginx.org/en/docs/http/ngx_http_core_module.html#server_tokens
 	// Default: true
@@ -479,7 +474,6 @@ func NewDefault() Configuration {
 		LimitConnZoneVariable:        defaultLimitConnZoneVariable,
 		BindAddressIpv4:              defBindAddress,
 		BindAddressIpv6:              defBindAddress,
-		RealClientFrom:               "auto",
 		ZipkinCollectorPort:          9411,
 		ZipkinServiceName:            "nginx",
 	}

controllers/nginx/pkg/template/configmap.go

@@ -19,7 +19,6 @@ package template
 import (
 	"fmt"
 	"net"
-	"regexp"
 	"strconv"
 	"strings"
 
@@ -39,10 +38,6 @@ const (
 	bindAddress          = "bind-address"
 )
 
-var (
-	realClientRegex = regexp.MustCompile(`auto|http-proxy|tcp-proxy`)
-)
-
 // ReadConfig obtains the configuration defined by the user merged with the defaults.
 func ReadConfig(src map[string]string) config.Configuration {
 	conf := map[string]string{}
@@ -125,11 +120,6 @@ func ReadConfig(src map[string]string) config.Configuration {
 		glog.Warningf("unexpected error merging defaults: %v", err)
 	}
 
-	if !realClientRegex.MatchString(to.RealClientFrom) {
-		glog.Warningf("unexpected value for RealClientFromSetting (%v). Using default \"auto\"", to.RealClientFrom)
-		to.RealClientFrom = "auto"
-	}
-
 	return to
 }
 

controllers/nginx/pkg/template/template.go

@@ -152,8 +152,6 @@ var (
 		},
 		"isValidClientBodyBufferSize": isValidClientBodyBufferSize,
 		"buildForwardedFor":           buildForwardedFor,
-		"trustHTTPHeaders":            trustHTTPHeaders,
-		"trustProxyProtocol":          trustProxyProtocol,
 		"buildAuthSignURL":            buildAuthSignURL,
 	}
 )
@@ -671,28 +669,6 @@ func buildForwardedFor(input interface{}) string {
 	return fmt.Sprintf("$http_%v", ffh)
 }
 
-func trustHTTPHeaders(input interface{}) bool {
-	conf, ok := input.(config.TemplateConfig)
-	if !ok {
-		glog.Errorf("%v", input)
-		return true
-	}
-
-	return conf.Cfg.RealClientFrom == "http-proxy" ||
-		(conf.Cfg.RealClientFrom == "auto" && !conf.Cfg.UseProxyProtocol)
-}
-
-func trustProxyProtocol(input interface{}) bool {
-	conf, ok := input.(config.TemplateConfig)
-	if !ok {
-		glog.Errorf("%v", input)
-		return true
-	}
-
-	return conf.Cfg.RealClientFrom == "tcp-proxy" ||
-		(conf.Cfg.RealClientFrom == "auto" && conf.Cfg.UseProxyProtocol)
-}
-
 func buildAuthSignURL(input interface{}) string {
 	s, ok := input.(string)
 	if !ok {
@@ -703,12 +679,12 @@ func buildAuthSignURL(input interface{}) string {
 	u, _ := url.Parse(s)
 	q := u.Query()
 	if len(q) == 0 {
-		return fmt.Sprintf("%v?rd=$request_uri", s)
+		return fmt.Sprintf("%v?rd=$scheme://$http_host$request_uri", s)
 	}
 
 	if q.Get("rd") != "" {
 		return s
 	}
 
-	return fmt.Sprintf("%v&rd=$request_uri", s)
+	return fmt.Sprintf("%v&rd=$scheme://$http_host$request_uri", s)
 }

controllers/nginx/pkg/template/template_test.go

@@ -359,8 +359,8 @@ func TestBuildAuthSignURL(t *testing.T) {
 	cases := map[string]struct {
 		Input, Output string
 	}{
-		"default url":       {"http://google.com", "http://google.com?rd=$request_uri"},
+		"default url":       {"http://google.com", "http://google.com?rd=$scheme://$http_host$request_uri"},
-		"with random field": {"http://google.com?cat=0", "http://google.com?cat=0&rd=$request_uri"},
+		"with random field": {"http://google.com?cat=0", "http://google.com?cat=0&rd=$scheme://$http_host$request_uri"},
 		"with rd field":     {"http://google.com?cat&rd=$request", "http://google.com?cat&rd=$request"},
 	}
 	for k, tc := range cases {

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

@@ -151,12 +151,13 @@ http {
         ''               close;
     }
 
-    {{ if (trustHTTPHeaders $all) }}
-    # Trust HTTP X-Forwarded-* Headers, but use direct values if they're missing.
     map {{ buildForwardedFor $cfg.ForwardedForHeader }} $the_real_ip {
-        # Get IP address from X-Forwarded-For HTTP header
+    {{ if $cfg.UseProxyProtocol }}
-        default          $realip_remote_addr;
+        # Get IP address from Proxy Protocol
-        ''               $remote_addr;
+        default          $proxy_protocol_addr;
+    {{ else }}
+        default          $remote_addr;
+    {{ end }}
     }
 
     # trust http_x_forwarded_proto headers correctly indicate ssl offloading
@@ -175,30 +176,6 @@ http {
         ''               $this_host;
     }
 
-    {{ else }}
-    # Do not trust HTTP X-Forwarded-* Headers
-    map {{ buildForwardedFor $cfg.ForwardedForHeader }} $the_real_ip {
-    {{ if (trustProxyProtocol $all) }}
-        # Get IP address from Proxy Protocol
-        default          $proxy_protocol_addr;
-    {{ else }}
-        # Get IP from direct remote address
-        default          $realip_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 }}
-
     {{ if $all.IsSSLPassthroughEnabled }}
     # map port {{ $all.ListenPorts.SSLProxy }} to 443 for header X-Forwarded-Port
     map $pass_server_port $pass_port {
@@ -212,21 +189,6 @@ http {
     }
     {{ end }}
 
-    # Map a response error watching the header Content-Type
-    map $http_accept $httpAccept {
-        default          html;
-        application/json json;
-        application/xml  xml;
-        text/plain       text;
-    }
-
-    map $httpAccept $httpReturnType {
-        default          text/html;
-        json             application/json;
-        xml              application/xml;
-        text             text/plain;
-    }
-
     # Obtain best http host
     map $http_host $this_host {
         default          $http_host;
@@ -688,8 +650,8 @@ stream {
 
         {{ end }}
 
-        location {{ $path }} {
 
+        location {{ $path }} {
             {{ if $all.Cfg.EnableVtsStatus }}{{ if $location.VtsFilterKey }} vhost_traffic_status_filter_by_set_key {{ $location.VtsFilterKey }};{{ end }}{{ end }}
 
             set $proxy_upstream_name "{{ buildUpstreamName $server.Hostname $all.Backends $location }}";
@@ -786,6 +748,9 @@ stream {
             proxy_set_header X-Original-URI         $request_uri;
             proxy_set_header X-Scheme               $pass_access_scheme;
 
+            # Pass the original X-Forwarded-For
+            proxy_set_header X-Original-Forwarded-For {{ buildForwardedFor $all.Cfg.ForwardedForHeader }};
+
             # mitigate HTTPoxy Vulnerability
             # https://www.nginx.com/blog/mitigating-the-httpoxy-vulnerability-with-nginx/
             proxy_set_header Proxy                  "";