From 6fdd444cb81bcf420feccc47c57d83fab8e50170 Mon Sep 17 00:00:00 2001 From: Christian Simon Date: Sun, 5 Jun 2016 15:19:55 +0100 Subject: [PATCH] ingress: adds configurable SSL redirect nginx controller * add global value to config map * add per ingress value as annotation to ingress resources --- ingress/controllers/nginx/Changelog.md | 12 ++--- ingress/controllers/nginx/README.md | 5 ++ ingress/controllers/nginx/controller.go | 16 +++--- ingress/controllers/nginx/nginx.tmpl | 10 ++-- .../controllers/nginx/nginx/rewrite/main.go | 52 ++++++++++++++++--- .../nginx/nginx/rewrite/main_test.go | 25 +++++++++ 6 files changed, 91 insertions(+), 29 deletions(-) diff --git a/ingress/controllers/nginx/Changelog.md b/ingress/controllers/nginx/Changelog.md index 3c6181adcf..f985d24192 100644 --- a/ingress/controllers/nginx/Changelog.md +++ b/ingress/controllers/nginx/Changelog.md @@ -1,5 +1,10 @@ Changelog +### next + +- [X] [#1063](https://github.com/kubernetes/contrib/pull/1063) watches referenced tls secrets +- [X] [#850](https://github.com/kubernetes/contrib/pull/850) adds configurable SSL redirect nginx controller + ### 0.7 - [X] [#898](https://github.com/kubernetes/contrib/pull/898) reorder locations. Location / must be the last one to avoid errors routing to subroutes @@ -16,10 +21,3 @@ Changelog - [X] [#1102](https://github.com/kubernetes/contrib/pull/1102) geolocation of traffic in stats - [X] [#884](https://github.com/kubernetes/contrib/issues/884) support services running ssl - [X] [#930](https://github.com/kubernetes/contrib/issues/930) detect changes in configuration configmaps - - -TODO - -- [ ] [#1063](https://github.com/kubernetes/contrib/pull/1063) watches referenced tls secrets -- [ ] [#850](https://github.com/kubernetes/contrib/pull/850) adds configurable SSL redirect nginx controller - diff --git a/ingress/controllers/nginx/README.md b/ingress/controllers/nginx/README.md index fef82cf2d7..58fd8b1ea0 100644 --- a/ingress/controllers/nginx/README.md +++ b/ingress/controllers/nginx/README.md @@ -131,6 +131,11 @@ To disable this behavior use `hsts=false` in the NGINX ConfigMap. NGINX provides the configuration option [ssl_buffer_size](http://nginx.org/en/docs/http/ngx_http_ssl_module.html#ssl_buffer_size) to allow the optimization of the TLS record size. This improves the [Time To First Byte](https://www.igvita.com/2013/12/16/optimizing-nginx-tls-time-to-first-byte/) (TTTFB). The default value in the Ingress controller is `4k` (nginx default is `16k`); +### Server-side HTTPS enforcement through redirect + +By default the controller redirects (301) to HTTPS if TLS is enabled for that ingress . If you want to disable that behaviour globally, you can use `ssl-redirect: "false"` in the NGINX ConfigMap. + +To configure this feature for specfic ingress resources, you can use the `ingress.kubernetes.io/ssl-redirect: "false"` annotation in theparticular resource. ## Proxy Protocol diff --git a/ingress/controllers/nginx/controller.go b/ingress/controllers/nginx/controller.go index c87a868e1b..e93185bd20 100644 --- a/ingress/controllers/nginx/controller.go +++ b/ingress/controllers/nginx/controller.go @@ -692,6 +692,11 @@ func (lbc *loadBalancerController) getUpstreamServers(ngxCfg config.Configuratio glog.V(3).Infof("error reading secure upstream in Ingress %v/%v: %v", ing.GetNamespace(), ing.GetName(), err) } + locRew, err := rewrite.ParseAnnotations(ngxCfg, ing) + if err != nil { + glog.V(3).Infof("error parsing rewrite annotations for Ingress rule %v/%v: %v", ing.GetNamespace(), ing.GetName(), err) + } + host := rule.Host if host == "" { host = defServerName @@ -721,13 +726,8 @@ func (lbc *loadBalancerController) getUpstreamServers(ngxCfg config.Configuratio loc.Upstream = *ups loc.Auth = *nginxAuth loc.RateLimit = *rl - loc.SecureUpstream = secUpstream - - locRew, err := rewrite.ParseAnnotations(ing) - if err != nil { - glog.V(3).Infof("error parsing rewrite annotations for Ingress rule %v/%v: %v", ing.GetNamespace(), ing.GetName(), err) - } loc.Redirect = *locRew + loc.SecureUpstream = secUpstream addLoc = false continue @@ -742,10 +742,6 @@ func (lbc *loadBalancerController) getUpstreamServers(ngxCfg config.Configuratio } if addLoc { - locRew, err := rewrite.ParseAnnotations(ing) - if err != nil { - glog.V(3).Infof("error parsing rewrite annotations for Ingress rule %v/%v: %v", ing.GetNamespace(), ing.GetName(), err) - } server.Locations = append(server.Locations, &nginx.Location{ Path: nginxPath, diff --git a/ingress/controllers/nginx/nginx.tmpl b/ingress/controllers/nginx/nginx.tmpl index 0cc11c0c14..8edc3d9be3 100644 --- a/ingress/controllers/nginx/nginx.tmpl +++ b/ingress/controllers/nginx/nginx.tmpl @@ -172,10 +172,6 @@ http { {{- end }} {{ if (and $server.SSL $cfg.hsts) -}} - if ($scheme = http) { - return 301 https://$host$request_uri; - } - more_set_headers "Strict-Transport-Security: max-age={{ $cfg.hstsMaxAge }}{{ if $cfg.hstsIncludeSubdomains }}; includeSubDomains{{ end }}; preload"; {{- end }} @@ -184,6 +180,12 @@ http { {{- range $location := $server.Locations }} {{ $path := buildLocation $location }} location {{ $path }} { + {{ if (and $server.SSL $location.Redirect.SSLRedirect) -}} + # enforce ssl on server side + if ($scheme = http) { + return 301 https://$host$request_uri; + } + {{- end }} {{/* if the location contains a rate limit annotation, create one */}} {{ $limits := buildRateLimit $location }} {{- range $limit := $limits }} diff --git a/ingress/controllers/nginx/nginx/rewrite/main.go b/ingress/controllers/nginx/nginx/rewrite/main.go index ed17f31a33..9695b10078 100644 --- a/ingress/controllers/nginx/nginx/rewrite/main.go +++ b/ingress/controllers/nginx/nginx/rewrite/main.go @@ -21,22 +21,36 @@ import ( "strconv" "k8s.io/kubernetes/pkg/apis/extensions" + + "k8s.io/contrib/ingress/controllers/nginx/nginx/config" ) const ( - rewriteTo = "ingress.kubernetes.io/rewrite-target" - addBaseURL = "ingress.kubernetes.io/add-base-url" + rewriteTo = "ingress.kubernetes.io/rewrite-target" + addBaseURL = "ingress.kubernetes.io/add-base-url" + sslRedirect = "ingress.kubernetes.io/ssl-redirect" ) -// Redirect returns authentication configuration for an Ingress rule +// Redirect describes the per location redirect config type Redirect struct { // Target URI where the traffic must be redirected Target string // AddBaseURL indicates if is required to add a base tag in the head // of the responses from the upstream servers AddBaseURL bool + // Should indicates if the location section should be accessible SSL only + SSLRedirect bool } +var ( + // ErrMissingSSLRedirect returned error when the ingress does not contains the + // ssl-redirect annotation + ErrMissingSSLRedirect = errors.New("ssl-redirect annotations is missing") + + // ErrInvalidBool gets returned when the str value is not convertible to a bool + ErrInvalidBool = errors.New("ssl-redirect annotations has invalid value") +) + type ingAnnotations map[string]string func (a ingAnnotations) addBaseURL() bool { @@ -57,17 +71,39 @@ func (a ingAnnotations) rewriteTo() string { return "" } +func (a ingAnnotations) sslRedirect() (bool, error) { + val, ok := a[sslRedirect] + if !ok { + return false, ErrMissingSSLRedirect + } + + sr, err := strconv.ParseBool(val) + if err != nil { + return false, ErrInvalidBool + } + + return sr, nil +} + // ParseAnnotations parses the annotations contained in the ingress // rule used to rewrite the defined paths -func ParseAnnotations(ing *extensions.Ingress) (*Redirect, error) { +func ParseAnnotations(cfg config.Configuration, ing *extensions.Ingress) (*Redirect, error) { if ing.GetAnnotations() == nil { return &Redirect{}, errors.New("no annotations present") } - rt := ingAnnotations(ing.GetAnnotations()).rewriteTo() - abu := ingAnnotations(ing.GetAnnotations()).addBaseURL() + annotations := ingAnnotations(ing.GetAnnotations()) + + sslRe, err := annotations.sslRedirect() + if err != nil { + sslRe = cfg.SSLRedirect + } + + rt := annotations.rewriteTo() + abu := annotations.addBaseURL() return &Redirect{ - Target: rt, - AddBaseURL: abu, + Target: rt, + AddBaseURL: abu, + SSLRedirect: sslRe, }, nil } diff --git a/ingress/controllers/nginx/nginx/rewrite/main_test.go b/ingress/controllers/nginx/nginx/rewrite/main_test.go index 5e16adb7b1..804863ff74 100644 --- a/ingress/controllers/nginx/nginx/rewrite/main_test.go +++ b/ingress/controllers/nginx/nginx/rewrite/main_test.go @@ -118,3 +118,28 @@ func TestRedirect(t *testing.T) { t.Errorf("Expected %v as redirect but returned %s", defRoute, redirect.Target) } } + +func TestSSLRedirect(t *testing.T) { + ing := buildIngress() + + cfg := config.Configuration{SSLRedirect: true} + + data := map[string]string{} + + ing.SetAnnotations(data) + + redirect, _ := ParseAnnotations(cfg, ing) + + if !redirect.SSLRedirect { + t.Errorf("Expected true but returned false") + } + + data[sslRedirect] = "false" + ing.SetAnnotations(data) + + redirect, _ = ParseAnnotations(cfg, ing) + + if redirect.SSLRedirect { + t.Errorf("Expected false but returned true") + } +}