Support for OIDC Proof Of Key for Code Exchange (PKCE)

[sberyozkin]

Fixes #12856

This PR resolves one of the oldest open OIDC issues. It introduces PKCE support (without the plain challenge algorithm which is not really required).

It is not really strictly necessary for Quarkus OIDC web-app applications which are confidential OIDC clients capable of storing safely the client secret - thus eliminating the risk of the code being used by someone else to get the token.
However it does add one more layer of protection even in this case, OAuth 2.1 will not accept the code flows without PKCE, and it will help with some integrations, example, with Twitter (@FroMage, FYI).

So it is the right time to add this feature.

The actual PKCE support is not difficult - but I had to introduce an option to encrypt the extra state cookie content to avoid leaking the code verifier. I decided for now not to encrypt it all into the actual state query parameter as it can stress the URI limit, so it is kept in the encrypted form in the state cookie. If the PKCE is not required then the state cookie will not have any encryption applied to it, there is nothing sensitive in it without the PKCE code verifier.

A256KW requiring a 32 byte keys is currently used by default which is very secure but I've opened a smallrye-jwt issue to facilitate the use of A256GCMKW and switch to it a bit later on.

Testing that it works against Keycloak proved easy enough, it had to be done against Keycloak. But testing that it does not work after submitting a code challenge and dropping the code verifier during the code exchange proved trickier, with Wiremock it would be straightforward but with Wiremock we can't really prove that quarkus-oidc calculates the code verifier and challenge correctly.

So I have updated the existing CodeFlowTest https tests to verify the content of the state cookie that it contains the correct code verifier and it matches, after the s256 digest applied to it, the code challenge query parameter, then I created a new test, where this verification also passes, then, before forwarding a redirect from Keycloak to Quarkus, I removed the existing state cookie, created a new one containing only the state value which is also returned from Keycloak, but without the code verifier, and this time it reports 401 as expected. Not sure if anything else can be done to verify this case.

Docs have also been updated.

[FroMage]

Twitter support would be good! I didn't figure out how to make it work.

[sberyozkin]

@pedroigor Hi Pedro, this is now ready for review

[sberyozkin]

@FroMage Hi Steph, according to https://developer.twitter.com/en/docs/authentication/oauth-2-0/authorization-code, if you choose OAuth2 (as opposed to OAuth 1.1) settings then it should work with this PR - since they require PKCE, see Authorize URL example there; once you get the code it should all work similarly to GitHub, they should have a UserInfo related endpoint, as I'm not sure they support IdTokens.

[sberyozkin]

@FroMage It is all described here, https://developer.twitter.com/en/docs/authentication/oauth-2-0/user-access-token

You can give it a quick try once PR is merged or even while it is being reviewed

[quarkus-bot]

This workflow status is outdated as a new workflow run has been triggered.

Failing Jobs - Building 99fde66

Status	Name	Step	Failures	Logs	Raw logs
✖	Initial JDK 11 Build	Build	Failures	Logs	Raw logs

Failures

⚙️ Initial JDK 11 Build #

- Failing: extensions/oidc/runtime 
! Skipped: devtools/bom-descriptor-json docs extensions/keycloak-authorization/deployment and 20 more

📦 extensions/oidc/runtime

✖ Failed to execute goal net.revelc.code.formatter:formatter-maven-plugin:2.17.1:validate (default) on project quarkus-oidc: File '/home/runner/work/quarkus/quarkus/extensions/oidc/runtime/src/main/java/io/quarkus/oidc/OidcTenantConfig.java' has not been previously formatted. Please format file and commit before running validation!

[sberyozkin]

Hmm, I might've done some JavaDocs updates at the last minute

[pedroigor]

@sberyozkin Nice !

Sorry if I missed it, but perhaps we should also make sure the AS supports S256 method when bootstrapping?

Or perhaps PKCE should be enabled whenever the AS supports method S256 with the option to disable it using the new option?

Other than those comments, LGTM.

[sberyozkin]

Hi @pedroigor Thanks for the helpful comments, see some of the responses above, and will follow with a charset update shortly

Sorry if I missed it, but perhaps we should also make sure the AS supports S256 method when bootstrapping?

It makes sense but would be a bit concerned that it can be problematic in cases where the provider does not support the discovery (Twitter probably does not) or when they just don't return the info about PKCE

Or perhaps PKCE should be enabled whenever the AS supports method S256 with the option to disable it using the new option?

That is interesting. I was contemplating something along these lines, but then decided not to because of

• there is some extra cost associated with supporting PKCE (namely the code verifier encryption/decryption) - it is not a big cost but for ex making it a default for all Keycloak web-app users might be of concern
• but I guess more important is that if the quarkus endpoint has a client id and a client secret (or JWT or MTLS authentication) then this Quarkus endpoint is a confidential client, so the code, even if hijacked, would fail with only the client id but without the secret, so PKCE is not strictly necessary in this case.

But, once we implement your idea of a secure profile - then I think it will have to be automatically enabled if the provider supports it unless the user has set pkce-required=false - as per your proposal.

Does it sound OK ?

However if Quarkus web-app has a public client then PKCE would certainly help - but there'd be no way to encrypt the code verifier in the state cookie since no secrets will be configured.

I think you reminding about nonce would work best when it comes to dealing with Quarkus web-app public clients - as it can always be transparently added. We can probably add it for confidential clients by default as well, let me open an issue a bit later on and discuss,

Hope it makes sense

[pedroigor]

@sberyozkin Thanks for the clarifications.

[sberyozkin]

Hey @pedroigor

I've opened #23579 and #23580, the last one is easier so I guess I'll tackle it first :-), the charset update is on the way, but I'd also like to tweak the code a bit and precalculate a secret key if only a default tenant is set (as related to your comment in the other PR, #23557)

[sberyozkin]

Hi @pedroigor I forgot each tenant has a corresponding TenantConfigContext so it can hold nicely the precalculated and verified secret key, I updated the code, thanks for the idea, will follow with the same in #23557

[rolfedh]

Hi, @sberyozkin
Would you be willing to review the following release note content?

• Google doc: https://docs.google.com/document/d/1TZgLzYNF0aEfS8AGun2gJ2B63B2q12M7v1wXVah1cUA/edit?usp=sharing

From:

• Merge request (private repo): https://gitlab.cee.redhat.com/quarkus-documentation/quarkus/-/merge_requests/735
• Preview (publicly visible): https://jenkins.dxp.redhat.com/job/CCS/job/ccs-mr-preview/34239/artifact/titles/release_notes/preview/index.html#cloud

Content:

- 1.3.1. Cloud

1.3.1.1. Support for Proof Key for Code Exchange (PKCE) in OIDC
This update adds support for Proof Key for Code Exchange (PKCE) to the 
Quarkus implementation of the OpenID Connect (OIDC) protocol. PKCE 
minimizes the risk of authorization code interception for Quarkus web 
applications that use OIDC. For more information, see 
[the PKCE-related section](https://quarkus.io/guides/security-openid-connect-web-authentication#proof-of-key-for-code-exchange-pkce) of the Quarkus "OpenID Connect (OIDC) Authorization 
Code Flow Mechanism" guide.