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

LogoutHandler doesn't handle expired sessions #10

Open
tafs7 opened this issue Nov 10, 2016 · 5 comments
Open

LogoutHandler doesn't handle expired sessions #10

tafs7 opened this issue Nov 10, 2016 · 5 comments

Comments

@tafs7
Copy link

tafs7 commented Nov 10, 2016

I am randomly getting this unhandled exception, SAML2.Saml20Exception: Unknown identity provider, when my users try to log out after having previously logged in. Here's the Stack Trace:

SAML2.Saml20Exception: Unknown identity provider ""
   at SAML2.Protocol.Saml20LogoutHandler.Handle(HttpContext context)
   at SAML2.Protocol.Saml20AbstractEndpointHandler.ProcessRequest(HttpContext context)
   at SAML2.Protocol.Saml20LogoutHandler.Handle(HttpContext context)
   at SAML2.Protocol.Saml20AbstractEndpointHandler.ProcessRequest(HttpContext context)

For the "logout" link, I am simply setting the HREF to the /Logout.ashx URL, which goes through the SAML2 handler and then redirects to the configured logout url (see config below).

I cannot seem to reproduce this and was hoping you could shine a light on where I can start looking to debug this issue.

Not sure what all additional information could be helpful, but here's my web.config showing the forms and the SAML configuration:

 <system.web>
    <customErrors mode="Off" />
    <compilation debug="true" targetFramework="4.5" />
    <httpRuntime targetFramework="4.5" />
    <authentication mode="Forms">
      <forms loginUrl="/Login.ashx" />
    </authentication>
  </system.web>

<system.webServer>
    <handlers>
      <!-- SAML2 lib handlers -->
      <remove name="SAML2.Protocol.Saml20SignonHandler" />
      <remove name="SAML2.Protocol.Saml20LogoutHandler" />
      <remove name="SAML2.Protocol.Saml20MetadataHandler" />
      <add name="SAML2.Protocol.Saml20SignonHandler" verb="*" path="Login.ashx" type="SAML2.Protocol.Saml20SignonHandler, SAML2" />
      <add name="SAML2.Protocol.Saml20LogoutHandler" verb="*" path="Logout.ashx" type="SAML2.Protocol.Saml20LogoutHandler, SAML2" />
      <add name="SAML2.Protocol.Saml20MetadataHandler" verb="*" path="Metadata.ashx" type="SAML2.Protocol.Saml20MetadataHandler, SAML2" />
      
      <remove name="ExtensionlessUrlHandler-Integrated-4.0" />
      <remove name="OPTIONSVerbHandler" />
      <remove name="TRACEVerbHandler" />
      <add name="ExtensionlessUrlHandler-Integrated-4.0" path="*." verb="*" type="System.Web.Handlers.TransferRequestHandler" preCondition="integratedMode,runtimeVersionv4.0" />
    </handlers>
  </system.webServer>

<saml2>
    <serviceProvider id="urn:Foo" server="https://foo/">
      <signingCertificate findValue="[redacted]" storeLocation="LocalMachine" storeName="My" x509FindType="FindByThumbprint" />
      <endpoints>
        <endpoint localPath="Login.ashx" type="SignOn" redirectUrl="/" />
        <endpoint localPath="Logout.ashx" type="Logout" redirectUrl="/" />
        <endpoint localPath="Metadata.ashx" type="Metadata" />
      </endpoints>
    </serviceProvider>
    <identityProviders metadata="Metadata/Dev">
      <add id="foo">
        <endpoints>
          <endpoint type="SignOn" url="https://foo/login.aspx" binding="Redirect" />
          <endpoint type="Logout" url="https://foo/logout.aspx" binding="Redirect" />
        </endpoints>
      </add>
    </identityProviders>
    <logging loggingFactory="SAML2.Logging.Log4Net.Log4NetLoggerFactory, SAML2.Logging.Log4Net" />
  </saml2>

I think I may have tracked this down to the user having had their ASP.NET Session expired on the server, and when the SAML2 Logout handler tries to access session data to find the identity provider, it is no longer available.

Not sure what the best way to handle this behavior more elegantly for my users. Should I provide a "shim" logout action that check session before redirecting to the Logout.ashx handler?

Any other thoughts?

@i8beef
Copy link
Owner

i8beef commented Nov 11, 2016

The logout code was inherited and I think it needs some work. However, I don't have an IDP anymore to test with, so frankly I am probably not qualified to make sweeping changes like what I need to be made here... but I do accept pull requests from people that are!

That said, just to document the problem:

The logout handler is dependent in several places on a valid session. It uses this to grab the name of the IDP the user was originally authorized against, so it knows where to send the logout request / response (depending on if its IDP initiated or not, there's several different flows possible here, so its a LITTLE complex, but not overly so). In the simple case (direct Logout.ashx browser redirect, no SAML request), a simple session check and short circuit could be viable. There's even a TODO in this area pointing to something better being possible than the exception being thrown here (e.g., do the local logout just in case, but don't bother sending an IDP logout request), just probably needs validating that that is kosher (though I wouldn't see why it wouldn't be: the session is already dead).

The harder cases are the IDP requested ones. There are two possible explanations for why it does what it does here:

  1. Lazy approach to getting the IDP metadata for validating signatures on request methods (e.g., instead one could parse the message FIRST, and then grab the IDP metadata for the issuer on the request instead, eliminating the need for the session call). Possible issue being encryption? Needs exploration.
  2. Security. Relying on the request coming in could open up a security hole (e.g., yeah, I'm totally legit, send something to this address please). Relying on the cached, validated logout endpoint is probably SAFER, however because you'd only be reliant on the IDP issuer name to lookup a known good value in cached IDP metadata anyway, that may not be a concern. Have to think this one through and see if there's a hole I'm not seeing for why this was originally written this way.

I think I could probably make some changes here that would be semi-safe refactoring. I'll play with it and see what I think.

In the mean time, to your original question:

If you are dealing with an IDP initiated logout flow, you might be a little SOL here. If you just have a link to Logout.ashx somewhere that people are clicking on to get to this, then you could absolutely shim your own endpoint in that they bounce through that adds this session check. In the case their session is already dead, just move on and redirect them somewhere else, otherwise redirect them to the Logout.ashx endpoint. Fairly simple little hack, if not ideal.

@i8beef i8beef changed the title SAML2.Saml20Exception: Unknown identity provider LogoutHandler doesn't handle expired sessions Nov 11, 2016
i8beef added a commit that referenced this issue Nov 11, 2016
@i8beef
Copy link
Owner

i8beef commented Nov 11, 2016

I made a branch with the changes that I THINK should be made here to make it better. It would need to be tested before merging back in though, and needs security review to make sure the changes around relying on request identifier for getting IDP metadata doesn't open up any unintended security issues.

@HDenBreejen
Copy link

I encountered the same problem. The SAML2 package relies on the ASP Session to hold some state that is required during logout.

I have had some success duplicating this state in the ASP authentication token, using a custom IAction.

`
namespace UsingSaml2Package.Authentication
{
///


/// Modelleert enkele IDP-sessiegegevens
/// IDP is de Identity Provider, zoals DigiD.
///

public class IDPSession
{
public string IDP { get; set; }
public string SessionID { get; set; }
public string NameIdFormat { get; set; }
}

/// <summary>
/// Voegt IDP-sessieinformatie toe aan het bestaande ASP authenticatieticket.
/// Op basis daarvan kan de IDP-sessie later correct worden beeindigd.
/// Deze Action moet dus draaien nadat de ASP authenticatiecookie is gecreeerd.
/// Direct erna ligt voor de hand. Je regelt dat in de config.
/// </summary>
public class ExtendAuthCookieAction : SAML2.Actions.IAction
{
    public string Name { get => nameof(ExtendAuthCookieAction); set => throw new NotImplementedException(); }

    /// <summary>
    /// Voegt IDP-sessieinformatie toe aan het bestaande ASP authenticatieticket.
    /// Op basis daarvan kan de IDP-sessie later correct worden beeindigd.
    /// </summary>
    /// <param name="handler">The handler initiating the call.</param>
    /// <param name="context">The current http context.</param>
    /// <param name="assertion">The SAML assertion of the currently logged in user.</param>
    public void SignOnAction(AbstractEndpointHandler handler, HttpContext context, Saml20Assertion assertion)
    {
        // het huidige authenticatiecookie ophalen
        var formsAuthCookie = context.Request.Cookies[FormsAuthentication.FormsCookieName];

        // Dat moet er zijn, want dat is in een vorige Action zojuist aangemaakt.
        if (formsAuthCookie == null)
            throw new InvalidOperationException("Geen authenticatiecookie");

        var formsAuthTicket = FormsAuthentication.Decrypt(formsAuthCookie.Value);

        var iDPSession = new IDPSession {
                                IDP = assertion.Issuer,
                                SessionID = assertion.SessionIndex,
                                NameIdFormat = assertion.Subject.Format,
                            };

        // Nieuw cookie maken dat gelijk is aan het bestaande authenticatiecookie, maar voeg
        // onze IDPSession als userdata toe.  
        var newFormsAuthTicket = new FormsAuthenticationTicket(
                                            formsAuthTicket.Version,
                                            formsAuthTicket.Name,
                                            formsAuthTicket.IssueDate,
                                            formsAuthTicket.Expiration,
                                            formsAuthTicket.IsPersistent,
                                            JsonConvert.SerializeObject(iDPSession),
                                            formsAuthTicket.CookiePath);

        var encryptedNewFormsAuthTicket = FormsAuthentication.Encrypt(newFormsAuthTicket);
        formsAuthCookie.Value = encryptedNewFormsAuthTicket;

        // Huidige cookie vervangen door de nieuwe.
        context.Request.Cookies.Remove(FormsAuthentication.FormsCookieName);
        context.Request.Cookies.Add(formsAuthCookie);
    }

    /// <summary>
    /// Action performed during logout.
    /// </summary>
    /// <param name="handler">The handler.</param>
    /// <param name="context">The context.</param>
    /// <param name="idpInitiated">During IdP initiated logout some actions such as redirecting should not be performed</param>
    public void LogoutAction(AbstractEndpointHandler handler, HttpContext context, bool idpInitiated)
    {
    }
}

`

In global.asax, for each HTTP request, I take these items from the cookie, and add them to the Session state, where SAML2 expects these items to be.

`
void Application_PreRequestHandlerExecute(object sender, EventArgs e)
{
// Het SAML2 package bewaart IDP-sessieinformatie in de ASP Session.
// Die informatie is nodig bij uitloggen bij de ISP

        // We willen dat SAML2 de gebruiker ook kan uitloggen als de gebruiker tijdens z'n
        // authenticatiesessie een nieuwe ASP Session heeft gekregen.

        // Daarom heben we vitale sesiegegevens ook in de ASP authenticatiecookie.
        // We gaan die IDP-sessieegenschappen uit de ASP authenticatiecookie halen
        // en in de ASP Session stoppen.  

        var session = HttpContext.Current.Session;
        if (session == null)
            return;

        var formsAuthCookie = HttpContext.Current.Request.Cookies[FormsAuthentication.FormsCookieName];

        if (formsAuthCookie == null)
            return;

        var formsAuthTicket = FormsAuthentication.Decrypt(formsAuthCookie.Value);

        if (string.IsNullOrWhiteSpace(formsAuthTicket.UserData))
            // De cookie bevat geen IDPSession in de user data.
            return;

        var iDPSession = JsonConvert.DeserializeObject<Authentication.IDPSession>(formsAuthTicket.UserData);

        session[SAML2.Protocol.Saml20AbstractEndpointHandler.IdpNameId] = formsAuthTicket.Name;
        session[SAML2.Protocol.Saml20AbstractEndpointHandler.IdpNameIdFormat] = iDPSession.NameIdFormat;
        session[SAML2.Protocol.Saml20AbstractEndpointHandler.IdpLoginSessionKey] = iDPSession.IDP;
        session[SAML2.Protocol.Saml20AbstractEndpointHandler.IdpSessionIdKey] = iDPSession.SessionID;
    }

`

Now during logout, SAML2 finds the required values in the ASP Session, even if this session is new and empty.
Basically, the ASP Session is not a suitable place for stuff that is related to the authentication session. These sessions have little relation.

Perhaps I will try making a custom state manager for SAML2 that holds SAML2's state in an encrypted cookie for this purpose alone.
I believe SAML2 can be configured to use a custom state manager.

@i8beef
Copy link
Owner

i8beef commented Mar 1, 2018

Did you try the branch mentioned here? It's waiting for merge based on testing, and it at least refactored part of this. It's been a while now, but if you need this as well and are willing to work through the testing with me, I can help get that resolved.

@tafs7
Copy link
Author

tafs7 commented Mar 2, 2018

I've not been able to test, as the project I used this lib on has been in production for a while and I don't have access to another IdP with SAML to test this out on. Would love to see this tested and confirmed, because it still is a "known bug" in our app when logging out.

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

No branches or pull requests

3 participants