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

Refactor X-Forwarded-* headers #1381

Merged
merged 12 commits into from
Sep 30, 2017
1 change: 1 addition & 0 deletions controllers/nginx/pkg/cmd/controller/nginx.go
Original file line number Diff line number Diff line change
Expand Up @@ -676,6 +676,7 @@ func (n *NGINXController) OnUpdate(ingressCfg ingress.Configuration) error {
RedirectServers: redirectServers,
IsSSLPassthroughEnabled: n.isSSLPassthroughEnabled,
ListenPorts: n.ports,
PublishService: n.controller.GetPublishService(),
}

content, err := n.t.Write(tc)
Expand Down
9 changes: 9 additions & 0 deletions controllers/nginx/pkg/config/config.go
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,8 @@ import (

"github.com/golang/glog"

apiv1 "k8s.io/api/core/v1"

"k8s.io/ingress/core/pkg/ingress"
"k8s.io/ingress/core/pkg/ingress/defaults"
)
Expand Down Expand Up @@ -259,6 +261,11 @@ 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
Expand Down Expand Up @@ -463,6 +470,7 @@ func NewDefault() Configuration {
LimitConnZoneVariable: defaultLimitConnZoneVariable,
BindAddressIpv4: defBindAddress,
BindAddressIpv6: defBindAddress,
RealClientFrom: "auto",
ZipkinCollectorPort: 9411,
ZipkinServiceName: "nginx",
}
Expand Down Expand Up @@ -503,6 +511,7 @@ type TemplateConfig struct {
IsSSLPassthroughEnabled bool
RedirectServers map[string]string
ListenPorts *ListenPorts
PublishService *apiv1.Service
}

// ListenPorts describe the ports required to run the
Expand Down
10 changes: 10 additions & 0 deletions controllers/nginx/pkg/template/configmap.go
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@ package template
import (
"fmt"
"net"
"regexp"
"strconv"
"strings"

Expand All @@ -38,6 +39,10 @@ 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{}
Expand Down Expand Up @@ -120,6 +125,11 @@ 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
}

Expand Down
27 changes: 27 additions & 0 deletions controllers/nginx/pkg/template/template.go
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,7 @@ import (

"github.com/pborman/uuid"

apiv1 "k8s.io/api/core/v1"
extensions "k8s.io/api/extensions/v1beta1"
"k8s.io/apimachinery/pkg/util/sets"
"k8s.io/ingress/controllers/nginx/pkg/config"
Expand Down Expand Up @@ -151,6 +152,8 @@ var (
},
"isValidClientBodyBufferSize": isValidClientBodyBufferSize,
"buildForwardedFor": buildForwardedFor,
"trustHTTPHeaders": trustHTTPHeaders,
"trustProxyProtocol": trustProxyProtocol,
}
)

Expand Down Expand Up @@ -666,3 +669,27 @@ func buildForwardedFor(input interface{}) string {
ffh = strings.ToLower(ffh)
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 ||
(conf.Cfg.RealClientFrom == "auto" && conf.PublishService != nil &&
conf.PublishService.Spec.Type == apiv1.ServiceTypeLoadBalancer))
}

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)
}
50 changes: 37 additions & 13 deletions controllers/nginx/rootfs/etc/nginx/template/nginx.tmpl
Original file line number Diff line number Diff line change
Expand Up @@ -151,27 +151,54 @@ 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
default $realip_remote_addr;
'' $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;
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 {{ buildForwardedFor $cfg.ForwardedForHeader }} $the_real_ip {
default {{ buildForwardedFor $cfg.ForwardedForHeader }};
"~*(?<ip>[0-9\.]+).*" $ip;
{{ if $cfg.UseProxyProtocol }}
'' $proxy_protocol_addr;
{{ if (trustProxyProtocol $cfg) }}
Copy link
Contributor

@rikatz rikatz Sep 29, 2017

Choose a reason for hiding this comment

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

Shouldn't this be $all instead of $cfg, as defined also in trustHTTPHeaders?

I'm facing an issue here, that even when UseProxyProtocol is false, it's returning a 'true' value here, using $proxy_protocol_addr instead of $realip_remote_addr.

Edit: After changing this to $all in my template, it works fine. I'm now going to test this with other options to see what's the behaviour.

# Get IP address from Proxy Protocol
Copy link

Choose a reason for hiding this comment

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

This wasn't created for me after setting real-client-from: "tcp-proxy"

default $proxy_protocol_addr;
{{ else }}
'' $realip_remote_addr;
# 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 {
Expand Down Expand Up @@ -206,11 +233,6 @@ http {
'' $host;
}

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

server_name_in_redirect off;
port_in_redirect off;

Expand Down Expand Up @@ -270,10 +292,12 @@ http {

{{ range $server := $upstream.Endpoints }}server {{ $server.Address | formatIP }}:{{ $server.Port }} max_fails={{ $server.MaxFails }} fail_timeout={{ $server.FailTimeout }};
{{ end }}

}

{{ end }}


upstream {{ $upstream.Name }} {
# Load balance algorithm; empty for round robin, which is the default
{{ if ne $cfg.LoadBalanceAlgorithm "round_robin" }}
Expand Down Expand Up @@ -364,7 +388,7 @@ http {

{{ template "CUSTOM_ERRORS" $all }}
}

{{ if $server.Alias }}
server {
server_name {{ $server.Alias }};
Expand Down