From 67b253a149965b0d7fe5aa356b2ff8bcffe55dbe Mon Sep 17 00:00:00 2001 From: Dario Nieuwenhuis Date: Mon, 25 Dec 2017 22:19:01 +0100 Subject: [PATCH 1/6] Add use-forwarded-headers configmap option. --- internal/ingress/controller/config/config.go | 4 ++ rootfs/etc/nginx/template/nginx.tmpl | 62 +++++++++++++------- 2 files changed, 46 insertions(+), 20 deletions(-) diff --git a/internal/ingress/controller/config/config.go b/internal/ingress/controller/config/config.go index 436bb0612e..c8151f4afe 100644 --- a/internal/ingress/controller/config/config.go +++ b/internal/ingress/controller/config/config.go @@ -427,6 +427,9 @@ type Configuration struct { // Sets the ipv6 addresses on which the server will accept requests. BindAddressIpv6 []string `json:"bind-address-ipv6,omitempty"` + // Sets whether to use incoming X-Forwarded headers. + UseForwardedHeaders bool `json:"use-forwarded-headers"` + // Sets the header field for identifying the originating IP address of a client // Default is X-Forwarded-For ForwardedForHeader string `json:"forwarded-for-header,omitempty"` @@ -559,6 +562,7 @@ func NewDefault() Configuration { EnableDynamicTLSRecords: true, EnableUnderscoresInHeaders: false, ErrorLogLevel: errorLevel, + UseForwardedHeaders: false, ForwardedForHeader: "X-Forwarded-For", ComputeFullForwardedFor: false, ProxyAddOriginalUriHeader: true, diff --git a/rootfs/etc/nginx/template/nginx.tmpl b/rootfs/etc/nginx/template/nginx.tmpl index c258ab2e38..c1679afaa1 100644 --- a/rootfs/etc/nginx/template/nginx.tmpl +++ b/rootfs/etc/nginx/template/nginx.tmpl @@ -75,7 +75,10 @@ http { } {{ end }} {{ end }} - {{/* we use the value of the header X-Forwarded-For to be able to use the geo_ip module */}} + + {{/* Enable the real_ip module only if we use either X-Forwarded headers or Proxy Protocol. */}} + {{/* we use the value of the real IP for the geo_ip module */}} + {{ if or $cfg.UseForwardedHeaders $cfg.UseProxyProtocol }} {{ if $cfg.UseProxyProtocol }} real_ip_header proxy_protocol; {{ else }} @@ -86,6 +89,7 @@ http { {{ range $trusted_ip := $cfg.ProxyRealIPCIDR }} set_real_ip_from {{ $trusted_ip }}; {{ end }} + {{ end }} {{ if $cfg.UseGeoIP }} {{/* databases used to determine the country depending on the client IP address */}} @@ -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" + # Needed because using set is not allowed outside server blocks. + map '' $the_real_ip { {{ if $cfg.UseProxyProtocol }} # Get IP address from Proxy Protocol default $proxy_protocol_addr; @@ -231,12 +237,44 @@ http { {{ end }} } + {{ if $cfg.UseForwardedHeaders }} # 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; + } + + # Obtain best http host + map $http_host $this_host { + default $http_host; + '' $host; + } + + map $http_x_forwarded_host $best_http_host { + default $http_x_forwarded_host; + '' $this_host; + } + {{ else }} + map '' $pass_access_scheme { + default $scheme; + } + + map '' $pass_server_port { + default $server_port; + } + + # Obtain best http host + map $http_host $best_http_host { + default $http_host; + '' $host; + } + {{ end }} + # validate $pass_access_scheme and $scheme are http to force a redirect map "$scheme:$pass_access_scheme" $redirect_to_https { default 0; @@ -244,11 +282,6 @@ http { "https:http" 1; } - map $http_x_forwarded_port $pass_server_port { - default $http_x_forwarded_port; - '' $server_port; - } - {{ if $all.IsSSLPassthroughEnabled }} # map port {{ $all.ListenPorts.SSLProxy }} to 443 for header X-Forwarded-Port map $pass_server_port $pass_port { @@ -262,17 +295,6 @@ http { } {{ end }} - # Obtain best http host - map $http_host $this_host { - default $http_host; - '' $host; - } - - map $http_x_forwarded_host $best_http_host { - default $http_x_forwarded_host; - '' $this_host; - } - # Reverse proxies can detect if a client provides a X-Request-ID header, and pass it on to the backend server. # If no such header is provided, it can provide a random value. map $http_x_request_id $req_id { @@ -282,7 +304,7 @@ http { {{ end }} } - {{ if $cfg.ComputeFullForwardedFor }} + {{ if and $cfg.UseForwardedHeaders $cfg.ComputeFullForwardedFor }} # We can't use $proxy_add_x_forwarded_for because the realip module # replaces the remote_addr too soon map $http_x_forwarded_for $full_x_forwarded_for { @@ -1028,7 +1050,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 }} {{ $proxySetHeader }} X-Forwarded-For $full_x_forwarded_for; {{ else }} {{ $proxySetHeader }} X-Forwarded-For $the_real_ip; From 35a6d508fbdee0116005a2dfdcc790b0ca347169 Mon Sep 17 00:00:00 2001 From: Dario Nieuwenhuis Date: Tue, 12 Jun 2018 21:33:34 +0200 Subject: [PATCH 2/6] Set use-forwarded-headers to true by default. --- internal/ingress/controller/config/config.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/internal/ingress/controller/config/config.go b/internal/ingress/controller/config/config.go index c8151f4afe..2e1c36c339 100644 --- a/internal/ingress/controller/config/config.go +++ b/internal/ingress/controller/config/config.go @@ -562,7 +562,7 @@ func NewDefault() Configuration { EnableDynamicTLSRecords: true, EnableUnderscoresInHeaders: false, ErrorLogLevel: errorLevel, - UseForwardedHeaders: false, + UseForwardedHeaders: true, ForwardedForHeader: "X-Forwarded-For", ComputeFullForwardedFor: false, ProxyAddOriginalUriHeader: true, From fa626a605f575be516f2112f6d838bdda46f21e6 Mon Sep 17 00:00:00 2001 From: Dario Nieuwenhuis Date: Tue, 12 Jun 2018 21:33:53 +0200 Subject: [PATCH 3/6] Add use-forwarded-headers e2e test. --- test/e2e/settings/forwarded_headers.go | 123 +++++++++++++++++++++++++ 1 file changed, 123 insertions(+) create mode 100644 test/e2e/settings/forwarded_headers.go diff --git a/test/e2e/settings/forwarded_headers.go b/test/e2e/settings/forwarded_headers.go new file mode 100644 index 0000000000..c8eee1b5e2 --- /dev/null +++ b/test/e2e/settings/forwarded_headers.go @@ -0,0 +1,123 @@ +/* +Copyright 2017 The Kubernetes Authors. + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +package settings + +import ( + "fmt" + "io/ioutil" + "net" + "strings" + + . "github.com/onsi/ginkgo" + . "github.com/onsi/gomega" + + "k8s.io/ingress-nginx/test/e2e/framework" +) + +var _ = framework.IngressNginxDescribe("X-Forwarded headers", func() { + f := framework.NewDefaultFramework("forwarded-headers") + + setting := "use-forwarded-headers" + + BeforeEach(func() { + err := f.NewEchoDeployment() + Expect(err).NotTo(HaveOccurred()) + + err = f.UpdateNginxConfigMapData(setting, "false") + Expect(err).NotTo(HaveOccurred()) + }) + + AfterEach(func() { + }) + + It("should trust X-Forwarded headers when setting is true", func() { + host := "forwarded-headers" + + err := f.UpdateNginxConfigMapData(setting, "true") + Expect(err).NotTo(HaveOccurred()) + + ing, err := f.EnsureIngress(framework.NewSingleIngress(host, "/", host, f.IngressController.Namespace, "http-svc", 80, nil)) + Expect(err).NotTo(HaveOccurred()) + Expect(ing).NotTo(BeNil()) + + err = f.WaitForNginxServer(host, + func(server string) bool { + return strings.Contains(server, "server_name forwarded-headers") + }) + Expect(err).NotTo(HaveOccurred()) + + ip, err := f.GetNginxIP() + Expect(err).NotTo(HaveOccurred()) + port, err := f.GetNginxPort("http") + Expect(err).NotTo(HaveOccurred()) + + conn, err := net.Dial("tcp", fmt.Sprintf("%v:%v", ip, port)) + 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")) + + data, err := ioutil.ReadAll(conn) + Expect(err).NotTo(HaveOccurred()) + body := string(data) + Expect(body).Should(ContainSubstring(fmt.Sprintf("host=myhost"))) + Expect(body).Should(ContainSubstring(fmt.Sprintf("x-forwarded-host=myhost"))) + Expect(body).Should(ContainSubstring(fmt.Sprintf("x-forwarded-proto=myproto"))) + Expect(body).Should(ContainSubstring(fmt.Sprintf("x-forwarded-port=1234"))) + Expect(body).Should(ContainSubstring(fmt.Sprintf("x-forwarded-for=1.2.3.4"))) + }) + It("should not trust X-Forwarded headers when setting is false", func() { + host := "forwarded-headers" + + err := f.UpdateNginxConfigMapData(setting, "false") + Expect(err).NotTo(HaveOccurred()) + + ing, err := f.EnsureIngress(framework.NewSingleIngress(host, "/", host, f.IngressController.Namespace, "http-svc", 80, nil)) + Expect(err).NotTo(HaveOccurred()) + Expect(ing).NotTo(BeNil()) + + err = f.WaitForNginxServer(host, + func(server string) bool { + return strings.Contains(server, "server_name forwarded-headers") + }) + Expect(err).NotTo(HaveOccurred()) + + ip, err := f.GetNginxIP() + Expect(err).NotTo(HaveOccurred()) + port, err := f.GetNginxPort("http") + Expect(err).NotTo(HaveOccurred()) + + conn, err := net.Dial("tcp", fmt.Sprintf("%v:%v", ip, port)) + 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")) + + data, err := ioutil.ReadAll(conn) + Expect(err).NotTo(HaveOccurred()) + body := string(data) + Expect(body).Should(ContainSubstring(fmt.Sprintf("host=forwarded-headers"))) + Expect(body).Should(ContainSubstring(fmt.Sprintf("x-forwarded-port=80"))) + Expect(body).Should(ContainSubstring(fmt.Sprintf("x-forwarded-proto=http"))) + Expect(body).Should(ContainSubstring(fmt.Sprintf("x-original-forwarded-for=1.2.3.4"))) + Expect(body).ShouldNot(ContainSubstring(fmt.Sprintf("host=myhost"))) + Expect(body).ShouldNot(ContainSubstring(fmt.Sprintf("x-forwarded-host=myhost"))) + Expect(body).ShouldNot(ContainSubstring(fmt.Sprintf("x-forwarded-proto=myproto"))) + Expect(body).ShouldNot(ContainSubstring(fmt.Sprintf("x-forwarded-port=1234"))) + Expect(body).ShouldNot(ContainSubstring(fmt.Sprintf("x-forwarded-for=1.2.3.4"))) + }) +}) From 94266ff167236a6865795c2c8ed7255c5498ba29 Mon Sep 17 00:00:00 2001 From: Dario Nieuwenhuis Date: Tue, 12 Jun 2018 21:48:32 +0200 Subject: [PATCH 4/6] Document use-forwarded-headers configmap option. --- docs/user-guide/nginx-configuration/configmap.md | 11 +++++++++-- 1 file changed, 9 insertions(+), 2 deletions(-) diff --git a/docs/user-guide/nginx-configuration/configmap.md b/docs/user-guide/nginx-configuration/configmap.md index e532305e99..3b02c2e4d9 100644 --- a/docs/user-guide/nginx-configuration/configmap.md +++ b/docs/user-guide/nginx-configuration/configmap.md @@ -103,6 +103,7 @@ The following table shows a configuration option's name, type, and the default v |[proxy-stream-timeout](#proxy-stream-timeout)|string|"600s"| |[proxy-stream-responses](#proxy-stream-responses)|int|1| |[bind-address](#bind-address)|[]string|""| +|[use-forwarded-headers](#use-forwarded-headers)|bool|"true"| |[forwarded-for-header](#forwarded-for-header)|string|"X-Forwarded-For"| |[compute-full-forwarded-for](#compute-full-forwarded-for)|bool|"false"| |[proxy-add-original-uri-header](#proxy-add-original-uri-header)|bool|"true"| @@ -545,7 +546,7 @@ The value can either be: - round_robin: to use the default round robin loadbalancer - least_conn: to use the least connected method - ip_hash: to use a hash of the server for routing. -- ewma: to use the peak ewma method for routing (only available with `enable-dynamic-configuration` flag) +- ewma: to use the peak ewma method for routing (only available with `enable-dynamic-configuration` flag) The default is least_conn. @@ -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. + +If false, nginx ignores incoming `X-Forwarded-*` headers, filling them with the request information it sees. Use this option if nginx is exposed directly to the internet, or it's behind a L3/packet-based load balancer that doesn't alter the source IP in the packets. + ## forwarded-for-header Sets the header field for identifying the originating IP address of a client. _**default:**_ X-Forwarded-For @@ -608,7 +615,7 @@ Append the remote address to the X-Forwarded-For header instead of replacing it. Adds an X-Original-Uri header with the original request URI to the backend request -## generate-request-id +## generate-request-id Ensures that X-Request-ID is defaulted to a random value, if no X-Request-ID is present in the request From 04d24e1ff722dc96c8b13d778807d44d1e0e3713 Mon Sep 17 00:00:00 2001 From: Dario Nieuwenhuis Date: Wed, 13 Jun 2018 21:10:33 +0200 Subject: [PATCH 5/6] Switch forwarded-headers e2e to use gorequest. --- test/e2e/settings/forwarded_headers.go | 50 ++++++++++++-------------- 1 file changed, 22 insertions(+), 28 deletions(-) diff --git a/test/e2e/settings/forwarded_headers.go b/test/e2e/settings/forwarded_headers.go index c8eee1b5e2..35f9a5fff3 100644 --- a/test/e2e/settings/forwarded_headers.go +++ b/test/e2e/settings/forwarded_headers.go @@ -18,12 +18,12 @@ package settings import ( "fmt" - "io/ioutil" - "net" + "net/http" "strings" . "github.com/onsi/ginkgo" . "github.com/onsi/gomega" + "github.com/parnurzeal/gorequest" "k8s.io/ingress-nginx/test/e2e/framework" ) @@ -60,20 +60,17 @@ var _ = framework.IngressNginxDescribe("X-Forwarded headers", func() { }) Expect(err).NotTo(HaveOccurred()) - ip, err := f.GetNginxIP() - Expect(err).NotTo(HaveOccurred()) - port, err := f.GetNginxPort("http") - Expect(err).NotTo(HaveOccurred()) - - conn, err := net.Dial("tcp", fmt.Sprintf("%v:%v", ip, port)) - 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")) + resp, body, errs := gorequest.New(). + Get(f.IngressController.HTTPURL). + Set("Host", host). + Set("X-Forwarded-Port", "1234"). + Set("X-Forwarded-Proto", "myproto"). + Set("X-Forwarded-For", "1.2.3.4"). + Set("X-Forwarded-Host", "myhost"). + End() - data, err := ioutil.ReadAll(conn) - Expect(err).NotTo(HaveOccurred()) - body := string(data) + Expect(len(errs)).Should(BeNumerically("==", 0)) + Expect(resp.StatusCode).Should(Equal(http.StatusOK)) Expect(body).Should(ContainSubstring(fmt.Sprintf("host=myhost"))) Expect(body).Should(ContainSubstring(fmt.Sprintf("x-forwarded-host=myhost"))) Expect(body).Should(ContainSubstring(fmt.Sprintf("x-forwarded-proto=myproto"))) @@ -96,20 +93,17 @@ var _ = framework.IngressNginxDescribe("X-Forwarded headers", func() { }) Expect(err).NotTo(HaveOccurred()) - ip, err := f.GetNginxIP() - Expect(err).NotTo(HaveOccurred()) - port, err := f.GetNginxPort("http") - Expect(err).NotTo(HaveOccurred()) - - conn, err := net.Dial("tcp", fmt.Sprintf("%v:%v", ip, port)) - 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")) + resp, body, errs := gorequest.New(). + Get(f.IngressController.HTTPURL). + Set("Host", host). + Set("X-Forwarded-Port", "1234"). + Set("X-Forwarded-Proto", "myproto"). + Set("X-Forwarded-For", "1.2.3.4"). + Set("X-Forwarded-Host", "myhost"). + End() - data, err := ioutil.ReadAll(conn) - Expect(err).NotTo(HaveOccurred()) - body := string(data) + Expect(len(errs)).Should(BeNumerically("==", 0)) + Expect(resp.StatusCode).Should(Equal(http.StatusOK)) Expect(body).Should(ContainSubstring(fmt.Sprintf("host=forwarded-headers"))) Expect(body).Should(ContainSubstring(fmt.Sprintf("x-forwarded-port=80"))) Expect(body).Should(ContainSubstring(fmt.Sprintf("x-forwarded-proto=http"))) From 4dfc83a698bb9f86cd65cb2d177960c60486347b Mon Sep 17 00:00:00 2001 From: Dario Nieuwenhuis Date: Wed, 13 Jun 2018 21:10:56 +0200 Subject: [PATCH 6/6] Capitalize NGINX in docs. --- docs/user-guide/nginx-configuration/configmap.md | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/docs/user-guide/nginx-configuration/configmap.md b/docs/user-guide/nginx-configuration/configmap.md index 3b02c2e4d9..631e204326 100644 --- a/docs/user-guide/nginx-configuration/configmap.md +++ b/docs/user-guide/nginx-configuration/configmap.md @@ -599,9 +599,9 @@ Sets the addresses on which the server will accept requests instead of *. It sho ## 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. +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. -If false, nginx ignores incoming `X-Forwarded-*` headers, filling them with the request information it sees. Use this option if nginx is exposed directly to the internet, or it's behind a L3/packet-based load balancer that doesn't alter the source IP in the packets. +If false, NGINX ignores incoming `X-Forwarded-*` headers, filling them with the request information it sees. Use this option if NGINX is exposed directly to the internet, or it's behind a L3/packet-based load balancer that doesn't alter the source IP in the packets. ## forwarded-for-header