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

SAML SP-initiated Single Logout (SLO) is not invalidating sessions prior to redirecting to the IdP #4713

Closed
akkornel opened this issue Dec 5, 2023 · 4 comments

Comments

@akkornel
Copy link

akkornel commented Dec 5, 2023

Describe the Bug

Hello! I'd like to report an issue with SAML Logout: When initiating a logout from within Bookstack, Bookstack is not invalidating its session before sending the user to the SAML Identity Provider (IdP).

SAML Logout can work in a couple of different ways:

  1. The logout can be initiated from outside of Bookstack. In this case, the SAML 2.0 IdP (Identity Provider) sends a request to Bookstack, asking for the user to be logged out. This is IdP-initiated Single Logout (SLO).

  2. The logout can be initiated from inside of Bookstack. In this case, Bookstack needs to invalidate its session, and then redirect the user to the IdP to continue the logout process. This is SP-initiated Single Logout (SLO).

I'm reporting a problem the the second method, SP-initiated SLO.

Steps to Reproduce

  1. Configure Bookstack for SAML 2.0 authentication, using an IdP that supports Single Logout (SLO).
  2. Go to the Bookstack main page, and log in to Bookstack via SAML. You are eventually redirected back to Bookstack, and you are logged in.
  3. From within Bookstack, select the "Logout" option. You are redirected to the SAML IdP.
  4. Go back to the Bookstack main page.

Expected Behaviour

I expected to be prompted to log in to Bookstack. Instead, I was presented with the Bookstack main page; my Bookstack session was still valid.

Screenshots or Additional Context

I apologize in advance: This is a messy issue, with a lot of moving parts. If any part of my report is confusing, please let me know!

In the SAML V2.0 Technical Overview, SAML 2.0 Logout is defined in Section 5.3. Section 5.3.2 has a good diagram, showing both IdP-initiated and SP-initiated SLO.

In the diagram, Service Provider sp1.example.com is going through SP-initiated SLO: The user (or, really, their web browser) has asked for a logout (Step 1), and the Service Provider (Bookstack) is redirecting the user to the IdP (Step 2). The redirect is working fine.

What's missing is the session invalidation, as described in the text below the diagram:

  1. The SP sp1.example.com destroys the local authentication session state for the user and then sends the idp.example.org identity provider a SAML <LogoutRequest> message…

The emphasized text is what I'm talking about: In case something goes wrong with the logout process (for example, maybe the IdP is down), the session (on the Bookstack side) should be "destroyed". That being said, Steps 5 and 6 make this more complicated:

  1. The identity provider returns a <LogoutResponse> message containing a suitable status code response to the original requesting service provider, sp1.example.com [Bookstack]. The response is digitally signed and returned (in this case) using the HTTP Redirect binding
  2. Finally, the service provider sp1.example.com [Bookstack] informs the user that they are logged out of all the providers.

The reason I say this is confusing is because, even though the authentication session state is supposed to be destroyed in Step 1, the SP (Bookstack) is still supposed to store enough information to be able to authenticate the message from the IdP in Step 5, and do something with the user in Step 6 (which I think, for Bookstack, is to return the user to the main page).

I understand that you're using php-saml, and looking through their documentation, I see there isn't any mention of the need to destroy local authentication session state (Step 1 from the top of this section). I'm wondering if there should also be an Issue raised with the php-saml folks.

Browser Details

No response

Exact BookStack Version

23.10.4

@ssddanbrown
Copy link
Member

Thanks for raising @akkornel, I agree this is currently not as per spec. I get the feeling I misunderstood the point of logout, by step 6 of the diagram specifically labelled as "Logged out" between the SP and browser.

I'll assign this to be addressed for the next BookStack feature release.
Thanks for the effort of the detailed explanation given in this issue.

@radiantwave
Copy link
Contributor

radiantwave commented Dec 6, 2023

#2553 related?

Have the same issue right now.
This is pretty drastic for me, since users can't change accounts at all.
Authenticating with another user doesn't log you into the desired account because the old sessions never end.

@ssddanbrown
Copy link
Member

@radiantwave Related, but not the same. In a full logout flow you should be returned to BookStack eventually where you'll then be logged out of BookStack. This is the wrong point of logout within BookStack, which is what this issue addresses, but logout should work in most cases unless cancelled (or something goes wrong) on the IdP side of things.

#2553 was maybe more similar to your issue but likely specific to meeting Azure's requirements.
Your issue is likely similar, but specific to your IdP, although it would also be solved by this issue once addressed (although that would still leave a question if why you're not reaching the latter parts of the flow).

ssddanbrown added a commit that referenced this issue Dec 8, 2023
This changes the point-of-logout to be within the initial part of the
SAML logout flow, as per 5.3.2 of the SAML spec, processing step 2.
This also improves the logout redirect handling to use the global
redirect suggestion so that auto-login handling is properly taken into
account.

Added tests to cover.
Manual testing performed against keycloak.
For #4713
@ssddanbrown
Copy link
Member

This has now been addressed via 8cbaa3e, with testing added and manual testing performed via Keycloak, via both SP and IdP initiated logout, with various configuration changes and forced breaking of the full auth flow.

These changes will be part of the next feature release.
Thanks again @akkornel for raising.

akkornel added a commit to stanford-rc/docker-bookstack-certbot that referenced this issue Mar 2, 2024
See https://github.com/BookStackApp/BookStack/releases/tag/v23.12 for
the list of improvements for 23.12.  The .1 and .2 releases are smaller
bugfixes, and .3 brings a security fix related to PDF generation.

This also brings in two SAML-related improvements
(BookStackApp/BookStack#4713 and BookStackApp/BookStack#4706).  Thanks
again ssddanbrown for them!  And thanks also to the LinuxServer folks
for the packaging.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

No branches or pull requests

3 participants