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

Add session id to audit log #85451

Merged
merged 4 commits into from
Dec 14, 2020
Merged

Add session id to audit log #85451

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
f638fc4
Add session id to audit log
thomheymann Dec 9, 2020
7393366
Merge remote-tracking branch 'origin/master' into audit/sessionid
thomheymann Dec 10, 2020
ce576d5
fix naming
thomheymann Dec 10, 2020
eb07214
Merge branch 'master' into audit/sessionid
kibanamachine Dec 14, 2020
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
40 changes: 34 additions & 6 deletions x-pack/plugins/security/server/audit/audit_events.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,8 +8,11 @@ import { KibanaRequest } from 'src/core/server';
import { AuthenticationResult } from '../authentication/authentication_result';

/**
* Audit event schema using ECS format.
* https://www.elastic.co/guide/en/ecs/1.6/index.html
* Audit event schema using ECS format: https://www.elastic.co/guide/en/ecs/1.6/index.html
*
* If you add additional fields to the schema ensure you update the Kibana Filebeat module:
* https://github.com/elastic/beats/tree/master/filebeat/module/kibana
*
* @public
*/
export interface AuditEvent {
Expand Down Expand Up @@ -37,20 +40,45 @@ export interface AuditEvent {
};
kibana?: {
/**
* Current space id of the request.
* The ID of the space associated with this event.
*/
space_id?: string;
/**
* Saved object that was created, changed, deleted or accessed as part of the action.
* The ID of the user session associated with this event. Each login attempt
* results in a unique session id.
*/
session_id?: string;
/**
* Saved object that was created, changed, deleted or accessed as part of this event.
*/
saved_object?: {
type: string;
id: string;
};
/**
* Any additional event specific fields.
* Name of authentication provider associated with a login event.
*/
authentication_provider?: string;
/**
* Type of authentication provider associated with a login event.
*/
authentication_type?: string;
/**
* Name of Elasticsearch realm that has authenticated the user.
*/
authentication_realm?: string;
/**
* Name of Elasticsearch realm where the user details were retrieved from.
*/
lookup_realm?: string;
/**
* Set of space IDs that a saved object was shared to.
*/
add_to_spaces?: readonly string[];
/**
* Set of space IDs that a saved object was removed from.
*/
[x: string]: any;
delete_from_spaces?: readonly string[];
};
error?: {
code?: string;
Expand Down
20 changes: 16 additions & 4 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 @@ -27,6 +27,7 @@ const { logging } = coreMock.createSetup();
const http = httpServiceMock.createSetupContract();
const getCurrentUser = jest.fn().mockReturnValue({ username: 'jdoe', roles: ['admin'] });
const getSpaceId = jest.fn().mockReturnValue('default');
const getSID = jest.fn().mockResolvedValue('SESSION_ID');

beforeEach(() => {
logger.info.mockClear();
Expand All @@ -45,6 +46,7 @@ describe('#setup', () => {
http,
getCurrentUser,
getSpaceId,
getSID,
})
).toMatchInlineSnapshot(`
Object {
Expand All @@ -70,6 +72,7 @@ describe('#setup', () => {
http,
getCurrentUser,
getSpaceId,
getSID,
});
expect(logging.configure).toHaveBeenCalledWith(expect.any(Observable));
});
Expand All @@ -82,6 +85,7 @@ describe('#setup', () => {
http,
getCurrentUser,
getSpaceId,
getSID,
});
expect(http.registerOnPostAuth).toHaveBeenCalledWith(expect.any(Function));
});
Expand All @@ -96,16 +100,17 @@ describe('#asScoped', () => {
http,
getCurrentUser,
getSpaceId,
getSID,
});
const request = httpServerMock.createKibanaRequest({
kibanaRequestState: { requestId: 'REQUEST_ID', requestUuid: 'REQUEST_UUID' },
});

audit.asScoped(request).log({ message: 'MESSAGE', event: { action: 'ACTION' } });
await audit.asScoped(request).log({ message: 'MESSAGE', event: { action: 'ACTION' } });
expect(logger.info).toHaveBeenCalledWith('MESSAGE', {
ecs: { version: '1.6.0' },
event: { action: 'ACTION' },
kibana: { space_id: 'default' },
kibana: { space_id: 'default', session_id: 'SESSION_ID' },
message: 'MESSAGE',
trace: { id: 'REQUEST_ID' },
user: { name: 'jdoe', roles: ['admin'] },
Expand All @@ -123,12 +128,13 @@ describe('#asScoped', () => {
http,
getCurrentUser,
getSpaceId,
getSID,
});
const request = httpServerMock.createKibanaRequest({
kibanaRequestState: { requestId: 'REQUEST_ID', requestUuid: 'REQUEST_UUID' },
});

audit.asScoped(request).log({ message: 'MESSAGE', event: { action: 'ACTION' } });
await audit.asScoped(request).log({ message: 'MESSAGE', event: { action: 'ACTION' } });
expect(logger.info).not.toHaveBeenCalled();
});

Expand All @@ -143,12 +149,13 @@ describe('#asScoped', () => {
http,
getCurrentUser,
getSpaceId,
getSID,
});
const request = httpServerMock.createKibanaRequest({
kibanaRequestState: { requestId: 'REQUEST_ID', requestUuid: 'REQUEST_UUID' },
});

audit.asScoped(request).log(undefined);
await audit.asScoped(request).log(undefined);
expect(logger.info).not.toHaveBeenCalled();
});
});
Expand Down Expand Up @@ -368,6 +375,7 @@ describe('#getLogger', () => {
http,
getCurrentUser,
getSpaceId,
getSID,
});

const auditLogger = auditService.getLogger(pluginId);
Expand Down Expand Up @@ -398,6 +406,7 @@ describe('#getLogger', () => {
http,
getCurrentUser,
getSpaceId,
getSID,
});

const auditLogger = auditService.getLogger(pluginId);
Expand Down Expand Up @@ -436,6 +445,7 @@ describe('#getLogger', () => {
http,
getCurrentUser,
getSpaceId,
getSID,
});

const auditLogger = auditService.getLogger(pluginId);
Expand Down Expand Up @@ -464,6 +474,7 @@ describe('#getLogger', () => {
http,
getCurrentUser,
getSpaceId,
getSID,
});

const auditLogger = auditService.getLogger(pluginId);
Expand Down Expand Up @@ -493,6 +504,7 @@ describe('#getLogger', () => {
http,
getCurrentUser,
getSpaceId,
getSID,
});

const auditLogger = auditService.getLogger(pluginId);
Expand Down
15 changes: 7 additions & 8 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 @@ -36,9 +36,6 @@ interface AuditLogMeta extends AuditEvent {
ecs: {
version: string;
};
session?: {
id: string;
};
trace: {
id: string;
};
Expand All @@ -57,6 +54,7 @@ interface AuditServiceSetupParams {
getCurrentUser(
request: KibanaRequest
): ReturnType<SecurityPluginSetup['authc']['getCurrentUser']> | undefined;
getSID(request: KibanaRequest): Promise<string | undefined>;
getSpaceId(
request: KibanaRequest
): ReturnType<SpacesPluginSetup['spacesService']['getSpaceId']> | undefined;
Expand Down Expand Up @@ -84,6 +82,7 @@ export class AuditService {
logging,
http,
getCurrentUser,
getSID,
getSpaceId,
}: AuditServiceSetupParams): AuditServiceSetup {
if (config.enabled && !config.appender) {
Expand Down Expand Up @@ -134,12 +133,13 @@ export class AuditService {
* });
* ```
*/
const log: AuditLogger['log'] = (event) => {
const log: AuditLogger['log'] = async (event) => {
if (!event) {
return;
}
const user = getCurrentUser(request);
const spaceId = getSpaceId(request);
const user = getCurrentUser(request);
const sessionId = await getSID(request);
thomheymann marked this conversation as resolved.
Show resolved Hide resolved
const meta: AuditLogMeta = {
ecs: { version: ECS_VERSION },
...event,
Expand All @@ -151,11 +151,10 @@ export class AuditService {
event.user,
kibana: {
space_id: spaceId,
session_id: sessionId,
thomheymann marked this conversation as resolved.
Show resolved Hide resolved
...event.kibana,
},
trace: {
id: request.id,
},
trace: { id: request.id },
};
if (filterEvent(meta, config.ignore_filters)) {
this.ecsLogger.info(event.message!, meta);
Expand Down
17 changes: 9 additions & 8 deletions x-pack/plugins/security/server/plugin.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -188,24 +188,25 @@ export class Plugin {

registerSecurityUsageCollector({ usageCollection, config, license });

const { session } = this.sessionManagementService.setup({
config,
clusterClient,
http: core.http,
kibanaIndexName: legacyConfig.kibana.index,
taskManager,
});

const audit = this.auditService.setup({
license,
config: config.audit,
logging: core.logging,
http: core.http,
getSpaceId: (request) => spaces?.spacesService.getSpaceId(request),
getSID: (request) => session.getSID(request),
getCurrentUser: (request) => authenticationSetup.getCurrentUser(request),
});
const legacyAuditLogger = new SecurityAuditLogger(audit.getLogger());

const { session } = this.sessionManagementService.setup({
config,
clusterClient,
http: core.http,
kibanaIndexName: legacyConfig.kibana.index,
taskManager,
});

const authenticationSetup = this.authenticationService.setup({
legacyAuditLogger,
audit,
Expand Down
1 change: 1 addition & 0 deletions x-pack/plugins/security/server/session_management/session.mock.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@ import { sessionIndexMock } from './session_index.mock';

export const sessionMock = {
create: (): jest.Mocked<PublicMethodsOf<Session>> => ({
getSID: jest.fn(),
get: jest.fn(),
create: jest.fn(),
update: jest.fn(),
Expand Down
14 changes: 14 additions & 0 deletions x-pack/plugins/security/server/session_management/session.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -56,6 +56,20 @@ describe('Session', () => {
});
});

describe('#getSID', () => {
const mockRequest = httpServerMock.createKibanaRequest();

it('returns `undefined` if session cookie does not exist', async () => {
mockSessionCookie.get.mockResolvedValue(null);
await expect(session.getSID(mockRequest)).resolves.toBeUndefined();
});

it('returns session id', async () => {
mockSessionCookie.get.mockResolvedValue(sessionCookieMock.createValue());
await expect(session.getSID(mockRequest)).resolves.toEqual('some-long-sid');
});
});

describe('#get', () => {
const mockAAD = Buffer.from([2, ...Array(255).keys()]).toString('base64');

Expand Down
11 changes: 11 additions & 0 deletions x-pack/plugins/security/server/session_management/session.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -99,6 +99,17 @@ export class Session {
this.crypto = nodeCrypto({ encryptionKey: this.options.config.encryptionKey });
}

/**
* Extracts session id for the specified request.
* @param request Request instance to get session value for.
*/
async getSID(request: KibanaRequest) {
const sessionCookieValue = await this.options.sessionCookie.get(request);
if (sessionCookieValue) {
return sessionCookieValue.sid;
}
Copy link
Member

Choose a reason for hiding this comment

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

question/nit: how do you feel about returning null instead of undefined here? So that it's consistent with getCurrentUser, Session.get and etc?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I find null a cause of unintended bugs and behaviours and the distinctions between "not set" (undefined) and "set but empty" (null) to be rarely important.

e.g. null can cause bugs when using optional parameters and default values since the default value will not be used. null is also of type object which is weird and can cause bugs so I rarely use it.

Having said that, I think in this case, we really are dealing with the "not set" scenario so I think undefined is correct.

Why are you using null for the other methods?

Copy link
Member

Choose a reason for hiding this comment

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

Why are you using null for the other methods?

TL;DR: We don't have any guidelines on that yet and even though I have a slightly stronger opinion on getCurrentUser and Session.get as explained below, I don't have the same strong reasons to use null in this particular case except for consistency. Both would work for me and I trust your judgement here 👍

I think that we historically treated null not as set but empty, but more like an intentional absence of any object value, like the user or session objects are absent in a particular context, it's expected and intentional, and we explicitly manifest that with null. As a side benefit, when you return null; you explicitly define an exit point, where undefined can be either intentional or not (e.g. forgotten return statement).

Regarding default and optional parameters, I'd say it's more about personal preference, I find the code that is explicit about using default and optional parameters a little bit easier to read and understand, and type system will prevent unintentional behavior in case parameter cannot be null.

Copy link
Contributor Author

@thomheymann thomheymann Dec 14, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

I think the definition makes sense* then. For object values use null, for primitives use undefined. That also explains why null is of type object.

* I'm using "sense" in the loosest meaning of the word here since most other programming languages have a single type the denote absence of a value and get by just fine 🤷‍♀️

}

/**
* Extracts session value for the specified request. Under the hood it can clear session if it is
* invalid or created by the legacy versions of Kibana.
Expand Down