diff --git a/x-pack/plugins/security/server/lib/authentication/authenticator.js b/x-pack/plugins/security/server/lib/authentication/authenticator.js index 63492b056687d..b8ea329227738 100644 --- a/x-pack/plugins/security/server/lib/authentication/authenticator.js +++ b/x-pack/plugins/security/server/lib/authentication/authenticator.js @@ -202,13 +202,20 @@ class Authenticator { return this._providers.get(sessionValue.provider).deauthenticate(request, sessionValue.state); } - // Normally when there is no active session in Kibana, `deauthenticate` method shouldn't do anything - // and user will eventually be redirected to the home page to log in. But if SAML is supported there - // is a special case when logout is initiated by the IdP or another SP, then IdP will request _every_ - // SP associated with the current user session to do the logout. So if Kibana (without active session) - // receives such a request it shouldn't redirect user to the home page, but rather redirect back to IdP - // with correct logout response and only Elasticsearch knows how to do that. - if (this._isSAMLRequest(request) && this._providers.has('saml')) { + // Normally when there is no active session in Kibana, `deauthenticate` method shouldn't do + // anything and user will eventually be redirected to the home page to log in. But when SAML SLO + // is supported there are two special cases that we need to handle even if there is no active + // Kibana session: + // + // 1. When IdP or another SP initiates logout, then IdP will request _every_ SP associated with + // the current user session to do the logout. So if Kibana receives such request it shouldn't + // redirect user to the home page, but rather redirect back to IdP with correct logout response + // and only Elasticsearch knows how to do that. + // + // 2. When Kibana initiates logout, then IdP may eventually respond with the logout response. So + // if Kibana receives such response it shouldn't redirect user to the home page, but rather + // redirect to the `loggedOut` URL instead. + if ((this._isSAMLRequestQuery(request) || this._isSAMLResponseQuery(request)) && this._providers.has('saml')) { return this._providers.get('saml').deauthenticate(request); } @@ -241,7 +248,7 @@ class Authenticator { // If there is no way to predict which provider to use first, let's use the order providers are configured in. // Otherwise return provider that either owns session or can handle 3rd-party login request first, and only then // the rest of providers. - const shouldHandleSAMLResponse = this._isSAMLResponse(request) && this._providers.has('saml'); + const shouldHandleSAMLResponse = this._isSAMLResponsePayload(request) && this._providers.has('saml'); if (!sessionValue && !shouldHandleSAMLResponse) { yield* this._providers; } else { @@ -278,22 +285,32 @@ class Authenticator { } /** - * Checks whether specified request represents SAML Request. + * Checks whether specified request has SAML Request in the query. * @param {Hapi.Request} request HapiJS request instance. * @returns {boolean} * @private */ - _isSAMLRequest(request) { + _isSAMLRequestQuery(request) { return !!(request.query && request.query.SAMLRequest); } /** - * Checks whether specified request represents SAML Response. + * Checks whether specified request has SAML Response in the query. * @param {Hapi.Request} request HapiJS request instance. * @returns {boolean} * @private */ - _isSAMLResponse(request) { + _isSAMLResponseQuery(request) { + return !!(request.query && request.query.SAMLResponse); + } + + /** + * Checks whether specified request has SAML Response in the payload. + * @param {Hapi.Request} request HapiJS request instance. + * @returns {boolean} + * @private + */ + _isSAMLResponsePayload(request) { return !!(request.payload && request.payload.SAMLResponse) && request.path === '/api/security/v1/saml'; } diff --git a/x-pack/plugins/security/server/lib/authentication/providers/__tests__/saml.js b/x-pack/plugins/security/server/lib/authentication/providers/__tests__/saml.js index 15204669e6ae1..6f7790a90d971 100644 --- a/x-pack/plugins/security/server/lib/authentication/providers/__tests__/saml.js +++ b/x-pack/plugins/security/server/lib/authentication/providers/__tests__/saml.js @@ -559,7 +559,7 @@ describe('SAMLAuthenticationProvider', () => { ); expect(authenticationResult.redirected()).to.be(true); - expect(authenticationResult.redirectURL).to.be('/logged_out'); + expect(authenticationResult.redirectURL).to.be('/test-base-path/logged_out'); }); it('redirects to /logged_out if `redirect` field in SAML logout response is not defined.', async () => { @@ -581,11 +581,11 @@ describe('SAMLAuthenticationProvider', () => { ); expect(authenticationResult.redirected()).to.be(true); - expect(authenticationResult.redirectURL).to.be('/logged_out'); + expect(authenticationResult.redirectURL).to.be('/test-base-path/logged_out'); }); it('relies on SAML logout if query string is not empty, but does not include SAMLRequest.', async () => { - const request = requestFixture({ search: '?Whatever=something%20unrelated' }); + const request = requestFixture({ search: '?Whatever=something%20unrelated&SAMLResponse=xxx%20yyy' }); const accessToken = 'x-saml-token'; const refreshToken = 'x-saml-refresh-token'; @@ -603,7 +603,7 @@ describe('SAMLAuthenticationProvider', () => { ); expect(authenticationResult.redirected()).to.be(true); - expect(authenticationResult.redirectURL).to.be('/logged_out'); + expect(authenticationResult.redirectURL).to.be('/test-base-path/logged_out'); }); it('relies SAML invalidate call even if access token is presented.', async () => { @@ -631,7 +631,7 @@ describe('SAMLAuthenticationProvider', () => { ); expect(authenticationResult.redirected()).to.be(true); - expect(authenticationResult.redirectURL).to.be('/logged_out'); + expect(authenticationResult.redirectURL).to.be('/test-base-path/logged_out'); }); it('redirects to /logged_out if `redirect` field in SAML invalidate response is null.', async () => { @@ -656,7 +656,7 @@ describe('SAMLAuthenticationProvider', () => { ); expect(authenticationResult.redirected()).to.be(true); - expect(authenticationResult.redirectURL).to.be('/logged_out'); + expect(authenticationResult.redirectURL).to.be('/test-base-path/logged_out'); }); it('redirects to /logged_out if `redirect` field in SAML invalidate response is not defined.', async () => { @@ -681,7 +681,18 @@ describe('SAMLAuthenticationProvider', () => { ); expect(authenticationResult.redirected()).to.be(true); - expect(authenticationResult.redirectURL).to.be('/logged_out'); + expect(authenticationResult.redirectURL).to.be('/test-base-path/logged_out'); + }); + + it('redirects to /logged_out if SAML logout response is received.', async () => { + const request = requestFixture({ search: '?SAMLResponse=xxx%20yyy' }); + + const authenticationResult = await provider.deauthenticate(request); + + sinon.assert.notCalled(callWithInternalUser); + + expect(authenticationResult.redirected()).to.be(true); + expect(authenticationResult.redirectURL).to.be('/test-base-path/logged_out'); }); it('redirects user to the IdP if SLO is supported by IdP in case of SP initiated logout.', async () => { diff --git a/x-pack/plugins/security/server/lib/authentication/providers/saml.js b/x-pack/plugins/security/server/lib/authentication/providers/saml.js index 335e8562ef74e..8332cc96e242b 100644 --- a/x-pack/plugins/security/server/lib/authentication/providers/saml.js +++ b/x-pack/plugins/security/server/lib/authentication/providers/saml.js @@ -387,20 +387,22 @@ export class SAMLAuthenticationProvider { async deauthenticate(request, state) { this._options.log(['debug', 'security', 'saml'], `Trying to deauthenticate user via ${request.url.path}.`); - if ((!state || !state.accessToken) && !request.query.SAMLRequest) { + const isIdPInitiatedSLORequest = !!(request.query && request.query.SAMLRequest); + const isSPInitiatedSLOResponse = !!(request.query && request.query.SAMLResponse); + if ((!state || !state.accessToken) && !isIdPInitiatedSLORequest && !isSPInitiatedSLOResponse) { this._options.log(['debug', 'security', 'saml'], 'There is neither access token nor SAML session to invalidate.'); return DeauthenticationResult.notHandled(); } let logoutArgs; - if (request.query.SAMLRequest) { + if (isIdPInitiatedSLORequest) { this._options.log(['debug', 'security', 'saml'], 'Logout has been initiated by the Identity Provider.'); logoutArgs = [ 'shield.samlInvalidate', // Elasticsearch expects `queryString` without leading `?`, so we should strip it with `slice`. { body: { queryString: request.url.search ? request.url.search.slice(1) : '', acs: this._getACS() } } ]; - } else { + } else if (state) { this._options.log(['debug', 'security', 'saml'], 'Logout has been initiated by the user.'); logoutArgs = [ 'shield.samlLogout', @@ -409,9 +411,15 @@ export class SAMLAuthenticationProvider { } try { - // This operation should be performed on behalf of the user with a privilege that normal - // user usually doesn't have `cluster:admin/xpack/security/saml/logout (invalidate)`. - const { redirect } = await this._options.client.callWithInternalUser(...logoutArgs); + // It may _theoretically_ (highly unlikely in practice though) happen that when user receives + // logout response they may already have a new SAML session (isSPInitiatedSLOResponse == true + // and state !== undefined). In this case case it'd be safer to trigger SP initiated logout + // for the new session as well. + const redirect = logoutArgs + // This operation should be performed on behalf of the user with a privilege that normal + // user usually doesn't have `cluster:admin/xpack/security/saml/logout (invalidate)`. + ? (await this._options.client.callWithInternalUser(...logoutArgs)).redirect + : null; this._options.log(['debug', 'security', 'saml'], 'User session has been successfully invalidated.'); @@ -423,7 +431,7 @@ export class SAMLAuthenticationProvider { return DeauthenticationResult.redirectTo(redirect); } - return DeauthenticationResult.redirectTo('/logged_out'); + return DeauthenticationResult.redirectTo(`${this._options.basePath}/logged_out`); } catch(err) { this._options.log(['debug', 'security', 'saml'], `Failed to deauthenticate user: ${err.message}`); return DeauthenticationResult.failed(err);