From d0d526767850b98e2454fa895862793f5bbb5bd8 Mon Sep 17 00:00:00 2001 From: Henning Perl Date: Thu, 28 Nov 2024 14:19:04 +0100 Subject: [PATCH 01/13] WIP --- embedx/config.schema.json | 69 +++++++++++++++++++++++++++ identity/credentials.go | 5 +- internal/client-go/go.sum | 1 + selfservice/strategy/oidc/strategy.go | 16 ++++++- 4 files changed, 88 insertions(+), 3 deletions(-) diff --git a/embedx/config.schema.json b/embedx/config.schema.json index 6a1cc1c90ede..7bb1afbe756e 100644 --- a/embedx/config.schema.json +++ b/embedx/config.schema.json @@ -666,6 +666,49 @@ } ] }, + "selfServiceSAMLProvider": { + "type": "object", + "properties": { + "id": { + "type": "string", + "examples": ["be2b30", "my-saml-provider"] + }, + "label": { + "title": "Optional string which will be used when generating labels for UI buttons.", + "type": "string" + }, + "mapper_url": { + "title": "Jsonnet Mapper URL", + "description": "The URL where the jsonnet source is located for mapping the provider's data to Ory Kratos data.", + "type": "string", + "format": "uri", + "examples": [ + "file://path/to/oidc.jsonnet", + "https://foo.bar.com/path/to/oidc.jsonnet", + "base64://bG9jYWwgc3ViamVjdCA9I..." + ] + }, + "raw_idp_metadata_xml": { + "title": "Raw IDP Metadata XML", + "description": "The raw IDP metadata XML for the SAML provider.", + "type": "string", + "format": "uri", + "examples": [ + "file://path/to/metadata.xml", + "https://foo.bar.com/path/to/metadata.xml", + "base64://bG9jYWwgc3ViamVjdCA9I..." + ] + }, + "organization_id": { + "title": "Organization ID", + "description": "The ID of the organization that this provider belongs to. Only effective in the Ory Network.", + "type": "string", + "examples": ["12345678-1234-1234-1234-123456789012"] + } + }, + "additionalProperties": true, + "required": ["id", "raw_idp_metadata_xml", "mapper_url"] + }, "selfServiceHooks": { "type": "array", "items": { @@ -1959,6 +2002,32 @@ } } } + }, + "saml": { + "type": "object", + "title": "Specify SAML configuration", + "additionalProperties": false, + "properties": { + "enabled": { + "type": "boolean", + "title": "Enables the SAML method", + "default": false + }, + "config": { + "type": "object", + "additionalProperties": false, + "properties": { + "providers": { + "title": "SAML Providers", + "description": "A list and configuration of SAML providers Ory Kratos should integrate with.", + "type": "array", + "items": { + "$ref": "#/definitions/selfServiceSAMLProvider" + } + } + } + } + } } } } diff --git a/identity/credentials.go b/identity/credentials.go index 9fc2d93851bb..12a2669c4b02 100644 --- a/identity/credentials.go +++ b/identity/credentials.go @@ -89,6 +89,7 @@ const ( CredentialsTypeCodeAuth CredentialsType = "code" CredentialsTypePasskey CredentialsType = "passkey" CredentialsTypeProfile CredentialsType = "profile" + CredentialsTypeSAML CredentialsType = "saml" ) func (c CredentialsType) String() string { @@ -99,7 +100,7 @@ func (c CredentialsType) ToUiNodeGroup() node.UiNodeGroup { switch c { case CredentialsTypePassword: return node.PasswordGroup - case CredentialsTypeOIDC: + case CredentialsTypeOIDC, CredentialsTypeSAML: return node.OpenIDConnectGroup case CredentialsTypeTOTP: return node.TOTPGroup @@ -124,6 +125,7 @@ var AllCredentialTypes = []CredentialsType{ CredentialsTypeWebAuthn, CredentialsTypeCodeAuth, CredentialsTypePasskey, + CredentialsTypeSAML, } const ( @@ -138,6 +140,7 @@ func ParseCredentialsType(in string) (CredentialsType, bool) { for _, t := range []CredentialsType{ CredentialsTypePassword, CredentialsTypeOIDC, + CredentialsTypeSAML, CredentialsTypeTOTP, CredentialsTypeLookup, CredentialsTypeWebAuthn, diff --git a/internal/client-go/go.sum b/internal/client-go/go.sum index c966c8ddfd0d..6cc3f5911d11 100644 --- a/internal/client-go/go.sum +++ b/internal/client-go/go.sum @@ -4,6 +4,7 @@ github.com/golang/protobuf v1.2.0/go.mod h1:6lQm79b+lXiMfvg/cZm0SGofjICqVBUtrP5y golang.org/x/net v0.0.0-20180724234803-3673e40ba225/go.mod h1:mL1N/T3taQHkDXs73rZJwtUhF3w3ftmwwsq0BUmARs4= golang.org/x/net v0.0.0-20190108225652-1e06a53dbb7e h1:bRhVy7zSSasaqNksaRZiA5EEI+Ei4I1nO5Jh72wfHlg= golang.org/x/net v0.0.0-20190108225652-1e06a53dbb7e/go.mod h1:mL1N/T3taQHkDXs73rZJwtUhF3w3ftmwwsq0BUmARs4= +golang.org/x/oauth2 v0.0.0-20190604053449-0f29369cfe45/go.mod h1:gOpvHmFTYa4IltrdGE7lF6nIHvwfUNPOp7c8zoXwtLw= golang.org/x/sync v0.0.0-20181221193216-37e7f081c4d4 h1:YUO/7uOKsKeq9UokNS62b8FYywz3ker1l1vDZRCRefw= golang.org/x/sync v0.0.0-20181221193216-37e7f081c4d4/go.mod h1:RxMgew5VJxzue5/jJTE5uejpjVlOe/izrB70Jof72aM= golang.org/x/text v0.3.0/go.mod h1:NqM8EUOU14njkJ3fqMW+pc6Ldnwhi/IjpwHt7yyuwOQ= diff --git a/selfservice/strategy/oidc/strategy.go b/selfservice/strategy/oidc/strategy.go index f2837e769b73..78ba2d6bd897 100644 --- a/selfservice/strategy/oidc/strategy.go +++ b/selfservice/strategy/oidc/strategy.go @@ -131,6 +131,7 @@ type Strategy struct { d Dependencies validator *schema.Validator dec *decoderx.HTTP + credType identity.CredentialsType } type AuthCodeContainer struct { @@ -212,11 +213,22 @@ func (s *Strategy) redirectToGET(w http.ResponseWriter, r *http.Request, _ httpr http.Redirect(w, r, dest.String(), http.StatusFound) } -func NewStrategy(d any) *Strategy { - return &Strategy{ +type NewStrategyOpt func(s *Strategy) + +func ForCredentialType(ct identity.CredentialsType) NewStrategyOpt { + return func(s *Strategy) { s.credType = ct } +} + +func NewStrategy(d any, opts ...NewStrategyOpt) *Strategy { + s := &Strategy{ d: d.(Dependencies), validator: schema.NewValidator(), } + for _, opt := range opts { + opt(s) + } + + return s } func (s *Strategy) ID() identity.CredentialsType { From 9ac6bd8adcf85c6a824c6beb2f055810670c550e Mon Sep 17 00:00:00 2001 From: Henning Perl Date: Tue, 3 Dec 2024 14:22:32 +0100 Subject: [PATCH 02/13] feat: jackson provider --- driver/config/config.go | 5 + embedx/config.schema.json | 16 ++++ embedx/embedx.go | 6 +- .../sql/identity/persister_identity.go | 2 +- selfservice/strategy/oidc/provider_config.go | 1 + .../strategy/oidc/provider_generic_oidc.go | 4 +- selfservice/strategy/oidc/provider_jackson.go | 96 +++++++++++++++++++ selfservice/strategy/oidc/strategy.go | 49 ++++------ selfservice/strategy/oidc/strategy_login.go | 10 +- .../strategy/oidc/strategy_registration.go | 2 +- 10 files changed, 153 insertions(+), 38 deletions(-) create mode 100644 selfservice/strategy/oidc/provider_jackson.go diff --git a/driver/config/config.go b/driver/config/config.go index 4eb0566963d2..b9e4019f1353 100644 --- a/driver/config/config.go +++ b/driver/config/config.go @@ -193,6 +193,7 @@ const ( ViperKeyIgnoreNetworkErrors = "selfservice.methods.password.config.ignore_network_errors" ViperKeyTOTPIssuer = "selfservice.methods.totp.config.issuer" ViperKeyOIDCBaseRedirectURL = "selfservice.methods.oidc.config.base_redirect_uri" + ViperKeySAMLBaseRedirectURL = "selfservice.methods.saml.config.base_redirect_uri" ViperKeyWebAuthnRPDisplayName = "selfservice.methods.webauthn.config.rp.display_name" ViperKeyWebAuthnRPID = "selfservice.methods.webauthn.config.rp.id" ViperKeyWebAuthnRPOrigin = "selfservice.methods.webauthn.config.rp.origin" @@ -616,6 +617,10 @@ func (p *Config) OIDCRedirectURIBase(ctx context.Context) *url.URL { return p.GetProvider(ctx).URIF(ViperKeyOIDCBaseRedirectURL, p.SelfPublicURL(ctx)) } +func (p *Config) SAMLRedirectURIBase(ctx context.Context) *url.URL { + return p.GetProvider(ctx).URIF(ViperKeySAMLBaseRedirectURL, p.SelfPublicURL(ctx)) +} + func (p *Config) IdentityTraitsSchemas(ctx context.Context) (ss Schemas, err error) { if err = p.GetProvider(ctx).Koanf.Unmarshal(ViperKeyIdentitySchemas, &ss); err != nil { return ss, nil diff --git a/embedx/config.schema.json b/embedx/config.schema.json index 7bb1afbe756e..a1a28750472f 100644 --- a/embedx/config.schema.json +++ b/embedx/config.schema.json @@ -906,6 +906,9 @@ "oidc": { "$ref": "#/definitions/selfServiceAfterSettingsAuthMethod" }, + "saml": { + "$ref": "#/definitions/selfServiceAfterSettingsAuthMethod" + }, "webauthn": { "$ref": "#/definitions/selfServiceAfterSettingsAuthMethod" }, @@ -951,6 +954,9 @@ "oidc": { "$ref": "#/definitions/selfServiceAfterOIDCLoginMethod" }, + "saml": { + "$ref": "#/definitions/selfServiceAfterOIDCLoginMethod" + }, "code": { "$ref": "#/definitions/selfServiceAfterDefaultLoginMethod" }, @@ -1044,6 +1050,9 @@ "oidc": { "$ref": "#/definitions/selfServiceAfterRegistrationMethod" }, + "saml": { + "$ref": "#/definitions/selfServiceAfterRegistrationMethod" + }, "code": { "$ref": "#/definitions/selfServiceAfterRegistrationMethod" }, @@ -2017,6 +2026,13 @@ "type": "object", "additionalProperties": false, "properties": { + "base_redirect_uri": { + "type": "string", + "title": "Base URL for SAML Redirect URIs", + "description": "Can be used to modify the base URL for SAML Redirect URLs. If unset, the Public Base URL will be used.", + "format": "uri", + "examples": ["https://auth.myexample.org/"] + }, "providers": { "title": "SAML Providers", "description": "A list and configuration of SAML providers Ory Kratos should integrate with.", diff --git a/embedx/embedx.go b/embedx/embedx.go index b91d86b8f692..5212337f4ea2 100644 --- a/embedx/embedx.go +++ b/embedx/embedx.go @@ -5,15 +5,13 @@ package embedx import ( "bytes" + _ "embed" "io" "github.com/pkg/errors" - - "github.com/ory/x/otelx" - "github.com/tidwall/gjson" - _ "embed" + "github.com/ory/x/otelx" ) //go:embed config.schema.json diff --git a/persistence/sql/identity/persister_identity.go b/persistence/sql/identity/persister_identity.go index 489b1fbb4360..8e579888d759 100644 --- a/persistence/sql/identity/persister_identity.go +++ b/persistence/sql/identity/persister_identity.go @@ -1332,7 +1332,7 @@ func FindIdentityCredentialsTypeByName(con *pop.Connection, ct identity.Credenti } if !found { - return uuid.Nil, errors.WithStack(herodot.ErrInternalServerError.WithReasonf("The SQL adapter failed to return the appropriate credentials_type for nane %s. This is a bug in the code.", ct)) + return uuid.Nil, errors.WithStack(herodot.ErrInternalServerError.WithReasonf("The SQL adapter failed to return the appropriate credentials_type for name %q. This is a bug in the code.", ct)) } return result, nil diff --git a/selfservice/strategy/oidc/provider_config.go b/selfservice/strategy/oidc/provider_config.go index 7e2b0b19dbfb..3a4e2c5808e9 100644 --- a/selfservice/strategy/oidc/provider_config.go +++ b/selfservice/strategy/oidc/provider_config.go @@ -178,6 +178,7 @@ var supportedProviders = map[string]func(config *Configuration, reg Dependencies "patreon": NewProviderPatreon, "lark": NewProviderLark, "x": NewProviderX, + "jackson": NewProviderJackson, } func (c ConfigurationCollection) Provider(id string, reg Dependencies) (Provider, error) { diff --git a/selfservice/strategy/oidc/provider_generic_oidc.go b/selfservice/strategy/oidc/provider_generic_oidc.go index 146505165807..f001b3e9dbff 100644 --- a/selfservice/strategy/oidc/provider_generic_oidc.go +++ b/selfservice/strategy/oidc/provider_generic_oidc.go @@ -6,6 +6,7 @@ package oidc import ( "context" "net/url" + "slices" "github.com/pkg/errors" "golang.org/x/oauth2" @@ -13,7 +14,6 @@ import ( gooidc "github.com/coreos/go-oidc/v3/oidc" "github.com/ory/herodot" - "github.com/ory/x/stringslice" ) var _ Provider = new(ProviderGenericOIDC) @@ -60,7 +60,7 @@ func (g *ProviderGenericOIDC) provider(ctx context.Context) (*gooidc.Provider, e func (g *ProviderGenericOIDC) oauth2ConfigFromEndpoint(ctx context.Context, endpoint oauth2.Endpoint) *oauth2.Config { scope := g.config.Scope - if !stringslice.Has(scope, gooidc.ScopeOpenID) { + if !slices.Contains(scope, gooidc.ScopeOpenID) { scope = append(scope, gooidc.ScopeOpenID) } diff --git a/selfservice/strategy/oidc/provider_jackson.go b/selfservice/strategy/oidc/provider_jackson.go new file mode 100644 index 000000000000..3a3970dcd34a --- /dev/null +++ b/selfservice/strategy/oidc/provider_jackson.go @@ -0,0 +1,96 @@ +// Copyright © 2023 Ory Corp +// SPDX-License-Identifier: Apache-2.0 + +package oidc + +import ( + "context" + "net/url" + + gooidc "github.com/coreos/go-oidc/v3/oidc" + "github.com/pkg/errors" + "golang.org/x/oauth2" + + "github.com/ory/herodot" + "github.com/ory/x/urlx" +) + +type ProviderJackson struct { + *ProviderGenericOIDC +} + +func NewProviderJackson( + config *Configuration, + reg Dependencies, +) Provider { + return &ProviderJackson{ + ProviderGenericOIDC: &ProviderGenericOIDC{ + config: config, + reg: reg, + }, + } +} + +func (j *ProviderJackson) setProvider(ctx context.Context) { + if j.ProviderGenericOIDC.p == nil { + config := gooidc.ProviderConfig{ + IssuerURL: j.config.IssuerURL, + AuthURL: j.config.AuthURL, + TokenURL: j.config.TokenURL, + DeviceAuthURL: "", + UserInfoURL: j.config.IssuerURL + "/api/oauth/userinfo", + JWKSURL: j.config.IssuerURL + "/oauth/jwks", + Algorithms: []string{"RS256"}, + } + j.ProviderGenericOIDC.p = config.NewProvider(j.withHTTPClientContext(ctx)) + } +} + +func (j *ProviderJackson) OAuth2(ctx context.Context) (*oauth2.Config, error) { + j.setProvider(ctx) + endpoint := j.ProviderGenericOIDC.p.Endpoint() + config := j.oauth2ConfigFromEndpoint(ctx, endpoint) + config.RedirectURL = urlx.AppendPaths( + j.reg.Config().SAMLRedirectURIBase(ctx), + "/self-service/methods/saml/callback/"+j.config.ID).String() + + return j.ProviderGenericOIDC.OAuth2(ctx) +} + +func (j *ProviderJackson) Claims(ctx context.Context, exchange *oauth2.Token, query url.Values) (*Claims, error) { + j.setProvider(ctx) + return j.claimsFromIDToken(ctx, exchange) +} + +func (j *ProviderJackson) claimsFromIDToken(ctx context.Context, exchange *oauth2.Token) (*Claims, error) { + p, raw, err := j.idTokenAndProvider(ctx, exchange) + if err != nil { + return nil, err + } + + return j.verifyAndDecodeClaimsWithProvider(ctx, p, raw) +} + +func (j *ProviderJackson) verifyAndDecodeClaimsWithProvider(ctx context.Context, provider *gooidc.Provider, raw string) (*Claims, error) { + verifier := provider.VerifierContext(j.withHTTPClientContext(ctx), &gooidc.Config{ + ClientID: j.config.ClientID, + SkipIssuerCheck: true, + }) + token, err := verifier.Verify(ctx, raw) + if err != nil { + return nil, errors.WithStack(herodot.ErrBadRequest.WithReasonf("%s", err)) + } + + var claims Claims + if err := token.Claims(&claims); err != nil { + return nil, errors.WithStack(herodot.ErrBadRequest.WithReasonf("%s", err)) + } + + var rawClaims map[string]interface{} + if err := token.Claims(&rawClaims); err != nil { + return nil, errors.WithStack(herodot.ErrBadRequest.WithReasonf("%s", err)) + } + claims.RawClaims = rawClaims + + return &claims, nil +} diff --git a/selfservice/strategy/oidc/strategy.go b/selfservice/strategy/oidc/strategy.go index 78ba2d6bd897..0a80c6d36f01 100644 --- a/selfservice/strategy/oidc/strategy.go +++ b/selfservice/strategy/oidc/strategy.go @@ -14,39 +14,19 @@ import ( "strings" "time" - "github.com/ory/x/sqlxx" - - "golang.org/x/exp/maps" - - "github.com/ory/x/urlx" - - "go.opentelemetry.io/otel/attribute" - "golang.org/x/oauth2" - - "github.com/ory/kratos/cipher" - oidcv1 "github.com/ory/kratos/gen/oidc/v1" - "github.com/ory/kratos/selfservice/sessiontokenexchange" - "github.com/ory/x/jsonnetsecure" - "github.com/ory/x/otelx" - - "github.com/ory/kratos/text" - - "github.com/ory/kratos/ui/container" - "github.com/ory/x/decoderx" - "github.com/ory/x/stringsx" - - "github.com/ory/kratos/ui/node" - "github.com/gofrs/uuid" "github.com/julienschmidt/httprouter" "github.com/pkg/errors" "github.com/tidwall/gjson" - - "github.com/ory/x/jsonx" + "go.opentelemetry.io/otel/attribute" + "golang.org/x/exp/maps" + "golang.org/x/oauth2" "github.com/ory/herodot" + "github.com/ory/kratos/cipher" "github.com/ory/kratos/continuity" "github.com/ory/kratos/driver/config" + oidcv1 "github.com/ory/kratos/gen/oidc/v1" "github.com/ory/kratos/identity" "github.com/ory/kratos/schema" "github.com/ory/kratos/selfservice/errorx" @@ -54,10 +34,19 @@ import ( "github.com/ory/kratos/selfservice/flow/login" "github.com/ory/kratos/selfservice/flow/registration" "github.com/ory/kratos/selfservice/flow/settings" - + "github.com/ory/kratos/selfservice/sessiontokenexchange" "github.com/ory/kratos/selfservice/strategy" "github.com/ory/kratos/session" + "github.com/ory/kratos/text" + "github.com/ory/kratos/ui/container" + "github.com/ory/kratos/ui/node" "github.com/ory/kratos/x" + "github.com/ory/x/decoderx" + "github.com/ory/x/jsonnetsecure" + "github.com/ory/x/otelx" + "github.com/ory/x/sqlxx" + "github.com/ory/x/stringsx" + "github.com/ory/x/urlx" ) const ( @@ -223,6 +212,7 @@ func NewStrategy(d any, opts ...NewStrategyOpt) *Strategy { s := &Strategy{ d: d.(Dependencies), validator: schema.NewValidator(), + credType: identity.CredentialsTypeOIDC, } for _, opt := range opts { opt(s) @@ -232,7 +222,7 @@ func NewStrategy(d any, opts ...NewStrategyOpt) *Strategy { } func (s *Strategy) ID() identity.CredentialsType { - return identity.CredentialsTypeOIDC + return s.credType } func (s *Strategy) validateFlow(ctx context.Context, r *http.Request, rid uuid.UUID) (flow.Flow, error) { @@ -542,8 +532,9 @@ func (s *Strategy) Config(ctx context.Context) (*ConfigurationCollection, error) var c ConfigurationCollection conf := s.d.Config().SelfServiceStrategy(ctx, string(s.ID())).Config - if err := jsonx. - NewStrictDecoder(bytes.NewBuffer(conf)). + // TODO: Better handle difference betweeen SAML and OIDC provider here, by not writing the raw XML metadata into the config at all. + if err := json. + NewDecoder(bytes.NewBuffer(conf)). Decode(&c); err != nil { s.d.Logger().WithError(err).WithField("config", conf) return nil, errors.WithStack(herodot.ErrInternalServerError.WithReasonf("Unable to decode OpenID Connect Provider configuration: %s", err)) diff --git a/selfservice/strategy/oidc/strategy_login.go b/selfservice/strategy/oidc/strategy_login.go index 07be40194d40..d9a9789edde3 100644 --- a/selfservice/strategy/oidc/strategy_login.go +++ b/selfservice/strategy/oidc/strategy_login.go @@ -102,7 +102,7 @@ func (s *Strategy) processLogin(ctx context.Context, w http.ResponseWriter, r *h ctx, span := s.d.Tracer(ctx).Tracer().Start(ctx, "selfservice.strategy.oidc.strategy.processLogin") defer otelx.End(span, &err) - i, c, err := s.d.PrivilegedIdentityPool().FindByCredentialsIdentifier(ctx, identity.CredentialsTypeOIDC, identity.OIDCUniqueID(provider.Config().ID, claims.Subject)) + i, c, err := s.d.PrivilegedIdentityPool().FindByCredentialsIdentifier(ctx, s.ID(), identity.OIDCUniqueID(provider.Config().ID, claims.Subject)) if err != nil { if errors.Is(err, sqlcon.ErrNoRows) { // If no account was found we're "manually" creating a new registration flow and redirecting the browser @@ -206,6 +206,14 @@ func (s *Strategy) Login(w http.ResponseWriter, r *http.Request, f *login.Flow, return nil, errors.WithStack(flow.ErrStrategyNotResponsible) } + // This is a hack for a lack of a `method` field in the form body. + if prefix, _, ok := strings.Cut(pid, ":"); ok { + if prefix != s.ID().String() { + span.SetAttributes(attribute.String("not_responsible_reason", "provider ID prefix does not match strategy")) + return nil, errors.WithStack(flow.ErrStrategyNotResponsible) + } + } + if !strings.EqualFold(strings.ToLower(p.Method), s.SettingsStrategyID()) && p.Method != "" { // the user is sending a method that is not oidc, but the payload includes a provider s.d.Audit(). diff --git a/selfservice/strategy/oidc/strategy_registration.go b/selfservice/strategy/oidc/strategy_registration.go index 82737df36a9d..c7a6e985baa7 100644 --- a/selfservice/strategy/oidc/strategy_registration.go +++ b/selfservice/strategy/oidc/strategy_registration.go @@ -347,7 +347,7 @@ func (s *Strategy) processRegistration(ctx context.Context, w http.ResponseWrite } i.SetCredentials(s.ID(), *creds) - if err := s.d.RegistrationExecutor().PostRegistrationHook(w, r, identity.CredentialsTypeOIDC, provider.Config().ID, provider.Config().OrganizationID, rf, i); err != nil { + if err := s.d.RegistrationExecutor().PostRegistrationHook(w, r, s.ID(), provider.Config().ID, provider.Config().OrganizationID, rf, i); err != nil { return nil, s.handleError(ctx, w, r, rf, provider.Config().ID, i.Traits, err) } From f6c0fc3796d71cae00ea3977b040dfdfd476c56e Mon Sep 17 00:00:00 2001 From: Henning Perl Date: Wed, 4 Dec 2024 14:13:26 +0100 Subject: [PATCH 03/13] fix redir URL --- selfservice/strategy/oidc/provider_jackson.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/selfservice/strategy/oidc/provider_jackson.go b/selfservice/strategy/oidc/provider_jackson.go index 3a3970dcd34a..f1737f5d26f9 100644 --- a/selfservice/strategy/oidc/provider_jackson.go +++ b/selfservice/strategy/oidc/provider_jackson.go @@ -54,7 +54,7 @@ func (j *ProviderJackson) OAuth2(ctx context.Context) (*oauth2.Config, error) { j.reg.Config().SAMLRedirectURIBase(ctx), "/self-service/methods/saml/callback/"+j.config.ID).String() - return j.ProviderGenericOIDC.OAuth2(ctx) + return config, nil } func (j *ProviderJackson) Claims(ctx context.Context, exchange *oauth2.Token, query url.Values) (*Claims, error) { From 0e5ee4cda5939e7a5777870555aebb1ef4910e00 Mon Sep 17 00:00:00 2001 From: Henning Perl Date: Fri, 6 Dec 2024 16:11:34 +0100 Subject: [PATCH 04/13] support SAML as org strategy --- selfservice/flow/login/handler.go | 2 +- selfservice/strategy/oidc/strategy.go | 1 - selfservice/strategy/oidc/strategy_registration.go | 7 +++++++ 3 files changed, 8 insertions(+), 2 deletions(-) diff --git a/selfservice/flow/login/handler.go b/selfservice/flow/login/handler.go index 267b33217216..f98ba66cd3fb 100644 --- a/selfservice/flow/login/handler.go +++ b/selfservice/flow/login/handler.go @@ -233,7 +233,7 @@ preLoginHook: // We only apply the filter on AAL1, because the OIDC strategy can only satsify // AAL1. strategyFilters = []StrategyFilter{func(s Strategy) bool { - return s.ID() == identity.CredentialsTypeOIDC + return s.ID() == identity.CredentialsTypeOIDC || s.ID() == identity.CredentialsTypeSAML }} } } diff --git a/selfservice/strategy/oidc/strategy.go b/selfservice/strategy/oidc/strategy.go index 99429385fff6..e58924af14c5 100644 --- a/selfservice/strategy/oidc/strategy.go +++ b/selfservice/strategy/oidc/strategy.go @@ -528,7 +528,6 @@ func (s *Strategy) Config(ctx context.Context) (*ConfigurationCollection, error) var c ConfigurationCollection conf := s.d.Config().SelfServiceStrategy(ctx, string(s.ID())).Config - // TODO: Better handle difference betweeen SAML and OIDC provider here, by not writing the raw XML metadata into the config at all. if err := json. NewDecoder(bytes.NewBuffer(conf)). Decode(&c); err != nil { diff --git a/selfservice/strategy/oidc/strategy_registration.go b/selfservice/strategy/oidc/strategy_registration.go index 05a7963c3763..b42fa476d33d 100644 --- a/selfservice/strategy/oidc/strategy_registration.go +++ b/selfservice/strategy/oidc/strategy_registration.go @@ -164,6 +164,13 @@ func (s *Strategy) Register(w http.ResponseWriter, r *http.Request, f *registrat span.SetAttributes(attribute.String("not_responsible_reason", "provider ID missing")) return errors.WithStack(flow.ErrStrategyNotResponsible) } + // This is a hack for a lack of a `method` field in the form body. + if prefix, _, ok := strings.Cut(pid, ":"); ok { + if prefix != s.ID().String() { + span.SetAttributes(attribute.String("not_responsible_reason", "provider ID prefix does not match strategy")) + return errors.WithStack(flow.ErrStrategyNotResponsible) + } + } f.TransientPayload = p.TransientPayload f.IDToken = p.IDToken From 3d50fc300f3ddcb5c92159ca8222b870fa625c6d Mon Sep 17 00:00:00 2001 From: Henning Perl Date: Mon, 9 Dec 2024 15:13:51 +0100 Subject: [PATCH 05/13] fix registration --- selfservice/flow/registration/handler.go | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/selfservice/flow/registration/handler.go b/selfservice/flow/registration/handler.go index de52abefd5a7..941eb0ecd478 100644 --- a/selfservice/flow/registration/handler.go +++ b/selfservice/flow/registration/handler.go @@ -141,7 +141,10 @@ func (h *Handler) NewRegistrationFlow(w http.ResponseWriter, r *http.Request, ft h.d.Logger().WithError(err).Warnf("ignoring invalid UUID %q in query parameter `organization`", rawOrg) } else { f.OrganizationID = uuid.NullUUID{UUID: orgID, Valid: true} - strategyFilters = []StrategyFilter{func(s Strategy) bool { return s.ID() == identity.CredentialsTypeOIDC }} + strategyFilters = []StrategyFilter{func(s Strategy) bool { + return s.ID() == identity.CredentialsTypeOIDC || + s.ID() == identity.CredentialsTypeSAML + }} } } for _, s := range h.d.RegistrationStrategies(r.Context(), strategyFilters...) { From fbfced10d3c6b39458880dc852fe3bd9cf794a9d Mon Sep 17 00:00:00 2001 From: Henning Perl Date: Tue, 10 Dec 2024 20:33:18 +0100 Subject: [PATCH 06/13] simplify, coverage --- selfservice/flow/registration/handler.go | 3 +- selfservice/strategy/oidc/provider_jackson.go | 51 +++---------------- .../strategy/oidc/provider_jackson_test.go | 33 ++++++++++++ 3 files changed, 40 insertions(+), 47 deletions(-) create mode 100644 selfservice/strategy/oidc/provider_jackson_test.go diff --git a/selfservice/flow/registration/handler.go b/selfservice/flow/registration/handler.go index 941eb0ecd478..c3f46d3a8397 100644 --- a/selfservice/flow/registration/handler.go +++ b/selfservice/flow/registration/handler.go @@ -142,8 +142,7 @@ func (h *Handler) NewRegistrationFlow(w http.ResponseWriter, r *http.Request, ft } else { f.OrganizationID = uuid.NullUUID{UUID: orgID, Valid: true} strategyFilters = []StrategyFilter{func(s Strategy) bool { - return s.ID() == identity.CredentialsTypeOIDC || - s.ID() == identity.CredentialsTypeSAML + return s.ID() == identity.CredentialsTypeOIDC || s.ID() == identity.CredentialsTypeSAML }} } } diff --git a/selfservice/strategy/oidc/provider_jackson.go b/selfservice/strategy/oidc/provider_jackson.go index f1737f5d26f9..f83a88306e62 100644 --- a/selfservice/strategy/oidc/provider_jackson.go +++ b/selfservice/strategy/oidc/provider_jackson.go @@ -5,13 +5,11 @@ package oidc import ( "context" - "net/url" + "strings" - gooidc "github.com/coreos/go-oidc/v3/oidc" - "github.com/pkg/errors" + "github.com/coreos/go-oidc/v3/oidc" "golang.org/x/oauth2" - "github.com/ory/herodot" "github.com/ory/x/urlx" ) @@ -33,13 +31,14 @@ func NewProviderJackson( func (j *ProviderJackson) setProvider(ctx context.Context) { if j.ProviderGenericOIDC.p == nil { - config := gooidc.ProviderConfig{ + internalHost := strings.TrimSuffix(j.config.TokenURL, "/api/oauth/token") + config := oidc.ProviderConfig{ IssuerURL: j.config.IssuerURL, AuthURL: j.config.AuthURL, TokenURL: j.config.TokenURL, DeviceAuthURL: "", - UserInfoURL: j.config.IssuerURL + "/api/oauth/userinfo", - JWKSURL: j.config.IssuerURL + "/oauth/jwks", + UserInfoURL: internalHost + "/api/oauth/userinfo", + JWKSURL: internalHost + "/oauth/jwks", Algorithms: []string{"RS256"}, } j.ProviderGenericOIDC.p = config.NewProvider(j.withHTTPClientContext(ctx)) @@ -56,41 +55,3 @@ func (j *ProviderJackson) OAuth2(ctx context.Context) (*oauth2.Config, error) { return config, nil } - -func (j *ProviderJackson) Claims(ctx context.Context, exchange *oauth2.Token, query url.Values) (*Claims, error) { - j.setProvider(ctx) - return j.claimsFromIDToken(ctx, exchange) -} - -func (j *ProviderJackson) claimsFromIDToken(ctx context.Context, exchange *oauth2.Token) (*Claims, error) { - p, raw, err := j.idTokenAndProvider(ctx, exchange) - if err != nil { - return nil, err - } - - return j.verifyAndDecodeClaimsWithProvider(ctx, p, raw) -} - -func (j *ProviderJackson) verifyAndDecodeClaimsWithProvider(ctx context.Context, provider *gooidc.Provider, raw string) (*Claims, error) { - verifier := provider.VerifierContext(j.withHTTPClientContext(ctx), &gooidc.Config{ - ClientID: j.config.ClientID, - SkipIssuerCheck: true, - }) - token, err := verifier.Verify(ctx, raw) - if err != nil { - return nil, errors.WithStack(herodot.ErrBadRequest.WithReasonf("%s", err)) - } - - var claims Claims - if err := token.Claims(&claims); err != nil { - return nil, errors.WithStack(herodot.ErrBadRequest.WithReasonf("%s", err)) - } - - var rawClaims map[string]interface{} - if err := token.Claims(&rawClaims); err != nil { - return nil, errors.WithStack(herodot.ErrBadRequest.WithReasonf("%s", err)) - } - claims.RawClaims = rawClaims - - return &claims, nil -} diff --git a/selfservice/strategy/oidc/provider_jackson_test.go b/selfservice/strategy/oidc/provider_jackson_test.go new file mode 100644 index 000000000000..506bfc43afb5 --- /dev/null +++ b/selfservice/strategy/oidc/provider_jackson_test.go @@ -0,0 +1,33 @@ +package oidc_test + +import ( + "context" + "strings" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + + "github.com/ory/kratos/internal" + "github.com/ory/kratos/selfservice/strategy/oidc" +) + +func TestProviderJackson(t *testing.T) { + _, reg := internal.NewVeryFastRegistryWithoutDB(t) + + j := oidc.NewProviderJackson(&oidc.Configuration{ + Provider: "jackson", + IssuerURL: "https://www.jackson.com/oauth", + AuthURL: "https://www.jackson.com/oauth/auth", + TokenURL: "https://www.jackson.com/api/oauth/token", + Mapper: "file://./stub/hydra.schema.json", + Scope: []string{"email", "profile"}, + ID: "some-id", + }, reg) + assert.NotNil(t, j) + + c, err := j.(oidc.OAuth2Provider).OAuth2(context.Background()) + require.NoError(t, err) + + assert.True(t, strings.HasSuffix(c.RedirectURL, "/self-service/methods/saml/callback/some-id")) +} From 988c6284209eba3be935179be5fd5f3d4a9cea33 Mon Sep 17 00:00:00 2001 From: Henning Perl Date: Wed, 11 Dec 2024 08:43:50 +0100 Subject: [PATCH 07/13] chore: format --- selfservice/strategy/oidc/provider_jackson_test.go | 3 +++ 1 file changed, 3 insertions(+) diff --git a/selfservice/strategy/oidc/provider_jackson_test.go b/selfservice/strategy/oidc/provider_jackson_test.go index 506bfc43afb5..4c8bfa0bbf55 100644 --- a/selfservice/strategy/oidc/provider_jackson_test.go +++ b/selfservice/strategy/oidc/provider_jackson_test.go @@ -1,3 +1,6 @@ +// Copyright © 2024 Ory Corp +// SPDX-License-Identifier: Apache-2.0 + package oidc_test import ( From aec9ff9d4574b957d1af85a8aa4463f50fc99c82 Mon Sep 17 00:00:00 2001 From: Henning Perl Date: Wed, 11 Dec 2024 12:07:29 +0100 Subject: [PATCH 08/13] fix test coverage --- selfservice/flow/registration/handler_test.go | 2 +- selfservice/strategy/oidc/strategy_test.go | 13 +++++++++++++ 2 files changed, 14 insertions(+), 1 deletion(-) diff --git a/selfservice/flow/registration/handler_test.go b/selfservice/flow/registration/handler_test.go index a9e7b842718c..c2767bdd4192 100644 --- a/selfservice/flow/registration/handler_test.go +++ b/selfservice/flow/registration/handler_test.go @@ -426,7 +426,7 @@ func TestOIDCStrategyOrder(t *testing.T) { // reorder the strategies reg.WithSelfserviceStrategies(t, []any{ - oidc.NewStrategy(reg), + oidc.NewStrategy(reg, oidc.ForCredentialType(identity.CredentialsTypeOIDC)), password.NewStrategy(reg), }) diff --git a/selfservice/strategy/oidc/strategy_test.go b/selfservice/strategy/oidc/strategy_test.go index dde3e1bda607..50797e8b57e1 100644 --- a/selfservice/strategy/oidc/strategy_test.go +++ b/selfservice/strategy/oidc/strategy_test.go @@ -90,6 +90,7 @@ func TestStrategy(t *testing.T) { newOIDCProvider(t, ts, remotePublic, remoteAdmin, "valid"), newOIDCProvider(t, ts, remotePublic, remoteAdmin, "valid2"), newOIDCProvider(t, ts, remotePublic, remoteAdmin, "secondProvider"), + newOIDCProvider(t, ts, remotePublic, remoteAdmin, "saml:provider"), newOIDCProvider(t, ts, remotePublic, remoteAdmin, "claimsViaUserInfo", func(c *oidc.Configuration) { c.ClaimsSource = oidc.ClaimsSourceUserInfo }), @@ -338,6 +339,18 @@ func TestStrategy(t *testing.T) { } }) + t.Run("case=should redirect to UI because strategy not responsible", func(t *testing.T) { + for k, v := range []string{ + loginAction(newBrowserLoginFlow(t, returnTS.URL, time.Minute).ID), + registerAction(newBrowserRegistrationFlow(t, returnTS.URL, time.Minute).ID), + } { + t.Run(fmt.Sprintf("case=%d", k), func(t *testing.T) { + res, body := makeRequest(t, "saml:provider", v, url.Values{}) + assert.Contains(t, res.Request.URL.String(), uiTS.URL, "Redirect does not point to UI server. Status: %d, body: %s", res.StatusCode, body) + }) + } + }) + t.Run("case=should fail because flow does not exist", func(t *testing.T) { for k, v := range []string{loginAction(x.NewUUID()), registerAction(x.NewUUID())} { t.Run(fmt.Sprintf("case=%d", k), func(t *testing.T) { From 42e8aef127fffc3fc253e7a417643e41af40d028 Mon Sep 17 00:00:00 2001 From: Henning Perl Date: Wed, 11 Dec 2024 14:24:45 +0100 Subject: [PATCH 09/13] code review --- embedx/config.schema.json | 3 ++- selfservice/strategy/oidc/strategy.go | 21 +++++++++++++-------- selfservice/strategy/oidc/strategy_login.go | 8 -------- selfservice/strategy/oidc/strategy_test.go | 13 ------------- 4 files changed, 15 insertions(+), 30 deletions(-) diff --git a/embedx/config.schema.json b/embedx/config.schema.json index bf06568ebe3f..5fbe0e90bdcb 100644 --- a/embedx/config.schema.json +++ b/embedx/config.schema.json @@ -678,7 +678,8 @@ "examples": ["be2b30", "my-saml-provider"] }, "label": { - "title": "Optional string which will be used when generating labels for UI buttons.", + "title": "Label", + "description": "Optional string which will be used when generating labels for UI buttons.", "type": "string" }, "mapper_url": { diff --git a/selfservice/strategy/oidc/strategy.go b/selfservice/strategy/oidc/strategy.go index e58924af14c5..f1ed22341626 100644 --- a/selfservice/strategy/oidc/strategy.go +++ b/selfservice/strategy/oidc/strategy.go @@ -118,10 +118,11 @@ func isForced(req interface{}) bool { // Strategy implements selfservice.LoginStrategy, selfservice.RegistrationStrategy and selfservice.SettingsStrategy. // It supports login, registration and settings via OpenID Providers. type Strategy struct { - d Dependencies - validator *schema.Validator - dec *decoderx.HTTP - credType identity.CredentialsType + d Dependencies + validator *schema.Validator + dec *decoderx.HTTP + credType identity.CredentialsType + handleUnknownProviderError func(err error) error } type AuthCodeContainer struct { @@ -208,12 +209,16 @@ type NewStrategyOpt func(s *Strategy) func ForCredentialType(ct identity.CredentialsType) NewStrategyOpt { return func(s *Strategy) { s.credType = ct } } +func WithUnknownProviderHandler(handler func(error) error) NewStrategyOpt { + return func(s *Strategy) { s.handleUnknownProviderError = handler } +} func NewStrategy(d any, opts ...NewStrategyOpt) *Strategy { s := &Strategy{ - d: d.(Dependencies), - validator: schema.NewValidator(), - credType: identity.CredentialsTypeOIDC, + d: d.(Dependencies), + validator: schema.NewValidator(), + credType: identity.CredentialsTypeOIDC, + handleUnknownProviderError: func(err error) error { return err }, } for _, opt := range opts { opt(s) @@ -542,7 +547,7 @@ func (s *Strategy) provider(ctx context.Context, id string) (Provider, error) { if c, err := s.Config(ctx); err != nil { return nil, err } else if provider, err := c.Provider(id, s.d); err != nil { - return nil, err + return nil, s.handleUnknownProviderError(err) } else { return provider, nil } diff --git a/selfservice/strategy/oidc/strategy_login.go b/selfservice/strategy/oidc/strategy_login.go index 8bbea3a3b66b..4ec58a25590b 100644 --- a/selfservice/strategy/oidc/strategy_login.go +++ b/selfservice/strategy/oidc/strategy_login.go @@ -206,14 +206,6 @@ func (s *Strategy) Login(w http.ResponseWriter, r *http.Request, f *login.Flow, return nil, errors.WithStack(flow.ErrStrategyNotResponsible) } - // This is a hack for a lack of a `method` field in the form body. - if prefix, _, ok := strings.Cut(pid, ":"); ok { - if prefix != s.ID().String() { - span.SetAttributes(attribute.String("not_responsible_reason", "provider ID prefix does not match strategy")) - return nil, errors.WithStack(flow.ErrStrategyNotResponsible) - } - } - if !strings.EqualFold(strings.ToLower(p.Method), s.SettingsStrategyID()) && p.Method != "" { // the user is sending a method that is not oidc, but the payload includes a provider s.d.Audit(). diff --git a/selfservice/strategy/oidc/strategy_test.go b/selfservice/strategy/oidc/strategy_test.go index 50797e8b57e1..dde3e1bda607 100644 --- a/selfservice/strategy/oidc/strategy_test.go +++ b/selfservice/strategy/oidc/strategy_test.go @@ -90,7 +90,6 @@ func TestStrategy(t *testing.T) { newOIDCProvider(t, ts, remotePublic, remoteAdmin, "valid"), newOIDCProvider(t, ts, remotePublic, remoteAdmin, "valid2"), newOIDCProvider(t, ts, remotePublic, remoteAdmin, "secondProvider"), - newOIDCProvider(t, ts, remotePublic, remoteAdmin, "saml:provider"), newOIDCProvider(t, ts, remotePublic, remoteAdmin, "claimsViaUserInfo", func(c *oidc.Configuration) { c.ClaimsSource = oidc.ClaimsSourceUserInfo }), @@ -339,18 +338,6 @@ func TestStrategy(t *testing.T) { } }) - t.Run("case=should redirect to UI because strategy not responsible", func(t *testing.T) { - for k, v := range []string{ - loginAction(newBrowserLoginFlow(t, returnTS.URL, time.Minute).ID), - registerAction(newBrowserRegistrationFlow(t, returnTS.URL, time.Minute).ID), - } { - t.Run(fmt.Sprintf("case=%d", k), func(t *testing.T) { - res, body := makeRequest(t, "saml:provider", v, url.Values{}) - assert.Contains(t, res.Request.URL.String(), uiTS.URL, "Redirect does not point to UI server. Status: %d, body: %s", res.StatusCode, body) - }) - } - }) - t.Run("case=should fail because flow does not exist", func(t *testing.T) { for k, v := range []string{loginAction(x.NewUUID()), registerAction(x.NewUUID())} { t.Run(fmt.Sprintf("case=%d", k), func(t *testing.T) { From 60c8aa8b127e16438c1c8b330d731c245c0fc565 Mon Sep 17 00:00:00 2001 From: Henning Perl Date: Wed, 11 Dec 2024 15:32:59 +0100 Subject: [PATCH 10/13] fix --- selfservice/strategy/oidc/strategy.go | 4 ++++ selfservice/strategy/oidc/strategy_login.go | 6 +++--- selfservice/strategy/oidc/strategy_registration.go | 6 +++--- 3 files changed, 10 insertions(+), 6 deletions(-) diff --git a/selfservice/strategy/oidc/strategy.go b/selfservice/strategy/oidc/strategy.go index f1ed22341626..5e3dc09e46b3 100644 --- a/selfservice/strategy/oidc/strategy.go +++ b/selfservice/strategy/oidc/strategy.go @@ -206,9 +206,13 @@ func (s *Strategy) redirectToGET(w http.ResponseWriter, r *http.Request, _ httpr type NewStrategyOpt func(s *Strategy) +// ForCredentialType overrides the credentials type for this strategy. func ForCredentialType(ct identity.CredentialsType) NewStrategyOpt { return func(s *Strategy) { s.credType = ct } } + +// WithUnknownProviderHandler overrides the error returned when the provider +// cannot be found. func WithUnknownProviderHandler(handler func(error) error) NewStrategyOpt { return func(s *Strategy) { s.handleUnknownProviderError = handler } } diff --git a/selfservice/strategy/oidc/strategy_login.go b/selfservice/strategy/oidc/strategy_login.go index 4ec58a25590b..7bc4e91b1f6d 100644 --- a/selfservice/strategy/oidc/strategy_login.go +++ b/selfservice/strategy/oidc/strategy_login.go @@ -217,12 +217,12 @@ func (s *Strategy) Login(w http.ResponseWriter, r *http.Request, f *login.Flow, return nil, errors.WithStack(flow.ErrStrategyNotResponsible) } - if err := flow.MethodEnabledAndAllowed(ctx, f.GetFlowName(), s.SettingsStrategyID(), s.SettingsStrategyID(), s.d); err != nil { + provider, err := s.provider(ctx, pid) + if err != nil { return nil, s.handleError(ctx, w, r, f, pid, nil, err) } - provider, err := s.provider(ctx, pid) - if err != nil { + if err := flow.MethodEnabledAndAllowed(ctx, f.GetFlowName(), s.SettingsStrategyID(), s.SettingsStrategyID(), s.d); err != nil { return nil, s.handleError(ctx, w, r, f, pid, nil, err) } diff --git a/selfservice/strategy/oidc/strategy_registration.go b/selfservice/strategy/oidc/strategy_registration.go index b42fa476d33d..dbed0348182d 100644 --- a/selfservice/strategy/oidc/strategy_registration.go +++ b/selfservice/strategy/oidc/strategy_registration.go @@ -187,12 +187,12 @@ func (s *Strategy) Register(w http.ResponseWriter, r *http.Request, f *registrat return errors.WithStack(flow.ErrStrategyNotResponsible) } - if err := flow.MethodEnabledAndAllowed(ctx, f.GetFlowName(), s.SettingsStrategyID(), s.SettingsStrategyID(), s.d); err != nil { + provider, err := s.provider(ctx, pid) + if err != nil { return s.handleError(ctx, w, r, f, pid, nil, err) } - provider, err := s.provider(ctx, pid) - if err != nil { + if err := flow.MethodEnabledAndAllowed(ctx, f.GetFlowName(), s.SettingsStrategyID(), s.SettingsStrategyID(), s.d); err != nil { return s.handleError(ctx, w, r, f, pid, nil, err) } From be22f90801454a2b2665a2fed9f7dca937b100e6 Mon Sep 17 00:00:00 2001 From: Henning Perl Date: Thu, 12 Dec 2024 08:30:47 +0100 Subject: [PATCH 11/13] fix tests --- selfservice/strategy/oidc/strategy.go | 26 ++++++++++++------- selfservice/strategy/oidc/strategy_login.go | 8 +++--- .../strategy/oidc/strategy_registration.go | 8 +++--- 3 files changed, 25 insertions(+), 17 deletions(-) diff --git a/selfservice/strategy/oidc/strategy.go b/selfservice/strategy/oidc/strategy.go index 5e3dc09e46b3..b49883757f5d 100644 --- a/selfservice/strategy/oidc/strategy.go +++ b/selfservice/strategy/oidc/strategy.go @@ -118,11 +118,12 @@ func isForced(req interface{}) bool { // Strategy implements selfservice.LoginStrategy, selfservice.RegistrationStrategy and selfservice.SettingsStrategy. // It supports login, registration and settings via OpenID Providers. type Strategy struct { - d Dependencies - validator *schema.Validator - dec *decoderx.HTTP - credType identity.CredentialsType - handleUnknownProviderError func(err error) error + d Dependencies + validator *schema.Validator + dec *decoderx.HTTP + credType identity.CredentialsType + handleUnknownProviderError func(err error) error + handleMethodNotAllowedError func(err error) error } type AuthCodeContainer struct { @@ -217,12 +218,19 @@ func WithUnknownProviderHandler(handler func(error) error) NewStrategyOpt { return func(s *Strategy) { s.handleUnknownProviderError = handler } } +// WithHandleMethodNotAllowedError overrides the error returned when method is +// not allowed. +func WithHandleMethodNotAllowedError(handler func(error) error) NewStrategyOpt { + return func(s *Strategy) { s.handleMethodNotAllowedError = handler } +} + func NewStrategy(d any, opts ...NewStrategyOpt) *Strategy { s := &Strategy{ - d: d.(Dependencies), - validator: schema.NewValidator(), - credType: identity.CredentialsTypeOIDC, - handleUnknownProviderError: func(err error) error { return err }, + d: d.(Dependencies), + validator: schema.NewValidator(), + credType: identity.CredentialsTypeOIDC, + handleUnknownProviderError: func(err error) error { return err }, + handleMethodNotAllowedError: func(err error) error { return err }, } for _, opt := range opts { opt(s) diff --git a/selfservice/strategy/oidc/strategy_login.go b/selfservice/strategy/oidc/strategy_login.go index 7bc4e91b1f6d..392009ec2241 100644 --- a/selfservice/strategy/oidc/strategy_login.go +++ b/selfservice/strategy/oidc/strategy_login.go @@ -217,12 +217,12 @@ func (s *Strategy) Login(w http.ResponseWriter, r *http.Request, f *login.Flow, return nil, errors.WithStack(flow.ErrStrategyNotResponsible) } - provider, err := s.provider(ctx, pid) - if err != nil { - return nil, s.handleError(ctx, w, r, f, pid, nil, err) + if err := flow.MethodEnabledAndAllowed(ctx, f.GetFlowName(), s.SettingsStrategyID(), s.SettingsStrategyID(), s.d); err != nil { + return nil, s.handleError(ctx, w, r, f, pid, nil, s.handleMethodNotAllowedError(err)) } - if err := flow.MethodEnabledAndAllowed(ctx, f.GetFlowName(), s.SettingsStrategyID(), s.SettingsStrategyID(), s.d); err != nil { + provider, err := s.provider(ctx, pid) + if err != nil { return nil, s.handleError(ctx, w, r, f, pid, nil, err) } diff --git a/selfservice/strategy/oidc/strategy_registration.go b/selfservice/strategy/oidc/strategy_registration.go index dbed0348182d..90fb5f7996b5 100644 --- a/selfservice/strategy/oidc/strategy_registration.go +++ b/selfservice/strategy/oidc/strategy_registration.go @@ -187,12 +187,12 @@ func (s *Strategy) Register(w http.ResponseWriter, r *http.Request, f *registrat return errors.WithStack(flow.ErrStrategyNotResponsible) } - provider, err := s.provider(ctx, pid) - if err != nil { - return s.handleError(ctx, w, r, f, pid, nil, err) + if err := flow.MethodEnabledAndAllowed(ctx, f.GetFlowName(), s.SettingsStrategyID(), s.SettingsStrategyID(), s.d); err != nil { + return s.handleError(ctx, w, r, f, pid, nil, s.handleMethodNotAllowedError(err)) } - if err := flow.MethodEnabledAndAllowed(ctx, f.GetFlowName(), s.SettingsStrategyID(), s.SettingsStrategyID(), s.d); err != nil { + provider, err := s.provider(ctx, pid) + if err != nil { return s.handleError(ctx, w, r, f, pid, nil, err) } From f5dc78749e7eb8ad6fd64d94e866944bebbc2cc5 Mon Sep 17 00:00:00 2001 From: Henning Perl Date: Thu, 12 Dec 2024 12:23:25 +0100 Subject: [PATCH 12/13] bump deps --- go.mod | 10 +++++----- go.sum | 10 ++++++++++ 2 files changed, 15 insertions(+), 5 deletions(-) diff --git a/go.mod b/go.mod index acdf909951c1..fb7135cdfd55 100644 --- a/go.mod +++ b/go.mod @@ -97,12 +97,12 @@ require ( go.opentelemetry.io/otel v1.32.0 go.opentelemetry.io/otel/sdk v1.32.0 go.opentelemetry.io/otel/trace v1.32.0 - golang.org/x/crypto v0.29.0 + golang.org/x/crypto v0.31.0 golang.org/x/exp v0.0.0-20240719175910-8a7402abbf56 golang.org/x/net v0.31.0 golang.org/x/oauth2 v0.24.0 - golang.org/x/sync v0.9.0 - golang.org/x/text v0.20.0 + golang.org/x/sync v0.10.0 + golang.org/x/text v0.21.0 google.golang.org/grpc v1.67.1 ) @@ -118,7 +118,7 @@ require ( github.com/dgraph-io/ristretto/v2 v2.0.0 // indirect github.com/jackc/pgx/v5 v5.6.0 // indirect github.com/rjeczalik/notify v0.9.3 // indirect - golang.org/x/term v0.26.0 // indirect + golang.org/x/term v0.27.0 // indirect gopkg.in/alecthomas/kingpin.v2 v2.2.6 // indirect mvdan.cc/sh/v3 v3.6.0 // indirect ) @@ -313,7 +313,7 @@ require ( go.opentelemetry.io/proto/otlp v1.3.1 // indirect go.uber.org/multierr v1.11.0 // indirect golang.org/x/mod v0.19.0 // indirect - golang.org/x/sys v0.27.0 // indirect + golang.org/x/sys v0.28.0 // indirect golang.org/x/tools v0.23.0 // indirect golang.org/x/xerrors v0.0.0-20231012003039-104605ab7028 // indirect google.golang.org/genproto/googleapis/api v0.0.0-20241104194629-dd2ea8efbc28 // indirect diff --git a/go.sum b/go.sum index d48ddcf554f0..615ae0a2d3f4 100644 --- a/go.sum +++ b/go.sum @@ -868,6 +868,8 @@ golang.org/x/crypto v0.19.0/go.mod h1:Iy9bg/ha4yyC70EfRS8jz+B6ybOBKMaSxLj6P6oBDf golang.org/x/crypto v0.21.0/go.mod h1:0BP7YvVV9gBbVKyeTG0Gyn+gZm94bibOW5BjDEYAOMs= golang.org/x/crypto v0.29.0 h1:L5SG1JTTXupVV3n6sUqMTeWbjAyfPwoda2DLX8J8FrQ= golang.org/x/crypto v0.29.0/go.mod h1:+F4F4N5hv6v38hfeYwTdx20oUvLLc+QfrE9Ax9HtgRg= +golang.org/x/crypto v0.31.0 h1:ihbySMvVjLAeSH1IbfcRTkD/iNscyz8rGzjF/E5hV6U= +golang.org/x/crypto v0.31.0/go.mod h1:kDsLvtWBEx7MV9tJOj9bnXsPbxwJQ6csT/x4KIN4Ssk= golang.org/x/exp v0.0.0-20190121172915-509febef88a4/go.mod h1:CJ0aWSM057203Lf6IL+f9T1iT9GByDxfZKAQTCR3kQA= golang.org/x/exp v0.0.0-20190306152737-a1d7652674e8/go.mod h1:CJ0aWSM057203Lf6IL+f9T1iT9GByDxfZKAQTCR3kQA= golang.org/x/exp v0.0.0-20190510132918-efd6b22b2522/go.mod h1:ZjyILWgesfNpC6sMxTJOJm9Kp84zZh5NQWvqDGG3Qr8= @@ -983,6 +985,8 @@ golang.org/x/sync v0.1.0/go.mod h1:RxMgew5VJxzue5/jJTE5uejpjVlOe/izrB70Jof72aM= golang.org/x/sync v0.3.0/go.mod h1:FU7BRWz2tNW+3quACPkgCx/L+uEAv1htQ0V83Z9Rj+Y= golang.org/x/sync v0.9.0 h1:fEo0HyrW1GIgZdpbhCRO0PkJajUS5H9IFUztCgEo2jQ= golang.org/x/sync v0.9.0/go.mod h1:Czt+wKu1gCyEFDUtn0jG5QVvpJ6rzVqr5aXyt9drQfk= +golang.org/x/sync v0.10.0 h1:3NQrjDixjgGwUOCaF8w2+VYHv0Ve/vGYSbdkTa98gmQ= +golang.org/x/sync v0.10.0/go.mod h1:Czt+wKu1gCyEFDUtn0jG5QVvpJ6rzVqr5aXyt9drQfk= golang.org/x/sys v0.0.0-20180830151530-49385e6e1522/go.mod h1:STP8DvDyc/dI5b8T5hshtkjS+E42TnysNCUPdjciGhY= golang.org/x/sys v0.0.0-20180905080454-ebe1bf3edb33/go.mod h1:STP8DvDyc/dI5b8T5hshtkjS+E42TnysNCUPdjciGhY= golang.org/x/sys v0.0.0-20180909124046-d0be0721c37e/go.mod h1:STP8DvDyc/dI5b8T5hshtkjS+E42TnysNCUPdjciGhY= @@ -1049,6 +1053,8 @@ golang.org/x/sys v0.17.0/go.mod h1:/VUhepiaJMQUp4+oa/7Zr1D23ma6VTLIYjOOTFZPUcA= golang.org/x/sys v0.18.0/go.mod h1:/VUhepiaJMQUp4+oa/7Zr1D23ma6VTLIYjOOTFZPUcA= golang.org/x/sys v0.27.0 h1:wBqf8DvsY9Y/2P8gAfPDEYNuS30J4lPHJxXSb/nJZ+s= golang.org/x/sys v0.27.0/go.mod h1:/VUhepiaJMQUp4+oa/7Zr1D23ma6VTLIYjOOTFZPUcA= +golang.org/x/sys v0.28.0 h1:Fksou7UEQUWlKvIdsqzJmUmCX3cZuD2+P3XyyzwMhlA= +golang.org/x/sys v0.28.0/go.mod h1:/VUhepiaJMQUp4+oa/7Zr1D23ma6VTLIYjOOTFZPUcA= golang.org/x/term v0.0.0-20201117132131-f5c789dd3221/go.mod h1:Nr5EML6q2oocZ2LXRh80K7BxOlk5/8JxuGnuhpl+muw= golang.org/x/term v0.0.0-20201126162022-7de9c90e9dd1/go.mod h1:bj7SfCRtBDWHUb9snDiAeCFNEtKQo2Wmx5Cou7ajbmo= golang.org/x/term v0.0.0-20210220032956-6a3ed077a48d/go.mod h1:bj7SfCRtBDWHUb9snDiAeCFNEtKQo2Wmx5Cou7ajbmo= @@ -1062,6 +1068,8 @@ golang.org/x/term v0.17.0/go.mod h1:lLRBjIVuehSbZlaOtGMbcMncT+aqLLLmKrsjNrUguwk= golang.org/x/term v0.18.0/go.mod h1:ILwASektA3OnRv7amZ1xhE/KTR+u50pbXfZ03+6Nx58= golang.org/x/term v0.26.0 h1:WEQa6V3Gja/BhNxg540hBip/kkaYtRg3cxg4oXSw4AU= golang.org/x/term v0.26.0/go.mod h1:Si5m1o57C5nBNQo5z1iq+XDijt21BDBDp2bK0QI8e3E= +golang.org/x/term v0.27.0 h1:WP60Sv1nlK1T6SupCHbXzSaN0b9wUmsPoRS9b61A23Q= +golang.org/x/term v0.27.0/go.mod h1:iMsnZpn0cago0GOrHO2+Y7u7JPn5AylBrcoWkElMTSM= golang.org/x/text v0.0.0-20170915032832-14c0d48ead0c/go.mod h1:NqM8EUOU14njkJ3fqMW+pc6Ldnwhi/IjpwHt7yyuwOQ= golang.org/x/text v0.3.0/go.mod h1:NqM8EUOU14njkJ3fqMW+pc6Ldnwhi/IjpwHt7yyuwOQ= golang.org/x/text v0.3.1-0.20180807135948-17ff2d5776d2/go.mod h1:NqM8EUOU14njkJ3fqMW+pc6Ldnwhi/IjpwHt7yyuwOQ= @@ -1076,6 +1084,8 @@ golang.org/x/text v0.9.0/go.mod h1:e1OnstbJyHTd6l/uOt8jFFHp6TRDWZR/bV3emEE/zU8= golang.org/x/text v0.14.0/go.mod h1:18ZOQIKpY8NJVqYksKHtTdi31H5itFRjB5/qKTNYzSU= golang.org/x/text v0.20.0 h1:gK/Kv2otX8gz+wn7Rmb3vT96ZwuoxnQlY+HlJVj7Qug= golang.org/x/text v0.20.0/go.mod h1:D4IsuqiFMhST5bX19pQ9ikHC2GsaKyk/oF+pn3ducp4= +golang.org/x/text v0.21.0 h1:zyQAAkrwaneQ066sspRyJaG9VNi/YJ1NfzcGB3hZ/qo= +golang.org/x/text v0.21.0/go.mod h1:4IBbMaMmOPCJ8SecivzSH54+73PCFmPWxNTLm+vZkEQ= golang.org/x/time v0.0.0-20181108054448-85acf8d2951c/go.mod h1:tRJNPiyCQ0inRvYxbN9jk5I+vvW/OXSQhTDSoE431IQ= golang.org/x/time v0.0.0-20190308202827-9d24e82272b4/go.mod h1:tRJNPiyCQ0inRvYxbN9jk5I+vvW/OXSQhTDSoE431IQ= golang.org/x/time v0.0.0-20191024005414-555d28b269f0/go.mod h1:tRJNPiyCQ0inRvYxbN9jk5I+vvW/OXSQhTDSoE431IQ= From 632044595c1cb514e3651f0e32e471c504977f8c Mon Sep 17 00:00:00 2001 From: Henning Perl Date: Wed, 18 Dec 2024 11:31:59 +0100 Subject: [PATCH 13/13] code review --- embedx/config.schema.json | 86 ------------------- .../strategy/oidc/strategy_registration.go | 7 -- 2 files changed, 93 deletions(-) diff --git a/embedx/config.schema.json b/embedx/config.schema.json index 5fbe0e90bdcb..5fcf826f4c2a 100644 --- a/embedx/config.schema.json +++ b/embedx/config.schema.json @@ -670,50 +670,6 @@ } ] }, - "selfServiceSAMLProvider": { - "type": "object", - "properties": { - "id": { - "type": "string", - "examples": ["be2b30", "my-saml-provider"] - }, - "label": { - "title": "Label", - "description": "Optional string which will be used when generating labels for UI buttons.", - "type": "string" - }, - "mapper_url": { - "title": "Jsonnet Mapper URL", - "description": "The URL where the jsonnet source is located for mapping the provider's data to Ory Kratos data.", - "type": "string", - "format": "uri", - "examples": [ - "file://path/to/oidc.jsonnet", - "https://foo.bar.com/path/to/oidc.jsonnet", - "base64://bG9jYWwgc3ViamVjdCA9I..." - ] - }, - "raw_idp_metadata_xml": { - "title": "Raw IDP Metadata XML", - "description": "The raw IDP metadata XML for the SAML provider.", - "type": "string", - "format": "uri", - "examples": [ - "file://path/to/metadata.xml", - "https://foo.bar.com/path/to/metadata.xml", - "base64://bG9jYWwgc3ViamVjdCA9I..." - ] - }, - "organization_id": { - "title": "Organization ID", - "description": "The ID of the organization that this provider belongs to. Only effective in the Ory Network.", - "type": "string", - "examples": ["12345678-1234-1234-1234-123456789012"] - } - }, - "additionalProperties": true, - "required": ["id", "raw_idp_metadata_xml", "mapper_url"] - }, "selfServiceHooks": { "type": "array", "items": { @@ -911,9 +867,6 @@ "oidc": { "$ref": "#/definitions/selfServiceAfterSettingsAuthMethod" }, - "saml": { - "$ref": "#/definitions/selfServiceAfterSettingsAuthMethod" - }, "webauthn": { "$ref": "#/definitions/selfServiceAfterSettingsAuthMethod" }, @@ -959,9 +912,6 @@ "oidc": { "$ref": "#/definitions/selfServiceAfterOIDCLoginMethod" }, - "saml": { - "$ref": "#/definitions/selfServiceAfterOIDCLoginMethod" - }, "code": { "$ref": "#/definitions/selfServiceAfterDefaultLoginMethod" }, @@ -1055,9 +1005,6 @@ "oidc": { "$ref": "#/definitions/selfServiceAfterRegistrationMethod" }, - "saml": { - "$ref": "#/definitions/selfServiceAfterRegistrationMethod" - }, "code": { "$ref": "#/definitions/selfServiceAfterRegistrationMethod" }, @@ -2015,39 +1962,6 @@ } } } - }, - "saml": { - "type": "object", - "title": "Specify SAML configuration", - "additionalProperties": false, - "properties": { - "enabled": { - "type": "boolean", - "title": "Enables the SAML method", - "default": false - }, - "config": { - "type": "object", - "additionalProperties": false, - "properties": { - "base_redirect_uri": { - "type": "string", - "title": "Base URL for SAML Redirect URIs", - "description": "Can be used to modify the base URL for SAML Redirect URLs. If unset, the Public Base URL will be used.", - "format": "uri", - "examples": ["https://auth.myexample.org/"] - }, - "providers": { - "title": "SAML Providers", - "description": "A list and configuration of SAML providers Ory Kratos should integrate with.", - "type": "array", - "items": { - "$ref": "#/definitions/selfServiceSAMLProvider" - } - } - } - } - } } } } diff --git a/selfservice/strategy/oidc/strategy_registration.go b/selfservice/strategy/oidc/strategy_registration.go index 90fb5f7996b5..5ed061119e7b 100644 --- a/selfservice/strategy/oidc/strategy_registration.go +++ b/selfservice/strategy/oidc/strategy_registration.go @@ -164,13 +164,6 @@ func (s *Strategy) Register(w http.ResponseWriter, r *http.Request, f *registrat span.SetAttributes(attribute.String("not_responsible_reason", "provider ID missing")) return errors.WithStack(flow.ErrStrategyNotResponsible) } - // This is a hack for a lack of a `method` field in the form body. - if prefix, _, ok := strings.Cut(pid, ":"); ok { - if prefix != s.ID().String() { - span.SetAttributes(attribute.String("not_responsible_reason", "provider ID prefix does not match strategy")) - return errors.WithStack(flow.ErrStrategyNotResponsible) - } - } f.TransientPayload = p.TransientPayload f.IDToken = p.IDToken