Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add use-forwarded-headers configmap option. #2616

Merged
merged 7 commits into from
Aug 16, 2018
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 8 additions & 1 deletion docs/user-guide/nginx-configuration/configmap.md
Original file line number Diff line number Diff line change
Expand Up @@ -102,6 +102,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"|
Expand Down Expand Up @@ -589,6 +590,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
Expand All @@ -601,7 +608,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

Expand Down
4 changes: 4 additions & 0 deletions internal/ingress/controller/config/config.go
Original file line number Diff line number Diff line change
Expand Up @@ -411,6 +411,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"`
Expand Down Expand Up @@ -552,6 +555,7 @@ func NewDefault() Configuration {
EnableDynamicTLSRecords: true,
EnableUnderscoresInHeaders: false,
ErrorLogLevel: errorLevel,
UseForwardedHeaders: true,
ForwardedForHeader: "X-Forwarded-For",
ComputeFullForwardedFor: false,
ProxyAddOriginalUriHeader: true,
Expand Down
62 changes: 42 additions & 20 deletions rootfs/etc/nginx/template/nginx.tmpl
Original file line number Diff line number Diff line change
Expand Up @@ -93,7 +93,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 }}
Expand All @@ -104,6 +107,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 */}}
Expand Down Expand Up @@ -241,7 +245,9 @@ http {
'' close;
}

map {{ buildForwardedFor $cfg.ForwardedForHeader }} $the_real_ip {
# The following is a sneaky way to do "set $the_real_ip $remote_addr"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd rather use a Go template comment to avoid writing this message to the config file.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IMO this could be useful to someone cating the rendered config from a running container, to help understand what's going on.

The rendered template already contains other similar comments, and this one is rendered just once so it's not like it'd cause much bloat.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You're right, no strong opinion on that one.

# 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;
Expand All @@ -250,24 +256,51 @@ 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;
"http:http" 1;
"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 {
Expand All @@ -281,17 +314,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 {
Expand All @@ -301,7 +323,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 {
Expand Down Expand Up @@ -1047,7 +1069,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 }}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we need another else if here, something like

else if $all.Cfg.ComputeFullForwardedFor
{ $proxySetHeader }} X-Forwarded-For $proxy_add_x_forwarded_for;

since this is still not using forwarded headers, rather just passing it to upstream

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

$full_x_forwarded_for is the same as $proxy_add_x_forwarded_for but done "by hand" in a map above.

Maybe this is so it appends the proxy protocol addr instead of $remote_addr if using proxy protocol, and nginx's $proxy_add_x_forwarded_for doesn't?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah that makes sense @Dirbaio

Why do you require $all.Cfg.UseForwardedHeaders to be true here though?

Copy link
Contributor Author

@Dirbaio Dirbaio Aug 18, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ComputeFullForwardedFor has no effect when UseForwardedHeaders = false.

There are 3 cases to consider:

  • UseForwardedHeaders = false: incoming X-Forwarded-For value is discarded and replaced with the source IP nginx-ingress sees.
  • UseForwardedHeaders = true, ComputeFullForwardedFor = false: incoming X-Forwarded-For value is passed through if present, otherwise it's filled with the source IP nginx-ingress sees.
  • UseForwardedHeaders = true, ComputeFullForwardedFor = true: incoming X-Forwarded-For value is passed through, appended with the source IP nginx-ingress sees.

For case 3 we use $full_x_forwarded_for, which has been computed earlier in the template.
For cases 1, 2 we use $the_real_ip, which earlier in the template has been set to be the appropriate source IP depending on UseForwardedHeaders

{{ $proxySetHeader }} X-Forwarded-For $full_x_forwarded_for;
{{ else }}
{{ $proxySetHeader }} X-Forwarded-For $the_real_ip;
Expand Down
117 changes: 117 additions & 0 deletions test/e2e/settings/forwarded_headers.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,117 @@
/*
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"
"net/http"
"strings"

. "github.com/onsi/ginkgo"
. "github.com/onsi/gomega"
"github.com/parnurzeal/gorequest"

"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())

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()

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")))
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())

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()

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")))
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")))
})
})