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

Add audit logging to space deletion #123378

Merged
merged 13 commits into from
Jan 25, 2022
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,7 @@ describe('audit_logger', () => {
describe('log function', () => {
const mockLogger: jest.Mocked<AuditLogger> = {
log: jest.fn(),
enabled: true,
};

let logger: AuthorizationAuditLogger;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@ describe('authorization', () => {
request = httpServerMock.createKibanaRequest();
mockLogger = {
log: jest.fn(),
enabled: true,
};
});

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -46,6 +46,7 @@ const fakeRequest = {

const auditLogger = {
log: jest.fn(),
enabled: true,
} as jest.Mocked<AuditLogger>;

describe('AlertsClientFactory', () => {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@ const alertingAuthMock = alertingAuthorizationMock.create();
const esClientMock = elasticsearchClientMock.createElasticsearchClient();
const auditLogger = {
log: jest.fn(),
enabled: true,
} as jest.Mocked<AuditLogger>;

const alertsClientParams: jest.Mocked<ConstructorOptions> = {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@ const alertingAuthMock = alertingAuthorizationMock.create();
const esClientMock = elasticsearchClientMock.createElasticsearchClient();
const auditLogger = {
log: jest.fn(),
enabled: true,
} as jest.Mocked<AuditLogger>;

const alertsClientParams: jest.Mocked<ConstructorOptions> = {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@ const alertingAuthMock = alertingAuthorizationMock.create();
const esClientMock = elasticsearchClientMock.createElasticsearchClient();
const auditLogger = {
log: jest.fn(),
enabled: true,
} as jest.Mocked<AuditLogger>;

const alertsClientParams: jest.Mocked<ConstructorOptions> = {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@ const alertingAuthMock = alertingAuthorizationMock.create();
const esClientMock = elasticsearchClientMock.createElasticsearchClient();
const auditLogger = {
log: jest.fn(),
enabled: true,
} as jest.Mocked<AuditLogger>;

const alertsClientParams: jest.Mocked<ConstructorOptions> = {
Expand Down
6 changes: 0 additions & 6 deletions x-pack/plugins/security/server/audit/audit_events.ts
Original file line number Diff line number Diff line change
Expand Up @@ -218,8 +218,6 @@ export enum SavedObjectAction {
UPDATE = 'saved_object_update',
DELETE = 'saved_object_delete',
FIND = 'saved_object_find',
ADD_TO_SPACES = 'saved_object_add_to_spaces',
DELETE_FROM_SPACES = 'saved_object_delete_from_spaces',
thomheymann marked this conversation as resolved.
Show resolved Hide resolved
REMOVE_REFERENCES = 'saved_object_remove_references',
OPEN_POINT_IN_TIME = 'saved_object_open_point_in_time',
CLOSE_POINT_IN_TIME = 'saved_object_close_point_in_time',
Expand All @@ -236,8 +234,6 @@ const savedObjectAuditVerbs: Record<SavedObjectAction, VerbsTuple> = {
saved_object_update: ['update', 'updating', 'updated'],
saved_object_delete: ['delete', 'deleting', 'deleted'],
saved_object_find: ['access', 'accessing', 'accessed'],
saved_object_add_to_spaces: ['update', 'updating', 'updated'],
saved_object_delete_from_spaces: ['update', 'updating', 'updated'],
saved_object_open_point_in_time: [
'open point-in-time',
'opening point-in-time',
Expand Down Expand Up @@ -272,8 +268,6 @@ const savedObjectAuditTypes: Record<SavedObjectAction, EcsEventType> = {
saved_object_update: 'change',
saved_object_delete: 'deletion',
saved_object_find: 'access',
saved_object_add_to_spaces: 'change',
saved_object_delete_from_spaces: 'change',
saved_object_open_point_in_time: 'creation',
saved_object_close_point_in_time: 'deletion',
saved_object_remove_references: 'change',
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -68,6 +68,7 @@ describe('#setup', () => {
Object {
"asScoped": [Function],
"withoutRequest": Object {
"enabled": true,
"log": [Function],
},
}
Expand Down
14 changes: 12 additions & 2 deletions x-pack/plugins/security/server/audit/audit_service.ts
Original file line number Diff line number Diff line change
Expand Up @@ -49,6 +49,14 @@ export interface AuditLogger {
* ```
*/
log: (event: AuditEvent | undefined) => void;

/**
* Indicates whether audit logging is enabled or not.
*
* Useful for skipping resource-intense operations that don't need to be performed when audit
* logging is disabled.
*/
readonly enabled: boolean;
}

export interface AuditServiceSetup {
Expand Down Expand Up @@ -122,7 +130,8 @@ export class AuditService {
);

// Record feature usage at a regular interval if enabled and license allows
if (config.enabled && config.appender) {
const enabled = !!(config.enabled && config.appender);
if (enabled) {
license.features$.subscribe((features) => {
clearInterval(this.usageIntervalId!);
if (features.allowAuditLogging) {
Expand Down Expand Up @@ -169,6 +178,7 @@ export class AuditService {
trace: { id: request.id },
});
},
enabled,
});

http.registerOnPostAuth((request, response, t) => {
Expand All @@ -180,7 +190,7 @@ export class AuditService {

return {
asScoped,
withoutRequest: { log },
withoutRequest: { log, enabled },
};
}

Expand Down
2 changes: 2 additions & 0 deletions x-pack/plugins/security/server/audit/index.mock.ts
Original file line number Diff line number Diff line change
Expand Up @@ -13,9 +13,11 @@ export const auditServiceMock = {
getLogger: jest.fn(),
asScoped: jest.fn().mockReturnValue({
log: jest.fn(),
enabled: true,
}),
withoutRequest: {
log: jest.fn(),
enabled: true,
},
} as jest.Mocked<ReturnType<AuditService['setup']>>;
},
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -263,6 +263,7 @@ describe('Authenticator', () => {
let mockSessVal: SessionValue;
const auditLogger = {
log: jest.fn(),
enabled: true,
};

beforeEach(() => {
Expand Down Expand Up @@ -1093,6 +1094,7 @@ describe('Authenticator', () => {
let mockSessVal: SessionValue;
const auditLogger = {
log: jest.fn(),
enabled: true,
};

beforeEach(() => {
Expand Down Expand Up @@ -1824,6 +1826,7 @@ describe('Authenticator', () => {
let mockSessVal: SessionValue;
const auditLogger = {
log: jest.fn(),
enabled: true,
};

beforeEach(() => {
Expand Down Expand Up @@ -1960,6 +1963,7 @@ describe('Authenticator', () => {
let mockSessionValue: SessionValue;
const auditLogger = {
log: jest.fn(),
enabled: true,
};

beforeEach(() => {
Expand Down
1 change: 1 addition & 0 deletions x-pack/plugins/security/server/plugin.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -68,6 +68,7 @@ describe('Security Plugin', () => {
"audit": Object {
"asScoped": [Function],
"withoutRequest": Object {
"enabled": false,
"log": [Function],
},
},
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,11 @@
import { mockEnsureAuthorized } from './secure_spaces_client_wrapper.test.mocks';

import { deepFreeze } from '@kbn/std';
import type { EcsEventOutcome, SavedObjectsClientContract } from 'src/core/server';
import type {
EcsEventOutcome,
SavedObjectsClientContract,
SavedObjectsFindResponse,
} from 'src/core/server';
import { SavedObjectsErrorHelpers } from 'src/core/server';
import { httpServerMock } from 'src/core/server/mocks';

Expand Down Expand Up @@ -63,6 +67,31 @@ const setup = ({ securityEnabled = false }: Opts = {}) => {
return space;
});

baseClient.createSavedObjectFinder.mockImplementation(() => ({
async *find() {
yield {
saved_objects: [
{
namespaces: undefined,
type: 'dashboard',
id: '1',
},
{
namespaces: ['existing_space'],
type: 'dashboard',
id: '2',
},
{
namespaces: ['default', 'existing_space'],
type: 'dashboard',
id: '3',
},
],
} as SavedObjectsFindResponse<unknown, unknown>;
},
async close() {},
}));

const authorization = authorizationMock.create({
version: 'unit-test',
applicationName: 'kibana',
Expand Down Expand Up @@ -602,7 +631,7 @@ describe('SecureSpacesClientWrapper', () => {
});
});

test(`throws a forbidden error when unauthorized`, async () => {
it(`throws a forbidden error when unauthorized`, async () => {
const username = 'some_user';

const { wrapper, baseClient, authorization, auditLogger, request } = setup({
Expand Down Expand Up @@ -637,7 +666,7 @@ describe('SecureSpacesClientWrapper', () => {
});
});

it('deletes the space when authorized', async () => {
it('deletes the space with all saved objects when authorized', async () => {
const username = 'some_user';

const { wrapper, baseClient, authorization, auditLogger, request } = setup({
Expand Down Expand Up @@ -669,6 +698,18 @@ describe('SecureSpacesClientWrapper', () => {
type: 'space',
id: space.id,
});
expectAuditEvent(auditLogger, SavedObjectAction.DELETE, 'unknown', {
type: 'dashboard',
id: '1',
});
expectAuditEvent(auditLogger, SavedObjectAction.DELETE, 'unknown', {
type: 'dashboard',
id: '2',
});
expectAuditEvent(auditLogger, SavedObjectAction.UPDATE_OBJECTS_SPACES, 'unknown', {
type: 'dashboard',
id: '3',
});
});
});

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -246,6 +246,10 @@ export class SecureSpacesClientWrapper implements ISpacesClient {
return this.spacesClient.update(id, space);
}

public createSavedObjectFinder(id: string) {
return this.spacesClient.createSavedObjectFinder(id);
}

public async delete(id: string) {
if (this.useRbac) {
try {
Expand All @@ -265,6 +269,30 @@ export class SecureSpacesClientWrapper implements ISpacesClient {
}
}

// Fetch saved objects to be removed for audit logging
if (this.auditLogger.enabled) {
const finder = this.spacesClient.createSavedObjectFinder(id);
try {
for await (const response of finder.find()) {
response.saved_objects.forEach((savedObject) => {
const isOnlySpace = !savedObject.namespaces || savedObject.namespaces.length === 1;
thomheymann marked this conversation as resolved.
Show resolved Hide resolved
this.auditLogger.log(
savedObjectEvent({
action: isOnlySpace
? SavedObjectAction.DELETE
: SavedObjectAction.UPDATE_OBJECTS_SPACES,
outcome: 'unknown',
savedObject: { type: savedObject.type, id: savedObject.id },
deleteFromSpaces: [id],
})
);
});
}
} finally {
await finder.close();
}
}

this.auditLogger.log(
spaceAuditEvent({
action: SpaceAuditAction.DELETE,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -5,12 +5,15 @@
* 2.0.
*/

import { savedObjectsRepositoryMock } from 'src/core/server/mocks';

import type { Space } from '../../common';
import { DEFAULT_SPACE_ID } from '../../common/constants';
import type { SpacesClient } from './spaces_client';

const createSpacesClientMock = () =>
({
const createSpacesClientMock = () => {
const repositoryMock = savedObjectsRepositoryMock.create();
return {
getAll: jest.fn().mockResolvedValue([
{
id: DEFAULT_SPACE_ID,
Expand All @@ -28,10 +31,11 @@ const createSpacesClientMock = () =>
}),
create: jest.fn().mockImplementation((space: Space) => Promise.resolve(space)),
update: jest.fn().mockImplementation((space: Space) => Promise.resolve(space)),
createSavedObjectFinder: repositoryMock.createPointInTimeFinder,
delete: jest.fn(),
disableLegacyUrlAliases: jest.fn(),
} as unknown as jest.Mocked<SpacesClient>);

} as unknown as jest.Mocked<SpacesClient>;
};
export const spacesClientMock = {
create: createSpacesClientMock,
};
Loading