Skip to content
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

RBAC - SecurityAuditLogger #19571

Merged
merged 9 commits into from
Jun 1, 2018
7 changes: 7 additions & 0 deletions x-pack/plugins/security/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,8 @@ import { registerPrivilegesWithCluster } from './server/lib/privileges';
import { createDefaultRoles } from './server/lib/authorization/create_default_roles';
import { initPrivilegesApi } from './server/routes/api/v1/privileges';
import { hasPrivilegesWithServer } from './server/lib/authorization/has_privileges';
import { SecurityAuditLogger } from './server/lib/audit_logger';
import { AuditLogger } from '../../server/lib/audit_logger';

export const security = (kibana) => new kibana.Plugin({
id: 'security',
Expand Down Expand Up @@ -51,6 +53,9 @@ export const security = (kibana) => new kibana.Plugin({
`may contain alphanumeric characters (a-z, A-Z, 0-9), underscores and hyphens`
),
}).default(),
audit: Joi.object({
enabled: Joi.boolean().default(false)
}).default(),
}).default();
},

Expand Down Expand Up @@ -98,6 +103,8 @@ export const security = (kibana) => new kibana.Plugin({
await createDefaultRoles(server);
});

server.expose('auditLogger', new SecurityAuditLogger(server.config(), new AuditLogger(server, 'security')));

// Register a function that is called whenever the xpack info changes,
// to re-compute the license check results for this plugin
xpackMainPlugin.info.feature(this.id).registerLicenseCheckResultsGenerator(checkLicense);
Expand Down
46 changes: 46 additions & 0 deletions x-pack/plugins/security/server/lib/audit_logger.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,46 @@
/*
* 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.
*/

export class SecurityAuditLogger {
constructor(config, auditLogger) {
this._enabled = config.get('xpack.security.audit.enabled');
this._auditLogger = auditLogger;
}

savedObjectsAuthorizationFailure(username, action, types, missing) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is args intentionally omitted from the failure case?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It was intentional, but it might not be what we really want. My reasoning was that in the case where we're denying the user from performing an action, the specifics of that action aren't relevant if they weren't able to perform it. So we're only logging the information that we used to deny the request.

Copy link
Member

@legrego legrego May 31, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's a fair argument. My counter-argument is:

  1. Adding args will make parsing the failure entries more consistent with the success entries
  2. For investigating incidents, it could be useful to know what saved objects (or request payloads) the user was attempting to send, malicious or otherwise.

I don't have a strong opinion either way here, so I'm fine with whatever you decide. This is something that's easily added later on if we want it.

edit: I missed an args/argument pun opportunity here

if (!this._enabled) {
return;
}

this._auditLogger.log(
'saved_objects_authorization_failure',
`${username} unauthorized to ${action} ${types.join(',')}, missing ${missing.join(',')}`,
{
username,
action,
types,
missing
}
);
}

savedObjectsAuthorizationSuccess(username, action, types, args) {
if (!this._enabled) {
return;
}

this._auditLogger.log(
'saved_objects_authorization_success',
`${username} authorized to ${action} ${types.join(',')}`,
{
username,
action,
types,
args,
}
);
}
}
109 changes: 109 additions & 0 deletions x-pack/plugins/security/server/lib/audit_logger.test.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,109 @@
/*
* 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 { SecurityAuditLogger } from './audit_logger';


const createMockConfig = (settings) => {
const mockConfig = {
get: jest.fn()
};

const defaultSettings = {};

mockConfig.get.mockImplementation(key => {
return key in settings ? settings[key] : defaultSettings[key];
});

return mockConfig;
};

const createMockAuditLogger = () => {
return {
log: jest.fn()
};
};

describe(`#savedObjectsAuthorizationFailure`, () => {
test(`doesn't log anything when xpack.security.audit.enabled is false`, () => {
const config = createMockConfig({
'xpack.security.audit.enabled': false
});
const auditLogger = createMockAuditLogger();

const securityAuditLogger = new SecurityAuditLogger(config, auditLogger);
securityAuditLogger.savedObjectsAuthorizationFailure();

expect(auditLogger.log).toHaveBeenCalledTimes(0);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This feels fragile to me. I think it's strange that we have to rely on the securityAuditLogger using a dependency in a very particular way in order to verify that the securityAuditLogger works as intended.

I don't have any recommendations here, but what's your take? Am I overthinking this?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, I've always struggled with this when it comes to writing "unit tests". In this situation, the "unit under test" is specifically the SecurityAuditLogger, so we're mocking out the AuditLogger itself, so we're forced to write the current style of tests where we ensure the security audit logger is calling the correct methods on the audit logger.

The other approach that I've taken before is to test the security audit logger and the audit logger as the "unit under test" and expand the boundaries so we'd be stubbing out the Kibana server itself and ensuring we're calling the appropriate "log" functions on the server itself. If we we were to do this, we'd have duplicate code between each of the implementations of the plugin specific audit loggers that ensure the appropriate tags are added, etc. and when we changed the internal implementation of the base AuditLogger in accordance with the new platform we'd have to rework a lot more tests.

Given the intent of the introduction of the base AuditLogger abstracting away the internal way that tags are used, I think the current approach is the best, but it definitely leaves something to be desired.

});

test('logs via auditLogger when xpack.security.audit.enabled is true', () => {
const config = createMockConfig({
'xpack.security.audit.enabled': true
});
const auditLogger = createMockAuditLogger();
const securityAuditLogger = new SecurityAuditLogger(config, auditLogger);
const username = 'foo-user';
const action = 'foo-action';
const types = [ 'foo-type-1', 'foo-type-2' ];
const missing = [`action:saved-objects/${types[0]}/foo-action`, `action:saved-objects/${types[1]}/foo-action`];

securityAuditLogger.savedObjectsAuthorizationFailure(username, action, types, missing);

expect(auditLogger.log).toHaveBeenCalledWith(
'saved_objects_authorization_failure',
expect.stringContaining(`${username} unauthorized to ${action}`),
{
username,
action,
types,
missing,
}
);
});
});

describe(`#savedObjectsAuthorizationSuccess`, () => {
test(`doesn't log anything when xpack.security.audit.enabled is false`, () => {
const config = createMockConfig({
'xpack.security.audit.enabled': false
});
const auditLogger = createMockAuditLogger();

const securityAuditLogger = new SecurityAuditLogger(config, auditLogger);
securityAuditLogger.savedObjectsAuthorizationSuccess();

expect(auditLogger.log).toHaveBeenCalledTimes(0);
});

test('logs via auditLogger when xpack.security.audit.enabled is true', () => {
const config = createMockConfig({
'xpack.security.audit.enabled': true
});
const auditLogger = createMockAuditLogger();
const securityAuditLogger = new SecurityAuditLogger(config, auditLogger);
const username = 'foo-user';
const action = 'foo-action';
const types = [ 'foo-type-1', 'foo-type-2' ];
const args = {
'foo': 'bar',
'dude': 'yup',
'women': 'yay!',
};

securityAuditLogger.savedObjectsAuthorizationSuccess(username, action, types, args);

expect(auditLogger.log).toHaveBeenCalledWith(
'saved_objects_authorization_success',
expect.stringContaining(`${username} authorized to ${action}`),
{
username,
action,
types,
args,
}
);
});
});
Original file line number Diff line number Diff line change
Expand Up @@ -48,7 +48,8 @@ export function hasPrivilegesWithServer(server) {

return {
success,
missing: missingPrivileges
missing: missingPrivileges,
username: privilegeCheck.username,
};
};
};
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -43,8 +43,9 @@ const createMockServer = ({ settings = {} } = {}) => {
return mockServer;
};

const mockResponse = (hasAllRequested, privileges, application = defaultApplication) => {
const mockResponse = (hasAllRequested, privileges, application = defaultApplication, username = '') => {
mockCallWithRequest.mockImplementationOnce(async () => ({
username: username,
has_all_requested: hasAllRequested,
application: {
[application]: {
Expand Down Expand Up @@ -151,30 +152,50 @@ test(`returns success when has_all_requested`, async () => {
expect(result.success).toBe(true);
});

test(`returns username from has_privileges response when has_all_requested`, async () => {
const mockServer = createMockServer();
const username = 'foo-username';
mockResponse(true, {
[getVersionPrivilege(defaultVersion)]: true,
[getLoginPrivilege()]: true,
foo: true,
}, defaultApplication, username);

const hasPrivilegesWithRequest = hasPrivilegesWithServer(mockServer);
const hasPrivileges = hasPrivilegesWithRequest({});
const result = await hasPrivileges(['foo']);
expect(result.username).toBe(username);
});

test(`returns false success when has_all_requested is false`, async () => {
const mockServer = createMockServer();
mockResponse(false, {
[getVersionPrivilege(defaultVersion)]: true,
[getLoginPrivilege()]: true,
foo: false,
});
mockCallWithRequest.mockImplementationOnce(async () => ({
has_all_requested: false,
application: {
[defaultApplication]: {
[DEFAULT_RESOURCE]: {
foo: false
}
}
}
}));

const hasPrivilegesWithRequest = hasPrivilegesWithServer(mockServer);
const hasPrivileges = hasPrivilegesWithRequest({});
const result = await hasPrivileges(['foo']);
expect(result.success).toBe(false);
});

test(`returns username from has_privileges when has_all_requested is false`, async () => {
const username = 'foo-username';
const mockServer = createMockServer();
mockResponse(false, {
[getVersionPrivilege(defaultVersion)]: true,
[getLoginPrivilege()]: true,
foo: false,
}, defaultApplication, username);

const hasPrivilegesWithRequest = hasPrivilegesWithServer(mockServer);
const hasPrivileges = hasPrivilegesWithRequest({ });
const result = await hasPrivileges(['foo']);
expect(result.username).toBe(username);
});

test(`returns missing privileges`, async () => {
const mockServer = createMockServer();
mockResponse(false, {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -10,10 +10,12 @@
export function secureSavedObjectsClientOptionsBuilder(server, hasPrivilegesWithRequest, options) {
const adminCluster = server.plugins.elasticsearch.getCluster('admin');
const { callWithInternalUser } = adminCluster;
const auditLogger = server.plugins.security.auditLogger;

return {
...options,
callCluster: callWithInternalUser,
hasPrivilegesWithRequest
hasPrivilegesWithRequest,
auditLogger
};
}
Original file line number Diff line number Diff line change
Expand Up @@ -12,60 +12,83 @@ export class SecureSavedObjectsClient {
request,
hasPrivilegesWithRequest,
baseClient,
auditLogger,
} = options;

this.errors = baseClient.errors;

this._client = baseClient;
this._hasPrivileges = hasPrivilegesWithRequest(request);
this._auditLogger = auditLogger;
}

async create(type, attributes = {}, options = {}) {
await this._performAuthorizationCheck(type, 'create');
await this._performAuthorizationCheck(type, 'create', {
type,
attributes,
options,
});

return await this._client.create(type, attributes, options);
}

async bulkCreate(objects, options = {}) {
const types = uniq(objects.map(o => o.type));
await this._performAuthorizationCheck(types, 'create');
await this._performAuthorizationCheck(types, 'create', {
objects,
options,
});

return await this._client.bulkCreate(objects, options);
}

async delete(type, id) {
await this._performAuthorizationCheck(type, 'delete');
await this._performAuthorizationCheck(type, 'delete', {
type,
id,
});

return await this._client.delete(type, id);
}

async find(options = {}) {
await this._performAuthorizationCheck(options.type, 'search');
await this._performAuthorizationCheck(options.type, 'search', {
options,
});

return await this._client.find(options);
}

async bulkGet(objects = []) {
for (const object of objects) {
await this._performAuthorizationCheck(object.type, 'mget');
}
const types = uniq(objects.map(o => o.type));
await this._performAuthorizationCheck(types, 'mget', {
objects,
});

return await this._client.bulkGet(objects);
}

async get(type, id) {
await this._performAuthorizationCheck(type, 'get');
await this._performAuthorizationCheck(type, 'get', {
type,
id,
});

return await this._client.get(type, id);
}

async update(type, id, attributes, options = {}) {
await this._performAuthorizationCheck(type, 'update');
await this._performAuthorizationCheck(type, 'update', {
type,
id,
attributes,
options,
});

return await this._client.update(type, id, attributes, options);
}

async _performAuthorizationCheck(typeOrTypes, action) {
async _performAuthorizationCheck(typeOrTypes, action, args) {
const types = Array.isArray(typeOrTypes) ? typeOrTypes : [typeOrTypes];
const actions = types.map(type => `action:saved-objects/${type}/${action}`);

Expand All @@ -77,7 +100,10 @@ export class SecureSavedObjectsClient {
throw this._client.errors.decorateGeneralError(error, reason);
}

if (!result.success) {
if (result.success) {
this._auditLogger.savedObjectsAuthorizationSuccess(result.username, action, types, args);
} else {
this._auditLogger.savedObjectsAuthorizationFailure(result.username, action, types, result.missing);
const msg = `Unable to ${action} ${types.join(',')}, missing ${result.missing.join(',')}`;
throw this._client.errors.decorateForbiddenError(new Error(msg));
}
Expand Down