Revert Implement pathType validation (#9511) (#9607)

Signed-off-by: James Strong <[email protected]>

Author: strongjz

charts/ingress-nginx/README.md

@@ -253,7 +253,6 @@ Kubernetes: `>=1.20.0-0`
 | Key | Type | Default | Description |
 |-----|------|---------|-------------|
 | commonLabels | object | `{}` |  |
-| controller.EnablePathTypeValidation | bool | `false` | This configuration defines if Ingress Controller should validate pathType. If false, special characters will be allowed on paths of any pathType. If true, special characters are only allowed on paths with pathType = ImplementationSpecific |
 | controller.addHeaders | object | `{}` | Will add custom headers before sending response traffic to the client according to: https://kubernetes.github.io/ingress-nginx/user-guide/nginx-configuration/configmap/#add-headers |
 | controller.admissionWebhooks.annotations | object | `{}` |  |
 | controller.admissionWebhooks.certManager.admissionCert.duration | string | `""` |  |

charts/ingress-nginx/templates/controller-configmap.yaml

@@ -14,7 +14,6 @@ metadata:
   namespace: {{ .Release.Namespace }}
 data:
   allow-snippet-annotations: "{{ .Values.controller.allowSnippetAnnotations }}"
-  enable-pathtype-validation: "{{ .Values.controller.EnablePathTypeValidation }}"
 {{- if .Values.controller.addHeaders }}
   add-headers: {{ .Release.Namespace }}/{{ include "ingress-nginx.fullname" . }}-custom-add-headers
 {{- end }}

charts/ingress-nginx/values.yaml

@@ -87,11 +87,6 @@ controller:
   # Global snippets in ConfigMap are still respected
   allowSnippetAnnotations: true
 
-  # -- This configuration defines if Ingress Controller should validate pathType.
-  # If false, special characters will be allowed on paths of any pathType.
-  # If true, special characters are only allowed on paths with pathType = ImplementationSpecific
-  EnablePathTypeValidation: false
-
   # -- Required for use with CNI based kubernetes installations (such as ones set up by kubeadm),
   # since CNI and hostport don't mix yet. Can be deprecated once https://github.com/kubernetes/kubernetes/issues/23920
   # is merged

internal/ingress/controller/config/config.go

@@ -782,18 +782,6 @@ type Configuration struct {
 	// http://nginx.org/en/docs/ngx_core_module.html#debug_connection
 	// Default: ""
 	DebugConnections []string `json:"debug-connections"`
-
-	// EnablePathTypeValidation allows the admin to enable the pathType validation.
-	// If EnablePathTypeValidation is enabled, the Controller will only allow alphanumeric
-	// characters on path (0-9, a-z, A-Z, "-", ".", "_", "~", "/")
-	// to control what characters are allowed set them with PathAdditionalAllowedChars
-	EnablePathTypeValidation bool `json:"enable-pathtype-validation"`
-
-	// PathAdditionalAllowedChars allows the admin to specify what are the additional
-	// characters allowed in case of pathType=ImplementationSpecific.
-	// Case enable-pathtype-validation=true, this characters will be only allowed on ImplementationSpecific.
-	// Defaults to: "^%$[](){}*+?"
-	PathAdditionalAllowedChars string `json:"path-additional-allowed-chars"`
 }
 
 // NewDefault returns the default nginx configuration
@@ -829,8 +817,6 @@ func NewDefault() Configuration {
 		ClientHeaderTimeout:              60,
 		ClientBodyBufferSize:             "8k",
 		ClientBodyTimeout:                60,
-		EnablePathTypeValidation:         false,
-		PathAdditionalAllowedChars:       "^%$[](){}*+?|",
 		EnableUnderscoresInHeaders:       false,
 		ErrorLogLevel:                    errorLevel,
 		UseForwardedHeaders:              false,

internal/ingress/controller/controller.go

@@ -325,10 +325,6 @@ func (n *NGINXController) CheckIngress(ing *networking.Ingress) error {
 
 	k8s.SetDefaultNGINXPathType(ing)
 
-	if err := utilingress.ValidateIngressPath(ing, cfg.EnablePathTypeValidation, cfg.PathAdditionalAllowedChars); err != nil {
-		return fmt.Errorf("ingress contains invalid characters: %s", err)
-	}
-
 	allIngresses := n.store.ListIngresses()
 
 	filter := func(toCheck *ingress.Ingress) bool {

internal/ingress/controller/controller_test.go

@@ -201,97 +201,6 @@ func TestCheckIngress(t *testing.T) {
 			},
 		},
 	}
-
-	t.Run("when validating pathType", func(t *testing.T) {
-		t.Run("When ingress contains invalid path and pathType validation is enabled", func(t *testing.T) {
-			nginx.store = fakeIngressStore{
-				ingresses: []*ingress.Ingress{},
-				configuration: ngx_config.Configuration{
-					EnablePathTypeValidation: true,
-				},
-			}
-			nginx.command = testNginxTestCommand{
-				t:        t,
-				err:      nil,
-				expected: "",
-			}
-			nginx.cfg.IngressClassConfiguration = &ingressclass.IngressClassConfiguration{
-				WatchWithoutClass: true,
-			}
-			ingPath := &networking.Ingress{
-				ObjectMeta: metav1.ObjectMeta{
-					Name:      "test-ingress1",
-					Namespace: "user-namespace1",
-					Annotations: map[string]string{
-						"kubernetes.io/ingress.class": "nginx",
-					},
-				},
-				Spec: networking.IngressSpec{
-					Rules: []networking.IngressRule{
-						{
-							Host: "example.com",
-							IngressRuleValue: networking.IngressRuleValue{
-								HTTP: &networking.HTTPIngressRuleValue{
-									Paths: []networking.HTTPIngressPath{
-										{
-											Path:     "/xpto/(a+)",
-											PathType: &pathTypePrefix,
-										},
-									},
-								},
-							},
-						},
-					},
-				},
-			}
-
-			if nginx.CheckIngress(ingPath) == nil {
-				t.Errorf("invalid path on pathTypePrefix and validation enabled should return an error")
-			}
-		})
-		t.Run("When ingress contains invalid path and pathType validation is disabled", func(t *testing.T) {
-			nginx.store = fakeIngressStore{
-				ingresses: []*ingress.Ingress{},
-				configuration: ngx_config.Configuration{
-					EnablePathTypeValidation:   false,
-					PathAdditionalAllowedChars: "^%$[](){}*+?|",
-				},
-			}
-			nginx.command = testNginxTestCommand{
-				t:        t,
-				err:      nil,
-				expected: "_,example.com",
-			}
-
-			ingPath := &networking.Ingress{
-				ObjectMeta: metav1.ObjectMeta{
-					Name:      "test-ingress2",
-					Namespace: "user-namespace2",
-				},
-				Spec: networking.IngressSpec{
-					Rules: []networking.IngressRule{
-						{
-							Host: "example.com",
-							IngressRuleValue: networking.IngressRuleValue{
-								HTTP: &networking.HTTPIngressRuleValue{
-									Paths: []networking.HTTPIngressPath{
-										{
-											Path:     "/example(/|$)(.*)",
-											PathType: &pathTypePrefix,
-										},
-									},
-								},
-							},
-						},
-					},
-				},
-			}
-
-			if nginx.CheckIngress(ingPath) != nil {
-				t.Errorf("invalid path on pathTypePrefix and validation disabled should not return an error: %s", nginx.CheckIngress(ingPath))
-			}
-		})
-	})
 	t.Run("when the class is the nginx one", func(t *testing.T) {
 		ing.ObjectMeta.Annotations["kubernetes.io/ingress.class"] = "nginx"
 		nginx.command = testNginxTestCommand{

internal/ingress/controller/store/store.go

@@ -846,11 +846,6 @@ func (s *k8sStore) syncIngress(ing *networkingv1.Ingress) {
 	copyIng := &networkingv1.Ingress{}
 	ing.ObjectMeta.DeepCopyInto(&copyIng.ObjectMeta)
 
-	if err := ingressutils.ValidateIngressPath(ing, s.backendConfig.EnablePathTypeValidation, s.backendConfig.PathAdditionalAllowedChars); err != nil {
-		klog.Errorf("ingress %s contains invalid path and will be skipped: %s", key, err)
-		return
-	}
-
 	if s.backendConfig.AnnotationValueWordBlocklist != "" {
 		if err := checkBadAnnotationValue(copyIng.Annotations, s.backendConfig.AnnotationValueWordBlocklist); err != nil {
 			klog.Warningf("skipping ingress %s: %s", key, err)
@@ -870,6 +865,10 @@ 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
+			}
 		}
 	}
 

internal/k8s/main.go

@@ -180,6 +180,9 @@ func SetDefaultNGINXPathType(ing *networkingv1.Ingress) {
 				p.PathType = &defaultPathType
 			}
 
+			if *p.PathType == networkingv1.PathTypeImplementationSpecific {
+				p.PathType = &defaultPathType
+			}
 		}
 	}
 }

magefiles/helm.go

@@ -153,6 +153,9 @@ func updateChartValue(key, value string) {
 	Info("HELM Ingress Nginx Helm Chart update %s %s", key, value)
 }
 
+func (Helm) Helmdocs() error {
+	return runHelmDocs()
+}
 func runHelmDocs() error {
 	err := installHelmDocs()
 	if err != nil {

pkg/util/ingress/ingress.go

@@ -23,22 +23,23 @@ import (
 
 	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\-\.\_\~\/` // This is the default allowed set on paths
+	alphaNumericChars = `\-\.\_\~a-zA-Z0-9/`
+	regexEnabledChars = `\^\$\[\]\(\)\{\}\*\+`
 )
 
 var (
-	// pathAlphaNumeric is a regex validation that allows only (0-9, a-z, A-Z, "-", ".", "_", "~", "/")
-	pathAlphaNumericRegex = regexp.MustCompile("^[" + alphaNumericChars + "]*$").MatchString
-
-	// default path type is Prefix to not break existing definitions
-	defaultPathType = networkingv1.PathTypePrefix
+	// 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 {
@@ -246,68 +247,12 @@ func BuildRedirects(servers []*ingress.Server) []*redirect {
 	return redirectServers
 }
 
-func ValidateIngressPath(copyIng *networkingv1.Ingress, enablePathTypeValidation bool, pathAdditionalAllowedChars string) error {
-
-	if copyIng == nil {
-		return nil
-	}
-
-	escapedPathAdditionalAllowedChars := regexp.QuoteMeta(pathAdditionalAllowedChars)
-	regexPath, err := regexp.Compile("^[" + alphaNumericChars + escapedPathAdditionalAllowedChars + "]*$")
-	if err != nil {
-		return fmt.Errorf("ingress has misconfigured validation regex on configmap: %s - %w", pathAdditionalAllowedChars, err)
-	}
-
-	for _, rule := range copyIng.Spec.Rules {
-
-		if rule.HTTP == nil {
-			continue
-		}
-
-		if err := checkPath(rule.HTTP.Paths, enablePathTypeValidation, regexPath); err != nil {
-			return fmt.Errorf("error validating ingressPath: %w", err)
-		}
-	}
-	return nil
-}
-
-func checkPath(paths []networkingv1.HTTPIngressPath, enablePathTypeValidation bool, regexSpecificChars *regexp.Regexp) error {
-
-	for _, path := range paths {
-		if path.PathType == nil {
-			path.PathType = &defaultPathType
-		}
-
-		klog.V(9).InfoS("PathType Validation", "enablePathTypeValidation", enablePathTypeValidation, "regexSpecificChars", regexSpecificChars.String(), "Path", path.Path)
-
-		switch pathType := *path.PathType; pathType {
-		case networkingv1.PathTypeImplementationSpecific:
-			if enablePathTypeValidation {
-				//only match on regex chars per Ingress spec when path is implementation specific
-				if !regexSpecificChars.MatchString(path.Path) {
-					return fmt.Errorf("path %s of type %s contains invalid characters", path.Path, *path.PathType)
-				}
-			}
-
-		case networkingv1.PathTypeExact, networkingv1.PathTypePrefix:
-			//enforce path type validation
-			if enablePathTypeValidation {
-				//only allow alphanumeric chars, no regex chars
-				if !pathAlphaNumericRegex(path.Path) {
-					return fmt.Errorf("path %s of type %s contains invalid characters", path.Path, *path.PathType)
-				}
-				continue
-			} else {
-				//path validation is disabled, so we check what regex chars are allowed by user
-				if !regexSpecificChars.MatchString(path.Path) {
-					return fmt.Errorf("path %s of type %s contains invalid characters", path.Path, *path.PathType)
-				}
-				continue
-			}
-
-		default:
-			return fmt.Errorf("unknown path type %v on path %v", *path.PathType, path.Path)
-		}
+// 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 nil
+	return pathAlphaNumeric(path)
 }