Validate rewrite annotation (#2734)

* Validate rewrite annotation * Disallow regex * fix unit tests * fix typo
nginx · Jun 10, 2022 · 70fa273 · 70fa273
1 parent b05e501
commit 70fa273
Show file tree

Hide file tree

Showing 4 changed files with 228 additions and 15 deletions.
diff --git a/...tent/configuration/ingress-resources/advanced-configuration-with-annotations.md b/...tent/configuration/ingress-resources/advanced-configuration-with-annotations.md
@@ -83,7 +83,6 @@ Note how the events section includes a Warning event with the Rejected reason.
 
 The following Ingress annotations currently have limited or no validation:
 
-- `nginx.org/rewrites`,
 - `nginx.com/jwt-key`,
 - `nginx.com/jwt-realm`,
 - `nginx.com/jwt-token`,
@@ -131,7 +130,7 @@ The table below summarizes the available annotations.
 | ---| ---| ---| ---| --- |
 |``nginx.org/proxy-hide-headers`` | ``proxy-hide-headers`` | Sets the value of one or more  [proxy_hide_header](https://nginx.org/en/docs/http/ngx_http_proxy_module.html#proxy_hide_header) directives. Example: ``"nginx.org/proxy-hide-headers": "header-a,header-b"`` | N/A |  |
 |``nginx.org/proxy-pass-headers`` | ``proxy-pass-headers`` | Sets the value of one or more   [proxy_pass_header](https://nginx.org/en/docs/http/ngx_http_proxy_module.html#proxy_pass_header) directives. Example: ``"nginx.org/proxy-pass-headers": "header-a,header-b"`` | N/A |  |
-|``nginx.org/rewrites`` | N/A | Configures URI rewriting. | N/A | [Rewrites Support](https://github.com/nginxinc/kubernetes-ingress/tree/v2.2.2/examples/rewrites). |
+|``nginx.org/rewrites`` | N/A | Configures URI rewriting using [proxy_pass](https://nginx.org/en/docs/http/ngx_http_proxy_module.html#proxy_pass) directive. | N/A | [Rewrites Support](https://github.com/nginxinc/kubernetes-ingress/tree/v2.2.2/examples/rewrites). |
 {{% /table %}}
 
 ### Auth and SSL/TLS

diff --git a/internal/configs/parsing_helpers.go b/internal/configs/parsing_helpers.go
@@ -333,28 +333,38 @@ func parseRewrites(service string) (serviceName string, rewrite string, err erro
 	parts := strings.SplitN(strings.TrimSpace(service), " ", 2)
 
 	if len(parts) != 2 {
-		return "", "", fmt.Errorf("Invalid rewrite format: %s", service)
+		return "", "", fmt.Errorf("'%s' is not a valid rewrite format, e.g. 'serviceName=tea-svc rewrite=/'", service)
 	}
 
 	svcNameParts := strings.Split(parts[0], "=")
-	if len(svcNameParts) != 2 {
-		return "", "", fmt.Errorf("Invalid rewrite format: %s", svcNameParts)
+	if len(svcNameParts) != 2 || svcNameParts[0] != "serviceName" {
+		return "", "", fmt.Errorf("'%s' is not a valid serviceName format, e.g. 'serviceName=tea-svc'", parts[0])
 	}
 
 	rwPathParts := strings.Split(parts[1], "=")
-	if len(rwPathParts) != 2 {
-		return "", "", fmt.Errorf("Invalid rewrite format: %s", rwPathParts)
+	if len(rwPathParts) != 2 || rwPathParts[0] != "rewrite" {
+		return "", "", fmt.Errorf("'%s' is not a valid rewrite path format, e.g. 'rewrite=/tea'", parts[1])
+	}
+
+	if !VerifyPath(rwPathParts[1]) {
+		return "", "", fmt.Errorf("path must start with '/' and must not include any whitespace character, '{', '}' or '$': '%s'", rwPathParts[1])
 	}
 
 	return svcNameParts[1], rwPathParts[1], nil
 }
 
 var (
-	threshEx  = regexp.MustCompile(`high=([1-9]|[1-9][0-9]|100) low=([1-9]|[1-9][0-9]|100)\b`)
-	threshExR = regexp.MustCompile(`low=([1-9]|[1-9][0-9]|100) high=([1-9]|[1-9][0-9]|100)\b`)
+	threshEx   = regexp.MustCompile(`high=([1-9]|[1-9][0-9]|100) low=([1-9]|[1-9][0-9]|100)\b`)
+	threshExR  = regexp.MustCompile(`low=([1-9]|[1-9][0-9]|100) high=([1-9]|[1-9][0-9]|100)\b`)
+	pathRegexp = regexp.MustCompile("^" + `/[^\s{};$]*` + "$")
 )
 
 // VerifyAppProtectThresholds ensures that threshold values are set correctly
 func VerifyAppProtectThresholds(value string) bool {
 	return threshEx.MatchString(value) || threshExR.MatchString(value)
 }
+
+// VerifyPath ensures that rewrite paths are in the correct format
+func VerifyPath(s string) bool {
+	return pathRegexp.MatchString(s)
+}
diff --git a/internal/k8s/validation.go b/internal/k8s/validation.go
@@ -589,8 +589,22 @@ func validateStickyServiceListAnnotation(context *annotationValidationContext) f
 
 func validateRewriteListAnnotation(context *annotationValidationContext) field.ErrorList {
 	allErrs := field.ErrorList{}
-	if _, err := configs.ParseRewriteList(context.value); err != nil {
-		return append(allErrs, field.Invalid(context.fieldPath, context.value, "must be a semicolon-separated list of rewrites"))
+	var unknownServices []string
+	rewrites, err := configs.ParseRewriteList(context.value)
+	if err != nil {
+		return append(allErrs, field.Invalid(context.fieldPath, context.value, err.Error()))
+	}
+	for rewrite := range rewrites {
+		if _, exists := context.specServices[rewrite]; !exists {
+			unknownServices = append(unknownServices, rewrite)
+		}
+	}
+	if len(unknownServices) > 0 {
+		errorMsg := fmt.Sprintf(
+			"The following services were not found: %s",
+			strings.Join(unknownServices, commaDelimiter),
+		)
+		return append(allErrs, field.Invalid(context.fieldPath, context.value, errorMsg))
 	}
 	return allErrs
 }

diff --git a/internal/k8s/validation_test.go b/internal/k8s/validation_test.go
@@ -2003,9 +2003,11 @@ func TestValidateNginxIngressAnnotations(t *testing.T) {
 
 		{
 			annotations: map[string]string{
-				"nginx.org/rewrites": "serviceName=service-1 rewrite=rewrite-1",
+				"nginx.org/rewrites": "serviceName=service-1 rewrite=/rewrite-1",
+			},
+			specServices: map[string]bool{
+				"service-1": true,
 			},
-			specServices:          map[string]bool{},
 			isPlus:                false,
 			appProtectEnabled:     false,
 			appProtectDosEnabled:  false,
@@ -2015,16 +2017,204 @@ func TestValidateNginxIngressAnnotations(t *testing.T) {
 		},
 		{
 			annotations: map[string]string{
-				"nginx.org/rewrites": "serviceName=service-1 rewrite=rewrite-1;serviceName=service-2 rewrite=rewrite-2",
+				"nginx.org/rewrites": "serviceName=service-1 rewrite=/rewrite-1/",
+			},
+			specServices: map[string]bool{
+				"service-1": true,
+			},
+			isPlus:                false,
+			appProtectEnabled:     false,
+			appProtectDosEnabled:  false,
+			internalRoutesEnabled: false,
+			expectedErrors:        nil,
+			msg:                   "valid nginx.org/rewrites annotation, single-value, trailing '/'",
+		},
+		{
+			annotations: map[string]string{
+				"nginx.org/rewrites": "serviceName=service-1 rewrite=/rewrite-1/rewrite",
+			},
+			specServices: map[string]bool{
+				"service-1": true,
+			}, isPlus: false,
+			appProtectEnabled:     false,
+			appProtectDosEnabled:  false,
+			internalRoutesEnabled: false,
+			expectedErrors:        nil,
+			msg:                   "valid nginx.org/rewrites annotation, single-value, uri levels",
+		},
+		{
+			annotations: map[string]string{
+				"nginx.org/rewrites": "serviceName=service-1 rewrite=rewrite-1",
+			},
+			specServices: map[string]bool{
+				"service-1": true,
+			},
+			isPlus:                false,
+			appProtectEnabled:     false,
+			appProtectDosEnabled:  false,
+			internalRoutesEnabled: false,
+			expectedErrors: []string{
+				`annotations.nginx.org/rewrites: Invalid value: "serviceName=service-1 rewrite=rewrite-1": path must start with '/' and must not include any whitespace character, '{', '}' or '$': 'rewrite-1'`,
+			},
+			msg: "invalid nginx.org/rewrites annotation, single-value, no '/' in the beginning",
+		},
+		{
+			annotations: map[string]string{
+				"nginx.org/rewrites": "service-1 rewrite=/rewrite-1",
+			},
+			specServices: map[string]bool{
+				"service-1": true,
+			},
+			isPlus:                false,
+			appProtectEnabled:     false,
+			appProtectDosEnabled:  false,
+			internalRoutesEnabled: false,
+			expectedErrors: []string{
+				`annotations.nginx.org/rewrites: Invalid value: "service-1 rewrite=/rewrite-1": 'service-1' is not a valid serviceName format, e.g. 'serviceName=tea-svc'`,
+			},
+			msg: "invalid nginx.org/rewrites annotation, single-value, invalid service name format, 'serviceName' missing",
+		},
+		{
+			annotations: map[string]string{
+				"nginx.org/rewrites": "serviceName1=service-1 rewrite=/rewrite-1",
+			},
+			specServices: map[string]bool{
+				"service-1": true,
+			},
+			isPlus:                false,
+			appProtectEnabled:     false,
+			appProtectDosEnabled:  false,
+			internalRoutesEnabled: false,
+			expectedErrors: []string{
+				`annotations.nginx.org/rewrites: Invalid value: "serviceName1=service-1 rewrite=/rewrite-1": 'serviceName1=service-1' is not a valid serviceName format, e.g. 'serviceName=tea-svc'`,
+			},
+			msg: "invalid nginx.org/rewrites annotation, single-value, invalid service name format, 'serviceName' typo",
+		},
+		{
+			annotations: map[string]string{
+				"nginx.org/rewrites": "serviceName=service-1 rewrit=/rewrite-1",
+			},
+			specServices: map[string]bool{
+				"service-1": true,
+			},
+			isPlus:                false,
+			appProtectEnabled:     false,
+			appProtectDosEnabled:  false,
+			internalRoutesEnabled: false,
+			expectedErrors: []string{
+				`annotations.nginx.org/rewrites: Invalid value: "serviceName=service-1 rewrit=/rewrite-1": 'rewrit=/rewrite-1' is not a valid rewrite path format, e.g. 'rewrite=/tea'`,
+			},
+			msg: "invalid nginx.org/rewrites annotation, single-value, invalid service name format, 'rewrite' typo ",
+		},
+		{
+			annotations: map[string]string{
+				"nginx.org/rewrites": "serviceName=service-1 rewrite=/rewrite",
 			},
 			specServices:          map[string]bool{},
 			isPlus:                false,
 			appProtectEnabled:     false,
 			appProtectDosEnabled:  false,
 			internalRoutesEnabled: false,
+			expectedErrors: []string{
+				`annotations.nginx.org/rewrites: Invalid value: "serviceName=service-1 rewrite=/rewrite": The following services were not found: service-1`,
+			},
+			msg: "invaild nginx.org/rewrites annotation, single-value, service does not exist",
+		},
+		{
+			annotations: map[string]string{
+				"nginx.org/rewrites": "serviceName=service-1 rewrite=/rewrite-{1}",
+			},
+			specServices: map[string]bool{
+				"service-1": true,
+			},
+			isPlus:                false,
+			appProtectEnabled:     false,
+			appProtectDosEnabled:  false,
+			internalRoutesEnabled: false,
+			expectedErrors: []string{
+				`annotations.nginx.org/rewrites: Invalid value: "serviceName=service-1 rewrite=/rewrite-{1}": path must start with '/' and must not include any whitespace character, '{', '}' or '$': '/rewrite-{1}'`,
+			},
+			msg: "invalid nginx.org/rewrites annotation, single-value, path containing special characters",
+		},
+		{
+			annotations: map[string]string{
+				"nginx.org/rewrites": "serviceName=service-1 rewrite=/rewr ite",
+			},
+			specServices: map[string]bool{
+				"service-1": true,
+			},
+			isPlus:                false,
+			appProtectEnabled:     false,
+			appProtectDosEnabled:  false,
+			internalRoutesEnabled: false,
+			expectedErrors: []string{
+				`annotations.nginx.org/rewrites: Invalid value: "serviceName=service-1 rewrite=/rewr ite": path must start with '/' and must not include any whitespace character, '{', '}' or '$': '/rewr ite'`,
+			},
+			msg: "invalid nginx.org/rewrites annotation, single-value, path containing white spaces",
+		},
+		{
+			annotations: map[string]string{
+				"nginx.org/rewrites": "serviceName=service-1 rewrite=/rewrite/$1",
+			},
+			specServices: map[string]bool{
+				"service-1": true,
+			}, isPlus: false,
+			appProtectEnabled:     false,
+			appProtectDosEnabled:  false,
+			internalRoutesEnabled: false,
+			expectedErrors: []string{
+				`annotations.nginx.org/rewrites: Invalid value: "serviceName=service-1 rewrite=/rewrite/$1": path must start with '/' and must not include any whitespace character, '{', '}' or '$': '/rewrite/$1'`,
+			},
+			msg: "invaild nginx.org/rewrites annotation, single-value, path containing regex characters",
+		},
+		{
+			annotations: map[string]string{
+				"nginx.org/rewrites": "serviceName=service-1 rewrite=/rewrite-1;serviceName=service-2 rewrite=/rewrite-2",
+			},
+			specServices: map[string]bool{
+				"service-1": true,
+				"service-2": true,
+			},
+			isPlus:                false,
+			appProtectEnabled:     false,
+			appProtectDosEnabled:  false,
+			internalRoutesEnabled: false,
 			expectedErrors:        nil,
 			msg:                   "valid nginx.org/rewrites annotation, multi-value",
 		},
+		{
+			annotations: map[string]string{
+				"nginx.org/rewrites": "serviceName=service-1 rewrite=/rewrite-1;serviceName=service-2 rewrite=/rewrite-2",
+			},
+			specServices: map[string]bool{
+				"service-1": true,
+			},
+			isPlus:                false,
+			appProtectEnabled:     false,
+			appProtectDosEnabled:  false,
+			internalRoutesEnabled: false,
+			expectedErrors: []string{
+				`annotations.nginx.org/rewrites: Invalid value: "serviceName=service-1 rewrite=/rewrite-1;serviceName=service-2 rewrite=/rewrite-2": The following services were not found: service-2`,
+			},
+			msg: "valid nginx.org/rewrites annotation, multi-value, service does not exist",
+		},
+		{
+			annotations: map[string]string{
+				"nginx.org/rewrites": "serviceName=service-1 rewrite=rewrite-1;serviceName=service-2 rewrite=/rewrite-2",
+			},
+			specServices: map[string]bool{
+				"service-1": true,
+				"service-2": true,
+			},
+			isPlus:                false,
+			appProtectEnabled:     false,
+			appProtectDosEnabled:  false,
+			internalRoutesEnabled: false,
+			expectedErrors: []string{
+				`annotations.nginx.org/rewrites: Invalid value: "serviceName=service-1 rewrite=rewrite-1;serviceName=service-2 rewrite=/rewrite-2": path must start with '/' and must not include any whitespace character, '{', '}' or '$': 'rewrite-1'`,
+			},
+			msg: "invalid nginx.org/rewrites annotation, multi-value without '/' in the beginning",
+		},
 		{
 			annotations: map[string]string{
 				"nginx.org/rewrites": "not_a_rewrite",
@@ -2035,7 +2225,7 @@ func TestValidateNginxIngressAnnotations(t *testing.T) {
 			appProtectDosEnabled:  false,
 			internalRoutesEnabled: true,
 			expectedErrors: []string{
-				`annotations.nginx.org/rewrites: Invalid value: "not_a_rewrite": must be a semicolon-separated list of rewrites`,
+				`annotations.nginx.org/rewrites: Invalid value: "not_a_rewrite": 'not_a_rewrite' is not a valid rewrite format, e.g. 'serviceName=tea-svc rewrite=/'`,
 			},
 			msg: "invalid nginx.org/rewrites annotation",
 		},