diff --git a/docs/user-guide/nginx-configuration/configmap.md b/docs/user-guide/nginx-configuration/configmap.md index 4c07115da..ffcf9a1d8 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"| @@ -221,20 +221,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 a0c658d4d..0e53a1443 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" @@ -759,21 +758,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") @@ -784,7 +768,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 61cc755b2..3d60f292c 100644 --- a/internal/ingress/controller/controller.go +++ b/internal/ingress/controller/controller.go @@ -238,7 +238,11 @@ 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), ",") + } for key, value := range ing.ObjectMeta.GetAnnotations() { @@ -247,8 +251,9 @@ 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)) { 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 7500e58ca..0438ec4e2 100644 --- a/internal/ingress/controller/store/store.go +++ b/internal/ingress/controller/store/store.go @@ -655,10 +655,11 @@ func (s *k8sStore) syncIngress(ing *networkingv1beta1.Ingress) { copyIng := &networkingv1beta1.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.Warningf("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 d2bb145ea..c205703c3 100644 --- a/test/e2e/admission/admission.go +++ b/test/e2e/admission/admission.go @@ -146,6 +146,34 @@ var _ = framework.IngressNginxDescribe("[Serial] admission controller", func() { } }) + ginkgo.It("should return an error if there is an invalid value in some annotation", func() { + host := "admission-test" + + 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.NetworkingV1beta1().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") + }) + + ginkgo.It("should return an error if there is a forbidden value in some annotation", func() { + host := "admission-test" + + 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.NetworkingV1beta1().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") + }) + ginkgo.It("should not return an error if the Ingress V1 definition is valid", func() { if !f.IsIngressV1Ready { ginkgo.Skip("Test requires Kubernetes v1.19 or higher") @@ -189,17 +217,6 @@ var _ = framework.IngressNginxDescribe("[Serial] admission controller", func() { _, err := f.KubeClientSet.NetworkingV1beta1().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") }) - - ginkgo.It("should return an error if there is a forbidden value in some annotation", func() { - host := "admission-test" - - annotations := map[string]string{ - "nginx.ingress.kubernetes.io/connection-proxy-header": "set_by_lua", - } - firstIngress := framework.NewSingleIngress("first-ingress", "/", host, f.Namespace, framework.EchoService, 80, annotations) - _, err := f.KubeClientSet.NetworkingV1beta1().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") - }) }) func uninstallChart(f *framework.Framework) error { diff --git a/test/e2e/framework/util.go b/test/e2e/framework/util.go index 3a15a2415..62603462f 100644 --- a/test/e2e/framework/util.go +++ b/test/e2e/framework/util.go @@ -38,7 +38,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) + + }) })