diff --git a/go.mod b/go.mod index 1aad274fb..10b9e2fda 100644 --- a/go.mod +++ b/go.mod @@ -77,7 +77,6 @@ require ( github.com/inconshreveable/mousetrap v1.0.1 // indirect github.com/josharian/intern v1.0.0 // indirect github.com/liggitt/tabwriter v0.0.0-20181228230101-89fcab3d43de // indirect - github.com/magefile/mage v1.14.0 // indirect github.com/mailru/easyjson v0.7.6 // indirect github.com/mattn/go-colorable v0.1.13 // indirect github.com/matttproud/golang_protobuf_extensions v1.0.4 // indirect diff --git a/go.sum b/go.sum index 1d99238cb..6e370c655 100644 --- a/go.sum +++ b/go.sum @@ -235,8 +235,6 @@ github.com/kylelemons/godebug v1.1.0 h1:RPNrshWIDI6G2gRW9EHilWtl7Z6Sb1BR0xunSBf0 github.com/kylelemons/godebug v1.1.0/go.mod h1:9/0rRGxNHcop5bhtWyNeEfOS8JIWk580+fNqagV/RAw= github.com/liggitt/tabwriter v0.0.0-20181228230101-89fcab3d43de h1:9TO3cAIGXtEhnIaL+V+BEER86oLrvS+kWobKpbJuye0= github.com/liggitt/tabwriter v0.0.0-20181228230101-89fcab3d43de/go.mod h1:zAbeS9B/r2mtpb6U+EI2rYA5OAXxsYw6wTamcNW+zcE= -github.com/magefile/mage v1.14.0 h1:6QDX3g6z1YvJ4olPhT1wksUcSa/V0a1B+pJb73fBjyo= -github.com/magefile/mage v1.14.0/go.mod h1:z5UZb/iS3GoOSn0JgWuiw7dxlurVYTu+/jHXqQg881A= github.com/mailru/easyjson v0.0.0-20190614124828-94de47d64c63/go.mod h1:C1wdFJiN94OJF2b5HbByQZoLdCWB1Yqtg26g4irojpc= github.com/mailru/easyjson v0.0.0-20190626092158-b2ccc519800e/go.mod h1:C1wdFJiN94OJF2b5HbByQZoLdCWB1Yqtg26g4irojpc= github.com/mailru/easyjson v0.7.6 h1:8yTIVnZgCoiM1TgqoeTl+LfU5Jg6/xL3QhGQnimLYnA= diff --git a/internal/ingress/controller/store/store.go b/internal/ingress/controller/store/store.go index 13af28137..b5731c4bf 100644 --- a/internal/ingress/controller/store/store.go +++ b/internal/ingress/controller/store/store.go @@ -59,7 +59,6 @@ import ( "k8s.io/ingress-nginx/internal/ingress/resolver" "k8s.io/ingress-nginx/internal/k8s" "k8s.io/ingress-nginx/pkg/apis/ingress" - ingressutils "k8s.io/ingress-nginx/pkg/util/ingress" ) // IngressFilterFunc decides if an Ingress should be omitted or not @@ -865,10 +864,6 @@ func (s *k8sStore) syncIngress(ing *networkingv1.Ingress) { if path.Path == "" { copyIng.Spec.Rules[ri].HTTP.Paths[pi].Path = "/" } - if !ingressutils.IsSafePath(copyIng, path.Path) { - klog.Warningf("ingress %s contains invalid path %s", key, path.Path) - return - } } } diff --git a/pkg/util/ingress/ingress.go b/pkg/util/ingress/ingress.go index 5fb3ee7b9..7df2cc114 100644 --- a/pkg/util/ingress/ingress.go +++ b/pkg/util/ingress/ingress.go @@ -18,30 +18,15 @@ package ingress import ( "fmt" - "regexp" "strings" - networkingv1 "k8s.io/api/networking/v1" "k8s.io/apimachinery/pkg/util/sets" - "k8s.io/ingress-nginx/internal/ingress/annotations/parser" "k8s.io/ingress-nginx/internal/k8s" "k8s.io/ingress-nginx/internal/net/ssl" "k8s.io/ingress-nginx/pkg/apis/ingress" "k8s.io/klog/v2" ) -const ( - alphaNumericChars = `\-\.\_\~a-zA-Z0-9/` - regexEnabledChars = `\^\$\[\]\(\)\{\}\*\+` -) - -var ( - // pathAlphaNumeric is a regex validation of something like "^/[a-zA-Z]+$" on path - pathAlphaNumeric = regexp.MustCompile("^/[" + alphaNumericChars + "]*$").MatchString - // pathRegexEnabled is a regex validation of paths that may contain regex. - pathRegexEnabled = regexp.MustCompile("^/[" + alphaNumericChars + regexEnabledChars + "]*$").MatchString -) - func GetRemovedHosts(rucfg, newcfg *ingress.Configuration) []string { oldSet := sets.NewString() newSet := sets.NewString() @@ -246,13 +231,3 @@ func BuildRedirects(servers []*ingress.Server) []*redirect { return redirectServers } - -// IsSafePath verifies if the path used in ingress object contains only valid characters. -// It will behave differently if regex is enabled or not -func IsSafePath(copyIng *networkingv1.Ingress, path string) bool { - isRegex, _ := parser.GetBoolAnnotation("use-regex", copyIng) - if isRegex { - return pathRegexEnabled(path) - } - return pathAlphaNumeric(path) -} diff --git a/pkg/util/ingress/ingress_test.go b/pkg/util/ingress/ingress_test.go index d829a57f1..24597fb6e 100644 --- a/pkg/util/ingress/ingress_test.go +++ b/pkg/util/ingress/ingress_test.go @@ -17,13 +17,8 @@ limitations under the License. package ingress import ( - "fmt" "testing" - metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" - - networkingv1 "k8s.io/api/networking/v1" - "k8s.io/ingress-nginx/internal/ingress/annotations/parser" "k8s.io/ingress-nginx/pkg/apis/ingress" ) @@ -135,83 +130,3 @@ func TestIsDynamicConfigurationEnough(t *testing.T) { t.Errorf("Expected new config to not change") } } - -func generateDumbIngressforPathTest(regexEnabled bool) *networkingv1.Ingress { - var annotations = make(map[string]string) - regexAnnotation := fmt.Sprintf("%s/use-regex", parser.AnnotationsPrefix) - if regexEnabled { - annotations[regexAnnotation] = "true" - } - return &networkingv1.Ingress{ - ObjectMeta: metav1.ObjectMeta{ - Name: "dumb", - Namespace: "default", - Annotations: annotations, - }, - } -} - -func TestIsSafePath(t *testing.T) { - tests := []struct { - name string - copyIng *networkingv1.Ingress - path string - want bool - }{ - { - name: "should accept valid path with regex disabled", - want: true, - copyIng: generateDumbIngressforPathTest(false), - path: "/xpto/~user/t-e_st.exe", - }, - { - name: "should accept valid path / with regex disabled", - want: true, - copyIng: generateDumbIngressforPathTest(false), - path: "/", - }, - { - name: "should reject invalid path with invalid chars", - want: false, - copyIng: generateDumbIngressforPathTest(false), - path: "/foo/bar/;xpto", - }, - { - name: "should reject regex path when regex is disabled", - want: false, - copyIng: generateDumbIngressforPathTest(false), - path: "/foo/bar/(.+)", - }, - { - name: "should accept valid path / with regex enabled", - want: true, - copyIng: generateDumbIngressforPathTest(true), - path: "/", - }, - { - name: "should accept regex path when regex is enabled", - want: true, - copyIng: generateDumbIngressforPathTest(true), - path: "/foo/bar/(.+)", - }, - { - name: "should reject regex path when regex is enabled but the path is invalid", - want: false, - copyIng: generateDumbIngressforPathTest(true), - path: "/foo/bar/;xpto", - }, - { - name: "should reject regex path when regex is enabled but the path is invalid", - want: false, - copyIng: generateDumbIngressforPathTest(true), - path: ";xpto", - }, - } - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - if got := IsSafePath(tt.copyIng, tt.path); got != tt.want { - t.Errorf("IsSafePath() = %v, want %v", got, tt.want) - } - }) - } -} diff --git a/test/e2e/security/invalid_paths.go b/test/e2e/security/invalid_paths.go deleted file mode 100644 index d75aefc2c..000000000 --- a/test/e2e/security/invalid_paths.go +++ /dev/null @@ -1,134 +0,0 @@ -/* -Copyright 2022 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 security - -import ( - "fmt" - "net/http" - "strings" - - "github.com/onsi/ginkgo/v2" - - "k8s.io/ingress-nginx/test/e2e/framework" -) - -const ( - validPath = "/xpto/~user/t-e_st.exe" - invalidPath = "/foo/bar/;xpto" - regexPath = "/foo/bar/(.+)" - host = "securitytest.com" -) - -var ( - annotationRegex = map[string]string{ - "nginx.ingress.kubernetes.io/use-regex": "true", - } -) - -var _ = framework.IngressNginxDescribe("[Security] validate path fields", func() { - f := framework.NewDefaultFramework("validate-path") - - ginkgo.BeforeEach(func() { - f.NewEchoDeployment() - }) - - ginkgo.It("should accept an ingress with valid path", func() { - - ing := framework.NewSingleIngress(host, validPath, host, f.Namespace, framework.EchoService, 80, nil) - - f.EnsureIngress(ing) - - f.WaitForNginxServer(host, - func(server string) bool { - return strings.Contains(server, fmt.Sprintf("server_name %s ;", host)) - }) - - f.HTTPTestClient(). - GET(validPath). - WithHeader("Host", host). - Expect(). - Status(http.StatusOK) - }) - - ginkgo.It("should drop an ingress with invalid path", func() { - - ing := framework.NewSingleIngress(host, invalidPath, host, f.Namespace, framework.EchoService, 80, nil) - f.EnsureIngress(ing) - - f.WaitForNginxServer(host, - func(server string) bool { - return !strings.Contains(server, fmt.Sprintf("server_name %s ;", host)) - }) - - f.HTTPTestClient(). - GET(invalidPath). - WithHeader("Host", host). - Expect(). - Status(http.StatusNotFound) - }) - - ginkgo.It("should drop an ingress with regex path and regex disabled", func() { - - ing := framework.NewSingleIngress(host, regexPath, host, f.Namespace, framework.EchoService, 80, nil) - f.EnsureIngress(ing) - - f.WaitForNginxServer(host, - func(server string) bool { - return !strings.Contains(server, fmt.Sprintf("server_name %s ;", host)) - }) - - f.HTTPTestClient(). - GET("/foo/bar/lalala"). - WithHeader("Host", host). - Expect(). - Status(http.StatusNotFound) - }) - - ginkgo.It("should accept an ingress with regex path and regex enabled", func() { - - ing := framework.NewSingleIngress(host, regexPath, host, f.Namespace, framework.EchoService, 80, annotationRegex) - f.EnsureIngress(ing) - - f.WaitForNginxServer(host, - func(server string) bool { - return strings.Contains(server, fmt.Sprintf("server_name %s ;", host)) - }) - - f.HTTPTestClient(). - GET("/foo/bar/lalala"). - WithHeader("Host", host). - Expect(). - Status(http.StatusOK) - }) - - ginkgo.It("should reject an ingress with invalid path and regex enabled", func() { - - ing := framework.NewSingleIngress(host, invalidPath, host, f.Namespace, framework.EchoService, 80, annotationRegex) - f.EnsureIngress(ing) - - f.WaitForNginxServer(host, - func(server string) bool { - return !strings.Contains(server, fmt.Sprintf("server_name %s ;", host)) - }) - - f.HTTPTestClient(). - GET(invalidPath). - WithHeader("Host", host). - Expect(). - Status(http.StatusNotFound) - }) -})