From 8eeeb669e3b053532b7f0b10121cb61ae23e5a91 Mon Sep 17 00:00:00 2001 From: Haywood Shannon <5781935+haywoodsh@users.noreply.github.com> Date: Mon, 19 Sep 2022 13:41:50 +0100 Subject: [PATCH 1/3] update path validation --- internal/k8s/validation.go | 82 +++++++++- internal/k8s/validation_test.go | 149 ++++++++++++++++++ .../validation/virtualserver_test.go | 6 + 3 files changed, 234 insertions(+), 3 deletions(-) diff --git a/internal/k8s/validation.go b/internal/k8s/validation.go index 4d51fce423..ae08b65085 100644 --- a/internal/k8s/validation.go +++ b/internal/k8s/validation.go @@ -72,18 +72,15 @@ const ( const ( commaDelimiter = "," annotationValueFmt = `([^"$\\]|\\[^$])*` - pathFmt = `/[^\s{};\\]*` jwtTokenValueFmt = "\\$" + annotationValueFmt ) const ( annotationValueFmtErrMsg = `a valid annotation value must have all '"' escaped and must not contain any '$' or end with an unescaped '\'` - pathErrMsg = "must start with / and must not include any whitespace character, `{`, `}` or `;`" jwtTokenValueFmtErrMsg = `a valid annotation value must start with '$', have all '"' escaped, and must not contain any '$' or end with an unescaped '\'` ) var ( - pathRegexp = regexp.MustCompile("^" + pathFmt + "$") validAnnotationValueRegex = regexp.MustCompile("^" + annotationValueFmt + "$") validJWTTokenAnnotationValueRegex = regexp.MustCompile("^" + jwtTokenValueFmt + "$") ) @@ -875,6 +872,13 @@ func validateBackend(backend *networking.IngressBackend, fieldPath *field.Path) return allErrs } +const ( + pathFmt = `/[^\s;\\]*` + pathErrMsg = "must start with / and must not include any whitespace character or `;`" +) + +var pathRegexp = regexp.MustCompile("^" + pathFmt + "$") + func validatePath(path string, fieldPath *field.Path) field.ErrorList { allErrs := field.ErrorList{} @@ -887,6 +891,78 @@ func validatePath(path string, fieldPath *field.Path) field.ErrorList { return append(allErrs, field.Invalid(fieldPath, path, msg)) } + allErrs = append(allErrs, validateRegexPath(path, fieldPath)...) + allErrs = append(allErrs, validateCurlyBraces(path, fieldPath)...) + allErrs = append(allErrs, validateIllegalKeywords(path, fieldPath)...) + + return allErrs +} + +func validateRegexPath(path string, fieldPath *field.Path) field.ErrorList { + allErrs := field.ErrorList{} + + if _, err := regexp.Compile(path); err != nil { + return append(allErrs, field.Invalid(fieldPath, path, fmt.Sprintf("must be a valid regular expression: %v", err))) + } + + if err := ValidateEscapedString(path, "*.jpg", "^/images/image_*.png$"); err != nil { + return append(allErrs, field.Invalid(fieldPath, path, err.Error())) + } + + return allErrs +} + +const ( + curlyBracesFmt = `\{(.*?)\}` + alphabetFmt = `[A-Za-z]` + curlyBracesMsg = `must not include curly braces containing alphabetical characters` +) + +var curlyBracesFmtRegexp = regexp.MustCompile(curlyBracesFmt) //nolint:gofumpt +var alphabetFmtRegex = regexp.MustCompile(alphabetFmt) + +func validateCurlyBraces(path string, fieldPath *field.Path) field.ErrorList { + allErrs := field.ErrorList{} + + bracesContents := curlyBracesFmtRegexp.FindAllStringSubmatch(path, -1) + for _, v := range bracesContents { + if alphabetFmtRegex.MatchString(v[1]) { + return append(allErrs, field.Invalid(fieldPath, path, curlyBracesMsg)) + } + } + return allErrs +} + +const ( + escapedStringsFmt = `([^"\\]|\\.)*` + escapedStringsErrMsg = `must have all '"' (double quotes) escaped and must not end with an unescaped '\' (backslash)` +) + +var escapedStringsFmtRegexp = regexp.MustCompile("^" + escapedStringsFmt + "$") + +// ValidateEscapedString validates an escaped string. +func ValidateEscapedString(body string, examples ...string) error { + if !escapedStringsFmtRegexp.MatchString(body) { + msg := validation.RegexError(escapedStringsErrMsg, escapedStringsFmt, examples...) + return fmt.Errorf(msg) + } + return nil +} + +const ( + illegalKeywordFmt = `/etc|/root` + illegalKeywordErrMsg = `must not contain invalid paths` +) + +var illegalKeywordFmtRegexp = regexp.MustCompile("^" + illegalKeywordFmt + "$") + +func validateIllegalKeywords(path string, fieldPath *field.Path) field.ErrorList { + allErrs := field.ErrorList{} + + if illegalKeywordFmtRegexp.MatchString(path) { + return append(allErrs, field.Invalid(fieldPath, path, illegalKeywordErrMsg)) + } + return allErrs } diff --git a/internal/k8s/validation_test.go b/internal/k8s/validation_test.go index 301c9e1e71..4c7163aa67 100644 --- a/internal/k8s/validation_test.go +++ b/internal/k8s/validation_test.go @@ -3335,3 +3335,152 @@ func TestGetSpecServices(t *testing.T) { } } } + +func TestValidateRegexPath(t *testing.T) { + t.Parallel() + tests := []struct { + regexPath string + msg string + }{ + { + regexPath: "~ ^/foo.*\\.jpg", + msg: "case sensitive regexp", + }, + { + regexPath: "~* ^/Bar.*\\.jpg", + msg: "case insensitive regexp", + }, + { + regexPath: `~ ^/f\"oo.*\\.jpg`, + msg: "regexp with escaped double quotes", + }, + { + regexPath: "~ [0-9a-z]{4}[0-9]+", + msg: "regexp with brackets", + }, + } + + for _, test := range tests { + allErrs := validateRegexPath(test.regexPath, field.NewPath("path")) + if len(allErrs) != 0 { + t.Errorf("validateRegexPath(%v) returned errors for valid input for the case of %v", test.regexPath, test.msg) + } + } +} + +func TestValidateRegexPathFails(t *testing.T) { + t.Parallel() + tests := []struct { + regexPath string + msg string + }{ + { + regexPath: "~ [{", + msg: "invalid regexp", + }, + { + regexPath: `~ /foo"`, + msg: "unescaped double quotes", + }, + { + regexPath: `~"`, + msg: "empty regex", + }, + { + regexPath: `~ /foo\`, + msg: "ending in backslash", + }, + } + + for _, test := range tests { + allErrs := validateRegexPath(test.regexPath, field.NewPath("path")) + if len(allErrs) == 0 { + t.Errorf("validateRegexPath(%v) returned no errors for invalid input for the case of %v", test.regexPath, test.msg) + } + } +} + +func TestValidatePath(t *testing.T) { + t.Parallel() + + validPaths := []string{ + "/", + "/path", + "/a-1/_A/", + "/[A-Za-z]{6}/[a-z]{1,2}", + "/[0-9a-z]{4}[0-9]", + } + + for _, path := range validPaths { + allErrs := validatePath(path, field.NewPath("path")) + if len(allErrs) > 0 { + t.Errorf("validatePath(%q) returned errors %v for valid input", path, allErrs) + } + } + + invalidPaths := []string{ + "", + " /", + "/ ", + "/abc;", + `/path\`, + `/path\n`, + `/etc/nginx/secrets`, + } + + for _, path := range invalidPaths { + allErrs := validatePath(path, field.NewPath("path")) + if len(allErrs) == 0 { + t.Errorf("validatePath(%q) returned no errors for invalid input", path) + } + } +} + +func TestValidateCurlyBraces(t *testing.T) { + t.Parallel() + + validPaths := []string{ + "/[a-z]{1,2}", + "/[A-Z]{6}", + "/[A-Z]{6}/[a-z]{1,2}", + "/path", + "/abc}{abc", + } + + for _, path := range validPaths { + allErrs := validateCurlyBraces(path, field.NewPath("path")) + if len(allErrs) > 0 { + t.Errorf("validatePath(%q) returned errors %v for valid input", path, allErrs) + } + } + + invalidPaths := []string{ + "/[A-Z]{a}", + "/{abc}abc", + "/abc{a1}", + } + + for _, path := range invalidPaths { + allErrs := validateCurlyBraces(path, field.NewPath("path")) + if len(allErrs) == 0 { + t.Errorf("validateCurlyBraces(%q) returned no errors for invalid input", path) + } + } +} + +func TestValidateIllegalKeywords(t *testing.T) { + t.Parallel() + + invalidPaths := []string{ + "/root", + "/etc/nginx/secrets", + "/etc/passwd", + } + + for _, path := range invalidPaths { + allErrs := validateIllegalKeywords(path, field.NewPath("path")) + if len(allErrs) == 0 { + t.Errorf("validateCurlyBraces(%q) returned no errors for invalid input", path) + } + } +} diff --git a/pkg/apis/configuration/validation/virtualserver_test.go b/pkg/apis/configuration/validation/virtualserver_test.go index de16fef91a..bf3a0964ea 100644 --- a/pkg/apis/configuration/validation/virtualserver_test.go +++ b/pkg/apis/configuration/validation/virtualserver_test.go @@ -1465,6 +1465,10 @@ func TestValidateRegexPath(t *testing.T) { regexPath: `~ ^/f\"oo.*\\.jpg`, msg: "regexp with escaped double quotes", }, + { + regexPath: "~ [0-9a-z]{4}[0-9]+", + msg: "regexp with brackets", + }, } for _, test := range tests { @@ -1526,6 +1530,8 @@ func TestValidateRoutePath(t *testing.T) { invalidPaths := []string{ "", "invalid", + // regex without preceding "~*" modifier + "^/foo.*\\.jpg", } for _, path := range invalidPaths { From 3744fc1a0eba3575dafb6019d3436227848d5537 Mon Sep 17 00:00:00 2001 From: Haywood Shannon <5781935+haywoodsh@users.noreply.github.com> Date: Fri, 23 Sep 2022 19:18:33 +0100 Subject: [PATCH 2/3] update regex --- internal/k8s/validation.go | 4 +-- internal/k8s/validation_test.go | 34 +++++++++++++------ .../validation/virtualserver_test.go | 2 +- 3 files changed, 27 insertions(+), 13 deletions(-) diff --git a/internal/k8s/validation.go b/internal/k8s/validation.go index ae08b65085..9f7bf98447 100644 --- a/internal/k8s/validation.go +++ b/internal/k8s/validation.go @@ -873,7 +873,7 @@ func validateBackend(backend *networking.IngressBackend, fieldPath *field.Path) } const ( - pathFmt = `/[^\s;\\]*` + pathFmt = `/[^\s;]*` pathErrMsg = "must start with / and must not include any whitespace character or `;`" ) @@ -950,7 +950,7 @@ func ValidateEscapedString(body string, examples ...string) error { } const ( - illegalKeywordFmt = `/etc|/root` + illegalKeywordFmt = `/etc/|/root|/var|\\n|\\r` illegalKeywordErrMsg = `must not contain invalid paths` ) diff --git a/internal/k8s/validation_test.go b/internal/k8s/validation_test.go index 4c7163aa67..1a383e028f 100644 --- a/internal/k8s/validation_test.go +++ b/internal/k8s/validation_test.go @@ -3343,20 +3343,20 @@ func TestValidateRegexPath(t *testing.T) { msg string }{ { - regexPath: "~ ^/foo.*\\.jpg", + regexPath: "/foo.*\\.jpg", msg: "case sensitive regexp", }, { - regexPath: "~* ^/Bar.*\\.jpg", + regexPath: "/Bar.*\\.jpg", msg: "case insensitive regexp", }, { - regexPath: `~ ^/f\"oo.*\\.jpg`, + regexPath: `/f\"oo.*\\.jpg`, msg: "regexp with escaped double quotes", }, { - regexPath: "~ [0-9a-z]{4}[0-9]+", - msg: "regexp with brackets", + regexPath: "/[0-9a-z]{4}[0-9]+", + msg: "regexp with curly braces", }, } @@ -3375,19 +3375,19 @@ func TestValidateRegexPathFails(t *testing.T) { msg string }{ { - regexPath: "~ [{", + regexPath: "[{", msg: "invalid regexp", }, { - regexPath: `~ /foo"`, + regexPath: `/foo"`, msg: "unescaped double quotes", }, { - regexPath: `~"`, + regexPath: `"`, msg: "empty regex", }, { - regexPath: `~ /foo\`, + regexPath: `/foo\`, msg: "ending in backslash", }, } @@ -3409,6 +3409,15 @@ func TestValidatePath(t *testing.T) { "/a-1/_A/", "/[A-Za-z]{6}/[a-z]{1,2}", "/[0-9a-z]{4}[0-9]", + "/foo.*\\.jpg", + "/Bar.*\\.jpg", + `/f\"oo.*\\.jpg`, + "/[0-9a-z]{4}[0-9]+", + "/[a-z]{1,2}", + "/[A-Z]{6}", + "/[A-Z]{6}/[a-z]{1,2}", + "/path", + "/abc}{abc", } for _, path := range validPaths { @@ -3425,7 +3434,9 @@ func TestValidatePath(t *testing.T) { "/abc;", `/path\`, `/path\n`, - `/etc/nginx/secrets`, + `/var/run/secrets`, + "/{autoindex on; root /var/run/secrets;}location /tea", + "/{root}", } for _, path := range invalidPaths { @@ -3475,6 +3486,9 @@ func TestValidateIllegalKeywords(t *testing.T) { "/root", "/etc/nginx/secrets", "/etc/passwd", + "/var/run/secrets", + `\n`, + `\r`, } for _, path := range invalidPaths { diff --git a/pkg/apis/configuration/validation/virtualserver_test.go b/pkg/apis/configuration/validation/virtualserver_test.go index bf3a0964ea..2280e48935 100644 --- a/pkg/apis/configuration/validation/virtualserver_test.go +++ b/pkg/apis/configuration/validation/virtualserver_test.go @@ -1467,7 +1467,7 @@ func TestValidateRegexPath(t *testing.T) { }, { regexPath: "~ [0-9a-z]{4}[0-9]+", - msg: "regexp with brackets", + msg: "regexp with curly braces", }, } From f66148660fcb09c3ceb0d4940282cdbbb27ffacf Mon Sep 17 00:00:00 2001 From: Haywood Shannon <5781935+haywoodsh@users.noreply.github.com> Date: Tue, 27 Sep 2022 18:04:50 +0100 Subject: [PATCH 3/3] linting --- internal/k8s/validation.go | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/internal/k8s/validation.go b/internal/k8s/validation.go index 9f7bf98447..f5f6a5fb8e 100644 --- a/internal/k8s/validation.go +++ b/internal/k8s/validation.go @@ -918,15 +918,17 @@ const ( curlyBracesMsg = `must not include curly braces containing alphabetical characters` ) -var curlyBracesFmtRegexp = regexp.MustCompile(curlyBracesFmt) //nolint:gofumpt -var alphabetFmtRegex = regexp.MustCompile(alphabetFmt) +var ( + curlyBracesFmtRegexp = regexp.MustCompile(curlyBracesFmt) + alphabetFmtRegexp = regexp.MustCompile(alphabetFmt) +) func validateCurlyBraces(path string, fieldPath *field.Path) field.ErrorList { allErrs := field.ErrorList{} bracesContents := curlyBracesFmtRegexp.FindAllStringSubmatch(path, -1) for _, v := range bracesContents { - if alphabetFmtRegex.MatchString(v[1]) { + if alphabetFmtRegexp.MatchString(v[1]) { return append(allErrs, field.Invalid(fieldPath, path, curlyBracesMsg)) } }