feat: add support for Proof Key For Code Exchange (PKCE) in OIDC social providers #4033
Conversation
|
|
OskarsPakers
changed the title
OskarsPakers
changed the title
alnr
changed the title
There was a problem hiding this comment.
Thank you for your contribution! This looks very good already. I have added some comments below, mostly to make sure that this code also works for registration and settings flows.
Also, I'd like to see more tests on this. So far, I could only see unit tests for the provider. We also need E2E tests that make sure that the challenge and verifier are correctly persisted and compared, with both happy and error paths covered.
| @@ -96,6 +98,25 @@ func (g *ProviderGenericOIDC) AuthCodeURLOptions(r ider) []oauth2.AuthCodeOption | |||
| return options | |||
| } | |||
|
|
|||
| func (g *ProviderGenericOIDC) addPKCSURLOptions(r ider, options []oauth2.AuthCodeOption) []oauth2.AuthCodeOption { | |||
| flow, err := g.reg.LoginFlowPersister().GetLoginFlow(context.Background(), r.GetID()) | |||
There was a problem hiding this comment.
r could also be a registration flow, correct? In that case, we won't find a login flow. Also, the context needs to be bound to the user's request context, not context.Background().
I think you can just add flow.InternalContexter to the ider interface, and use r directly.
| @@ -386,3 +404,34 @@ func (s *Strategy) PopulateLoginMethodIdentifierFirstCredentials(r *http.Request | |||
| func (s *Strategy) PopulateLoginMethodIdentifierFirstIdentification(r *http.Request, f *login.Flow) error { | |||
| return s.populateMethod(r, f, text.NewInfoLoginWith) | |||
| } | |||
|
|
|||
| func SetPKCSContext(flow flow.InternalContexter, context PkcsContext) error { | |||
There was a problem hiding this comment.
I'd move this and the next method into the selfservice/flow package. There is probably also some opportunity to make a generic getter/setter, as these methods are very similar to Get/SetDuplicateCredentials.
| client := s.d.HTTPClient(ctx) | ||
| ctx = context.WithValue(ctx, oauth2.HTTPClient, client.HTTPClient) | ||
| token, err = te.Exchange(ctx, code) | ||
| return token, err | ||
| switch loginFlow := flow.(type) { |
There was a problem hiding this comment.
Why does this not work for registration flows?
| if pkcsContext.Verifier != "" && (pkcsContext.Method == "S256" || pkcsContext.Method == "plain") { | ||
| return te.Exchange(ctx, code, oauth2.VerifierOption(pkcsContext.Verifier)) | ||
| } else { | ||
| return nil, errors.Errorf("Invalid PKCS method: %s or empty verifier: %s", pkcsContext.Method, pkcsContext.Verifier) |
There was a problem hiding this comment.
This should be a herodot error as well.
| @@ -255,6 +262,17 @@ func (s *Strategy) Login(w http.ResponseWriter, r *http.Request, f *login.Flow, | |||
| } | |||
|
|
|||
| state := generateState(f.ID.String()) | |||
|
|
|||
| if provider.Config().PKCSMethod != "" { | |||
There was a problem hiding this comment.
This also needs to be done for registration. And how about settings?
|
Closing this one as it seems to be continued in #4033 |
Adds support for Proof Key For Code Exchange (PKCS) in OIDC social providers according to rfc7636 https://datatracker.ietf.org/doc/html/rfc7636
Related issue(s)
#4009
Checklist
introduces a new feature.
contributing code guidelines.
vulnerability. If this pull request addresses a security vulnerability, I
confirm that I got the approval (please contact
[email protected]) from the maintainers to push
the changes.
works.
Further Comments
Supports methods s256 and plain.
AuthCodeURLOptions method signature does not return err, therefore can`t return an error. Should we change the signature of the method and lift the functionality up to the strategy code?