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

A few issues with OIDC logout flow #831

Closed
aliaksander-samuseu opened this Issue Jun 22, 2018 · 4 comments

Comments

Projects
None yet
4 participants
@aliaksander-samuseu
Contributor

aliaksander-samuseu commented Jun 22, 2018

Environment:
CentOS 7.2, gluu-server--3.1.3-1-4.centos7

Per @qbert2k request I'm posting my report on issues with OIDC logout flow

Preamble:

According to the OIDC session management spec, id_token_hint is recommended parameter (not a mandatory), and post_logout_redirect_uri and session_id are optional.

Findings:

1) Issues with id_token_hint and session_id parameters used in logout request

Current version of oxAuth module is not allowed to process logout request without any parameters included at all. This is due to this code called from this point which ensures that either id_token_hint or session_id is passed. If neither were, it will return "invalid grant" type of error and session won't be killed.

The curious part is that, technically, oxAuth doesn't need either of them to end a session. This can be verified by passing it totally bogus hint (to circumvent the check from above), and no session_id, or other parameters at all - it will throw "post_logout_uri_not_passed" error (what is curious on itself, more on this later), yet oxAuth's session will be killed anyway. This is due to the fact that oxAuth seems to be capable to get all data it needs for session termination using just cookies sent in logout request alone. Here is, briefly, course of execution for this particular case:

  • Start here

  • Pass this check as id_token_hint is just invalid, not absent

  • Enter endSession() function here

  • Pass through this series of checks with "null" results, as no such id_token can be found

  • Enter removeSessionId() function here

  • Follow this branch as we don't pass "session_id" to /end_session either and retrieve session id from cookies in request here

  • With ldapSessionId it gets as a result of above steps, it then seems to be able to complete all usual clean up steps

Thus reason behind those initial checks is not very clear. At best, they seem redundant, and lead to different processing of technically similar requests (is request with invalid id_token that much different from request from which it's just absent? current code doesn't even check it for validity, it just needs not be empty, it seems)

2) Issue with post_logout_redirect_uri handling

In case when an invalid token (a random garbage or a stale token which was already cleaned from Gluu's caches) passed as id_token_hint, but post_logout_redirect_uri is not passed, session is killed, but user still is sent to an error page with "POST_LOGOUT_URI_NOT_PASSED" message and is stuck there.

Course of execution in this case is like this.

  • Steps listed in previous chapter are executed, session data is cleaned from LDAP and cache

  • httpBased() function is entered here

  • This check takes place; as authorizationGrant at this point is "null" (due to the fact invalid/stale id_token wasn't found anywhere, as we assumed initially), it calls one specific version of validatePostLogoutRedirectUri() function, this one. Out of those 2 versions, it's the only one which checks for presence of post_logout_redirect_uri and throws "POST_LOGOUT_URI_NOT_PASSED" if it's empty. This is unnecessary ambiguous on itself - there is hardly a reason to check for it only in one of those functions (that's on top of the fact it makes optional (by spec) parameter mandatory)

Questions:

  • As oxAuth is capable to terminate session without any (non-mandatory by the spec) items provided to it, why do we still need to ensure that some of them were passed? One reason is probably the fact it may be more efficient in terms of search overhead to pass id_token as hint, as from what I see, in latest packages id_tokens are by default stored in memory cache (but not other kind tokens, so my guess it's because of the need to efficiently search for them during logouts). But even in such case, do we realy need to block it in code in such way, when simply sending a random string instead of id_token still is enough to circumvent this block? It should be either allowed, or not, not some middle-ground, it looks confusing. Currently it will log you out ok if you'll send non-valid token (still with an misleading error message, see below), but won't if you'll send none. Technically, there is little difference between those 2 cases, yet they are handled completely different.

  • Post-logout URI shouldn't be a mandatory one in any way, as it isn't by the spec. It actually isn't, in certain cases, and the fact in others it is seems to be (according to Javier) due to dubious additions to the code done in the past. This as well may add to confusion and should be streamlined. For example, in case a stale (i.e. it was long removed from the cache and cannot be looked up) or truncated (with part of signature lost due to some bug, for example) id_token is passed as hint, it currently (due to reasons explained above) throws misguding error message about missing post_logout_redirect_uri (which isn't even mandatory item), while still silently killing the session. If a valid id_token is passed, still without post_logout_redirect_uri, there is no error, yet. This is very confusing behaviour.

Some ideas:

  • We could decide to stop enforcing presence of any parameters in logout request in code as oxAuth doesn't really need them; may prove itself useful for RPs which can't use parameters in such request, or want to employ simpler flows

  • Instead, in OIDC logout docs we would emphasize importance of sending valid id_token_hint when possible, to save server's CPU cycles (assuming it's stored in memory cache to speed up searches)

  • Current strange handling of post_logout_redirect_uri should be decoupled from handling of id_token_hint, the former must stay optional regardless of what branch of code is used. The correct error message must be logged in case id_token passed as hint is not found/invalid (currently it makes user think that absence of post_logout_redirect_uri is the real problem). Users shouldn't find themselves stuck at some error page, unless it's really needed.

@yuriyz

This comment has been minimized.

Contributor

yuriyz commented Jun 25, 2018

@nynymike @aliaksander-samuseu @qbert2k

1. post_logout_redirect_uri
post_logout_redirect_uri is not mandatory but if it is provided then we must validate it. From spec The value MUST have been previously registered with the OP. It means that we have kind of dual behavior description directly in specification:

a) case A - post_logout_redirect_uri is not provided, then we just end session and we are done.
b) case B - post_logout_redirect_uri is provided then we MUST validate it because it is says by in spec.

Confusion is in case B. Should we end the session or not if post_logout_redirect_uri fails validation? If we say no, then it conflicts with case A, because if do not provide it at all then we end session! I propose to provide better error message for case B, like : Session is ended successfully but redirect to post logout redirect uri is not performed because it fails validation.

**2. id_token_hint and session_id. **

We have session reference in cookie. We don't really need id_token_hint and session_id parameters. But from other side if there is call with some trash inside id_token_hint, what should we do ?

case A - reject the call. It looks reasonable but then what should happen if we do not provide id_token_hint at all (case B) ? According to spec we have to end the session because it is not mandatory.
I already see many support tickets on this topic :). I propose in case A do NOT end the session and return error and in case B end the session successfully.

@nynymike

This comment has been minimized.

Contributor

nynymike commented Jun 25, 2018

Agreed on points 1 and 2. We need to update the docs so we can point to it when people inevitably ask questions.

@yuriyz

This comment has been minimized.

Contributor

yuriyz commented Sep 4, 2018

@qbert2k I will take over this if you don't mind.

yuriyz added a commit that referenced this issue Sep 5, 2018

#831 (4.0.0) : fixed /end_session endpoint with proper validation and…
… info/error messages.

#831

(cherry picked from commit e4cee99)

yuriyz added a commit that referenced this issue Sep 5, 2018

yuriyz added a commit that referenced this issue Sep 5, 2018

yuriyz added a commit that referenced this issue Sep 5, 2018

@yuriyz

This comment has been minimized.

Contributor

yuriyz commented Sep 7, 2018

done in 3.1.4 and master. @aliaksander-samuseu please test it on 3.1.4 RC3 which hopefully will be built today.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment