DuendeSoftware / Support

Support for Duende Software products
20 stars 0 forks source link

EndSession w/Invalid Hint Drops All Params When Redirecting To Logout Controller #1253

Closed jdlegan closed 3 months ago

jdlegan commented 3 months ago

We have a need for the processing of a signout, when the session is no longer valid at the IdP to pass values beyond that of logoutid to the redirect, which in our case is a controller action /account/logout.

We have attempted to do so via a few methods, including adding params to the protocol message, manipulating the post-logout redirect URI, etc. but to no avail. It appears that in the absence of a valid session, everything is dropped.

We achieved this in our fork of IdentityService v3 quite easily but there does not appear to be any extensibility points to modify the behavior of /endsession.

To provide context, though it does not matter how the hint lookup fails, our IdP, built largely on IdentityServer 7 provides auth services against a multi-tenanted platform of apps. While many of the apps can resolve their own tenant through other means, certain apps share a single URL but are tenantized via the claims principal (once logged in) and during login/logout, prior to the claims principal being available or when the endsession cannot form a proper logout context, a query string. Due to the limitation we are seeing, without the post-logout redirect URI or the additional params being added to the protocol message being passed to the redirect, we lose the ability to then tenantize the page or offer a login link to log back in again.

Short of introducing middleware to intercept the call to /endsession and get creative on how to ensure the necessary values a persisted through the redirect, how can we achieve this?

josephdecock commented 3 months ago

You can pass custom parameters to the endsession endpoint and access those in the logout UI, but to get at them you either have to have an active session at IdentityServer or pass a valid id_token_hint parameter - essentially have the client persist their id token and send it back during the logout process.

The parameters to the endsession endpoint get passed to the logout UI using the abstraction of the IMessageStore, which by default writes the parameters into a data protected cookie. However, that cookie only gets written if the validation process successfully validates the logout request and determines the client id from either the id_token_hint or the active session.

Can you have your client applications send the id_token_hint? If they're ASP.NET (.NET core) clients, you can just set SaveTokens = true in the OIDC authentication handler. The .NET Framework OIDC owin middleware requires a little bit of customization to do the same thing, which we show in our sample here.

jdlegan commented 3 months ago

@josephdecock

The issue is not on the client side. It appears to be due to the fact that the ODIC spec has since changed and the id_token_hint is no longer required (but recommended) and Identity Server is, as you stated, preventing the passing of any params if the id_token_hint validation fails.

It is not a matter of the client passing it or not, in our case it is. However we have a multi-tenanted environment whereby a user could be a consumer of multiple different brands (tenants). The logins do not support SSO, rather the users have individual accounts within one or more given tenants that do not even necessarily represent the same app.

To IdP does not have unique subdomains or vanity domains, but rather uses a common URL (think auth.commondomain.com) with the clients segregating themselves by subdomain or vanity domain.

If a user logs into one such app, ASP.NET Identity Core persists that session in a cookie at the IdP, token issuance, etc works great. However, if that user then logs into another tenant, who will also see a fully branded auth experience, that cookie at the IdP is lost when the second session auths to the other tenant. Again, all auth works great, refresh tokens, etc, during the life of the logins. However when the first session goes to logout (with the hint that is otherwise valid), its cookie is no longer viable at the IdP, validation fails, we are unable to display the proper tenantized logout screen or provide them a generic login screen for that tenant because none of the params are passed.

I understand that you guys may have made this choice to protect users from not validating the post logout redirect URL and providing an attack surface that a user that is unfamiliar with OIDC may open by blindly passing the params regardless of validation and having to make sure everyone calls a guard or “GetContext” type call, but this lack of extensibility required us to do the following:

We introduced a middleware which runs before /endession is hit, we parse the non-security related elements out of the params we are already sending over but which were not available to us in the event that the hint fails validation (client ID, tenant) and persists it into a cookie. If we get into our logout controller and we are unable to obtain a context from the hint, we fall back to reading those values out of the cookie, allowing us to properly tenantize the logged out page (tenant == brand, ClientID == app), and providing a generic “log back in” button which, using those same two values, is a link from our own URL builder which builds tenantized links.

I appreciate the post_logout_url not being passed for valid security reasons, but the lack of passing custom values is an unnecessary constraint to place on implementers and in this case necessitated the introduction of additional middleware that runs only on /endsession to work around it.

josephdecock commented 3 months ago

Thanks for these details! We could consider adding more extensibility points to make this easier in the future. We'd just want to be careful as well of some of the security issues you're pointing out.

I'm curious to understand which part of validation was failing. When it did, were you getting any log messages - perhaps "Current user does not match identity token"?

jdlegan commented 3 months ago

@josephdecock

Precisely. I just pulled it from Seq. End session request validation failure: Current user does not match the identity token.

josephdecock commented 3 months ago

Okay, thanks. I've created an issue (see above) to track our thoughts on this. In a future release, we can consider adding a virtual method to the end session validator that would allow you to describe the expected relationship between the id token and session. There's also some details in the final version of the RP initiated logout spec that might motivate a change to our default behavior, and it potentially cuts across into other features like server side sessions. We want to avoid a breaking change, and it sounds like you've got a middleware based solution that works for you, so we probably won't be looking at this in the very short term. Does that sound good?

jdlegan commented 3 months ago

@josephdecock,

First and foremost, yes, we are technically solved at this point. We have some slight race condition exposure but the risk and impact is low. Some further thoughts I had as a I peeled this particular onion before arriving at the middleware after @brockallen asked me to drop this message here during an email exchange.

I spent a lot of time in MembershipReboot and IdvSrv3 and and we solved this in a way that involved our server side session implementation in our current IdP. Our new IdP (this project and the enterprise license we are about to execute on before we go into production testing) is based, in part, on v7. As I was going through it all it, I was interested to see that there is a slight disconnect, possibly out of your control, whereby the lookup, which may require additional elements persisted in the server side session that are no there today, to either directly perform the lookup in the server side session store or fallback in the event that the cookie failed, when server side sessions are enabled. This would in theory solve the issue as long as the session was still active and an easy way to address the limitation of "one" session in cookie persistence under a shared domain model.

josephdecock commented 3 months ago

Okay, interesting to think about ... I'll close this issue and track further developments in https://github.com/DuendeSoftware/IdentityServer/issues/1555.