From 337896b6722f99737e66663dd6b84955fb1daab6 Mon Sep 17 00:00:00 2001 From: Court Ewing Date: Wed, 21 Nov 2018 13:46:11 -0500 Subject: [PATCH] Fail out of auth flow on first provider failure In practical terms, the flexibility afforded by providers being able to recover from the failures of previously configured providers isn't compelling, but the ambiguity is not ideal. --- .../authentication/__tests__/authenticator.js | 2 +- .../lib/authentication/authenticator.js | 4 ++ .../providers/__tests__/basic.js | 6 +-- .../providers/__tests__/saml.js | 5 +-- .../lib/authentication/providers/basic.js | 42 +++++++++++++------ .../lib/authentication/providers/saml.js | 42 +++++++++++++------ .../apis/security/basic_login.js | 2 +- .../apis/security/saml_login.js | 2 +- 8 files changed, 70 insertions(+), 35 deletions(-) diff --git a/x-pack/plugins/security/server/lib/authentication/__tests__/authenticator.js b/x-pack/plugins/security/server/lib/authentication/__tests__/authenticator.js index 1d47e953a1cc5..af786bd5faaac 100644 --- a/x-pack/plugins/security/server/lib/authentication/__tests__/authenticator.js +++ b/x-pack/plugins/security/server/lib/authentication/__tests__/authenticator.js @@ -102,7 +102,7 @@ describe('Authenticator', () => { } }); - it('fails if all authentication providers fail.', async () => { + it('fails if any authentication providers fail.', async () => { const request = requestFixture({ headers: { authorization: 'Basic ***' } }); session.get.withArgs(request).returns(Promise.resolve(null)); diff --git a/x-pack/plugins/security/server/lib/authentication/authenticator.js b/x-pack/plugins/security/server/lib/authentication/authenticator.js index e731c6724e9d4..398f187930cfa 100644 --- a/x-pack/plugins/security/server/lib/authentication/authenticator.js +++ b/x-pack/plugins/security/server/lib/authentication/authenticator.js @@ -169,6 +169,10 @@ class Authenticator { } } + if (authenticationResult.failed()) { + return authenticationResult; + } + if (authenticationResult.succeeded()) { // we have to do this here, as the auth scope's could be dependent on this await this._authorizationMode.initialize(request); diff --git a/x-pack/plugins/security/server/lib/authentication/providers/__tests__/basic.js b/x-pack/plugins/security/server/lib/authentication/providers/__tests__/basic.js index 931728a139814..dc0170264aa6f 100644 --- a/x-pack/plugins/security/server/lib/authentication/providers/__tests__/basic.js +++ b/x-pack/plugins/security/server/lib/authentication/providers/__tests__/basic.js @@ -4,7 +4,6 @@ * you may not use this file except in compliance with the Elastic License. */ -import Boom from 'boom'; import expect from 'expect.js'; import sinon from 'sinon'; import { requestFixture } from '../../../__tests__/__fixtures__/request'; @@ -97,7 +96,7 @@ describe('BasicAuthenticationProvider', () => { sinon.assert.calledOnce(callWithRequest); }); - it('fails if `authorization` header has unsupported schema even if state contains valid credentials.', async () => { + it('does not handle `authorization` header with unsupported schema even if state contains valid credentials.', async () => { const request = requestFixture({ headers: { authorization: 'Bearer ***' } }); const authorization = generateAuthorizationHeader('user', 'password'); @@ -105,8 +104,7 @@ describe('BasicAuthenticationProvider', () => { sinon.assert.notCalled(callWithRequest); expect(request.headers.authorization).to.be('Bearer ***'); - expect(authenticationResult.failed()).to.be(true); - expect(authenticationResult.error).to.eql(Boom.badRequest('Unsupported authentication schema: Bearer')); + expect(authenticationResult.notHandled()).to.be(true); }); it('fails if state contains invalid credentials.', async () => { 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 ad9720422cfa3..c1c4b7ab1b6b6 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 @@ -206,7 +206,7 @@ describe('SAMLAuthenticationProvider', () => { expect(authenticationResult.state).to.be(undefined); }); - it('fails if `authorization` header has unsupported schema even if state contains a valid token.', async () => { + it('does not handle `authorization` header with unsupported schema even if state contains a valid token.', async () => { const request = requestFixture({ headers: { authorization: 'Basic some:credentials' } }); const authenticationResult = await provider.authenticate(request, { @@ -216,8 +216,7 @@ describe('SAMLAuthenticationProvider', () => { sinon.assert.notCalled(callWithRequest); expect(request.headers.authorization).to.be('Basic some:credentials'); - expect(authenticationResult.failed()).to.be(true); - expect(authenticationResult.error).to.eql(Boom.badRequest('Unsupported authentication schema: Basic')); + expect(authenticationResult.notHandled()).to.be(true); }); it('fails if token from the state is rejected because of unknown reason.', async () => { diff --git a/x-pack/plugins/security/server/lib/authentication/providers/basic.js b/x-pack/plugins/security/server/lib/authentication/providers/basic.js index 87b193c9e053e..a234efef84040 100644 --- a/x-pack/plugins/security/server/lib/authentication/providers/basic.js +++ b/x-pack/plugins/security/server/lib/authentication/providers/basic.js @@ -4,7 +4,6 @@ * you may not use this file except in compliance with the Elastic License. */ -import Boom from 'boom'; import { canRedirectRequest } from '../../can_redirect_request'; import { AuthenticationResult } from '../authentication_result'; import { DeauthenticationResult } from '../deauthentication_result'; @@ -21,6 +20,14 @@ import { DeauthenticationResult } from '../deauthentication_result'; * }} ProviderOptions */ +/** + * Object that represents return value of internal header auth + * @typedef {{ + * authenticationResult: AuthenticationResult, + * headerNotRecognized?: boolean + * }} HeaderAuthAttempt + */ + /** * Provider that supports request authentication via Basic HTTP Authentication. */ @@ -49,7 +56,13 @@ export class BasicAuthenticationProvider { async authenticate(request, state) { this._options.log(['debug', 'security', 'basic'], `Trying to authenticate user request to ${request.url.path}.`); - let authenticationResult = await this._authenticateViaHeader(request); + let { + authenticationResult, + headerNotRecognized, // eslint-disable-line prefer-const + } = await this._authenticateViaHeader(request); + if (headerNotRecognized) { + return authenticationResult; + } if (authenticationResult.notHandled() && state) { authenticationResult = await this._authenticateViaState(request, state); @@ -81,7 +94,7 @@ export class BasicAuthenticationProvider { * Validates whether request contains `Basic ***` Authorization header and just passes it * forward to Elasticsearch backend. * @param {Hapi.Request} request HapiJS request instance. - * @returns {Promise.} + * @returns {Promise.} * @private */ async _authenticateViaHeader(request) { @@ -90,19 +103,18 @@ export class BasicAuthenticationProvider { const authorization = request.headers.authorization; if (!authorization) { this._options.log(['debug', 'security', 'basic'], 'Authorization header is not presented.'); - return AuthenticationResult.notHandled(); + return { + authenticationResult: AuthenticationResult.notHandled() + }; } const authenticationSchema = authorization.split(/\s+/)[0]; if (authenticationSchema.toLowerCase() !== 'basic') { this._options.log(['debug', 'security', 'basic'], `Unsupported authentication schema: ${authenticationSchema}`); - - // It's essential that we fail if non-empty, but unsupported authentication schema - // is provided to allow authenticator to consult other authentication providers - // that may support that schema. - return AuthenticationResult.failed( - Boom.badRequest(`Unsupported authentication schema: ${authenticationSchema}`) - ); + return { + authenticationResult: AuthenticationResult.notHandled(), + headerNotRecognized: true + }; } try { @@ -110,10 +122,14 @@ export class BasicAuthenticationProvider { this._options.log(['debug', 'security', 'basic'], 'Request has been authenticated via header.'); - return AuthenticationResult.succeeded(user, { authorization }); + return { + authenticationResult: AuthenticationResult.succeeded(user, { authorization }) + }; } catch(err) { this._options.log(['debug', 'security', 'basic'], `Failed to authenticate request via header: ${err.message}`); - return AuthenticationResult.failed(err); + return { + authenticationResult: AuthenticationResult.failed(err) + }; } } 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 dbe6091d987dd..a2239d4a5ac49 100644 --- a/x-pack/plugins/security/server/lib/authentication/providers/saml.js +++ b/x-pack/plugins/security/server/lib/authentication/providers/saml.js @@ -21,6 +21,14 @@ import { DeauthenticationResult } from '../deauthentication_result'; * }} ProviderOptions */ +/** + * Object that represents return value of internal header auth + * @typedef {{ + * authenticationResult: AuthenticationResult, + * headerNotRecognized?: boolean + * }} HeaderAuthAttempt + */ + /** * Checks the error returned by Elasticsearch as the result of `authenticate` call and returns `true` if request * has been rejected because of expired token, otherwise returns `false`. @@ -74,7 +82,14 @@ export class SAMLAuthenticationProvider { async authenticate(request, state) { this._options.log(['debug', 'security', 'saml'], `Trying to authenticate user request to ${request.url.path}.`); - let authenticationResult = await this._authenticateViaHeader(request); + let { + authenticationResult, + headerNotRecognized, // eslint-disable-line prefer-const + } = await this._authenticateViaHeader(request); + if (headerNotRecognized) { + return authenticationResult; + } + if (state && authenticationResult.notHandled()) { authenticationResult = await this._authenticateViaState(request, state); if (authenticationResult.failed() && isAccessTokenExpiredError(authenticationResult.error)) { @@ -98,7 +113,7 @@ export class SAMLAuthenticationProvider { * Validates whether request contains `Bearer ***` Authorization header and just passes it * forward to Elasticsearch backend. * @param {Hapi.Request} request HapiJS request instance. - * @returns {Promise.} + * @returns {Promise.} * @private */ async _authenticateViaHeader(request) { @@ -107,19 +122,18 @@ export class SAMLAuthenticationProvider { const authorization = request.headers.authorization; if (!authorization) { this._options.log(['debug', 'security', 'saml'], 'Authorization header is not presented.'); - return AuthenticationResult.notHandled(); + return { + authenticationResult: AuthenticationResult.notHandled() + }; } const authenticationSchema = authorization.split(/\s+/)[0]; if (authenticationSchema.toLowerCase() !== 'bearer') { this._options.log(['debug', 'security', 'saml'], `Unsupported authentication schema: ${authenticationSchema}`); - - // It's essential that we fail if non-empty, but unsupported authentication schema - // is provided to allow authenticator to consult other authentication providers - // that may support that schema. - return AuthenticationResult.failed( - Boom.badRequest(`Unsupported authentication schema: ${authenticationSchema}`) - ); + return { + authenticationResult: AuthenticationResult.notHandled(), + headerNotRecognized: true + }; } try { @@ -130,10 +144,14 @@ export class SAMLAuthenticationProvider { this._options.log(['debug', 'security', 'saml'], 'Request has been authenticated via header.'); - return AuthenticationResult.succeeded(user); + return { + authenticationResult: AuthenticationResult.succeeded(user) + }; } catch(err) { this._options.log(['debug', 'security', 'saml'], `Failed to authenticate request via header: ${err.message}`); - return AuthenticationResult.failed(err); + return { + authenticationResult: AuthenticationResult.failed(err) + }; } } diff --git a/x-pack/test/api_integration/apis/security/basic_login.js b/x-pack/test/api_integration/apis/security/basic_login.js index 2ea7c347e5d60..df99c38dca381 100644 --- a/x-pack/test/api_integration/apis/security/basic_login.js +++ b/x-pack/test/api_integration/apis/security/basic_login.js @@ -189,7 +189,7 @@ export default function ({ getService }) { .set('kbn-xsrf', 'xxx') .set('Authorization', 'Bearer AbCdEf') .set('Cookie', sessionCookie.cookieString()) - .expect(400); + .expect(401); expect(apiResponse.headers['set-cookie']).to.be(undefined); }); diff --git a/x-pack/test/saml_api_integration/apis/security/saml_login.js b/x-pack/test/saml_api_integration/apis/security/saml_login.js index 53550d1914bc8..029190048ca0e 100644 --- a/x-pack/test/saml_api_integration/apis/security/saml_login.js +++ b/x-pack/test/saml_api_integration/apis/security/saml_login.js @@ -275,7 +275,7 @@ export default function ({ getService }) { .set('kbn-xsrf', 'xxx') .set('Authorization', 'Basic AbCdEf') .set('Cookie', sessionCookie.cookieString()) - .expect(400); + .expect(401); expect(apiResponse.headers['set-cookie']).to.be(undefined); });