IDP-Initiated Single Logout Issue

[Samuel Santos]

Hi,

I have been able to set up SSO successfully in my application (SP) using pac4j with many different IdPs, but I noticed an issue when attempting to perform IdP initated SLO. The issue also occurred when using your spring-webmvc-pac4j sample application.

I attemped with Salesforce and Azure AD/Office and I had the same issue in both.
For some reason, in ADFS it seems to work.

So the issue is the following:
When the logout request is sent from the IdP to my app it doesn't contain the pac4j Session Cookie, thus, when pac4j attempts to destroy the session it simply can't.

In the code I'm referring to this method:
    at org.pac4j.core.logout.handler.DefaultLogoutHandler.destroySessionFront(DefaultLogoutHandler.java:71)
    at org.pac4j.saml.logout.impl.SAML2LogoutValidator.validateLogoutRequest(SAML2LogoutValidator.java:150)
    at org.pac4j.saml.logout.impl.SAML2LogoutValidator.validate(SAML2LogoutValidator.java:81)
    at org.pac4j.saml.profile.impl.AbstractSAML2MessageReceiver.receiveMessage(AbstractSAML2MessageReceiver.java:53)

This results in always showing the error:
"The user profiles (and session) can not be destroyed for the front channel logout because the provided key is not the same as the one linked to the current session"

It seems to me that the issue is on the cookie not being present, I could be completly wrong though.
If I'm right, is it a failure on the IdP side for not storing the cookie on their side before redirecting to the SP?
Or this is how it is supposed to work and I should not be relying on cookies to perform IdP-initiated SLO?

I'll attach some screenshots of SAML tracer and pac4j logs that may be useful.
In this case I'm running your spring-webmvc-pac4j in localhost with an nginx server, so that I could provide valid URLs to the IdPs that I used instead of http://localhost/.
I changed the code so that my Salesforce IdP is used instead of your Okta testpac4j account.


Obs: Central logout starting at the SP always works, because the SP destroys its session regardless of what happens after sending the logout request.

Screenshot 1: Cookie being set after performing SSO. User can access protected route after being redirected.

Screenshot 2:

SLO is trigeered by the IdP but no cookies are sent on the SAML Logout request.

Before sending a 200 response, the error (The user profiles (and session) can not be destroyed for the front channel logout because the provided key is not the same as the one linked to the current session) is shown on the webserver.

I assume a new session ID is sent back in the set-cookie header because there wasn't a session id in the first request to begin with?

Screenshot 3: Server Logs.

Thanks in advance! 

[Jérôme LELEU]

Hi,

When the pac4j webapp receives a SAML logout request, it takes the sessionIndex from the SAML request and compares it to the sessionIndex stored in the web session.

This seems to be a useless check for a front channel logout request, but the idea is to prevent crafted logout requests (even if it's not a big deal).

About the callback, to prevent against another kind of attack, it renews the session during the authentication process, but this is normally taken into account for the SLO process.

Is the session cookie really sent on the SAML logout request (to the pac4j webapp)?

If so, can you try to disable session renewal to test again? pac4j.callback.renewSession: false

Thanks.

Best regards,

Jérôme

--
You received this message because you are subscribed to the Google Groups "Pac4j users mailing list" group.
To unsubscribe from this group and stop receiving emails from it, send an email to pac4j-users...@googlegroups.com.
To view this discussion on the web visit https://groups.google.com/d/msgid/pac4j-users/3a15031e-4145-477c-a422-bddef9d89d2en%40googlegroups.com.

[Samuel Santos]

Hello Jérôme, thank you for your answer.

The session cookie is not sent on the SAML logout request. That was exactly the issue that I mentioned in my first post (see Screenshot 2).

Hence my previous question: "is it a failure on the IdP side for not storing the cookie on their side before redirecting to the SP?" (so that they could later send it on the saml logout request)

Disabling session renewal did not solve the issue. The cookie is still not being sent by the IdP.

Once more, I emphasize, this happened to me with both Salesforce and Azure as Identity Providers.

Best regards,

Samuel Santos

[Jérôme LELEU]

Hi,

It may depend on the binding used for the logout request and the previous request and the session cookie policy.

Switch between GET/POST requests may not work with the SameSite cookie attribute at Lax, and so on...

You can change these settings or you can try pac4j v5.5.0-SNAPSHOT where I just added: defaultLogoutHandler.setCheckSessionForFrontChannelLogout(false) to ignore the session check.

Thanks.

Best regards,

Jérôme

To view this discussion on the web visit https://groups.google.com/d/msgid/pac4j-users/dc45f955-5923-4b3a-a8c9-5f5e947a3642n%40googlegroups.com.

[Samuel Santos]

Hello,

I exchanged some private emails with Jérôme and a solution was found.

The problem, as Jérôme mentioned, was indeed because of the conflict between SameSite=Lax cookies and GET/POST requests. 

Using SameSite=none for the pac4j cookie would solve the issue, however, this would not a good solution in terms of security.

Jérôme created a new flag and suggested to use defaultLogoutHandler.setCheckSessionForFrontChannelLogout(false). 

This did not work since, although it would bypass the first comparison (CommonHelper.areEquals(key, sessionToKey)) and move to the calling destroy(context, sessionStore, "front"); function call, it would still require the session cookie to destroy the correct session.

Finally, Jérôme asked me to try the destroySessionBack method instead of the destroySessionFront, and it worked perfectly.

He explained that "As we can't rely on the session cookie, we should take the back channel approach (only rely on the session index passed by the SAML logout request)." which makes total sense.

The code that I used:

    @Override
    public void destroySessionFront(final WebContext context, final SessionStore sessionStore, final String key) {
        store.remove(key);

        if (sessionStore == null) {
            logger.error("No session store available for this web context");
        } else {
            final Optional<String> optCurrentSessionId = sessionStore.getSessionId(context, false);

            if (optCurrentSessionId.isPresent()) {
                final String currentSessionId = optCurrentSessionId.get();
                logger.debug("currentSessionId: {}", currentSessionId);
                final var sessionToKey = (String) store.get(currentSessionId).orElse(null);
                logger.debug("-> key: {}", key);
                store.remove(currentSessionId);

                if (CommonHelper.areEquals(key, sessionToKey)) {
                    destroy(context, sessionStore, "front");
                } else {
                    logger.error("The user profiles (and session) can not be destroyed for the front channel logout because the provided "
                        + "key is not the same as the one linked to the current session");
                }
            } else {
                destroySessionBack(context, sessionStore, key);
            }
        }
    }

For now this will work for my application, but I hope you can commit a similar solution for this issue.

Thank you for your help Jérôme!

Best Regards,

Samuel Santos

[Samuel Santos]

Hi, 

Sorry, there is actually a bug on the code that I pasted before.

The line store.remove(key); must be inside the if (optCurrentSessionId.isPresent()).

Like this:

    @Override
    public void destroySessionFront(final WebContext context, final SessionStore sessionStore, final String key) {

        if (sessionStore == null) {
            logger.error("No session store available for this web context");
        } else {
            final Optional<String> optCurrentSessionId = sessionStore.getSessionId(context, false);

            if (optCurrentSessionId.isPresent()) {

                store.remove(key);

                final String currentSessionId = optCurrentSessionId.get();
                logger.debug("currentSessionId: {}", currentSessionId);
                final var sessionToKey = (String) store.get(currentSessionId).orElse(null);
                logger.debug("-> key: {}", key);
                store.remove(currentSessionId);

                if (CommonHelper.areEquals(key, sessionToKey)) {
                    destroy(context, sessionStore, "front");
                } else {
                    logger.error("The user profiles (and session) can not be destroyed for the front channel logout because the provided "
                        + "key is not the same as the one linked to the current session");
                }
            } else {
                destroySessionBack(context, sessionStore, key);
            }
        }
    }

Best Regards,

Samuel Santos

[Jérôme LELEU]

Hi,

It looks good to me. Would you mind submitting a PR for that?

Thanks.

Best regards,

Jérôme

To view this discussion on the web visit https://groups.google.com/d/msgid/pac4j-users/ea839ab4-cc97-4e51-aa2b-78ceb0f16624n%40googlegroups.com.

[Samuel Santos]

Hi,

Sure, here it is: https://github.com/pac4j/pac4j/pull/2323

Best Regards,

Samuel Santos

[Jérôme LELEU]

Hi,

Thank you. I just merged it in the master branch (future version 5.5.0).

Best regards,

Jérôme

To view this discussion on the web visit https://groups.google.com/d/msgid/pac4j-users/7fd40d24-da45-45d5-93a1-82e2857140fdn%40googlegroups.com.