Enhance OIDCNative validation and error handling; add tests for missing secrets and invalid configurations, address copilot feedback

Author: AlexFenlon

internal/configs/policy.go

@@ -904,8 +904,20 @@ func resolveOIDCNativeClientSecret(
 		res.isError = true
 		return "", false
 	}
+	if secretRef.Secret == nil {
+		res.addWarningf("OIDCNative policy %s references an invalid secret %s: secret doesn't exist", polKey, secretKey)
+		res.isError = true
+		return "", false
+	}
+
+	clientSecretBytes, ok := secretRef.Secret.Data[ClientSecretKey]
+	if !ok {
+		res.addWarningf("OIDCNative policy %s references a secret %s missing '%s' key", polKey, secretKey, ClientSecretKey)
+		res.isError = true
+		return "", false
+	}
 
-	return string(secretRef.Secret.Data[ClientSecretKey]), true
+	return string(clientSecretBytes), true
 }
 
 func resolveOIDCNativeTrustedCert(

internal/configs/policy_test.go

@@ -4835,6 +4835,93 @@ func TestGeneratePoliciesFails(t *testing.T) {
 			},
 			msg: "oidcNative missing secret",
 		},
+		{
+			policyRefs: []conf_v1.PolicyReference{
+				{
+					Name:      "oidc-native-policy",
+					Namespace: "default",
+				},
+			},
+			policies: map[string]*conf_v1.Policy{
+				"default/oidc-native-policy": {
+					ObjectMeta: meta_v1.ObjectMeta{
+						Name:      "oidc-native-policy",
+						Namespace: "default",
+					},
+					Spec: conf_v1.PolicySpec{
+						OIDCNative: &conf_v1.OIDCNative{
+							Issuer:       "https://accounts.google.com",
+							ClientID:     "my-client",
+							ClientSecret: "oidc-secret",
+						},
+					},
+				},
+			},
+			policyOpts: policyOptions{
+				secretRefs: map[string]*secrets.SecretReference{
+					"default/oidc-secret": {
+						Secret: &api_v1.Secret{
+							Type: secrets.SecretTypeOIDC,
+							Data: map[string][]byte{},
+						},
+					},
+				},
+			},
+			context: "route",
+			expected: policiesCfg{
+				ErrorReturn: &version2.Return{
+					Code: 500,
+				},
+			},
+			expectedWarnings: Warnings{
+				nil: {
+					`OIDCNative policy default/oidc-native-policy references a secret default/oidc-secret missing 'client-secret' key`,
+				},
+			},
+			msg: "oidcNative secret missing client-secret key",
+		},
+		{
+			policyRefs: []conf_v1.PolicyReference{
+				{
+					Name:      "oidc-native-policy",
+					Namespace: "default",
+				},
+			},
+			policies: map[string]*conf_v1.Policy{
+				"default/oidc-native-policy": {
+					ObjectMeta: meta_v1.ObjectMeta{
+						Name:      "oidc-native-policy",
+						Namespace: "default",
+					},
+					Spec: conf_v1.PolicySpec{
+						OIDCNative: &conf_v1.OIDCNative{
+							Issuer:       "https://accounts.google.com",
+							ClientID:     "my-client",
+							ClientSecret: "oidc-secret",
+						},
+					},
+				},
+			},
+			policyOpts: policyOptions{
+				secretRefs: map[string]*secrets.SecretReference{
+					"default/oidc-secret": {
+						Secret: nil,
+					},
+				},
+			},
+			context: "route",
+			expected: policiesCfg{
+				ErrorReturn: &version2.Return{
+					Code: 500,
+				},
+			},
+			expectedWarnings: Warnings{
+				nil: {
+					`OIDCNative policy default/oidc-native-policy references an invalid secret default/oidc-secret: secret doesn't exist`,
+				},
+			},
+			msg: "oidcNative secret reference with nil Secret",
+		},
 		{
 			policyRefs: []conf_v1.PolicyReference{
 				{

internal/configs/version2/__snapshots__/templates_test.snap

@@ -4558,7 +4558,7 @@ oidc_provider oidc_default_my_provider_default_cafe {
     client_id my-client-id;
     client_secret "my-resolved-secret";
     config_url https://accounts.google.com/.well-known/openid-configuration;
-    scope openid+profile;
+    scope openid profile;
     redirect_uri /callback;
     logout_uri /logout;
     post_logout_uri /logged_out;

internal/configs/version2/templates_test.go

@@ -3855,7 +3855,7 @@ func TestVirtualServerForNginxPlusWithOIDCNative(t *testing.T) {
 				ClientID:              "my-client-id",
 				ClientSecret:          "my-resolved-secret",
 				ConfigURL:             "https://accounts.google.com/.well-known/openid-configuration",
-				Scope:                 "openid+profile",
+				Scope:                 "openid profile",
 				RedirectURI:           "/callback",
 				CookieName:            "MY_SESSION",
 				ExtraAuthArgs:         "prompt=login",

pkg/apis/configuration/v1/types.go

@@ -801,6 +801,7 @@ type PolicySpec struct {
 	// +kubebuilder:validation:XValidation:rule="(self.sslVerify == true) || (self.sslVerify == false && !has(self.trustedCertSecret))",message="trustedCertSecret can be set only if sslVerify is true"
 	OIDC *OIDC `json:"oidc"`
 	// The OpenID Connect policy configures NGINX to authenticate client requests by validating a JWT token against an OAuth2/OIDC token provider, such as Auth0 or Keycloak. NGINX Plus native.
+	// +kubebuilder:validation:XValidation:rule="(self.sslVerify == true) || (self.sslVerify == false && !has(self.trustedCertSecret))",message="trustedCertSecret can be set only if sslVerify is true"
 	OIDCNative *OIDCNative `json:"oidcNative"`
 	// The WAF policy configures WAF and log configuration policies for NGINX AppProtect
 	WAF *WAF `json:"waf"`

pkg/apis/configuration/validation/policy.go

@@ -477,6 +477,11 @@ func validateOIDCNative(oidcNative *v1.OIDCNative, fieldPath *field.Path) field.
 
 	allErrs := field.ErrorList{}
 
+	if oidcNative.SSLVerify != nil && !*oidcNative.SSLVerify && oidcNative.TrustedCertSecret != "" {
+		allErrs = append(allErrs, field.Forbidden(fieldPath.Child("trustedCertSecret"),
+			"trustedCertSecret can be set only if sslVerify is true"))
+	}
+
 	// Issuer hostname/port validation goes beyond the CRD pattern (which only checks https:// + host).
 	allErrs = append(allErrs, validateIssuerURL(oidcNative.Issuer, fieldPath.Child("issuer"))...)
 	// ClientID char validation (rejects $, unescaped \, etc.) — not expressible in a simple CRD pattern.
@@ -1063,6 +1068,9 @@ func validateIssuerURL(issuer string, fieldPath *field.Path) field.ErrorList {
 	if u.Host == "" {
 		return field.ErrorList{field.Invalid(fieldPath, issuer, "hostname required")}
 	}
+	if u.RawQuery != "" || u.Fragment != "" || u.User != nil {
+		return field.ErrorList{field.Invalid(fieldPath, issuer, "must not include userinfo, query, or fragment")}
+	}
 
 	host, port, err := net.SplitHostPort(u.Host)
 	if err != nil {

pkg/apis/configuration/validation/policy_test.go

@@ -2215,6 +2215,16 @@ func TestValidateOIDCNative_FailsOnInvalidInput(t *testing.T) {
 			fieldPath:  "oidcNative.scope",
 			msg:        "openid must be a complete scope token",
 		},
+		{
+			oidcNative: &v1.OIDCNative{Issuer: "https://accounts.google.com", ClientID: "my-client", SSLVerify: new(bool), TrustedCertSecret: "my-ca"},
+			fieldPath:  "oidcNative.trustedCertSecret",
+			msg:        "trustedCertSecret set when sslVerify is false",
+		},
+		{
+			oidcNative: &v1.OIDCNative{Issuer: "https://accounts.google.com?query=1", ClientID: "my-client"},
+			fieldPath:  "oidcNative.issuer",
+			msg:        "issuer contains query parameter",
+		},
 	}
 
 	for _, test := range tests {