From e881884a794bd7870db0b3a8340e2c3fe45b00bd Mon Sep 17 00:00:00 2001
From: Jakub Jarosz <j.jarosz@f5.com>
Date: Thu, 4 May 2023 14:20:39 +0100
Subject: [PATCH 1/9] Fix - add offline_access to the allowed OIDC scope

---
 pkg/apis/configuration/validation/policy.go   |  44 ++----
 .../configuration/validation/policy_test.go   | 147 ++++++++++++++----
 2 files changed, 138 insertions(+), 53 deletions(-)

diff --git a/pkg/apis/configuration/validation/policy.go b/pkg/apis/configuration/validation/policy.go
index c822527d1d..ae033a5bd4 100644
--- a/pkg/apis/configuration/validation/policy.go
+++ b/pkg/apis/configuration/validation/policy.go
@@ -9,6 +9,7 @@ import (
 	"strings"
 
 	v1 "github.com/nginxinc/kubernetes-ingress/pkg/apis/configuration/v1"
+	"golang.org/x/exp/slices"
 	"k8s.io/apimachinery/pkg/util/validation"
 	"k8s.io/apimachinery/pkg/util/validation/field"
 )
@@ -187,17 +188,15 @@ func validateJWT(jwt *v1.JWTAuth, fieldPath *field.Path) field.ErrorList {
 }
 
 func validateBasic(basic *v1.BasicAuth, fieldPath *field.Path) field.ErrorList {
-	allErrs := field.ErrorList{}
+	if basic.Secret == "" {
+		return field.ErrorList{field.Required(fieldPath.Child("secret"), "")}
+	}
 
+	allErrs := field.ErrorList{}
 	if basic.Realm != "" {
 		allErrs = append(allErrs, validateRealm(basic.Realm, fieldPath.Child("realm"))...)
 	}
-
-	if basic.Secret == "" {
-		return append(allErrs, field.Required(fieldPath.Child("secret"), ""))
-	}
 	allErrs = append(allErrs, validateSecretName(basic.Secret, fieldPath.Child("secret"))...)
-
 	return allErrs
 }
 
@@ -237,24 +236,23 @@ func validateEgressMTLS(egressMTLS *v1.EgressMTLS, fieldPath *field.Path) field.
 }
 
 func validateOIDC(oidc *v1.OIDC, fieldPath *field.Path) field.ErrorList {
-	allErrs := field.ErrorList{}
-
 	if oidc.AuthEndpoint == "" {
-		return append(allErrs, field.Required(fieldPath.Child("authEndpoint"), ""))
+		return field.ErrorList{field.Required(fieldPath.Child("authEndpoint"), "")}
 	}
 	if oidc.TokenEndpoint == "" {
-		return append(allErrs, field.Required(fieldPath.Child("tokenEndpoint"), ""))
+		return field.ErrorList{field.Required(fieldPath.Child("tokenEndpoint"), "")}
 	}
 	if oidc.JWKSURI == "" {
-		return append(allErrs, field.Required(fieldPath.Child("jwksURI"), ""))
+		return field.ErrorList{field.Required(fieldPath.Child("jwksURI"), "")}
 	}
 	if oidc.ClientID == "" {
-		return append(allErrs, field.Required(fieldPath.Child("clientID"), ""))
+		return field.ErrorList{field.Required(fieldPath.Child("clientID"), "")}
 	}
 	if oidc.ClientSecret == "" {
-		return append(allErrs, field.Required(fieldPath.Child("clientSecret"), ""))
+		return field.ErrorList{field.Required(fieldPath.Child("clientSecret"), "")}
 	}
 
+	allErrs := field.ErrorList{}
 	if oidc.Scope != "" {
 		allErrs = append(allErrs, validateOIDCScope(oidc.Scope, fieldPath.Child("scope"))...)
 	}
@@ -342,30 +340,22 @@ func validateClientID(client string, fieldPath *field.Path) field.ErrorList {
 	return allErrs
 }
 
-var validScopes = map[string]bool{
-	"openid":  true,
-	"profile": true,
-	"email":   true,
-	"address": true,
-	"phone":   true,
-}
-
 // https://openid.net/specs/openid-connect-core-1_0.html#ScopeClaims
 func validateOIDCScope(scope string, fieldPath *field.Path) field.ErrorList {
-	allErrs := field.ErrorList{}
-
 	if !strings.Contains(scope, "openid") {
-		return append(allErrs, field.Required(fieldPath, "openid scope"))
+		return field.ErrorList{field.Required(fieldPath, "openid scope")}
 	}
 
+	allowedScopes := []string{"openid", "profile", "email", "address", "phone", "offline_access"}
+	msg := fmt.Sprintf("invalid Scope. Accepted scopes are: %v", strings.Join(allowedScopes, ", "))
+
+	allErrs := field.ErrorList{}
 	s := strings.Split(scope, "+")
 	for _, v := range s {
-		if !validScopes[v] {
-			msg := fmt.Sprintf("invalid Scope. Accepted scopes are: %v", mapToPrettyString(validScopes))
+		if !slices.Contains(allowedScopes, v) {
 			allErrs = append(allErrs, field.Invalid(fieldPath, v, msg))
 		}
 	}
-
 	return allErrs
 }
 
diff --git a/pkg/apis/configuration/validation/policy_test.go b/pkg/apis/configuration/validation/policy_test.go
index 48bbf441d6..479754758b 100644
--- a/pkg/apis/configuration/validation/policy_test.go
+++ b/pkg/apis/configuration/validation/policy_test.go
@@ -7,7 +7,7 @@ import (
 	"k8s.io/apimachinery/pkg/util/validation/field"
 )
 
-func TestValidatePolicy(t *testing.T) {
+func TestValidatePolicy_PassesOnValidInput(t *testing.T) {
 	t.Parallel()
 	tests := []struct {
 		policy           *v1.Policy
@@ -84,7 +84,7 @@ func TestValidatePolicy(t *testing.T) {
 	}
 }
 
-func TestValidatePolicyFails(t *testing.T) {
+func TestValidatePolicy_FailsOnInvalidInput(t *testing.T) {
 	t.Parallel()
 	tests := []struct {
 		policy           *v1.Policy
@@ -242,7 +242,7 @@ func TestValidatePolicyFails(t *testing.T) {
 	}
 }
 
-func TestValidateAccessControl(t *testing.T) {
+func TestValidateAccessControl_PassesOnValidInput(t *testing.T) {
 	t.Parallel()
 	validInput := []*v1.AccessControl{
 		{
@@ -267,7 +267,7 @@ func TestValidateAccessControl(t *testing.T) {
 	}
 }
 
-func TestValidateAccessControlFails(t *testing.T) {
+func TestValidateAccessControl_FailsOnInvalidInput(t *testing.T) {
 	t.Parallel()
 	tests := []struct {
 		accessControl *v1.AccessControl
@@ -309,7 +309,7 @@ func TestValidateAccessControlFails(t *testing.T) {
 	}
 }
 
-func TestValidateRateLimit(t *testing.T) {
+func TestValidateRateLimit_PassesOnValidInput(t *testing.T) {
 	t.Parallel()
 	dryRun := true
 	noDelay := false
@@ -362,7 +362,7 @@ func createInvalidRateLimit(f func(r *v1.RateLimit)) *v1.RateLimit {
 	return validRateLimit
 }
 
-func TestValidateRateLimitFails(t *testing.T) {
+func TestValidateRateLimit_FailsOnInvalidInput(t *testing.T) {
 	t.Parallel()
 	tests := []struct {
 		rateLimit *v1.RateLimit
@@ -422,7 +422,7 @@ func TestValidateRateLimitFails(t *testing.T) {
 	}
 }
 
-func TestValidateJWT(t *testing.T) {
+func TestValidateJWT_PassesOnValidInput(t *testing.T) {
 	t.Parallel()
 	tests := []struct {
 		jwt *v1.JWTAuth
@@ -461,7 +461,7 @@ func TestValidateJWT(t *testing.T) {
 	}
 }
 
-func TestValidateJWTFails(t *testing.T) {
+func TestValidateJWT_FailsOnInvalidInput(t *testing.T) {
 	t.Parallel()
 	tests := []struct {
 		msg string
@@ -564,7 +564,7 @@ func TestValidateJWTFails(t *testing.T) {
 	}
 }
 
-func TestValidateIPorCIDR(t *testing.T) {
+func TestValidateIPorCIDR_PassesOnValidInput(t *testing.T) {
 	t.Parallel()
 	validInput := []string{
 		"192.168.1.1",
@@ -579,6 +579,10 @@ func TestValidateIPorCIDR(t *testing.T) {
 			t.Errorf("validateIPorCIDR(%q) returned errors %v for valid input", input, allErrs)
 		}
 	}
+}
+
+func TestValidateIPorCIDR_FailsOnInvalidInput(t *testing.T) {
+	t.Parallel()
 
 	invalidInput := []string{
 		"localhost",
@@ -595,7 +599,7 @@ func TestValidateIPorCIDR(t *testing.T) {
 	}
 }
 
-func TestValidateRate(t *testing.T) {
+func TestValidateRate_PassesOnValidInput(t *testing.T) {
 	t.Parallel()
 	validInput := []string{
 		"10r/s",
@@ -609,7 +613,10 @@ func TestValidateRate(t *testing.T) {
 			t.Errorf("validateRate(%q) returned errors %v for valid input", input, allErrs)
 		}
 	}
+}
 
+func TestValidateRate_FailsOnInvalidInput(t *testing.T) {
+	t.Parallel()
 	invalidInput := []string{
 		"10s",
 		"10r/",
@@ -625,7 +632,7 @@ func TestValidateRate(t *testing.T) {
 	}
 }
 
-func TestValidatePositiveInt(t *testing.T) {
+func TestValidatePositiveInt_PassesOnValidInput(t *testing.T) {
 	t.Parallel()
 	validInput := []int{1, 2}
 
@@ -635,6 +642,10 @@ func TestValidatePositiveInt(t *testing.T) {
 			t.Errorf("validatePositiveInt(%q) returned errors %v for valid input", input, allErrs)
 		}
 	}
+}
+
+func TestValidatePositiveInt_FailsOnInvalidInput(t *testing.T) {
+	t.Parallel()
 
 	invalidInput := []int{-1, 0}
 
@@ -646,7 +657,7 @@ func TestValidatePositiveInt(t *testing.T) {
 	}
 }
 
-func TestValidateRateLimitZoneSize(t *testing.T) {
+func TestValidateRateLimitZoneSize_PassesOnValidInput(t *testing.T) {
 	t.Parallel()
 	validInput := []string{"32", "32k", "32K", "10m"}
 
@@ -667,8 +678,21 @@ func TestValidateRateLimitZoneSize(t *testing.T) {
 	}
 }
 
-func TestValidateRateLimitLogLevel(t *testing.T) {
+func TestValidateRateLimitZoneSize_FailsOnInvalidInput(t *testing.T) {
+	t.Parallel()
+	invalidInput := []string{"", "31", "31k", "0", "0M"}
+
+	for _, test := range invalidInput {
+		allErrs := validateRateLimitZoneSize(test, field.NewPath("size"))
+		if len(allErrs) == 0 {
+			t.Errorf("validateRateLimitZoneSize(%q) didn't return error for invalid input", test)
+		}
+	}
+}
+
+func TestValidateRateLimitLogLevel_PassesOnValidInput(t *testing.T) {
 	t.Parallel()
+
 	validInput := []string{"error", "info", "warn", "notice"}
 
 	for _, test := range validInput {
@@ -677,6 +701,10 @@ func TestValidateRateLimitLogLevel(t *testing.T) {
 			t.Errorf("validateRateLimitLogLevel(%q) returned an error for valid input", test)
 		}
 	}
+}
+
+func TestValidateRateLimitLogLevel_FailsOnInvalidInput(t *testing.T) {
+	t.Parallel()
 
 	invalidInput := []string{"warn ", "info error", ""}
 
@@ -688,7 +716,7 @@ func TestValidateRateLimitLogLevel(t *testing.T) {
 	}
 }
 
-func TestValidateJWTToken(t *testing.T) {
+func TestValidateJWTToken_PassesOnValidInput(t *testing.T) {
 	t.Parallel()
 	validTests := []struct {
 		token string
@@ -717,7 +745,10 @@ func TestValidateJWTToken(t *testing.T) {
 			t.Errorf("validateJWTToken(%v) returned an error for valid input for the case of %v", test.token, test.msg)
 		}
 	}
+}
 
+func TestValidateJWTToken_FailsOnInvalidInput(t *testing.T) {
+	t.Parallel()
 	invalidTests := []struct {
 		token string
 		msg   string
@@ -751,7 +782,7 @@ func TestValidateJWTToken(t *testing.T) {
 	}
 }
 
-func TestValidateIngressMTLS(t *testing.T) {
+func TestValidateIngressMTLS_PassesOnValidInput(t *testing.T) {
 	t.Parallel()
 	tests := []struct {
 		ing *v1.IngressMTLS
@@ -788,7 +819,7 @@ func TestValidateIngressMTLS(t *testing.T) {
 	}
 }
 
-func TestValidateIngressMTLSInvalid(t *testing.T) {
+func TestValidateIngressMTLS_FailsOnInvalidInput(t *testing.T) {
 	t.Parallel()
 	tests := []struct {
 		ing *v1.IngressMTLS
@@ -830,7 +861,7 @@ func TestValidateIngressMTLSInvalid(t *testing.T) {
 	}
 }
 
-func TestValidateIngressMTLSVerifyClient(t *testing.T) {
+func TestValidateIngressMTLSVerifyClient_PassesOnValidInput(t *testing.T) {
 	t.Parallel()
 	validInput := []string{"on", "off", "optional", "optional_no_ca"}
 
@@ -840,7 +871,10 @@ func TestValidateIngressMTLSVerifyClient(t *testing.T) {
 			t.Errorf("validateIngressMTLSVerifyClient(%q) returned errors %v for valid input", allErrs, test)
 		}
 	}
+}
 
+func TestValidateIngressMTLSVerifyClient_FailsOnInvalidInput(t *testing.T) {
+	t.Parallel()
 	invalidInput := []string{"true", "false"}
 
 	for _, test := range invalidInput {
@@ -851,7 +885,7 @@ func TestValidateIngressMTLSVerifyClient(t *testing.T) {
 	}
 }
 
-func TestValidateEgressMTLS(t *testing.T) {
+func TestValidateEgressMTLS_PassesOnValidInput(t *testing.T) {
 	t.Parallel()
 	tests := []struct {
 		eg  *v1.EgressMTLS
@@ -893,7 +927,7 @@ func TestValidateEgressMTLS(t *testing.T) {
 	}
 }
 
-func TestValidateEgressMTLSInvalid(t *testing.T) {
+func TestValidateEgressMTLS_FailsOnInvalidInput(t *testing.T) {
 	t.Parallel()
 	tests := []struct {
 		eg  *v1.EgressMTLS
@@ -935,7 +969,7 @@ func TestValidateEgressMTLSInvalid(t *testing.T) {
 	}
 }
 
-func TestValidateOIDCValid(t *testing.T) {
+func TestValidateOIDC_PassesOnValidOIDC(t *testing.T) {
 	t.Parallel()
 	tests := []struct {
 		oidc *v1.OIDC
@@ -994,6 +1028,18 @@ func TestValidateOIDCValid(t *testing.T) {
 			},
 			msg: "ip address",
 		},
+		{
+			oidc: &v1.OIDC{
+				AuthEndpoint:      "http://127.0.0.1:8080/auth/realms/master/protocol/openid-connect/auth",
+				TokenEndpoint:     "http://127.0.0.1:8080/auth/realms/master/protocol/openid-connect/token",
+				JWKSURI:           "http://127.0.0.1:8080/auth/realms/master/protocol/openid-connect/certs",
+				ClientID:          "client",
+				ClientSecret:      "secret",
+				Scope:             "openid+offline_access",
+				AccessTokenEnable: true,
+			},
+			msg: "offline access scope",
+		},
 	}
 
 	for _, test := range tests {
@@ -1004,7 +1050,7 @@ func TestValidateOIDCValid(t *testing.T) {
 	}
 }
 
-func TestValidateOIDCInvalid(t *testing.T) {
+func TestValidateOIDC_FailsOnInvalidOIDC(t *testing.T) {
 	t.Parallel()
 	tests := []struct {
 		oidc *v1.OIDC
@@ -1016,6 +1062,18 @@ func TestValidateOIDCInvalid(t *testing.T) {
 			},
 			msg: "missing required field auth",
 		},
+		{
+			oidc: &v1.OIDC{
+				AuthEndpoint:      "http://127.0.0.1:8080/auth/realms/master/protocol/openid-connect/auth",
+				TokenEndpoint:     "http://127.0.0.1:8080/auth/realms/master/protocol/openid-connect/token",
+				JWKSURI:           "http://127.0.0.1:8080/auth/realms/master/protocol/openid-connect/certs",
+				ClientID:          "client",
+				ClientSecret:      "secret",
+				Scope:             "bogus",
+				AccessTokenEnable: true,
+			},
+			msg: "missing openid in scope",
+		},
 		{
 			oidc: &v1.OIDC{
 				AuthEndpoint:      "https://login.microsoftonline.com/dd-fff-eee-1234-9be/oauth2/v2.0/authorize",
@@ -1132,8 +1190,9 @@ func TestValidateOIDCInvalid(t *testing.T) {
 	}
 }
 
-func TestValidateClientID(t *testing.T) {
+func TestValidateClientID_PassesOnValidInput(t *testing.T) {
 	t.Parallel()
+
 	validInput := []string{"myid", "your.id", "id-sf-sjfdj.com", "foo_bar~vni"}
 
 	for _, test := range validInput {
@@ -1142,7 +1201,10 @@ func TestValidateClientID(t *testing.T) {
 			t.Errorf("validateClientID(%q) returned errors %v for valid input", allErrs, test)
 		}
 	}
+}
 
+func TestValidateClientID_FailsOnInvalidInput(t *testing.T) {
+	t.Parallel()
 	invalidInput := []string{"$boo", "foo$bar", `foo_bar"vni`, `client\`}
 
 	for _, test := range invalidInput {
@@ -1153,8 +1215,9 @@ func TestValidateClientID(t *testing.T) {
 	}
 }
 
-func TestValidateOIDCScope(t *testing.T) {
+func TestValidateOIDCScope_PassesOnValidInput(t *testing.T) {
 	t.Parallel()
+
 	validInput := []string{"openid", "openid+profile", "openid+email", "openid+phone"}
 
 	for _, test := range validInput {
@@ -1163,6 +1226,10 @@ func TestValidateOIDCScope(t *testing.T) {
 			t.Errorf("validateOIDCScope(%q) returned errors %v for valid input", allErrs, test)
 		}
 	}
+}
+
+func TestValidateOIDCScope_FailsOnInvalidInput(t *testing.T) {
+	t.Parallel()
 
 	invalidInput := []string{"profile", "openid+web", `openid+foobar.com`}
 
@@ -1174,8 +1241,9 @@ func TestValidateOIDCScope(t *testing.T) {
 	}
 }
 
-func TestValidateURL(t *testing.T) {
+func TestValidateURL_PassesOnValidInput(t *testing.T) {
 	t.Parallel()
+
 	validInput := []string{"http://google.com/auth", "https://foo.bar/baz", "http://127.0.0.1/bar", "http://openid.connect.com:8080/foo"}
 
 	for _, test := range validInput {
@@ -1184,6 +1252,10 @@ func TestValidateURL(t *testing.T) {
 			t.Errorf("validateURL(%q) returned errors %v for valid input", allErrs, test)
 		}
 	}
+}
+
+func TestValidateURL_FailsOnInvalidInput(t *testing.T) {
+	t.Parallel()
 
 	invalidInput := []string{"www.google..foo.com", "http://{foo.bar", `https://google.foo\bar`, "http://foo.bar:8080", "http://foo.bar:812345/fooo"}
 
@@ -1195,8 +1267,9 @@ func TestValidateURL(t *testing.T) {
 	}
 }
 
-func TestValidateQueryStringt(t *testing.T) {
+func TestValidateQueryString_PassesOnValidInput(t *testing.T) {
 	t.Parallel()
+
 	validInput := []string{"foo=bar", "foo", "foo=bar&baz=zot", "foo=bar&foo=baz", "foo=bar%3Bbaz"}
 
 	for _, test := range validInput {
@@ -1205,6 +1278,10 @@ func TestValidateQueryStringt(t *testing.T) {
 			t.Errorf("validateQueryString(%q) returned errors %v for valid input", allErrs, test)
 		}
 	}
+}
+
+func TestValidateQueryString_FailsOnInvalidInput(t *testing.T) {
+	t.Parallel()
 
 	invalidInput := []string{"foo=bar;baz"}
 
@@ -1216,7 +1293,7 @@ func TestValidateQueryStringt(t *testing.T) {
 	}
 }
 
-func TestValidateWAF(t *testing.T) {
+func TestValidateWAF_PassesOnValidInput(t *testing.T) {
 	t.Parallel()
 	tests := []struct {
 		waf *v1.WAF
@@ -1356,3 +1433,21 @@ func TestValidateWAF_FailsOnInvalidApPolicy(t *testing.T) {
 		}
 	}
 }
+
+func TestValidateBasic_PassesOnNotEmptySecret(t *testing.T) {
+	t.Parallel()
+
+	errList := validateBasic(&v1.BasicAuth{Realm: "", Secret: "secret"}, field.NewPath("secret"))
+	if len(errList) != 0 {
+		t.Errorf("want no errors, got %v", errList)
+	}
+}
+
+func TestValidateBasic_FailsOnMissingSecret(t *testing.T) {
+	t.Parallel()
+
+	errList := validateBasic(&v1.BasicAuth{Realm: "realm", Secret: ""}, field.NewPath("secret"))
+	if len(errList) == 0 {
+		t.Error("want error on invalid input")
+	}
+}

From cf817c116c9d0b483a0b4741174516b4f9c8c137 Mon Sep 17 00:00:00 2001
From: Jakub Jarosz <j.jarosz@f5.com>
Date: Thu, 4 May 2023 14:25:19 +0100
Subject: [PATCH 2/9] Update valid scopes

---
 pkg/apis/configuration/validation/policy.go | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/pkg/apis/configuration/validation/policy.go b/pkg/apis/configuration/validation/policy.go
index ae033a5bd4..dc603fa6d0 100644
--- a/pkg/apis/configuration/validation/policy.go
+++ b/pkg/apis/configuration/validation/policy.go
@@ -346,13 +346,13 @@ func validateOIDCScope(scope string, fieldPath *field.Path) field.ErrorList {
 		return field.ErrorList{field.Required(fieldPath, "openid scope")}
 	}
 
-	allowedScopes := []string{"openid", "profile", "email", "address", "phone", "offline_access"}
-	msg := fmt.Sprintf("invalid Scope. Accepted scopes are: %v", strings.Join(allowedScopes, ", "))
+	validScopes := []string{"openid", "profile", "email", "address", "phone", "offline_access"}
+	msg := fmt.Sprintf("invalid Scope. Accepted scopes are: %v", strings.Join(validScopes, ", "))
 
 	allErrs := field.ErrorList{}
 	s := strings.Split(scope, "+")
 	for _, v := range s {
-		if !slices.Contains(allowedScopes, v) {
+		if !slices.Contains(validScopes, v) {
 			allErrs = append(allErrs, field.Invalid(fieldPath, v, msg))
 		}
 	}

From b456b507fb1072793aff7abeb8a26f131aa34002 Mon Sep 17 00:00:00 2001
From: Jakub Jarosz <j.jarosz@f5.com>
Date: Thu, 4 May 2023 17:05:06 +0100
Subject: [PATCH 3/9] Update validators

---
 pkg/apis/configuration/validation/common.go |  51 +++----
 pkg/apis/configuration/validation/policy.go | 140 ++++++--------------
 2 files changed, 59 insertions(+), 132 deletions(-)

diff --git a/pkg/apis/configuration/validation/common.go b/pkg/apis/configuration/validation/common.go
index ed6dfc8106..46937c2f9c 100644
--- a/pkg/apis/configuration/validation/common.go
+++ b/pkg/apis/configuration/validation/common.go
@@ -27,13 +27,11 @@ func ValidateEscapedString(body string, examples ...string) error {
 }
 
 func validateVariable(nVar string, validVars map[string]bool, fieldPath *field.Path) field.ErrorList {
-	allErrs := field.ErrorList{}
-
 	if !validVars[nVar] {
 		msg := fmt.Sprintf("'%v' contains an invalid NGINX variable. Accepted variables are: %v", nVar, mapToPrettyString(validVars))
-		allErrs = append(allErrs, field.Invalid(fieldPath, nVar, msg))
+		return field.ErrorList{field.Invalid(fieldPath, nVar, msg)}
 	}
-	return allErrs
+	return nil
 }
 
 // isValidSpecialHeaderLikeVariable validates special variables $http_, $jwt_header_, $jwt_claim_
@@ -103,12 +101,11 @@ func validateSpecialVariable(nVar string, fieldPath *field.Path, isPlus bool) fi
 }
 
 func validateStringWithVariables(str string, fieldPath *field.Path, specialVars []string, validVars map[string]bool, isPlus bool) field.ErrorList {
-	allErrs := field.ErrorList{}
-
 	if strings.HasSuffix(str, "$") {
-		return append(allErrs, field.Invalid(fieldPath, str, "must not end with $"))
+		return field.ErrorList{field.Invalid(fieldPath, str, "must not end with $")}
 	}
 
+	allErrs := field.ErrorList{}
 	for i, c := range str {
 		if c == '$' {
 			msg := "variables must be enclosed in curly braces, for example ${host}"
@@ -144,67 +141,55 @@ func validateStringWithVariables(str string, fieldPath *field.Path, specialVars
 }
 
 func validateTime(time string, fieldPath *field.Path) field.ErrorList {
-	allErrs := field.ErrorList{}
-
 	if time == "" {
-		return allErrs
+		return nil
 	}
-
 	if _, err := configs.ParseTime(time); err != nil {
-		return append(allErrs, field.Invalid(fieldPath, time, err.Error()))
+		return field.ErrorList{field.Invalid(fieldPath, time, err.Error())}
 	}
-
-	return allErrs
+	return nil
 }
 
 // http://nginx.org/en/docs/syntax.html
 const offsetErrMsg = "must consist of numeric characters followed by a valid size suffix. 'k|K|m|M|g|G"
 
 func validateOffset(offset string, fieldPath *field.Path) field.ErrorList {
-	allErrs := field.ErrorList{}
-
 	if offset == "" {
-		return allErrs
+		return nil
 	}
-
 	if _, err := configs.ParseOffset(offset); err != nil {
 		msg := validation.RegexError(offsetErrMsg, configs.OffsetFmt, "16", "32k", "64M", "2G")
-		return append(allErrs, field.Invalid(fieldPath, offset, msg))
+		return field.ErrorList{field.Invalid(fieldPath, offset, msg)}
 	}
-
-	return allErrs
+	return nil
 }
 
 // http://nginx.org/en/docs/syntax.html
 const sizeErrMsg = "must consist of numeric characters followed by a valid size suffix. 'k|K|m|M"
 
 func validateSize(size string, fieldPath *field.Path) field.ErrorList {
-	allErrs := field.ErrorList{}
-
 	if size == "" {
-		return allErrs
+		return nil
 	}
 
 	if _, err := configs.ParseSize(size); err != nil {
 		msg := validation.RegexError(sizeErrMsg, configs.SizeFmt, "16", "32k", "64M")
-		return append(allErrs, field.Invalid(fieldPath, size, msg))
+		return field.ErrorList{field.Invalid(fieldPath, size, msg)}
 	}
-	return allErrs
+	return nil
 }
 
 // validateSecretName checks if a secret name is valid.
 // It performs the same validation as ValidateSecretName from k8s.io/kubernetes/pkg/apis/core/validation/validation.go.
 func validateSecretName(name string, fieldPath *field.Path) field.ErrorList {
-	allErrs := field.ErrorList{}
-
 	if name == "" {
-		return allErrs
+		return nil
 	}
 
+	allErrs := field.ErrorList{}
 	for _, msg := range validation.IsDNS1123Subdomain(name) {
 		allErrs = append(allErrs, field.Invalid(fieldPath, name, msg))
 	}
-
 	return allErrs
 }
 
@@ -220,11 +205,9 @@ func mapToPrettyString(m map[string]bool) string {
 
 // ValidateParameter validates a parameter against a map of valid parameters for the directive
 func ValidateParameter(nPar string, validParams map[string]bool, fieldPath *field.Path) field.ErrorList {
-	allErrs := field.ErrorList{}
-
 	if !validParams[nPar] {
 		msg := fmt.Sprintf("'%v' contains an invalid NGINX parameter. Accepted parameters are: %v", nPar, mapToPrettyString(validParams))
-		allErrs = append(allErrs, field.Invalid(fieldPath, nPar, msg))
+		return field.ErrorList{field.Invalid(fieldPath, nPar, msg)}
 	}
-	return allErrs
+	return nil
 }
diff --git a/pkg/apis/configuration/validation/policy.go b/pkg/apis/configuration/validation/policy.go
index dc603fa6d0..deb8bd949a 100644
--- a/pkg/apis/configuration/validation/policy.go
+++ b/pkg/apis/configuration/validation/policy.go
@@ -123,9 +123,7 @@ func validateAccessControl(accessControl *v1.AccessControl, fieldPath *field.Pat
 }
 
 func validateRateLimit(rateLimit *v1.RateLimit, fieldPath *field.Path, isPlus bool) field.ErrorList {
-	allErrs := field.ErrorList{}
-
-	allErrs = append(allErrs, validateRateLimitZoneSize(rateLimit.ZoneSize, fieldPath.Child("zoneSize"))...)
+	allErrs := validateRateLimitZoneSize(rateLimit.ZoneSize, fieldPath.Child("zoneSize"))
 	allErrs = append(allErrs, validateRate(rateLimit.Rate, fieldPath.Child("rate"))...)
 	allErrs = append(allErrs, validateRateLimitKey(rateLimit.Key, fieldPath.Child("key"), isPlus)...)
 
@@ -196,18 +194,14 @@ func validateBasic(basic *v1.BasicAuth, fieldPath *field.Path) field.ErrorList {
 	if basic.Realm != "" {
 		allErrs = append(allErrs, validateRealm(basic.Realm, fieldPath.Child("realm"))...)
 	}
-	allErrs = append(allErrs, validateSecretName(basic.Secret, fieldPath.Child("secret"))...)
-	return allErrs
+	return append(allErrs, validateSecretName(basic.Secret, fieldPath.Child("secret"))...)
 }
 
 func validateIngressMTLS(ingressMTLS *v1.IngressMTLS, fieldPath *field.Path) field.ErrorList {
-	allErrs := field.ErrorList{}
-
 	if ingressMTLS.ClientCertSecret == "" {
-		return append(allErrs, field.Required(fieldPath.Child("clientCertSecret"), ""))
+		return field.ErrorList{field.Required(fieldPath.Child("clientCertSecret"), "")}
 	}
-	allErrs = append(allErrs, validateSecretName(ingressMTLS.ClientCertSecret, fieldPath.Child("clientCertSecret"))...)
-
+	allErrs := validateSecretName(ingressMTLS.ClientCertSecret, fieldPath.Child("clientCertSecret"))
 	allErrs = append(allErrs, validateIngressMTLSVerifyClient(ingressMTLS.VerifyClient, fieldPath.Child("verifyClient"))...)
 
 	if ingressMTLS.VerifyDepth != nil {
@@ -217,9 +211,7 @@ func validateIngressMTLS(ingressMTLS *v1.IngressMTLS, fieldPath *field.Path) fie
 }
 
 func validateEgressMTLS(egressMTLS *v1.EgressMTLS, fieldPath *field.Path) field.ErrorList {
-	allErrs := field.ErrorList{}
-
-	allErrs = append(allErrs, validateSecretName(egressMTLS.TLSSecret, fieldPath.Child("tlsSecret"))...)
+	allErrs := validateSecretName(egressMTLS.TLSSecret, fieldPath.Child("tlsSecret"))
 
 	if egressMTLS.VerifyServer && egressMTLS.TrustedCertSecret == "" {
 		return append(allErrs, field.Required(fieldPath.Child("trustedCertSecret"), "must be set when verifyServer is 'true'"))
@@ -229,10 +221,7 @@ func validateEgressMTLS(egressMTLS *v1.EgressMTLS, fieldPath *field.Path) field.
 	if egressMTLS.VerifyDepth != nil {
 		allErrs = append(allErrs, validatePositiveIntOrZero(*egressMTLS.VerifyDepth, fieldPath.Child("verifyDepth"))...)
 	}
-
-	allErrs = append(allErrs, validateSSLName(egressMTLS.SSLName, fieldPath.Child("sslName"))...)
-
-	return allErrs
+	return append(allErrs, validateSSLName(egressMTLS.SSLName, fieldPath.Child("sslName"))...)
 }
 
 func validateOIDC(oidc *v1.OIDC, fieldPath *field.Path) field.ErrorList {
@@ -273,9 +262,7 @@ func validateOIDC(oidc *v1.OIDC, fieldPath *field.Path) field.ErrorList {
 	allErrs = append(allErrs, validateURL(oidc.TokenEndpoint, fieldPath.Child("tokenEndpoint"))...)
 	allErrs = append(allErrs, validateURL(oidc.JWKSURI, fieldPath.Child("jwksURI"))...)
 	allErrs = append(allErrs, validateSecretName(oidc.ClientSecret, fieldPath.Child("clientSecret"))...)
-	allErrs = append(allErrs, validateClientID(oidc.ClientID, fieldPath.Child("clientID"))...)
-
-	return allErrs
+	return append(allErrs, validateClientID(oidc.ClientID, fieldPath.Child("clientID"))...)
 }
 
 func validateWAF(waf *v1.WAF, fieldPath *field.Path) field.ErrorList {
@@ -305,7 +292,6 @@ func validateWAF(waf *v1.WAF, fieldPath *field.Path) field.ErrorList {
 	if waf.SecurityLog != nil {
 		allErrs = append(allErrs, validateLogConf(waf.SecurityLog.ApLogConf, waf.SecurityLog.LogDest, fieldPath.Child("securityLog"))...)
 	}
-
 	return allErrs
 }
 
@@ -326,18 +312,15 @@ func validateLogConf(logConf, logDest string, fieldPath *field.Path) field.Error
 }
 
 func validateClientID(client string, fieldPath *field.Path) field.ErrorList {
-	allErrs := field.ErrorList{}
-
 	// isValidHeaderValue checks for $ and " in the string
 	if isValidHeaderValue(client) != nil {
-		allErrs = append(allErrs, field.Invalid(
+		return field.ErrorList{field.Invalid(
 			fieldPath,
 			client,
 			`invalid string. String must contain valid ASCII characters, must have all '"' escaped and must not contain any '$' or end with an unescaped '\'
-		`))
+		`)}
 	}
-
-	return allErrs
+	return nil
 }
 
 // https://openid.net/specs/openid-connect-core-1_0.html#ScopeClaims
@@ -360,24 +343,18 @@ func validateOIDCScope(scope string, fieldPath *field.Path) field.ErrorList {
 }
 
 func validateURL(name string, fieldPath *field.Path) field.ErrorList {
-	allErrs := field.ErrorList{}
-
 	u, err := url.Parse(name)
 	if err != nil {
-		return append(allErrs, field.Invalid(fieldPath, name, err.Error()))
+		return field.ErrorList{field.Invalid(fieldPath, name, err.Error())}
 	}
-	var msg string
 	if u.Scheme == "" {
-		msg = "scheme required, please use the prefix http(s)://"
-		return append(allErrs, field.Invalid(fieldPath, name, msg))
+		return field.ErrorList{field.Invalid(fieldPath, name, "scheme required, please use the prefix http(s)://")}
 	}
 	if u.Host == "" {
-		msg = "hostname required"
-		return append(allErrs, field.Invalid(fieldPath, name, msg))
+		return field.ErrorList{field.Invalid(fieldPath, name, "hostname required")}
 	}
 	if u.Path == "" {
-		msg = "path required"
-		return append(allErrs, field.Invalid(fieldPath, name, msg))
+		return field.ErrorList{field.Invalid(fieldPath, name, "path required")}
 	}
 
 	host, port, err := net.SplitHostPort(u.Host)
@@ -385,33 +362,28 @@ func validateURL(name string, fieldPath *field.Path) field.ErrorList {
 		host = u.Host
 	}
 
-	allErrs = append(allErrs, validateSSLName(host, fieldPath)...)
+	allErrs := validateSSLName(host, fieldPath)
 	if port != "" {
 		allErrs = append(allErrs, validatePortNumber(port, fieldPath)...)
 	}
-
 	return allErrs
 }
 
 func validateQueryString(queryString string, fieldPath *field.Path) field.ErrorList {
-	allErrs := field.ErrorList{}
-
 	_, err := url.ParseQuery(queryString)
 	if err != nil {
-		return append(allErrs, field.Invalid(fieldPath, queryString, err.Error()))
+		return field.ErrorList{field.Invalid(fieldPath, queryString, err.Error())}
 	}
-
-	return allErrs
+	return nil
 }
 
 func validatePortNumber(port string, fieldPath *field.Path) field.ErrorList {
-	allErrs := field.ErrorList{}
 	portInt, _ := strconv.Atoi(port)
 	msg := validation.IsValidPortNum(portInt)
 	if msg != nil {
-		allErrs = append(allErrs, field.Invalid(fieldPath, port, msg[0]))
+		return field.ErrorList{field.Invalid(fieldPath, port, msg[0])}
 	}
-	return allErrs
+	return nil
 }
 
 func validateSSLName(name string, fieldPath *field.Path) field.ErrorList {
@@ -433,11 +405,10 @@ var validateVerifyClientKeyParameters = map[string]bool{
 }
 
 func validateIngressMTLSVerifyClient(verifyClient string, fieldPath *field.Path) field.ErrorList {
-	allErrs := field.ErrorList{}
 	if verifyClient != "" {
-		allErrs = append(allErrs, ValidateParameter(verifyClient, validateVerifyClientKeyParameters, fieldPath)...)
+		return ValidateParameter(verifyClient, validateVerifyClientKeyParameters, fieldPath)
 	}
-	return allErrs
+	return nil
 }
 
 const (
@@ -448,27 +419,21 @@ const (
 var rateRegexp = regexp.MustCompile("^" + rateFmt + "$")
 
 func validateRate(rate string, fieldPath *field.Path) field.ErrorList {
-	allErrs := field.ErrorList{}
-
 	if rate == "" {
-		return append(allErrs, field.Required(fieldPath, ""))
+		return field.ErrorList{field.Required(fieldPath, "")}
 	}
-
 	if !rateRegexp.MatchString(rate) {
 		msg := validation.RegexError(rateErrMsg, rateFmt, "16r/s", "32r/m", "64r/s")
-		return append(allErrs, field.Invalid(fieldPath, rate, msg))
+		return field.ErrorList{field.Invalid(fieldPath, rate, msg)}
 	}
-	return allErrs
+	return nil
 }
 
 func validateRateLimitZoneSize(zoneSize string, fieldPath *field.Path) field.ErrorList {
-	allErrs := field.ErrorList{}
-
 	if zoneSize == "" {
-		return append(allErrs, field.Required(fieldPath, ""))
+		return field.ErrorList{field.Required(fieldPath, "")}
 	}
-
-	allErrs = append(allErrs, validateSize(zoneSize, fieldPath)...)
+	allErrs := validateSize(zoneSize, fieldPath)
 
 	kbZoneSize := strings.TrimSuffix(strings.ToLower(zoneSize), "k")
 	kbZoneSizeNum, err := strconv.Atoi(kbZoneSize)
@@ -479,7 +444,6 @@ func validateRateLimitZoneSize(zoneSize string, fieldPath *field.Path) field.Err
 	if err == nil && kbZoneSizeNum < 32 || mbErr == nil && mbZoneSizeNum == 0 {
 		allErrs = append(allErrs, field.Invalid(fieldPath, zoneSize, "must be greater than 31k"))
 	}
-
 	return allErrs
 }
 
@@ -494,33 +458,26 @@ var rateLimitKeyVariables = map[string]bool{
 }
 
 func validateRateLimitKey(key string, fieldPath *field.Path, isPlus bool) field.ErrorList {
-	allErrs := field.ErrorList{}
-
 	if key == "" {
-		return append(allErrs, field.Required(fieldPath, ""))
+		return field.ErrorList{field.Required(fieldPath, "")}
 	}
-
+	allErrs := field.ErrorList{}
 	if err := ValidateEscapedString(key, `Hello World! \n`, `\"${request_uri}\" is unavailable. \n`); err != nil {
 		allErrs = append(allErrs, field.Invalid(fieldPath, key, err.Error()))
 	}
-
-	allErrs = append(allErrs, validateStringWithVariables(key, fieldPath, rateLimitKeySpecialVariables, rateLimitKeyVariables, isPlus)...)
-
-	return allErrs
+	return append(allErrs, validateStringWithVariables(key, fieldPath, rateLimitKeySpecialVariables, rateLimitKeyVariables, isPlus)...)
 }
 
 var jwtTokenSpecialVariables = []string{"arg_", "http_", "cookie_"}
 
 func validateJWTToken(token string, fieldPath *field.Path) field.ErrorList {
-	allErrs := field.ErrorList{}
-
 	if token == "" {
-		return allErrs
+		return nil
 	}
 
 	nginxVars := strings.Split(token, "$")
 	if len(nginxVars) != 2 {
-		return append(allErrs, field.Invalid(fieldPath, token, "must have 1 var"))
+		return field.ErrorList{field.Invalid(fieldPath, token, "must have 1 var")}
 	}
 	nVar := token[1:]
 
@@ -532,6 +489,7 @@ func validateJWTToken(token string, fieldPath *field.Path) field.ErrorList {
 		}
 	}
 
+	allErrs := field.ErrorList{}
 	if special {
 		// validateJWTToken is called only when NGINX Plus is running
 		isPlus := true
@@ -539,7 +497,6 @@ func validateJWTToken(token string, fieldPath *field.Path) field.ErrorList {
 	} else {
 		return append(allErrs, field.Invalid(fieldPath, token, "must only have special vars"))
 	}
-
 	return allErrs
 }
 
@@ -551,14 +508,11 @@ var validLogLevels = map[string]bool{
 }
 
 func validateRateLimitLogLevel(logLevel string, fieldPath *field.Path) field.ErrorList {
-	allErrs := field.ErrorList{}
-
 	if !validLogLevels[logLevel] {
-		allErrs = append(allErrs, field.Invalid(fieldPath, logLevel, fmt.Sprintf("Accepted values: %s",
-			mapToPrettyString(validLogLevels))))
+		return field.ErrorList{field.Invalid(fieldPath, logLevel, fmt.Sprintf("Accepted values: %s",
+			mapToPrettyString(validLogLevels)))}
 	}
-
-	return allErrs
+	return nil
 }
 
 const (
@@ -569,40 +523,30 @@ const (
 var realmFmtRegexp = regexp.MustCompile("^" + realmFmt + "$")
 
 func validateRealm(realm string, fieldPath *field.Path) field.ErrorList {
-	allErrs := field.ErrorList{}
-
 	if !realmFmtRegexp.MatchString(realm) {
 		msg := validation.RegexError(realmFmtErrMsg, realmFmt, "MyAPI", "My Product API")
-		allErrs = append(allErrs, field.Invalid(fieldPath, realm, msg))
+		return field.ErrorList{field.Invalid(fieldPath, realm, msg)}
 	}
-
-	return allErrs
+	return nil
 }
 
 func validateIPorCIDR(ipOrCIDR string, fieldPath *field.Path) field.ErrorList {
-	allErrs := field.ErrorList{}
-
 	_, _, err := net.ParseCIDR(ipOrCIDR)
 	if err == nil {
 		// valid CIDR
-		return allErrs
+		return nil
 	}
-
 	ip := net.ParseIP(ipOrCIDR)
 	if ip != nil {
 		// valid IP
-		return allErrs
+		return nil
 	}
-
-	return append(allErrs, field.Invalid(fieldPath, ipOrCIDR, "must be a CIDR or IP"))
+	return field.ErrorList{field.Invalid(fieldPath, ipOrCIDR, "must be a CIDR or IP")}
 }
 
 func validatePositiveInt(n int, fieldPath *field.Path) field.ErrorList {
-	allErrs := field.ErrorList{}
-
 	if n <= 0 {
-		return append(allErrs, field.Invalid(fieldPath, n, "must be positive"))
+		return field.ErrorList{field.Invalid(fieldPath, n, "must be positive")}
 	}
-
-	return allErrs
+	return nil
 }

From ed0b72944156ecc0947af8711ecf88591b82ae13 Mon Sep 17 00:00:00 2001
From: Jakub Jarosz <j.jarosz@f5.com>
Date: Thu, 4 May 2023 17:37:53 +0100
Subject: [PATCH 4/9] Validate JWT token

---
 pkg/apis/configuration/validation/policy.go | 12 ++++--------
 1 file changed, 4 insertions(+), 8 deletions(-)

diff --git a/pkg/apis/configuration/validation/policy.go b/pkg/apis/configuration/validation/policy.go
index deb8bd949a..1fa4b6102c 100644
--- a/pkg/apis/configuration/validation/policy.go
+++ b/pkg/apis/configuration/validation/policy.go
@@ -489,15 +489,11 @@ func validateJWTToken(token string, fieldPath *field.Path) field.ErrorList {
 		}
 	}
 
-	allErrs := field.ErrorList{}
-	if special {
-		// validateJWTToken is called only when NGINX Plus is running
-		isPlus := true
-		allErrs = append(allErrs, validateSpecialVariable(nVar, fieldPath, isPlus)...)
-	} else {
-		return append(allErrs, field.Invalid(fieldPath, token, "must only have special vars"))
+	if !special {
+		return field.ErrorList{field.Invalid(fieldPath, token, "must only have special vars")}
 	}
-	return allErrs
+	// validateJWTToken is called only when NGINX Plus is running
+	return validateSpecialVariable(nVar, fieldPath, true)
 }
 
 var validLogLevels = map[string]bool{

From 09adb40a9d4125ace1e28f07693614afd143892f Mon Sep 17 00:00:00 2001
From: Jakub Jarosz <j.jarosz@f5.com>
Date: Thu, 4 May 2023 20:12:17 +0100
Subject: [PATCH 5/9] Relax OIDC scope validation

---
 pkg/apis/configuration/validation/policy.go   | 15 +-------
 .../configuration/validation/policy_test.go   | 37 ++++++++++++-------
 2 files changed, 26 insertions(+), 26 deletions(-)

diff --git a/pkg/apis/configuration/validation/policy.go b/pkg/apis/configuration/validation/policy.go
index 1fa4b6102c..34abe691ef 100644
--- a/pkg/apis/configuration/validation/policy.go
+++ b/pkg/apis/configuration/validation/policy.go
@@ -9,7 +9,6 @@ import (
 	"strings"
 
 	v1 "github.com/nginxinc/kubernetes-ingress/pkg/apis/configuration/v1"
-	"golang.org/x/exp/slices"
 	"k8s.io/apimachinery/pkg/util/validation"
 	"k8s.io/apimachinery/pkg/util/validation/field"
 )
@@ -328,18 +327,8 @@ func validateOIDCScope(scope string, fieldPath *field.Path) field.ErrorList {
 	if !strings.Contains(scope, "openid") {
 		return field.ErrorList{field.Required(fieldPath, "openid scope")}
 	}
-
-	validScopes := []string{"openid", "profile", "email", "address", "phone", "offline_access"}
-	msg := fmt.Sprintf("invalid Scope. Accepted scopes are: %v", strings.Join(validScopes, ", "))
-
-	allErrs := field.ErrorList{}
-	s := strings.Split(scope, "+")
-	for _, v := range s {
-		if !slices.Contains(validScopes, v) {
-			allErrs = append(allErrs, field.Invalid(fieldPath, v, msg))
-		}
-	}
-	return allErrs
+	// todo: Add scope values validation (characters) described in the rfc6749 doc
+	return nil
 }
 
 func validateURL(name string, fieldPath *field.Path) field.ErrorList {
diff --git a/pkg/apis/configuration/validation/policy_test.go b/pkg/apis/configuration/validation/policy_test.go
index 479754758b..182ab92fa3 100644
--- a/pkg/apis/configuration/validation/policy_test.go
+++ b/pkg/apis/configuration/validation/policy_test.go
@@ -1050,6 +1050,30 @@ func TestValidateOIDC_PassesOnValidOIDC(t *testing.T) {
 	}
 }
 
+func TestValidateOIDCScope_FailsOnInvalidInput(t *testing.T) {
+	t.Parallel()
+
+	scopes := []string{"", " ", "mycustomscope"}
+	for _, v := range scopes {
+		allErrs := validateOIDCScope(v, field.NewPath("scope"))
+		if len(allErrs) == 0 {
+			t.Error("want err on missing `openid` scope")
+		}
+	}
+}
+
+func TestValidateOIDCScope_PassesOnValidScopes(t *testing.T) {
+	t.Parallel()
+
+	scopes := []string{"openid", "validScope+openid", "SecondScope+openid+CustomScope"}
+	for _, v := range scopes {
+		allErrs := validateOIDCScope(v, field.NewPath("scope"))
+		if len(allErrs) != 0 {
+			t.Errorf("want no err, got %v", allErrs)
+		}
+	}
+}
+
 func TestValidateOIDC_FailsOnInvalidOIDC(t *testing.T) {
 	t.Parallel()
 	tests := []struct {
@@ -1228,19 +1252,6 @@ func TestValidateOIDCScope_PassesOnValidInput(t *testing.T) {
 	}
 }
 
-func TestValidateOIDCScope_FailsOnInvalidInput(t *testing.T) {
-	t.Parallel()
-
-	invalidInput := []string{"profile", "openid+web", `openid+foobar.com`}
-
-	for _, test := range invalidInput {
-		allErrs := validateOIDCScope(test, field.NewPath("scope"))
-		if len(allErrs) == 0 {
-			t.Errorf("validateOIDCScope(%q) didn't return error for invalid input", test)
-		}
-	}
-}
-
 func TestValidateURL_PassesOnValidInput(t *testing.T) {
 	t.Parallel()
 

From c77031ed6dbd383d5adf3435b38d2d215289c361 Mon Sep 17 00:00:00 2001
From: Jakub Jarosz <j.jarosz@f5.com>
Date: Fri, 5 May 2023 19:13:57 +0100
Subject: [PATCH 6/9] Make access token scopes comply with RFC6749 sec 3.3

This PR updates validation rules for OIDC scope tokens. It makes them conform to the RFC6749 section 3.3. The section lists ranges of allowed characters.
---
 pkg/apis/configuration/validation/policy.go   | 23 +++++++-
 .../configuration/validation/policy_test.go   | 53 ++++++++++++-------
 2 files changed, 54 insertions(+), 22 deletions(-)

diff --git a/pkg/apis/configuration/validation/policy.go b/pkg/apis/configuration/validation/policy.go
index 34abe691ef..d763d0013d 100644
--- a/pkg/apis/configuration/validation/policy.go
+++ b/pkg/apis/configuration/validation/policy.go
@@ -7,6 +7,7 @@ import (
 	"regexp"
 	"strconv"
 	"strings"
+	"unicode"
 
 	v1 "github.com/nginxinc/kubernetes-ingress/pkg/apis/configuration/v1"
 	"k8s.io/apimachinery/pkg/util/validation"
@@ -322,12 +323,30 @@ func validateClientID(client string, fieldPath *field.Path) field.ErrorList {
 	return nil
 }
 
+// Allowed unicode ranges in OIDC scope tokens.
+// Ref. https://datatracker.ietf.org/doc/html/rfc6749#section-3.3
+var validOIDCScopeRanges = &unicode.RangeTable{
+	R16: []unicode.Range16{
+		{0x21, 0x21, 1},
+		{0x23, 0x5B, 1},
+		{0x5D, 0x7E, 1},
+	},
+}
+
 // https://openid.net/specs/openid-connect-core-1_0.html#ScopeClaims
 func validateOIDCScope(scope string, fieldPath *field.Path) field.ErrorList {
 	if !strings.Contains(scope, "openid") {
-		return field.ErrorList{field.Required(fieldPath, "openid scope")}
+		return field.ErrorList{field.Required(fieldPath, "openid is required")}
+	}
+
+	for _, token := range strings.Split(scope, "+") {
+		for _, v := range token {
+			if !unicode.Is(validOIDCScopeRanges, v) {
+				msg := fmt.Sprintf("not allowed character %v in scope %s", v, scope)
+				return field.ErrorList{field.Invalid(fieldPath, scope, msg)}
+			}
+		}
 	}
-	// todo: Add scope values validation (characters) described in the rfc6749 doc
 	return nil
 }
 
diff --git a/pkg/apis/configuration/validation/policy_test.go b/pkg/apis/configuration/validation/policy_test.go
index 182ab92fa3..a21b37968d 100644
--- a/pkg/apis/configuration/validation/policy_test.go
+++ b/pkg/apis/configuration/validation/policy_test.go
@@ -1050,23 +1050,37 @@ func TestValidateOIDC_PassesOnValidOIDC(t *testing.T) {
 	}
 }
 
-func TestValidateOIDCScope_FailsOnInvalidInput(t *testing.T) {
+func TestValidateOIDCScope_ErrorsOnInvalidInput(t *testing.T) {
 	t.Parallel()
 
-	scopes := []string{"", " ", "mycustomscope"}
-	for _, v := range scopes {
+	invalidInput := []string{
+		"",
+		" ",
+		"openid+scope\x5c",
+		"mycustom\x7fscope",
+		"openid+myscope\x20",
+		"openid+cus\x19tom",
+	}
+
+	for _, v := range invalidInput {
 		allErrs := validateOIDCScope(v, field.NewPath("scope"))
 		if len(allErrs) == 0 {
-			t.Error("want err on missing `openid` scope")
+			t.Error("want err on invalid scope, got no error")
 		}
 	}
 }
 
-func TestValidateOIDCScope_PassesOnValidScopes(t *testing.T) {
+func TestValidateOIDCScope_PassesOnValidInput(t *testing.T) {
 	t.Parallel()
 
-	scopes := []string{"openid", "validScope+openid", "SecondScope+openid+CustomScope"}
-	for _, v := range scopes {
+	validInput := []string{
+		"openid",
+		"validScope+openid",
+		"SecondScope+openid+CustomScope",
+		"validScope\x26+openid",
+		"openid+my\x33scope",
+	}
+	for _, v := range validInput {
 		allErrs := validateOIDCScope(v, field.NewPath("scope"))
 		if len(allErrs) != 0 {
 			t.Errorf("want no err, got %v", allErrs)
@@ -1098,6 +1112,18 @@ func TestValidateOIDC_FailsOnInvalidOIDC(t *testing.T) {
 			},
 			msg: "missing openid in scope",
 		},
+		{
+			oidc: &v1.OIDC{
+				AuthEndpoint:      "http://127.0.0.1:8080/auth/realms/master/protocol/openid-connect/auth",
+				TokenEndpoint:     "http://127.0.0.1:8080/auth/realms/master/protocol/openid-connect/token",
+				JWKSURI:           "http://127.0.0.1:8080/auth/realms/master/protocol/openid-connect/certs",
+				ClientID:          "client",
+				ClientSecret:      "secret",
+				Scope:             "openid+bogus\x7f",
+				AccessTokenEnable: true,
+			},
+			msg: "invalid unicode in scope",
+		},
 		{
 			oidc: &v1.OIDC{
 				AuthEndpoint:      "https://login.microsoftonline.com/dd-fff-eee-1234-9be/oauth2/v2.0/authorize",
@@ -1239,19 +1265,6 @@ func TestValidateClientID_FailsOnInvalidInput(t *testing.T) {
 	}
 }
 
-func TestValidateOIDCScope_PassesOnValidInput(t *testing.T) {
-	t.Parallel()
-
-	validInput := []string{"openid", "openid+profile", "openid+email", "openid+phone"}
-
-	for _, test := range validInput {
-		allErrs := validateOIDCScope(test, field.NewPath("scope"))
-		if len(allErrs) != 0 {
-			t.Errorf("validateOIDCScope(%q) returned errors %v for valid input", allErrs, test)
-		}
-	}
-}
-
 func TestValidateURL_PassesOnValidInput(t *testing.T) {
 	t.Parallel()
 

From dbab3d28b10d4996ae3def73b65ebe5cae0ab9ed Mon Sep 17 00:00:00 2001
From: Jakub Jarosz <j.jarosz@f5.com>
Date: Mon, 8 May 2023 11:03:42 +0100
Subject: [PATCH 7/9] Update docs

Updated docs no longer list allowed scopes. Only one mandatory scope is , possibly followed by a number of other, user-defined, custom scopes
---
 docs/content/configuration/policy-resource.md | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/docs/content/configuration/policy-resource.md b/docs/content/configuration/policy-resource.md
index 94af5cd2d4..68f369e218 100644
--- a/docs/content/configuration/policy-resource.md
+++ b/docs/content/configuration/policy-resource.md
@@ -442,7 +442,7 @@ The OIDC policy defines a few internal locations that can't be customized: `/_jw
 |``authExtraArgs`` | A list of extra URL arguments to pass to the authorization endpoint provided by your OpenID Connect provider. Arguments must be URL encoded, multiple arguments may be included in the list, for example ``[ arg1=value1, arg2=value2 ]`` | ``string[]`` | No |
 |``tokenEndpoint`` | URL for the token endpoint provided by your OpenID Connect provider. | ``string`` | Yes |
 |``jwksURI`` | URL for the JSON Web Key Set (JWK) document provided by your OpenID Connect provider. | ``string`` | Yes |
-|``scope`` | List of OpenID Connect scopes. Possible values are ``openid``, ``profile``, ``email``, ``address`` and ``phone``. The scope ``openid`` always needs to be present and others can be added concatenating them with a ``+`` sign, for example ``openid+profile+email``. The default is ``openid``. | ``string`` | No |
+|``scope`` | List of OpenID Connect scopes. The scope ``openid`` always needs to be present and others can be added concatenating them with a ``+`` sign, for example ``openid+profile+email``, ``openid+email+userDefinedScope``. The default is ``openid``. | ``string`` | No |
 |``redirectURI`` | Allows overriding the default redirect URI. The default is ``/_codexch``. | ``string`` | No |
 |``zoneSyncLeeway`` | Specifies the maximum timeout in milliseconds for synchronizing ID/access tokens and shared values between Ingress Controller pods. The default is ``200``. | ``int`` | No |
 |``accessTokenEnable`` | Option of whether Bearer token is used to authorize NGINX to access protected backend. | ``boolean`` | No |

From 2cb2cd81c8c4da8c0d3811d2fca48baed541f071 Mon Sep 17 00:00:00 2001
From: Jakub Jarosz <j.jarosz@f5.com>
Date: Mon, 8 May 2023 13:59:39 +0100
Subject: [PATCH 8/9] Increase test coverage

---
 pkg/apis/configuration/validation/policy_test.go | 16 ++++++++++++++--
 1 file changed, 14 insertions(+), 2 deletions(-)

diff --git a/pkg/apis/configuration/validation/policy_test.go b/pkg/apis/configuration/validation/policy_test.go
index a21b37968d..645e8a6cc0 100644
--- a/pkg/apis/configuration/validation/policy_test.go
+++ b/pkg/apis/configuration/validation/policy_test.go
@@ -1268,7 +1268,12 @@ func TestValidateClientID_FailsOnInvalidInput(t *testing.T) {
 func TestValidateURL_PassesOnValidInput(t *testing.T) {
 	t.Parallel()
 
-	validInput := []string{"http://google.com/auth", "https://foo.bar/baz", "http://127.0.0.1/bar", "http://openid.connect.com:8080/foo"}
+	validInput := []string{
+		"http://google.com/auth",
+		"https://foo.bar/baz",
+		"http://127.0.0.1/bar",
+		"http://openid.connect.com:8080/foo",
+	}
 
 	for _, test := range validInput {
 		allErrs := validateURL(test, field.NewPath("authEndpoint"))
@@ -1281,7 +1286,14 @@ func TestValidateURL_PassesOnValidInput(t *testing.T) {
 func TestValidateURL_FailsOnInvalidInput(t *testing.T) {
 	t.Parallel()
 
-	invalidInput := []string{"www.google..foo.com", "http://{foo.bar", `https://google.foo\bar`, "http://foo.bar:8080", "http://foo.bar:812345/fooo"}
+	invalidInput := []string{
+		"www.google..foo.com",
+		"http://{foo.bar",
+		`https://google.foo\bar`,
+		"http://foo.bar:8080",
+		"http://foo.bar:812345/fooo",
+		"http://",
+	}
 
 	for _, test := range invalidInput {
 		allErrs := validateURL(test, field.NewPath("authEndpoint"))

From 156b6b6c43f335d36271b7ec6393486e37b8340d Mon Sep 17 00:00:00 2001
From: Jakub Jarosz <j.jarosz@f5.com>
Date: Mon, 8 May 2023 14:20:34 +0100
Subject: [PATCH 9/9] Add func docs

---
 pkg/apis/configuration/validation/policy.go | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/pkg/apis/configuration/validation/policy.go b/pkg/apis/configuration/validation/policy.go
index d763d0013d..4e295bcc0c 100644
--- a/pkg/apis/configuration/validation/policy.go
+++ b/pkg/apis/configuration/validation/policy.go
@@ -333,7 +333,14 @@ var validOIDCScopeRanges = &unicode.RangeTable{
 	},
 }
 
-// https://openid.net/specs/openid-connect-core-1_0.html#ScopeClaims
+// validateOIDCScope takes a scope representing OIDC scope tokens and
+// checks if the scope is valid. OIDC scope must contain scope token
+// "openid". Additionally, custom scope tokens can be added to the scope.
+//
+// Ref:
+// - https://openid.net/specs/openid-connect-core-1_0.html#ScopeClaims
+//
+// Scope tokens must be separated by "+", and the "+" can't be a part of the token.
 func validateOIDCScope(scope string, fieldPath *field.Path) field.ErrorList {
 	if !strings.Contains(scope, "openid") {
 		return field.ErrorList{field.Required(fieldPath, "openid is required")}