Skip to content

OIDC Backchannel Logout Support #12570

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.

Sign up for GitHub

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

Jump to bottom
Closed
wants to merge 12 commits into from
Closed

OIDC Backchannel Logout Support #12570

wants to merge 12 commits into from

Conversation

jzheaux
Copy link
Contributor

@jzheaux jzheaux commented Jan 20, 2023 •
edited by jgrandja
Loading

Closes gh-7845

@jzheaux jzheaux added type: enhancement A general enhancement in: oauth2 An issue in OAuth2 modules (oauth2-core, oauth2-client, oauth2-resource-server, oauth2-jose) labels Jan 20, 2023
sjohnr
sjohnr reviewed Feb 15, 2023
View reviewed changes
Copy link
Contributor

@sjohnr sjohnr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is looking good @jzheaux! At the moment, I just have one comment below for discussion.

jgrandja
jgrandja requested changes Feb 27, 2023
View reviewed changes
Copy link
Contributor

@jgrandja jgrandja left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the PR @jzheaux !

Please see review comments.

Also, I would recommend taking a look at the OpenID Connect 1.0 Logout Endpoint (spring-projects/spring-authorization-server@98e3fe8) in Spring Authorization Server and to see how it makes use of SessionRegistry as we want to align the usage in this PR as well.

@jzheaux jzheaux self-assigned this Mar 14, 2023
jgrandja referenced this pull request in jzheaux/spring-security Apr 11, 2023
@jzheaux
Try SessionRegistry
f63a103
@jgrandja jgrandja mentioned this pull request Jun 16, 2023
Closed
@ch4mpy
Copy link
Contributor

ch4mpy commented Jun 16, 2023 •
edited
Loading

I might have overlooked this PR, but it seems entirely focused on sessions when, as I understand the spec, the logout token might contain either a sid or the sub of the user to "logout" (and it is legit for the sid to be missing from the logout token).

Additionally, the user sessions are not necessarily to be closed on all clients notified with the Back-Channel Logout event: I believe that what we need to remove are the authorized-clients (with authorization-code flow) for the emitting OP. The session on a specific client is to be closed only when we remove the last authorized-clients with authorization-code flow. Let's consider a client with authorized clients to an internal OP and one or more external ones (for optional API calls, for instance). If it is pretty convenient to be notified of a logout event on any of this OPs (to stop trying to send requests and adapt the UI), it would also probably be abrupt to end the session on the client, when the user still has a valid session on other OPs.

@jzheaux
Copy link
Contributor Author

jzheaux commented Jun 20, 2023

Hey, @ch4mpy! Thanks for the feedback.

I might have overlooked this PR, but it seems entirely focused on sessions when, as I understand the spec, the logout token might contain either a sid or the sub of the user to "logout" (and it is legit for the sid to be missing from the logout token).

The intent of the PR is to address both scenarios; sub or sid. Please see the InMemoryOidcProviderSessionRegistry which inspects the OidcLogoutToken for either and searches accordingly. Do you see an oversight in it?

Additionally, the user sessions are not necessarily to be closed on all clients notified with the Back-Channel Logout event: I believe that what we need to remove are the authorized-clients (with authorization-code flow) for the emitting OP. The session on a specific client is to be closed only when we remove the last authorized-clients with authorization-code flow.

This is interesting, thanks for bringing it up.

@jgrandja, I'm a little unfamiliar with this use case of having multiple authorized clients tied to a single session. The OAuth2AuthorizedClientRepository seems to imply one authorized client per registration id. Does Spring Security support this use case that @ch4mpy is describing?

@jzheaux jzheaux force-pushed the oidc-session-strategy branch from 7b0cb43 to b75b3df Compare June 21, 2023 00:20
@jgrandja
Copy link
Contributor

jgrandja commented Jun 21, 2023 •
edited
Loading

@ch4mpy

The session on a specific client is to be closed only when we remove the last authorized-clients with authorization-code flow.

This would be custom behaviour and is not detailed in the spec. If I'm missing it, please provide a link to the spec reference.

After a client completes an OIDC flow, it results in an OAuth2AuthorizedClient instance. During the client application session, more OAuth2AuthorizedClient instances may be stored in the AuthorizedClientRepository , for example, after another 2x authorization_code (non-OIDC) flows is completed. This would result in 3x OAuth2AuthorizedClient instances in total during the client session. So what I believe you are stating @ch4mpy is that all 3x OAuth2AuthorizedClient instances should be removed before the session is invalidated. Correct?

jgrandja
jgrandja requested changes Jun 23, 2023
View reviewed changes
Copy link
Contributor

@jgrandja jgrandja left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the updates @jzheaux.

In addition to the review comments, can you also address the following:

  • Update @since 6.1 to @since 6.2
  • There are a few invalid references in @param javadoc
  • Some of the class / method javadoc are either missing or require editing

@ch4mpy
Copy link
Contributor

ch4mpy commented Jun 25, 2023 •
edited
Loading

So what I believe you are stating @ch4mpy is that all 3x OAuth2AuthorizedClient instances should be removed before the session is invalidated. Correct?

@jgrandja no. I mean exactly what I wrote: only the authorized-clients matching the Back-Channel Logout request should be removed (those corresponding to the emitting OP and sid or sub contained in the request). The session should be closed on the client (with remaining non-authorization-code authorized clients) only when we remove the last authorized-client with authorization_code in this session (the user is not logged in anywhere anymore).

@jzheaux yes, it is possible to have a user logged-in on several OP at the same time with spring security. I have a running sample there: https://crs.demo.c4-soft.com/ui/ (you can login as keycloak-user, cognito-user or jwacongne@c4-soft.com depending on the OP, all 3 having p4sSword! as secret). This is a simplistic sample consuming a single API, but in a "real-world" use-case, I use the different identities to call different audiences (different APIs trusting different OPs). The source code is there.

Of course, I had to hack a bit to get things working. I provided some details 3 month ago when opening this ticket (which is still waiting for triage), as:

  • the security-context is designed for a single authentication instance
  • OAuth2AuthenticationToken is designed to store tokens from a single OP (one access token, one ID token and one user subject)
  • default authorized client repository (and service) are designed to retrieve an instance based on registration ID and user principal (but this principal changes with the OP and there is only the last acquired principal in the security-context)

@jzheaux
Copy link
Contributor Author

jzheaux commented Jun 29, 2023

@ch4mpy, I think that #12862 requires enhancements that are out of scope for this PR (for example, the removeAuthorizedClients method).

Otherwise, I'm not clear on how to query OAuth2AuthorizedClientRepository for a set of clients and perform the check that you propose. While I think it's an interesting use case, I don't see yet how this PR could offer general-purpose support for it at this point in time. Do you have any suggestions?

@ch4mpy
Copy link
Contributor

ch4mpy commented Jun 29, 2023 •
edited
Loading

@jzheaux I think there are two different concerns in what I commented:

  • keeping the session alive if there are authorized clients with authorization_code for other OPs than the one issuing the Back Channel Logout request
  • retrieving the right authorized client to remove: the one with the principal (user subject) in the Back Channel Logout request and that is not necessarily the one in the OAuth2AuthenticationToken from the last login.

I agree that the second point is related to gh-12862.

For the first point, iterating over authorized clients should be enough.

Also, this is not directly related, but I don't understand why the repository interface doesn't expose a method to iterate over authorized clients and the InMemory implementation just implement it instead of both defining and implementing (maybe should I open a ticket for that?).

sjohnr
sjohnr suggested changes Jul 7, 2023
View reviewed changes
Copy link
Contributor

@sjohnr sjohnr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the updates, @jzheaux! I've added some additional comments below.

jzheaux added 7 commits July 11, 2023 13:40
@jzheaux
Responding to Feedback
b59be57 
This tries to respond to the feedback that the session registry logic
belongs in the authentication manager. This does have some nice benefits,
for example it appears to remove a couple of classes in the PR.

However, it has the potential downside of the authentication manager
causing side-effects, which I'm not clear on whether our other
authentication managers do. In effect, the sessions get deregistered
from the session registry in order to be supplied to the
BackchannelLogoutAuthentication instance. Does it make sense to tell
a user who is customizing Logout Token authentication that they,
as part of authenticating that token, need to supply the sessions
attributed to that token?

It also has the somewhat uncomfortable consequence of using the session
registry in such a way as to make it tricky to expose it and in the DSL.
That's because the session registry gets applied to multiple beans and
so even if a person is supplying a custom, the coder still needs to
know that any custom session registry set on the authentication manager
still needs to be set on the DSL as well.

I believe this could be a consequence of trying to overuse the
authentication API. I'd advocate for not using AuthenticationManager
to validate the logout token. Instead, we could introduce
LogoutTokenValidator that validates and returns the token. The
filter would take this token and invoke the session registry. Then,
it would formulate the appropriate Authentication to perform
backchannel logout.
@jzheaux
Only invalidate sessions tied to the audience in the logout token
05f6407
@jzheaux
Responding to Feedback
556b242 
- Moved session registry logic into OidcBackChannelLogoutHandler
- Updating naming conventions
- Moved packages
- Adjusted exception handling
- Added audience checks
@jzheaux
Move OidcSessionRegistry Login Configuration to oauth2Login
3e14022
@jzheaux
Add OidcLogoutTokenAuthenticationConverter
5d60876 
- Note that the reason OidcLogoutAuthenticationToken
has a ClientRegistration instance (instead of a reference)
is because OAuth2LoginAuthenticationToken does
@jzheaux
Move OidcLogoutTokenAuthenticationConverter
f236c8d
@jzheaux
Fix Import Order
95960e8
@ch4mpy ch4mpy mentioned this pull request Aug 13, 2023
Closed
@jzheaux jzheaux mentioned this pull request Sep 1, 2023
Open
@jzheaux
Polish
e2530e5 
- JavaDoc
- Improved Logout Error Handling
jzheaux added a commit to jzheaux/spring-security that referenced this pull request Sep 12, 2023
@jzheaux
Add OIDC Back-Channel Logout Support
3a8dd4c 
Closes spring-projectsgh-12570
@andrewkcarter
Copy link

Looking forward to having this support! Thanks @jzheaux

jzheaux added a commit to jzheaux/spring-security that referenced this pull request Sep 16, 2023
@jzheaux
Add OIDC Back-Channel Logout Support
52b4b3f 
Closes spring-projectsgh-12570
@jzheaux jzheaux closed this in cb33fd7 Sep 16, 2023
jzheaux added a commit that referenced this pull request Sep 18, 2023
@jzheaux
Fix Documentation Errors
6b0d822 
Issue gh-12570
sjohnr pushed a commit that referenced this pull request Sep 19, 2023
@jzheaux
Fix Documentation Errors
af6fce4 
Issue gh-12570

(cherry picked from commit 6b0d822)
@lmorocz
Copy link

lmorocz commented Oct 2, 2023

There is a reactive xref in the end of the servlet advanced login documentation.

docs/modules/ROOT/pages/servlet/oauth2/login/advanced.adoc

Then, you can proceed to configure xref:reactive/oauth2/login/logout.adoc[logout]

This should be:

Then, you can proceed to configure xref:servlet/oauth2/login/logout.adoc[logout]

BTW thanks for this great implementation @jzheaux! This is the last piece we needed to replace the deprecated Keycloak Spring Security Adapter. 👍

@jgrandja jgrandja added this to the 6.2.0-M3 milestone Oct 13, 2023
@jgrandja jgrandja added the status: duplicate A duplicate of another issue label Oct 13, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@alarkvell alarkvell alarkvell left review comments

@sjohnr sjohnr sjohnr requested changes

@jgrandja jgrandja jgrandja requested changes

Assignees

@jzheaux jzheaux

Labels
in: oauth2 An issue in OAuth2 modules (oauth2-core, oauth2-client, oauth2-resource-server, oauth2-jose) status: duplicate A duplicate of another issue type: enhancement A general enhancement
Projects
None yet
Milestone
6.2.0-M3
Development

Successfully merging this pull request may close these issues.

Support OpenID Connect Back-Channel Logout
7 participants
@jzheaux @ch4mpy @jgrandja @andrewkcarter @lmorocz @alarkvell @sjohnr