-
-
Notifications
You must be signed in to change notification settings - Fork 5
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
Disable PKCE for confidential clients #16
Conversation
Confidential clients can (and should) still use PKCE. Were you working with a client which doesn't validate when PKCE is used? |
I've used OpenID providers before that supported (and even suggested) using PKCE for confidential clients. But I've also seen multiple implementations doing what this PR does. The RFC mentions this about the topic:
https://datatracker.ietf.org/doc/html/rfc7636#section-1 While this suggests that the RFC is designed for public clients, it never specifically restricts it to public clients. If there's any providers considering PKCE for confidential clients as a problem, we should certainly offer a way to disable it. But then I would probably not decide based on a property called |
@@ -4,7 +4,7 @@ defmodule Oidcc.Plug.MixProject do | |||
def project do | |||
[ | |||
app: :oidcc_plug, | |||
version: "0.1.1", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Releases are handled outside of PRs.
Pull Request Test Coverage Report for Build 38b8a5e12f83d1c26e2b53a2f752a2397aa688af-PR-16Details
💛 - Coveralls |
It's also RECOMMENDED by OAuth2 Security Best Current Practice: https://datatracker.ietf.org/doc/html/draft-ietf-oauth-security-topics#section-2.1.1
If you want to disable PKCE, I believe adding a configuration override to set |
@paulswartz Oh, good to know 👍 In that case I tend to agree. But we should probably document this. |
I agree, the coupling of a confidential client with enable/disable PKCE is not correct, the option should be specific to PKCE. At the time I was unsuccessful combining a confidential client and PKCE with Keycloak. Your comments reinforce that this must be possible, so I will revisit and follow up. |
If you continue to run into issues, feel free to check in on Slack: I'm @paulswartz on both the Elixir and EEF instances. |
I've discovered my specific problem and it appears to be a bug in Keycloak when the code_verifier string contains URL escaped chars, like %2F for /, it fails. To isolate I changed to base32 and reduced the crypt string to keep the length < 128 and then it started working. Are any of these properties in question from a standards perspective? |
main fixes this problem completely! |
Awesome! I‘ll close this PR then. I‘m a bit busy at the moment. (Traveling to Elixir Conf today 😁) I‘ll get a release out next week. |
@danj3 Fix is released as It was great meeting you at Elixir Conf 😄 |
I found this necessary when using oidcc_plug to enable generation auth urls using confidential clients. If there was another approach, it was not apparent to me. Thank you for writing oidcc, a vast improvement over the ad-hoc combination of OAuth2 and Keycloak projects I was using.