Legacy cherrypick (#7925)

* fix: fix thread synchronization issue #6245 (#7800) * Add option to sanitize annotation inputs (#7874) * Add option to sanitize annotation inputs * Fix e2e tests after string sanitization * Add proxy_pass and serviceaccount as denied values * Trim spaces from badword items (#7921) * Fix tests from cherrypick Co-authored-by: Jens Reimann <[email protected]>
kubernetes · Nov 16, 2021 · b159577 · b159577
1 parent 3673519
commit b159577
Show file tree

Hide file tree

Showing 13 changed files with 314 additions and 17 deletions.
diff --git a/docs/user-guide/nginx-configuration/configmap.md b/docs/user-guide/nginx-configuration/configmap.md
@@ -30,6 +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","{","}","'","\"
 |[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"|
@@ -217,6 +218,23 @@ Enables Ingress to parse and add *-snippet annotations/directives created by the
 Warning: We recommend enabling this option only if you TRUST users with permission to create Ingress objects, as this 
 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.
+
+When an annotation is detected with a value that matches one of the blocked badwords, the whole Ingress wont be configured.
+
+_**default:**_ `"load_module,lua_package,_by_lua,location,root,proxy_pass,serviceaccount,{,},',\"`
+
+
+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.
+
+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/annotations/parser/main_test.go b/internal/ingress/annotations/parser/main_test.go
@@ -116,6 +116,12 @@ rewrite (?i)/arcgis/services/Utilities/Geometry/GeometryServer(.*)$ /arcgis/serv
 			}
 			continue
 		}
+		if !test.expErr {
+			if err != nil {
+				t.Errorf("%v: didn't expected error but error was returned: %v", test.name, err)
+			}
+			continue
+		}
 		if s != test.exp {
 			t.Errorf("%v: expected \"%v\" but \"%v\" was returned", test.name, test.exp, s)
 		}

diff --git a/internal/ingress/controller/config/config.go b/internal/ingress/controller/config/config.go
@@ -18,6 +18,7 @@ package config
 
 import (
 	"strconv"
+	"strings"
 	"time"
 
 	"k8s.io/klog/v2"
@@ -97,6 +98,11 @@ type Configuration struct {
 	// If disabled, only snippets added via ConfigMap are added to ingress.
 	AllowSnippetAnnotations bool `json:"allow-snippet-annotations"`
 
+	// AnnotationValueWordBlocklist defines words that should not be part of an user annotation value
+	// (can be used to run arbitrary code or configs, for example) and that should be dropped.
+	// This list should be separated by "," character
+	AnnotationValueWordBlocklist string `json:"annotation-value-word-blocklist"`
+
 	// Sets the name of the configmap that contains the headers to pass to the client
 	AddHeaders string `json:"add-headers,omitempty"`
 
@@ -754,6 +760,20 @@ func NewDefault() Configuration {
 	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")
@@ -764,6 +784,7 @@ func NewDefault() Configuration {
 
 		AllowSnippetAnnotations:          true,
 		AllowBackendServerHeader:         false,
+		AnnotationValueWordBlocklist:     strings.Join(defAnnotationValueWordBlocklist, ","),
 		AccessLogPath:                    "/var/log/nginx/access.log",
 		AccessLogParams:                  "",
 		EnableAccessLogForDefaultBackend: false,

diff --git a/internal/ingress/controller/controller.go b/internal/ingress/controller/controller.go
@@ -238,12 +238,22 @@ func (n *NGINXController) CheckIngress(ing *networking.Ingress) error {
 	cfg := n.store.GetBackendConfiguration()
 	cfg.Resolver = n.resolver
 
-	for key := range ing.ObjectMeta.GetAnnotations() {
+	arraybadWords := strings.Split(strings.TrimSpace(cfg.AnnotationValueWordBlocklist), ",")
+
+	for key, value := range ing.ObjectMeta.GetAnnotations() {
+
 		if parser.AnnotationsPrefix != parser.DefaultAnnotationsPrefix {
 			if strings.HasPrefix(key, fmt.Sprintf("%s/", parser.DefaultAnnotationsPrefix)) {
 				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.Contains(value, strings.TrimSpace(forbiddenvalue)) {
+					return fmt.Errorf("%s annotation contains invalid word %s", key, forbiddenvalue)
+				}
+			}
+		}
 
 		if !cfg.AllowSnippetAnnotations && strings.HasSuffix(key, "-snippet") {
 			return fmt.Errorf("%s annotation cannot be used. Snippet directives are disabled by the Ingress administrator", key)

diff --git a/internal/ingress/controller/controller_test.go b/internal/ingress/controller/controller_test.go
@@ -279,6 +279,27 @@ func TestCheckIngress(t *testing.T) {
 			}
 		})
 
+		t.Run("When invalid directives are used in annotation values", func(t *testing.T) {
+			nginx.store = fakeIngressStore{
+				ingresses: []*ingress.Ingress{},
+				configuration: ngx_config.Configuration{
+					AnnotationValueWordBlocklist: "invalid_directive, another_directive",
+				},
+			}
+			nginx.command = testNginxTestCommand{
+				t:   t,
+				err: nil,
+			}
+			ing.ObjectMeta.Annotations["nginx.ingress.kubernetes.io/custom-headers"] = "invalid_directive"
+			if err := nginx.CheckIngress(ing); err == nil {
+				t.Errorf("with an invalid value in annotation the ingress should be rejected")
+			}
+			ing.ObjectMeta.Annotations["nginx.ingress.kubernetes.io/custom-headers"] = "another_directive"
+			if err := nginx.CheckIngress(ing); err == nil {
+				t.Errorf("with an invalid value in annotation the ingress should be rejected")
+			}
+		})
+
 		t.Run("When a new catch-all ingress is being created despite catch-alls being disabled ", func(t *testing.T) {
 			backendBefore := ing.Spec.Backend
 			disableCatchAllBefore := nginx.cfg.DisableCatchAll

diff --git a/internal/ingress/controller/store/store.go b/internal/ingress/controller/store/store.go
@@ -23,6 +23,7 @@ import (
 	"os"
 	"reflect"
 	"sort"
+	"strings"
 	"sync"
 	"time"
 
@@ -630,6 +631,21 @@ func hasCatchAllIngressRule(spec networkingv1beta1.IngressSpec) bool {
 	return spec.Backend != nil
 }
 
+func checkBadAnnotationValue(annotations map[string]string, badwords string) error {
+	arraybadWords := strings.Split(strings.TrimSpace(badwords), ",")
+
+	for annotation, value := range annotations {
+		if strings.HasPrefix(annotation, fmt.Sprintf("%s/", parser.AnnotationsPrefix)) {
+			for _, forbiddenvalue := range arraybadWords {
+				if strings.Contains(value, forbiddenvalue) {
+					return fmt.Errorf("%s annotation contains invalid word %s", annotation, forbiddenvalue)
+				}
+			}
+		}
+	}
+	return nil
+}
+
 // syncIngress parses ingress annotations converting the value of the
 // annotation to a go struct
 func (s *k8sStore) syncIngress(ing *networkingv1beta1.Ingress) {
@@ -638,6 +654,13 @@ 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
+	}
+
 	ing.Spec.DeepCopyInto(&copyIng.Spec)
 	ing.Status.DeepCopyInto(&copyIng.Status)
 

diff --git a/internal/ingress/controller/template/template.go b/internal/ingress/controller/template/template.go
@@ -62,6 +62,9 @@ const (
 
 // Writer is the interface to render a template
 type Writer interface {
+	// Write renders the template.
+	// NOTE: Implementors must ensure that the content of the returned slice is not modified by the implementation
+	// after the return of this function.
 	Write(conf config.TemplateConfig) ([]byte, error)
 }
 
@@ -201,7 +204,12 @@ func (t *Template) Write(conf config.TemplateConfig) ([]byte, error) {
 		return nil, err
 	}
 
-	return outCmdBuf.Bytes(), nil
+	// make a copy to ensure that we are no longer modifying the content of the buffer
+	out := outCmdBuf.Bytes()
+	res := make([]byte, len(out))
+	copy(res, out)
+
+	return res, nil
 }
 
 var (

diff --git a/test/e2e/admission/admission.go b/test/e2e/admission/admission.go
@@ -178,6 +178,28 @@ var _ = framework.IngressNginxDescribe("[Serial] admission controller", func() {
 			assert.NotNil(ginkgo.GinkgoT(), err, "creating an ingress with invalid configuration should return an error")
 		}
 	})
+
+	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;}",
+		}
+		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",
+		}
+		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/annotations/globalratelimit.go b/test/e2e/annotations/globalratelimit.go
@@ -40,6 +40,11 @@ var _ = framework.DescribeAnnotation("annotation-global-rate-limit", func() {
 		annotations["nginx.ingress.kubernetes.io/global-rate-limit"] = "5"
 		annotations["nginx.ingress.kubernetes.io/global-rate-limit-window"] = "2m"
 
+		// We need to allow { and } characters for this annotation to work
+		f.UpdateNginxConfigMapData("annotation-value-word-blocklist", "load_module, lua_package, _by_lua, location, root")
+		// Sleep a while just to guarantee that the configmap is applied
+		framework.Sleep()
+
 		ing := framework.NewSingleIngress(host, "/", host, f.Namespace, framework.EchoService, 80, annotations)
 		ing = f.EnsureIngress(ing)
 		namespace := strings.Replace(string(ing.UID), "-", "", -1)

diff --git a/test/e2e/annotations/modsecurity.go b/test/e2e/annotations/modsecurity.go
@@ -165,7 +165,9 @@ var _ = framework.DescribeAnnotation("modsecurity owasp", func() {
 			"nginx.ingress.kubernetes.io/enable-modsecurity":  "true",
 			"nginx.ingress.kubernetes.io/modsecurity-snippet": snippet,
 		}
-
+		f.UpdateNginxConfigMapData("annotation-value-word-blocklist", "load_module, lua_package, _by_lua, location, root, {, }")
+		// Sleep a while just to guarantee that the configmap is applied
+		framework.Sleep()
 		ing := framework.NewSingleIngress(host, "/", host, nameSpace, framework.EchoService, 80, annotations)
 		f.EnsureIngress(ing)
 
@@ -198,7 +200,9 @@ var _ = framework.DescribeAnnotation("modsecurity owasp", func() {
 		annotations := map[string]string{
 			"nginx.ingress.kubernetes.io/modsecurity-snippet": snippet,
 		}
-
+		f.UpdateNginxConfigMapData("annotation-value-word-blocklist", "load_module, lua_package, _by_lua, location, root, {, }")
+		// Sleep a while just to guarantee that the configmap is applied
+		framework.Sleep()
 		ing := framework.NewSingleIngress(host, "/", host, nameSpace, framework.EchoService, 80, annotations)
 		f.EnsureIngress(ing)
 
@@ -232,7 +236,9 @@ var _ = framework.DescribeAnnotation("modsecurity owasp", func() {
 		annotations := map[string]string{
 			"nginx.ingress.kubernetes.io/modsecurity-snippet": snippet,
 		}
-
+		f.UpdateNginxConfigMapData("annotation-value-word-blocklist", "load_module, lua_package, _by_lua, location, root, {, }")
+		// Sleep a while just to guarantee that the configmap is applied
+		framework.Sleep()
 		ing := framework.NewSingleIngress(host, "/", host, nameSpace, framework.EchoService, 80, annotations)
 		f.EnsureIngress(ing)
 
@@ -268,7 +274,9 @@ var _ = framework.DescribeAnnotation("modsecurity owasp", func() {
 		annotations := map[string]string{
 			"nginx.ingress.kubernetes.io/modsecurity-snippet": snippet,
 		}
-
+		f.UpdateNginxConfigMapData("annotation-value-word-blocklist", "load_module, lua_package, _by_lua, location, root, {, }")
+		// Sleep a while just to guarantee that the configmap is applied
+		framework.Sleep()
 		ing := framework.NewSingleIngress(host, "/", host, nameSpace, framework.EchoService, 80, annotations)
 		f.EnsureIngress(ing)
 
@@ -307,7 +315,9 @@ var _ = framework.DescribeAnnotation("modsecurity owasp", func() {
 		annotations := map[string]string{
 			"nginx.ingress.kubernetes.io/modsecurity-snippet": snippet,
 		}
-
+		f.UpdateNginxConfigMapData("annotation-value-word-blocklist", "load_module, lua_package, _by_lua, location, root, {, }")
+		// Sleep a while just to guarantee that the configmap is applied
+		framework.Sleep()
 		ing := framework.NewSingleIngress(host, "/", host, nameSpace, framework.EchoService, 80, annotations)
 		f.EnsureIngress(ing)
 

diff --git 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 = 5 * time.Minute
+	DefaultTimeout = 30 * time.Second
 )
 
 func nowStamp() string {

diff --git a/test/e2e/ingress/pathtype_mixed.go b/test/e2e/ingress/pathtype_mixed.go
@@ -44,14 +44,14 @@ var _ = framework.IngressNginxDescribe("[Ingress] [PathType] mix Exact and Prefi
 		host := "mixed.path"
 
 		annotations := map[string]string{
-			"nginx.ingress.kubernetes.io/configuration-snippet": `more_set_input_headers "pathType: exact";more_set_input_headers "pathlocation: /";`,
+			"nginx.ingress.kubernetes.io/configuration-snippet": `more_set_input_headers "pathType: exact";more_set_input_headers "pathheader: /";`,
 		}
 		ing := framework.NewSingleIngress("exact-root", "/", host, f.Namespace, framework.EchoService, 80, annotations)
 		ing.Spec.Rules[0].IngressRuleValue.HTTP.Paths[0].PathType = &exactPathType
 		f.EnsureIngress(ing)
 
 		annotations = map[string]string{
-			"nginx.ingress.kubernetes.io/configuration-snippet": `more_set_input_headers "pathType: prefix";more_set_input_headers "pathlocation: /";`,
+			"nginx.ingress.kubernetes.io/configuration-snippet": `more_set_input_headers "pathType: prefix";more_set_input_headers "pathheader: /";`,
 		}
 		ing = framework.NewSingleIngress("prefix-root", "/", host, f.Namespace, framework.EchoService, 80, annotations)
 		f.EnsureIngress(ing)
@@ -74,7 +74,7 @@ var _ = framework.IngressNginxDescribe("[Ingress] [PathType] mix Exact and Prefi
 
 		assert.NotContains(ginkgo.GinkgoT(), body, "pathtype=prefix")
 		assert.Contains(ginkgo.GinkgoT(), body, "pathtype=exact")
-		assert.Contains(ginkgo.GinkgoT(), body, "pathlocation=/")
+		assert.Contains(ginkgo.GinkgoT(), body, "pathheader=/")
 
 		ginkgo.By("Checking prefix request to /bar")
 		body = f.HTTPTestClient().
@@ -87,17 +87,17 @@ var _ = framework.IngressNginxDescribe("[Ingress] [PathType] mix Exact and Prefi
 
 		assert.Contains(ginkgo.GinkgoT(), body, "pathtype=prefix")
 		assert.NotContains(ginkgo.GinkgoT(), body, "pathtype=exact")
-		assert.Contains(ginkgo.GinkgoT(), body, "pathlocation=/")
+		assert.Contains(ginkgo.GinkgoT(), body, "pathheader=/")
 
 		annotations = map[string]string{
-			"nginx.ingress.kubernetes.io/configuration-snippet": `more_set_input_headers "pathType: exact";more_set_input_headers "pathlocation: /foo";`,
+			"nginx.ingress.kubernetes.io/configuration-snippet": `more_set_input_headers "pathType: exact";more_set_input_headers "pathheader: /foo";`,
 		}
 		ing = framework.NewSingleIngress("exact-foo", "/foo", host, f.Namespace, framework.EchoService, 80, annotations)
 		ing.Spec.Rules[0].IngressRuleValue.HTTP.Paths[0].PathType = &exactPathType
 		f.EnsureIngress(ing)
 
 		annotations = map[string]string{
-			"nginx.ingress.kubernetes.io/configuration-snippet": `more_set_input_headers "pathType: prefix";more_set_input_headers "pathlocation: /foo";`,
+			"nginx.ingress.kubernetes.io/configuration-snippet": `more_set_input_headers "pathType: prefix";more_set_input_headers "pathheader: /foo";`,
 		}
 		ing = framework.NewSingleIngress("prefix-foo", "/foo", host, f.Namespace, framework.EchoService, 80, annotations)
 		f.EnsureIngress(ing)
@@ -120,7 +120,7 @@ var _ = framework.IngressNginxDescribe("[Ingress] [PathType] mix Exact and Prefi
 
 		assert.NotContains(ginkgo.GinkgoT(), body, "pathtype=prefix")
 		assert.Contains(ginkgo.GinkgoT(), body, "pathtype=exact")
-		assert.Contains(ginkgo.GinkgoT(), body, "pathlocation=/foo")
+		assert.Contains(ginkgo.GinkgoT(), body, "pathheader=/foo")
 
 		ginkgo.By("Checking prefix request to /foo/bar")
 		body = f.HTTPTestClient().
@@ -132,7 +132,7 @@ var _ = framework.IngressNginxDescribe("[Ingress] [PathType] mix Exact and Prefi
 			Raw()
 
 		assert.Contains(ginkgo.GinkgoT(), body, "pathtype=prefix")
-		assert.Contains(ginkgo.GinkgoT(), body, "pathlocation=/foo")
+		assert.Contains(ginkgo.GinkgoT(), body, "pathheader=/foo")
 
 		ginkgo.By("Checking prefix request to /foobar")
 		body = f.HTTPTestClient().
@@ -144,6 +144,6 @@ var _ = framework.IngressNginxDescribe("[Ingress] [PathType] mix Exact and Prefi
 			Raw()
 
 		assert.Contains(ginkgo.GinkgoT(), body, "pathtype=prefix")
-		assert.Contains(ginkgo.GinkgoT(), body, "pathlocation=/")
+		assert.Contains(ginkgo.GinkgoT(), body, "pathheader=/")
 	})
 })