Validate path types (#9967)

* Validate path types

* Fix the year of header

* Update internal/ingress/controller/config/config.go

Co-authored-by: Jintao Zhang <tao12345666333@163.com>

---------

Co-authored-by: Jintao Zhang <tao12345666333@163.com>

docs/user-guide/nginx-configuration/configmap.md

@@ -229,6 +229,7 @@ The following table shows a configuration option's name, type, and the default v
 |[service-upstream](#service-upstream)|bool|"false"|
 |[ssl-reject-handshake](#ssl-reject-handshake)|bool|"false"|
 |[debug-connections](#debug-connections)|[]string|"127.0.0.1,1.1.1.1/24"|
+|[strict-validate-path-type](#strict-validate-path-type)|bool|"false" (v1.7.x)|
 
 ## add-headers
 
@@ -1379,3 +1380,17 @@ _**default:**_ ""
 
 _References:_
 [http://nginx.org/en/docs/ngx_core_module.html#debug_connection](http://nginx.org/en/docs/ngx_core_module.html#debug_connection)
+
+## strict-validate-path-type
+Ingress objects contains a field called pathType that defines the proxy behavior. It can be `Exact`, `Prefix` and `ImplementationSpecific`.
+
+When pathType is configured as `Exact` or `Prefix`, there should be a more strict validation, allowing only paths starting with "/" and
+containing only alphanumeric characters and "-", "_" and additional "/".
+
+When this option is enabled, the validation will happen on the Admission Webhook, making any Ingress not using pathType `ImplementationSpecific`
+and containing invalid characters to be denied.
+
+This means that Ingress objects that rely on paths containing regex characters should use `ImplementationSpecific` pathType.
+
+The cluster admin should establish validation rules using mechanisms like [Open Policy Agent](https://www.openpolicyagent.org/) to 
+validate that only authorized users can use `ImplementationSpecific` pathType and that only the authorized characters can be used.

internal/ingress/controller/config/config.go

@@ -830,6 +830,12 @@ type Configuration struct {
 	// http://nginx.org/en/docs/ngx_core_module.html#debug_connection
 	// Default: ""
 	DebugConnections []string `json:"debug-connections"`
+
+	// StrictValidatePathType enable the strict validation of Ingress Paths
+	// It enforces that pathType of type Exact or Prefix should start with / and contain only
+	// alphanumeric chars, "-", "_", "/".In case of additional characters,
+	// like used on Rewrite configurations the user should use pathType as ImplementationSpecific
+	StrictValidatePathType bool `json:"strict-validate-path-type"`
 }
 
 // NewDefault returns the default nginx configuration
@@ -1002,6 +1008,7 @@ func NewDefault() Configuration {
 		GlobalRateLimitMemcachedPoolSize:       50,
 		GlobalRateLimitStatucCode:              429,
 		DebugConnections:                       []string{},
+		StrictValidatePathType:                 false, // TODO: This will be true in future releases
 	}
 
 	if klog.V(5).Enabled() {

internal/ingress/controller/controller.go

@@ -270,11 +270,13 @@ func (n *NGINXController) CheckIngress(ing *networking.Ingress) error {
 	if !ing.DeletionTimestamp.IsZero() {
 		return nil
 	}
+
 	if n.cfg.DeepInspector {
 		if err := inspector.DeepInspect(ing); err != nil {
 			return fmt.Errorf("invalid object: %w", err)
 		}
 	}
+
 	// Do not attempt to validate an ingress that's not meant to be controlled by the current instance of the controller.
 	if ingressClass, err := n.store.GetIngressClass(ing, n.cfg.IngressClassConfiguration); ingressClass == "" {
 		klog.Warningf("ignoring ingress %v in %v based on annotation %v: %v", ing.Name, ing.ObjectMeta.Namespace, ingressClass, err)
@@ -293,6 +295,13 @@ func (n *NGINXController) CheckIngress(ing *networking.Ingress) error {
 	cfg := n.store.GetBackendConfiguration()
 	cfg.Resolver = n.resolver
 
+	// Adds the pathType Validation
+	if cfg.StrictValidatePathType {
+		if err := inspector.ValidatePathType(ing); err != nil {
+			return fmt.Errorf("ingress contains invalid paths: %w", err)
+		}
+	}
+
 	var arrayBadWords []string
 
 	if cfg.AnnotationValueWordBlocklist != "" {

internal/ingress/inspector/inspector.go

@@ -17,6 +17,9 @@ limitations under the License.
 package inspector
 
 import (
+	"errors"
+	"fmt"
+
 	corev1 "k8s.io/api/core/v1"
 	networking "k8s.io/api/networking/v1"
 	"k8s.io/klog/v2"
@@ -36,3 +39,29 @@ func DeepInspect(obj interface{}) error {
 		return nil
 	}
 }
+
+var (
+	implSpecific = networking.PathTypeImplementationSpecific
+)
+
+func ValidatePathType(ing *networking.Ingress) error {
+	if ing == nil {
+		return fmt.Errorf("received null ingress")
+	}
+	var err error
+	for _, rule := range ing.Spec.Rules {
+		if rule.HTTP != nil {
+			for _, path := range rule.HTTP.Paths {
+				if path.Path == "" {
+					continue
+				}
+				if path.PathType == nil || *path.PathType != implSpecific {
+					if isValid := validPathType.MatchString(path.Path); !isValid {
+						err = errors.Join(err, fmt.Errorf("path %s cannot be used with pathType %s", path.Path, string(*path.PathType)))
+					}
+				}
+			}
+		}
+	}
+	return err
+}

internal/ingress/inspector/inspector_test.go

@@ -0,0 +1,191 @@
+/*
+Copyright 2023 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 inspector
+
+import (
+	"errors"
+	"fmt"
+	"testing"
+
+	networking "k8s.io/api/networking/v1"
+)
+
+var (
+	exact  = networking.PathTypeExact
+	prefix = networking.PathTypePrefix
+)
+
+var (
+	validIngress = &networking.Ingress{
+		Spec: networking.IngressSpec{
+			Rules: []networking.IngressRule{
+				{
+					IngressRuleValue: networking.IngressRuleValue{
+						HTTP: &networking.HTTPIngressRuleValue{
+							Paths: []networking.HTTPIngressPath{
+								{
+									Path: "/test",
+								},
+								{
+									PathType: &prefix,
+									Path:     "/xpto/ab0/x_ss-9",
+								},
+								{
+									PathType: &exact,
+									Path:     "/bla/",
+								},
+							},
+						},
+					},
+				},
+			},
+		},
+	}
+
+	emptyIngress = &networking.Ingress{
+		Spec: networking.IngressSpec{
+			Rules: []networking.IngressRule{
+				{
+					IngressRuleValue: networking.IngressRuleValue{
+						HTTP: &networking.HTTPIngressRuleValue{
+							Paths: []networking.HTTPIngressPath{
+								{
+									PathType: &exact,
+								},
+							},
+						},
+					},
+				},
+			},
+		},
+	}
+
+	invalidIngress = &networking.Ingress{
+		Spec: networking.IngressSpec{
+			Rules: []networking.IngressRule{
+				{
+					IngressRuleValue: networking.IngressRuleValue{
+						HTTP: &networking.HTTPIngressRuleValue{
+							Paths: []networking.HTTPIngressPath{
+								{
+									PathType: &exact,
+									Path:     "/foo.+",
+								},
+								{
+									PathType: &exact,
+									Path:     "xpto/lala",
+								},
+								{
+									PathType: &exact,
+									Path:     "/xpto/lala",
+								},
+								{
+									PathType: &prefix,
+									Path:     "/foo/bar/[a-z]{3}",
+								},
+								{
+									PathType: &prefix,
+									Path:     "/lala/xp\ntest",
+								},
+							},
+						},
+					},
+				},
+			},
+		},
+	}
+
+	validImplSpecific = &networking.Ingress{
+		Spec: networking.IngressSpec{
+			Rules: []networking.IngressRule{
+				{
+					IngressRuleValue: networking.IngressRuleValue{
+						HTTP: &networking.HTTPIngressRuleValue{
+							Paths: []networking.HTTPIngressPath{
+								{
+									PathType: &implSpecific,
+									Path:     "/foo.+",
+								},
+								{
+									PathType: &implSpecific,
+									Path:     "xpto/lala",
+								},
+							},
+						},
+					},
+				},
+			},
+		},
+	}
+)
+
+var aErr = func(s, pathType string) error {
+	return fmt.Errorf("path %s cannot be used with pathType %s", s, pathType)
+}
+
+func TestValidatePathType(t *testing.T) {
+	tests := []struct {
+		name    string
+		ing     *networking.Ingress
+		wantErr bool
+		err     error
+	}{
+		{
+			name:    "nil should return an error",
+			ing:     nil,
+			wantErr: true,
+			err:     fmt.Errorf("received null ingress"),
+		},
+		{
+			name:    "valid should not return an error",
+			ing:     validIngress,
+			wantErr: false,
+		},
+		{
+			name:    "empty should not return an error",
+			ing:     emptyIngress,
+			wantErr: false,
+		},
+		{
+			name:    "empty should not return an error",
+			ing:     validImplSpecific,
+			wantErr: false,
+		},
+		{
+			name:    "invalid should return multiple errors",
+			ing:     invalidIngress,
+			wantErr: true,
+			err: errors.Join(
+				aErr("/foo.+", "Exact"),
+				aErr("xpto/lala", "Exact"),
+				aErr("/foo/bar/[a-z]{3}", "Prefix"),
+				aErr("/lala/xp\ntest", "Prefix"),
+			),
+		},
+	}
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			err := ValidatePathType(tt.ing)
+			if (err != nil) != tt.wantErr {
+				t.Errorf("ValidatePathType() error = %v, wantErr %v", err, tt.wantErr)
+			}
+			if (err != nil && tt.err != nil) && tt.err.Error() != err.Error() {
+				t.Errorf("received invalid error: want = %v, expected %v", tt.err, err)
+			}
+		})
+	}
+}

internal/ingress/inspector/rules.go

@@ -28,6 +28,14 @@ var (
 	invalidSecretsDir     = regexp.MustCompile(`/var/run/secrets`)
 	invalidByLuaDirective = regexp.MustCompile(`.*_by_lua.*`)
 
+	// validPathType enforces alphanumeric, -, _ and / characters.
+	// The field (?i) turns this regex case insensitive
+	// The remaining regex says that the string must start with a "/" (^/)
+	// the group [[:alnum:]\_\-\/]* says that any amount of characters (A-Za-z0-9), _, - and /
+	// are accepted until the end of the line
+	// Nothing else is accepted.
+	validPathType = regexp.MustCompile(`(?i)^/[[:alnum:]\_\-\/]*$`)
+
 	invalidRegex = []regexp.Regexp{}
 )
 

test/e2e/admission/admission.go

@@ -30,6 +30,8 @@ import (
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 
 	"k8s.io/ingress-nginx/test/e2e/framework"
+
+	networking "k8s.io/api/networking/v1"
 )
 
 var _ = framework.IngressNginxDescribeSerial("[Admission] admission controller", func() {
@@ -161,6 +163,41 @@ var _ = framework.IngressNginxDescribeSerial("[Admission] admission controller",
 		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 an invalid path and wrong pathType is set", func() {
+		host := "path-validation"
+		var (
+			exactPathType  = networking.PathTypeExact
+			prefixPathType = networking.PathTypePrefix
+			implSpecific   = networking.PathTypeImplementationSpecific
+		)
+
+		f.UpdateNginxConfigMapData("strict-validate-path-type", "true")
+
+		invalidPath := framework.NewSingleIngress("first-ingress", "/foo/bar/[a-z]{3}", host, f.Namespace, framework.EchoService, 80, nil)
+		invalidPath.Spec.Rules[0].IngressRuleValue.HTTP.Paths[0].PathType = &exactPathType
+
+		_, err := f.KubeClientSet.NetworkingV1().Ingresses(f.Namespace).Create(context.TODO(), invalidPath, metav1.CreateOptions{})
+		assert.NotNil(ginkgo.GinkgoT(), err, "creating an ingress with invalid path value should return an error")
+
+		invalidPath.Spec.Rules[0].IngressRuleValue.HTTP.Paths[0].PathType = &prefixPathType
+		_, err = f.KubeClientSet.NetworkingV1().Ingresses(f.Namespace).Create(context.TODO(), invalidPath, metav1.CreateOptions{})
+		assert.NotNil(ginkgo.GinkgoT(), err, "creating an ingress with invalid path value should return an error")
+
+		annotations := map[string]string{
+			"nginx.ingress.kubernetes.io/use-regex":      "true",
+			"nginx.ingress.kubernetes.io/rewrite-target": "/new/backend",
+		}
+		pathSpecific := framework.NewSingleIngress("pathspec-ingress", "/foo/bar/[a-z]{3}", host, f.Namespace, framework.EchoService, 80, annotations)
+		pathSpecific.Spec.Rules[0].IngressRuleValue.HTTP.Paths[0].PathType = &implSpecific
+		_, err = f.KubeClientSet.NetworkingV1().Ingresses(f.Namespace).Create(context.TODO(), pathSpecific, metav1.CreateOptions{})
+		assert.Nil(ginkgo.GinkgoT(), err, "creating an ingress with arbitrary path and implSpecific value should not return an error")
+
+		validPath := framework.NewSingleIngress("second-ingress", "/bloblo", host, f.Namespace, framework.EchoService, 80, nil)
+		_, err = f.KubeClientSet.NetworkingV1().Ingresses(f.Namespace).Create(context.TODO(), validPath, metav1.CreateOptions{})
+		assert.Nil(ginkgo.GinkgoT(), err, "creating an ingress with valid path should not return an error")
+
+	})
+
 	ginkgo.It("should not return an error if the Ingress V1 definition is valid with Ingress Class", func() {
 		out, err := createIngress(f.Namespace, validV1Ingress)
 		assert.Equal(ginkgo.GinkgoT(), "ingress.networking.k8s.io/extensions created\n", out)