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 @@ -656,6 +656,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"

api "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 @@ -448,6 +455,7 @@ func NewDefault() Configuration {
LimitConnZoneVariable: defaultLimitConnZoneVariable,
BindAddressIpv4: defBindAddress,
BindAddressIpv6: defBindAddress,
RealClientFrom: "auto",
}

if glog.V(5) {
Expand Down Expand Up @@ -486,6 +494,7 @@ type TemplateConfig struct {
IsSSLPassthroughEnabled bool
RedirectServers map[string]string
ListenPorts *ListenPorts
PublishService *api.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
24 changes: 24 additions & 0 deletions controllers/nginx/pkg/template/template.go
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,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 @@ -159,6 +160,8 @@ var (
"buildAuthSignURL": buildAuthSignURL,
"isValidClientBodyBufferSize": isValidClientBodyBufferSize,
"buildForwardedFor": buildForwardedFor,
"trustHTTPHeaders": trustHTTPHeaders,
"trustProxyProtocol": trustProxyProtocol,
}
)

Expand Down Expand Up @@ -658,3 +661,24 @@ 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 {
return true
Copy link

@jammm jammm Sep 19, 2017

Choose a reason for hiding this comment

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

(Continuing from my comment on nginx.tmpl:146)
Maybe !ok always evaluates to true here.
Could you check if input.(config.TemplateConfig) is returning conf correctly? Or maybe I'm missing something..

Copy link
Member Author

Choose a reason for hiding this comment

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

fixed

}

return conf.Cfg.RealClientFrom == "http-proxy" ||
(conf.Cfg.RealClientFrom == "auto" && !conf.Cfg.UseProxyProtocol &&
(conf.PublishService != nil && conf.PublishService.Spec.Type == apiv1.ServiceTypeLoadBalancer))
}

func trustProxyProtocol(input interface{}) bool {
conf, ok := input.(config.TemplateConfig)
if !ok {
return true
}

return conf.Cfg.RealClientFrom == "tcp-proxy" ||
(conf.Cfg.RealClientFrom == "auto" && !conf.Cfg.UseProxyProtocol)
Copy link

Choose a reason for hiding this comment

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

I could be wrong, but shouldn't this be (conf.Cfg.RealClientFrom == "auto" && conf.Cfg.UseProxyProtocol) ?

}
51 changes: 39 additions & 12 deletions controllers/nginx/rootfs/etc/nginx/template/nginx.tmpl
Original file line number Diff line number Diff line change
Expand Up @@ -143,27 +143,59 @@ http {
'' close;
}

{{ if (trustHTTPHeaders $cfg) }}
Copy link

@jammm jammm Sep 19, 2017

Choose a reason for hiding this comment

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

For some reason, trustHTTPHeaders is always returning true for me. This block here is always being created regardless of whether I set real-client-from: "http-proxy" or real-client-from: "tcp-proxy".

I checked to make sure whether it's loading real-client-from properly by setting an invalid value, and it does seem to show up on the logs and sets to "auto" so I'm almost certain this value is being read up to trustHTTPHeaders.

# 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 {{ buildForwardedFor $cfg.ForwardedForHeader }};
'' $realip_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"

{{ if (ne (len $cfg.ProxyRealIPCIDR) 0) }}
# using trusted real IP CIDR
default $realip_remote_addr;
{{ else }}
'' $realip_remote_addr;
default $proxy_protocol_addr;
{{ end }}
{{ 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;
Copy link

Choose a reason for hiding this comment

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

Indentation could be used here.

Copy link
Member Author

Choose a reason for hiding this comment

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

done

}

{{ 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 @@ -198,11 +230,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