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.

Sign up for GitHub

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

Jump to bottom

Remove the legacy audit logger #116191

Merged
merged 8 commits into from
Oct 26, 2021
Merged

Remove the legacy audit logger #116191

Show file tree
Hide file tree
Changes from 7 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
d26cde4
Create default audit log appender
legrego Oct 25, 2021
ded04b7
Fix types; remove legacy audit implementation
legrego Oct 25, 2021
344c055
Merge branch 'master' into security/remove-legacy-audit-logger
kibanamachine Oct 25, 2021
a61cbc2
Fix configuration
legrego Oct 26, 2021
21478d7
Fix test
legrego Oct 26, 2021
91ac2b5
Merge branch 'master' of github.com:elastic/kibana into security/remo…
legrego Oct 26, 2021
5df776b
Merge branch 'security/remove-legacy-audit-logger' of github.com:legr…
legrego Oct 26, 2021
f3582b0
Fix config spreading 🤦
legrego Oct 26, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
218 changes: 22 additions & 196 deletions x-pack/plugins/security/server/audit/audit_service.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@
* 2.0.
*/

import { BehaviorSubject, Observable, of } from 'rxjs';
import { Observable, of } from 'rxjs';

import {
coreMock,
Expand All @@ -14,10 +14,9 @@ import {
loggingSystemMock,
} from 'src/core/server/mocks';

import type { SecurityLicenseFeatures } from '../../common/licensing';
import { licenseMock } from '../../common/licensing/index.mock';
import type { ConfigType } from '../config';
import { ConfigSchema } from '../config';
import { ConfigSchema, createConfig } from '../config';
import type { AuditEvent } from './audit_events';
import {
AuditService,
Expand All @@ -28,13 +27,15 @@ import {

jest.useFakeTimers();

const createConfig = (settings: Partial<ConfigType['audit']>) => {
return ConfigSchema.validate({ audit: settings }).audit;
};

const logger = loggingSystemMock.createLogger();
const license = licenseMock.create();
const config = createConfig({ enabled: true });

const createAuditConfig = (settings: Partial<ConfigType['audit']>) => {
return createConfig(ConfigSchema.validate({ audit: settings }), logger, { isTLSEnabled: false })
.audit;
};

const config = createAuditConfig({ enabled: true });
const { logging } = coreMock.createSetup();
const http = httpServiceMock.createSetupContract();
const getCurrentUser = jest.fn().mockReturnValue({ username: 'jdoe', roles: ['admin'] });
Expand Down Expand Up @@ -132,6 +133,7 @@ describe('#setup', () => {
license,
config: {
enabled: false,
appender: undefined,
},
logging,
http,
Expand Down Expand Up @@ -198,6 +200,12 @@ describe('#asScoped', () => {
license,
config: {
enabled: true,
appender: {
type: 'console',
layout: {
type: 'json',
},
},
ignore_filters: [{ actions: ['ACTION'] }],
},
logging,
Expand All @@ -222,6 +230,12 @@ describe('#asScoped', () => {
license,
config: {
enabled: true,
appender: {
type: 'console',
layout: {
type: 'json',
},
},
ignore_filters: [{ actions: ['ACTION'] }],
},
logging,
Expand Down Expand Up @@ -306,22 +320,6 @@ describe('#createLoggingConfig', () => {
expect(loggingConfig.loggers![0].level).toEqual('off');
});

test('sets log level to `off` when appender is not defined', async () => {
const features$ = of({
allowAuditLogging: true,
});

const loggingConfig = await features$
.pipe(
createLoggingConfig({
enabled: true,
})
)
.toPromise();

expect(loggingConfig.loggers![0].level).toEqual('off');
});

test('sets log level to `off` when license does not allow audit logging', async () => {
const features$ = of({
allowAuditLogging: false,
Expand Down Expand Up @@ -563,175 +561,3 @@ describe('#filterEvent', () => {
).toBeFalsy();
});
});

describe('#getLogger', () => {
test('calls the underlying logger with the provided message and requisite tags', () => {
const pluginId = 'foo';

const licenseWithFeatures = licenseMock.create();
licenseWithFeatures.features$ = new BehaviorSubject({
allowLegacyAuditLogging: true,
} as SecurityLicenseFeatures).asObservable();

const auditService = new AuditService(logger).setup({
license: licenseWithFeatures,
config,
logging,
http,
getCurrentUser,
getSpaceId,
getSID,
recordAuditLoggingUsage,
});

const auditLogger = auditService.getLogger(pluginId);

const eventType = 'bar';
const message = 'this is my audit message';
auditLogger.log(eventType, message);

expect(logger.info).toHaveBeenCalledTimes(1);
expect(logger.info).toHaveBeenCalledWith(message, {
eventType,
tags: [pluginId, eventType],
});
});

test('calls the underlying logger with the provided metadata', () => {
const pluginId = 'foo';

const licenseWithFeatures = licenseMock.create();
licenseWithFeatures.features$ = new BehaviorSubject({
allowLegacyAuditLogging: true,
} as SecurityLicenseFeatures).asObservable();

const auditService = new AuditService(logger).setup({
license: licenseWithFeatures,
config,
logging,
http,
getCurrentUser,
getSpaceId,
getSID,
recordAuditLoggingUsage,
});

const auditLogger = auditService.getLogger(pluginId);

const eventType = 'bar';
const message = 'this is my audit message';
const metadata = Object.freeze({
property1: 'value1',
property2: false,
property3: 123,
});
auditLogger.log(eventType, message, metadata);

expect(logger.info).toHaveBeenCalledTimes(1);
expect(logger.info).toHaveBeenCalledWith(message, {
eventType,
tags: [pluginId, eventType],
property1: 'value1',
property2: false,
property3: 123,
});
});

test('does not call the underlying logger if license does not support audit logging', () => {
const pluginId = 'foo';

const licenseWithFeatures = licenseMock.create();
licenseWithFeatures.features$ = new BehaviorSubject({
allowLegacyAuditLogging: false,
} as SecurityLicenseFeatures).asObservable();

const auditService = new AuditService(logger).setup({
license: licenseWithFeatures,
config,
logging,
http,
getCurrentUser,
getSpaceId,
getSID,
recordAuditLoggingUsage,
});

const auditLogger = auditService.getLogger(pluginId);

const eventType = 'bar';
const message = 'this is my audit message';
auditLogger.log(eventType, message);

expect(logger.info).not.toHaveBeenCalled();
});

test('does not call the underlying logger if security audit logging is not enabled', () => {
const pluginId = 'foo';

const licenseWithFeatures = licenseMock.create();
licenseWithFeatures.features$ = new BehaviorSubject({
allowLegacyAuditLogging: true,
} as SecurityLicenseFeatures).asObservable();

const auditService = new AuditService(logger).setup({
license: licenseWithFeatures,
config: createConfig({
enabled: false,
}),
logging,
http,
getCurrentUser,
getSpaceId,
getSID,
recordAuditLoggingUsage,
});

const auditLogger = auditService.getLogger(pluginId);

const eventType = 'bar';
const message = 'this is my audit message';
auditLogger.log(eventType, message);

expect(logger.info).not.toHaveBeenCalled();
});

test('calls the underlying logger after license upgrade', () => {
const pluginId = 'foo';

const licenseWithFeatures = licenseMock.create();

const features$ = new BehaviorSubject({
allowLegacyAuditLogging: false,
} as SecurityLicenseFeatures);

licenseWithFeatures.features$ = features$.asObservable();

const auditService = new AuditService(logger).setup({
license: licenseWithFeatures,
config,
logging,
http,
getCurrentUser,
getSpaceId,
getSID,
recordAuditLoggingUsage,
});

const auditLogger = auditService.getLogger(pluginId);

const eventType = 'bar';
const message = 'this is my audit message';
auditLogger.log(eventType, message);

expect(logger.info).not.toHaveBeenCalled();

// perform license upgrade
features$.next({
allowLegacyAuditLogging: true,
} as SecurityLicenseFeatures);

auditLogger.log(eventType, message);

expect(logger.info).toHaveBeenCalledTimes(1);
});
});
35 changes: 2 additions & 33 deletions x-pack/plugins/security/server/audit/audit_service.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,6 @@
* 2.0.
*/

import type { Subscription } from 'rxjs';
import { distinctUntilKeyChanged, map } from 'rxjs/operators';

import type {
Expand Down Expand Up @@ -58,18 +57,10 @@ interface AuditServiceSetupParams {
}

export class AuditService {
/**
* @deprecated
*/
private licenseFeaturesSubscription?: Subscription;
/**
* @deprecated
*/
private allowLegacyAuditLogging = false;
private ecsLogger: Logger;
private usageIntervalId?: NodeJS.Timeout;

constructor(private readonly logger: Logger) {
constructor(logger: Logger) {
this.ecsLogger = logger.get('ecs');
}

Expand All @@ -83,14 +74,6 @@ export class AuditService {
getSpaceId,
recordAuditLoggingUsage,
}: AuditServiceSetupParams): AuditServiceSetup {
if (config.enabled && !config.appender) {
this.licenseFeaturesSubscription = license.features$.subscribe(
({ allowLegacyAuditLogging }) => {
this.allowLegacyAuditLogging = allowLegacyAuditLogging;
}
);
}

// Configure logging during setup and when license changes
logging.configure(
license.features$.pipe(
Expand Down Expand Up @@ -181,17 +164,7 @@ export class AuditService {
*/
const getLogger = (id?: string): LegacyAuditLogger => {
return {
log: (eventType: string, message: string, data?: Record<string, any>) => {
if (!this.allowLegacyAuditLogging) {
return;
}

this.logger.info(message, {
tags: id ? [id, eventType] : [eventType],
eventType,
...data,
});
},
log: (eventType: string, message: string, data?: Record<string, any>) => {},
Copy link
Member Author

Choose a reason for hiding this comment

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

The legacy audit logger is a no-op now. We should remove the function altogether in a followup. I opted not to do so here because that involves pinging a number of other teams for codeowner's review, and we don't have the luxury of time.

};
};

Expand All @@ -206,10 +179,6 @@ export class AuditService {
}

stop() {
if (this.licenseFeaturesSubscription) {
this.licenseFeaturesSubscription.unsubscribe();
this.licenseFeaturesSubscription = undefined;
}
clearInterval(this.usageIntervalId!);
}
}
Expand Down
Loading