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.

Sign up for GitHub

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

Jump to bottom

feat(op): always verify code challenge when available #721

Merged
merged 7 commits into from
Mar 24, 2025
Merged

feat(op): always verify code challenge when available #721

merged 7 commits into from
Mar 24, 2025

Conversation

ay4toh5i
Copy link
Contributor

@ay4toh5i ay4toh5i commented Mar 1, 2025 •
edited
Loading

refs: #254, #471

Finally the RFC Best Current Practice for OAuth 2.0 Security has been approved.

According to the RFC:

Authorization servers MUST support PKCE [RFC7636].

If a client sends a valid PKCE code_challenge parameter in the authorization request, the authorization server MUST enforce the correct usage of code_verifier at the token endpoint.

Isn’t it time we strengthen PKCE support a bit more?

This PR updates the logic so that PKCE is always verified, even when the Auth Method is not "none".
Could you please review this PR?

And please let me know if there’s anything missing or if there’s anything else I should address.

Definition of Ready

  • I am happy with the code
  • Short description of the feature/issue is added in the pr description
  • PR is linked to the corresponding user story
  • Acceptance criteria are met
  • All open todos and follow ups are defined in a new ticket and justified
  • Deviations from the acceptance criteria and design are agreed with the PO and documented.
  • No debug or dead code
  • My code has no repetitions
  • Critical parts are tested automatically
  • Where possible E2E tests are implemented
  • Documentation/examples are up-to-date
  • All non-functional requirements are met
  • Functionality of the acceptance criteria is checked manually on the dev system.

@ay4toh5i ay4toh5i mentioned this pull request Mar 1, 2025
Closed
@hifabienne hifabienne requested a review from livio-a March 3, 2025 10:29
@muhlemmer muhlemmer requested review from muhlemmer and removed request for livio-a March 12, 2025 10:47
muhlemmer
muhlemmer reviewed Mar 12, 2025
View reviewed changes
Copy link
Collaborator

@muhlemmer muhlemmer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

With this change AuthorizeCodeChallenge is called 3 different times within AuthorizeCodeClient depending on the code path.

Can we simplify the function to only do a single check, perhaps in a higher scope at the start of the AuthorizeCodeClient function?

See how we do it in Zitadel for example:

https://github.com/zitadel/zitadel/blob/306ce97828aefa567e2bc0b1d207f98a65cf7ba7/internal/api/oidc/token_code.go#L64-L68

pkg/op/token_code.go Outdated
codeChallenge := request.GetCodeChallenge()
if codeChallenge != nil && codeChallenge.Challenge != "" {
err = AuthorizeCodeChallenge(tokenReq.CodeVerifier, request.GetCodeChallenge())

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pass the codeChallenge variable here instead, no need to re-get.

muhlemmer

This comment was marked as off-topic.

Sign in to view

@muhlemmer
Copy link
Collaborator

Oops. responded to the wrong PR with the last comment. Disregard that please.

@muhlemmer muhlemmer added the waiting For some reason, this issue will have to wait. This can be a feedback that is being waited for, a de label Mar 12, 2025
@ay4toh5i
Copy link
Contributor Author

ay4toh5i commented Mar 13, 2025 •
edited
Loading

@muhlemmer
Yes, I was thinking the same thing. I tried making some changes based on the example you provided—what do you think?
b06e76c

I’m still in the process of fixing the tests, so please bear with me a bit longer until everything’s ready.

→ Done.

@ay4toh5i ay4toh5i requested a review from muhlemmer March 15, 2025 13:34
@muhlemmer muhlemmer removed the waiting For some reason, this issue will have to wait. This can be a feedback that is being waited for, a de label Mar 24, 2025
muhlemmer
muhlemmer approved these changes Mar 24, 2025
View reviewed changes
Copy link
Collaborator

@muhlemmer muhlemmer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

great changes, thanks!

@muhlemmer muhlemmer changed the title Check PKCE even when the Auth Method is not “none”. feat(op): always verify code challenge when available Mar 24, 2025
@muhlemmer muhlemmer merged commit c51628e into zitadel:main Mar 24, 2025
5 checks passed
@github-actions GitHub Actions
Copy link

🎉 This PR is included in version 3.37.0 🎉

The release is available on GitHub release

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@muhlemmer muhlemmer muhlemmer approved these changes

Assignees
No one assigned
Labels
os-contribution released
Projects
Product Management
Status: ✅ Done
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@ay4toh5i @muhlemmer @hifabienne