diff --git a/api/v1alpha1/organization_types.go b/api/v1alpha1/organization_types.go old mode 100644 new mode 100755 index c3da84d8d..ade7ec622 --- a/api/v1alpha1/organization_types.go +++ b/api/v1alpha1/organization_types.go @@ -85,6 +85,22 @@ type OIDCConfig struct { // OAuth2ClientRedirectURIs are a registered set of redirect URIs. When redirecting from the idproxy to // the client application, the URI requested to redirect to must be contained in this list. OAuth2ClientRedirectURIs []string `json:"oauth2ClientRedirectURIs,omitempty"` + // ExtraConfig contains additional OIDC configuration for claim mapping and token validation behavior. + ExtraConfig *OIDCExtraConfig `json:"extraConfig,omitempty"` +} + +type OIDCExtraConfig struct { + // InsecureSkipEmailVerified, if set to true, forces the email_verified claim to true + // regardless of the value reported by the upstream IdP, including overriding an explicit + // email_verified=false. Enable only for IdPs where the email address is verified by other + // means, e.g. Keycloak with SAML or user federation, or providers that omit the claim + // entirely such as some Okta, EntraID or CloudFoundry configurations. + // +kubebuilder:default:=false + InsecureSkipEmailVerified bool `json:"insecureSkipEmailVerified,omitempty"` + // UserIDClaim is the claim to be used as both user ID and username. + // When set, it overrides both UserIDKey and UserNameKey in the dex OIDC connector config. + // +kubebuilder:default:="login_name" + UserIDClaim string `json:"userIDClaim,omitempty"` } type SCIMConfig struct { diff --git a/api/v1alpha1/zz_generated.deepcopy.go b/api/v1alpha1/zz_generated.deepcopy.go index f7b889c7a..011bd1e9f 100644 --- a/api/v1alpha1/zz_generated.deepcopy.go +++ b/api/v1alpha1/zz_generated.deepcopy.go @@ -861,6 +861,11 @@ func (in *OIDCConfig) DeepCopyInto(out *OIDCConfig) { *out = make([]string, len(*in)) copy(*out, *in) } + if in.ExtraConfig != nil { + in, out := &in.ExtraConfig, &out.ExtraConfig + *out = new(OIDCExtraConfig) + **out = **in + } } // DeepCopy is an autogenerated deepcopy function, copying the receiver, creating a new OIDCConfig. @@ -873,6 +878,21 @@ func (in *OIDCConfig) DeepCopy() *OIDCConfig { return out } +// DeepCopyInto is an autogenerated deepcopy function, copying the receiver, writing into out. in must be non-nil. +func (in *OIDCExtraConfig) DeepCopyInto(out *OIDCExtraConfig) { + *out = *in +} + +// DeepCopy is an autogenerated deepcopy function, copying the receiver, creating a new OIDCExtraConfig. +func (in *OIDCExtraConfig) DeepCopy() *OIDCExtraConfig { + if in == nil { + return nil + } + out := new(OIDCExtraConfig) + in.DeepCopyInto(out) + return out +} + // DeepCopyInto is an autogenerated deepcopy function, copying the receiver, writing into out. in must be non-nil. func (in *OptionsOverride) DeepCopyInto(out *OptionsOverride) { *out = *in diff --git a/charts/manager/crds/greenhouse.sap_organizations.yaml b/charts/manager/crds/greenhouse.sap_organizations.yaml index e1336723d..275e530f4 100644 --- a/charts/manager/crds/greenhouse.sap_organizations.yaml +++ b/charts/manager/crds/greenhouse.sap_organizations.yaml @@ -92,6 +92,26 @@ spec: - key - name type: object + extraConfig: + description: ExtraConfig contains additional OIDC configuration + for claim mapping and token validation behavior. + properties: + insecureSkipEmailVerified: + default: false + description: |- + InsecureSkipEmailVerified, if set to true, forces the email_verified claim to true + regardless of the value reported by the upstream IdP, including overriding an explicit + email_verified=false. Enable only for IdPs where the email address is verified by other + means, e.g. Keycloak with SAML or user federation, or providers that omit the claim + entirely such as some Okta, EntraID or CloudFoundry configurations. + type: boolean + userIDClaim: + default: login_name + description: |- + UserIDClaim is the claim to be used as both user ID and username. + When set, it overrides both UserIDKey and UserNameKey in the dex OIDC connector config. + type: string + type: object issuer: description: Issuer is the URL of the identity service. type: string diff --git a/config/samples/organization/demo.yaml b/config/samples/organization/demo.yaml index c9f091bd4..051ad3796 100644 --- a/config/samples/organization/demo.yaml +++ b/config/samples/organization/demo.yaml @@ -65,3 +65,5 @@ spec: # name: demo-oidc # issuer: https://global.accounts.dev # redirectURI: https://bogus.accounts.foo +# extraConfig: +# userIDClaim: email diff --git a/docs/reference/api/index.html b/docs/reference/api/index.html index 67f0c179c..0d5f04161 100644 --- a/docs/reference/api/index.html +++ b/docs/reference/api/index.html @@ -2085,6 +2085,66 @@

OIDCConfig the client application, the URI requested to redirect to must be contained in this list.

+ + +extraConfig
+ + +OIDCExtraConfig + + + + +

ExtraConfig contains additional OIDC configuration for claim mapping and token validation behavior.

+ + + + + + +

OIDCExtraConfig +

+

+(Appears on: +OIDCConfig) +

+
+
+ + + + + + + + + + + + + + + +
FieldDescription
+insecureSkipEmailVerified
+ +bool + +
+

InsecureSkipEmailVerified, if set to true, forces the email_verified claim to true +regardless of the value reported by the upstream IdP, including overriding an explicit +email_verified=false. Enable only for IdPs where the email address is verified by other +means, e.g. Keycloak with SAML or user federation, or providers that omit the claim +entirely such as some Okta, EntraID or CloudFoundry configurations.

+
+userIDClaim
+ +string + +
+

UserIDClaim is the claim to be used as both user ID and username. +When set, it overrides both UserIDKey and UserNameKey in the dex OIDC connector config.

+
diff --git a/docs/reference/api/openapi.yaml b/docs/reference/api/openapi.yaml index 95a099a30..b2c3c5f6b 100755 --- a/docs/reference/api/openapi.yaml +++ b/docs/reference/api/openapi.yaml @@ -719,6 +719,25 @@ components: - key - name type: object + extraConfig: + description: ExtraConfig contains additional OIDC configuration for claim mapping and token validation behavior. + properties: + insecureSkipEmailVerified: + default: false + description: |- + InsecureSkipEmailVerified, if set to true, forces the email_verified claim to true + regardless of the value reported by the upstream IdP, including overriding an explicit + email_verified=false. Enable only for IdPs where the email address is verified by other + means, e.g. Keycloak with SAML or user federation, or providers that omit the claim + entirely such as some Okta, EntraID or CloudFoundry configurations. + type: boolean + userIDClaim: + default: login_name + description: |- + UserIDClaim is the claim to be used as both user ID and username. + When set, it overrides both UserIDKey and UserNameKey in the dex OIDC connector config. + type: string + type: object issuer: description: Issuer is the URL of the identity service. type: string diff --git a/internal/controller/organization/dex.go b/internal/controller/organization/dex.go old mode 100644 new mode 100755 index a027f8c2f..8b6d79c28 --- a/internal/controller/organization/dex.go +++ b/internal/controller/organization/dex.go @@ -113,15 +113,24 @@ func (r *OrganizationReconciler) reconcileDexConnector(ctx context.Context, org if err != nil { return err } + var userIDClaim = "login_name" + var skipEmailVerified = false + if org.Spec.Authentication.OIDCConfig.ExtraConfig != nil { + if org.Spec.Authentication.OIDCConfig.ExtraConfig.UserIDClaim != "" { + userIDClaim = org.Spec.Authentication.OIDCConfig.ExtraConfig.UserIDClaim + } + skipEmailVerified = org.Spec.Authentication.OIDCConfig.ExtraConfig.InsecureSkipEmailVerified + } oidcConfig := &oidc.Config{ - Issuer: org.Spec.Authentication.OIDCConfig.Issuer, - ClientID: clientID, - ClientSecret: clientSecret, - RedirectURI: redirectURL, - UserNameKey: "login_name", - UserIDKey: "login_name", - InsecureSkipVerify: true, - InsecureEnableGroups: true, + Issuer: org.Spec.Authentication.OIDCConfig.Issuer, + ClientID: clientID, + ClientSecret: clientSecret, + RedirectURI: redirectURL, + UserNameKey: userIDClaim, + UserIDKey: userIDClaim, + InsecureSkipEmailVerified: skipEmailVerified, + InsecureSkipVerify: true, + InsecureEnableGroups: true, } configByte, err := json.Marshal(oidcConfig) if err != nil { diff --git a/internal/controller/organization/organization_controller_test.go b/internal/controller/organization/organization_controller_test.go index e09efb785..ff7d77af6 100644 --- a/internal/controller/organization/organization_controller_test.go +++ b/internal/controller/organization/organization_controller_test.go @@ -4,10 +4,12 @@ package organization_test import ( + "encoding/json" "fmt" "slices" + dexoidc "github.com/dexidp/dex/connector/oidc" . "github.com/onsi/ginkgo/v2" . "github.com/onsi/gomega" . "github.com/onsi/gomega/gstruct" @@ -390,6 +392,61 @@ var _ = Describe("Test Organization reconciliation", Ordered, func() { } test.EventuallyDeleted(test.Ctx, test.K8sClient, team) }) + + It("should propagate ExtraConfig to dex connector", func() { + team := setup.CreateTeam(test.Ctx, "test-team-extra", test.WithTeamLabel(greenhouseapis.LabelKeySupportGroup, "true")) + + defaultOrg := setup.CreateDefaultOrgWithOIDCSecret(test.Ctx, team.Name) + test.EventuallyCreated(test.Ctx, test.K8sClient, &corev1.Namespace{ObjectMeta: metav1.ObjectMeta{Name: defaultOrg.Name}}) + Eventually(func(g Gomega) { + err := test.K8sClient.Get(test.Ctx, types.NamespacedName{Name: defaultOrg.Name}, defaultOrg) + g.Expect(err).ToNot(HaveOccurred()) + oidcCondition := defaultOrg.Status.GetConditionByType(greenhousev1alpha1.OrganizationOICDConfigured) + g.Expect(oidcCondition).ToNot(BeNil()) + g.Expect(oidcCondition.IsTrue()).To(BeTrue()) + }).Should(Succeed()) + + By("creating a test organization with ExtraConfig") + extraConfigOrg := setup.CreateOrganization(test.Ctx, "test-extraconfig-org", test.WithMappedAdminIDPGroup("EXTRA_CONFIG_GROUP")) + test.EventuallyCreated(test.Ctx, test.K8sClient, &corev1.Namespace{ObjectMeta: metav1.ObjectMeta{Name: extraConfigOrg.Name}}) + + oidcSecret := setup.CreateOrgOIDCSecret(test.Ctx, extraConfigOrg.Name, team.Name) + extraConfigOrg = setup.UpdateOrganization(test.Ctx, + extraConfigOrg.Name, + test.WithOIDCConfig(test.OIDCIssuer, oidcSecret.Name, test.OIDCClientIDKey, test.OIDCClientSecretKey), + test.WithOIDCExtraConfig("email", true), + ) + + By("checking Organization OIDC status is ready") + Eventually(func(g Gomega) { + err := test.K8sClient.Get(test.Ctx, types.NamespacedName{Name: extraConfigOrg.Name}, extraConfigOrg) + g.Expect(err).ToNot(HaveOccurred()) + oidcCondition := extraConfigOrg.Status.GetConditionByType(greenhousev1alpha1.OrganizationOICDConfigured) + g.Expect(oidcCondition).ToNot(BeNil(), "OrganizationOICDConfigured should be set") + g.Expect(oidcCondition.IsTrue()).To(BeTrue(), "OrganizationOICDConfigured should be True") + }).Should(Succeed()) + + if DexStorageType == dex.K8s { + By("verifying the dex connector config contains ExtraConfig values") + connectors := &dexapi.ConnectorList{} + err := setup.List(test.Ctx, connectors) + Expect(err).ToNot(HaveOccurred()) + + filteredConnectors := slices.DeleteFunc(connectors.Items, func(c dexapi.Connector) bool { + return c.ID != extraConfigOrg.Name + }) + Expect(filteredConnectors).To(HaveLen(1), "there should be exactly one connector for the org") + + var connectorConfig dexoidc.Config + Expect(json.Unmarshal(filteredConnectors[0].Config, &connectorConfig)).To(Succeed(), "connector config should be valid JSON") + Expect(connectorConfig.UserNameKey).To(Equal("email"), "UserNameKey should be set to the custom claim") + Expect(connectorConfig.UserIDKey).To(Equal("email"), "UserIDKey should be set to the custom claim") + Expect(connectorConfig.InsecureSkipEmailVerified).To(BeTrue(), "InsecureSkipEmailVerified should be true") + } + + test.EventuallyDeleted(test.Ctx, test.K8sClient, &greenhousev1alpha1.Organization{ObjectMeta: metav1.ObjectMeta{Name: extraConfigOrg.Name}}) + test.EventuallyDeleted(test.Ctx, test.K8sClient, team) + }) }) When("reconciling PluginDefinitionCatalog ServiceAccount for regular organization", Ordered, func() { diff --git a/internal/dex/connector.go b/internal/dex/connector.go index c74fd8080..19c0fab92 100644 --- a/internal/dex/connector.go +++ b/internal/dex/connector.go @@ -53,20 +53,22 @@ func (c *OIDCConfig) Open(id string, logger *slog.Logger) (connector.Connector, } return &oidcConnector{ - conn: conn, - logger: logger, - client: c.client, - id: id, - keepUpstreamGroups: c.KeepUpstreamGroups, + conn: conn, + logger: logger, + client: c.client, + id: id, + keepUpstreamGroups: c.KeepUpstreamGroups, + overrideEmailVerified: c.InsecureSkipEmailVerified, }, nil } type oidcConnector struct { - conn connector.Connector - logger *slog.Logger - client client.Client - id string - keepUpstreamGroups bool + conn connector.Connector + logger *slog.Logger + client client.Client + id string + keepUpstreamGroups bool + overrideEmailVerified bool } func (c *oidcConnector) LoginURL(s connector.Scopes, callbackURL, state string) (string, []byte, error) { //nolint:gocritic @@ -79,6 +81,15 @@ func (c *oidcConnector) HandleCallback(s connector.Scopes, connData []byte, r *h return identity, err } + // overrideEmailVerified forces the email_verified claim to true regardless of the value + // reported by the upstream IdP, including overriding an explicit email_verified=false. + // It is required for IdPs that report email_verified=false (or omit the claim) for users + // federated via SAML, where the email is verified by other means. Enabled per organization + // via the effective InsecureSkipEmailVerified flag. + if c.overrideEmailVerified { + identity.EmailVerified = true + } + groups, groupsErr := c.getGroups(c.id, identity.Groups, r.Context()) if groupsErr != nil { c.logger.Info("failed getting group", "groupID", c.id, "error", groupsErr) @@ -108,6 +119,11 @@ func (c *oidcConnector) Refresh(ctx context.Context, s connector.Scopes, identit return identity, err } + // Keep email_verified consistent with HandleCallback when the token is refreshed. + if c.overrideEmailVerified { + identity.EmailVerified = true + } + groups, groupsErr := c.getGroups(c.id, identity.Groups, ctx) if groupsErr != nil { c.logger.Info("failed getting groups", "connectorID", c.id, "error", groupsErr) diff --git a/internal/dex/connector_test.go b/internal/dex/connector_test.go index c8da16883..ad196cfda 100644 --- a/internal/dex/connector_test.go +++ b/internal/dex/connector_test.go @@ -6,6 +6,7 @@ package dex import ( "context" "log/slog" + "net/http" "os" "testing" @@ -142,3 +143,55 @@ var _ = Describe("Getting groups for token", func() { Expect(groups).To(Equal([]string{"organization:" + test.TestNamespace, "team:test-team-1", "test-team-category-1:test-team-1", "role:" + test.TestNamespace + ":admin", "team:test-team-2", "test-team-category-2:test-team-2", "team:test-team-3", "team:test-team-4"}), "The groups should be correct") }) }) + +// fakeUpstreamConnector is a minimal connector.Connector used to drive +// oidcConnector.HandleCallback and Refresh without a real upstream IdP. +type fakeUpstreamConnector struct { + identity connector.Identity +} + +var ( + _ connector.CallbackConnector = (*fakeUpstreamConnector)(nil) + _ connector.RefreshConnector = (*fakeUpstreamConnector)(nil) +) + +func (f *fakeUpstreamConnector) LoginURL(connector.Scopes, string, string) (loginURL string, connData []byte, err error) { + return "", nil, nil +} + +func (f *fakeUpstreamConnector) HandleCallback(connector.Scopes, []byte, *http.Request) (connector.Identity, error) { + return f.identity, nil +} + +func (f *fakeUpstreamConnector) Refresh(context.Context, connector.Scopes, connector.Identity) (connector.Identity, error) { + return f.identity, nil +} + +var _ = Describe("Overriding email_verified", func() { + It("Should force EmailVerified to true on HandleCallback when overrideEmailVerified is set", func() { + logger := slog.New(slog.NewJSONHandler(os.Stdout, nil)) + upstream := &fakeUpstreamConnector{identity: connector.Identity{Email: "user@example.com", EmailVerified: false}} + c := oidcConnector{conn: upstream, logger: logger, client: test.K8sClient, id: test.TestNamespace, overrideEmailVerified: true} + identity, err := c.HandleCallback(connector.Scopes{}, nil, &http.Request{}) + Expect(err).ToNot(HaveOccurred(), "There should be no error handling the callback") + Expect(identity.EmailVerified).To(BeTrue(), "EmailVerified should be forced to true even though the upstream reported false") + }) + + It("Should keep the upstream EmailVerified on HandleCallback when overrideEmailVerified is not set", func() { + logger := slog.New(slog.NewJSONHandler(os.Stdout, nil)) + upstream := &fakeUpstreamConnector{identity: connector.Identity{Email: "user@example.com", EmailVerified: false}} + c := oidcConnector{conn: upstream, logger: logger, client: test.K8sClient, id: test.TestNamespace, overrideEmailVerified: false} + identity, err := c.HandleCallback(connector.Scopes{}, nil, &http.Request{}) + Expect(err).ToNot(HaveOccurred(), "There should be no error handling the callback") + Expect(identity.EmailVerified).To(BeFalse(), "EmailVerified should reflect the upstream value") + }) + + It("Should force EmailVerified to true on Refresh when overrideEmailVerified is set", func() { + logger := slog.New(slog.NewJSONHandler(os.Stdout, nil)) + upstream := &fakeUpstreamConnector{identity: connector.Identity{Email: "user@example.com", EmailVerified: false}} + c := oidcConnector{conn: upstream, logger: logger, client: test.K8sClient, id: test.TestNamespace, overrideEmailVerified: true} + identity, err := c.Refresh(context.TODO(), connector.Scopes{}, connector.Identity{}) + Expect(err).ToNot(HaveOccurred(), "There should be no error refreshing the identity") + Expect(identity.EmailVerified).To(BeTrue(), "EmailVerified should be forced to true even though the upstream reported false") + }) +}) diff --git a/internal/test/resources.go b/internal/test/resources.go index 190eedd4b..d7faab166 100644 --- a/internal/test/resources.go +++ b/internal/test/resources.go @@ -133,6 +133,24 @@ func WithOIDCConfig(issuer, secretName, clientIDKey, clientSecretKey string) fun } } +// WithOIDCExtraConfig sets the OIDCExtraConfig on an Organization's OIDCConfig. +// Must be used after WithOIDCConfig: WithOIDCConfig replaces OIDCConfig wholesale, +// so any ExtraConfig set beforehand would be discarded. +func WithOIDCExtraConfig(userIDClaim string, insecureSkipEmailVerified bool) func(*greenhousev1alpha1.Organization) { + return func(org *greenhousev1alpha1.Organization) { + if org.Spec.Authentication == nil { + org.Spec.Authentication = &greenhousev1alpha1.Authentication{} + } + if org.Spec.Authentication.OIDCConfig == nil { + org.Spec.Authentication.OIDCConfig = &greenhousev1alpha1.OIDCConfig{} + } + org.Spec.Authentication.OIDCConfig.ExtraConfig = &greenhousev1alpha1.OIDCExtraConfig{ + UserIDClaim: userIDClaim, + InsecureSkipEmailVerified: insecureSkipEmailVerified, + } + } +} + // NewOrganization returns a greenhousev1alpha1.Organization object. Opts can be used to set the desired state of the Organization. func NewOrganization(ctx context.Context, name string, opts ...func(*greenhousev1alpha1.Organization)) *greenhousev1alpha1.Organization { org := &greenhousev1alpha1.Organization{ diff --git a/types/typescript/schema.d.ts b/types/typescript/schema.d.ts index 5f3987e5c..41ebe1fa5 100644 --- a/types/typescript/schema.d.ts +++ b/types/typescript/schema.d.ts @@ -556,6 +556,21 @@ export interface components { /** @description Name of the secret in the same namespace. */ name: string; }; + /** @description ExtraConfig contains additional OIDC configuration for claim mapping and token validation behavior. */ + extraConfig?: { + /** + * @description InsecureSkipEmailVerified if set to true, treats email_verified as true when the claim is absent from the ID token. + * This does not override an explicit email_verified=false. Only enable for providers that omit the claim entirely (e.g. some Okta, EntraID or CloudFoundry configurations). + * @default false + */ + insecureSkipEmailVerified: boolean; + /** + * @description UserIDClaim is the claim to be used as both user ID and username. + * When set, it overrides both UserIDKey and UserNameKey in the dex OIDC connector config. + * @default login_name + */ + userIDClaim: string; + }; /** @description Issuer is the URL of the identity service. */ issuer: string; /**