Enable PKCE for reactive logout SPA flow test

[michalvavrik]

Summary

Enables PKCE for logout SPA flow test in LogoutSinglePageAppFlowIT and OpenShiftOidcSinglePageAppLogoutFlowIT. Reasoning explained in depth in https://github.com/quarkus-qe/quarkus-test-plans/blob/main/QUARKUS-2491.md. We know tests verifies PKCE as with changes in kc-logout-realm.json and without enabling PKCE in application.properties, tests are failing.

Please select the relevant options.

• Bug fix (non-breaking change which fixes an issue)
• Dependency update
• Refactoring
• Backport
• New scenario (non-breaking change which adds functionality)
• This change requires a documentation update
• This change requires execution against OCP (use run tests phrase in comment)

Checklist:

• Methods and classes used in PR scenarios are meaningful
• Commits are well encapsulated and follow the best practices

[michalvavrik]

run tests

[michalvavrik]

run tests

[rsvoboda]

Is there a TP for this?

[michalvavrik]

@rsvoboda that's in description, I know TP review might change a scope of this PR heavily, but I still wanted to run CI :-)

[michalvavrik]

@rsvoboda https://github.com/quarkus-qe/quarkus-test-plans/blob/main/QUARKUS-2491.md

[rsvoboda]

TP is already merged, so I will put mi thoughts here

quarkus.oidc.authentication.pkce-required is not set, but Keycloack expects that
This is what I get in the test:

[ERROR] Tests run: 1, Failures: 0, Errors: 1, Skipped: 0, Time elapsed: 38.071 s <<< FAILURE! - in io.quarkus.ts.security.keycloak.oidcclient.reactive.extended.LogoutSinglePageAppFlowIT
[ERROR] singlePageAppLogoutFlow  Time elapsed: 2.068 s  <<< ERROR!
com.gargoylesoftware.htmlunit.FailingHttpStatusCodeException: 401 Unauthorized for http://localhost:1101/code-flow?error=invalid_request&error_description=Missing+parameter%3A+code_challenge_method&state=330baf5c-786b-4d3f-9253-f4baa6a40ae8
	at com.gargoylesoftware.htmlunit.WebClient.throwFailingHttpStatusCodeExceptionIfNecessary(WebClient.java:765)
	at com.gargoylesoftware.htmlunit.WebClient.getPage(WebClient.java:520)
	at com.gargoylesoftware.htmlunit.WebClient.getPage(WebClient.java:417)
	at com.gargoylesoftware.htmlunit.WebClient.getPage(WebClient.java:555)
	at com.gargoylesoftware.htmlunit.WebClient.getPage(WebClient.java:537)
	at io.quarkus.ts.security.keycloak.oidcclient.reactive.extended.LogoutSinglePageAppFlowIT.makeHttpPostFormLogin(LogoutSinglePageAppFlowIT.java:76)
	at io.quarkus.ts.security.keycloak.oidcclient.reactive.extended.LogoutSinglePageAppFlowIT.singlePageAppLogoutFlow(LogoutSinglePageAppFlowIT.java:50)

Can be the used somehow notified about the need for PKCE / https://quarkus.io/guides/security-openid-connect-web-authentication#proof-of-key-for-code-exchange-pkce ?
I know the response comes from Keycloak which is not aware of the client details / Quarkus. but still would be nice if this could be detected on Quarkus side. WDYT?

Keycloak with "pkce.code.challenge.method" : "plain"
Quarkus impl supports only S256 but what if the provider is configured to plain?
In the test I get com.gargoylesoftware.htmlunit.FailingHttpStatusCodeException: 401 Unauthorized for http://localhost:1101/code-flow?error=invalid_request&error_description=Invalid+parameter%3A+code+challenge+method+is+not+configured+one&state=7653818c-ce5b-412e-935a-ab6ed4bbd87b when I change the relevant json file part.
The message is again quite PKCE specific and user may not be necessarily aware of that config being available / enabled. Maybe it's an input towards Keycloak to add PKCE keyword. WDYT?

Is PKCE optional?
When Keycloak is set to support PKCE, it seems all the apps need to be adjusted to use that. Is it correct conclusion?

[michalvavrik]

I think these are damn good questions, but I had a long day, I'll investigate tomorrow and let you know.

[rsvoboda]

Sure, no worries. We have time to check the stuff with Minor priority, nobody pushes us to that this week. We will handle them in January.

[rsvoboda]

@michalvavrik fyi quarkusio/quarkus#29816

[michalvavrik]

TP is already merged, so I will put mi thoughts here

quarkus.oidc.authentication.pkce-required is not set, but Keycloack expects that This is what I get in the test:

[ERROR] Tests run: 1, Failures: 0, Errors: 1, Skipped: 0, Time elapsed: 38.071 s <<< FAILURE! - in io.quarkus.ts.security.keycloak.oidcclient.reactive.extended.LogoutSinglePageAppFlowIT
[ERROR] singlePageAppLogoutFlow  Time elapsed: 2.068 s  <<< ERROR!
com.gargoylesoftware.htmlunit.FailingHttpStatusCodeException: 401 Unauthorized for http://localhost:1101/code-flow?error=invalid_request&error_description=Missing+parameter%3A+code_challenge_method&state=330baf5c-786b-4d3f-9253-f4baa6a40ae8
	at com.gargoylesoftware.htmlunit.WebClient.throwFailingHttpStatusCodeExceptionIfNecessary(WebClient.java:765)
	at com.gargoylesoftware.htmlunit.WebClient.getPage(WebClient.java:520)
	at com.gargoylesoftware.htmlunit.WebClient.getPage(WebClient.java:417)
	at com.gargoylesoftware.htmlunit.WebClient.getPage(WebClient.java:555)
	at com.gargoylesoftware.htmlunit.WebClient.getPage(WebClient.java:537)
	at io.quarkus.ts.security.keycloak.oidcclient.reactive.extended.LogoutSinglePageAppFlowIT.makeHttpPostFormLogin(LogoutSinglePageAppFlowIT.java:76)
	at io.quarkus.ts.security.keycloak.oidcclient.reactive.extended.LogoutSinglePageAppFlowIT.singlePageAppLogoutFlow(LogoutSinglePageAppFlowIT.java:50)

Can be the used somehow notified about the need for PKCE / https://quarkus.io/guides/security-openid-connect-web-authentication#proof-of-key-for-code-exchange-pkce ? I know the response comes from Keycloak which is not aware of the client details / Quarkus. but still would be nice if this could be detected on Quarkus side. WDYT?

I think Quarkus OIDC should work for all providers and I don't see something about this in specs, but I'm very far from being expert here. I think we could open Quarkus feature issue and ask Pedro Igor or Sergey if there is some Keycloak endpoint that provides the information. Personally I wouldn't do it as it necessarily requires call to Keycloak to find out, right? However it will make sense to do with Oauth2.1 and code flow that shouldn't work without PKCE.

Anyway I found a note in one Quarkus issue that IMHO suggests it can be discovered, please see quarkusio/quarkus#23579 (comment)

Keycloak with "pkce.code.challenge.method" : "plain" Quarkus impl supports only S256 but what if the provider is configured to plain?

Docs mentions just s256 https://quarkus.io/guides/security-openid-connect-web-authentication#proof-of-key-for-code-exchange-pkce and the PR says plain is not important quarkusio/quarkus#23423. I checked opened issues and I don't see any user request. If provider is set to plain, challenge won't ever be successful on provider side.

In the test I get com.gargoylesoftware.htmlunit.FailingHttpStatusCodeException: 401 Unauthorized for http://localhost:1101/code-flow?error=invalid_request&error_description=Invalid+parameter%3A+code+challenge+method+is+not+configured+one&state=7653818c-ce5b-412e-935a-ab6ed4bbd87b when I change the relevant json file part. The message is again quite PKCE specific and user may not be necessarily aware of that config being available / enabled. Maybe it's an input towards Keycloak to add PKCE keyword. WDYT?

I agree having PKCE in description would be nice thing, but response exactly comply specs https://www.rfc-editor.org/rfc/rfc7636#section-4.4.1, code challenge method should be known term to anyone who enable PKCE and PKCE is not enabled by default. You usually develop in small steps/iterations, so such a small step would be to enable PKCE, get error, google error message and first thing you get is PKCE. I'll open PR in Keycloak that adjusts description (busy now), but I don't know if it will be accepted......

Is PKCE optional? When Keycloak is set to support PKCE, it seems all the apps need to be adjusted to use that. Is it correct conclusion?

IMHO no, citing specs If the server requires Proof Key for Code Exchange (PKCE) by OAuth
public clients and the client does not send the "code_challenge" in
the request, the authorization endpoint MUST return the authorization
error response with the "error" value set to "invalid_request".
and Keycloak expects that too https://github.com/keycloak/keycloak/blob/main/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/oauth/OAuthProofKeyForCodeExchangeTest.java#L619 and does https://github.com/keycloak/keycloak/blob/main/services/src/main/java/org/keycloak/broker/oidc/AbstractOAuth2IdentityProvider.java#L359

P.S. I'll add you @rsvoboda as (optional) reviewer as you seem to know a lot about the topic.

[rsvoboda]

Personally I wouldn't do it as it necessarily requires call to Keycloak to find out, right?

Yes, seems not worth the extra code and call in PKCE case.

However it will make sense to do with Oauth2.1 and code flow that shouldn't work without PKCE.

+1

I'll open PR in Keycloak that adjusts description

+1

you seem to know a lot about the topic

Thanks, but not a pro in the area. I was just trying to explore various usa-cases.