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

Baseline for OpenID Connect Logout #1882

Open
wants to merge 1 commit into
base: 2.x
Choose a base branch
from

Conversation

pedroigor
Copy link
Contributor

@pedroigor pedroigor commented Mar 17, 2023

The scope is:

  • RP-Initiated Logout
  • Frontchannel Logout
  • Backchannel Logout

In terms of testing, we are going to have some minimal level of testing on our side too. Differently from here, we are going to run integration tests (running Wildfly).

I should mark the PR ready to review as soon as our testsuite is ready to test Elytron OIDC.

I should also be running some more tests to make sure what is missing (or failing) from recommended security practices for logout.

@pedroigor
Copy link
Contributor Author

pedroigor commented Mar 17, 2023

Regarding RP-Initiated support, the mechanism is going to pass the id_token_hint on every logout request. That means logout endpoints are only accessible if a security context is established.

The following logout parameters are not going to be supported:

  • client_id, not needed as we are passing the id_token_hint parameter
  • logout_hint, Keycloak does not support it.

In terms of configuration, we might need the following options:

  • logout.uri, a relative URI referencing the endpoint that will start a logout (RP-Initiated). Defaults to /logout.
  • logout.callback.uri, a relative URI referencing the endpoint that will handle logout requests and tokens from the OP. I don't think we need separate endpoints for front and back channel but decide on one or another based on the HTTP method (e.g.: GET and POST). Defaults to /logout/callback.
  • logout.session.required, indicates whether the mechanism should enforce both sid and iss parameters when processing frontchannel logout requests. If set to false, there will be no check and any authenticated request is going to invalidate the local user session. Defaults to true.
  • logout.post-logout-uri, a relative URI referencing the endpoint that OPs should redirect users after logging out sessions. Default not set.
  • logout.session.invalidation.limit, the limit to set to a bounded map when marking sessions for invalidation during back-channel logout.

@pedroigor pedroigor force-pushed the logout branch 2 times, most recently from c20dde1 to 05456d7 Compare March 22, 2023 20:20
@pedroigor
Copy link
Contributor Author

@fjuma Marking the PR as ready for review. Please, let me know if you want me to update the commit message with the appropriate issue/jira.

@pedroigor
Copy link
Contributor Author

pedroigor commented Mar 22, 2023

@fjuma W.r.t. to back-channel logout, the solution is the follows:

  • The OP sends a logout request with the logout token
  • Elytron verifies if the configuration is set to validate the sid from the logout token. If not set, back-channel logout won't do anything but fail with a 400.
  • Otherwise, Elytron marks the sid for invalidation by using a bounded map so that subsequent requests from the corresponding user are going to force the session to be invalidated.

The limit of the bounded map can be set through a configuration option. By default, we can store 100 sessions for invalidation.

@ssingh-cls
Copy link

ssingh-cls commented Aug 29, 2023

Do we have this fix in any released version, please? @pedroigor @fjuma @Skyllarr
I am looking to use logout functionality here. Thanks!

@fjuma
Copy link
Contributor

fjuma commented Aug 29, 2023

@ssingh-cls This hasn't been included in a release yet.

@ssingh-cls
Copy link

ssingh-cls commented Aug 29, 2023

@ssingh-cls This hasn't been included in a release yet.

Thank you @fjuma for quick reply. Do you have any plan to include this feature in future release? Also, to clarify to logout from OIDC server for now, do we have any workaround via existing wildfly-elytron release or we have to rely on other options provided by OIDC such as using their SDK API e.g.?

@fjuma
Copy link
Contributor

fjuma commented Aug 30, 2023

@ssingh-cls Yes, we are planning on including this in a future release, please keep an eye on https://issues.redhat.com/browse/ELY-2534 (or this PR) for more updates.

I don't think there are any workarounds for RP-Initiated logout, front-channel logout, and back-channel logout in the meantime.

@ssingh-cls
Copy link

@ssingh-cls Yes, we are planning on including this in a future release, please keep an eye on https://issues.redhat.com/browse/ELY-2534 (or this PR) for more updates.

I don't think there are any workarounds for RP-Initiated logout, front-channel logout, and back-channel logout in the meantime.

Thank you very much @fjuma for providing this update.

@rioy-soptim
Copy link

Greetings,
I've wondered when we can expect this feature to be rolled out? Will this be included in Wildfly 31 or 32 (or later)? @fjuma

@fjuma
Copy link
Contributor

fjuma commented Nov 28, 2023

Hi @rioy-soptim, apologies for the delay. We'd like to return to getting this reviewed and determining if any corresponding attributes are needed in the elytron-oidc-client subsystem as soon as we can. Please watch https://issues.redhat.com/browse/ELY-2534 for updates.

@fjuma
Copy link
Contributor

fjuma commented May 7, 2024

In terms of testing, we are going to have some minimal level of testing on our side too. Differently from here, we are going to run integration tests (running Wildfly).

I should mark the PR ready to review as soon as our testsuite is ready to test Elytron OIDC.

I should also be running some more tests to make sure what is missing (or failing) from recommended security practices for logout.

Hi @pedroigor, thanks again for this PR! I'm finally going to be picking this up and will look at the configuration options that we should add.

Just wanted to ask about the comments about testing in the PR description.

Did you end up working on any tests outside of what's in this PR?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants