quarkus icon indicating copy to clipboard operation
quarkus copied to clipboard

Support for OIDC FrontChannel logout

Open sberyozkin opened this issue 3 years ago • 6 comments

Fixes #23478

This PR supports a front-channel logout request. If the current request path matches the configured frontchannel logout URL then the sid and iss query parameters are compared against the verified ID token's values and if all is good then the session cookie is removed.

As part of this work I also moved the backchannel logout check from the earlier PR to the same stage where frontchannel logout URL is checked, after id token has been verified (instead of doing a limited token validation in the backchannel check code) - this allowed to include sending both the backchannel and frontchannel logout CDI events, referencing the identity of the now logged out user.

Wiremock based test has been added.

I can also try to do a manual verification directly against Keycloak (it is not possible to set up the frontchannel URL in the Client using the admin API)

sberyozkin avatar May 03 '22 17:05 sberyozkin

Not sure if supporting POST requests is necessary, the spec talks about the query parameters only - but if needed it can be easily added. I'll test with Keycloak next

sberyozkin avatar May 03 '22 21:05 sberyozkin

@pedroigor I haven't managed to confirm it with Keycloak - I have enabled FrontChannel logout and added a quarkus endpoint url which should be called. Then I logged in to Quarkus as alice, then started an RP initiated logout which redirected me to Keycloak but at this point I expected, not exactly sure what, I guess some page which would internally call to this Quarkus frontchannel logout path, but it does not happen. How can one tell Keycloak to return a page to the user with the iframe calling out to Quarkus frontchannel logout URL ?

sberyozkin avatar May 04 '22 17:05 sberyozkin

@tassadar81 Can you please check this PR and confirm it a front-channel logout works for you ? I can't make it work with Keycloak - I have configured Keycloak client to link to a Quarkus endpoint's frontchannel URL, and initiated a logout from Quarkus, however I'm not getting a frontchannel endpoint call, it is most likely to do with the way I'm trying to reproduce it, as far as this PR is concerned, it just adds a simple code to verify it is a valid frontchannel call

sberyozkin avatar May 06 '22 13:05 sberyozkin

@tassadar81 any chance you could check if this PR works for you?

gsmet avatar Jun 21 '22 14:06 gsmet

@tassadar81 any chance you could check if this PR works for you?

Hello Guillaume, hello all, it's in the todo list, sorry for that but i'm overwhelmed at work

tassadar81 avatar Jun 22 '22 10:06 tassadar81

@sberyozkin this will need a rebase unfortunately @tassadar81 friendly ping for the test, you should be able to test this branch even before the rebase

gsmet avatar Aug 09 '22 21:08 gsmet

@sberyozkin this will need a rebase unfortunately @tassadar81 friendly ping for the test, you should be able to test this branch even before the rebase

gsmet avatar Aug 31 '22 20:08 gsmet

What's the status of this one?

geoand avatar Nov 04 '22 06:11 geoand

@geoand Trying to finalize it next

sberyozkin avatar Nov 24 '22 19:11 sberyozkin

@pjgg Hi Pablo, FYI, this PR introduced a front-channel logout support, but along the way, as I mentioned earlier, events are generated for both initiating and completing back channel logout. One test is failing now, I haven't had time to investigate yet, @michalvavrik, Hi Michal, quick question, this exception, which I have debugged is being thrown when this test line is run, but CodeAuthenticationMechanism#getChallenge is not invoked, it does appear to be similar to the problem you fixed with CodeFlowTest#testRPInitiatedLogout, do we need to tighten that fix a bit to take care of AuthenticationFailedException as well, when this PR was opened awhile back this test was passing. I'll be happy to have a look if you think it can be related, just hoping to clarify a few details first :-)

sberyozkin avatar Nov 24 '22 19:11 sberyozkin

@michalvavrik, Hi Michal, quick question, this exception, which I have debugged is being thrown when this test line is run, but CodeAuthenticationMechanism#getChallenge is not invoked, it does appear to be similar to the problem you fixed with CodeFlowTest#testRPInitiatedLogout, do we need to tighten that fix a bit to take care of AuthenticationFailedException as well, when this PR was opened awhile back this test was passing. I'll be happy to have a look if you think it can be related, just hoping to clarify a few details first :-)

When AuthenticationFailedException is thrown here https://github.com/quarkusio/quarkus/pull/25343/files#diff-d6d40795b1093d6a39a62041f2567d0931b1f7fa30ebfea44e6663f45441df75R261, it's caught here https://github.com/quarkusio/quarkus/blob/3e90e9aec4ec7746bc3c4d044b8331a293fa1e03/extensions/oidc/runtime/src/main/java/io/quarkus/oidc/runtime/CodeAuthenticationMechanism.java#L283 and AuthenticationCompletionException is thrown, thus 401 and no challenge. I didn't investigate why it worked before is it's bit late, but if you want me to have a look, no problem, please let me know.

michalvavrik avatar Nov 24 '22 22:11 michalvavrik

@michalvavrik Oh sorry, should've guessed myself, I just got side-tracked by remembering your fix :-), I'll take care of fixing it. Thanks for having a look.

sberyozkin avatar Nov 24 '22 22:11 sberyozkin


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


Failing Jobs - Building d6a40cea5c248d16dbefa98890cb47a2d277521a

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

Failures

:gear: Initial JDK 11 Build #

- Failing: extensions/oidc/runtime 
! Skipped: devtools/bom-descriptor-json docs extensions/keycloak-admin-client-reactive/deployment and 24 more

:package: extensions/oidc/runtime

Failed to execute goal org.apache.maven.plugins:maven-compiler-plugin:3.8.1:compile (default-compile) on project quarkus-oidc: Compilation failure

quarkus-bot[bot] avatar Nov 25 '22 12:11 quarkus-bot[bot]

Sorry, forgot to include a new class in the commit

sberyozkin avatar Nov 25 '22 14:11 sberyozkin

Hi @pjgg Can you please check this PR against your OIDC back channel demo, Pedro approved this PR awhile back and I only had to tweak it a bit for the tests to start passing again, but the backchannel code has changed compared to what you have been testing against: now the back channel logout can be completed only after the ID token has been verified, it is a bit stricter now, and in addition you can get backchannel initiate and completion CDI events - which you were interested in I believe.

Note the events are typically including a verified SecurityIdentity but with the back-channel initiation, when the request is coming from KC as opposed to from the user, SecurityIdentity is not available - so in this case one can get a JsonObject representing a parsed/verified logout token; the back-channel completion event will though get SecurityIdentity included - which is the main reason why the back channel logout completion now is done only after the ID token has been verified.

As far as the front-channel logout is concerned - it can be verified with a demo later, the PR wiremock test should be sufficient for now, I've added a release noteworthy label... Thanks

sberyozkin avatar Nov 25 '22 14:11 sberyozkin

OK, I'm going to go ahead and merge once the builds pass; it is not going to be backported so it is not urgent for QE to verify, Pablo is very busy elsewhere right now but it will be eventually verified as well in a few weeks or so

sberyozkin avatar Nov 25 '22 15:11 sberyozkin