From 320343035401b5128f23c8897a614dcaee4f4151 Mon Sep 17 00:00:00 2001 From: Mikko Piuhola Date: Tue, 20 Apr 2021 17:26:59 +0300 Subject: [PATCH] APIGW: support SLO without cookies, drop logout cookie - Many browsers are starting to disable 3rd party cookies by default (even SameSite None) which breaks Single Logout with passport-saml that currently only supports logging out with a cookie - Browser details: https://webkit.org/blog/10218/full-third-party-cookie-blocking-and-more/ and https://blog.google/products/chrome/more-intuitive-privacy-and-security-controls-chrome/ - passport-saml issues: https://github.com/node-saml/passport-saml/issues/419 and https://github.com/node-saml/passport-saml/issues/221 - This is actually a real security issue as passport-saml will give a successful looking response from `passport.authenticate()` in the logout callback which results in IdPs showing "successfully logged out" messages to the user -- even though the user is still fully logged in! - This also causes an usability issue when the user tries to initate SLO from another application, fails to end the eVaka session, attempts a logout from eVaka and gets an ugly JSON error message as a response when APIGW attempts to make a LogoutRequest to the IdP that already ended the session -> will be fixed separately - Other systems like Shibboleth get around the 3rd party cookie issue with Single Logout by not utilizing cookies at all for this but instead use the SAML nameID (and sessionIndex) properties presented in all SAML LogoutRequest messages - Source: https://wiki.shibboleth.net/confluence/display/DEV/IdP+SameSite+Testing#IdPSameSiteTesting-SameSiteandSingleLogout - When logouts are always only done through SAML there's no need for the logout cookie itself but the idea is actually useful as an effective "secondary index" for Redis: - By storing a nameID + sessionIndex keyed item pointing to the session ID, we effectively create a second index that can be used with just the SAML LogoutRequest's nameID and sessionIndex properties --- apigw/src/enduser/app.ts | 2 +- apigw/src/internal/app.ts | 4 +- apigw/src/internal/routes/auth-status.ts | 5 +- apigw/src/shared/auth/ad-saml.ts | 1 + apigw/src/shared/express.ts | 2 +- apigw/src/shared/passport-saml.d.ts | 27 +++++ apigw/src/shared/routes/auth/saml/index.ts | 21 ++-- apigw/src/shared/session.ts | 127 +++++++++++++-------- 8 files changed, 126 insertions(+), 63 deletions(-) diff --git a/apigw/src/enduser/app.ts b/apigw/src/enduser/app.ts index 99963876878..7342eeeab8c 100644 --- a/apigw/src/enduser/app.ts +++ b/apigw/src/enduser/app.ts @@ -48,7 +48,7 @@ app.use(passport.initialize()) app.use(passport.session()) passport.serializeUser((user, done) => done(null, user)) passport.deserializeUser((user, done) => done(null, user)) -app.use(refreshLogoutToken('enduser')) +app.use(refreshLogoutToken()) setupLoggingMiddleware(app) function apiRouter() { diff --git a/apigw/src/internal/app.ts b/apigw/src/internal/app.ts index cbd07ca1d64..8c090c39863 100644 --- a/apigw/src/internal/app.ts +++ b/apigw/src/internal/app.ts @@ -52,7 +52,7 @@ app.use(passport.initialize()) app.use(passport.session()) passport.serializeUser((user, done) => done(null, user)) passport.deserializeUser((user, done) => done(null, user)) -app.use(refreshLogoutToken('employee')) +app.use(refreshLogoutToken()) setupLoggingMiddleware(app) app.use('/api/csp', csp) @@ -69,7 +69,7 @@ function internalApiRouter() { router.all('/system/*', (req, res) => res.sendStatus(404)) router.all('/auth/*', (req: express.Request, res, next) => { - if (req.session?.logoutToken?.idpProvider === 'evaka') { + if (req.session?.idpProvider === 'evaka') { req.url = req.url.replace('saml', 'evaka') } next() diff --git a/apigw/src/internal/routes/auth-status.ts b/apigw/src/internal/routes/auth-status.ts index e9a60a63558..f2f452acf24 100644 --- a/apigw/src/internal/routes/auth-status.ts +++ b/apigw/src/internal/routes/auth-status.ts @@ -10,6 +10,7 @@ import { } from '../../shared/service-client' import { logoutExpress } from '../../shared/session' import { fromCallback } from '../../shared/promise-utils' +import { SAML } from 'passport-saml' export default toRequestHandler(async (req, res) => { const user = req.user @@ -28,8 +29,8 @@ export default toRequestHandler(async (req, res) => { roles: [...globalRoles, ...allScopedRoles] }) } else { - // device has been removed - await logoutExpress(req, res, 'employee') + // device has been removed (SAML config not relevant) + await logoutExpress(req, res, 'employee', new SAML({})) res.status(200).json({ loggedIn: false }) } } else { diff --git a/apigw/src/shared/auth/ad-saml.ts b/apigw/src/shared/auth/ad-saml.ts index 984a1e76be1..f634a03487f 100755 --- a/apigw/src/shared/auth/ad-saml.ts +++ b/apigw/src/shared/auth/ad-saml.ts @@ -65,6 +65,7 @@ async function verifyProfile(profile: AdProfile): Promise { } export function createSamlConfig(redisClient?: RedisClient): SamlConfig { + if (devLoginEnabled) return {} if (!adConfig) throw Error('Missing AD SAML configuration') return { acceptedClockSkewMs: 0, diff --git a/apigw/src/shared/express.ts b/apigw/src/shared/express.ts index bf2df0d45e6..c27b8d083ad 100644 --- a/apigw/src/shared/express.ts +++ b/apigw/src/shared/express.ts @@ -11,7 +11,6 @@ export interface LogoutToken { // milliseconds value of a Date. Not an actual Date because it will be JSONified expiresAt: number value: string - idpProvider?: string | null } export type AsyncRequestHandler = ( @@ -52,6 +51,7 @@ export class InvalidRequest extends BaseError {} // Use TS interface merging to add fields to express req.session declare module 'express-session' { interface SessionData { + idpProvider?: string | null logoutToken?: LogoutToken } } diff --git a/apigw/src/shared/passport-saml.d.ts b/apigw/src/shared/passport-saml.d.ts index b31661a1a1e..9727009486b 100644 --- a/apigw/src/shared/passport-saml.d.ts +++ b/apigw/src/shared/passport-saml.d.ts @@ -12,6 +12,7 @@ declare module 'passport-saml' { import type passport from 'passport' import type express from 'express' + import { ParsedQs } from 'qs' export interface CacheItem { createdAt: number @@ -177,4 +178,30 @@ declare module 'passport-saml' { callback: (err: Error | null, metadata?: string) => void ): string } + + // SPDX-FileCopyrightText: 2017-2021 City of Espoo + // + // SPDX-License-Identifier: LGPL-2.1-or-later + export class SAML { + constructor(options: SamlConfig) + + validateRedirect( + container: ParsedQs, + originalQuery: string, + callback: ( + err: Error | null, + profile?: Profile | null, + loggedOut?: boolean + ) => void + ): void + + validatePostRequest( + container: Record, + callback: ( + err: Error | null, + profile?: Profile | null, + loggedOut?: boolean + ) => void + ): void + } } diff --git a/apigw/src/shared/routes/auth/saml/index.ts b/apigw/src/shared/routes/auth/saml/index.ts index 9ef81791463..221236ad5e0 100755 --- a/apigw/src/shared/routes/auth/saml/index.ts +++ b/apigw/src/shared/routes/auth/saml/index.ts @@ -13,7 +13,7 @@ import { logoutExpress, saveLogoutToken } from '../../../session' import { fromCallback } from '../../../promise-utils' import { getEmployees } from '../../../dev-api' import _ from 'lodash' -import type { AuthenticateOptions } from 'passport-saml' +import { AuthenticateOptions, SAML } from 'passport-saml' const urlencodedParser = urlencoded({ extended: false }) @@ -53,8 +53,7 @@ function getRedirectUrl(req: express.Request): string { } function createLoginHandler({ - strategyName, - sessionType + strategyName }: SamlEndpointConfig): express.RequestHandler { return (req, res, next) => { logAuditEvent( @@ -93,7 +92,7 @@ function createLoginHandler({ req, 'User logged in successfully' ) - await saveLogoutToken(req, res, sessionType, strategyName) + await saveLogoutToken(req, strategyName) const redirectUrl = getRedirectUrl(req) logDebug(`Redirecting to ${redirectUrl}`, req, { redirectUrl }) return res.redirect(redirectUrl) @@ -115,10 +114,14 @@ function createLoginHandler({ } function createLogoutHandler({ + samlConfig, strategy, strategyName, sessionType }: SamlEndpointConfig): express.RequestHandler { + // For parsing SAML messages outside the strategy + const saml = new SAML(samlConfig) + return toRequestHandler(async (req, res) => { logAuditEvent( `evaka.saml.${strategyName}.sign_out_requested`, @@ -131,7 +134,7 @@ function createLogoutHandler({ strategy.logout(req, cb) ) logDebug('Logging user out from passport.', req) - await logoutExpress(req, res, sessionType) + await logoutExpress(req, res, sessionType, saml) return res.redirect(redirectUrl) } catch (err) { logAuditEvent( @@ -148,7 +151,7 @@ function createLogoutHandler({ 'User signed out' ) logDebug('Logging user out from passport.', req) - await logoutExpress(req, res, sessionType) + await logoutExpress(req, res, sessionType, saml) res.redirect(getRedirectUrl(req)) } }) @@ -161,7 +164,9 @@ function createLogoutHandler({ // * HTTP redirect: the browser makes a GET request with query parameters // * HTTP POST: the browser makes a POST request with URI-encoded form body export default function createSamlRouter(config: SamlEndpointConfig): Router { - const { strategyName, strategy, pathIdentifier } = config + const { strategyName, strategy, samlConfig, pathIdentifier } = config + // For parsing SAML messages outside the strategy + const saml = new SAML(samlConfig) passport.use(strategyName, strategy) @@ -173,7 +178,7 @@ export default function createSamlRouter(config: SamlEndpointConfig): Router { req, 'Logout callback called' ) - await logoutExpress(req, res, config.sessionType) + await logoutExpress(req, res, config.sessionType, saml) }) const router = Router() diff --git a/apigw/src/shared/session.ts b/apigw/src/shared/session.ts index e2a14897b99..dc724e53100 100644 --- a/apigw/src/shared/session.ts +++ b/apigw/src/shared/session.ts @@ -9,10 +9,10 @@ import { differenceInSeconds, isDate } from 'date-fns' -import express, { CookieOptions } from 'express' +import express, { CookieOptions, Request } from 'express' import session from 'express-session' +import { Profile, SAML } from 'passport-saml' import { RedisClient } from 'redis' -import { v4 as uuidv4 } from 'uuid' import AsyncRedisClient from './async-redis-client' import { cookieSecret, sessionTimeoutMinutes, useSecureCookies } from './config' import { toMiddleware } from './express' @@ -31,70 +31,89 @@ const sessionCookieOptions: CookieOptions = { sameSite: 'lax' } -const logoutCookieOptions: express.CookieOptions = { - path: '/', - httpOnly: true, - secure: useSecureCookies, - sameSite: useSecureCookies ? 'none' : undefined -} - function cookiePrefix(sessionType: SessionType) { return sessionType === 'enduser' ? 'evaka.eugw' : 'evaka.employee' } -function logoutCookie(sessionType: SessionType) { - return `${cookiePrefix(sessionType)}.logout` -} - export function sessionCookie(sessionType: SessionType) { return `${cookiePrefix(sessionType)}.session` } -export function refreshLogoutToken(sessionType: SessionType) { - return toMiddleware(async (req, res) => { +export function refreshLogoutToken() { + return toMiddleware(async (req) => { if (!req.session) return if (!req.session.logoutToken) return if (!isDate(req.session.cookie.expires)) return const sessionExpires = req.session.cookie.expires as Date const logoutExpires = new Date(req.session.logoutToken.expiresAt) - const cookieToken = req.cookies && req.cookies[logoutCookie(sessionType)] // Logout token should always expire at least 30 minutes later than the session - if ( - differenceInMinutes(logoutExpires, sessionExpires) < 30 || - cookieToken !== req.session.logoutToken.value - ) { - await saveLogoutToken( - req, - res, - sessionType, - req.session.logoutToken.idpProvider - ) + if (differenceInMinutes(logoutExpires, sessionExpires) < 30) { + await saveLogoutToken(req, req.session.idpProvider) } }) } +function logoutKey(nameID: string, sessionIndex?: string) { + return `slo:${nameID}.${sessionIndex}` +} + +async function tryParseProfile( + req: Request, + saml: SAML +): Promise { + // NOTE: This duplicate parsing can be removed if passport-saml ever exposes + // an alternative for passport.authenticate() that either lets us hook into + // it before any redirects or separate XML parsing and authentication methods. + if (req.query?.SAMLRequest) { + // Redirects have signatures in the original query parameteru + const dummyOrigin = 'http://evaka' + const originalQuery = new URL(req.url, dummyOrigin).search.replace( + /^\?/, + '' + ) + return await fromCallback((cb) => + saml.validateRedirect(req.query, originalQuery, cb) + ) + } else if (req.body?.SAMLRequest) { + // POST logout callbacks have the signature in the message body directly + return await fromCallback((cb) => + saml.validatePostRequest(req.body, cb) + ) + } +} + +/** + * Save a logout token for a user session to be consumed during logout. + * + * The token is generated by creating an effective secondary + * index in Redis from SAML session identifiers (nameID and sessionIndex). + * This token can then be used with LogoutRequests without relying + * on 3rd party cookies which are starting to be disabled by default on many + * browsers, enabling Single Logout. + * + * This token can be removed if this passport-saml issue is ever fixed: + * https://github.com/node-saml/passport-saml/issues/419 + */ export async function saveLogoutToken( req: express.Request, - res: express.Response, - sessionType: SessionType, strategyName: string | null | undefined ): Promise { - if (!req.session) return + if (!req.session || !req.user?.nameID) return + + // Persist in session to allow custom logic per strategy + req.session.idpProvider = strategyName + + if (!asyncRedisClient) return + const key = logoutKey(req.user.nameID, req.user.sessionIndex) + const now = new Date() const expires = addMinutes(now, sessionTimeoutMinutes + 60) - const idpProvider = strategyName const logoutToken = { expiresAt: expires.valueOf(), - value: req.session.logoutToken ? req.session.logoutToken.value : uuidv4(), - idpProvider + value: req.session.logoutToken?.value || key } req.session.logoutToken = logoutToken - res.cookie(logoutCookie(sessionType), logoutToken.value, { - ...logoutCookieOptions, - expires - }) - if (!asyncRedisClient) return - const key = `logout:${logoutToken.value}` + const ttlSeconds = differenceInSeconds(expires, now) // https://redis.io/commands/expire - Set a timeout on key // Return value: @@ -108,33 +127,43 @@ export async function saveLogoutToken( await asyncRedisClient.set(key, req.session.id, 'EX', ttlSeconds) } -// If a logout token is available, delete it and delete the session it points to -export async function consumeLogoutToken( +async function consumeLogoutRequest( req: express.Request, - res: express.Response, - sessionType: SessionType + saml: SAML ): Promise { - const token = req.cookies && req.cookies[logoutCookie(sessionType)] - if (!token || !asyncRedisClient) return - res.clearCookie(logoutCookie(sessionType), logoutCookieOptions) - const sid = await asyncRedisClient.get(`logout:${token}`) + if (!asyncRedisClient) return + + const profile = await tryParseProfile(req, saml) + // Prefer details from the SAML message (profile) but fall back to details + // from the session in case a) this wasn't a SAMLRequest b) it's malformed + // to ensure the logout token is deleted from the store even in non-SLO cases. + const nameID = profile?.nameID ?? req.user?.nameID + const sessionIndex = profile?.sessionIndex ?? req.user?.sessionIndex + + if (!nameID) return // Nothing to consume without a SAMLRequest or session + + const key = logoutKey(nameID, sessionIndex) + const sid = await asyncRedisClient.get(key) if (sid) { - await asyncRedisClient.del(`sess:${sid}`, `logout:${token}`) + // Ensure both session and logout keys are cleared in case no cookies were + // available -> no req.session was available to be deleted. + await asyncRedisClient.del(`sess:${sid}`, key) } } export async function logoutExpress( req: express.Request, res: express.Response, - sessionType: SessionType + sessionType: SessionType, + saml: SAML ) { req.logout() + await consumeLogoutRequest(req, saml) if (req.session) { const session = req.session await fromCallback((cb) => session.destroy(cb)) } res.clearCookie(sessionCookie(sessionType)) - await consumeLogoutToken(req, res, sessionType) } export default (sessionType: SessionType, redisClient?: RedisClient) => {