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

Telescope SAML single logout implementation doesn't respect specification #3834

Open
srd90 opened this issue Sep 7, 2023 · 0 comments
Open
Labels
type: bug Something isn't working

Comments

@srd90
Copy link

srd90 commented Sep 7, 2023

  1. About session participant issuing <LogoutRequest> (case SP aka Telescope sends LogoutRequest when user has clicked logout button at Telescope):
    SAML 2.0 Core spec SLO processing rules, lines 2530-2531 states that session participant sends it after it has terminated (local) session.
  2. About session participant receiving <LogoutRequest> (case SP aka Telescope receives LogoutRequest when IdP is propagating SLO started at some other SP):
    SAML 2.0 Core spec SLO processing rules, lines 2589-2616 states among other things that session designated by e.g. NameId, sessionindex (i.e. information packed into <LogoutRequest>) must be used to terminate SP side session and result of SP side session termination must be reported back to IdP with <LogoutResponse> so that IdP may present correct information for end user of the result of SLO propagation over multiple SPs.

This blog post NSSSSSSO: Not So Simple Seneca SAML SSO ( https://blog.humphd.org/not-so-simple-saml/ ) which describes Seneca's logout implementation has following examples (at the time of writing this issue):

  1. SP initiated SLO

    app.get('/logout', (req, res) => {
     passport._strategy('saml').logout(req, (err, requestUrl) => {
       // Continue onto the Seneca logout endpoint if possible
       res.redirect(err ? '/' : requestUrl);
     });
    });

    implicates that local session is not terminted before issuing <LogoutRequest> to IdP (i.e. as if it would sit and wait that IdP comes back with message (in this case <LogoutResponse> to logout callback endpoint which would perform actual SP side session termination). In fact blog comment after code block says this:

    As with login, we can provide a way to logout in our app (<a href="/logout">Logout</a>), which will trigger passport to create a SAML Single Logout Request, and give us a URL to which we can redirect the user. Once there, the Seneca Identity Provider will log them out, and send them back to our app's /logout/callback. Here, we can remove the user information from our session with passport's req.logout().

    What happens from end user point of view if IdP fails to come back with <LogoutResponse> which would be used to trigger local session termination? If user is using shared computer there is a chance that at least local session of this service stays open if user doesn't clear browser state

  2. IdP initiated SLO

    app.get('/logout/callback', (req, res) => {
     // Tell passport to get rid of the user info on the session
     req.logout();
     // And go back to the web app's home page
     res.redirect('/');
    });

    When this code block receives <LogoutRequest> it must terminate local session and respond with <LogoutResponse> with correct response status. At the moment it redirects to /. I.e. this SP is behaving badly and breaks SLO propagtion completely if IdP is using redirects from one SP to another (most IdP's use iframes due to badly behaving SPs). When this code block receives <LogoutResponse> it is a signal that SLO initiated by this SP has ended and outcome of that process can be presented to end user (information is inside LogoutResponse message).
    NOTE: you must not expect to have any session cookies available even when <LogoutRequest> is received, see passport-saml #419. req.logout() goes through silently even if session is missing / not the one that should be terminated. For additional info about SLO see also passport-saml #221 (comment).


Lets see if this project's implementations are still like described at blog post (code snippet links are pinned to version which was at the head of master branch when this issue report was written):

  1. SP initiated SLO

    /**
    * /logout allows users to use Single Logout and clear login tokens
    * from their passport.js session.
    */
    router.get(
    '/',
    validateRedirectAndStateParams(),
    validateRedirectUriOrigin(),
    captureAuthDetailsOnSession(),
    (req, res, next) => {
    try {
    // eslint-disable-next-line no-underscore-dangle
    passport._strategy('saml').logout(req, (error, requestUrl) => {
    if (error) {
    logger.error({ error }, 'logout error - unable to generate logout URL');
    }
    res.redirect(requestUrl);
    });
    } catch (error) {
    logger.error({ error }, 'logout error');
    next(createError(500, `unable to logout`));
    }
    }
    );

    It doesn't terminate local session prior to issuing <LogoutRequest>

  2. IdP initiated SLO

    /**
    * /logout/callback is where the external SAML SSO provider will redirect
    * users upon successful logout. We support both GET and POST, since different
    * IdPs do it different ways.
    */
    function logout() {
    return (req, res, next) => {
    // We should have a logout object in the session. If not, something's wrong.
    if (!req.session.authDetails) {
    logger.warn('/logout/callback route hit without valid logout session details');
    next(createError(400, `unexpected access of /logout/callback`));
    return;
    }
    const { redirectUri, state } = req.session.authDetails;
    delete req.session.authDetails;
    logger.debug({ redirectUri, state }, 'processing /logout/callback');
    // Clear session passport.js user info
    req.logout();
    // Add the state on the redirect if present
    const url = state ? `${redirectUri}?state=${state}` : redirectUri;
    res.redirect(url);
    };
    }
    router.get('/callback', logout());
    router.post('/callback', logout());

    It doesn't response with <LogoutResponse> with proper status code (failure to terminate local session or success) when <LogoutRequest> is received (see also passport-saml #419).

@srd90 srd90 added the type: bug Something isn't working label Sep 7, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type: bug Something isn't working
Projects
None yet
Development

No branches or pull requests

1 participant