DuendeSoftware / IdentityServer

The most flexible and standards-compliant OpenID Connect and OAuth 2.x framework for ASP.NET Core
https://duendesoftware.com/products/identityserver
Other
1.4k stars 317 forks source link

Consider extensibility point for sub/sid validation during endsession #1555

Open josephdecock opened 1 month ago

josephdecock commented 1 month ago

We could take the existing logic that compares the id token hint's sub to the session's sub and move it into a new virtual method. More flexibility in the end session validation would help with customer customizations, such as https://github.com/DuendeSoftware/Support/issues/1253.

We might need to rework we convey that the user needs to confirm logout, and make that something that could be set from this new method.

One thing that seems interesting is that the current validator is more strict with the case where an id token was passed but is different from the current session than if no token was passed at all.

Another interesting thing to note is that our implementation verifies that the sub claim in the hint matches the sub claim in the session (and has for many years). The OIDC RP initiated logout spec language changed a bit after we implemented it that way, emphasizing the sid instead, and even allows for a sid that isn't the current session as long as it is "recent" (recent is not defined though).

The OP SHOULD accept ID Tokens when the RP identified by the ID Token's aud claim and/or sid claim has a current session or had a recent session at the OP, even when the exp time has passed. If the ID Token's sid claim does not correspond to the RP's current session or a recent session at the OP, the OP SHOULD treat the logout request as suspect, and MAY decline to act upon it.

We might want to switch from comparing sub by default to comparing sid instead, at least for the current session. The idea of "recent" sessions is harder, since we don't have a good way to check if a sid was recently used in a session. Theoretically recent sids could be tracked or the server side session store could do a soft delete.

AndersAbel commented 1 month ago

IMHO new wording is to handle the case where the session has expired, not when it has been replaced by a completely different session. To me those are completely different cases.

If a logout request is received for a recent SID and there is no current session, I think it could make sense to issue logout notifications to any clients in that recent session. This would handle the case where the client sessions are "orphaned" because the coordinating IdSrv session is expired. (I know that we currently loose our book-keeping of clients in the session when the session is lost, that would have to be solved too).

If a logout request is received for a recent SID and the IdSrv has another session active that indicates that the previous session was over-written, possibly without logging out attached clients in the previous session.