maxmustermann/kibana
1
0
Fork 0
Code Issues Pull requests Projects Releases Wiki Activity

Properly validate current user password during password change. (#43447)

Browse source
This commit is contained in:
Aleh Zasypkin 2019-08-16 20:58:55 +02:00 committed by GitHub
parent b8303e4cb1
commit 36b1760481
No known key found for this signature in database
GPG key ID: 4AEE18F83AFDEB23
11 changed files with 266 additions and 169 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

112
x-pack/legacy/plugins/security/server/routes/api/v1/__tests__/users.js
View file

@ -10,7 +10,7 @@ import sinon from 'sinon';
import { serverFixture } from '../../../../lib/__tests__/__fixtures__/server';
import { requestFixture } from '../../../../lib/__tests__/__fixtures__/request';
import { AuthenticationResult, BasicCredentials } from '../../../../../../../../plugins/security/server';
import { AuthenticationResult } from '../../../../../../../../plugins/security/server';
import { initUsersApi } from '../users';
import * as ClientShield from '../../../../../../../server/lib/get_client_shield';
import { KibanaRequest } from '../../../../../../../../../src/core/server';
@ -71,35 +71,31 @@ describe('User routes', () => {
});
describe('own password', () => {
let getUserStub;
beforeEach(() => {
request.params.username = request.auth.credentials.username;
getUserStub = serverStub.plugins.security.getUser
loginStub = loginStub
.withArgs(
sinon.match(BasicCredentials.decorateRequest(request, 'user', 'old-password'))
);
sinon.match.instanceOf(KibanaRequest),
{ provider: 'basic', value: { username: 'user', password: 'old-password' }, stateless: true }
)
.resolves(AuthenticationResult.succeeded({}));
});
it('returns 401 if old password is wrong.', async () => {
getUserStub.returns(Promise.reject(new Error('Something went wrong.')));
loginStub.resolves(AuthenticationResult.failed(new Error('Something went wrong.')));
return changePasswordRoute
.handler(request)
.catch((response) => {
sinon.assert.notCalled(clusterStub.callWithRequest);
expect(response.isBoom).to.be(true);
expect(response.output.payload).to.eql({
statusCode: 401,
error: 'Unauthorized',
message: 'Something went wrong.'
});
});
const response = await changePasswordRoute.handler(request);
sinon.assert.notCalled(clusterStub.callWithRequest);
expect(response.isBoom).to.be(true);
expect(response.output.payload).to.eql({
statusCode: 401,
error: 'Unauthorized',
message: 'Something went wrong.'
});
});
it('returns 401 if user can authenticate with new password.', async () => {
getUserStub.returns(Promise.resolve({}));
loginStub
.withArgs(
sinon.match.instanceOf(KibanaRequest),
@ -107,24 +103,22 @@ describe('User routes', () => {
)
.resolves(AuthenticationResult.failed(new Error('Something went wrong.')));
return changePasswordRoute
.handler(request)
.catch((response) => {
sinon.assert.calledOnce(clusterStub.callWithRequest);
sinon.assert.calledWithExactly(
clusterStub.callWithRequest,
sinon.match.same(request),
'shield.changePassword',
{ username: 'user', body: { password: 'new-password' } }
);
const response = await changePasswordRoute.handler(request);
expect(response.isBoom).to.be(true);
expect(response.output.payload).to.eql({
statusCode: 401,
error: 'Unauthorized',
message: 'Something went wrong.'
});
});
sinon.assert.calledOnce(clusterStub.callWithRequest);
sinon.assert.calledWithExactly(
clusterStub.callWithRequest,
sinon.match.same(request),
'shield.changePassword',
{ username: 'user', body: { password: 'new-password' } }
);
expect(response.isBoom).to.be(true);
expect(response.output.payload).to.eql({
statusCode: 401,
error: 'Unauthorized',
message: 'Something went wrong.'
});
});
it('returns 500 if password update request fails.', async () => {
@ -134,23 +128,19 @@ describe('User routes', () => {
'shield.changePassword',
{ username: 'user', body: { password: 'new-password' } }
)
.returns(Promise.reject(new Error('Request failed.')));
.rejects(new Error('Request failed.'));
return changePasswordRoute
.handler(request)
.catch((response) => {
expect(response.isBoom).to.be(true);
expect(response.output.payload).to.eql({
statusCode: 500,
error: 'Internal Server Error',
message: 'An internal server error occurred'
});
});
const response = await changePasswordRoute.handler(request);
expect(response.isBoom).to.be(true);
expect(response.output.payload).to.eql({
statusCode: 500,
error: 'Internal Server Error',
message: 'An internal server error occurred'
});
});
it('successfully changes own password if provided old password is correct.', async () => {
getUserStub.returns(Promise.resolve({}));
loginStub
.withArgs(
sinon.match.instanceOf(KibanaRequest),
@ -186,19 +176,17 @@ describe('User routes', () => {
)
.returns(Promise.reject(new Error('Request failed.')));
return changePasswordRoute
.handler(request)
.catch((response) => {
sinon.assert.notCalled(serverStub.plugins.security.getUser);
sinon.assert.notCalled(loginStub);
const response = await changePasswordRoute.handler(request);
expect(response.isBoom).to.be(true);
expect(response.output.payload).to.eql({
statusCode: 500,
error: 'Internal Server Error',
message: 'An internal server error occurred'
});
});
sinon.assert.notCalled(serverStub.plugins.security.getUser);
sinon.assert.notCalled(loginStub);
expect(response.isBoom).to.be(true);
expect(response.output.payload).to.eql({
statusCode: 500,
error: 'Internal Server Error',
message: 'An internal server error occurred'
});
});
it('successfully changes user password.', async () => {

33
x-pack/legacy/plugins/security/server/routes/api/v1/users.js
View file

@ -10,7 +10,7 @@ import Joi from 'joi';
import { getClient } from '../../../../../../server/lib/get_client_shield';
import { userSchema } from '../../../lib/user_schema';
import { routePreCheckLicense } from '../../../lib/route_pre_check_license';
import { BasicCredentials, wrapError } from '../../../../../../../plugins/security/server';
import { wrapError } from '../../../../../../../plugins/security/server';
import { KibanaRequest } from '../../../../../../../../src/core/server';
export function initUsersApi({ authc: { login }, config }, server) {
@ -88,14 +88,27 @@ export function initUsersApi({ authc: { login }, config }, server) {
const { password, newPassword } = request.payload;
const isCurrentUser = username === request.auth.credentials.username;
// If user tries to change own password, let's check if old password is valid first.
// We should prefer `token` over `basic` if possible.
const providerToLoginWith = config.authc.providers.includes('token')
? 'token'
: 'basic';
// If user tries to change own password, let's check if old password is valid first by trying
// to login.
if (isCurrentUser) {
try {
await server.plugins.security.getUser(
BasicCredentials.decorateRequest(request, username, password)
);
const authenticationResult = await login(KibanaRequest.from(request), {
provider: providerToLoginWith,
value: { username, password },
// We shouldn't alter authentication state just yet.
stateless: true,
});
if (!authenticationResult.succeeded()) {
return Boom.unauthorized(authenticationResult.error);
}
} catch(err) {
throw Boom.unauthorized(err);
return Boom.unauthorized(err);
}
}
@ -105,21 +118,17 @@ export function initUsersApi({ authc: { login }, config }, server) {
// Now we authenticate user with the new password again updating current session if any.
if (isCurrentUser) {
// We should prefer `token` over `basic` if possible.
const providerToLoginWith = config.authc.providers.includes('token')
? 'token'
: 'basic';
const authenticationResult = await login(KibanaRequest.from(request), {
provider: providerToLoginWith,
value: { username, password: newPassword }
});
if (!authenticationResult.succeeded()) {
throw Boom.unauthorized((authenticationResult.error));
return Boom.unauthorized((authenticationResult.error));
}
}
} catch(err) {
throw wrapError(err);
return wrapError(err);
}
return h.response().code(204);

69
x-pack/plugins/security/server/authentication/authenticator.test.ts
View file

@ -210,6 +210,75 @@ describe('Authenticator', () => {
expect(mockSessionStorage.set).not.toHaveBeenCalled();
expect(mockSessionStorage.clear).toHaveBeenCalled();
});
describe('stateless login', () => {
it('does not create session even if authentication provider returns state', async () => {
const user = mockAuthenticatedUser();
const request = httpServerMock.createKibanaRequest();
const authorization = `Basic ${Buffer.from('foo:bar').toString('base64')}`;
mockBasicAuthenticationProvider.login.mockResolvedValue(
AuthenticationResult.succeeded(user, { state: { authorization } })
);
const authenticationResult = await authenticator.login(request, {
provider: 'basic',
value: {},
stateless: true,
});
expect(authenticationResult.succeeded()).toBe(true);
expect(authenticationResult.user).toEqual(user);
expect(mockBasicAuthenticationProvider.login).toHaveBeenCalledWith(request, {}, null);
expect(mockSessionStorage.get).not.toHaveBeenCalled();
expect(mockSessionStorage.set).not.toHaveBeenCalled();
expect(mockSessionStorage.clear).not.toHaveBeenCalled();
});
it('does not clear session even if provider asked to do so.', async () => {
const user = mockAuthenticatedUser();
const request = httpServerMock.createKibanaRequest();
mockBasicAuthenticationProvider.login.mockResolvedValue(
AuthenticationResult.succeeded(user, { state: null })
);
const authenticationResult = await authenticator.login(request, {
provider: 'basic',
value: {},
stateless: true,
});
expect(authenticationResult.succeeded()).toBe(true);
expect(authenticationResult.user).toEqual(user);
expect(mockBasicAuthenticationProvider.login).toHaveBeenCalledWith(request, {}, null);
expect(mockSessionStorage.get).not.toHaveBeenCalled();
expect(mockSessionStorage.set).not.toHaveBeenCalled();
expect(mockSessionStorage.clear).not.toHaveBeenCalled();
});
it('does not clear session even if provider failed with 401.', async () => {
const request = httpServerMock.createKibanaRequest();
const failureReason = Boom.unauthorized();
mockBasicAuthenticationProvider.login.mockResolvedValue(
AuthenticationResult.failed(failureReason)
);
const authenticationResult = await authenticator.login(request, {
provider: 'basic',
value: {},
stateless: true,
});
expect(authenticationResult.failed()).toBe(true);
expect(authenticationResult.error).toBe(failureReason);
expect(mockBasicAuthenticationProvider.login).toHaveBeenCalledWith(request, {}, null);
expect(mockSessionStorage.get).not.toHaveBeenCalled();
expect(mockSessionStorage.set).not.toHaveBeenCalled();
expect(mockSessionStorage.clear).not.toHaveBeenCalled();
});
});
});
describe('`authenticate` method', () => {

11
x-pack/plugins/security/server/authentication/authenticator.ts
View file

@ -66,6 +66,13 @@ export interface ProviderLoginAttempt {
* Login attempt can have any form and defined by the specific provider.
*/
value: unknown;
/**
* Indicates whether login attempt should be performed in a "stateless" manner. If `true` provider
* performing login will neither be able to retrieve or update existing state if any nor persist
* any new state it may produce as a result of the login attempt. It's `false` by default.
*/
stateless?: boolean;
}
export interface AuthenticatorOptions {
@ -220,7 +227,7 @@ export class Authenticator {
// If we detect an existing session that belongs to a different provider than the one requested
// to perform a login we should clear such session.
let existingSession = await this.getSessionValue(sessionStorage);
let existingSession = attempt.stateless ? null : await this.getSessionValue(sessionStorage);
if (existingSession && existingSession.provider !== attempt.provider) {
this.logger.debug(
`Clearing existing session of another ("${existingSession.provider}") provider.`
@ -247,7 +254,7 @@ export class Authenticator {
(authenticationResult.failed() && getErrorStatusCode(authenticationResult.error) === 401);
if (existingSession && shouldClearSession) {
sessionStorage.clear();
} else if (authenticationResult.shouldUpdateState()) {
} else if (!attempt.stateless && authenticationResult.shouldUpdateState()) {
sessionStorage.set({
state: authenticationResult.state,
provider: attempt.provider,

2
x-pack/plugins/security/server/authentication/index.ts
View file

@ -20,7 +20,7 @@ export { canRedirectRequest } from './can_redirect_request';
export { Authenticator, ProviderLoginAttempt } from './authenticator';
export { AuthenticationResult } from './authentication_result';
export { DeauthenticationResult } from './deauthentication_result';
export { BasicCredentials, OIDCAuthenticationFlow } from './providers';
export { OIDCAuthenticationFlow } from './providers';
interface SetupAuthenticationParams {
core: CoreSetup;

48
x-pack/plugins/security/server/authentication/providers/basic.test.ts
View file

@ -10,18 +10,10 @@ import { httpServerMock } from '../../../../../../src/core/server/mocks';
import { mockAuthenticatedUser } from '../../../common/model/authenticated_user.mock';
import { mockAuthenticationProviderOptions, mockScopedClusterClient } from './base.mock';
import { BasicAuthenticationProvider, BasicCredentials } from './basic';
import { BasicAuthenticationProvider } from './basic';
function generateAuthorizationHeader(username: string, password: string) {
const {
headers: { authorization },
} = BasicCredentials.decorateRequest(
{ headers: {} as Record<string, string> },
username,
password
);
return authorization as string;
return `Basic ${Buffer.from(`${username}:${password}`).toString('base64')}`;
}
describe('BasicAuthenticationProvider', () => {
@ -215,40 +207,4 @@ describe('BasicAuthenticationProvider', () => {
);
});
});
describe('BasicCredentials', () => {
it('`decorateRequest` fails if username or password is not provided.', () => {
expect(() =>
BasicCredentials.decorateRequest(undefined as any, undefined as any, undefined as any)
).toThrowError('Request should be a valid object');
expect(() =>
BasicCredentials.decorateRequest({} as any, undefined as any, undefined as any)
).toThrowError('Username should be a valid non-empty string');
expect(() => BasicCredentials.decorateRequest({} as any, '', undefined as any)).toThrowError(
'Username should be a valid non-empty string'
);
expect(() => BasicCredentials.decorateRequest({} as any, '', '')).toThrowError(
'Username should be a valid non-empty string'
);
expect(() => BasicCredentials.decorateRequest({} as any, 'username', '')).toThrowError(
'Password should be a valid non-empty string'
);
expect(() => BasicCredentials.decorateRequest({} as any, '', 'password')).toThrowError(
'Username should be a valid non-empty string'
);
});
it('`decorateRequest` correctly sets authorization header.', () => {
const oneRequest = { headers: {} as Record<string, string> };
const anotherRequest = { headers: { authorization: 'Basic ***' } };
BasicCredentials.decorateRequest(oneRequest, 'one-user', 'one-password');
BasicCredentials.decorateRequest(anotherRequest, 'another-user', 'another-password');
expect(oneRequest.headers.authorization).toBe('Basic b25lLXVzZXI6b25lLXBhc3N3b3Jk');
expect(anotherRequest.headers.authorization).toBe(
'Basic YW5vdGhlci11c2VyOmFub3RoZXItcGFzc3dvcmQ='
);
});
});
});

49
x-pack/plugins/security/server/authentication/providers/basic.ts
View file

@ -4,49 +4,12 @@
* you may not use this file except in compliance with the Elastic License.
*/
/* eslint-disable max-classes-per-file */
import { FakeRequest, KibanaRequest } from '../../../../../../src/core/server';
import { KibanaRequest } from '../../../../../../src/core/server';
import { canRedirectRequest } from '../can_redirect_request';
import { AuthenticationResult } from '../authentication_result';
import { DeauthenticationResult } from '../deauthentication_result';
import { BaseAuthenticationProvider } from './base';
/**
* Utility class that knows how to decorate request with proper Basic authentication headers.
*/
export class BasicCredentials {
/**
* Takes provided `username` and `password`, transforms them into proper `Basic ***` authorization
* header and decorates passed request with it.
* @param request Request instance.
* @param username User name.
* @param password User password.
*/
public static decorateRequest<T extends KibanaRequest | FakeRequest>(
request: T,
username: string,
password: string
) {
const typeOfRequest = typeof request;
if (!request || typeOfRequest !== 'object') {
throw new Error('Request should be a valid object.');
}
if (!username || typeof username !== 'string') {
throw new Error('Username should be a valid non-empty string.');
}
if (!password || typeof password !== 'string') {
throw new Error('Password should be a valid non-empty string.');
}
const basicCredentials = Buffer.from(`${username}:${password}`).toString('base64');
request.headers.authorization = `Basic ${basicCredentials}`;
return request;
}
}
/**
* Describes the parameters that are required by the provider to process the initial login request.
*/
@ -84,13 +47,11 @@ export class BasicAuthenticationProvider extends BaseAuthenticationProvider {
) {
this.logger.debug('Trying to perform a login.');
try {
const { headers: authHeaders } = BasicCredentials.decorateRequest(
{ headers: {} },
username,
password
);
const authHeaders = {
authorization: `Basic ${Buffer.from(`${username}:${password}`).toString('base64')}`,
};
try {
const user = await this.getUser(request, authHeaders);
this.logger.debug('Login has been successfully performed.');

2
x-pack/plugins/security/server/authentication/providers/index.ts
View file

@ -9,7 +9,7 @@ export {
AuthenticationProviderOptions,
AuthenticationProviderSpecificOptions,
} from './base';
export { BasicAuthenticationProvider, BasicCredentials } from './basic';
export { BasicAuthenticationProvider } from './basic';
export { KerberosAuthenticationProvider } from './kerberos';
export { SAMLAuthenticationProvider, isSAMLRequestQuery } from './saml';
export { TokenAuthenticationProvider } from './token';

1
x-pack/plugins/security/server/index.ts
View file

@ -16,7 +16,6 @@ export { wrapError } from './errors';
export {
canRedirectRequest,
AuthenticationResult,
BasicCredentials,
DeauthenticationResult,
OIDCAuthenticationFlow,
} from './authentication';

107
x-pack/test/api_integration/apis/security/change_password.ts Normal file
View file

@ -0,0 +1,107 @@
/*
* Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one
* or more contributor license agreements. Licensed under the Elastic License;
* you may not use this file except in compliance with the Elastic License.
*/
import { Cookie, cookie } from 'request';
import { FtrProviderContext } from '../../ftr_provider_context';
import { SecurityService } from '../../../common/services/security';
export default function({ getService }: FtrProviderContext) {
const supertest = getService('supertestWithoutAuth');
const security: SecurityService = getService('security');
const mockUserName = 'test-user';
const mockUserPassword = 'test-password';
describe('Change password', () => {
let sessionCookie: Cookie;
beforeEach(async () => {
await security.user.create(mockUserName, { password: mockUserPassword, roles: [] });
const loginResponse = await supertest
.post('/api/security/v1/login')
.set('kbn-xsrf', 'xxx')
.send({ username: mockUserName, password: mockUserPassword })
.expect(204);
sessionCookie = cookie(loginResponse.headers['set-cookie'][0])!;
});
afterEach(async () => await security.user.delete(mockUserName));
it('should reject password change if current password is wrong', async () => {
const wrongPassword = `wrong-${mockUserPassword}`;
const newPassword = `xxx-${mockUserPassword}-xxx`;
await supertest
.post(`/api/security/v1/users/${mockUserName}/password`)
.set('kbn-xsrf', 'xxx')
.set('Cookie', sessionCookie.cookieString())
.send({ password: wrongPassword, newPassword })
.expect(401);
// Let's check that we can't login with wrong password, just in case.
await supertest
.post('/api/security/v1/login')
.set('kbn-xsrf', 'xxx')
.send({ username: mockUserName, password: wrongPassword })
.expect(401);
// Let's check that we can't login with the password we were supposed to set.
await supertest
.post('/api/security/v1/login')
.set('kbn-xsrf', 'xxx')
.send({ username: mockUserName, password: newPassword })
.expect(401);
// And can login with the current password.
await supertest
.post('/api/security/v1/login')
.set('kbn-xsrf', 'xxx')
.send({ username: mockUserName, password: mockUserPassword })
.expect(204);
});
it('should allow password change if current password is correct', async () => {
const newPassword = `xxx-${mockUserPassword}-xxx`;
const passwordChangeResponse = await supertest
.post(`/api/security/v1/users/${mockUserName}/password`)
.set('kbn-xsrf', 'xxx')
.set('Cookie', sessionCookie.cookieString())
.send({ password: mockUserPassword, newPassword })
.expect(204);
const newSessionCookie = cookie(passwordChangeResponse.headers['set-cookie'][0])!;
// Let's check that previous cookie isn't valid anymore.
await supertest
.get('/api/security/v1/me')
.set('kbn-xsrf', 'xxx')
.set('Cookie', sessionCookie.cookieString())
.expect(401);
// And that we can't login with the old password.
await supertest
.post('/api/security/v1/login')
.set('kbn-xsrf', 'xxx')
.send({ username: mockUserName, password: mockUserPassword })
.expect(401);
// But new cookie should be valid.
await supertest
.get('/api/security/v1/me')
.set('kbn-xsrf', 'xxx')
.set('Cookie', newSessionCookie.cookieString())
.expect(200);
// And that we can login with new credentials.
await supertest
.post('/api/security/v1/login')
.set('kbn-xsrf', 'xxx')
.send({ username: mockUserName, password: newPassword })
.expect(204);
});
});
}

1
x-pack/test/api_integration/apis/security/index.js
View file

@ -10,6 +10,7 @@ export default function ({ loadTestFile }) {
loadTestFile(require.resolve('./basic_login'));
loadTestFile(require.resolve('./builtin_es_privileges'));
loadTestFile(require.resolve('./change_password'));
loadTestFile(require.resolve('./index_fields'));
loadTestFile(require.resolve('./roles'));
loadTestFile(require.resolve('./privileges'));