diff --git a/internal/configs/virtualserver.go b/internal/configs/virtualserver.go index abfcca71ad..6e3de4a46c 100644 --- a/internal/configs/virtualserver.go +++ b/internal/configs/virtualserver.go @@ -673,13 +673,13 @@ func (p *policiesCfg) addRateLimitConfig( } else { curOptions := generateLimitReqOptions(rateLimit) if curOptions.DryRun != p.LimitReqOptions.DryRun { - res.addWarningf("RateLimit policy %q with limit request option dryRun=%v is overridden to dryRun=%v by the first policy reference in this context", polKey, curOptions.DryRun, p.LimitReqOptions.DryRun) + res.addWarningf("RateLimit policy %s with limit request option dryRun='%v' is overridden to dryRun='%v' by the first policy reference in this context", polKey, curOptions.DryRun, p.LimitReqOptions.DryRun) } if curOptions.LogLevel != p.LimitReqOptions.LogLevel { - res.addWarningf("RateLimit policy %q with limit request option logLevel=%v is overridden to logLevel=%v by the first policy reference in this context", polKey, curOptions.LogLevel, p.LimitReqOptions.LogLevel) + res.addWarningf("RateLimit policy %s with limit request option logLevel='%v' is overridden to logLevel='%v' by the first policy reference in this context", polKey, curOptions.LogLevel, p.LimitReqOptions.LogLevel) } if curOptions.RejectCode != p.LimitReqOptions.RejectCode { - res.addWarningf("RateLimit policy %q with limit request option rejectCode=%v is overridden to rejectCode=%v by the first policy reference in this context", polKey, curOptions.RejectCode, p.LimitReqOptions.RejectCode) + res.addWarningf("RateLimit policy %s with limit request option rejectCode='%v' is overridden to rejectCode='%v' by the first policy reference in this context", polKey, curOptions.RejectCode, p.LimitReqOptions.RejectCode) } } return res @@ -693,7 +693,7 @@ func (p *policiesCfg) addJWTAuthConfig( ) *validationResults { res := newValidationResults() if p.JWTAuth != nil { - res.addWarningf("Multiple jwt policies in the same context is not valid. JWT policy %q will be ignored", polKey) + res.addWarningf("Multiple jwt policies in the same context is not valid. JWT policy %s will be ignored", polKey) return res } @@ -704,11 +704,11 @@ func (p *policiesCfg) addJWTAuthConfig( secretType = secretRef.Secret.Type } if secretType != "" && secretType != secrets.SecretTypeJWK { - res.addWarningf("JWT policy %q references a Secret of an incorrect type %q", polKey, secretType) + res.addWarningf("JWT policy %s references a secret %s of a wrong type '%s', must be '%s'", polKey, jwtSecretKey, secretType, secrets.SecretTypeJWK) res.isError = true return res } else if secretRef.Error != nil { - res.addWarningf("JWT policy %q references an invalid Secret: %v", polKey, secretRef.Error) + res.addWarningf("JWT policy %s references an invalid secret %s: %v", polKey, jwtSecretKey, secretRef.Error) res.isError = true return res } @@ -731,17 +731,17 @@ func (p *policiesCfg) addIngressMTLSConfig( ) *validationResults { res := newValidationResults() if !tls { - res.addWarningf("TLS configuration needed for IngressMTLS policy") + res.addWarningf("TLS must be enabled in VirtualServer for IngressMTLS policy %s", polKey) res.isError = true return res } if context != specContext { - res.addWarningf("IngressMTLS policy is not allowed in the %v context", context) + res.addWarningf("IngressMTLS policy %s is not allowed in the %v context", polKey, context) res.isError = true return res } if p.IngressMTLS != nil { - res.addWarningf("Multiple ingressMTLS policies are not allowed. IngressMTLS policy %q will be ignored", polKey) + res.addWarningf("Multiple ingressMTLS policies are not allowed. IngressMTLS policy %s will be ignored", polKey) return res } @@ -752,11 +752,11 @@ func (p *policiesCfg) addIngressMTLSConfig( secretType = secretRef.Secret.Type } if secretType != "" && secretType != secrets.SecretTypeCA { - res.addWarningf("IngressMTLS policy %q references a Secret of an incorrect type %q", polKey, secretType) + res.addWarningf("IngressMTLS policy %s references a secret %s of a wrong type '%s', must be '%s'", polKey, secretKey, secretType, secrets.SecretTypeCA) res.isError = true return res } else if secretRef.Error != nil { - res.addWarningf("IngressMTLS policy %q references an invalid Secret: %v", polKey, secretRef.Error) + res.addWarningf("IngressMTLS policy %q references an invalid secret %s: %v", polKey, secretKey, secretRef.Error) res.isError = true return res } @@ -787,7 +787,7 @@ func (p *policiesCfg) addEgressMTLSConfig( res := newValidationResults() if p.EgressMTLS != nil { res.addWarningf( - "Multiple egressMTLS policies in the same context is not valid. EgressMTLS policy %q will be ignored", + "Multiple egressMTLS policies in the same context is not valid. EgressMTLS policy %s will be ignored", polKey, ) return res @@ -804,11 +804,11 @@ func (p *policiesCfg) addEgressMTLSConfig( secretType = secretRef.Secret.Type } if secretType != "" && secretType != api_v1.SecretTypeTLS { - res.addWarningf("EgressMTLS policy %q references a Secret of an incorrect type %q", polKey, secretType) + res.addWarningf("EgressMTLS policy %s references a secret %s of a wrong type '%s', must be '%s'", polKey, egressTLSSecret, secretType, api_v1.SecretTypeTLS) res.isError = true return res } else if secretRef.Error != nil { - res.addWarningf("EgressMTLS policy %q references an invalid Secret: %v", polKey, secretRef.Error) + res.addWarningf("EgressMTLS policy %s references an invalid secret %s: %v", polKey, egressTLSSecret, secretRef.Error) res.isError = true return res } @@ -827,11 +827,11 @@ func (p *policiesCfg) addEgressMTLSConfig( secretType = secretRef.Secret.Type } if secretType != "" && secretType != secrets.SecretTypeCA { - res.addWarningf("EgressMTLS policy %q references a Secret of an incorrect type %q", polKey, secretType) + res.addWarningf("EgressMTLS policy %s references a secret %s of a wrong type '%s', must be '%s'", polKey, trustedCertSecret, secretType, secrets.SecretTypeCA) res.isError = true return res } else if secretRef.Error != nil { - res.addWarningf("EgressMTLS policy %q references an invalid Secret: %v", polKey, secretRef.Error) + res.addWarningf("EgressMTLS policy %s references an invalid secret %s: %v", polKey, trustedCertSecret, secretRef.Error) res.isError = true return res } @@ -864,7 +864,7 @@ func (p *policiesCfg) addOIDCConfig( res := newValidationResults() if p.OIDC { res.addWarningf( - "Multiple oidc policies in the same context is not valid. OIDC policy %q will be ignored", + "Multiple oidc policies in the same context is not valid. OIDC policy %s will be ignored", polKey, ) return res @@ -873,7 +873,7 @@ func (p *policiesCfg) addOIDCConfig( if oidcPolCfg.oidc != nil { if oidcPolCfg.key != polKey { res.addWarningf( - "Only one OIDC policy is allowed in a VirtualServer and its VirtualServerRoutes. Can't use %q. Use %q", + "Only one oidc policy is allowed in a VirtualServer and its VirtualServerRoutes. Can't use %s. Use %s", polKey, oidcPolCfg.key, ) @@ -882,33 +882,23 @@ func (p *policiesCfg) addOIDCConfig( } } else { secretKey := fmt.Sprintf("%v/%v", polNamespace, oidc.ClientSecret) - secretRef, exists := secretRefs[secretKey] - if !exists { - res.addWarningf("OIDC policy %q references a non-existent Secret %v", polKey, secretKey) - res.isError = true - return res - } + secretRef := secretRefs[secretKey] var secretType api_v1.SecretType if secretRef.Secret != nil { secretType = secretRef.Secret.Type } if secretType != "" && secretType != secrets.SecretTypeOIDC { - res.addWarningf("OIDC policy %q references a Secret of an incorrect type %q", polKey, secretType) + res.addWarningf("OIDC policy %s references a secret %s of a wrong type '%s', must be '%s'", polKey, secretKey, secretType, secrets.SecretTypeOIDC) res.isError = true return res } else if secretRef.Error != nil { - res.addWarningf("OIDC policy %q references an invalid Secret: %v", polKey, secretRef.Error) + res.addWarningf("OIDC policy %s references an invalid secret %s: %v", polKey, secretKey, secretRef.Error) res.isError = true return res } - clientSecret, exists := secretRef.Secret.Data[ClientSecretKey] - if !exists { - res.addWarningf("OIDC policy %q references a Secret without the data field %v", polKey, ClientSecretKey) - res.isError = true - return res - } + clientSecret := secretRef.Secret.Data[ClientSecretKey] redirectURI := oidc.RedirectURI if redirectURI == "" { diff --git a/internal/configs/virtualserver_test.go b/internal/configs/virtualserver_test.go index 6b5ee21eb1..4563ade3fd 100644 --- a/internal/configs/virtualserver_test.go +++ b/internal/configs/virtualserver_test.go @@ -2203,6 +2203,7 @@ func TestGeneratePoliciesFails(t *testing.T) { policyOpts policyOptions trustedCAFileName string context string + oidcPolCfg *oidcPolicyCfg expected policiesCfg expectedWarnings Warnings expectedOidc *oidcPolicyCfg @@ -2333,9 +2334,9 @@ func TestGeneratePoliciesFails(t *testing.T) { }, expectedWarnings: Warnings{ nil: { - `RateLimit policy "default/rateLimit-policy2" with limit request option dryRun=true is overridden to dryRun=false by the first policy reference in this context`, - `RateLimit policy "default/rateLimit-policy2" with limit request option logLevel=info is overridden to logLevel=error by the first policy reference in this context`, - `RateLimit policy "default/rateLimit-policy2" with limit request option rejectCode=505 is overridden to rejectCode=503 by the first policy reference in this context`, + `RateLimit policy default/rateLimit-policy2 with limit request option dryRun='true' is overridden to dryRun='false' by the first policy reference in this context`, + `RateLimit policy default/rateLimit-policy2 with limit request option logLevel='info' is overridden to logLevel='error' by the first policy reference in this context`, + `RateLimit policy default/rateLimit-policy2 with limit request option rejectCode='505' is overridden to rejectCode='503' by the first policy reference in this context`, }, }, expectedOidc: &oidcPolicyCfg{}, @@ -2379,7 +2380,7 @@ func TestGeneratePoliciesFails(t *testing.T) { }, expectedWarnings: Warnings{ nil: { - `JWT policy "default/jwt-policy" references an invalid Secret: secret is invalid`, + `JWT policy default/jwt-policy references an invalid secret default/jwt-secret: secret is invalid`, }, }, expectedOidc: &oidcPolicyCfg{}, @@ -2422,7 +2423,7 @@ func TestGeneratePoliciesFails(t *testing.T) { }, expectedWarnings: Warnings{ nil: { - `JWT policy "default/jwt-policy" references a Secret of an incorrect type "nginx.org/ca"`, + `JWT policy default/jwt-policy references a secret default/jwt-secret of a wrong type 'nginx.org/ca', must be 'nginx.org/jwk'`, }, }, expectedOidc: &oidcPolicyCfg{}, @@ -2489,7 +2490,7 @@ func TestGeneratePoliciesFails(t *testing.T) { }, expectedWarnings: Warnings{ nil: { - `Multiple jwt policies in the same context is not valid. JWT policy "default/jwt-policy2" will be ignored`, + `Multiple jwt policies in the same context is not valid. JWT policy default/jwt-policy2 will be ignored`, }, }, expectedOidc: &oidcPolicyCfg{}, @@ -2531,7 +2532,7 @@ func TestGeneratePoliciesFails(t *testing.T) { }, expectedWarnings: Warnings{ nil: { - `IngressMTLS policy "default/ingress-mtls-policy" references an invalid Secret: secret is invalid`, + `IngressMTLS policy "default/ingress-mtls-policy" references an invalid secret default/ingress-mtls-secret: secret is invalid`, }, }, expectedOidc: &oidcPolicyCfg{}, @@ -2575,7 +2576,7 @@ func TestGeneratePoliciesFails(t *testing.T) { }, expectedWarnings: Warnings{ nil: { - `IngressMTLS policy "default/ingress-mtls-policy" references a Secret of an incorrect type "kubernetes.io/tls"`, + `IngressMTLS policy default/ingress-mtls-policy references a secret default/ingress-mtls-secret of a wrong type 'kubernetes.io/tls', must be 'nginx.org/ca'`, }, }, expectedOidc: &oidcPolicyCfg{}, @@ -2633,7 +2634,7 @@ func TestGeneratePoliciesFails(t *testing.T) { }, expectedWarnings: Warnings{ nil: { - `Multiple ingressMTLS policies are not allowed. IngressMTLS policy "default/ingress-mtls-policy2" will be ignored`, + `Multiple ingressMTLS policies are not allowed. IngressMTLS policy default/ingress-mtls-policy2 will be ignored`, }, }, expectedOidc: &oidcPolicyCfg{}, @@ -2678,7 +2679,7 @@ func TestGeneratePoliciesFails(t *testing.T) { }, expectedWarnings: Warnings{ nil: { - `IngressMTLS policy is not allowed in the route context`, + `IngressMTLS policy default/ingress-mtls-policy is not allowed in the route context`, }, }, expectedOidc: &oidcPolicyCfg{}, @@ -2723,7 +2724,7 @@ func TestGeneratePoliciesFails(t *testing.T) { }, expectedWarnings: Warnings{ nil: { - `TLS configuration needed for IngressMTLS policy`, + `TLS must be enabled in VirtualServer for IngressMTLS policy default/ingress-mtls-policy`, }, }, expectedOidc: &oidcPolicyCfg{}, @@ -2789,7 +2790,7 @@ func TestGeneratePoliciesFails(t *testing.T) { }, expectedWarnings: Warnings{ nil: { - `Multiple egressMTLS policies in the same context is not valid. EgressMTLS policy "default/egress-mtls-policy2" will be ignored`, + `Multiple egressMTLS policies in the same context is not valid. EgressMTLS policy default/egress-mtls-policy2 will be ignored`, }, }, expectedOidc: &oidcPolicyCfg{}, @@ -2834,7 +2835,7 @@ func TestGeneratePoliciesFails(t *testing.T) { }, expectedWarnings: Warnings{ nil: { - `EgressMTLS policy "default/egress-mtls-policy" references an invalid Secret: secret is invalid`, + `EgressMTLS policy default/egress-mtls-policy references an invalid secret default/egress-trusted-secret: secret is invalid`, }, }, expectedOidc: &oidcPolicyCfg{}, @@ -2878,7 +2879,7 @@ func TestGeneratePoliciesFails(t *testing.T) { }, expectedWarnings: Warnings{ nil: { - `EgressMTLS policy "default/egress-mtls-policy" references a Secret of an incorrect type "nginx.org/ca"`, + `EgressMTLS policy default/egress-mtls-policy references a secret default/egress-mtls-secret of a wrong type 'nginx.org/ca', must be 'kubernetes.io/tls'`, }, }, expectedOidc: &oidcPolicyCfg{}, @@ -2922,7 +2923,7 @@ func TestGeneratePoliciesFails(t *testing.T) { }, expectedWarnings: Warnings{ nil: { - `EgressMTLS policy "default/egress-mtls-policy" references a Secret of an incorrect type "kubernetes.io/tls"`, + `EgressMTLS policy default/egress-mtls-policy references a secret default/egress-trusted-secret of a wrong type 'kubernetes.io/tls', must be 'nginx.org/ca'`, }, }, expectedOidc: &oidcPolicyCfg{}, @@ -2967,7 +2968,7 @@ func TestGeneratePoliciesFails(t *testing.T) { }, expectedWarnings: Warnings{ nil: { - `EgressMTLS policy "default/egress-mtls-policy" references an invalid Secret: secret is invalid`, + `EgressMTLS policy default/egress-mtls-policy references an invalid secret default/egress-mtls-secret: secret is invalid`, }, }, expectedOidc: &oidcPolicyCfg{}, @@ -3011,7 +3012,7 @@ func TestGeneratePoliciesFails(t *testing.T) { }, expectedWarnings: Warnings{ nil: { - `OIDC policy "default/oidc-policy" references an invalid Secret: secret is invalid`, + `OIDC policy default/oidc-policy references an invalid secret default/oidc-secret: secret is invalid`, }, }, expectedOidc: &oidcPolicyCfg{}, @@ -3057,12 +3058,100 @@ func TestGeneratePoliciesFails(t *testing.T) { }, expectedWarnings: Warnings{ nil: { - `OIDC policy "default/oidc-policy" references a Secret of an incorrect type "kubernetes.io/tls"`, + `OIDC policy default/oidc-policy references a secret default/oidc-secret of a wrong type 'kubernetes.io/tls', must be 'nginx.org/oidc'`, }, }, expectedOidc: &oidcPolicyCfg{}, msg: "oidc secret referencing wrong secret type", }, + { + policyRefs: []conf_v1.PolicyReference{ + { + Name: "oidc-policy-2", + Namespace: "default", + }, + }, + policies: map[string]*conf_v1.Policy{ + "default/oidc-policy-1": { + ObjectMeta: meta_v1.ObjectMeta{ + Name: "oidc-policy-1", + Namespace: "default", + }, + Spec: conf_v1.PolicySpec{ + OIDC: &conf_v1.OIDC{ + ClientID: "foo", + ClientSecret: "oidc-secret", + AuthEndpoint: "https://foo.com/auth", + TokenEndpoint: "https://foo.com/token", + JWKSURI: "https://foo.com/certs", + }, + }, + }, + "default/oidc-policy-2": { + ObjectMeta: meta_v1.ObjectMeta{ + Name: "oidc-policy-2", + Namespace: "default", + }, + Spec: conf_v1.PolicySpec{ + OIDC: &conf_v1.OIDC{ + ClientID: "foo", + ClientSecret: "oidc-secret", + AuthEndpoint: "https://bar.com/auth", + TokenEndpoint: "https://bar.com/token", + JWKSURI: "https://bar.com/certs", + }, + }, + }, + }, + policyOpts: policyOptions{ + secretRefs: map[string]*secrets.SecretReference{ + "default/oidc-secret": { + Secret: &api_v1.Secret{ + Type: secrets.SecretTypeOIDC, + Data: map[string][]byte{ + "client-secret": []byte("super_secret_123"), + }, + }, + }, + }, + }, + context: "route", + oidcPolCfg: &oidcPolicyCfg{ + oidc: &version2.OIDC{ + AuthEndpoint: "https://foo.com/auth", + TokenEndpoint: "https://foo.com/token", + JwksURI: "https://foo.com/certs", + ClientID: "foo", + ClientSecret: "super_secret_123", + RedirectURI: "/_codexch", + Scope: "openid", + }, + key: "default/oidc-policy-1", + }, + expected: policiesCfg{ + ErrorReturn: &version2.Return{ + Code: 500, + }, + }, + expectedWarnings: Warnings{ + nil: { + `Only one oidc policy is allowed in a VirtualServer and its VirtualServerRoutes. Can't use default/oidc-policy-2. Use default/oidc-policy-1`, + }, + }, + expectedOidc: &oidcPolicyCfg{ + oidc: &version2.OIDC{ + AuthEndpoint: "https://foo.com/auth", + TokenEndpoint: "https://foo.com/token", + JwksURI: "https://foo.com/certs", + ClientID: "foo", + ClientSecret: "super_secret_123", + RedirectURI: "/_codexch", + Scope: "openid", + }, + key: "default/oidc-policy-1", + }, + msg: "multiple oidc policies", + }, { policyRefs: []conf_v1.PolicyReference{ { @@ -3097,7 +3186,7 @@ func TestGeneratePoliciesFails(t *testing.T) { }, Spec: conf_v1.PolicySpec{ OIDC: &conf_v1.OIDC{ - ClientSecret: "oidc-secret2", + ClientSecret: "oidc-secret", AuthEndpoint: "https://bar.com/auth", TokenEndpoint: "https://bar.com/token", JWKSURI: "https://bar.com/certs", @@ -3124,7 +3213,7 @@ func TestGeneratePoliciesFails(t *testing.T) { }, expectedWarnings: Warnings{ nil: { - `Multiple oidc policies in the same context is not valid. OIDC policy "default/oidc-policy2" will be ignored`, + `Multiple oidc policies in the same context is not valid. OIDC policy default/oidc-policy2 will be ignored`, }, }, expectedOidc: &oidcPolicyCfg{ @@ -3146,6 +3235,10 @@ func TestGeneratePoliciesFails(t *testing.T) { for _, test := range tests { vsc := newVirtualServerConfigurator(&ConfigParams{}, false, false, &StaticConfigParams{}) + if test.oidcPolCfg != nil { + vsc.oidcPolCfg = test.oidcPolCfg + } + result := vsc.generatePolicies(ownerDetails, test.policyRefs, test.policies, test.context, test.policyOpts) if diff := cmp.Diff(test.expected, result); diff != "" { t.Errorf("generatePolicies() '%v' mismatch (-want +got):\n%s", test.msg, diff) diff --git a/tests/suite/test_jwt_policies_vsr.py b/tests/suite/test_jwt_policies_vsr.py index 2691fd9319..d444036186 100644 --- a/tests/suite/test_jwt_policies_vsr.py +++ b/tests/suite/test_jwt_policies_vsr.py @@ -352,7 +352,7 @@ def test_jwt_policy_delete_secret( assert f"Request ID:" in resp1.text assert crd_info["status"]["state"] == "Warning" assert ( - "references an invalid Secret: secret doesn't exist or of an unsupported type" + f"references an invalid secret {v_s_route_setup.route_m.namespace}/{secret}: secret doesn't exist or of an unsupported type" in crd_info["status"]["message"] ) assert resp2.status_code == 500