From f022d445c60902b9681300461342092fd0346ba4 Mon Sep 17 00:00:00 2001 From: Nico Schieder Date: Fri, 18 Nov 2016 22:47:26 +0100 Subject: [PATCH 1/2] Added hsts settings, Refactored bool/integer parsing --- nginx-controller/controller/controller.go | 41 ++++-- nginx-controller/nginx/config.go | 4 + nginx-controller/nginx/configurator.go | 55 +++++--- nginx-controller/nginx/convert.go | 46 +++++++ nginx-controller/nginx/convert_test.go | 149 ++++++++++++++++++++++ nginx-controller/nginx/ingress.tmpl | 4 +- nginx-controller/nginx/nginx.go | 17 ++- 7 files changed, 278 insertions(+), 38 deletions(-) create mode 100644 nginx-controller/nginx/convert.go create mode 100644 nginx-controller/nginx/convert_test.go diff --git a/nginx-controller/controller/controller.go b/nginx-controller/controller/controller.go index d6b90b4409..b8fec17e8d 100644 --- a/nginx-controller/controller/controller.go +++ b/nginx-controller/controller/controller.go @@ -19,7 +19,6 @@ package controller import ( "fmt" "reflect" - "strconv" "strings" "time" @@ -322,22 +321,38 @@ func (lbc *LoadBalancerController) syncCfgm(key string) { if serverNamesHashMaxSize, exists := cfgm.Data["server-names-hash-max-size"]; exists { cfg.MainServerNamesHashMaxSize = serverNamesHashMaxSize } - if HTTP2Str, exists := cfgm.Data["http2"]; exists { - if HTTP2, err := strconv.ParseBool(HTTP2Str); err == nil { - cfg.HTTP2 = HTTP2 - } else { - glog.Errorf("In configmap %v/%v 'http2' contains invalid declaration: %v, ignoring", cfgm.Namespace, cfgm.Name, err) - } + HTTP2, err := nginx.GetMapKeyAsBool(cfgm.Data, "http2", cfgm) + if err != nil && err != nginx.ErrorKeyNotFound { + glog.Error(err) + } else { + cfg.HTTP2 = HTTP2 + } + HSTS, err := nginx.GetMapKeyAsBool(cfgm.Data, "hsts", cfgm) + if err != nil && err != nginx.ErrorKeyNotFound { + glog.Error(err) + } else { + cfg.HSTS = HSTS + } + HSTSMaxAge, err := nginx.GetMapKeyAsInt(cfgm.Data, "hsts-max-age", cfgm) + if err != nil && err != nginx.ErrorKeyNotFound { + glog.Error(err) + } else { + cfg.HSTSMaxAge = HSTSMaxAge + } + HSTSIncludeSubdomains, err := nginx.GetMapKeyAsBool(cfgm.Data, "hsts-include-subdomains", cfgm) + if err != nil && err != nginx.ErrorKeyNotFound { + glog.Error(err) + } else { + cfg.HSTSIncludeSubdomains = HSTSIncludeSubdomains } if logFormat, exists := cfgm.Data["log-format"]; exists { cfg.MainLogFormat = logFormat } - if proxyBufferingStr, exists := cfgm.Data["proxy-buffering"]; exists { - if ProxyBuffering, err := strconv.ParseBool(proxyBufferingStr); err == nil { - cfg.ProxyBuffering = ProxyBuffering - } else { - glog.Errorf("In configmap %v/%v 'proxy-buffering' contains invalid declaration: %v, ignoring", cfgm.Namespace, cfgm.Name, err) - } + ProxyBuffering, err := nginx.GetMapKeyAsBool(cfgm.Data, "proxy-buffering", cfgm) + if err != nil && err != nginx.ErrorKeyNotFound { + glog.Error(err) + } else { + cfg.ProxyBuffering = ProxyBuffering } if proxyBuffers, exists := cfgm.Data["proxy-buffers"]; exists { cfg.ProxyBuffers = proxyBuffers diff --git a/nginx-controller/nginx/config.go b/nginx-controller/nginx/config.go index d7de99ff79..90837ec919 100644 --- a/nginx-controller/nginx/config.go +++ b/nginx-controller/nginx/config.go @@ -13,6 +13,9 @@ type Config struct { ProxyBuffers string ProxyBufferSize string ProxyMaxTempFileSize string + HSTS bool + HSTSMaxAge int64 + HSTSIncludeSubdomains bool } // NewDefaultConfig creates a Config with default values @@ -23,5 +26,6 @@ func NewDefaultConfig() *Config { ClientMaxBodySize: "1m", MainServerNamesHashMaxSize: "512", ProxyBuffering: true, + HSTSMaxAge: 2592000, } } diff --git a/nginx-controller/nginx/configurator.go b/nginx-controller/nginx/configurator.go index aaab417326..261359c3b3 100644 --- a/nginx-controller/nginx/configurator.go +++ b/nginx-controller/nginx/configurator.go @@ -2,7 +2,6 @@ package nginx import ( "fmt" - "strconv" "strings" "sync" @@ -104,8 +103,11 @@ func (cnf *Configurator) generateNginxCfg(ingEx *IngressEx, pems map[string]stri } server := Server{ - Name: serverName, - HTTP2: ingCfg.HTTP2, + Name: serverName, + HTTP2: ingCfg.HTTP2, + HSTS: ingCfg.HSTS, + HSTSMaxAge: ingCfg.HSTSMaxAge, + HSTSIncludeSubdomains: ingCfg.HSTSIncludeSubdomains, } if pemFile, ok := pems[serverName]; ok { @@ -145,8 +147,11 @@ func (cnf *Configurator) generateNginxCfg(ingEx *IngressEx, pems map[string]stri if len(ingEx.Ingress.Spec.Rules) == 0 && ingEx.Ingress.Spec.Backend != nil { server := Server{ - Name: emptyHost, - HTTP2: ingCfg.HTTP2, + Name: emptyHost, + HTTP2: ingCfg.HTTP2, + HSTS: ingCfg.HSTS, + HSTSMaxAge: ingCfg.HSTSMaxAge, + HSTSIncludeSubdomains: ingCfg.HSTSIncludeSubdomains, } if pemFile, ok := pems[emptyHost]; ok { @@ -180,19 +185,35 @@ func (cnf *Configurator) createConfig(ingEx *IngressEx) Config { if clientMaxBodySize, exists := ingEx.Ingress.Annotations["nginx.org/client-max-body-size"]; exists { ingCfg.ClientMaxBodySize = clientMaxBodySize } - if HTTP2Str, exists := ingEx.Ingress.Annotations["nginx.org/http2"]; exists { - if HTTP2, err := strconv.ParseBool(HTTP2Str); err == nil { - ingCfg.HTTP2 = HTTP2 - } else { - glog.Errorf("In %v/%v nginx.org/http2 contains invalid declaration: %v, ignoring", ingEx.Ingress.Namespace, ingEx.Ingress.Name, err) - } + HTTP2, err := GetMapKeyAsBool(ingEx.Ingress.Annotations, "nginx.org/http2", ingEx.Ingress) + if err != nil && err != ErrorKeyNotFound { + glog.Error(err) + } else { + ingCfg.HTTP2 = HTTP2 } - if proxyBufferingStr, exists := ingEx.Ingress.Annotations["nginx.org/proxy-buffering"]; exists { - if ProxyBuffering, err := strconv.ParseBool(proxyBufferingStr); err == nil { - ingCfg.ProxyBuffering = ProxyBuffering - } else { - glog.Errorf("In %v/%v nginx.org/proxy-buffering contains invalid declaration: %v, ignoring", ingEx.Ingress.Namespace, ingEx.Ingress.Name, err) - } + ProxyBuffering, err := GetMapKeyAsBool(ingEx.Ingress.Annotations, "nginx.org/proxy-buffering", ingEx.Ingress) + if err != nil && err != ErrorKeyNotFound { + glog.Error(err) + } else { + ingCfg.ProxyBuffering = ProxyBuffering + } + HSTS, err := GetMapKeyAsBool(ingEx.Ingress.Annotations, "nginx.org/hsts", ingEx.Ingress) + if err != nil && err != ErrorKeyNotFound { + glog.Error(err) + } else { + ingCfg.HSTS = HSTS + } + HSTSMaxAge, err := GetMapKeyAsInt(ingEx.Ingress.Annotations, "nginx.org/hsts-max-age", ingEx.Ingress) + if err != nil && err != ErrorKeyNotFound { + glog.Error(err) + } else { + ingCfg.HSTSMaxAge = HSTSMaxAge + } + HSTSIncludeSubdomains, err := GetMapKeyAsBool(ingEx.Ingress.Annotations, "nginx.org/hsts-include-subdomains", ingEx.Ingress) + if err != nil && err != ErrorKeyNotFound { + glog.Error(err) + } else { + ingCfg.HSTSIncludeSubdomains = HSTSIncludeSubdomains } if proxyBuffers, exists := ingEx.Ingress.Annotations["nginx.org/proxy-buffers"]; exists { ingCfg.ProxyBuffers = proxyBuffers diff --git a/nginx-controller/nginx/convert.go b/nginx-controller/nginx/convert.go new file mode 100644 index 0000000000..ba246ff248 --- /dev/null +++ b/nginx-controller/nginx/convert.go @@ -0,0 +1,46 @@ +package nginx + +import ( + "errors" + "fmt" + "strconv" + + "k8s.io/kubernetes/pkg/api/meta" + "k8s.io/kubernetes/pkg/runtime" +) + +var ( + // ErrorKeyNotFound is returned if the key was not found in the map + ErrorKeyNotFound = errors.New("Key not found in map") +) + +// There seems to be no composite interface in the kubernetes api package, +// so we have to declare our own. +type apiObject interface { + meta.Object + runtime.Object +} + +// GetMapKeyAsBool searches the map for the given key and parses the key as bool +func GetMapKeyAsBool(m map[string]string, key string, context apiObject) (bool, error) { + if str, exists := m[key]; exists { + b, err := strconv.ParseBool(str) + if err != nil { + return false, fmt.Errorf("%s %v/%v '%s' contains invalid bool: %v, ignoring", context.GetObjectKind().GroupVersionKind().Kind, context.GetNamespace(), context.GetName(), key, err) + } + return b, nil + } + return false, ErrorKeyNotFound +} + +// GetMapKeyAsInt tries to find and parse a key in a map as int64 +func GetMapKeyAsInt(m map[string]string, key string, context apiObject) (int64, error) { + if str, exists := m[key]; exists { + i, err := strconv.ParseInt(str, 10, 64) + if err != nil { + return 0, fmt.Errorf("%s %v/%v '%s' contains invalid integer: %v, ignoring", context.GetObjectKind().GroupVersionKind().Kind, context.GetNamespace(), context.GetName(), key, err) + } + return i, nil + } + return 0, ErrorKeyNotFound +} diff --git a/nginx-controller/nginx/convert_test.go b/nginx-controller/nginx/convert_test.go new file mode 100644 index 0000000000..542190ecbe --- /dev/null +++ b/nginx-controller/nginx/convert_test.go @@ -0,0 +1,149 @@ +package nginx + +import ( + "testing" + + "k8s.io/kubernetes/pkg/api" + "k8s.io/kubernetes/pkg/api/unversioned" + "k8s.io/kubernetes/pkg/apis/extensions" +) + +var configMap = api.ConfigMap{ + ObjectMeta: api.ObjectMeta{ + Name: "test", + Namespace: "default", + }, + TypeMeta: unversioned.TypeMeta{ + Kind: "ConfigMap", + APIVersion: "v1", + }, +} +var ingress = extensions.Ingress{ + ObjectMeta: api.ObjectMeta{ + Name: "test", + Namespace: "kube-system", + }, + TypeMeta: unversioned.TypeMeta{ + Kind: "Ingress", + APIVersion: "extensions/v1beta1", + }, +} + +// +// GetMapKeyAsBool +// +func TestGetMapKeyAsBool(t *testing.T) { + configMap := configMap + configMap.Data = map[string]string{ + "key": "True", + } + + b, err := GetMapKeyAsBool(configMap.Data, "key", &configMap) + if err != nil { + t.Errorf("Unexpected error: %v", err) + } + if b != true { + t.Errorf("Result should be true") + } +} + +func TestGetMapKeyAsBoolNotFound(t *testing.T) { + configMap := configMap + configMap.Data = map[string]string{} + + _, err := GetMapKeyAsBool(configMap.Data, "key", &configMap) + if err != ErrorKeyNotFound { + t.Errorf("ErrorKeyNotFound was expected, got: %v", err) + } +} + +func TestGetMapKeyAsBoolErrorMessage(t *testing.T) { + cfgm := configMap + cfgm.Data = map[string]string{ + "key": "string", + } + + // Test with configmap + _, err := GetMapKeyAsBool(cfgm.Data, "key", &cfgm) + if err == nil { + t.Error("An error was expected") + } + expected := `ConfigMap default/test 'key' contains invalid bool: strconv.ParseBool: parsing "string": invalid syntax, ignoring` + if err.Error() != expected { + t.Errorf("The error message does not match expectations:\nGot: %v\nExpected: %v", err, expected) + } + + // Test with ingress object + ingress := ingress + ingress.Annotations = map[string]string{ + "key": "other_string", + } + _, err = GetMapKeyAsBool(ingress.Annotations, "key", &ingress) + if err == nil { + t.Error("An error was expected") + } + expected = `Ingress kube-system/test 'key' contains invalid bool: strconv.ParseBool: parsing "other_string": invalid syntax, ignoring` + if err.Error() != expected { + t.Errorf("The error message does not match expectations:\nGot: %v\nExpected: %v", err, expected) + } +} + +// +// GetMapKeyAsInt +// +func TestGetMapKeyAsInt(t *testing.T) { + configMap := configMap + configMap.Data = map[string]string{ + "key": "123456789", + } + + i, err := GetMapKeyAsInt(configMap.Data, "key", &configMap) + if err != nil { + t.Errorf("Unexpected error: %v", err) + } + var expected int64 = 123456789 + if i != expected { + t.Errorf("Unexpected return value:\nGot: %v\nExpected: %v", i, expected) + } +} + +func TestGetMapKeyAsIntNotFound(t *testing.T) { + configMap := configMap + configMap.Data = map[string]string{} + + _, err := GetMapKeyAsInt(configMap.Data, "key", &configMap) + if err != ErrorKeyNotFound { + t.Errorf("ErrorKeyNotFound was expected, got: %v", err) + } +} + +func TestGetMapKeyAsIntErrorMessage(t *testing.T) { + cfgm := configMap + cfgm.Data = map[string]string{ + "key": "string", + } + + // Test with configmap + _, err := GetMapKeyAsInt(cfgm.Data, "key", &cfgm) + if err == nil { + t.Error("An error was expected") + } + expected := `ConfigMap default/test 'key' contains invalid integer: strconv.ParseInt: parsing "string": invalid syntax, ignoring` + if err.Error() != expected { + t.Errorf("The error message does not match expectations:\nGot: %v\nExpected: %v", err, expected) + } + + // Test with ingress object + ingress := ingress + ingress.Annotations = map[string]string{ + "key": "other_string", + } + _, err = GetMapKeyAsInt(ingress.Annotations, "key", &ingress) + if err == nil { + t.Error("An error was expected") + } + expected = `Ingress kube-system/test 'key' contains invalid integer: strconv.ParseInt: parsing "other_string": invalid syntax, ignoring` + if err.Error() != expected { + t.Errorf("The error message does not match expectations:\nGot: %v\nExpected: %v", err, expected) + } +} diff --git a/nginx-controller/nginx/ingress.tmpl b/nginx-controller/nginx/ingress.tmpl index 755940b12b..78c2278561 100644 --- a/nginx-controller/nginx/ingress.tmpl +++ b/nginx-controller/nginx/ingress.tmpl @@ -21,7 +21,9 @@ server { if ($scheme = http) { return 301 https://$host$request_uri; } - {{end}} + {{- if $server.HSTS}} + add_header Strict-Transport-Security "max-age={{$server.HSTSMaxAge}}; {{if $server.HSTSIncludeSubdomains}}includeSubDomains; {{end}}preload" always;{{end}} + {{- end}} {{range $location := $server.Locations}} location {{$location.Path}} { diff --git a/nginx-controller/nginx/nginx.go b/nginx-controller/nginx/nginx.go index 5d32b166c1..2d2fb17050 100644 --- a/nginx-controller/nginx/nginx.go +++ b/nginx-controller/nginx/nginx.go @@ -3,10 +3,10 @@ package nginx import ( "bytes" "fmt" - "text/template" "os" "os/exec" "path" + "text/template" "github.com/golang/glog" ) @@ -38,12 +38,15 @@ type UpstreamServer struct { // Server describes an NGINX server type Server struct { - Name string - Locations []Location - SSL bool - SSLCertificate string - SSLCertificateKey string - HTTP2 bool + Name string + Locations []Location + SSL bool + SSLCertificate string + SSLCertificateKey string + HTTP2 bool + HSTS bool + HSTSMaxAge int64 + HSTSIncludeSubdomains bool } // Location describes an NGINX location From faf07d59df7e7f6d4179aba57057b857be0aec02 Mon Sep 17 00:00:00 2001 From: Nico Schieder Date: Wed, 23 Nov 2016 11:56:49 +0100 Subject: [PATCH 2/2] Added HSTS settings are only used if there is no error in any of them --- nginx-controller/controller/controller.go | 16 ++++++++++++---- nginx-controller/nginx/configurator.go | 16 ++++++++++++---- 2 files changed, 24 insertions(+), 8 deletions(-) diff --git a/nginx-controller/controller/controller.go b/nginx-controller/controller/controller.go index b8fec17e8d..78ef6c3b42 100644 --- a/nginx-controller/controller/controller.go +++ b/nginx-controller/controller/controller.go @@ -327,24 +327,32 @@ func (lbc *LoadBalancerController) syncCfgm(key string) { } else { cfg.HTTP2 = HTTP2 } + + // HSTS block + HSTSErrors := false HSTS, err := nginx.GetMapKeyAsBool(cfgm.Data, "hsts", cfgm) if err != nil && err != nginx.ErrorKeyNotFound { glog.Error(err) - } else { - cfg.HSTS = HSTS + HSTSErrors = true } HSTSMaxAge, err := nginx.GetMapKeyAsInt(cfgm.Data, "hsts-max-age", cfgm) if err != nil && err != nginx.ErrorKeyNotFound { glog.Error(err) - } else { - cfg.HSTSMaxAge = HSTSMaxAge + HSTSErrors = true } HSTSIncludeSubdomains, err := nginx.GetMapKeyAsBool(cfgm.Data, "hsts-include-subdomains", cfgm) if err != nil && err != nginx.ErrorKeyNotFound { glog.Error(err) + HSTSErrors = true + } + if HSTSErrors { + glog.Warningf("Configmap %s/%s: There are configuration issues with hsts annotations, skipping options for all hsts settings", cfgm.GetNamespace(), cfgm.GetName()) } else { + cfg.HSTS = HSTS + cfg.HSTSMaxAge = HSTSMaxAge cfg.HSTSIncludeSubdomains = HSTSIncludeSubdomains } + if logFormat, exists := cfgm.Data["log-format"]; exists { cfg.MainLogFormat = logFormat } diff --git a/nginx-controller/nginx/configurator.go b/nginx-controller/nginx/configurator.go index 261359c3b3..2f4d55e039 100644 --- a/nginx-controller/nginx/configurator.go +++ b/nginx-controller/nginx/configurator.go @@ -197,24 +197,32 @@ func (cnf *Configurator) createConfig(ingEx *IngressEx) Config { } else { ingCfg.ProxyBuffering = ProxyBuffering } + + // HSTS block + HSTSErrors := false HSTS, err := GetMapKeyAsBool(ingEx.Ingress.Annotations, "nginx.org/hsts", ingEx.Ingress) if err != nil && err != ErrorKeyNotFound { glog.Error(err) - } else { - ingCfg.HSTS = HSTS + HSTSErrors = true } HSTSMaxAge, err := GetMapKeyAsInt(ingEx.Ingress.Annotations, "nginx.org/hsts-max-age", ingEx.Ingress) if err != nil && err != ErrorKeyNotFound { glog.Error(err) - } else { - ingCfg.HSTSMaxAge = HSTSMaxAge + HSTSErrors = true } HSTSIncludeSubdomains, err := GetMapKeyAsBool(ingEx.Ingress.Annotations, "nginx.org/hsts-include-subdomains", ingEx.Ingress) if err != nil && err != ErrorKeyNotFound { glog.Error(err) + HSTSErrors = true + } + if HSTSErrors { + glog.Warningf("Ingress %s/%s: There are configuration issues with hsts annotations, skipping annotions for all hsts settings", ingEx.Ingress.GetNamespace(), ingEx.Ingress.GetName()) } else { + ingCfg.HSTS = HSTS + ingCfg.HSTSMaxAge = HSTSMaxAge ingCfg.HSTSIncludeSubdomains = HSTSIncludeSubdomains } + if proxyBuffers, exists := ingEx.Ingress.Annotations["nginx.org/proxy-buffers"]; exists { ingCfg.ProxyBuffers = proxyBuffers }