From d4a6ade65fcb5bda8af3a3017399c4619f667315 Mon Sep 17 00:00:00 2001 From: James Strong Date: Tue, 23 Nov 2021 12:06:17 -0500 Subject: [PATCH] update default block list,docs, tests (#7942) * update default block list,docs, tests * fix config for admin test * gofmt * remove the err return --- .../nginx-configuration/configmap.md | 17 ++--- internal/ingress/controller/config/config.go | 18 +---- internal/ingress/controller/controller.go | 13 +++- internal/ingress/controller/store/store.go | 9 +-- test/e2e/admission/admission.go | 6 ++ test/e2e/framework/util.go | 2 +- test/e2e/settings/badannotationvalues.go | 67 +++++++++++-------- 7 files changed, 69 insertions(+), 63 deletions(-) diff --git a/docs/user-guide/nginx-configuration/configmap.md b/docs/user-guide/nginx-configuration/configmap.md index d58ef17cd..0bac62ac1 100755 --- a/docs/user-guide/nginx-configuration/configmap.md +++ b/docs/user-guide/nginx-configuration/configmap.md @@ -30,7 +30,7 @@ The following table shows a configuration option's name, type, and the default v |[add-headers](#add-headers)|string|""| |[allow-backend-server-header](#allow-backend-server-header)|bool|"false"| |[allow-snippet-annotations](#allow-snippet-annotations)|bool|true| -|[annotation-value-word-blocklist](#annotation-value-word-blocklist)|string array|"load_module","lua_package","_by_lua","location","root","proxy_pass","serviceaccount","{","}","'","\" +|[annotation-value-word-blocklist](#annotation-value-word-blocklist)|string array|""| |[hide-headers](#hide-headers)|string array|empty| |[access-log-params](#access-log-params)|string|""| |[access-log-path](#access-log-path)|string|"/var/log/nginx/access.log"| @@ -226,20 +226,17 @@ may allow a user to add restricted configurations to the final nginx.conf file ## annotation-value-word-blocklist Contains a comma-separated value of chars/words that are well known of being used to abuse Ingress configuration -and must be blocked. +and must be blocked. Related to [CVE-2021-25742](https://github.com/kubernetes/ingress-nginx/issues/7837) -When an annotation is detected with a value that matches one of the blocked badwords, the whole Ingress wont be configured. +When an annotation is detected with a value that matches one of the blocked bad words, the whole Ingress won't be configured. -_**default:**_ `"load_module,lua_package,_by_lua,location,root,proxy_pass,serviceaccount,{,},',\"` +_**default:**_ `""` +When doing this, the default blocklist is override, which means that the Ingress admin should add all the words +that should be blocked, here is a suggested block list. -Warning: The default value already contains a sane set of badwords. Some features like mod_security needs characters that are blocked, and it's up to the Ingress admin to remove this characters from the blocklist. +_**suggested:**_ `"load_module,lua_package,_by_lua,location,root,proxy_pass,serviceaccount,{,},',\"` -When doing this, the default blocklist is overrided, which means that the Ingress admin should add all the words -that should be blocked. - -If you find some word should not be on the default list, or if you think that we should add more badwords, please -feel free to open an issue with your case! ## hide-headers Sets additional header that will not be passed from the upstream server to the client response. diff --git a/internal/ingress/controller/config/config.go b/internal/ingress/controller/config/config.go index b54257ebc..cbe0675a2 100644 --- a/internal/ingress/controller/config/config.go +++ b/internal/ingress/controller/config/config.go @@ -18,7 +18,6 @@ package config import ( "strconv" - "strings" "time" "k8s.io/klog/v2" @@ -767,21 +766,6 @@ func NewDefault() Configuration { defNginxStatusIpv4Whitelist := make([]string, 0) defNginxStatusIpv6Whitelist := make([]string, 0) defResponseHeaders := make([]string, 0) - - defAnnotationValueWordBlocklist := []string{ - "load_module", - "lua_package", - "_by_lua", - "location", - "root", - "proxy_pass", - "serviceaccount", - "{", - "}", - "'", - "\\", - } - defIPCIDR = append(defIPCIDR, "0.0.0.0/0") defNginxStatusIpv4Whitelist = append(defNginxStatusIpv4Whitelist, "127.0.0.1") defNginxStatusIpv6Whitelist = append(defNginxStatusIpv6Whitelist, "::1") @@ -792,7 +776,7 @@ func NewDefault() Configuration { AllowSnippetAnnotations: true, AllowBackendServerHeader: false, - AnnotationValueWordBlocklist: strings.Join(defAnnotationValueWordBlocklist, ","), + AnnotationValueWordBlocklist: "", AccessLogPath: "/var/log/nginx/access.log", AccessLogParams: "", EnableAccessLogForDefaultBackend: false, diff --git a/internal/ingress/controller/controller.go b/internal/ingress/controller/controller.go index fb06a58c6..3c2969c2c 100644 --- a/internal/ingress/controller/controller.go +++ b/internal/ingress/controller/controller.go @@ -242,7 +242,12 @@ func (n *NGINXController) CheckIngress(ing *networking.Ingress) error { cfg := n.store.GetBackendConfiguration() cfg.Resolver = n.resolver - arraybadWords := strings.Split(strings.TrimSpace(cfg.AnnotationValueWordBlocklist), ",") + var arrayBadWords []string + + if cfg.AnnotationValueWordBlocklist != "" { + arrayBadWords = strings.Split(strings.TrimSpace(cfg.AnnotationValueWordBlocklist), ",") + klog.Warningf("Blocklist is %s", cfg.AnnotationValueWordBlocklist) + } for key, value := range ing.ObjectMeta.GetAnnotations() { @@ -251,9 +256,11 @@ func (n *NGINXController) CheckIngress(ing *networking.Ingress) error { return fmt.Errorf("This deployment has a custom annotation prefix defined. Use '%s' instead of '%s'", parser.AnnotationsPrefix, parser.DefaultAnnotationsPrefix) } } - if strings.HasPrefix(key, fmt.Sprintf("%s/", parser.AnnotationsPrefix)) { - for _, forbiddenvalue := range arraybadWords { + + if strings.HasPrefix(key, fmt.Sprintf("%s/", parser.AnnotationsPrefix)) && len(arrayBadWords) != 0 { + for _, forbiddenvalue := range arrayBadWords { if strings.Contains(value, strings.TrimSpace(forbiddenvalue)) { + klog.Errorf("%s annotation contains invalid word %s", key, forbiddenvalue) return fmt.Errorf("%s annotation contains invalid word %s", key, forbiddenvalue) } } diff --git a/internal/ingress/controller/store/store.go b/internal/ingress/controller/store/store.go index fe0d1e0d7..c76b59bb0 100644 --- a/internal/ingress/controller/store/store.go +++ b/internal/ingress/controller/store/store.go @@ -823,10 +823,11 @@ func (s *k8sStore) syncIngress(ing *networkingv1.Ingress) { copyIng := &networkingv1.Ingress{} ing.ObjectMeta.DeepCopyInto(©Ing.ObjectMeta) - klog.Errorf("Blocklist: %v", s.backendConfig.AnnotationValueWordBlocklist) - if err := checkBadAnnotationValue(copyIng.Annotations, s.backendConfig.AnnotationValueWordBlocklist); err != nil { - klog.Errorf("skipping ingress %s: %s", key, err) - return + if s.backendConfig.AnnotationValueWordBlocklist != "" { + if err := checkBadAnnotationValue(copyIng.Annotations, s.backendConfig.AnnotationValueWordBlocklist); err != nil { + klog.Errorf("skipping ingress %s: %s", key, err) + return + } } ing.Spec.DeepCopyInto(©Ing.Spec) diff --git a/test/e2e/admission/admission.go b/test/e2e/admission/admission.go index 8b85f35d1..030c3854a 100644 --- a/test/e2e/admission/admission.go +++ b/test/e2e/admission/admission.go @@ -127,6 +127,9 @@ var _ = framework.IngressNginxDescribe("[Serial] admission controller", func() { annotations := map[string]string{ "nginx.ingress.kubernetes.io/connection-proxy-header": "a;}", } + + f.UpdateNginxConfigMapData("annotation-value-word-blocklist", "}") + firstIngress := framework.NewSingleIngress("first-ingress", "/", host, f.Namespace, framework.EchoService, 80, annotations) _, err := f.KubeClientSet.NetworkingV1().Ingresses(f.Namespace).Create(context.TODO(), firstIngress, metav1.CreateOptions{}) assert.NotNil(ginkgo.GinkgoT(), err, "creating an ingress with invalid annotation value should return an error") @@ -138,6 +141,9 @@ var _ = framework.IngressNginxDescribe("[Serial] admission controller", func() { annotations := map[string]string{ "nginx.ingress.kubernetes.io/connection-proxy-header": "set_by_lua", } + + f.UpdateNginxConfigMapData("annotation-value-word-blocklist", "set_by_lua") + firstIngress := framework.NewSingleIngress("first-ingress", "/", host, f.Namespace, framework.EchoService, 80, annotations) _, err := f.KubeClientSet.NetworkingV1().Ingresses(f.Namespace).Create(context.TODO(), firstIngress, metav1.CreateOptions{}) assert.NotNil(ginkgo.GinkgoT(), err, "creating an ingress with invalid annotation value should return an error") diff --git a/test/e2e/framework/util.go b/test/e2e/framework/util.go index 3befb8369..af2545b89 100644 --- a/test/e2e/framework/util.go +++ b/test/e2e/framework/util.go @@ -42,7 +42,7 @@ const ( Poll = 2 * time.Second // DefaultTimeout time to wait for operations to complete - DefaultTimeout = 30 * time.Second + DefaultTimeout = 90 * time.Second ) func nowStamp() string { diff --git a/test/e2e/settings/badannotationvalues.go b/test/e2e/settings/badannotationvalues.go index 74ce1c21e..cae6605cc 100644 --- a/test/e2e/settings/badannotationvalues.go +++ b/test/e2e/settings/badannotationvalues.go @@ -33,7 +33,7 @@ var _ = framework.DescribeAnnotation("Bad annotation values", func() { f.NewEchoDeployment() }) - ginkgo.It("should drop an ingress if there is an invalid character in some annotation", func() { + ginkgo.It("[BAD_ANNOTATIONS] should drop an ingress if there is an invalid character in some annotation", func() { host := "invalid-value-test" annotations := map[string]string{ @@ -43,6 +43,8 @@ var _ = framework.DescribeAnnotation("Bad annotation values", func() { ing := framework.NewSingleIngress(host, "/", host, f.Namespace, framework.EchoService, 80, annotations) f.UpdateNginxConfigMapData("allow-snippet-annotations", "true") + f.UpdateNginxConfigMapData("annotation-value-word-blocklist", "something_forbidden,otherthing_forbidden,{") + f.EnsureIngress(ing) f.WaitForNginxServer(host, @@ -62,7 +64,7 @@ var _ = framework.DescribeAnnotation("Bad annotation values", func() { Status(http.StatusNotFound) }) - ginkgo.It("should drop an ingress if there is a forbidden word in some annotation", func() { + ginkgo.It("[BAD_ANNOTATIONS] should drop an ingress if there is a forbidden word in some annotation", func() { host := "forbidden-value-test" annotations := map[string]string{ @@ -75,6 +77,7 @@ var _ = framework.DescribeAnnotation("Bad annotation values", func() { ing := framework.NewSingleIngress(host, "/", host, f.Namespace, framework.EchoService, 80, annotations) f.UpdateNginxConfigMapData("allow-snippet-annotations", "true") + f.UpdateNginxConfigMapData("annotation-value-word-blocklist", "something_forbidden,otherthing_forbidden,content_by_lua_block") // Sleep a while just to guarantee that the configmap is applied framework.Sleep() f.EnsureIngress(ing) @@ -96,13 +99,7 @@ var _ = framework.DescribeAnnotation("Bad annotation values", func() { Status(http.StatusNotFound) }) - ginkgo.It("should drop an ingress if there is a custom blocklist config in place and allow others to pass", func() { - host := "custom-forbidden-value-test" - - annotations := map[string]string{ - "nginx.ingress.kubernetes.io/configuration-snippet": ` - # something_forbidden`, - } + ginkgo.It("[BAD_ANNOTATIONS] should allow an ingress if there is a default blocklist config in place", func() { hostValid := "custom-allowed-value-test" annotationsValid := map[string]string{ @@ -110,44 +107,58 @@ var _ = framework.DescribeAnnotation("Bad annotation values", func() { # bla_by_lua`, } - ing := framework.NewSingleIngress(host, "/", host, f.Namespace, framework.EchoService, 80, annotations) ingValid := framework.NewSingleIngress(hostValid, "/", hostValid, f.Namespace, framework.EchoService, 80, annotationsValid) - f.UpdateNginxConfigMapData("annotation-value-word-blocklist", "something_forbidden,otherthing_forbidden") + // Sleep a while just to guarantee that the configmap is applied framework.Sleep() - f.EnsureIngress(ing) f.EnsureIngress(ingValid) - f.WaitForNginxServer(host, - func(server string) bool { - return !strings.Contains(server, fmt.Sprintf("server_name %s ;", host)) - }) - f.WaitForNginxServer(hostValid, func(server string) bool { return strings.Contains(server, fmt.Sprintf("server_name %s ;", hostValid)) }) - f.WaitForNginxServer(host, - func(server string) bool { - return !strings.Contains(server, "# something_forbidden") - }) - f.WaitForNginxServer(hostValid, func(server string) bool { return strings.Contains(server, "# bla_by_lua") }) - f.HTTPTestClient(). - GET("/"). - WithHeader("Host", host). - Expect(). - Status(http.StatusNotFound) - f.HTTPTestClient(). GET("/"). WithHeader("Host", hostValid). Expect(). Status(http.StatusOK) }) + + ginkgo.It("[BAD_ANNOTATIONS] should drop an ingress if there is a custom blocklist config in place and allow others to pass", func() { + host := "custom-forbidden-value-test" + + annotations := map[string]string{ + "nginx.ingress.kubernetes.io/configuration-snippet": ` + # something_forbidden`, + } + + ing := framework.NewSingleIngress(host, "/", host, f.Namespace, framework.EchoService, 80, annotations) + f.UpdateNginxConfigMapData("annotation-value-word-blocklist", "something_forbidden,otherthing_forbidden") + // Sleep a while just to guarantee that the configmap is applied + framework.Sleep() + f.EnsureIngress(ing) + + f.WaitForNginxServer(host, + func(server string) bool { + return !strings.Contains(server, fmt.Sprintf("server_name %s ;", host)) + }) + + f.WaitForNginxServer(host, + func(server string) bool { + return !strings.Contains(server, "# something_forbidden") + }) + + f.HTTPTestClient(). + GET("/"). + WithHeader("Host", host). + Expect(). + Status(http.StatusNotFound) + + }) })