lua-resty-openidc icon indicating copy to clipboard operation
lua-resty-openidc copied to clipboard

support OpenID Connect Front Channel Logout

Open bodewig opened this issue 4 years ago • 10 comments

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

see also #308

bodewig avatar Mar 27 '20 16:03 bodewig

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.

eriksunsol avatar Apr 29 '20 08:04 eriksunsol

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.

bodewig avatar Apr 29 '20 09:04 bodewig

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.

eriksunsol avatar Apr 29 '20 13:04 eriksunsol

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.

bodewig avatar Apr 30 '20 07:04 bodewig

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 avatar Apr 30 '20 21:04 eriksunsol

@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.

eriksunsol avatar May 06 '20 06:05 eriksunsol

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

zandbelt avatar Sep 10 '20 13:09 zandbelt

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.

bodewig avatar Sep 10 '20 14:09 bodewig

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.

thorstenfleischmann avatar Mar 02 '21 08:03 thorstenfleischmann