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

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

Open
josephdecock opened this issue May 20, 2024 · 1 comment
Open

Comments

@josephdecock
Copy link
Member

josephdecock commented May 20, 2024

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 DuendeSoftware/Support#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
Copy link
Member

AndersAbel commented May 22, 2024

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.

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

No branches or pull requests

2 participants