From f490a4a53b4666a27d997fc00175f663981b863e Mon Sep 17 00:00:00 2001 From: Ricardo Pchevuzinske Katz Date: Sat, 23 Oct 2021 12:07:16 -0300 Subject: [PATCH 1/3] Add option to sanitize annotation inputs --- .../ingress/annotations/parser/main_test.go | 6 + internal/ingress/controller/config/config.go | 13 ++ internal/ingress/controller/controller.go | 15 +- .../ingress/controller/controller_test.go | 34 +++++ internal/ingress/controller/store/store.go | 25 ++++ test/e2e/admission/admission.go | 22 +++ test/e2e/framework/util.go | 2 +- test/e2e/settings/badannotationvalues.go | 129 ++++++++++++++++++ 8 files changed, 244 insertions(+), 2 deletions(-) create mode 100644 test/e2e/settings/badannotationvalues.go diff --git a/internal/ingress/annotations/parser/main_test.go b/internal/ingress/annotations/parser/main_test.go index f63560ca34..7b01a12301 100644 --- 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 index a29c1b094f..7bb5467c92 100644 --- a/internal/ingress/controller/config/config.go +++ b/internal/ingress/controller/config/config.go @@ -97,6 +97,14 @@ type Configuration struct { // If disabled, only snippets added via ConfigMap are added to ingress. AllowSnippetAnnotations bool `json:"allow-snippet-annotations"` + // AnnotationValueCharBlocklist defines characters that should not be part of an user annotation value + // (can be used to escape strings, for example) and that should be dropped + AnnotationValueCharBlocklist []string `json:"annotation-value-char-blocklist"` + + // 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 + 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 +762,9 @@ func NewDefault() Configuration { defNginxStatusIpv6Whitelist := make([]string, 0) defResponseHeaders := make([]string, 0) + defAnnotationValueWordBlocklist := []string{"load_module", "lua_package", "_by_lua", "location", "root"} + defAnnotationValueCharBlocklist := []string{"{", "}", "'", "\"", "\\"} + defIPCIDR = append(defIPCIDR, "0.0.0.0/0") defNginxStatusIpv4Whitelist = append(defNginxStatusIpv4Whitelist, "127.0.0.1") defNginxStatusIpv6Whitelist = append(defNginxStatusIpv6Whitelist, "::1") @@ -764,6 +775,8 @@ func NewDefault() Configuration { AllowSnippetAnnotations: true, AllowBackendServerHeader: false, + AnnotationValueWordBlocklist: defAnnotationValueWordBlocklist, + AnnotationValueCharBlocklist: defAnnotationValueCharBlocklist, AccessLogPath: "/var/log/nginx/access.log", AccessLogParams: "", EnableAccessLogForDefaultBackend: false, diff --git a/internal/ingress/controller/controller.go b/internal/ingress/controller/controller.go index f935f5fff3..eae77a3ddc 100644 --- a/internal/ingress/controller/controller.go +++ b/internal/ingress/controller/controller.go @@ -237,12 +237,25 @@ func (n *NGINXController) CheckIngress(ing *networking.Ingress) error { cfg := n.store.GetBackendConfiguration() cfg.Resolver = n.resolver - for key := range ing.ObjectMeta.GetAnnotations() { + 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 cfg.AnnotationValueWordBlocklist { + if strings.Contains(value, forbiddenvalue) { + return fmt.Errorf("%s annotation contains invalid word %s", key, forbiddenvalue) + } + } + for _, invalidchar := range cfg.AnnotationValueCharBlocklist { + if strings.ContainsAny(value, invalidchar) { + return fmt.Errorf("%s annotation contains invalid character %s", key, invalidchar) + } + } + } 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 index d7020bb48d..fb5efe7d6a 100644 --- a/internal/ingress/controller/controller_test.go +++ b/internal/ingress/controller/controller_test.go @@ -268,6 +268,40 @@ 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: []string{"invalid_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") + } + }) + + t.Run("When invalid chars are used in annotation values", func(t *testing.T) { + nginx.store = fakeIngressStore{ + ingresses: []*ingress.Ingress{}, + configuration: ngx_config.Configuration{ + AnnotationValueCharBlocklist: []string{"{", "}"}, + }, + } + nginx.command = testNginxTestCommand{ + t: t, + err: nil, + } + ing.ObjectMeta.Annotations["nginx.ingress.kubernetes.io/custom-headers"] = "{differentheader" + if err := nginx.CheckIngress(ing); err == nil { + t.Errorf("with an invalid chars 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.DefaultBackend disableCatchAllBefore := nginx.cfg.DisableCatchAll diff --git a/internal/ingress/controller/store/store.go b/internal/ingress/controller/store/store.go index 6852f63b29..c4e6d8640f 100644 --- 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" @@ -734,6 +735,24 @@ func hasCatchAllIngressRule(spec networkingv1.IngressSpec) bool { return spec.DefaultBackend != nil } +func checkBadAnnotationValue(annotations map[string]string, badwords, badchars []string) error { + for annotation, value := range annotations { + if strings.HasPrefix(annotation, fmt.Sprintf("%s/", parser.AnnotationsPrefix)) { + for _, forbiddenvalue := range badwords { + if strings.Contains(value, forbiddenvalue) { + return fmt.Errorf("%s annotation contains invalid word %s", annotation, forbiddenvalue) + } + } + for _, invalidchar := range badchars { + if strings.ContainsAny(value, invalidchar) { + return fmt.Errorf("%s annotation contains invalid character %s", annotation, invalidchar) + } + } + } + } + return nil +} + // syncIngress parses ingress annotations converting the value of the // annotation to a go struct func (s *k8sStore) syncIngress(ing *networkingv1.Ingress) { @@ -742,6 +761,12 @@ func (s *k8sStore) syncIngress(ing *networkingv1.Ingress) { copyIng := &networkingv1.Ingress{} ing.ObjectMeta.DeepCopyInto(©Ing.ObjectMeta) + + if err := checkBadAnnotationValue(copyIng.Annotations, s.backendConfig.AnnotationValueWordBlocklist, s.backendConfig.AnnotationValueCharBlocklist); err != nil { + klog.Error("skipping ingress %v: %s", err) + return + } + ing.Spec.DeepCopyInto(©Ing.Spec) ing.Status.DeepCopyInto(©Ing.Status) diff --git a/test/e2e/admission/admission.go b/test/e2e/admission/admission.go index 121d0d37aa..8b85f35d1f 100644 --- a/test/e2e/admission/admission.go +++ b/test/e2e/admission/admission.go @@ -121,6 +121,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.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") + }) + + 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.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") + }) + ginkgo.It("should not return an error if the Ingress V1 definition is valid with Ingress Class", func() { err := createIngress(f.Namespace, validV1Ingress) assert.Nil(ginkgo.GinkgoT(), err, "creating an ingress using kubectl") diff --git a/test/e2e/framework/util.go b/test/e2e/framework/util.go index 75fcb58ea4..753e31bfce 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 = 5 * time.Minute + DefaultTimeout = 30 * time.Second ) func nowStamp() string { diff --git a/test/e2e/settings/badannotationvalues.go b/test/e2e/settings/badannotationvalues.go new file mode 100644 index 0000000000..779cdb39dd --- /dev/null +++ b/test/e2e/settings/badannotationvalues.go @@ -0,0 +1,129 @@ +/* +Copyright 2021 The Kubernetes Authors. + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +package settings + +import ( + "fmt" + "net/http" + "strings" + + "github.com/onsi/ginkgo" + + "k8s.io/ingress-nginx/test/e2e/framework" +) + +var _ = framework.DescribeAnnotation("Bad annotation values", func() { + f := framework.NewDefaultFramework("bad-annotation") + + ginkgo.BeforeEach(func() { + f.NewEchoDeployment() + }) + + ginkgo.It("should drop an ingress if there is an invalid character in some annotation", func() { + host := "invalid-value-test" + + annotations := map[string]string{ + "nginx.ingress.kubernetes.io/configuration-snippet": ` + # abc { }`, + } + + ing := framework.NewSingleIngress(host, "/", host, f.Namespace, framework.EchoService, 80, annotations) + f.UpdateNginxConfigMapData("allow-snippet-annotations", "true") + 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, "# abc { }") + }) + + f.HTTPTestClient(). + GET("/"). + WithHeader("Host", host). + Expect(). + Status(http.StatusNotFound) + }) + + ginkgo.It("should drop an ingress if there is a forbidden word in some annotation", func() { + host := "forbidden-value-test" + + annotations := map[string]string{ + "nginx.ingress.kubernetes.io/configuration-snippet": ` + default_type text/plain; + content_by_lua_block { + ngx.say("Hello World") + }`, + } + + ing := framework.NewSingleIngress(host, "/", host, f.Namespace, framework.EchoService, 80, annotations) + f.UpdateNginxConfigMapData("allow-snippet-annotations", "true") + // 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, `ngx.say("Hello World")`) + }) + + f.HTTPTestClient(). + GET("/"). + WithHeader("Host", host). + Expect(). + Status(http.StatusNotFound) + }) + + ginkgo.It("should drop an ingress if there is a custom blocklisted word in some annotation", 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) + }) +}) From c5dd4f1345d5a65246f808315e8b085bfb4b53af Mon Sep 17 00:00:00 2001 From: Ricardo Pchevuzinske Katz Date: Tue, 2 Nov 2021 22:41:19 -0300 Subject: [PATCH 2/3] Fix e2e tests after string sanitization --- internal/ingress/controller/config/config.go | 26 ++++++++++++------- internal/ingress/controller/controller.go | 9 +++---- .../ingress/controller/controller_test.go | 19 +------------- internal/ingress/controller/store/store.go | 16 +++++------- test/e2e/annotations/globalratelimit.go | 5 ++++ .../annotations/modsecurity/modsecurity.go | 20 ++++++++++---- test/e2e/ingress/pathtype_mixed.go | 18 ++++++------- test/e2e/settings/badannotationvalues.go | 26 ++++++++++++++++++- 8 files changed, 81 insertions(+), 58 deletions(-) diff --git a/internal/ingress/controller/config/config.go b/internal/ingress/controller/config/config.go index 7bb5467c92..ee90d174e9 100644 --- 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,13 +98,10 @@ type Configuration struct { // If disabled, only snippets added via ConfigMap are added to ingress. AllowSnippetAnnotations bool `json:"allow-snippet-annotations"` - // AnnotationValueCharBlocklist defines characters that should not be part of an user annotation value - // (can be used to escape strings, for example) and that should be dropped - AnnotationValueCharBlocklist []string `json:"annotation-value-char-blocklist"` - // 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 - AnnotationValueWordBlocklist []string `json:"annotation-value-word-blocklist"` + // (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"` @@ -762,8 +760,17 @@ func NewDefault() Configuration { defNginxStatusIpv6Whitelist := make([]string, 0) defResponseHeaders := make([]string, 0) - defAnnotationValueWordBlocklist := []string{"load_module", "lua_package", "_by_lua", "location", "root"} - defAnnotationValueCharBlocklist := []string{"{", "}", "'", "\"", "\\"} + defAnnotationValueWordBlocklist := []string{ + "load_module", + "lua_package", + "_by_lua", + "location", + "root", + "{", + "}", + "'", + "\\", + } defIPCIDR = append(defIPCIDR, "0.0.0.0/0") defNginxStatusIpv4Whitelist = append(defNginxStatusIpv4Whitelist, "127.0.0.1") @@ -775,8 +782,7 @@ func NewDefault() Configuration { AllowSnippetAnnotations: true, AllowBackendServerHeader: false, - AnnotationValueWordBlocklist: defAnnotationValueWordBlocklist, - AnnotationValueCharBlocklist: defAnnotationValueCharBlocklist, + 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 index eae77a3ddc..5db56d300d 100644 --- a/internal/ingress/controller/controller.go +++ b/internal/ingress/controller/controller.go @@ -237,6 +237,8 @@ func (n *NGINXController) CheckIngress(ing *networking.Ingress) error { cfg := n.store.GetBackendConfiguration() cfg.Resolver = n.resolver + arraybadWords := strings.Split(strings.TrimSpace(cfg.AnnotationValueWordBlocklist), ",") + for key, value := range ing.ObjectMeta.GetAnnotations() { if parser.AnnotationsPrefix != parser.DefaultAnnotationsPrefix { @@ -245,16 +247,11 @@ func (n *NGINXController) CheckIngress(ing *networking.Ingress) error { } } if strings.HasPrefix(key, fmt.Sprintf("%s/", parser.AnnotationsPrefix)) { - for _, forbiddenvalue := range cfg.AnnotationValueWordBlocklist { + for _, forbiddenvalue := range arraybadWords { if strings.Contains(value, forbiddenvalue) { return fmt.Errorf("%s annotation contains invalid word %s", key, forbiddenvalue) } } - for _, invalidchar := range cfg.AnnotationValueCharBlocklist { - if strings.ContainsAny(value, invalidchar) { - return fmt.Errorf("%s annotation contains invalid character %s", key, invalidchar) - } - } } if !cfg.AllowSnippetAnnotations && strings.HasSuffix(key, "-snippet") { diff --git a/internal/ingress/controller/controller_test.go b/internal/ingress/controller/controller_test.go index fb5efe7d6a..f9d60974fe 100644 --- a/internal/ingress/controller/controller_test.go +++ b/internal/ingress/controller/controller_test.go @@ -272,7 +272,7 @@ func TestCheckIngress(t *testing.T) { nginx.store = fakeIngressStore{ ingresses: []*ingress.Ingress{}, configuration: ngx_config.Configuration{ - AnnotationValueWordBlocklist: []string{"invalid_directive"}, + AnnotationValueWordBlocklist: "invalid_directive, another_directive", }, } nginx.command = testNginxTestCommand{ @@ -285,23 +285,6 @@ func TestCheckIngress(t *testing.T) { } }) - t.Run("When invalid chars are used in annotation values", func(t *testing.T) { - nginx.store = fakeIngressStore{ - ingresses: []*ingress.Ingress{}, - configuration: ngx_config.Configuration{ - AnnotationValueCharBlocklist: []string{"{", "}"}, - }, - } - nginx.command = testNginxTestCommand{ - t: t, - err: nil, - } - ing.ObjectMeta.Annotations["nginx.ingress.kubernetes.io/custom-headers"] = "{differentheader" - if err := nginx.CheckIngress(ing); err == nil { - t.Errorf("with an invalid chars 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.DefaultBackend disableCatchAllBefore := nginx.cfg.DisableCatchAll diff --git a/internal/ingress/controller/store/store.go b/internal/ingress/controller/store/store.go index c4e6d8640f..a914435498 100644 --- a/internal/ingress/controller/store/store.go +++ b/internal/ingress/controller/store/store.go @@ -735,19 +735,16 @@ func hasCatchAllIngressRule(spec networkingv1.IngressSpec) bool { return spec.DefaultBackend != nil } -func checkBadAnnotationValue(annotations map[string]string, badwords, badchars []string) error { +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 badwords { + for _, forbiddenvalue := range arraybadWords { if strings.Contains(value, forbiddenvalue) { return fmt.Errorf("%s annotation contains invalid word %s", annotation, forbiddenvalue) } } - for _, invalidchar := range badchars { - if strings.ContainsAny(value, invalidchar) { - return fmt.Errorf("%s annotation contains invalid character %s", annotation, invalidchar) - } - } } } return nil @@ -762,8 +759,9 @@ func (s *k8sStore) syncIngress(ing *networkingv1.Ingress) { copyIng := &networkingv1.Ingress{} ing.ObjectMeta.DeepCopyInto(©Ing.ObjectMeta) - if err := checkBadAnnotationValue(copyIng.Annotations, s.backendConfig.AnnotationValueWordBlocklist, s.backendConfig.AnnotationValueCharBlocklist); err != nil { - klog.Error("skipping ingress %v: %s", err) + 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 } diff --git a/test/e2e/annotations/globalratelimit.go b/test/e2e/annotations/globalratelimit.go index dd985c68c2..ca93028926 100644 --- 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/modsecurity.go b/test/e2e/annotations/modsecurity/modsecurity.go index cfd6286e45..f88d6541e2 100644 --- a/test/e2e/annotations/modsecurity/modsecurity.go +++ b/test/e2e/annotations/modsecurity/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/ingress/pathtype_mixed.go b/test/e2e/ingress/pathtype_mixed.go index 28e3049c92..cf2172bd16 100644 --- a/test/e2e/ingress/pathtype_mixed.go +++ b/test/e2e/ingress/pathtype_mixed.go @@ -41,14 +41,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) @@ -71,7 +71,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(). @@ -84,17 +84,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) @@ -117,7 +117,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(). @@ -129,7 +129,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(). @@ -141,6 +141,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=/") }) }) diff --git a/test/e2e/settings/badannotationvalues.go b/test/e2e/settings/badannotationvalues.go index 779cdb39dd..74ce1c21e2 100644 --- a/test/e2e/settings/badannotationvalues.go +++ b/test/e2e/settings/badannotationvalues.go @@ -96,7 +96,7 @@ var _ = framework.DescribeAnnotation("Bad annotation values", func() { Status(http.StatusNotFound) }) - ginkgo.It("should drop an ingress if there is a custom blocklisted word in some annotation", func() { + 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{ @@ -104,26 +104,50 @@ var _ = framework.DescribeAnnotation("Bad annotation values", func() { # something_forbidden`, } + 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, 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) }) }) From cfa573a36631b831f9f38bff00b8cf38e44c278d Mon Sep 17 00:00:00 2001 From: Ricardo Pchevuzinske Katz Date: Tue, 2 Nov 2021 22:51:33 -0300 Subject: [PATCH 3/3] Add proxy_pass and serviceaccount as denied values --- internal/ingress/controller/config/config.go | 2 ++ 1 file changed, 2 insertions(+) diff --git a/internal/ingress/controller/config/config.go b/internal/ingress/controller/config/config.go index ee90d174e9..3c233879ae 100644 --- a/internal/ingress/controller/config/config.go +++ b/internal/ingress/controller/config/config.go @@ -766,6 +766,8 @@ func NewDefault() Configuration { "_by_lua", "location", "root", + "proxy_pass", + "serviceaccount", "{", "}", "'",