diff --git a/api/v1alpha1/oidc_types.go b/api/v1alpha1/oidc_types.go index 7c11927d9c8..64ec47b1d2d 100644 --- a/api/v1alpha1/oidc_types.go +++ b/api/v1alpha1/oidc_types.go @@ -10,9 +10,13 @@ import ( gwapiv1 "sigs.k8s.io/gateway-api/apis/v1" ) -const OIDCClientSecretKey = "client-secret" +const ( + OIDCClientSecretKey = "client-secret" + OIDCClientIDKey = "client-id" +) // OIDC defines the configuration for the OpenID Connect (OIDC) authentication. +// +kubebuilder:validation:XValidation:rule="(has(self.clientID) && !has(self.clientIDRef)) || (!has(self.clientID) && has(self.clientIDRef))", message="only one of clientID or clientIDRef must be set" type OIDC struct { // The OIDC Provider configuration. Provider OIDCProvider `json:"provider"` @@ -20,18 +24,19 @@ type OIDC struct { // The client ID to be used in the OIDC // [Authentication Request](https://openid.net/specs/openid-connect-core-1_0.html#AuthRequest). // + // Only one of clientID or clientIDRef must be set. + // +optional // +kubebuilder:validation:MinLength=1 - ClientID string `json:"clientID"` - - // TODO zhaohuabing make ClientID optional in the implementation PR + ClientID *string `json:"clientID,omitempty"` // The Kubernetes secret which contains the client ID to be used in the // [Authentication Request](https://openid.net/specs/openid-connect-core-1_0.html#AuthRequest). // Exactly one of clientID or clientIDRef must be set. // This is an Opaque secret. The client ID should be stored in the key "client-id". // + // Only one of clientID or clientIDRef must be set. + // // +optional - // +notImplementedHide ClientIDRef *gwapiv1.SecretObjectReference `json:"clientIDRef,omitempty"` // The Kubernetes secret which contains the OIDC client secret to be used in the diff --git a/api/v1alpha1/zz_generated.deepcopy.go b/api/v1alpha1/zz_generated.deepcopy.go index a5393676149..0c04c7058cb 100644 --- a/api/v1alpha1/zz_generated.deepcopy.go +++ b/api/v1alpha1/zz_generated.deepcopy.go @@ -4608,6 +4608,11 @@ func (in *Lua) DeepCopy() *Lua { func (in *OIDC) DeepCopyInto(out *OIDC) { *out = *in in.Provider.DeepCopyInto(&out.Provider) + if in.ClientID != nil { + in, out := &in.ClientID, &out.ClientID + *out = new(string) + **out = **in + } if in.ClientIDRef != nil { in, out := &in.ClientIDRef, &out.ClientIDRef *out = new(v1.SecretObjectReference) diff --git a/charts/gateway-crds-helm/templates/generated/gateway.envoyproxy.io_securitypolicies.yaml b/charts/gateway-crds-helm/templates/generated/gateway.envoyproxy.io_securitypolicies.yaml index 7479b7cbc78..fb947fca1ae 100644 --- a/charts/gateway-crds-helm/templates/generated/gateway.envoyproxy.io_securitypolicies.yaml +++ b/charts/gateway-crds-helm/templates/generated/gateway.envoyproxy.io_securitypolicies.yaml @@ -3711,6 +3711,8 @@ spec: description: |- The client ID to be used in the OIDC [Authentication Request](https://openid.net/specs/openid-connect-core-1_0.html#AuthRequest). + + Only one of clientID or clientIDRef must be set. minLength: 1 type: string clientIDRef: @@ -3719,6 +3721,8 @@ spec: [Authentication Request](https://openid.net/specs/openid-connect-core-1_0.html#AuthRequest). Exactly one of clientID or clientIDRef must be set. This is an Opaque secret. The client ID should be stored in the key "client-id". + + Only one of clientID or clientIDRef must be set. properties: group: default: "" @@ -4943,10 +4947,13 @@ spec: type: string type: array required: - - clientID - clientSecret - provider type: object + x-kubernetes-validations: + - message: only one of clientID or clientIDRef must be set + rule: (has(self.clientID) && !has(self.clientIDRef)) || (!has(self.clientID) + && has(self.clientIDRef)) targetRef: description: |- TargetRef is the name of the resource this policy is being attached to. diff --git a/charts/gateway-helm/crds/generated/gateway.envoyproxy.io_securitypolicies.yaml b/charts/gateway-helm/crds/generated/gateway.envoyproxy.io_securitypolicies.yaml index 63c62388d2c..f1a4bf57abc 100644 --- a/charts/gateway-helm/crds/generated/gateway.envoyproxy.io_securitypolicies.yaml +++ b/charts/gateway-helm/crds/generated/gateway.envoyproxy.io_securitypolicies.yaml @@ -3710,6 +3710,8 @@ spec: description: |- The client ID to be used in the OIDC [Authentication Request](https://openid.net/specs/openid-connect-core-1_0.html#AuthRequest). + + Only one of clientID or clientIDRef must be set. minLength: 1 type: string clientIDRef: @@ -3718,6 +3720,8 @@ spec: [Authentication Request](https://openid.net/specs/openid-connect-core-1_0.html#AuthRequest). Exactly one of clientID or clientIDRef must be set. This is an Opaque secret. The client ID should be stored in the key "client-id". + + Only one of clientID or clientIDRef must be set. properties: group: default: "" @@ -4942,10 +4946,13 @@ spec: type: string type: array required: - - clientID - clientSecret - provider type: object + x-kubernetes-validations: + - message: only one of clientID or clientIDRef must be set + rule: (has(self.clientID) && !has(self.clientIDRef)) || (!has(self.clientID) + && has(self.clientIDRef)) targetRef: description: |- TargetRef is the name of the resource this policy is being attached to. diff --git a/internal/gatewayapi/securitypolicy.go b/internal/gatewayapi/securitypolicy.go index 126feb73075..33f294d39e7 100644 --- a/internal/gatewayapi/securitypolicy.go +++ b/internal/gatewayapi/securitypolicy.go @@ -1120,6 +1120,7 @@ func (t *Translator) buildOIDC( var ( oidc = policy.Spec.OIDC provider *ir.OIDCProvider + clientID string clientSecret *corev1.Secret redirectURL = defaultRedirectURL redirectPath = defaultRedirectPath @@ -1140,6 +1141,25 @@ func (t *Translator) buildOIDC( namespace: policy.Namespace, } + // Client ID can be specified either as a string or as a reference to a secret. + switch { + case oidc.ClientID != nil: + clientID = *oidc.ClientID + case oidc.ClientIDRef != nil: + var clientIDSecret *corev1.Secret + if clientIDSecret, err = t.validateSecretRef(false, from, *oidc.ClientIDRef, resources); err != nil { + return nil, err + } + clientIDBytes, ok := clientIDSecret.Data[egv1a1.OIDCClientIDKey] + if !ok || len(clientIDBytes) == 0 { + return nil, fmt.Errorf("client ID not found in secret %s/%s", clientIDSecret.Namespace, clientIDSecret.Name) + } + clientID = string(clientIDBytes) + default: + // This is just a sanity check - the CRD validation should have caught this. + return nil, fmt.Errorf("client ID must be specified in OIDC policy %s/%s", policy.Namespace, policy.Name) + } + if clientSecret, err = t.validateSecretRef( false, from, oidc.ClientSecret, resources); err != nil { return nil, err @@ -1198,7 +1218,7 @@ func (t *Translator) buildOIDC( return &ir.OIDC{ Name: irConfigName(policy), Provider: *provider, - ClientID: oidc.ClientID, + ClientID: clientID, ClientSecret: clientSecretBytes, Scopes: scopes, Resources: oidc.Resources, diff --git a/internal/gatewayapi/testdata/securitypolicy-with-oidc.in.yaml b/internal/gatewayapi/testdata/securitypolicy-with-oidc.in.yaml index b5031e6aa50..5a69a72b748 100644 --- a/internal/gatewayapi/testdata/securitypolicy-with-oidc.in.yaml +++ b/internal/gatewayapi/testdata/securitypolicy-with-oidc.in.yaml @@ -13,13 +13,7 @@ secrets: name: client2-secret data: client-secret: Y2xpZW50MTpzZWNyZXQK -- apiVersion: v1 - kind: Secret - metadata: - namespace: default - name: client3-secret - data: - client-secret: Y2xpZW50MTpzZWNyZXQK + client-id: Y2xpZW50Mi5vYXV0aC5mb28uY29t - apiVersion: v1 kind: Secret metadata: @@ -121,7 +115,8 @@ securityPolicies: issuer: "https://oauth.foo.com" authorizationEndpoint: "https://oauth.foo.com/oauth2/v2/auth" tokenEndpoint: "https://oauth.foo.com/token" - clientID: "client2.oauth.foo.com" + clientIDRef: + name: "client2-secret" clientSecret: name: "client2-secret" scopes: ["openid", "email", "profile"] diff --git a/internal/gatewayapi/testdata/securitypolicy-with-oidc.out.yaml b/internal/gatewayapi/testdata/securitypolicy-with-oidc.out.yaml index 9b1666bf185..6b8b0edf44b 100644 --- a/internal/gatewayapi/testdata/securitypolicy-with-oidc.out.yaml +++ b/internal/gatewayapi/testdata/securitypolicy-with-oidc.out.yaml @@ -146,7 +146,10 @@ securityPolicies: uid: 08335a80-83ba-4592-888f-6ac0bba44ce4 spec: oidc: - clientID: client2.oauth.foo.com + clientIDRef: + group: null + kind: null + name: client2-secret clientSecret: group: null kind: null diff --git a/internal/provider/kubernetes/controller.go b/internal/provider/kubernetes/controller.go index d5503364edf..2e4cc699225 100644 --- a/internal/provider/kubernetes/controller.go +++ b/internal/provider/kubernetes/controller.go @@ -789,6 +789,25 @@ func (r *gatewayAPIReconciler) processSecurityPolicyObjectRefs( "policy", policy, "secretRef", oidc.ClientSecret) } + if oidc.ClientIDRef != nil { + if err := r.processSecretRef( + ctx, + resourceMap, + resourceTree, + resource.KindSecurityPolicy, + policy.Namespace, + policy.Name, + *oidc.ClientIDRef); err != nil { + // If the error is transient, we return it to allow Reconcile to retry. + if isTransientError(err) { + return err + } + r.log.Error(err, + "failed to process OIDC ClientIDRef for SecurityPolicy", + "policy", policy, "secretRef", oidc.ClientIDRef) + } + } + var backendRefs []gwapiv1.BackendObjectReference if oidc.Provider.BackendRef != nil { backendRefs = append(backendRefs, *oidc.Provider.BackendRef) diff --git a/internal/provider/kubernetes/indexers.go b/internal/provider/kubernetes/indexers.go index 188117da0ee..a3047d0a059 100644 --- a/internal/provider/kubernetes/indexers.go +++ b/internal/provider/kubernetes/indexers.go @@ -589,6 +589,9 @@ func secretSecurityPolicyIndexFunc(rawObj client.Object) []string { if securityPolicy.Spec.OIDC != nil { secretReferences = append(secretReferences, securityPolicy.Spec.OIDC.ClientSecret) + if securityPolicy.Spec.OIDC.ClientIDRef != nil { + secretReferences = append(secretReferences, *securityPolicy.Spec.OIDC.ClientIDRef) + } } if securityPolicy.Spec.APIKeyAuth != nil { secretReferences = append(secretReferences, securityPolicy.Spec.APIKeyAuth.CredentialRefs...) diff --git a/internal/provider/kubernetes/predicates_test.go b/internal/provider/kubernetes/predicates_test.go index ec34defd077..50215429d5f 100644 --- a/internal/provider/kubernetes/predicates_test.go +++ b/internal/provider/kubernetes/predicates_test.go @@ -371,7 +371,7 @@ func TestValidateSecretForReconcile(t *testing.T) { AuthorizationEndpoint: ptr.To("https://accounts.google.com/o/oauth2/v2/auth"), TokenEndpoint: ptr.To("https://oauth2.googleapis.com/token"), }, - ClientID: "client-id", + ClientID: ptr.To("client-id"), ClientSecret: gwapiv1.SecretObjectReference{ Name: "secret", }, diff --git a/release-notes/current.yaml b/release-notes/current.yaml index ef853a1a5f7..7e23d86a53e 100644 --- a/release-notes/current.yaml +++ b/release-notes/current.yaml @@ -39,6 +39,7 @@ new features: | Added metric `watchable_publish_total` counting store events in watchable message queues. Added support for forwarding client ID header and sanitizing API keys for API Key authentication in SecurityPolicy. Accessloggers of type ALS now have http2 enabled on the cluster by default. + Added support for using Secret as a source of the OIDC client ID. Added support for listeners and routes in PostTranslateModifyHook extension hook. Added admin console support with web UI for the Envoy Gateway admin server. diff --git a/site/content/en/latest/api/extension_types.md b/site/content/en/latest/api/extension_types.md index 79eab9ba04e..980823aef93 100644 --- a/site/content/en/latest/api/extension_types.md +++ b/site/content/en/latest/api/extension_types.md @@ -3262,7 +3262,8 @@ _Appears in:_ | Field | Type | Required | Default | Description | | --- | --- | --- | --- | --- | | `provider` | _[OIDCProvider](#oidcprovider)_ | true | | The OIDC Provider configuration. | -| `clientID` | _string_ | true | | The client ID to be used in the OIDC
[Authentication Request](https://openid.net/specs/openid-connect-core-1_0.html#AuthRequest). | +| `clientID` | _string_ | false | | The client ID to be used in the OIDC
[Authentication Request](https://openid.net/specs/openid-connect-core-1_0.html#AuthRequest).
Only one of clientID or clientIDRef must be set. | +| `clientIDRef` | _[SecretObjectReference](https://gateway-api.sigs.k8s.io/references/spec/#gateway.networking.k8s.io/v1.SecretObjectReference)_ | false | | The Kubernetes secret which contains the client ID to be used in the
[Authentication Request](https://openid.net/specs/openid-connect-core-1_0.html#AuthRequest).
Exactly one of clientID or clientIDRef must be set.
This is an Opaque secret. The client ID should be stored in the key "client-id".
Only one of clientID or clientIDRef must be set. | | `clientSecret` | _[SecretObjectReference](https://gateway-api.sigs.k8s.io/references/spec/#gateway.networking.k8s.io/v1.SecretObjectReference)_ | true | | The Kubernetes secret which contains the OIDC client secret to be used in the
[Authentication Request](https://openid.net/specs/openid-connect-core-1_0.html#AuthRequest).
This is an Opaque secret. The client secret should be stored in the key
"client-secret". | | `cookieNames` | _[OIDCCookieNames](#oidccookienames)_ | false | | The optional cookie name overrides to be used for Bearer and IdToken cookies in the
[Authentication Request](https://openid.net/specs/openid-connect-core-1_0.html#AuthRequest).
If not specified, uses a randomly generated suffix | | `cookieConfig` | _[OIDCCookieConfig](#oidccookieconfig)_ | false | | CookieConfigs allows setting the SameSite attribute for OIDC cookies.
By default, its unset. | diff --git a/test/cel-validation/securitypolicy_test.go b/test/cel-validation/securitypolicy_test.go index 04f3391983f..10a057894e2 100644 --- a/test/cel-validation/securitypolicy_test.go +++ b/test/cel-validation/securitypolicy_test.go @@ -1335,7 +1335,7 @@ func TestSecurityPolicyTarget(t *testing.T) { AuthorizationEndpoint: ptr.To("https://accounts.google.com/o/oauth2/v2/auth"), TokenEndpoint: ptr.To("https://oauth2.googleapis.com/token"), }, - ClientID: "client-id", + ClientID: ptr.To("client-id"), ClientSecret: gwapiv1b1.SecretObjectReference{ Name: "secret", }, @@ -1380,7 +1380,7 @@ func TestSecurityPolicyTarget(t *testing.T) { AuthorizationEndpoint: ptr.To("https://accounts.google.com/o/oauth2/v2/auth"), TokenEndpoint: ptr.To("https://oauth2.googleapis.com/token"), }, - ClientID: "client-id", + ClientID: ptr.To("client-id"), ClientSecret: gwapiv1b1.SecretObjectReference{ Name: "secret", }, @@ -1389,6 +1389,68 @@ func TestSecurityPolicyTarget(t *testing.T) { }, wantErrors: []string{"Retry timeout is not supported", "HTTPStatusCodes is not supported"}, }, + { + desc: "oidc-without-clientid", + mutate: func(sp *egv1a1.SecurityPolicy) { + sp.Spec = egv1a1.SecurityPolicySpec{ + PolicyTargetReferences: egv1a1.PolicyTargetReferences{ + TargetSelectors: []egv1a1.TargetSelector{ + { + Group: ptr.To(gwapiv1a2.Group("gateway.networking.k8s.io")), + Kind: "HTTPRoute", + MatchLabels: map[string]string{ + "eg/namespace": "reference-apps", + }, + }, + }, + }, + OIDC: &egv1a1.OIDC{ + Provider: egv1a1.OIDCProvider{ + Issuer: "https://accounts.google.com", + AuthorizationEndpoint: ptr.To("https://accounts.google.com/o/oauth2/v2/auth"), + TokenEndpoint: ptr.To("https://oauth2.googleapis.com/token"), + }, + ClientSecret: gwapiv1b1.SecretObjectReference{ + Name: "secret", + }, + }, + } + }, + wantErrors: []string{"only one of clientID or clientIDRef must be set"}, + }, + { + desc: "oidc-two-clientids", + mutate: func(sp *egv1a1.SecurityPolicy) { + sp.Spec = egv1a1.SecurityPolicySpec{ + PolicyTargetReferences: egv1a1.PolicyTargetReferences{ + TargetSelectors: []egv1a1.TargetSelector{ + { + Group: ptr.To(gwapiv1a2.Group("gateway.networking.k8s.io")), + Kind: "HTTPRoute", + MatchLabels: map[string]string{ + "eg/namespace": "reference-apps", + }, + }, + }, + }, + OIDC: &egv1a1.OIDC{ + Provider: egv1a1.OIDCProvider{ + Issuer: "https://accounts.google.com", + AuthorizationEndpoint: ptr.To("https://accounts.google.com/o/oauth2/v2/auth"), + TokenEndpoint: ptr.To("https://oauth2.googleapis.com/token"), + }, + ClientID: ptr.To("client-id"), + ClientIDRef: &gwapiv1b1.SecretObjectReference{ + Name: "secret", + }, + ClientSecret: gwapiv1b1.SecretObjectReference{ + Name: "secret", + }, + }, + } + }, + wantErrors: []string{"only one of clientID or clientIDRef must be set"}, + }, } for _, tc := range cases { diff --git a/test/e2e/testdata/oidc-securitypolicy-backendcluster.yaml b/test/e2e/testdata/oidc-securitypolicy-backendcluster.yaml index 36f4b9b8894..fa4b3decb97 100644 --- a/test/e2e/testdata/oidc-securitypolicy-backendcluster.yaml +++ b/test/e2e/testdata/oidc-securitypolicy-backendcluster.yaml @@ -34,6 +34,14 @@ spec: --- apiVersion: v1 kind: Secret +metadata: + namespace: gateway-conformance-infra + name: oidctest-client-id +data: + client-id: b2lkY3Rlc3Q= # base64 encoding of "oidctest" +--- +apiVersion: v1 +kind: Secret metadata: namespace: gateway-conformance-infra name: oidctest-secret @@ -67,7 +75,8 @@ spec: retryOn: triggers: ["5xx", "gateway-error", "reset"] issuer: "https://keycloak.gateway-conformance-infra/realms/master" # Test fetching auth endpoint from the issuer url - clientID: "oidctest" + clientIDRef: + name: "oidctest-client-id" clientSecret: name: "oidctest-secret" redirectURL: "http://www.example.com/myapp/oauth2/callback" diff --git a/test/helm/gateway-crds-helm/all.out.yaml b/test/helm/gateway-crds-helm/all.out.yaml index be75734e7a5..c16d276ed17 100644 --- a/test/helm/gateway-crds-helm/all.out.yaml +++ b/test/helm/gateway-crds-helm/all.out.yaml @@ -42878,6 +42878,8 @@ spec: description: |- The client ID to be used in the OIDC [Authentication Request](https://openid.net/specs/openid-connect-core-1_0.html#AuthRequest). + + Only one of clientID or clientIDRef must be set. minLength: 1 type: string clientIDRef: @@ -42886,6 +42888,8 @@ spec: [Authentication Request](https://openid.net/specs/openid-connect-core-1_0.html#AuthRequest). Exactly one of clientID or clientIDRef must be set. This is an Opaque secret. The client ID should be stored in the key "client-id". + + Only one of clientID or clientIDRef must be set. properties: group: default: "" @@ -44110,10 +44114,13 @@ spec: type: string type: array required: - - clientID - clientSecret - provider type: object + x-kubernetes-validations: + - message: only one of clientID or clientIDRef must be set + rule: (has(self.clientID) && !has(self.clientIDRef)) || (!has(self.clientID) + && has(self.clientIDRef)) targetRef: description: |- TargetRef is the name of the resource this policy is being attached to. diff --git a/test/helm/gateway-crds-helm/envoy-gateway-crds.out.yaml b/test/helm/gateway-crds-helm/envoy-gateway-crds.out.yaml index 14d6f4bf1dc..fce013c4ed4 100644 --- a/test/helm/gateway-crds-helm/envoy-gateway-crds.out.yaml +++ b/test/helm/gateway-crds-helm/envoy-gateway-crds.out.yaml @@ -25566,6 +25566,8 @@ spec: description: |- The client ID to be used in the OIDC [Authentication Request](https://openid.net/specs/openid-connect-core-1_0.html#AuthRequest). + + Only one of clientID or clientIDRef must be set. minLength: 1 type: string clientIDRef: @@ -25574,6 +25576,8 @@ spec: [Authentication Request](https://openid.net/specs/openid-connect-core-1_0.html#AuthRequest). Exactly one of clientID or clientIDRef must be set. This is an Opaque secret. The client ID should be stored in the key "client-id". + + Only one of clientID or clientIDRef must be set. properties: group: default: "" @@ -26798,10 +26802,13 @@ spec: type: string type: array required: - - clientID - clientSecret - provider type: object + x-kubernetes-validations: + - message: only one of clientID or clientIDRef must be set + rule: (has(self.clientID) && !has(self.clientIDRef)) || (!has(self.clientID) + && has(self.clientIDRef)) targetRef: description: |- TargetRef is the name of the resource this policy is being attached to.