Skip to content
Open
Show file tree
Hide file tree
Changes from 5 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
4 changes: 4 additions & 0 deletions docs/user-guide/nginx-configuration/configmap.md
Original file line number Diff line number Diff line change
Expand Up @@ -929,6 +929,10 @@ If false, NGINX ignores incoming `X-Forwarded-*` headers, filling them with the

`enable-real-ip` enables the configuration of [https://nginx.org/en/docs/http/ngx_http_realip_module.html](https://nginx.org/en/docs/http/ngx_http_realip_module.html). Specific attributes of the module can be configured further by using `forwarded-for-header` and `proxy-real-ip-cidr` settings.

## enable-real-ip-recursive

`enable-real-ip-recursive` enables the configuration of [https://nginx.org/en/docs/http/ngx_http_realip_module.html#real_ip_recursive](https://nginx.org/en/docs/http/ngx_http_realip_module.html#real_ip_recursive).

## forwarded-for-header

Sets the header field for identifying the originating IP address of a client. _**default:**_ X-Forwarded-For
Expand Down
6 changes: 6 additions & 0 deletions internal/ingress/controller/config/config.go
Original file line number Diff line number Diff line change
Expand Up @@ -556,6 +556,11 @@ type Configuration struct {
// Sets whether to enable the real ip module
EnableRealIP bool `json:"enable-real-ip"`

// Sets whether to use recursive search in the real ip module
// https://nginx.org/en/docs/http/ngx_http_realip_module.html#real_ip_recursive
// Default: true
EnableRealIPRecursive bool `json:"enable-real-ip-recursive"`

// 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 @@ -790,6 +795,7 @@ func NewDefault() Configuration {
ErrorLogLevel: errorLevel,
UseForwardedHeaders: false,
EnableRealIP: false,
EnableRealIPRecursive: true,
Copy link
Member

Choose a reason for hiding this comment

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

shouldn't this be disabled by default?

Copy link
Member

Choose a reason for hiding this comment

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

there is a "problem" with this approach of a flag defaulted to true: if you set it as false on your configmap, it may or may not represent as empty (depending on the marshaller, eg using omitempty on json flag) so because it is empty it will be defaulted to...true :)

To confirm this behavior, can you add some e2e test with both EnableRealIPRecursive = true or false on the configmap, and check if the template is rendered correctly?

Copy link
Author

Choose a reason for hiding this comment

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

Hi @rikatz ,

I've set the default to false. My initial reasoning for it to be true was that I think the more secure option should be enabled by default if it doesn't otherwise impact any functionality. Because the setting itself is dependent on EnableRealIp being set to true, the more secure configuration would be the default when EnableRealIp would be used. This is a taste thing, and not a deal breaker for getting this merged.

Copy link
Member

Choose a reason for hiding this comment

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

The nginx default is off, so let's set this to false.

ForwardedForHeader: "X-Forwarded-For",
ComputeFullForwardedFor: false,
ProxyAddOriginalURIHeader: false,
Expand Down
5 changes: 5 additions & 0 deletions rootfs/etc/nginx/template/nginx.tmpl
Original file line number Diff line number Diff line change
Expand Up @@ -143,7 +143,12 @@ http {
real_ip_header {{ $cfg.ForwardedForHeader }};
{{ end }}

{{ if $cfg.EnableRealIPRecursive }}
real_ip_recursive on;
{{ else }}
real_ip_recursive off;
{{ end }}

{{ range $trusted_ip := $cfg.ProxyRealIPCIDR }}
set_real_ip_from {{ $trusted_ip }};
{{ end }}
Expand Down
102 changes: 102 additions & 0 deletions test/e2e/settings/enable_real_ip_recursive.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,102 @@
/*
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 (
"net/http"
"strings"

"github.com/onsi/ginkgo"
"github.com/stretchr/testify/assert"

"k8s.io/ingress-nginx/test/e2e/framework"
)

const forwardedForHost = "forwarded-for-header"

var _ = framework.DescribeSetting("enable-real-ip-recursive", func() {
f := framework.NewDefaultFramework("enable-real-ip-recursive")

setting := "enable-real-ip-recursive"

ginkgo.BeforeEach(func() {
f.NewEchoDeployment()

f.SetNginxConfigMapData(map[string]string{
"log-format-escape-json": "true",
"log-format-upstream": "clientip=\"$remote_addr\"",
"use-forwarded-headers": "true",
setting: "false",
})
})

ginkgo.It("should use the first IP in X-Forwarded-For header when setting is true", func() {
host := forwardedForHost

f.UpdateNginxConfigMapData(setting, "true")

ing := framework.NewSingleIngress(host, "/", host, f.Namespace, framework.EchoService, 80, nil)
f.EnsureIngress(ing)

f.WaitForNginxConfiguration(func(conf string) bool {
return strings.Contains(conf, "real_ip_recursive on;")
})

ginkgo.By("ensuring single values are parsed correctly")
body := f.HTTPTestClient().
GET("/").
WithHeader("Host", host).
WithHeader("X-Forwarded-For", "127.0.0.1, 1.2.3.4").
Expect().
Status(http.StatusOK).
Body().
Raw()

assert.Contains(ginkgo.GinkgoT(), body, "x-forwarded-for=127.0.0.1")

logs, err := f.NginxLogs()
assert.Nil(ginkgo.GinkgoT(), err, "obtaining nginx logs")
assert.Contains(ginkgo.GinkgoT(), logs, "clientip=\"127.0.0.1\"")
})

ginkgo.It("should use the last IP in X-Forwarded-For header when setting is false", func() {
host := forwardedForHost

f.UpdateNginxConfigMapData(setting, "false")

f.EnsureIngress(framework.NewSingleIngress(host, "/", host, f.Namespace, framework.EchoService, 80, nil))

f.WaitForNginxConfiguration(func(conf string) bool {
return strings.Contains(conf, "real_ip_recursive off;")
})

body := f.HTTPTestClient().
GET("/").
WithHeader("Host", host).
WithHeader("X-Forwarded-For", "127.0.0.1, 1.2.3.4").
Expect().
Status(http.StatusOK).
Body().
Raw()

assert.Contains(ginkgo.GinkgoT(), body, "x-forwarded-for=1.2.3.4")

logs, err := f.NginxLogs()
assert.Nil(ginkgo.GinkgoT(), err, "obtaining nginx logs")
assert.Contains(ginkgo.GinkgoT(), logs, "clientip=\"1.2.3.4\"")
})
})