feat: client-side PKCE support during OIDC and generic redirect_uri #4059

alnr · 2024-08-26T15:10:33Z

This change introduces a new configuration for OIDC providers: pkce with values auto (default), never, force.

When auto is specified or the field is omitted, Kratos will perform autodiscovery and perform PKCE when the server advertises support for it. This requires the issuer_url to be set for the provider.

never completely disables PKCE support. This is only theoretically useful: when a provider advertises PKCE support but doesn't actually implement it.

force always sends a PKCE challenge in the initial redirect URL, regardless of what the provider advertises. This setting is useful when the provider offers PKCE but doesn't advertise it in his ./well-known/openid-configuration.

Important: When setting pkce: force, you must whitelist a different return URL for your OAuth2 client in the provider's configuration. Instead of <base-url>/self-service/methods/oidc/callback/<provider>, you must use <base-url>/self-service/methods/oidc/callback (note missing last path segment). This is to enable the use of the same OAuth client ID+secret when configuring several Kratos OIDC providers, without having to whitelist individual redirect_uris for each Kratos provider config.

Supersedes #4033
Closes #4009
Ref https://github.com/ory-corp/cloud/issues/6613

codecov · 2024-08-27T10:37:23Z

Codecov Report

Attention: Patch coverage is 64.11483% with 75 lines in your changes missing coverage. Please review.

Project coverage is 78.35%. Comparing base (7945104) to head (fb86964).

Files	Patch %	Lines
selfservice/strategy/oidc/strategy.go	52.72%	21 Missing and 5 partials ⚠️
selfservice/strategy/oidc/state.go	69.44%	5 Missing and 6 partials ⚠️
selfservice/strategy/oidc/strategy_login.go	46.15%	3 Missing and 4 partials ⚠️
selfservice/strategy/oidc/strategy_registration.go	53.33%	3 Missing and 4 partials ⚠️
selfservice/strategy/oidc/strategy_settings.go	53.33%	3 Missing and 4 partials ⚠️
selfservice/strategy/oidc/pkce.go	92.30%	2 Missing and 2 partials ⚠️
selfservice/strategy/oidc/provider_dingtalk.go	0.00%	2 Missing ⚠️
selfservice/strategy/oidc/provider_apple.go	0.00%	1 Missing ⚠️
selfservice/strategy/oidc/provider_discord.go	0.00%	1 Missing ⚠️
selfservice/strategy/oidc/provider_github.go	0.00%	1 Missing ⚠️
... and 8 more

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #4059      +/-   ##
==========================================
- Coverage   78.39%   78.35%   -0.04%     
==========================================
  Files         370      372       +2     
  Lines       26127    26241     +114     
==========================================
+ Hits        20481    20562      +81     
- Misses       4088     4102      +14     
- Partials     1558     1577      +19

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

aeneasr · 2024-08-28T08:04:51Z

Important: When setting pkce: force, you must whitelist a different return URL for your OAuth2 client in the provider's configuration. Instead of /self-service/methods/oidc/callback/, you must use /self-service/methods/oidc/callback (note missing last path segment). This is to enable the use of the same OAuth client ID+secret when configuring several Kratos OIDC providers, without having to whitelist individual redirect_uris for each Kratos provider config.

Why is that?

aeneasr

Good stuff! I'm primarily concerned:

That the internal context gets correctly passed around and we don't lose track of the PKCE challenge or provider ID
That the organization flag is still working correctly
That we're returning the wrong flow in one instance (which could lead to some errors)
That the generic callback keeps track of the currently active provider ID across multiple login / sign up attempts

Please also add e2e tests verifying that PKCE works end-to-end, and that the generic callback also works correctly.

selfservice/strategy/oidc/pkce_test.go

aeneasr · 2024-08-28T08:11:01Z

selfservice/strategy/oidc/provider_config.go

+	// IMPORTANT: If you set this to `force`, you must whitelist a different return URL for your OAuth2 client in the provider's configuration.
+	// Instead of <base-url>/self-service/methods/oidc/callback/<provider>, you must use <base-url>/self-service/methods/oidc/callback
+	// (Note the missing <provider> path segment and no trailing slash).
+	PKCE string `json:"pkce"`


You could make this an enum, in which case this would also end up correctly in swagger API definitions and so on.

aeneasr · 2024-08-28T08:19:03Z

selfservice/strategy/oidc/state.go

+	f.EnsureInternalContext()
+	bytes, err := sjson.SetBytes(
+		f.GetInternalContext(),
+		"oidc_provider",


The internal context is prone to problems when retrying flows. I believe this is/was a problem for several MFA methods. Relying on the internal context implies that we probably need to test if multiple oidc flows (same provider / different provider) work when this feature is used. It's quite possible that we already have these tests.

selfservice/strategy/oidc/strategy.go

aeneasr · 2024-08-28T08:30:21Z

selfservice/strategy/oidc/strategy_login.go

@@ -173,8 +173,11 @@ func (s *Strategy) processLogin(w http.ResponseWriter, r *http.Request, loginFlo
 	}

 	sess := session.NewInactiveSession()
-	sess.CompletedLoginForWithProvider(s.ID(), identity.AuthenticatorAssuranceLevel1, provider.Config().ID,
-		httprouter.ParamsFromContext(r.Context()).ByName("organization"))
+	orgID := ""


This change looks incorrect? Previously we fetched this from the http route and now we use it from the loginFlow (but do we know if it is correctly set there?)

I ended up simplifying the original code and take the OrgID from the provider config directly, instead of looking at the current URL.

aeneasr · 2024-08-28T08:34:59Z

selfservice/strategy/oidc/strategy_registration.go

@@ -245,7 +259,7 @@ func (s *Strategy) Register(w http.ResponseWriter, r *http.Request, f *registrat
 	return errors.WithStack(flow.ErrCompletedByStrategy)
 }

-func (s *Strategy) registrationToLogin(w http.ResponseWriter, r *http.Request, rf *registration.Flow, providerID string) (*login.Flow, error) {
+func (s *Strategy) registrationToLogin(w http.ResponseWriter, r *http.Request, rf *registration.Flow) (*login.Flow, error) {


Does this method keep working with PKCE or do we somehow lose track of PKCE? To test it, you could create an e2e test:

For an PKCE oauth2 provider with pkce force

Sign in with account A which does not exit

Kratos asks to sign up

Sign up with account A

Be logged in

Sign out

Perform sign up with account A

Expect log in

Log in with account A

Be signed in

If you want to be extra sure add validation errors in between (e.g. incomplete profile data from oidc)

All this should continue working as before, because it's orthogonal to PKCE from what I can see in the code.

Whenever we move between flows, the OAuth2 token exchange has already taken place, so all PKCE-related stuff is already done.

Essentially, we store the verifier directly before redirecting the end user, and verify it right after he comes back (during token exchange). There are no cross-flow (registrationToLogin or the other way around) shenanigans to get in the way, I don't think.

aeneasr · 2024-08-28T08:36:08Z

selfservice/strategy/oidc/strategy_registration.go

-	if orgID := httprouter.ParamsFromContext(r.Context()).ByName("organization"); orgID != "" {
-		i.OrganizationID = uuid.NullUUID{UUID: x.ParseUUID(orgID), Valid: true}
-	}
+	i.OrganizationID = a.OrganizationID


How are you certain that this is not a regression?

I did end up changing the code to get the correct OrgID directly from the provider configuration.

aeneasr · 2024-08-28T08:37:33Z

selfservice/strategy/oidc/strategy_settings.go

+		return s.handleError(w, r, ctxUpdate.Flow, p.Link, nil, errors.WithStack(herodot.ErrInternalServerError.WithReason("Could not update flow").WithDebug(err.Error())))
+	}
+
+	codeURL, err := getAuthRedirectURL(ctx, provider, ctxUpdate.Flow, state, up)


ctxUpdate.Flow does not represent the most up-to-date version of the flow. Use the return value of _, err = s.validateFlow(ctx, r, ctxUpdate.Flow.ID) instead (like it was before)

aeneasr · 2024-08-28T08:39:07Z

selfservice/strategy/oidc/strategy_test.go

@@ -470,6 +479,67 @@ func TestStrategy(t *testing.T) {
 		return id
 	}

+	t.Run("case=force PKCE", func(t *testing.T) {


Here you can conveniently add tests that check if more complex flows also pass:

Sign in with wrong provider, then try again with another

Does PKCE actually get validated? Expect an error case

Does the automatic detection of whether to sign in or up with PKCE work as well?

We already have a bunch of these tests in the context of login hints/account linking. I've added a test which checks a mismatch between providers in URL and state is correctly detected and the flow aborted.

Added tests.

Test is already there.

aeneasr · 2024-08-30T10:33:21Z

Since a few other changes landed, there are a few conflicts now. Probably mostly beacuse a lot of functions got contexts now.

selfservice/strategy/oidc/strategy.go

selfservice/strategy/oidc/strategy_helper_test.go

x/swagger/swagger_types_global.go

alnr · 2024-08-30T10:36:51Z

Yep, I'm rebasing.

OskarsPakers · 2024-09-02T06:00:36Z

Important: When setting pkce: force, you must whitelist a different return URL for your OAuth2 client in the provider's configuration. Instead of <base-url>/self-service/methods/oidc/callback/<provider>, you must use <base-url>/self-service/methods/oidc/callback (note missing last path segment). This is to enable the use of the same OAuth client ID+secret when configuring several Kratos OIDC providers, without having to whitelist individual redirect_uris for each Kratos provider config.

Would the regular callback with provider id in url work too?
If not, This way it would be mean that toggling the setting requires registering new redirect url. That would not be great.

alnr · 2024-09-02T08:23:50Z

Important: When setting pkce: force, you must whitelist a different return URL for your OAuth2 client in the provider's configuration. Instead of <base-url>/self-service/methods/oidc/callback/<provider>, you must use <base-url>/self-service/methods/oidc/callback (note missing last path segment). This is to enable the use of the same OAuth client ID+secret when configuring several Kratos OIDC providers, without having to whitelist individual redirect_uris for each Kratos provider config.

Would the regular callback with provider id in url work too? If not, This way it would be mean that toggling the setting requires registering new redirect url. That would not be great.

Having the generic redirect URI forces PKCE for security reasons. If you want to keep your redirect URI as-is, you'll still benefic from automatic PKCE if the provider advertises it.

# Conflicts: # selfservice/strategy/oidc/strategy.go # selfservice/strategy/oidc/strategy_login.go # selfservice/strategy/oidc/strategy_registration.go # selfservice/strategy/oidc/strategy_settings.go # Conflicts: # selfservice/strategy/oidc/strategy.go

…tion

alnr · 2024-09-10T10:40:15Z

Superseeded by #4078

alnr self-assigned this Aug 26, 2024

alnr requested review from aeneasr, zepatrik and hperl as code owners August 26, 2024 15:10

alnr force-pushed the alnr/oidc-callback-url branch from a784ec2 to 872581e Compare August 27, 2024 09:50

alnr force-pushed the alnr/oidc-callback-url branch from ceff2da to fb86964 Compare August 27, 2024 11:31

aeneasr requested changes Aug 28, 2024

View reviewed changes

aeneasr reviewed Aug 30, 2024

View reviewed changes

selfservice/strategy/oidc/strategy.go Outdated Show resolved Hide resolved

selfservice/strategy/oidc/strategy_helper_test.go Show resolved Hide resolved

x/swagger/swagger_types_global.go Show resolved Hide resolved

alnr added 2 commits September 2, 2024 10:40

fix: pass URL params explicitly

5195727

alnr force-pushed the alnr/oidc-callback-url branch from fd56e48 to 5195727 Compare September 2, 2024 14:07

alnr added 5 commits September 2, 2024 16:57

chore: add test

6e07440

chore: get final organization ID from provider config during registra…

c0bf316

…tion

fix: settings flow

8ab4d4c

fix: take OrgID from provider config during login

3ddc46f

fix: assertion

e56f142

alnr force-pushed the alnr/oidc-callback-url branch from 6b39809 to e56f142 Compare September 2, 2024 16:33

alnr mentioned this pull request Sep 10, 2024

feat: client-side PKCE take 3 #4078

Merged

alnr closed this Sep 10, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: client-side PKCE support during OIDC and generic redirect_uri #4059

feat: client-side PKCE support during OIDC and generic redirect_uri #4059

alnr commented Aug 26, 2024

codecov bot commented Aug 27, 2024 •

edited

Loading

aeneasr commented Aug 28, 2024

aeneasr left a comment

aeneasr Aug 28, 2024

aeneasr Aug 28, 2024

aeneasr Aug 28, 2024

alnr Sep 2, 2024

aeneasr Aug 28, 2024

alnr Sep 2, 2024

aeneasr Aug 28, 2024

alnr Sep 2, 2024 •

edited

Loading

aeneasr Aug 28, 2024

alnr Sep 2, 2024

aeneasr Aug 28, 2024

alnr Sep 2, 2024

aeneasr commented Aug 30, 2024

alnr commented Aug 30, 2024

OskarsPakers commented Sep 2, 2024

alnr commented Sep 2, 2024

alnr commented Sep 10, 2024

feat: client-side PKCE support during OIDC and generic redirect_uri #4059

feat: client-side PKCE support during OIDC and generic redirect_uri #4059

Conversation

alnr commented Aug 26, 2024

codecov bot commented Aug 27, 2024 • edited Loading

Codecov Report

aeneasr commented Aug 28, 2024

aeneasr left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alnr Sep 2, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aeneasr commented Aug 30, 2024

alnr commented Aug 30, 2024

OskarsPakers commented Sep 2, 2024

alnr commented Sep 2, 2024

alnr commented Sep 10, 2024

codecov bot commented Aug 27, 2024 •

edited

Loading

alnr Sep 2, 2024 •

edited

Loading