From 645f8b6a0920884f47ee9c3b5052678c423be216 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=E2=80=9Cshaun-nx=E2=80=9D?= <“s.odonovan@f5.com”> Date: Thu, 2 Jun 2022 11:47:14 +0100 Subject: [PATCH 1/7] Add string validation to server-token annotation --- internal/k8s/validation.go | 12 +++++++++- internal/k8s/validation_test.go | 42 +++++++++++++++++++++++++++++++++ 2 files changed, 53 insertions(+), 1 deletion(-) diff --git a/internal/k8s/validation.go b/internal/k8s/validation.go index 6f07cc3da6..65c9d9487d 100644 --- a/internal/k8s/validation.go +++ b/internal/k8s/validation.go @@ -3,6 +3,7 @@ package k8s import ( "errors" "fmt" + "regexp" "sort" "strings" @@ -62,7 +63,8 @@ const ( ) const ( - commaDelimiter = "," + commaDelimiter = "," + specialCharPattern = "[{}$;]" ) type annotationValidationContext struct { @@ -84,6 +86,8 @@ type ( validatorFunc func(val string) error ) +var specialCharRegex = regexp.MustCompile(specialCharPattern) + var ( // annotationValidations defines the various validations which will be applied in order to each ingress annotation. // If any specified validation fails, the remaining validations for that annotation will not be run. @@ -434,6 +438,12 @@ func validateServerTokensAnnotation(context *annotationValidationContext) field. return append(allErrs, field.Invalid(context.fieldPath, context.value, "must be a boolean")) } } + + if specialCharRegex.MatchString(context.value) { + allErrs = append(allErrs, field.Invalid(context.fieldPath, context.value, + "cannot contain special characters")) + } + return allErrs } diff --git a/internal/k8s/validation_test.go b/internal/k8s/validation_test.go index 6f93b9e9df..552e09f923 100644 --- a/internal/k8s/validation_test.go +++ b/internal/k8s/validation_test.go @@ -534,6 +534,48 @@ func TestValidateNginxIngressAnnotations(t *testing.T) { }, msg: "invalid nginx.org/server-tokens annotation, must be a boolean", }, + { + annotations: map[string]string{ + "nginx.org/server-tokens": "$custom_setting", + }, + specServices: map[string]bool{}, + isPlus: true, + appProtectEnabled: false, + appProtectDosEnabled: false, + internalRoutesEnabled: false, + expectedErrors: []string{ + `annotations.nginx.org/server-tokens: Invalid value: "$custom_setting": cannot contain special characters`, + }, + msg: "invalid nginx.org/server-tokens annotation, cannot contain special characters", + }, + { + annotations: map[string]string{ + "nginx.org/server-tokens": "{custom_setting}", + }, + specServices: map[string]bool{}, + isPlus: true, + appProtectEnabled: false, + appProtectDosEnabled: false, + internalRoutesEnabled: false, + expectedErrors: []string{ + `annotations.nginx.org/server-tokens: Invalid value: "{custom_setting}": cannot contain special characters`, + }, + msg: "invalid nginx.org/server-tokens annotation, cannot contain special characters", + }, + { + annotations: map[string]string{ + "nginx.org/server-tokens": "custom_setting;", + }, + specServices: map[string]bool{}, + isPlus: true, + appProtectEnabled: false, + appProtectDosEnabled: false, + internalRoutesEnabled: false, + expectedErrors: []string{ + `annotations.nginx.org/server-tokens: Invalid value: "custom_setting;": cannot contain special characters`, + }, + msg: "invalid nginx.org/server-tokens annotation, cannot contain special characters", + }, { annotations: map[string]string{ From 0a4091c7007b0707ee1d90c07f594dabde8ed689 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=E2=80=9Cshaun-nx=E2=80=9D?= <“s.odonovan@f5.com”> Date: Thu, 2 Jun 2022 13:51:24 +0100 Subject: [PATCH 2/7] Update documentation --- .../ingress-resources/advanced-configuration-with-annotations.md | 1 - 1 file changed, 1 deletion(-) diff --git a/docs/content/configuration/ingress-resources/advanced-configuration-with-annotations.md b/docs/content/configuration/ingress-resources/advanced-configuration-with-annotations.md index 0a0f800815..77f505853c 100644 --- a/docs/content/configuration/ingress-resources/advanced-configuration-with-annotations.md +++ b/docs/content/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/server-tokens`, - `nginx.org/rewrites`, - `nginx.com/jwt-key`, - `nginx.com/jwt-realm`, From 1f6d1935cded1cfab761e89f65d7851e4d3735ed Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=E2=80=9Cshaun-nx=E2=80=9D?= <“s.odonovan@f5.com”> Date: Thu, 9 Jun 2022 10:17:33 +0100 Subject: [PATCH 3/7] Update regex for server token validation --- examples/complete-example/cafe-ingress.yaml | 2 ++ internal/k8s/validation.go | 15 +++++++------ internal/k8s/validation_test.go | 24 +++++---------------- 3 files changed, 15 insertions(+), 26 deletions(-) diff --git a/examples/complete-example/cafe-ingress.yaml b/examples/complete-example/cafe-ingress.yaml index 50a8893990..ed16ba3cb4 100644 --- a/examples/complete-example/cafe-ingress.yaml +++ b/examples/complete-example/cafe-ingress.yaml @@ -2,6 +2,8 @@ apiVersion: networking.k8s.io/v1 kind: Ingress metadata: name: cafe-ingress + annotations: + nginx.org/server-tokens: "\custom_setting" spec: ingressClassName: nginx tls: diff --git a/internal/k8s/validation.go b/internal/k8s/validation.go index 65c9d9487d..917231dc8b 100644 --- a/internal/k8s/validation.go +++ b/internal/k8s/validation.go @@ -63,8 +63,9 @@ const ( ) const ( - commaDelimiter = "," - specialCharPattern = "[{}$;]" + commaDelimiter = "," + headerValueFmt = `([^"$\\]|\\[^$])*` + headerValueFmtErrMsg = `a valid header must have all '"' escaped and must not contain any '$' or end with an unescaped '\'` ) type annotationValidationContext struct { @@ -86,7 +87,7 @@ type ( validatorFunc func(val string) error ) -var specialCharRegex = regexp.MustCompile(specialCharPattern) +var validHeaderValueRegex = regexp.MustCompile("^" + headerValueFmt + "$") var ( // annotationValidations defines the various validations which will be applied in order to each ingress annotation. @@ -273,7 +274,7 @@ var ( validateRewriteListAnnotation, }, stickyCookieServicesAnnotation: { - validatePlusOnlyAnnotation, + //validatePlusOnlyAnnotation, validateRequiredAnnotation, validateStickyServiceListAnnotation, }, @@ -433,15 +434,15 @@ func validateLBMethodAnnotation(context *annotationValidationContext) field.Erro func validateServerTokensAnnotation(context *annotationValidationContext) field.ErrorList { allErrs := field.ErrorList{} + if !context.isPlus { if _, err := configs.ParseBool(context.value); err != nil { return append(allErrs, field.Invalid(context.fieldPath, context.value, "must be a boolean")) } } - if specialCharRegex.MatchString(context.value) { - allErrs = append(allErrs, field.Invalid(context.fieldPath, context.value, - "cannot contain special characters")) + if !validHeaderValueRegex.MatchString(context.value) { + allErrs = append(allErrs, field.Invalid(context.fieldPath, context.value, headerValueFmtErrMsg)) } return allErrs diff --git a/internal/k8s/validation_test.go b/internal/k8s/validation_test.go index 552e09f923..2ba4728eab 100644 --- a/internal/k8s/validation_test.go +++ b/internal/k8s/validation_test.go @@ -544,13 +544,13 @@ func TestValidateNginxIngressAnnotations(t *testing.T) { appProtectDosEnabled: false, internalRoutesEnabled: false, expectedErrors: []string{ - `annotations.nginx.org/server-tokens: Invalid value: "$custom_setting": cannot contain special characters`, + `annotations.nginx.org/server-tokens: Invalid value: "$custom_setting": ` + headerValueFmtErrMsg, }, - msg: "invalid nginx.org/server-tokens annotation, cannot contain special characters", + msg: "invalid nginx.org/server-tokens annotation, " + headerValueFmtErrMsg, }, { annotations: map[string]string{ - "nginx.org/server-tokens": "{custom_setting}", + "nginx.org/server-tokens": `custom_setting\`, }, specServices: map[string]bool{}, isPlus: true, @@ -558,23 +558,9 @@ func TestValidateNginxIngressAnnotations(t *testing.T) { appProtectDosEnabled: false, internalRoutesEnabled: false, expectedErrors: []string{ - `annotations.nginx.org/server-tokens: Invalid value: "{custom_setting}": cannot contain special characters`, + `annotations.nginx.org/server-tokens: Invalid value: "custom_setting\\": ` + headerValueFmtErrMsg, }, - msg: "invalid nginx.org/server-tokens annotation, cannot contain special characters", - }, - { - annotations: map[string]string{ - "nginx.org/server-tokens": "custom_setting;", - }, - specServices: map[string]bool{}, - isPlus: true, - appProtectEnabled: false, - appProtectDosEnabled: false, - internalRoutesEnabled: false, - expectedErrors: []string{ - `annotations.nginx.org/server-tokens: Invalid value: "custom_setting;": cannot contain special characters`, - }, - msg: "invalid nginx.org/server-tokens annotation, cannot contain special characters", + msg: "invalid nginx.org/server-tokens annotation, " + headerValueFmtErrMsg, }, { From 2f77409212588f51f870b6250ef30750bb223c7c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=E2=80=9Cshaun-nx=E2=80=9D?= <“s.odonovan@f5.com”> Date: Thu, 9 Jun 2022 10:19:25 +0100 Subject: [PATCH 4/7] Fix test --- internal/k8s/validation.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/internal/k8s/validation.go b/internal/k8s/validation.go index 917231dc8b..8eca2cfd9a 100644 --- a/internal/k8s/validation.go +++ b/internal/k8s/validation.go @@ -274,7 +274,7 @@ var ( validateRewriteListAnnotation, }, stickyCookieServicesAnnotation: { - //validatePlusOnlyAnnotation, + validatePlusOnlyAnnotation, validateRequiredAnnotation, validateStickyServiceListAnnotation, }, From b213740c4cc9a58c18d34cbd37cb60781bfaebe2 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=E2=80=9Cshaun-nx=E2=80=9D?= <“s.odonovan@f5.com”> Date: Thu, 9 Jun 2022 10:24:27 +0100 Subject: [PATCH 5/7] Add test to disallow `"` in the middle of the string --- internal/k8s/validation_test.go | 14 ++++++++++++++ 1 file changed, 14 insertions(+) diff --git a/internal/k8s/validation_test.go b/internal/k8s/validation_test.go index 2ba4728eab..ac4c7cee5b 100644 --- a/internal/k8s/validation_test.go +++ b/internal/k8s/validation_test.go @@ -548,6 +548,20 @@ func TestValidateNginxIngressAnnotations(t *testing.T) { }, msg: "invalid nginx.org/server-tokens annotation, " + headerValueFmtErrMsg, }, + { + annotations: map[string]string{ + "nginx.org/server-tokens": "custom_\"setting", + }, + specServices: map[string]bool{}, + isPlus: true, + appProtectEnabled: false, + appProtectDosEnabled: false, + internalRoutesEnabled: false, + expectedErrors: []string{ + `annotations.nginx.org/server-tokens: Invalid value: "custom_\"setting": ` + headerValueFmtErrMsg, + }, + msg: "invalid nginx.org/server-tokens annotation, " + headerValueFmtErrMsg, + }, { annotations: map[string]string{ "nginx.org/server-tokens": `custom_setting\`, From 0c399189733429deb77f8eb85a4254d44c25e719 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=E2=80=9Cshaun-nx=E2=80=9D?= <“s.odonovan@f5.com”> Date: Thu, 9 Jun 2022 11:04:38 +0100 Subject: [PATCH 6/7] Revert cafe-ingress.yaml --- examples/complete-example/cafe-ingress.yaml | 2 -- 1 file changed, 2 deletions(-) diff --git a/examples/complete-example/cafe-ingress.yaml b/examples/complete-example/cafe-ingress.yaml index ed16ba3cb4..50a8893990 100644 --- a/examples/complete-example/cafe-ingress.yaml +++ b/examples/complete-example/cafe-ingress.yaml @@ -2,8 +2,6 @@ apiVersion: networking.k8s.io/v1 kind: Ingress metadata: name: cafe-ingress - annotations: - nginx.org/server-tokens: "\custom_setting" spec: ingressClassName: nginx tls: From f891e11b93e55c50dc8f298fbfad1351332c7cf3 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=E2=80=9Cshaun-nx=E2=80=9D?= <“s.odonovan@f5.com”> Date: Thu, 9 Jun 2022 11:07:36 +0100 Subject: [PATCH 7/7] Rename regex variables --- internal/k8s/validation.go | 12 ++++++------ internal/k8s/validation_test.go | 12 ++++++------ 2 files changed, 12 insertions(+), 12 deletions(-) diff --git a/internal/k8s/validation.go b/internal/k8s/validation.go index 8eca2cfd9a..fc463e0857 100644 --- a/internal/k8s/validation.go +++ b/internal/k8s/validation.go @@ -63,9 +63,9 @@ const ( ) const ( - commaDelimiter = "," - headerValueFmt = `([^"$\\]|\\[^$])*` - headerValueFmtErrMsg = `a valid header must have all '"' escaped and must not contain any '$' or end with an unescaped '\'` + commaDelimiter = "," + annotationValueFmt = `([^"$\\]|\\[^$])*` + annotationValueFmtErrMsg = `a valid annotation value must have all '"' escaped and must not contain any '$' or end with an unescaped '\'` ) type annotationValidationContext struct { @@ -87,7 +87,7 @@ type ( validatorFunc func(val string) error ) -var validHeaderValueRegex = regexp.MustCompile("^" + headerValueFmt + "$") +var validAnnotationValueRegex = regexp.MustCompile("^" + annotationValueFmt + "$") var ( // annotationValidations defines the various validations which will be applied in order to each ingress annotation. @@ -441,8 +441,8 @@ func validateServerTokensAnnotation(context *annotationValidationContext) field. } } - if !validHeaderValueRegex.MatchString(context.value) { - allErrs = append(allErrs, field.Invalid(context.fieldPath, context.value, headerValueFmtErrMsg)) + if !validAnnotationValueRegex.MatchString(context.value) { + allErrs = append(allErrs, field.Invalid(context.fieldPath, context.value, annotationValueFmtErrMsg)) } return allErrs diff --git a/internal/k8s/validation_test.go b/internal/k8s/validation_test.go index 16542a8f28..b6915facec 100644 --- a/internal/k8s/validation_test.go +++ b/internal/k8s/validation_test.go @@ -586,9 +586,9 @@ func TestValidateNginxIngressAnnotations(t *testing.T) { appProtectDosEnabled: false, internalRoutesEnabled: false, expectedErrors: []string{ - `annotations.nginx.org/server-tokens: Invalid value: "$custom_setting": ` + headerValueFmtErrMsg, + `annotations.nginx.org/server-tokens: Invalid value: "$custom_setting": ` + annotationValueFmtErrMsg, }, - msg: "invalid nginx.org/server-tokens annotation, " + headerValueFmtErrMsg, + msg: "invalid nginx.org/server-tokens annotation, " + annotationValueFmtErrMsg, }, { annotations: map[string]string{ @@ -600,9 +600,9 @@ func TestValidateNginxIngressAnnotations(t *testing.T) { appProtectDosEnabled: false, internalRoutesEnabled: false, expectedErrors: []string{ - `annotations.nginx.org/server-tokens: Invalid value: "custom_\"setting": ` + headerValueFmtErrMsg, + `annotations.nginx.org/server-tokens: Invalid value: "custom_\"setting": ` + annotationValueFmtErrMsg, }, - msg: "invalid nginx.org/server-tokens annotation, " + headerValueFmtErrMsg, + msg: "invalid nginx.org/server-tokens annotation, " + annotationValueFmtErrMsg, }, { annotations: map[string]string{ @@ -614,9 +614,9 @@ func TestValidateNginxIngressAnnotations(t *testing.T) { appProtectDosEnabled: false, internalRoutesEnabled: false, expectedErrors: []string{ - `annotations.nginx.org/server-tokens: Invalid value: "custom_setting\\": ` + headerValueFmtErrMsg, + `annotations.nginx.org/server-tokens: Invalid value: "custom_setting\\": ` + annotationValueFmtErrMsg, }, - msg: "invalid nginx.org/server-tokens annotation, " + headerValueFmtErrMsg, + msg: "invalid nginx.org/server-tokens annotation, " + annotationValueFmtErrMsg, }, {