From 41d82005ec49c5a0a900d82ad9b00e632a94fde2 Mon Sep 17 00:00:00 2001 From: agile6v Date: Mon, 11 May 2020 16:31:08 +0800 Subject: [PATCH 1/4] Add annotation ssl-prefer-server-ciphers. --- .../nginx-configuration/annotations.md | 7 +++++ internal/ingress/annotations/annotations.go | 4 +-- .../ingress/annotations/sslcipher/main.go | 26 +++++++++++++++++-- .../annotations/sslcipher/main_test.go | 2 +- internal/ingress/controller/controller.go | 14 +++++++--- internal/ingress/types.go | 3 +++ internal/ingress/types_equals.go | 3 +++ rootfs/etc/nginx/template/nginx.tmpl | 4 +++ 8 files changed, 54 insertions(+), 9 deletions(-) diff --git a/docs/user-guide/nginx-configuration/annotations.md b/docs/user-guide/nginx-configuration/annotations.md index 78c77d5db..4709eb7a5 100755 --- a/docs/user-guide/nginx-configuration/annotations.md +++ b/docs/user-guide/nginx-configuration/annotations.md @@ -100,6 +100,7 @@ You can add these Kubernetes annotations to specific Ingress objects to customiz |[nginx.ingress.kubernetes.io/proxy-buffer-size](#proxy-buffer-size)|string| |[nginx.ingress.kubernetes.io/proxy-max-temp-file-size](#proxy-max-temp-file-size)|string| |[nginx.ingress.kubernetes.io/ssl-ciphers](#ssl-ciphers)|string| +|[nginx.ingress.kubernetes.io/ssl-prefer-server-ciphers](#ssl-ciphers)|"true" or "false"| |[nginx.ingress.kubernetes.io/connection-proxy-header](#connection-proxy-header)|string| |[nginx.ingress.kubernetes.io/enable-access-log](#enable-access-log)|"true" or "false"| |[nginx.ingress.kubernetes.io/enable-opentracing](#enable-opentracing)|"true" or "false"| @@ -646,6 +647,12 @@ Using this annotation will set the `ssl_ciphers` directive at the server level. nginx.ingress.kubernetes.io/ssl-ciphers: "ALL:!aNULL:!EXPORT56:RC4+RSA:+HIGH:+MEDIUM:+LOW:+SSLv2:+EXP" ``` +The following annotation will set the `ssl_prefer_server_ciphers` directive at the server level. This configuration specifies that server ciphers should be preferred over client ciphers when using the SSLv3 and TLS protocols. + +```yaml +nginx.ingress.kubernetes.io/ssl-prefer-server-ciphers: "true" +``` + ### Connection proxy header Using this annotation will override the default connection header set by NGINX. diff --git a/internal/ingress/annotations/annotations.go b/internal/ingress/annotations/annotations.go index d46a09ffa..7fe64efd8 100644 --- a/internal/ingress/annotations/annotations.go +++ b/internal/ingress/annotations/annotations.go @@ -108,7 +108,7 @@ type Ingress struct { UpstreamVhost string Whitelist ipwhitelist.SourceRange XForwardedPrefix string - SSLCiphers string + SSLCipher sslcipher.Config Logs log.Config InfluxDB influxdb.Config ModSecurity modsecurity.Config @@ -156,7 +156,7 @@ func NewAnnotationExtractor(cfg resolver.Resolver) Extractor { "UpstreamVhost": upstreamvhost.NewParser(cfg), "Whitelist": ipwhitelist.NewParser(cfg), "XForwardedPrefix": xforwardedprefix.NewParser(cfg), - "SSLCiphers": sslcipher.NewParser(cfg), + "SSLCipher": sslcipher.NewParser(cfg), "Logs": log.NewParser(cfg), "InfluxDB": influxdb.NewParser(cfg), "BackendProtocol": backendprotocol.NewParser(cfg), diff --git a/internal/ingress/annotations/sslcipher/main.go b/internal/ingress/annotations/sslcipher/main.go index 267694fef..a3a6b5ba4 100644 --- a/internal/ingress/annotations/sslcipher/main.go +++ b/internal/ingress/annotations/sslcipher/main.go @@ -27,13 +27,35 @@ type sslCipher struct { r resolver.Resolver } +type Config struct { + SSLCiphers string + SSLPreferServerCiphers string +} + // NewParser creates a new sslCipher annotation parser func NewParser(r resolver.Resolver) parser.IngressAnnotation { return sslCipher{r} } // Parse parses the annotations contained in the ingress rule -// used to add ssl-ciphers to the server name +// used to add ssl-ciphers & ssl-prefer-server-ciphers to the server name func (sc sslCipher) Parse(ing *networking.Ingress) (interface{}, error) { - return parser.GetStringAnnotation("ssl-ciphers", ing) + config := &Config{} + var err error + var sslPreferServerCiphers bool + + sslPreferServerCiphers, err = parser.GetBoolAnnotation("ssl-prefer-server-ciphers", ing) + if err != nil { + config.SSLPreferServerCiphers = "" + } else { + if sslPreferServerCiphers { + config.SSLPreferServerCiphers = "on" + } else { + config.SSLPreferServerCiphers = "off" + } + } + + config.SSLCiphers, _ = parser.GetStringAnnotation("ssl-ciphers", ing) + + return config, nil } diff --git a/internal/ingress/annotations/sslcipher/main_test.go b/internal/ingress/annotations/sslcipher/main_test.go index dbb0f500f..5957db203 100644 --- a/internal/ingress/annotations/sslcipher/main_test.go +++ b/internal/ingress/annotations/sslcipher/main_test.go @@ -56,7 +56,7 @@ func TestParse(t *testing.T) { for _, testCase := range testCases { ing.SetAnnotations(testCase.annotations) result, _ := ap.Parse(ing) - if result != testCase.expected { + if result.SSLCiphers != testCase.expected { t.Errorf("expected %v but returned %v, annotations: %s", testCase.expected, result, testCase.annotations) } } diff --git a/internal/ingress/controller/controller.go b/internal/ingress/controller/controller.go index b46b95c92..461ade9f9 100644 --- a/internal/ingress/controller/controller.go +++ b/internal/ingress/controller/controller.go @@ -1051,8 +1051,9 @@ func (n *NGINXController) createServers(data []*ingress.Ingress, Locations: []*ingress.Location{ loc, }, - SSLPassthrough: anns.SSLPassthrough, - SSLCiphers: anns.SSLCiphers, + SSLPassthrough: anns.SSLPassthrough, + SSLCiphers: anns.SSLCipher.SSLCiphers, + SSLPreferServerCiphers: anns.SSLCipher.SSLPreferServerCiphers, } } } @@ -1092,8 +1093,13 @@ func (n *NGINXController) createServers(data []*ingress.Ingress, } // only add SSL ciphers if the server does not have them previously configured - if servers[host].SSLCiphers == "" && anns.SSLCiphers != "" { - servers[host].SSLCiphers = anns.SSLCiphers + if servers[host].SSLCiphers == "" && anns.SSLCipher.SSLCiphers != "" { + servers[host].SSLCiphers = anns.SSLCipher.SSLCiphers + } + + // only add SSLPreferServerCiphers if the server does not have them previously configured + if servers[host].SSLPreferServerCiphers == "" && anns.SSLCipher.SSLPreferServerCiphers != "" { + servers[host].SSLPreferServerCiphers = anns.SSLCipher.SSLPreferServerCiphers } // only add a certificate if the server does not have one previously configured diff --git a/internal/ingress/types.go b/internal/ingress/types.go index dcf56a015..e082f0d62 100644 --- a/internal/ingress/types.go +++ b/internal/ingress/types.go @@ -200,6 +200,9 @@ type Server struct { ServerSnippet string `json:"serverSnippet"` // SSLCiphers returns list of ciphers to be enabled SSLCiphers string `json:"sslCiphers,omitempty"` + // SSLPreferServerCiphers indicates that server ciphers should be preferred + // over client ciphers when using the SSLv3 and TLS protocols. + SSLPreferServerCiphers string `sslPreferServerCiphers,omitempty` // AuthTLSError contains the reason why the access to a server should be denied AuthTLSError string `json:"authTLSError,omitempty"` } diff --git a/internal/ingress/types_equals.go b/internal/ingress/types_equals.go index 8ea8fba0f..358bdd248 100644 --- a/internal/ingress/types_equals.go +++ b/internal/ingress/types_equals.go @@ -308,6 +308,9 @@ func (s1 *Server) Equal(s2 *Server) bool { if s1.SSLCiphers != s2.SSLCiphers { return false } + if s1.SSLPreferServerCiphers != s2.SSLPreferServerCiphers { + return false + } if s1.AuthTLSError != s2.AuthTLSError { return false } diff --git a/rootfs/etc/nginx/template/nginx.tmpl b/rootfs/etc/nginx/template/nginx.tmpl index 9cf9cd22d..bdc321e1f 100755 --- a/rootfs/etc/nginx/template/nginx.tmpl +++ b/rootfs/etc/nginx/template/nginx.tmpl @@ -875,6 +875,10 @@ stream { ssl_ciphers {{ $server.SSLCiphers }}; {{ end }} + {{ if not (empty $server.SSLPreferServerCiphers) }} + ssl_prefer_server_ciphers {{ $server.SSLPreferServerCiphers }}; + {{ end }} + {{ if not (empty $server.ServerSnippet) }} {{ $server.ServerSnippet }} {{ end }} From 38a8556c4f9a3192174267332748fa1f3a8464ab Mon Sep 17 00:00:00 2001 From: agile6v Date: Wed, 13 May 2020 10:40:56 +0800 Subject: [PATCH 2/4] Add comments for sslcipher.Config struct. --- internal/ingress/annotations/sslcipher/main.go | 1 + 1 file changed, 1 insertion(+) diff --git a/internal/ingress/annotations/sslcipher/main.go b/internal/ingress/annotations/sslcipher/main.go index a3a6b5ba4..d100a0da4 100644 --- a/internal/ingress/annotations/sslcipher/main.go +++ b/internal/ingress/annotations/sslcipher/main.go @@ -27,6 +27,7 @@ type sslCipher struct { r resolver.Resolver } +// Config contains the ssl-ciphers & ssl-prefer-server-ciphers configuration type Config struct { SSLCiphers string SSLPreferServerCiphers string From 38f99cefb28dcdbfcdbb9aeea2eb878d9693865b Mon Sep 17 00:00:00 2001 From: agile6v Date: Wed, 13 May 2020 11:03:15 +0800 Subject: [PATCH 3/4] Update testcase for sslCipher. --- internal/ingress/annotations/sslcipher/main_test.go | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/internal/ingress/annotations/sslcipher/main_test.go b/internal/ingress/annotations/sslcipher/main_test.go index 5957db203..29f512c7b 100644 --- a/internal/ingress/annotations/sslcipher/main_test.go +++ b/internal/ingress/annotations/sslcipher/main_test.go @@ -56,7 +56,11 @@ func TestParse(t *testing.T) { for _, testCase := range testCases { ing.SetAnnotations(testCase.annotations) result, _ := ap.Parse(ing) - if result.SSLCiphers != testCase.expected { + config, ok := result.(*Config) + if !ok { + t.Fatalf("expected a Config type") + } + if config.SSLCiphers != testCase.expected { t.Errorf("expected %v but returned %v, annotations: %s", testCase.expected, result, testCase.annotations) } } From 0e79ad8e4f1814b8d220501d966534840f5feda2 Mon Sep 17 00:00:00 2001 From: agile6v Date: Thu, 21 May 2020 02:19:13 +0800 Subject: [PATCH 4/4] Update unit & e2e tests. --- .../annotations/sslcipher/main_test.go | 28 ++++++++++--------- test/e2e/annotations/sslciphers.go | 6 ++-- 2 files changed, 19 insertions(+), 15 deletions(-) diff --git a/internal/ingress/annotations/sslcipher/main_test.go b/internal/ingress/annotations/sslcipher/main_test.go index 29f512c7b..8110697dc 100644 --- a/internal/ingress/annotations/sslcipher/main_test.go +++ b/internal/ingress/annotations/sslcipher/main_test.go @@ -17,6 +17,7 @@ limitations under the License. package sslcipher import ( + "reflect" "testing" api "k8s.io/api/core/v1" @@ -27,22 +28,27 @@ import ( ) func TestParse(t *testing.T) { - annotation := parser.GetAnnotationWithPrefix("ssl-ciphers") ap := NewParser(&resolver.Mock{}) if ap == nil { t.Fatalf("expected a parser.IngressAnnotation but returned nil") } + annotationSSLCiphers := parser.GetAnnotationWithPrefix("ssl-ciphers") + annotationSSLPreferServerCiphers := parser.GetAnnotationWithPrefix("ssl-prefer-server-ciphers") + testCases := []struct { annotations map[string]string - expected string + expected Config }{ - {map[string]string{annotation: "ALL:!aNULL:!EXPORT56:RC4+RSA:+HIGH:+MEDIUM:+LOW:+SSLv2:+EXP"}, "ALL:!aNULL:!EXPORT56:RC4+RSA:+HIGH:+MEDIUM:+LOW:+SSLv2:+EXP"}, - {map[string]string{annotation: "ECDHE-ECDSA-AES256-GCM-SHA384:ECDHE-RSA-AES256-GCM-SHA384:ECDHE-ECDSA-CHACHA20-POLY1305:ECDHE-RSA-CHACHA20-POLY1305:ECDHE-ECDSA-AES128-GCM-SHA256:ECDHE-RSA-AES128-GCM-SHA256:ECDHE-ECDSA-AES256-SHA384:ECDHE-RSA-AES256-SHA384:ECDHE-ECDSA-AES128-SHA256:ECDHE-RSA-AES128-SHA256"}, - "ECDHE-ECDSA-AES256-GCM-SHA384:ECDHE-RSA-AES256-GCM-SHA384:ECDHE-ECDSA-CHACHA20-POLY1305:ECDHE-RSA-CHACHA20-POLY1305:ECDHE-ECDSA-AES128-GCM-SHA256:ECDHE-RSA-AES128-GCM-SHA256:ECDHE-ECDSA-AES256-SHA384:ECDHE-RSA-AES256-SHA384:ECDHE-ECDSA-AES128-SHA256:ECDHE-RSA-AES128-SHA256"}, - {map[string]string{annotation: ""}, ""}, - {map[string]string{}, ""}, - {nil, ""}, + {map[string]string{annotationSSLCiphers: "ALL:!aNULL:!EXPORT56:RC4+RSA:+HIGH:+MEDIUM:+LOW:+SSLv2:+EXP"}, Config{"ALL:!aNULL:!EXPORT56:RC4+RSA:+HIGH:+MEDIUM:+LOW:+SSLv2:+EXP", ""}}, + {map[string]string{annotationSSLCiphers: "ECDHE-ECDSA-AES256-GCM-SHA384:ECDHE-RSA-AES256-GCM-SHA384:ECDHE-ECDSA-CHACHA20-POLY1305:ECDHE-RSA-CHACHA20-POLY1305:ECDHE-ECDSA-AES128-GCM-SHA256:ECDHE-RSA-AES128-GCM-SHA256:ECDHE-ECDSA-AES256-SHA384:ECDHE-RSA-AES256-SHA384:ECDHE-ECDSA-AES128-SHA256:ECDHE-RSA-AES128-SHA256"}, + Config{"ECDHE-ECDSA-AES256-GCM-SHA384:ECDHE-RSA-AES256-GCM-SHA384:ECDHE-ECDSA-CHACHA20-POLY1305:ECDHE-RSA-CHACHA20-POLY1305:ECDHE-ECDSA-AES128-GCM-SHA256:ECDHE-RSA-AES128-GCM-SHA256:ECDHE-ECDSA-AES256-SHA384:ECDHE-RSA-AES256-SHA384:ECDHE-ECDSA-AES128-SHA256:ECDHE-RSA-AES128-SHA256", ""}}, + {map[string]string{annotationSSLCiphers: ""}, Config{"", ""}}, + {map[string]string{annotationSSLPreferServerCiphers: "true"}, Config{"", "on"}}, + {map[string]string{annotationSSLPreferServerCiphers: "false"}, Config{"", "off"}}, + {map[string]string{annotationSSLCiphers: "ALL:!aNULL:!EXPORT56:RC4+RSA:+HIGH:+MEDIUM:+LOW:+SSLv2:+EXP", annotationSSLPreferServerCiphers: "true"}, Config{"ALL:!aNULL:!EXPORT56:RC4+RSA:+HIGH:+MEDIUM:+LOW:+SSLv2:+EXP", "on"}}, + {map[string]string{}, Config{"", ""}}, + {nil, Config{"", ""}}, } ing := &networking.Ingress{ @@ -56,11 +62,7 @@ func TestParse(t *testing.T) { for _, testCase := range testCases { ing.SetAnnotations(testCase.annotations) result, _ := ap.Parse(ing) - config, ok := result.(*Config) - if !ok { - t.Fatalf("expected a Config type") - } - if config.SSLCiphers != testCase.expected { + if !reflect.DeepEqual(result, &testCase.expected) { t.Errorf("expected %v but returned %v, annotations: %s", testCase.expected, result, testCase.annotations) } } diff --git a/test/e2e/annotations/sslciphers.go b/test/e2e/annotations/sslciphers.go index ca464bf3f..0e2753b52 100644 --- a/test/e2e/annotations/sslciphers.go +++ b/test/e2e/annotations/sslciphers.go @@ -34,7 +34,8 @@ var _ = framework.DescribeAnnotation("ssl-ciphers", func() { ginkgo.It("should change ssl ciphers", func() { host := "ciphers.foo.com" annotations := map[string]string{ - "nginx.ingress.kubernetes.io/ssl-ciphers": "ALL:!aNULL:!EXPORT56:RC4+RSA:+HIGH:+MEDIUM:+LOW:+SSLv2:+EXP", + "nginx.ingress.kubernetes.io/ssl-ciphers": "ALL:!aNULL:!EXPORT56:RC4+RSA:+HIGH:+MEDIUM:+LOW:+SSLv2:+EXP", + "nginx.ingress.kubernetes.io/ssl-prefer-server-ciphers": "false", } ing := framework.NewSingleIngress(host, "/something", host, f.Namespace, framework.EchoService, 80, annotations) @@ -42,7 +43,8 @@ var _ = framework.DescribeAnnotation("ssl-ciphers", func() { f.WaitForNginxServer(host, func(server string) bool { - return strings.Contains(server, "ssl_ciphers ALL:!aNULL:!EXPORT56:RC4+RSA:+HIGH:+MEDIUM:+LOW:+SSLv2:+EXP;") + return strings.Contains(server, "ssl_ciphers ALL:!aNULL:!EXPORT56:RC4+RSA:+HIGH:+MEDIUM:+LOW:+SSLv2:+EXP;") && + strings.Contains(server, "ssl_prefer_server_ciphers off;") }) }) })