Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

PKCE support is not enough #471

Closed
2 tasks done
ay4toh5i opened this issue Oct 30, 2023 · 1 comment
Closed
2 tasks done

PKCE support is not enough #471

ay4toh5i opened this issue Oct 30, 2023 · 1 comment

Comments

@ay4toh5i
Copy link
Contributor

ay4toh5i commented Oct 30, 2023

Preflight Checklist

  • I could not find a solution in the existing issues, docs, nor discussions
  • I have joined the ZITADEL chat

Describe your problem

Thanks such a nice library!

I notice that pkce is verified only when the auth method is none in code exchenge.

oidc/pkg/op/token_code.go

Lines 96 to 103 in d58ab6a

if client.AuthMethod() == oidc.AuthMethodNone {
request, err = AuthRequestByCode(ctx, exchanger.Storage(), tokenReq.Code)
if err != nil {
return nil, nil, err
}
err = AuthorizeCodeChallenge(tokenReq.CodeVerifier, request.GetCodeChallenge())
return request, client, err
}

However, pkce should be verified in other methods to mitigate vulnerabilities as decribed below.

PKCE is not a form of client authentication, and PKCE is not a replacement for a client secret or other client authentication. PKCE is recommended even if a client is using a client secret or other form of client authentication like private_key_jwt.

https://oauth.net/2/pkce/

Describe your ideal solution

If pkce params requested in authorize, verifying that as possible is ideal.

Version

No response

Environment

Self-hosted

Additional Context

No response

@muhlemmer
Copy link
Collaborator

Although I agree with the recommendation, oauth.net is not a definitive resource for the standard, which is described in RFCs. What you describe is Oauth V2.1 I believe. We already have an issue open for that: #254. You can vote there with a thumbs up and we can assign priorities based on that.

At the moment we are compliant with Oauth V2. Closing as duplicate.

@muhlemmer muhlemmer closed this as not planned Won't fix, can't repro, duplicate, stale Oct 31, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants