feat: client-side PKCE take 3 #4078

alnr · 2024-09-04T14:54:42Z

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
Supersedes #4059
Closes #4009
Ref https://github.com/ory-corp/cloud/issues/6613

selfservice/strategy/oidc/strategy.go

codecov · 2024-09-09T19:33:32Z

Codecov Report

Attention: Patch coverage is 64.23841% with 108 lines in your changes missing coverage. Please review.

Project coverage is 78.46%. Comparing base (5592029) to head (91b5cd3).
Report is 1 commits behind head on master.

Files with missing lines	Patch %	Lines
gen/oidc/v1/state.pb.go	57.53%	29 Missing and 2 partials ⚠️
selfservice/strategy/oidc/strategy.go	56.86%	20 Missing and 2 partials ⚠️
selfservice/strategy/oidc/strategy_registration.go	45.00%	19 Missing and 3 partials ⚠️
selfservice/strategy/oidc/strategy_login.go	44.00%	13 Missing and 1 partial ⚠️
selfservice/strategy/oidc/state.go	78.33%	6 Missing and 7 partials ⚠️
cipher/chacha20.go	0.00%	1 Missing and 1 partial ⚠️
selfservice/strategy/oidc/pkce.go	94.28%	1 Missing and 1 partial ⚠️
selfservice/strategy/oidc/strategy_settings.go	60.00%	1 Missing and 1 partial ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #4078      +/-   ##
==========================================
- Coverage   78.92%   78.46%   -0.46%     
==========================================
  Files         373      376       +3     
  Lines       27123    26728     -395     
==========================================
- Hits        21406    20973     -433     
- Misses       4129     4150      +21     
- Partials     1588     1605      +17

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

hperl

Very nice and clean! The only thing that could suprise users is that pkce set to force will also change the callback URL, but at least it is well documented in the config schema.

Maybe we could set the issuer URL for more of the pre-configured providers, so that this will be an automatic security improvement for as many users as possible?

selfservice/strategy/oidc/pkce.go

selfservice/strategy/oidc/state.go

…tion and login

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.

aeneasr

Good stuff! I'm mostly worried about regressions for existing OIDC providers. In particular I'm worried about redirects we do in Ory Network.

The easiest way to test this is to set up kratos before this patch with some OIDC connections, and then check if those work like they did before after those changes.

aeneasr · 2024-09-12T09:52:10Z

selfservice/strategy/oidc/state.go

+func encryptState(ctx context.Context, c cipher.Cipher, state *oidcv1.State) (ciphertext string, err error) {
+	m, err := proto.Marshal(state)
+	if err != nil {
+		return "", herodot.ErrInternalServerError.WithReasonf("Unable to marshal state: %s", err)


Add WithStack for trace information across all error returns, it makes debugging significantly easier :)

aeneasr · 2024-09-12T09:57:22Z

selfservice/strategy/oidc/state.go

+
+var newStyleState = false
+
+func TestHookEnableNewStyleState(t *testing.T) {


Please don't reference the testing library in production code, but instead move this to _test.go files. If you include it, the testing library adds flags and has other side effects on the production binary. If you need to include this function in another else (like we do with persister tests), move it to a separate package. If the package is not referenced by production code, it is not included.

For example, running go run . version -test.short no longer fails with an invalid argument (as opposed to go run . version -asdf), because the flag is now defined. This can have unintended consequences / side effects.

This code will be removed as soon as we have completed the two-step rollout on Ory Network.

aeneasr · 2024-09-12T10:02:23Z

cipher/chacha20.go

@@ -43,6 +44,11 @@ func (c *XChaCha20Poly1305) Encrypt(ctx context.Context, message []byte) (string
 		return "", herodot.ErrInternalServerError.WithWrap(err).WithReason("Unable to generate key")
 	}

+	// Make sure the size calculation does not overflow.
+	if len(message) > math.MaxInt-aead.NonceSize()-aead.Overhead() {


This method is taken from cryptopasta, if I recall correctly. Could you point me to the documentation / issue this addresses? This method is being used in several places, and I am worried this will introduce regressions. Granted, MaxInt is pretty large.

The AES-GCM code comes from cryptopasta. The XChaCha code was hand-written from the start.

In an earlier revision of this PR, I touched this code but reverted it later to the original version. GitHub's security scanner then started complaining, correctly, that the size calculation below could overflow. Even though the code was unchanged from master. So I added this assertion, which is the exact same as the one in Hydra.

aeneasr · 2024-09-12T10:06:40Z

cmd/identities/get_test.go

@@ -63,10 +62,12 @@ func TestGetCmd(t *testing.T) {
 				return out
 			}
 			transform := func(token string) string {
-				if !encrypt {
-					return token
+				if encrypt {


I assume this changed because we now, somewhere, properly check if the token can be decrypted and then this test failed?

Exactly. I changed most tests across Kratos to actually use encryption, and this one was the only one to fail. Good sign :)

aeneasr · 2024-09-12T10:07:49Z

internal/driver.go

+		config.ViperKeySecretsCookie:   []string{randx.MustString(32, randx.AlphaNum)},
+		config.ViperKeySecretsDefault:  []string{randx.MustString(32, randx.AlphaNum)},


Are these now always required? If so, have the quickstarts been adjusted?

They are not strictly required, but I would consider it insecure to run Kratos without these even before this changeset.

aeneasr · 2024-09-12T10:09:09Z

...ng_used_by_password_credentials-case=should_fail_registration_id_first_strategy_enabled.json

@@ -5,6 +5,28 @@
  "ui": {
    "method": "POST",
    "nodes": [
+      {


Is it expected for these existing OIDC snapshots to be updated?

Yes, because I configured additional OIDC providers (those with PKCE=force,auto,never).

aeneasr · 2024-09-12T10:11:38Z

selfservice/strategy/oidc/pkce.go

+	return []oauth2.AuthCodeOption{oauth2.VerifierOption(s.GetPkceVerifier())}
+}
+
+func maybePKCE(ctx context.Context, d pkceDependencies, _p Provider) (verifier string) {


nit: underscore is possible to use in Go, but uncommon since it's the throwaway variable name, e.g. _ context.Context. Better would be to use p Provider and oauth2Provider, ok := p.(OAuth2Provider)

aeneasr · 2024-09-12T10:20:05Z

selfservice/strategy/oidc/strategy.go

-}
-
-func (s *State) String() string {
-	return base64.RawURLEncoding.EncodeToString([]byte(fmt.Sprintf("%s:%s", s.FlowID, s.Data)))


We rely on this format in RedirectSocialAuthenticationFlows in our other repository. Changing this method will most likely cause regressions there. Are you aware of this?

Yes I am aware, that's why the toggle (which defaults to the old-style state generation) exists in the first place. I will adjust the code in cloud to work with both state generation versions before we activate the new one.

aeneasr · 2024-09-12T10:20:43Z

selfservice/strategy/oidc/state.go

+}
+
+func ParseStateCompatiblity(ctx context.Context, c cipher.Cipher, s string) (*oidcv1.State, error) {
+	// new-style: encrypted


New style probably won't work with RedirectSocialAuthenticationFlows

Yep, we need a two-step rollout for this (hence the temporary toggle).

alnr self-assigned this Sep 4, 2024

alnr requested review from aeneasr, zepatrik and hperl as code owners September 4, 2024 14:54

github-advanced-security bot found potential problems Sep 4, 2024

View reviewed changes

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

alnr marked this pull request as draft September 4, 2024 15:05

alnr force-pushed the alnr/oidc-callback-url-2 branch 2 times, most recently from 79b76d6 to 03478f9 Compare September 9, 2024 15:38

alnr marked this pull request as ready for review September 9, 2024 17:16

alnr force-pushed the alnr/oidc-callback-url-2 branch from 408913d to 8397b5c Compare September 9, 2024 18:59

alnr mentioned this pull request Sep 10, 2024

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

Closed

alnr force-pushed the alnr/oidc-callback-url-2 branch 3 times, most recently from b098326 to 0c294ed Compare September 10, 2024 14:42

hperl approved these changes Sep 11, 2024

View reviewed changes

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

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

alnr added 4 commits September 11, 2024 16:29

chore: regenerate SDK, bump DB versions, cleanup tool install

51453e4

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

b8a5832

…tion and login

chore: fixup OIDC function signatures and improve tests

ee0fcd3

alnr force-pushed the alnr/oidc-callback-url-2 branch from 0c294ed to 91b5cd3 Compare September 11, 2024 14:38

alnr merged commit f7c1024 into master Sep 12, 2024
28 of 29 checks passed

alnr deleted the alnr/oidc-callback-url-2 branch September 12, 2024 07:47

aeneasr reviewed Sep 12, 2024

View reviewed changes

alnr mentioned this pull request Sep 13, 2024

fix: add PKCE config key to config schema #4098

Merged

alnr mentioned this pull request Sep 24, 2024

feat: enable new-style OIDC state generation #4121

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: client-side PKCE take 3 #4078

feat: client-side PKCE take 3 #4078

alnr commented Sep 4, 2024 •

edited

Loading

codecov bot commented Sep 9, 2024 •

edited

Loading

hperl left a comment

aeneasr left a comment

aeneasr Sep 12, 2024

aeneasr Sep 12, 2024

alnr Sep 12, 2024

aeneasr Sep 12, 2024

alnr Sep 12, 2024

aeneasr Sep 12, 2024

alnr Sep 12, 2024

aeneasr Sep 12, 2024

alnr Sep 12, 2024

aeneasr Sep 12, 2024

alnr Sep 12, 2024

aeneasr Sep 12, 2024

aeneasr Sep 12, 2024

alnr Sep 12, 2024

aeneasr Sep 12, 2024

alnr Sep 12, 2024


		var newStyleState = false

		func TestHookEnableNewStyleState(t *testing.T) {

		config.ViperKeySecretsCookie: []string{randx.MustString(32, randx.AlphaNum)},
		config.ViperKeySecretsDefault: []string{randx.MustString(32, randx.AlphaNum)},

feat: client-side PKCE take 3 #4078

feat: client-side PKCE take 3 #4078

Conversation

alnr commented Sep 4, 2024 • edited Loading

codecov bot commented Sep 9, 2024 • edited Loading

Codecov Report

hperl left a comment

Choose a reason for hiding this comment

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

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

Choose a reason for hiding this comment

alnr commented Sep 4, 2024 •

edited

Loading

codecov bot commented Sep 9, 2024 •

edited

Loading