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

support OpenID Connect Front Channel Logout #321

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Conversation

bodewig
Copy link
Collaborator

@bodewig bodewig commented Mar 27, 2020

this adds a function that can be used as lua content handler for a frontchannel logout URI.

see also #308

@bodewig bodewig requested a review from zandbelt March 27, 2020 16:42
@bodewig bodewig changed the title suport OpenID Connect Front Channel Logout support OpenID Connect Front Channel Logout Mar 28, 2020
@eriksunsol
Copy link

I'm interested in this as well, so I took some time to investigate...

My understanding is that the whole point of the front channel logout mechanism is that it enables the OP to ask the RP to terminate all active sessions for a user, which is why the sid argument can be included in the request. According to the front channel logout specification section 3, the OP renders one <iframe> tag for each RP which is currently logged in with their respective logout URIs. If I'm right about this, the sid argument should not be verified against the id of the current session, but instead the session corresponding to the sid argument should be looked up and destroyed.

I'm playing around with AzureAD which supports front channel logout, and in their case (not saying that it is normative) they also support section 3 of the section management specification and provide the session_state parameter to the redirect callback. Their interpretation is that the sid argument passed to the logout URI matches the session_state provided to the redirect callback. The specs are a bit vague so I suppose there may be other interpretations out there.

So to make it work with AzureAD's interpretation of sid, you would have to maintain a mapping from session_state as received with the redirect callback, to the session, and if sid is provided with the logout request it should be used to find the corresponding session to be destroyed. I haven't wrapped my head around the best way to maintain this mapping.

@bodewig
Copy link
Collaborator Author

bodewig commented Apr 29, 2020

I'm afraid such a mapping depends on how the session is stored. Also I don't think you can look up a session by its id in lua-resty-session, so you cannot destroy any session other than "the current one". Also, there always is only one session attached to a single browser instance (modulo anonymous mode) as at least the session id is stored in a single session cookie.

@eriksunsol
Copy link

eriksunsol commented Apr 29, 2020

If the session is stored entirely in the cookie, then obviously it is impossible to destroy any other session than the one associated with the browser which is executing the requests, but for the other storage adapters it should be possible.

I have looked through the source code of lua-resty-session and currently exploring the idea to create either a custom session strategy, or a storage adapter which wraps one of the existing ones (except cookie) to maintain the session_state mapping and provide a method to wipe all sessions associated with a session_state value. Note that especially when using the regenerate session strategy it is likely that there is going to be many sessions in the store associated with a single session_state value.

@bodewig
Copy link
Collaborator Author

bodewig commented Apr 30, 2020

I've thought about a similar approach as well - a wrapper around existing session strategies. This would allow us to support backchannel logout as well.

@eriksunsol
Copy link

eriksunsol commented Apr 30, 2020

I have a prototype strategy going which which works in my tests with shm storage, but should work also with e.g. redis or any of the other non-cookie adapters. Actually it's just a small change to make it work with cookie too. I decided to re-use the storage adapter already configured for the sessions to store a list of revoked sessions. The strategy has an extended API which allows you to

  1. Indicate that a callback (redirect_uri) from the OP is being processed and provide the iss and sid arguments for future revocation. These will be stored in the session.
  2. Revoke the session by providing the iss and sid arguments for sessions to be added to the revocation list.

The strategy wraps/inherits another strategy and overrides save() to store the identifier for revocation, and open() to check the revocation list and set session.data to an empty object if the session has been revoked. Ideally only the data owned by openidc, or relevant parts of it, should be cleared, so there I have some more work to do.
In order to make it work with cookie storage all I need to do is add a configuration option to allow specifying a different storage adapter for the revocation list.

If I can get this to work reliably, extending the concept to support back-channel logout would be easy. There is one aspect of the back-channel logout draft which is problematic though. In a back-channel logout request either a sub or a sid claim may be included. If the sid claim is included the request can be processed similar to a front-channel logout, but if there is just a sub claim the revocation list approach is not going to work, and an RP which is also an OP would be unable to reliably propagate the request to downstream RPs. In all other aspects the back-channel draft seems more robust than the front-channel draft and in all honesty I fail to see why they are so different at all. I have posted my suggestions to improve the drafts here.

Edit: A closer look at the back-channel logout spec: the logout_token of the back-channel request may contain either a sid claim or a sub claim or both. If a sid claim is present it can be processed the same as a front-channel logout. If only the sub claim is present the spec states that it shall be used to identify the associated sessions. I my mind this is unreliable and can lead to undesired results, especially if propagation to downstream systems fails or is delayed, which is however dealt with (sort of) in the spec by requiring that the response code is set accordingly. Some of the undesirable effects could be mitigated by using the iat claim which is required to be present in the logout_token e.g. to leave alone sessions which were started after the logout request was posted. On the other hand, at least with dynamic client registration, RPs can indicate to the OP that the sid claim is required to be present.

Realizing that I may be hijacking this PR for a broader discussion, please advise if there is a more appropriate place to discuss this.

@eriksunsol
Copy link

@bodewig I have made a proposal for a revocable session strategy available as PR #330. You are welcome to incorporate it here if you like. You have already put some effort in tests, etc, and I see no reason to duplicate all that work.

see #308

Signed-off-by: Stefan Bodewig <stefan.bodewig@innoq.com>
@zandbelt
Copy link
Contributor

@bodewig do you want me to consider this pull request before releasing, or should I release 1.7.3 as master is right now

@bodewig
Copy link
Collaborator Author

bodewig commented Sep 10, 2020

Unfortunately my weekends have been devoted to other things lately,

I've been meaning to come back to this to verify @eriksunsol 's comment that I may have misunderstood the spec. In that case merging this PR wouldn't be a good idea, I'm not sure when I will find time to re-read the spec (and my implementation, it's been a while). Let's not hold up the next release and rather be quick with another one once we've sorted things out.

@thorstenfleischmann
Copy link
Contributor

I implemented front-channel logout with my IdP using OIDC Session Management and the end-session-endpoint. I had to extend "on_authenticated" to save the session_state to the session, so I can use it later (at the front end)

    lifecycle = { 
      on_authenticated = function (session)
        -- save session_state to session (for front channel logout)
        session.data.session_state = uri_args.session_state
      end

The logout is initiated from the client by calling logout with "Accept: image/png" after receiving a "changed" notification. This may not be suitable for everyone but served my needs.

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

Successfully merging this pull request may close these issues.

None yet

4 participants