From f38f49e77057dd0758a83367d76fd7835beb3081 Mon Sep 17 00:00:00 2001 From: Manuel de Brito Fontes Date: Sun, 17 Sep 2017 15:03:05 -0300 Subject: [PATCH 1/9] Refactor X-Forwarded-* headers --- controllers/nginx/pkg/cmd/controller/nginx.go | 4 +- controllers/nginx/pkg/config/config.go | 9 ++++ controllers/nginx/pkg/template/configmap.go | 10 ++++ controllers/nginx/pkg/template/template.go | 24 +++++++++ .../rootfs/etc/nginx/template/nginx.tmpl | 53 ++++++++++++++----- 5 files changed, 85 insertions(+), 15 deletions(-) diff --git a/controllers/nginx/pkg/cmd/controller/nginx.go b/controllers/nginx/pkg/cmd/controller/nginx.go index bfcdfe4ff..30244aac8 100644 --- a/controllers/nginx/pkg/cmd/controller/nginx.go +++ b/controllers/nginx/pkg/cmd/controller/nginx.go @@ -35,7 +35,6 @@ import ( "github.com/spf13/pflag" proxyproto "github.com/armon/go-proxyproto" - api "k8s.io/api/core/v1" api_v1 "k8s.io/api/core/v1" extensions "k8s.io/api/extensions/v1beta1" @@ -309,7 +308,7 @@ func (n NGINXController) DefaultEndpoint() ingress.Endpoint { return ingress.Endpoint{ Address: "127.0.0.1", Port: fmt.Sprintf("%v", n.ports.Default), - Target: &api.ObjectReference{}, + Target: &api_v1.ObjectReference{}, } } @@ -657,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) diff --git a/controllers/nginx/pkg/config/config.go b/controllers/nginx/pkg/config/config.go index d4121e35b..69c4d913a 100644 --- a/controllers/nginx/pkg/config/config.go +++ b/controllers/nginx/pkg/config/config.go @@ -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" ) @@ -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 @@ -448,6 +455,7 @@ func NewDefault() Configuration { LimitConnZoneVariable: defaultLimitConnZoneVariable, BindAddressIpv4: defBindAddress, BindAddressIpv6: defBindAddress, + RealClientFrom: "auto", } if glog.V(5) { @@ -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 diff --git a/controllers/nginx/pkg/template/configmap.go b/controllers/nginx/pkg/template/configmap.go index 7113aef4e..1a275c982 100644 --- a/controllers/nginx/pkg/template/configmap.go +++ b/controllers/nginx/pkg/template/configmap.go @@ -19,6 +19,7 @@ package template import ( "fmt" "net" + "regexp" "strconv" "strings" @@ -37,6 +38,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{} @@ -119,6 +124,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 } diff --git a/controllers/nginx/pkg/template/template.go b/controllers/nginx/pkg/template/template.go index d42c153a8..325f6e3e5 100644 --- a/controllers/nginx/pkg/template/template.go +++ b/controllers/nginx/pkg/template/template.go @@ -32,6 +32,7 @@ import ( "github.com/golang/glog" "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" @@ -158,6 +159,8 @@ var ( "buildAuthSignURL": buildAuthSignURL, "isValidClientBodyBufferSize": isValidClientBodyBufferSize, "buildForwardedFor": buildForwardedFor, + "trustHTTPHeaders": trustHTTPHeaders, + "trustProxyProtocol": trustProxyProtocol, } ) @@ -657,3 +660,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 + } + + 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) +} diff --git a/controllers/nginx/rootfs/etc/nginx/template/nginx.tmpl b/controllers/nginx/rootfs/etc/nginx/template/nginx.tmpl index cfa721acd..089c16f07 100644 --- a/controllers/nginx/rootfs/etc/nginx/template/nginx.tmpl +++ b/controllers/nginx/rootfs/etc/nginx/template/nginx.tmpl @@ -143,6 +143,14 @@ http { '' close; } + {{ if (trustHTTPHeaders $cfg) }} + # 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; @@ -150,20 +158,44 @@ http { } 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; } - map {{ buildForwardedFor $cfg.ForwardedForHeader }} $the_real_ip { - default {{ buildForwardedFor $cfg.ForwardedForHeader }}; - "~*(?[0-9\.]+).*" $ip; - {{ if $cfg.UseProxyProtocol }} - '' $proxy_protocol_addr; {{ else }} - '' $realip_remote_addr; + # Do not trust HTTP X-Forwarded-* Headers + map {{ buildForwardedFor $cfg.ForwardedForHeader }} $the_real_ip { + {{ if (trustProxyProtocol $cfg) }} + # Get IP address from Proxy Protocol + {{ if (ne (len $cfg.ProxyRealIPCIDR) 0) }} + # using trusted real IP CIDR + default $realip_remote_addr; + {{ else }} + 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; + } + + {{ end }} + {{ if $all.IsSSLPassthroughEnabled }} # map port {{ $all.ListenPorts.SSLProxy }} to 443 for header X-Forwarded-Port map $pass_server_port $pass_port { @@ -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; From df57b8bab6af11cb9b685902dbfbd229dc2898d1 Mon Sep 17 00:00:00 2001 From: Manuel de Brito Fontes Date: Sun, 17 Sep 2017 17:01:28 -0300 Subject: [PATCH 2/9] 1 --- controllers/nginx/pkg/config/config.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/controllers/nginx/pkg/config/config.go b/controllers/nginx/pkg/config/config.go index 69c4d913a..7e5e952a1 100644 --- a/controllers/nginx/pkg/config/config.go +++ b/controllers/nginx/pkg/config/config.go @@ -23,7 +23,7 @@ import ( "github.com/golang/glog" - api "k8s.io/api/core/v1" + apiv1 "k8s.io/api/core/v1" "k8s.io/ingress/core/pkg/ingress" "k8s.io/ingress/core/pkg/ingress/defaults" @@ -494,7 +494,7 @@ type TemplateConfig struct { IsSSLPassthroughEnabled bool RedirectServers map[string]string ListenPorts *ListenPorts - PublishService *api.Service + PublishService *apiv1.Service } // ListenPorts describe the ports required to run the From 6ee2b726f870e58e6b8bd0874e07c6a0c796ed1a Mon Sep 17 00:00:00 2001 From: Manuel de Brito Fontes Date: Tue, 26 Sep 2017 14:20:36 -0300 Subject: [PATCH 3/9] Fix template mappings --- controllers/nginx/pkg/template/template.go | 2 +- controllers/nginx/rootfs/etc/nginx/template/nginx.tmpl | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/controllers/nginx/pkg/template/template.go b/controllers/nginx/pkg/template/template.go index 319a8b244..eca21b54a 100644 --- a/controllers/nginx/pkg/template/template.go +++ b/controllers/nginx/pkg/template/template.go @@ -706,5 +706,5 @@ func trustProxyProtocol(input interface{}) bool { } return conf.Cfg.RealClientFrom == "tcp-proxy" || - (conf.Cfg.RealClientFrom == "auto" && !conf.Cfg.UseProxyProtocol) + (conf.Cfg.RealClientFrom == "auto" && conf.Cfg.UseProxyProtocol) } diff --git a/controllers/nginx/rootfs/etc/nginx/template/nginx.tmpl b/controllers/nginx/rootfs/etc/nginx/template/nginx.tmpl index d42b6c3af..f15accd6c 100644 --- a/controllers/nginx/rootfs/etc/nginx/template/nginx.tmpl +++ b/controllers/nginx/rootfs/etc/nginx/template/nginx.tmpl @@ -151,7 +151,7 @@ http { '' close; } - {{ if (trustHTTPHeaders $cfg) }} + {{ 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 @@ -391,7 +391,7 @@ http { {{ template "CUSTOM_ERRORS" $all }} } - + {{ if $server.Alias }} server { server_name {{ $server.Alias }}; From 78e166f22c782c4960e3ce2fee35b843bf3bb09a Mon Sep 17 00:00:00 2001 From: Manuel de Brito Fontes Date: Tue, 26 Sep 2017 15:04:21 -0300 Subject: [PATCH 4/9] Fix cast error --- core/pkg/ingress/controller/controller.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/core/pkg/ingress/controller/controller.go b/core/pkg/ingress/controller/controller.go index 4a1efbe7f..f11192785 100644 --- a/core/pkg/ingress/controller/controller.go +++ b/core/pkg/ingress/controller/controller.go @@ -255,8 +255,8 @@ func (ic *GenericController) syncIngress(key interface{}) error { // Sort ingress rules using the ResourceVersion field ings := ic.listers.Ingress.List() sort.SliceStable(ings, func(i, j int) bool { - ir := ings[i].(*ingress.SSLCert).ResourceVersion - jr := ings[j].(*ingress.SSLCert).ResourceVersion + ir := ings[i].(*extensions.Ingress).ResourceVersion + jr := ings[j].(*extensions.Ingress).ResourceVersion return ir < jr }) From db12b517c96ca4e705be349fa1134aa7e128a4b3 Mon Sep 17 00:00:00 2001 From: Manuel de Brito Fontes Date: Fri, 29 Sep 2017 09:17:55 -0300 Subject: [PATCH 5/9] Fix identation --- controllers/nginx/rootfs/etc/nginx/template/nginx.tmpl | 2 +- examples/aws/nginx/nginx-ingress-controller.yaml | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/controllers/nginx/rootfs/etc/nginx/template/nginx.tmpl b/controllers/nginx/rootfs/etc/nginx/template/nginx.tmpl index 5f15abf01..fb3a55619 100644 --- a/controllers/nginx/rootfs/etc/nginx/template/nginx.tmpl +++ b/controllers/nginx/rootfs/etc/nginx/template/nginx.tmpl @@ -199,7 +199,7 @@ http { default $scheme; } map $http_x_forwarded_port $pass_server_port { - default $server_port; + default $server_port; } {{ end }} diff --git a/examples/aws/nginx/nginx-ingress-controller.yaml b/examples/aws/nginx/nginx-ingress-controller.yaml index b05eddd02..b30fee5c5 100644 --- a/examples/aws/nginx/nginx-ingress-controller.yaml +++ b/examples/aws/nginx/nginx-ingress-controller.yaml @@ -101,7 +101,7 @@ spec: spec: terminationGracePeriodSeconds: 60 containers: - - image: gcr.io/google_containers/nginx-ingress-controller:0.9.0-beta.13 + - image: quay.io/aledbf/nginx-ingress-controller:0.241 name: ingress-nginx imagePullPolicy: Always ports: From fe2386b0a62a4a02d74ecb976b980b0de2d817d8 Mon Sep 17 00:00:00 2001 From: Manuel de Brito Fontes Date: Fri, 29 Sep 2017 09:57:16 -0300 Subject: [PATCH 6/9] Cleanup --- controllers/nginx/rootfs/etc/nginx/template/nginx.tmpl | 7 ++----- 1 file changed, 2 insertions(+), 5 deletions(-) diff --git a/controllers/nginx/rootfs/etc/nginx/template/nginx.tmpl b/controllers/nginx/rootfs/etc/nginx/template/nginx.tmpl index fb3a55619..59c247352 100644 --- a/controllers/nginx/rootfs/etc/nginx/template/nginx.tmpl +++ b/controllers/nginx/rootfs/etc/nginx/template/nginx.tmpl @@ -180,12 +180,7 @@ http { map {{ buildForwardedFor $cfg.ForwardedForHeader }} $the_real_ip { {{ if (trustProxyProtocol $cfg) }} # Get IP address from Proxy Protocol - {{ if (ne (len $cfg.ProxyRealIPCIDR) 0) }} - # using trusted real IP CIDR - default $realip_remote_addr; - {{ else }} default $proxy_protocol_addr; - {{ end }} {{ else }} # Get IP from direct remote address default $realip_remote_addr; @@ -297,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" }} From b1b75f9c694089f42321e0c0a6e506f90510e0c3 Mon Sep 17 00:00:00 2001 From: Manuel de Brito Fontes Date: Fri, 29 Sep 2017 09:59:53 -0300 Subject: [PATCH 7/9] Rollback change in docker image --- examples/aws/nginx/nginx-ingress-controller.yaml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/examples/aws/nginx/nginx-ingress-controller.yaml b/examples/aws/nginx/nginx-ingress-controller.yaml index b30fee5c5..b05eddd02 100644 --- a/examples/aws/nginx/nginx-ingress-controller.yaml +++ b/examples/aws/nginx/nginx-ingress-controller.yaml @@ -101,7 +101,7 @@ spec: spec: terminationGracePeriodSeconds: 60 containers: - - image: quay.io/aledbf/nginx-ingress-controller:0.241 + - image: gcr.io/google_containers/nginx-ingress-controller:0.9.0-beta.13 name: ingress-nginx imagePullPolicy: Always ports: From f549e03cbdb53bd0de5e7be8c498864447b189ce Mon Sep 17 00:00:00 2001 From: Manuel de Brito Fontes Date: Fri, 29 Sep 2017 12:11:35 -0300 Subject: [PATCH 8/9] Fix remote address --- controllers/nginx/rootfs/etc/nginx/template/nginx.tmpl | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/controllers/nginx/rootfs/etc/nginx/template/nginx.tmpl b/controllers/nginx/rootfs/etc/nginx/template/nginx.tmpl index 59c247352..c827d994f 100644 --- a/controllers/nginx/rootfs/etc/nginx/template/nginx.tmpl +++ b/controllers/nginx/rootfs/etc/nginx/template/nginx.tmpl @@ -155,7 +155,7 @@ http { # 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 }}; + default $remote_addr; '' $realip_remote_addr; } From f253d249f2c5f7c95f1280488f542721c7123f8c Mon Sep 17 00:00:00 2001 From: Manuel de Brito Fontes Date: Fri, 29 Sep 2017 18:03:27 -0300 Subject: [PATCH 9/9] Cleanup --- controllers/nginx/pkg/template/template.go | 7 +++++-- controllers/nginx/rootfs/etc/nginx/template/nginx.tmpl | 4 ++-- 2 files changed, 7 insertions(+), 4 deletions(-) diff --git a/controllers/nginx/pkg/template/template.go b/controllers/nginx/pkg/template/template.go index 111a7626c..0cbb52f61 100644 --- a/controllers/nginx/pkg/template/template.go +++ b/controllers/nginx/pkg/template/template.go @@ -673,17 +673,20 @@ func buildForwardedFor(input interface{}) string { 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.PublishService != nil && conf.PublishService.Spec.Type == apiv1.ServiceTypeLoadBalancer)) + (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 } diff --git a/controllers/nginx/rootfs/etc/nginx/template/nginx.tmpl b/controllers/nginx/rootfs/etc/nginx/template/nginx.tmpl index c827d994f..88413515d 100644 --- a/controllers/nginx/rootfs/etc/nginx/template/nginx.tmpl +++ b/controllers/nginx/rootfs/etc/nginx/template/nginx.tmpl @@ -155,8 +155,8 @@ http { # 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 $remote_addr; - '' $realip_remote_addr; + default $realip_remote_addr; + '' $remote_addr; } # trust http_x_forwarded_proto headers correctly indicate ssl offloading