-
Notifications
You must be signed in to change notification settings - Fork 475
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
passport-saml's IdP initiated LogoutRequest handling doesn't always close sessions #419
Comments
Thanks for the detailed report. |
@srd90 You have a clear understanding of the issue. Are you willing to submit a patch for it? |
Original issue description talked about combination where end user has explicitly blocked third party cookies and certain IdP setups (iframe based SLO propagation) in cross-domain IdP initiated SLO situation. IMHO upcoming change where browser vendors start to apply samesite lax cookie policy by default (https://www.chromestatus.com/feature/5088147346030592) breaks also those cross-domain SLO scenarios (when passport-saml is involved in the loop) which use http redirect hops from one SP to another. For additional information about samesite see for example
IMHO in a scenario described above redirect hops from one site to another those redirects would not be treated as "a top level navigation" which would/could mean that cookies are not attached to requests. I haven't actually tested this but it seems quite obvious. Regarding your question about patch. It must contain at least functionality which prevents I don't have skills to provide such a patch and I'm not using or planning to use Patch could look something like code visible in a diff at below but I can't see how that would ever work in clustered environment with all sort of session stores and possibly concurrent http requests from same browser instance etc. This diff contains few lines of pseudo-code and must not be used as-is in production: diff --git a/lib/passport-saml/saml.js b/lib/passport-saml/saml.js
index e7e9283..e68fb97 100644
--- a/lib/passport-saml/saml.js
+++ b/lib/passport-saml/saml.js
@@ -287,7 +287,8 @@ SAML.prototype.generateLogoutResponse = function (req, logoutRequest) {
},
'samlp:Status': {
'samlp:StatusCode': {
- '@Value': 'urn:oasis:names:tc:SAML:2.0:status:Success'
+ //
+ '@Value': req.samlLogoutResponseStatusCode
}
}
}
diff --git a/lib/passport-saml/strategy.js b/lib/passport-saml/strategy.js
index 4d57599..56c9881 100644
--- a/lib/passport-saml/strategy.js
+++ b/lib/passport-saml/strategy.js
@@ -43,9 +43,46 @@ Strategy.prototype.authenticate = function (req, options) {
}
if (loggedOut) {
+ let reqUsersNameIdAndSessionIndexMatchesLogoutRequestInfo = false;
+ // check that we have authenticated session
+ if (req.isAuthenticated()) {
+ // check that LogoutRequest by IdP contains necessary information
+ // to check whether req.user matches with information in logout request
+ if (profile && profile.nameID && profile.sessionIndex) {
+ // check that req.user (which is alias for req.session.passport.user
+ // and content was deserialized by some storage by function provided
+ // by surrounding application) contains same nameID and sessionIndex
+ // as logoutrequest sent by IdP
+ // NOTE: assumes that deserialize function has placed nameId and sessionIndex
+ // into following positions at deserialized user
+ if (req.user.nameID && req.user.sessionIndex) {
+ // check that nameID and sessionIndex matches
+ if (req.user.nameID === profile.nameID && req.user.sessionIndex === profile.sessionIndex) {
+ reqUsersNameIdAndSessionIndexMatchesLogoutRequestInfo = true;
+ }
+ }
+ }
+ }
+ // AFAIK it is possible that same browser instance has triggered two requests to
+ // application which are handled concurrently at different nodejs instances
+ // behind LB. Based on documentation express session resaves automatically
+ // even if there are not any changes to session (and automatically if changes
+ // were made) so its possible that concurrent request handling has a session
+ // which contains user data and that session is rewritten to session store
+ // after logoutrequest handling is ended which means that logout has not happened
+ // because next request is able to deserialize user object again.
req.logout();
if (profile) {
req.samlLogoutRequest = profile;
+ req.samlLogoutResponseStatusCode = "....by default some error code....";
+
+ if (reqUsersNameIdAndSessionIndexMatchesLogoutRequestInfo === true) {
+ //
+ // assumes that req.logout() managed to perform logout successfully
+ // (see comments before req.logout() line).
+ // ugly way to pass information for saml.generateLogoutResponse(...)
+ req.samlLogoutResponseStatusCode = "....success resultcode....";
+ }
return self._saml.getLogoutResponseUrl(req, options, redirectIfSuccess);
}
return self.pass(); If patch is defined as "solution for IdP initiated logout which just works"... Idea 1: This would not work
Idea 2: Existence of this mapping could be checked during every http invocation (after deserialize function has provided data to req.user ). If query with req.user.nameID + req.user.sessionIndex does not return anything session must have been terminated and request would have to be modified so that from that point on it is treated as unauthenticated (for example with invocation of req.logout() which is alias for passport's logout function just removes user property and deletes property from session ... see implementation of that function). If query returned a timestamp one could check if notOnOrAfter provided by IdP for this SP session has been exceeded and modify request so that it is treated as unauthenticated (and mapping should be removed). When IdP initiated LogoutRequest is received existence of nameId+sessionIndex mapping is checked and if it exists it is removed from "saml session cache", so that subsequent lookup operations do not return anything. If When SP is logout initiator it must clear aforementioned mapping (along with other cleanup stuff made during logout process) from "saml session cache" before it redirect browser to IdP to initiate SLO. Problems:
Difference to "idea 1" is that Ideas described above are NOT tested in any way. Links to external sites (from this comment) were valid Sun, 9 Feb 2020 |
I affirm that other SAML projects are also discussing the impact of Chrome's M80 release and "SameSite=None" cookies on SLO. Here's a description of the issue on the Shibboleth wiki: |
IMHO wiki entry you referenced says: ShibbolethSP's SLO is unaffected of samesite change because it doesn't use/require cookies during SLO processing. It uses information from logout request (nameid) to invalidate login session managed by ShibbolethSP's own sessionmanagement. If appliction which sits behind ShibbolethSP rely only on ShibbolethSP's session management and shibd is used in ”active mode” then SLO over redirect and POST binding keeps working from protected application point of view. If application which sits behind ShibbolethSP has application specific session meaning that if application uses own session management in addition to ShibbolethSP's session mgmt (and if shibd is not used in ”active mode”) it is up to application how it is able to terminate user session upon receiving notification of session termination from ShibbolethSP (which received logout request from IdP via redirect or POST binding). This issue is not about ShibbolethSP. IMO passport-saml SP has locally exploitable vulnerability in SLO handling. It seems to has had it years and recent samesite change has increased ”impact surface” from browser installations which has been configured to block 3rd party cookies to mainstream browser installations which block cross domain cookies by default (case: iframe based SLO orchestration at e.g. Shibboleth IdPv3 side). To make things worst SLO propagation could be working from end user point of as it has been working earlier. Visual feedback could be unaffected. IdP's SLO orchestration page could be showing that passport-saml site (among other sites that participted to same SSO) has terminated session but under the hood sessions are not actually terminated anymore by passport-saml. IdP trusts that if SAML SP instance provides Success in logout response that session is actually closed at SP side. Passport-saml answers always Success with and without web application's session cookies available during logout request handling. Because passport/express-session's session management expects cookies and passport-saml doesn't have any extra session manamement stuff there is no way passport-saml is reporting correctly under every circumstances. I suggest adding vulnerability label to this issue. |
- 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: node-saml/passport-saml#419 and node-saml/passport-saml#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
- 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: node-saml/passport-saml#419 and node-saml/passport-saml#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
- 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: node-saml/passport-saml#419 and node-saml/passport-saml#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
- 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: node-saml/passport-saml#419 and node-saml/passport-saml#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
- 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: node-saml/passport-saml#419 and node-saml/passport-saml#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
- 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: node-saml/passport-saml#419 and node-saml/passport-saml#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
- 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: node-saml/passport-saml#419 and node-saml/passport-saml#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
- 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: node-saml/passport-saml#419 and node-saml/passport-saml#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
- 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: node-saml/passport-saml#419 and node-saml/passport-saml#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
- 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: node-saml/passport-saml#419 and node-saml/passport-saml#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
- 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: node-saml/passport-saml#419 and node-saml/passport-saml#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
- 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: node-saml/passport-saml#419 and node-saml/passport-saml#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
- 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: node-saml/passport-saml#419 and node-saml/passport-saml#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
- 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: node-saml/passport-saml#419 and node-saml/passport-saml#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
- 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: node-saml/passport-saml#419 and node-saml/passport-saml#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
- 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: node-saml/passport-saml#419 and node-saml/passport-saml#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
- 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: node-saml/passport-saml#419 and node-saml/passport-saml#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
- 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: node-saml/passport-saml#419 and node-saml/passport-saml#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
- 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: node-saml/passport-saml#419 and node-saml/passport-saml#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
- 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: node-saml/passport-saml#419 and node-saml/passport-saml#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
- As passport-saml and SAML authentication as a whole is very deeply integrated into the application, tests for it need to unfortunately set up its own instances of almost everything for the app in order to override important configs - Session store must use Redis as the Single Logout token system relies on it being available (instead of the default MemoryStore for local development) -> need to override client setup to use a in-memory mock-Redis - SAML configuration must use "real" certificates but obviously nothing from a real environment - SAML configuration needs additional adjustments to simplify testing - As simulating real 3rd party cookie situations would be really complex, deleting and re-adding cookies is used to replicate the situation - Also implement a reference test case where cookies are available - Add necessary XML + crypto dependencies for creating and parsing SAML messages - Although passport-saml has the SAML class (with missing official types...), it doesn't really provide useful methods/abstractions -- they go too far into request handling instead of just parsing and generating messages -- so decided to impelement these manually - Based on the wonderful examples from node-saml/passport-saml#419
- 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: node-saml/passport-saml#419 and node-saml/passport-saml#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
- As passport-saml and SAML authentication as a whole is very deeply integrated into the application, tests for it need to unfortunately set up its own instances of almost everything for the app in order to override important configs - Session store must use Redis as the Single Logout token system relies on it being available (instead of the default MemoryStore for local development) -> need to override client setup to use a in-memory mock-Redis - SAML configuration must use "real" certificates but obviously nothing from a real environment - SAML configuration needs additional adjustments to simplify testing - As simulating real 3rd party cookie situations would be really complex, deleting and re-adding cookies is used to replicate the situation - Also implement a reference test case where cookies are available - Add necessary XML + crypto dependencies for creating and parsing SAML messages - Although passport-saml has the SAML class (with missing official types...), it doesn't really provide useful methods/abstractions -- they go too far into request handling instead of just parsing and generating messages -- so decided to impelement these manually - Based on the wonderful examples from node-saml/passport-saml#419
The approach that we seem to be taking with the code the way it is relies on the correct implementation of Express and Passport: https://www.passportjs.org/docs/logout/ and https://www.passportjs.org/docs/configure/ ("sessions" section). You might also reference https://stackoverflow.com/questions/22052258/what-does-passport-session-middleware-do. The problem that is being exposed is that Having said all that, it is completely correct to say that we are calling |
@srd90 , I see you describe the changes that we've made as "half fixes" for this problem. Based on my explanation of how Express and Passport are involved here, I'd like to hear what you have to say about potentially getting the other "half" fixed, though I'm not sure there is a way as that appears to me to be outside the scope of |
For the record (for those who might read this issue comment in the future) "half fix" refer to these (see also discussions from collapsed code review comments):
tl;dr; those fixes aim to NOT return "Success" by default unless client code of this library provide function which gives "permission" to report "Success" (i.e. those fixes aim to delegate responsibility of result code of SLO to client libraries). I described those as "half fixes" because @cjbarth About "other half": I cannot see any way to implement "fully functional out of the box" support due to
Maybe "other half" could be handled by modifying
or something like that. If client SW implementors are eager to implement fully functional IdP initiated SLO they can inspect currently available comments and implementations (e.g. those that are discoverable via linked PRs / commits of other repositories).
|
It's possible that
passport-saml
responds to IdP initiated logout request that sessions are closed even though sessions are still alive. This can happen at least in following situation:Possible result from end use point of view: IdP reports that session related to
passport-saml
site is successfully closed even though it is not touched during logout process in anyway.This problem has potential to hit a lot of end users when chrome starts to block third party cookies by default.
Following code is sending LogoutRequests to passport-saml like
console.logs
written by example code are available at the end of the issue.Output of
reference case (with appliaction's session mgmt cookie available during LogoutRequest handling)
Output of
IdP initiated LogoutRequest must work without additional information from e.g. cookies
The text was updated successfully, but these errors were encountered: