Sustainsys / Saml2

Saml2 Authentication services for ASP.NET
Other
960 stars 603 forks source link

Issues with Federated Logout with Kentor/Idsv #421

Closed federicobarera closed 6 years ago

federicobarera commented 8 years ago

Hi @AndersAbel ,

I saw that your guys recently released the federated logout functionality. Thanks for that, great effort. I am currently in the process of deploying IdentityServer in front of our applications, using several SAML Idps (using kentor) There are several problems in the "Federated Logout" area, while having IdentityServer and Kentor working together:

  1. OpenID has the concept of post logout redirect uri. Idsv implement it, but required additional configuration while implementing external idps. Here the link: https://identityserver.github.io/Documentation/docsv2/advanced/federated-post-logout-redirect.html

I tested the flow using 0.17.0 and 0.17.1 (SLO enabled) versions of your dlls and, how indicated by the documentation, the ?id parameter on the logout page is lost after Kentor Middleware kicks in. As I could not find any "Logout" event in the middleware which is invoked before starting any signout logic, I assume it's not possible to wire such functionality in. Can you please confirm?

  1. I tested the "SampleIdentityServer3" sample, having few problems:

Line 192: if (context.Authentication.AuthenticationResponseRevoke != null && context.Response.StatusCode % 100 == 3 && !HttpContext.Current.Response.HeadersWritten) {

I can't get the middleware to fire up the signout cleanup. The !HttpContext part throws an exception as HttpContext is not defined. Even if I am hosting in IIS/Express I am pretty sure that would never work if in SelfHosted (no System.Web). I can why it is used tho, as removing the check will fail the "StatusCode = 200" if headers have already being written.

Thanks,

Federico

AndersAbel commented 8 years ago

Thanks for your feedback.

Post Logout Redirect

The post logout redirect as described in the IdSrv3 documentation doesn't work with AuthServices. Adding a logout event would help when doing SP-initiated logout, but not for Idp-initiated logout. Instead try using the UseIdSrv3LogoutOnFederatedLogout found in the SampleIdentityServer3 project. I've talked to Brock Allen of IdentityServer about the logout flow and if I can find some time I'll look into how to make them work better together. I think that changes are needed in IdSrv3 to get a simple setup of this.

HttpContext.Current

The reference to HttpContext.Current is in the SampleIdentityServer3 project, which is hosted on IIS. For a self hosted application that need to be changed. But if you're hosting on IIS I don't understand why you get null there... It might be due to different usage of await/async further downstream. I'll see if I can make a quick fix to the sample.

AndersAbel commented 8 years ago

I pushed a fix to a branch 421_HttpContextIdSrv3. Any chance you can get that branch, build it and try the SampleIdSrv3 app to see if it works better?

federicobarera commented 8 years ago

Thanks Anders,

I'll try out the fix tomorrow morning (sydney time :) ) and I'll let you know, thanks for the quick reply. I'll open a new ticket to discuss the federated signout flow while in Idsv.

As I'll have to sort out this issue in a relatively short amount of time, I'll give you updates if I find a "relatively clean" way to hook into the logout using a middleware of I'll have to change your code to add an event.

Cheers,

F

AndersAbel commented 8 years ago

Great. The SampleIdentityServer3 application in the repo features two OpenID Connect clients: One JS-based and one server-based. When testing I have got both sign in flow and logout working for those. Logout with both RP-initiated and Idp-initiated logout. But I might still have missed something.

federicobarera commented 8 years ago

Wow. There is some serious magic going on there. Debugging the entire flow through idsv and kentor took a while... Alright, let's start with the good news.

I can confirm that retrieving the HttpContextBase from the Owin Environment fixed the bug. There is still the question mark about using HttpContext at all in an Owin Environment, but as a demo application I think it is fair enough.

There is a caveat tho. The Isdv federated signout (triggered by /signoutcleanup) works only when the logout is idp initiated. If that is the intended behavior no problem. If not, unfortunately that anonymous middleware you register at the very end it's not going to work. At that stage the headers have already been written and none of the status / content changes are going to be actioned by the browser.

This is either true when the 303 response is going out to the idp to logout or when the logout saml response is processed and the LogoutCommand is creating a 303 to Idsv logout page (via the cookie value)

Similarly I also couldn't find any vaguely clean approach to initiate the post logout redirect in the anonymous middleware at the bottom. I ended up adding a PostLogoutRedirect Property in the SPOptions and slightly modify the LogoutCommand.

Tomorrow i'll fork and start a new discussion ticket on it.

Cheers,

F

AndersAbel commented 8 years ago

Yes, there is a lot of stuff going on under the hood. My design goals have been to make it as easy as possible to use, but at the same time not make the middleware IdentityServer3-specific. Even though it is used together with IdSrv3 in many cases, it is also used without it.

The anonymous middleware is only called with Idp-initiated logout. Thats's because the redirect flow of the user's browser is SAML2 Idp->IdSrv3->Saml2 Idp. While at the IdSrv3, the logout view must be rendered and hence the hack with the middleware. For SP/RP-initiated logout the flow is OpenID Connect RP->IdSrv3->Saml2 Idp->IdSrv3(->OpenID Connect RP). In that case the IdSrv3 logout view is rendered after coming back from the Saml2 Idp. That works in the Sample IdSrv3 application in the repo. But the sign out parameter mentioned in the documentation is probably lost.

There are plans to update the logout flow of IdSrv3 to use a return url mechanism through AuthenticationProperties to make the SP/RP-initiated flow easier to implement, see IdentityServer/IdentityServer3#2613. For Idp-initiated logout something similar to my middleware is probably needed in IdSrv3.

parkinsona commented 8 years ago

Hi @AndersAbel ,

I have got the latest version of idsvr3 (v2.5) that includes the change for 2613. What do I need to do to enable this logout redirect to appear?

In my startup Identity AuthenticationOptions I've added:

EnableAutoCallbackForFederatedSignout = true,

Is there something else I need to do in the SAML config? In your idsvr SAMPL, you add in a UseIdSrv3LogoutOnFederatedLogout function. Is this still required with v2.5 of identity server?

federicobarera commented 8 years ago

Hi @parkinsona ,

I wasn't aware Identity Server implemented that functionality. Thanks for flagging. I had to "workaround" this issue, so in case this won't be compatible with KentorIT follow the example here: https://github.com/KentorIT/authservices/pull/424

From brokallen's commit, seems that it's just necessary to enable the flag their end. Remember anyway that during login you have to save the id_token generated by identity server, and send it back as IdTokenHint. Otherwise, for security reason, the redirect won't be triggered.

There is an article here with sample at the bottom:

https://leastprivilege.com/2014/10/14/identityserver-v3-and-post-logout-redirect/

parkinsona commented 8 years ago

I have tried just enabling the flag on the idsvr end, but it hasn't worked yet. I've got the id_token passed through as this works for my non-saml clients.

I wonder if there is something else I need to setup.

federicobarera commented 8 years ago

Oh ok. The last time I checked, Kentor saves the redirect url in a cookie, which is used after the federated logout happens, to redirect the user where they were coming from.

When you do federated logout with non-saml identity providers, do you get redirected to the identity server logout page (/logout) with a ?id parameter?

parkinsona commented 8 years ago

Yes, for non-saml identity providers I get redirected to /logout?id=xxxxx

For saml providers, it only redirects to /logout

AndersAbel commented 8 years ago

Good morning, refreshing to find a great discussion going on.

The SampleIdentityServer application is upgraded to IdSrv 2.5 and tested. The EnableAutoCallbackForFederatedSignout works with AuthServices 0.17.2 for SP-initiated signout.

@parkinsona Are you sure that you do get a SAML2 federated logout and not only a local logout in IdSrv3? Check the SAML2 metadata at http://yourhost/idsrvpath/AuthServices. Does it contain SingleLogout endpoints? If not, you've not configured AuthServices correctly for single logout. Note that a signing certificate is needed for SLO to be enabled.

For idp-initiated signout the hack with UseIdSrv3LogoutOnFederatedLogout is still needed, otherwise the RPs authenticated to IdSrv3 won't be signed out.

parkinsona commented 8 years ago

Hi Anders,

Thanks for the input. All I'm after is to having the postlogout redirect url displayed in identity server on the logout screen. At this stage, I'm not too worried if I don't log out of the SAML2 idp, I only want to log out of Identity Server. Do I still need a SingleLogoutService in my metadata for idsvr to pick up my logout redirect?

One of my providers has a SingleLogoutService, but the other doesn't. The one that has it, doesn't have a proper dev environment, so I'll have to look into adding it into my Demo Okta Provider to see if that will make the difference.

AndersAbel commented 8 years ago

@parkinsona If you're not using the single logout feature of AuthServices and only want to do a logout on IdSrv3 you're not affected by this at all. I think that IdSrv3 handles the signout id internally in that case and never shows it as query string param.

parkinsona commented 8 years ago

That's right, just a logout on idsvr3. Perhaps I should check on the idsvr issue?

AndersAbel commented 8 years ago

@parkinsona Yes, If you're not using single logout in SAML2 AuthServices is not involved in the signout sequence.

AndersAbel commented 6 years ago

Looks old and no longer relevant, closing.