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

Disable PKCE for confidential clients #16

Closed
wants to merge 3 commits into from

Conversation

danj3
Copy link

@danj3 danj3 commented Apr 15, 2024

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.

@paulswartz
Copy link

Confidential clients can (and should) still use PKCE. Were you working with a client which doesn't validate when PKCE is used?

@maennchen maennchen added the enhancement New feature or request label Apr 15, 2024
@maennchen maennchen changed the title Feat/access type Disable PKCE for confidential clients Apr 15, 2024
@maennchen
Copy link
Member

maennchen commented Apr 15, 2024

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:

OAuth 2.0 [RFC6749] public clients are susceptible to the
authorization code interception attack.

In this attack, the attacker intercepts the authorization code
returned from the authorization endpoint within a communication path
not protected by Transport Layer Security (TLS), such as inter-
application communication within the client's operating system.

Once the attacker has gained access to the authorization code, it can
use it to obtain the access token.

[...]

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 access_type.

@@ -4,7 +4,7 @@ defmodule Oidcc.Plug.MixProject do
def project do
[
app: :oidcc_plug,
version: "0.1.1",
Copy link
Member

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.

@coveralls
Copy link

coveralls commented Apr 15, 2024

Pull Request Test Coverage Report for Build 38b8a5e12f83d1c26e2b53a2f752a2397aa688af-PR-16

Details

  • 3 of 4 (75.0%) changed or added relevant lines in 1 file are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage decreased (-0.8%) to 99.187%

Changes Missing Coverage Covered Lines Changed/Added Lines %
lib/oidcc/plug/authorize.ex 3 4 75.0%
Totals Coverage Status
Change from base Build 94c6b71639887651eb4e78d4b57db5dce97975bc: -0.8%
Covered Lines: 122
Relevant Lines: 123

💛 - Coveralls

@paulswartz
Copy link

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

For confidential clients, the use of PKCE [RFC7636] is RECOMMENDED, as it provides a strong protection against misuse and injection of authorization codes as described in Section 4.5.3.1 and, as a side-effect, prevents CSRF even in presence of strong attackers as described in Section 4.7.1.

If you want to disable PKCE, I believe adding a configuration override to set code_challenge_methods_supported to [] might work?

@maennchen
Copy link
Member

maennchen commented Apr 15, 2024

@paulswartz Oh, good to know 👍

In that case I tend to agree. But we should probably document this.

@danj3
Copy link
Author

danj3 commented Apr 16, 2024

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.

@paulswartz
Copy link

paulswartz commented Apr 16, 2024

If you continue to run into issues, feel free to check in on Slack: I'm @paulswartz on both the Elixir and EEF instances.

@danj3
Copy link
Author

danj3 commented Apr 16, 2024

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?

@maennchen
Copy link
Member

@danj3 Oh, I never released #13

Are your problems going away if you’re using the newest main?

@danj3
Copy link
Author

danj3 commented Apr 16, 2024

main fixes this problem completely!

@maennchen
Copy link
Member

maennchen commented Apr 17, 2024

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.

@maennchen
Copy link
Member

@danj3 Fix is released as v0.1.2.

It was great meeting you at Elixir Conf 😄

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants