update default block list,docs, tests (kubernetes#7942)

* update default block list,docs, tests

* fix config for admin test

* gofmt

* remove the err return

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.

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,

internal/ingress/controller/controller.go

@@ -238,7 +238,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() {
 
@@ -247,9 +252,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)
 				}
 			}

internal/ingress/controller/store/store.go

@@ -655,10 +655,11 @@ func (s *k8sStore) syncIngress(ing *networkingv1beta1.Ingress) {
 	copyIng := &networkingv1beta1.Ingress{}
 	ing.ObjectMeta.DeepCopyInto(&copyIng.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(&copyIng.Spec)

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")

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 {

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,46 +99,59 @@ 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{
 			"nginx.ingress.kubernetes.io/configuration-snippet": `
 			# 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,
+		f.WaitForNginxServer(hostValid,
 			func(server string) bool {
-				return !strings.Contains(server, fmt.Sprintf("server_name %s ;", host))
+				return strings.Contains(server, fmt.Sprintf("server_name %s ;", hostValid))
 			})
 
 		f.WaitForNginxServer(hostValid,
 			func(server string) bool {
-				return strings.Contains(server, fmt.Sprintf("server_name %s ;", hostValid))
+				return strings.Contains(server, "# bla_by_lua")
 			})
 
+		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, "# something_forbidden")
+				return !strings.Contains(server, fmt.Sprintf("server_name %s ;", host))
 			})
 
-		f.WaitForNginxServer(hostValid,
+		f.WaitForNginxServer(host,
 			func(server string) bool {
-				return strings.Contains(server, "# bla_by_lua")
+				return !strings.Contains(server, "# something_forbidden")
 			})
 
 		f.HTTPTestClient().
@@ -144,10 +160,5 @@ var _ = framework.DescribeAnnotation("Bad annotation values", func() {
 			Expect().
 			Status(http.StatusNotFound)
 
-		f.HTTPTestClient().
-			GET("/").
-			WithHeader("Host", hostValid).
-			Expect().
-			Status(http.StatusOK)
 	})
 })