From b759f46ecf17f6691dcf09ca8ab3f52e28473813 Mon Sep 17 00:00:00 2001 From: Thom Heymann Date: Thu, 6 Jan 2022 09:40:54 +0000 Subject: [PATCH 1/5] Add session cleanup audit logging --- docs/user/security/audit-logging.asciidoc | 5 +- .../server/audit/audit_events.test.ts | 32 +++++ .../security/server/audit/audit_events.ts | 29 +++++ .../server/audit/audit_service.test.ts | 79 ++++++++++++ .../security/server/audit/audit_service.ts | 110 ++++++++++------- .../security/server/audit/index.mock.ts | 3 + x-pack/plugins/security/server/audit/index.ts | 1 + x-pack/plugins/security/server/plugin.ts | 5 +- .../session_management/session_index.test.ts | 113 ++++++++++++++---- .../session_management/session_index.ts | 67 ++++++++--- .../session_management_service.test.ts | 44 ++++--- .../session_management_service.ts | 4 + 12 files changed, 390 insertions(+), 102 deletions(-) diff --git a/docs/user/security/audit-logging.asciidoc b/docs/user/security/audit-logging.asciidoc index 33e1c2b523511..1e7eb1971af08 100644 --- a/docs/user/security/audit-logging.asciidoc +++ b/docs/user/security/audit-logging.asciidoc @@ -53,8 +53,11 @@ Refer to the corresponding {es} logs for potential write errors. | `user_logout` | `unknown` | User is logging out. +| `session_cleanup` +| `unknown` | Removing invalid or expired session. + | `access_agreement_acknowledged` -| N/A | User has acknowledged the access agreement. +| n/a | User has acknowledged the access agreement. 3+a| ===== Category: database diff --git a/x-pack/plugins/security/server/audit/audit_events.test.ts b/x-pack/plugins/security/server/audit/audit_events.test.ts index df796b0603176..0a7337e453274 100644 --- a/x-pack/plugins/security/server/audit/audit_events.test.ts +++ b/x-pack/plugins/security/server/audit/audit_events.test.ts @@ -15,6 +15,7 @@ import { httpRequestEvent, SavedObjectAction, savedObjectEvent, + sessionCleanupEvent, SpaceAuditAction, spaceAuditEvent, userLoginEvent, @@ -352,6 +353,37 @@ describe('#userLogoutEvent', () => { }); }); +describe('#sessionCleanupEvent', () => { + test('creates event with `unknown` outcome', () => { + expect( + sessionCleanupEvent({ + usernameHash: 'abcdef', + sessionId: 'sid', + provider: { name: 'basic1', type: 'basic' }, + }) + ).toMatchInlineSnapshot(` + Object { + "event": Object { + "action": "session_cleanup", + "category": Array [ + "authentication", + ], + "outcome": "unknown", + }, + "kibana": Object { + "authentication_provider": "basic1", + "authentication_type": "basic", + "session_id": "sid", + }, + "message": "Removing invalid or expired session for user [hash=abcdef]", + "user": Object { + "hash": "abcdef", + }, + } + `); + }); +}); + describe('#httpRequestEvent', () => { test('creates event with `unknown` outcome', () => { expect( diff --git a/x-pack/plugins/security/server/audit/audit_events.ts b/x-pack/plugins/security/server/audit/audit_events.ts index 96bc85c1be37e..2dfaf8ece004f 100644 --- a/x-pack/plugins/security/server/audit/audit_events.ts +++ b/x-pack/plugins/security/server/audit/audit_events.ts @@ -156,6 +156,35 @@ export function userLogoutEvent({ username, provider }: UserLogoutParams): Audit }; } +export interface SessionCleanupParams { + sessionId: string; + usernameHash?: string; + provider: AuthenticationProvider; +} + +export function sessionCleanupEvent({ + usernameHash, + sessionId, + provider, +}: SessionCleanupParams): AuditEvent { + return { + message: `Removing invalid or expired session for user [hash=${usernameHash}]`, + event: { + action: 'session_cleanup', + category: ['authentication'], + outcome: 'unknown', + }, + user: { + hash: usernameHash, + }, + kibana: { + session_id: sessionId, + authentication_provider: provider.name, + authentication_type: provider.type, + }, + }; +} + export interface AccessAgreementAcknowledgedParams { username: string; provider: AuthenticationProvider; diff --git a/x-pack/plugins/security/server/audit/audit_service.test.ts b/x-pack/plugins/security/server/audit/audit_service.test.ts index 493490a8e8b9f..1815f617dceae 100644 --- a/x-pack/plugins/security/server/audit/audit_service.test.ts +++ b/x-pack/plugins/security/server/audit/audit_service.test.ts @@ -67,6 +67,9 @@ describe('#setup', () => { ).toMatchInlineSnapshot(` Object { "asScoped": [Function], + "withoutRequest": Object { + "log": [Function], + }, } `); audit.stop(); @@ -254,6 +257,82 @@ describe('#asScoped', () => { }); }); +describe('#withoutRequest', () => { + it('logs event without additional meta data', async () => { + const audit = new AuditService(logger); + const auditSetup = audit.setup({ + license, + config, + logging, + http, + getCurrentUser, + getSpaceId, + getSID, + recordAuditLoggingUsage, + }); + + await auditSetup.withoutRequest.log({ message: 'MESSAGE', event: { action: 'ACTION' } }); + expect(logger.info).toHaveBeenCalledWith('MESSAGE', { + event: { action: 'ACTION' }, + }); + audit.stop(); + }); + + it('does not log to audit logger if event matches ignore filter', async () => { + const audit = new AuditService(logger); + const auditSetup = audit.setup({ + license, + config: { + enabled: true, + appender: { + type: 'console', + layout: { + type: 'json', + }, + }, + ignore_filters: [{ actions: ['ACTION'] }], + }, + logging, + http, + getCurrentUser, + getSpaceId, + getSID, + recordAuditLoggingUsage, + }); + + await auditSetup.withoutRequest.log({ message: 'MESSAGE', event: { action: 'ACTION' } }); + expect(logger.info).not.toHaveBeenCalled(); + audit.stop(); + }); + + it('does not log to audit logger if no event was generated', async () => { + const audit = new AuditService(logger); + const auditSetup = audit.setup({ + license, + config: { + enabled: true, + appender: { + type: 'console', + layout: { + type: 'json', + }, + }, + ignore_filters: [{ actions: ['ACTION'] }], + }, + logging, + http, + getCurrentUser, + getSpaceId, + getSID, + recordAuditLoggingUsage, + }); + + await auditSetup.withoutRequest.log(undefined); + expect(logger.info).not.toHaveBeenCalled(); + audit.stop(); + }); +}); + describe('#createLoggingConfig', () => { test('sets log level to `info` when audit logging is enabled and appender is defined', async () => { const features$ = of({ diff --git a/x-pack/plugins/security/server/audit/audit_service.ts b/x-pack/plugins/security/server/audit/audit_service.ts index fb03669ca0fc5..a29ec221b3474 100644 --- a/x-pack/plugins/security/server/audit/audit_service.ts +++ b/x-pack/plugins/security/server/audit/audit_service.ts @@ -26,11 +26,58 @@ export const ECS_VERSION = '1.6.0'; export const RECORD_USAGE_INTERVAL = 60 * 60 * 1000; // 1 hour export interface AuditLogger { + /** + * Logs an {@link AuditEvent} and automatically adds meta data about the + * current user, space and correlation id. + * + * Guidelines around what events should be logged and how they should be + * structured can be found in: `/x-pack/plugins/security/README.md` + * + * @example + * ```typescript + * const auditLogger = securitySetup.audit.asScoped(request); + * auditLogger.log({ + * message: 'User is updating dashboard [id=123]', + * event: { + * action: 'saved_object_update', + * outcome: 'unknown' + * }, + * kibana: { + * saved_object: { type: 'dashboard', id: '123' } + * }, + * }); + * ``` + */ log: (event: AuditEvent | undefined) => void; } export interface AuditServiceSetup { + /** + * Creates an {@link AuditLogger} scoped to the current request. + * + * This audit logger logs events with all required user and session info and should be used for + * all user-initiated actions. + * + * @example + * ```typescript + * const auditLogger = securitySetup.audit.asScoped(request); + * auditLogger.log(event); + * ``` + */ asScoped: (request: KibanaRequest) => AuditLogger; + + /** + * {@link AuditLogger} for background tasks only. + * + * This audit logger logs events without any user or session info and should never be used to log + * user-initiated actions. + * + * @example + * ```typescript + * securitySetup.audit.withoutRequest.log(event); + * ``` + */ + withoutRequest: AuditLogger; } interface AuditServiceSetupParams { @@ -88,46 +135,25 @@ export class AuditService { }); } - /** - * Creates an {@link AuditLogger} scoped to the current request. - * - * @example - * ```typescript - * const auditLogger = securitySetup.audit.asScoped(request); - * auditLogger.log(event); - * ``` - */ - const asScoped = (request: KibanaRequest): AuditLogger => { - /** - * Logs an {@link AuditEvent} and automatically adds meta data about the - * current user, space and correlation id. - * - * Guidelines around what events should be logged and how they should be - * structured can be found in: `/x-pack/plugins/security/README.md` - * - * @example - * ```typescript - * const auditLogger = securitySetup.audit.asScoped(request); - * auditLogger.log({ - * message: 'User is updating dashboard [id=123]', - * event: { - * action: 'saved_object_update', - * outcome: 'unknown' - * }, - * kibana: { - * saved_object: { type: 'dashboard', id: '123' } - * }, - * }); - * ``` - */ - const log: AuditLogger['log'] = async (event) => { + const log = (event: AuditEvent | undefined) => { + if (!event) { + return; + } + if (filterEvent(event, config.ignore_filters)) { + const { message, ...eventMeta } = event; + this.logger.info(message, eventMeta); + } + }; + + const asScoped = (request: KibanaRequest): AuditLogger => ({ + log: async (event) => { if (!event) { return; } const spaceId = getSpaceId(request); const user = getCurrentUser(request); const sessionId = await getSID(request); - const meta: AuditEvent = { + log({ ...event, user: (user && { @@ -141,14 +167,9 @@ export class AuditService { ...event.kibana, }, trace: { id: request.id }, - }; - if (filterEvent(meta, config.ignore_filters)) { - const { message, ...eventMeta } = meta; - this.logger.info(message, eventMeta); - } - }; - return { log }; - }; + }); + }, + }); http.registerOnPostAuth((request, response, t) => { if (request.auth.isAuthenticated) { @@ -157,7 +178,10 @@ export class AuditService { return t.next(); }); - return { asScoped }; + return { + asScoped, + withoutRequest: { log }, + }; } stop() { diff --git a/x-pack/plugins/security/server/audit/index.mock.ts b/x-pack/plugins/security/server/audit/index.mock.ts index ce6885aee50de..c84faacff0147 100644 --- a/x-pack/plugins/security/server/audit/index.mock.ts +++ b/x-pack/plugins/security/server/audit/index.mock.ts @@ -14,6 +14,9 @@ export const auditServiceMock = { asScoped: jest.fn().mockReturnValue({ log: jest.fn(), }), + withoutRequest: { + log: jest.fn(), + }, } as jest.Mocked>; }, }; diff --git a/x-pack/plugins/security/server/audit/index.ts b/x-pack/plugins/security/server/audit/index.ts index f83c7a7f3bd8a..0bd8492b79670 100644 --- a/x-pack/plugins/security/server/audit/index.ts +++ b/x-pack/plugins/security/server/audit/index.ts @@ -11,6 +11,7 @@ export type { AuditEvent } from './audit_events'; export { userLoginEvent, userLogoutEvent, + sessionCleanupEvent, accessAgreementAcknowledgedEvent, httpRequestEvent, savedObjectEvent, diff --git a/x-pack/plugins/security/server/plugin.ts b/x-pack/plugins/security/server/plugin.ts index e8f7aa2aacfdd..5d56307547558 100644 --- a/x-pack/plugins/security/server/plugin.ts +++ b/x-pack/plugins/security/server/plugin.ts @@ -310,9 +310,7 @@ export class SecurityPlugin }); return Object.freeze({ - audit: { - asScoped: this.auditSetup.asScoped, - }, + audit: this.auditSetup, authc: { getCurrentUser: (request) => this.getAuthentication().getCurrentUser(request) }, authz: { actions: this.authorizationSetup.actions, @@ -345,6 +343,7 @@ export class SecurityPlugin const clusterClient = core.elasticsearch.client; const { watchOnlineStatus$ } = this.elasticsearchService.start(); const { session } = this.sessionManagementService.start({ + auditLogger: this.auditSetup!.withoutRequest, elasticsearchClient: clusterClient.asInternalUser, kibanaIndexName: this.getKibanaIndexName(), online$: watchOnlineStatus$(), diff --git a/x-pack/plugins/security/server/session_management/session_index.test.ts b/x-pack/plugins/security/server/session_management/session_index.test.ts index 251a0a3edb061..9ad94020a25f6 100644 --- a/x-pack/plugins/security/server/session_management/session_index.test.ts +++ b/x-pack/plugins/security/server/session_management/session_index.test.ts @@ -11,6 +11,8 @@ import type { DeeplyMockedKeys } from '@kbn/utility-types/jest'; import type { ElasticsearchClient } from 'src/core/server'; import { elasticsearchServiceMock, loggingSystemMock } from 'src/core/server/mocks'; +import type { AuditLogger } from '../audit'; +import { auditServiceMock } from '../audit/index.mock'; import { ConfigSchema, createConfig } from '../config'; import { securityMock } from '../mocks'; import { getSessionIndexTemplate, SessionIndex } from './session_index'; @@ -19,11 +21,13 @@ import { sessionIndexMock } from './session_index.mock'; describe('Session index', () => { let mockElasticsearchClient: DeeplyMockedKeys; let sessionIndex: SessionIndex; + let auditLogger: AuditLogger; const indexName = '.kibana_some_tenant_security_session_1'; const indexTemplateName = '.kibana_some_tenant_security_session_index_template_1'; beforeEach(() => { mockElasticsearchClient = elasticsearchServiceMock.createElasticsearchClient(); - const sessionIndexOptions = { + auditLogger = auditServiceMock.create().withoutRequest; + sessionIndex = new SessionIndex({ logger: loggingSystemMock.createLogger(), kibanaIndexName: '.kibana_some_tenant', config: createConfig( @@ -32,9 +36,8 @@ describe('Session index', () => { { isTLSEnabled: false } ), elasticsearchClient: mockElasticsearchClient, - }; - - sessionIndex = new SessionIndex(sessionIndexOptions); + auditLogger, + }); }); describe('#initialize', () => { @@ -219,18 +222,38 @@ describe('Session index', () => { describe('#cleanUp', () => { const now = 123456; + const sessionValue = { + _id: 'SESSION_ID', + _source: { usernameHash: 'USERNAME_HASH', provider: { name: 'basic1', type: 'basic' } }, + }; beforeEach(() => { - mockElasticsearchClient.deleteByQuery.mockResolvedValue( - securityMock.createApiResponse({ body: {} as any }) + mockElasticsearchClient.search.mockResolvedValue( + securityMock.createApiResponse({ + body: { hits: { hits: [sessionValue] } } as any, + }) + ); + mockElasticsearchClient.bulk.mockResolvedValue( + securityMock.createApiResponse({ + body: { items: [{}] } as any, + }) ); jest.spyOn(Date, 'now').mockImplementation(() => now); }); - it('throws if call to Elasticsearch fails', async () => { + it('throws if search call to Elasticsearch fails', async () => { const failureReason = new errors.ResponseError( securityMock.createApiResponse(securityMock.createApiResponse({ body: { type: 'Uh oh.' } })) ); - mockElasticsearchClient.deleteByQuery.mockRejectedValue(failureReason); + mockElasticsearchClient.search.mockRejectedValue(failureReason); + + await expect(sessionIndex.cleanUp()).rejects.toBe(failureReason); + }); + + it('throws if bulk delete call to Elasticsearch fails', async () => { + const failureReason = new errors.ResponseError( + securityMock.createApiResponse(securityMock.createApiResponse({ body: { type: 'Uh oh.' } })) + ); + mockElasticsearchClient.bulk.mockRejectedValue(failureReason); await expect(sessionIndex.cleanUp()).rejects.toBe(failureReason); }); @@ -238,11 +261,11 @@ describe('Session index', () => { it('when neither `lifespan` nor `idleTimeout` is configured', async () => { await sessionIndex.cleanUp(); - expect(mockElasticsearchClient.deleteByQuery).toHaveBeenCalledTimes(1); - expect(mockElasticsearchClient.deleteByQuery).toHaveBeenCalledWith( + expect(mockElasticsearchClient.search).toHaveBeenCalledTimes(1); + expect(mockElasticsearchClient.search).toHaveBeenCalledWith( { index: indexName, - refresh: true, + _source_includes: 'usernameHash,provider', body: { query: { bool: { @@ -287,6 +310,12 @@ describe('Session index', () => { }, { ignore: [409, 404] } ); + + expect(mockElasticsearchClient.bulk).toHaveBeenCalledTimes(1); + expect(mockElasticsearchClient.bulk).toHaveBeenCalledWith({ + index: indexName, + operations: [{ delete: { _id: sessionValue._id } }], + }); }); it('when only `lifespan` is configured', async () => { @@ -299,15 +328,16 @@ describe('Session index', () => { { isTLSEnabled: false } ), elasticsearchClient: mockElasticsearchClient, + auditLogger: auditServiceMock.create().withoutRequest, }); await sessionIndex.cleanUp(); - expect(mockElasticsearchClient.deleteByQuery).toHaveBeenCalledTimes(1); - expect(mockElasticsearchClient.deleteByQuery).toHaveBeenCalledWith( + expect(mockElasticsearchClient.search).toHaveBeenCalledTimes(1); + expect(mockElasticsearchClient.search).toHaveBeenCalledWith( { index: indexName, - refresh: true, + _source_includes: 'usernameHash,provider', body: { query: { bool: { @@ -362,6 +392,12 @@ describe('Session index', () => { }, { ignore: [409, 404] } ); + + expect(mockElasticsearchClient.bulk).toHaveBeenCalledTimes(1); + expect(mockElasticsearchClient.bulk).toHaveBeenCalledWith({ + index: indexName, + operations: [{ delete: { _id: sessionValue._id } }], + }); }); it('when only `idleTimeout` is configured', async () => { @@ -375,15 +411,16 @@ describe('Session index', () => { { isTLSEnabled: false } ), elasticsearchClient: mockElasticsearchClient, + auditLogger: auditServiceMock.create().withoutRequest, }); await sessionIndex.cleanUp(); - expect(mockElasticsearchClient.deleteByQuery).toHaveBeenCalledTimes(1); - expect(mockElasticsearchClient.deleteByQuery).toHaveBeenCalledWith( + expect(mockElasticsearchClient.search).toHaveBeenCalledTimes(1); + expect(mockElasticsearchClient.search).toHaveBeenCalledWith( { index: indexName, - refresh: true, + _source_includes: 'usernameHash,provider', body: { query: { bool: { @@ -432,6 +469,12 @@ describe('Session index', () => { }, { ignore: [409, 404] } ); + + expect(mockElasticsearchClient.bulk).toHaveBeenCalledTimes(1); + expect(mockElasticsearchClient.bulk).toHaveBeenCalledWith({ + index: indexName, + operations: [{ delete: { _id: sessionValue._id } }], + }); }); it('when both `lifespan` and `idleTimeout` are configured', async () => { @@ -445,15 +488,16 @@ describe('Session index', () => { { isTLSEnabled: false } ), elasticsearchClient: mockElasticsearchClient, + auditLogger: auditServiceMock.create().withoutRequest, }); await sessionIndex.cleanUp(); - expect(mockElasticsearchClient.deleteByQuery).toHaveBeenCalledTimes(1); - expect(mockElasticsearchClient.deleteByQuery).toHaveBeenCalledWith( + expect(mockElasticsearchClient.search).toHaveBeenCalledTimes(1); + expect(mockElasticsearchClient.search).toHaveBeenCalledWith( { index: indexName, - refresh: true, + _source_includes: 'usernameHash,provider', body: { query: { bool: { @@ -512,6 +556,12 @@ describe('Session index', () => { }, { ignore: [409, 404] } ); + + expect(mockElasticsearchClient.bulk).toHaveBeenCalledTimes(1); + expect(mockElasticsearchClient.bulk).toHaveBeenCalledWith({ + index: indexName, + operations: [{ delete: { _id: sessionValue._id } }], + }); }); it('when both `lifespan` and `idleTimeout` are configured and multiple providers are enabled', async () => { @@ -540,15 +590,16 @@ describe('Session index', () => { { isTLSEnabled: false } ), elasticsearchClient: mockElasticsearchClient, + auditLogger: auditServiceMock.create().withoutRequest, }); await sessionIndex.cleanUp(); - expect(mockElasticsearchClient.deleteByQuery).toHaveBeenCalledTimes(1); - expect(mockElasticsearchClient.deleteByQuery).toHaveBeenCalledWith( + expect(mockElasticsearchClient.search).toHaveBeenCalledTimes(1); + expect(mockElasticsearchClient.search).toHaveBeenCalledWith( { index: indexName, - refresh: true, + _source_includes: 'usernameHash,provider', body: { query: { bool: { @@ -640,6 +691,22 @@ describe('Session index', () => { }, { ignore: [409, 404] } ); + + expect(mockElasticsearchClient.bulk).toHaveBeenCalledTimes(1); + expect(mockElasticsearchClient.bulk).toHaveBeenCalledWith({ + index: indexName, + operations: [{ delete: { _id: sessionValue._id } }], + }); + }); + + it('should log audit event', async () => { + await sessionIndex.cleanUp(); + + expect(auditLogger.log).toHaveBeenCalledWith( + expect.objectContaining({ + event: { action: 'session_cleanup', category: ['authentication'], outcome: 'unknown' }, + }) + ); }); }); diff --git a/x-pack/plugins/security/server/session_management/session_index.ts b/x-pack/plugins/security/server/session_management/session_index.ts index 801597dad6baf..ef04f5704b040 100644 --- a/x-pack/plugins/security/server/session_management/session_index.ts +++ b/x-pack/plugins/security/server/session_management/session_index.ts @@ -5,9 +5,13 @@ * 2.0. */ +import type { BulkOperationContainer } from '@elastic/elasticsearch/lib/api/typesWithBodyKey'; + import type { ElasticsearchClient, Logger } from 'src/core/server'; import type { AuthenticationProvider } from '../../common/model'; +import type { AuditLogger } from '../audit'; +import { sessionCleanupEvent } from '../audit'; import type { ConfigType } from '../config'; export interface SessionIndexOptions { @@ -15,6 +19,7 @@ export interface SessionIndexOptions { readonly kibanaIndexName: string; readonly config: Pick; readonly logger: Logger; + readonly auditLogger: AuditLogger; } /** @@ -484,24 +489,58 @@ export class SessionIndex { }); } + const operations: Array>> = []; try { - const { body: response } = await this.options.elasticsearchClient.deleteByQuery( - { - index: this.indexName, - refresh: true, - body: { query: { bool: { should: deleteQueries } } }, - }, - { ignore: [409, 404] } - ); - - if (response.deleted! > 0) { - this.options.logger.debug( - `Cleaned up ${response.deleted} invalid or expired session values.` + const { body: searchResponse } = + await this.options.elasticsearchClient.search( + { + index: this.indexName, + body: { query: { bool: { should: deleteQueries } } }, + _source_includes: 'usernameHash,provider', + }, + { ignore: [409, 404] } ); - } + searchResponse.hits.hits.forEach(({ _id, _source }) => { + const { usernameHash, provider } = _source!; + this.options.auditLogger.log( + sessionCleanupEvent({ sessionId: _id, usernameHash, provider }) + ); + operations.push({ delete: { _id } }); + }); } catch (err) { - this.options.logger.error(`Failed to clean up sessions: ${err.message}`); + this.options.logger.error(`Failed to look up invalid or expired sessions: ${err.message}`); throw err; } + + if (operations.length > 0) { + try { + const { body: deleteResponse } = await this.options.elasticsearchClient.bulk({ + index: this.indexName, + operations, + }); + if (deleteResponse.errors) { + const errorCount = deleteResponse.items.reduce( + (count, item) => (item.delete!.error ? count + 1 : count), + 0 + ); + if (errorCount < deleteResponse.items.length) { + this.options.logger.warn( + `Failed to clean up ${errorCount} of ${deleteResponse.items.length} invalid or expired sessions. The remaining sessions were cleaned up successfully.` + ); + } else { + this.options.logger.error( + `Failed to clean up ${deleteResponse.items.length} invalid or expired sessions.` + ); + } + } else { + this.options.logger.debug( + `Cleaned up ${deleteResponse.items.length} invalid or expired sessions.` + ); + } + } catch (err) { + this.options.logger.error(`Failed to clean up sessions: ${err.message}`); + throw err; + } + } } } diff --git a/x-pack/plugins/security/server/session_management/session_management_service.test.ts b/x-pack/plugins/security/server/session_management/session_management_service.test.ts index 7e99181981e85..100d0b30082c6 100644 --- a/x-pack/plugins/security/server/session_management/session_management_service.test.ts +++ b/x-pack/plugins/security/server/session_management/session_management_service.test.ts @@ -15,6 +15,8 @@ import type { TaskRunCreatorFunction, } from '../../../task_manager/server'; import { taskManagerMock } from '../../../task_manager/server/mocks'; +import type { AuditLogger } from '../audit'; +import { auditServiceMock } from '../audit/index.mock'; import { ConfigSchema, createConfig } from '../config'; import type { OnlineStatusRetryScheduler } from '../elasticsearch'; import { Session } from './session'; @@ -24,10 +26,23 @@ import { SessionManagementService, } from './session_management_service'; +const mockSessionIndexInitialize = jest.spyOn(SessionIndex.prototype, 'initialize'); +mockSessionIndexInitialize.mockResolvedValue(); + +const mockSessionIndexCleanUp = jest.spyOn(SessionIndex.prototype, 'cleanUp'); +mockSessionIndexCleanUp.mockResolvedValue(); + describe('SessionManagementService', () => { let service: SessionManagementService; + let auditLogger: AuditLogger; beforeEach(() => { service = new SessionManagementService(loggingSystemMock.createLogger()); + auditLogger = auditServiceMock.create().withoutRequest; + }); + + afterEach(() => { + mockSessionIndexInitialize.mockReset(); + mockSessionIndexCleanUp.mockReset(); }); describe('setup()', () => { @@ -56,12 +71,9 @@ describe('SessionManagementService', () => { }); describe('start()', () => { - let mockSessionIndexInitialize: jest.SpyInstance; let mockTaskManager: jest.Mocked; let sessionCleanupTaskRunCreator: TaskRunCreatorFunction; beforeEach(() => { - mockSessionIndexInitialize = jest.spyOn(SessionIndex.prototype, 'initialize'); - mockTaskManager = taskManagerMock.createStart(); mockTaskManager.ensureScheduled.mockResolvedValue(undefined as any); @@ -84,14 +96,11 @@ describe('SessionManagementService', () => { sessionCleanupTaskRunCreator = createTaskRunner; }); - afterEach(() => { - mockSessionIndexInitialize.mockReset(); - }); - it('exposes proper contract', () => { const mockStatusSubject = new Subject(); expect( service.start({ + auditLogger, elasticsearchClient: elasticsearchServiceMock.createElasticsearchClient(), kibanaIndexName: '.kibana', online$: mockStatusSubject.asObservable(), @@ -100,10 +109,10 @@ describe('SessionManagementService', () => { ).toEqual({ session: expect.any(Session) }); }); - it('registers proper session index cleanup task runner', () => { - const mockSessionIndexCleanUp = jest.spyOn(SessionIndex.prototype, 'cleanUp'); + it('registers proper session index cleanup task runner', async () => { const mockStatusSubject = new Subject(); service.start({ + auditLogger, elasticsearchClient: elasticsearchServiceMock.createElasticsearchClient(), kibanaIndexName: '.kibana', online$: mockStatusSubject.asObservable(), @@ -113,16 +122,17 @@ describe('SessionManagementService', () => { expect(mockSessionIndexCleanUp).not.toHaveBeenCalled(); const runner = sessionCleanupTaskRunCreator({} as any); - runner.run(); + await runner.run(); expect(mockSessionIndexCleanUp).toHaveBeenCalledTimes(1); - runner.run(); + await runner.run(); expect(mockSessionIndexCleanUp).toHaveBeenCalledTimes(2); }); it('initializes session index and schedules session index cleanup task when Elasticsearch goes online', async () => { const mockStatusSubject = new Subject(); service.start({ + auditLogger, elasticsearchClient: elasticsearchServiceMock.createElasticsearchClient(), kibanaIndexName: '.kibana', online$: mockStatusSubject.asObservable(), @@ -160,6 +170,7 @@ describe('SessionManagementService', () => { it('removes old cleanup task if cleanup interval changes', async () => { const mockStatusSubject = new Subject(); service.start({ + auditLogger, elasticsearchClient: elasticsearchServiceMock.createElasticsearchClient(), kibanaIndexName: '.kibana', online$: mockStatusSubject.asObservable(), @@ -195,6 +206,7 @@ describe('SessionManagementService', () => { it('does not remove old cleanup task if cleanup interval does not change', async () => { const mockStatusSubject = new Subject(); service.start({ + auditLogger, elasticsearchClient: elasticsearchServiceMock.createElasticsearchClient(), kibanaIndexName: '.kibana', online$: mockStatusSubject.asObservable(), @@ -221,6 +233,7 @@ describe('SessionManagementService', () => { it('schedules retry if index initialization fails', async () => { const mockStatusSubject = new Subject(); service.start({ + auditLogger, elasticsearchClient: elasticsearchServiceMock.createElasticsearchClient(), kibanaIndexName: '.kibana', online$: mockStatusSubject.asObservable(), @@ -257,6 +270,7 @@ describe('SessionManagementService', () => { it('schedules retry if cleanup task registration fails', async () => { const mockStatusSubject = new Subject(); service.start({ + auditLogger, elasticsearchClient: elasticsearchServiceMock.createElasticsearchClient(), kibanaIndexName: '.kibana', online$: mockStatusSubject.asObservable(), @@ -291,11 +305,8 @@ describe('SessionManagementService', () => { }); describe('stop()', () => { - let mockSessionIndexInitialize: jest.SpyInstance; let mockTaskManager: jest.Mocked; beforeEach(() => { - mockSessionIndexInitialize = jest.spyOn(SessionIndex.prototype, 'initialize'); - mockTaskManager = taskManagerMock.createStart(); mockTaskManager.ensureScheduled.mockResolvedValue(undefined as any); @@ -309,13 +320,10 @@ describe('SessionManagementService', () => { }); }); - afterEach(() => { - mockSessionIndexInitialize.mockReset(); - }); - it('properly unsubscribes from status updates', () => { const mockStatusSubject = new Subject(); service.start({ + auditLogger, elasticsearchClient: elasticsearchServiceMock.createElasticsearchClient(), kibanaIndexName: '.kibana', online$: mockStatusSubject.asObservable(), diff --git a/x-pack/plugins/security/server/session_management/session_management_service.ts b/x-pack/plugins/security/server/session_management/session_management_service.ts index fcd8e8c53cbe5..03a5d6130c3c1 100644 --- a/x-pack/plugins/security/server/session_management/session_management_service.ts +++ b/x-pack/plugins/security/server/session_management/session_management_service.ts @@ -14,6 +14,7 @@ import type { TaskManagerSetupContract, TaskManagerStartContract, } from '../../../task_manager/server'; +import type { AuditLogger } from '../audit'; import type { ConfigType } from '../config'; import type { OnlineStatusRetryScheduler } from '../elasticsearch'; import { Session } from './session'; @@ -31,6 +32,7 @@ export interface SessionManagementServiceStartParams { readonly kibanaIndexName: string; readonly online$: Observable; readonly taskManager: TaskManagerStartContract; + readonly auditLogger: AuditLogger; } export interface SessionManagementServiceStart { @@ -78,12 +80,14 @@ export class SessionManagementService { kibanaIndexName, online$, taskManager, + auditLogger, }: SessionManagementServiceStartParams): SessionManagementServiceStart { this.sessionIndex = new SessionIndex({ config: this.config, elasticsearchClient, kibanaIndexName, logger: this.logger.get('index'), + auditLogger, }); this.statusSubscription = online$.subscribe(async ({ scheduleRetry }) => { From 24d3ff6d53e9595d421932e66bda7554a80f0f6d Mon Sep 17 00:00:00 2001 From: Thom Heymann Date: Thu, 6 Jan 2022 12:03:57 +0000 Subject: [PATCH 2/5] Update snapshots --- x-pack/plugins/security/server/plugin.test.ts | 3 +++ 1 file changed, 3 insertions(+) diff --git a/x-pack/plugins/security/server/plugin.test.ts b/x-pack/plugins/security/server/plugin.test.ts index 3d43129b63809..85c2fff5a438e 100644 --- a/x-pack/plugins/security/server/plugin.test.ts +++ b/x-pack/plugins/security/server/plugin.test.ts @@ -67,6 +67,9 @@ describe('Security Plugin', () => { Object { "audit": Object { "asScoped": [Function], + "withoutRequest": Object { + "log": [Function], + }, }, "authc": Object { "getCurrentUser": [Function], From 615d6a90a7cd0fdf04d8aac805aa744553d762ae Mon Sep 17 00:00:00 2001 From: Thom Heymann Date: Mon, 10 Jan 2022 15:58:38 +0000 Subject: [PATCH 3/5] Added suggestions from code review --- .../session_management/session_index.test.ts | 615 +++++++++--------- .../session_management/session_index.ts | 42 +- 2 files changed, 353 insertions(+), 304 deletions(-) diff --git a/x-pack/plugins/security/server/session_management/session_index.test.ts b/x-pack/plugins/security/server/session_management/session_index.test.ts index 9ad94020a25f6..e7cb34be0278f 100644 --- a/x-pack/plugins/security/server/session_management/session_index.test.ts +++ b/x-pack/plugins/security/server/session_management/session_index.test.ts @@ -6,6 +6,7 @@ */ import { errors } from '@elastic/elasticsearch'; +import type { BulkResponse, SearchResponse } from '@elastic/elasticsearch/lib/api/types'; import type { DeeplyMockedKeys } from '@kbn/utility-types/jest'; import type { ElasticsearchClient } from 'src/core/server'; @@ -229,12 +230,12 @@ describe('Session index', () => { beforeEach(() => { mockElasticsearchClient.search.mockResolvedValue( securityMock.createApiResponse({ - body: { hits: { hits: [sessionValue] } } as any, + body: { hits: { total: 1, hits: [sessionValue] } } as SearchResponse, }) ); mockElasticsearchClient.bulk.mockResolvedValue( securityMock.createApiResponse({ - body: { items: [{}] } as any, + body: { items: [{}] } as BulkResponse, }) ); jest.spyOn(Date, 'now').mockImplementation(() => now); @@ -262,60 +263,63 @@ describe('Session index', () => { await sessionIndex.cleanUp(); expect(mockElasticsearchClient.search).toHaveBeenCalledTimes(1); - expect(mockElasticsearchClient.search).toHaveBeenCalledWith( - { - index: indexName, - _source_includes: 'usernameHash,provider', - body: { - query: { - bool: { - should: [ - // All expired sessions based on the lifespan, no matter which provider they belong to. - { range: { lifespanExpiration: { lte: now } } }, - // All sessions that belong to the providers that aren't configured. - { - bool: { - must_not: { - bool: { - should: [ - { - bool: { - must: [ - { term: { 'provider.type': 'basic' } }, - { term: { 'provider.name': 'basic' } }, - ], - }, + expect(mockElasticsearchClient.search).toHaveBeenCalledWith({ + index: indexName, + _source_includes: 'usernameHash,provider', + size: 1_000, + body: { + query: { + bool: { + should: [ + // All expired sessions based on the lifespan, no matter which provider they belong to. + { range: { lifespanExpiration: { lte: now } } }, + // All sessions that belong to the providers that aren't configured. + { + bool: { + must_not: { + bool: { + should: [ + { + bool: { + must: [ + { term: { 'provider.type': 'basic' } }, + { term: { 'provider.name': 'basic' } }, + ], }, - ], - minimum_should_match: 1, - }, + }, + ], + minimum_should_match: 1, }, }, }, - // The sessions that belong to a particular provider that are expired based on the idle timeout. - { - bool: { - must: [ - { term: { 'provider.type': 'basic' } }, - { term: { 'provider.name': 'basic' } }, - ], - should: [{ range: { idleTimeoutExpiration: { lte: now } } }], - minimum_should_match: 1, - }, + }, + // The sessions that belong to a particular provider that are expired based on the idle timeout. + { + bool: { + must: [ + { term: { 'provider.type': 'basic' } }, + { term: { 'provider.name': 'basic' } }, + ], + should: [{ range: { idleTimeoutExpiration: { lte: now } } }], + minimum_should_match: 1, }, - ], - }, + }, + ], }, }, }, - { ignore: [409, 404] } - ); + }); expect(mockElasticsearchClient.bulk).toHaveBeenCalledTimes(1); - expect(mockElasticsearchClient.bulk).toHaveBeenCalledWith({ - index: indexName, - operations: [{ delete: { _id: sessionValue._id } }], - }); + expect(mockElasticsearchClient.bulk).toHaveBeenCalledWith( + { + index: indexName, + operations: [{ delete: { _id: sessionValue._id } }], + }, + { + ignore: [409, 404], + } + ); }); it('when only `lifespan` is configured', async () => { @@ -334,70 +338,73 @@ describe('Session index', () => { await sessionIndex.cleanUp(); expect(mockElasticsearchClient.search).toHaveBeenCalledTimes(1); - expect(mockElasticsearchClient.search).toHaveBeenCalledWith( - { - index: indexName, - _source_includes: 'usernameHash,provider', - body: { - query: { - bool: { - should: [ - // All expired sessions based on the lifespan, no matter which provider they belong to. - { range: { lifespanExpiration: { lte: now } } }, - // All sessions that belong to the providers that aren't configured. - { - bool: { - must_not: { - bool: { - should: [ - { - bool: { - must: [ - { term: { 'provider.type': 'basic' } }, - { term: { 'provider.name': 'basic' } }, - ], - }, + expect(mockElasticsearchClient.search).toHaveBeenCalledWith({ + index: indexName, + _source_includes: 'usernameHash,provider', + size: 1_000, + body: { + query: { + bool: { + should: [ + // All expired sessions based on the lifespan, no matter which provider they belong to. + { range: { lifespanExpiration: { lte: now } } }, + // All sessions that belong to the providers that aren't configured. + { + bool: { + must_not: { + bool: { + should: [ + { + bool: { + must: [ + { term: { 'provider.type': 'basic' } }, + { term: { 'provider.name': 'basic' } }, + ], }, - ], - minimum_should_match: 1, - }, + }, + ], + minimum_should_match: 1, }, }, }, - // The sessions that belong to a particular provider but don't have a configured lifespan. - { - bool: { - must: [ - { term: { 'provider.type': 'basic' } }, - { term: { 'provider.name': 'basic' } }, - ], - must_not: { exists: { field: 'lifespanExpiration' } }, - }, + }, + // The sessions that belong to a particular provider but don't have a configured lifespan. + { + bool: { + must: [ + { term: { 'provider.type': 'basic' } }, + { term: { 'provider.name': 'basic' } }, + ], + must_not: { exists: { field: 'lifespanExpiration' } }, }, - // The sessions that belong to a particular provider that are expired based on the idle timeout. - { - bool: { - must: [ - { term: { 'provider.type': 'basic' } }, - { term: { 'provider.name': 'basic' } }, - ], - should: [{ range: { idleTimeoutExpiration: { lte: now } } }], - minimum_should_match: 1, - }, + }, + // The sessions that belong to a particular provider that are expired based on the idle timeout. + { + bool: { + must: [ + { term: { 'provider.type': 'basic' } }, + { term: { 'provider.name': 'basic' } }, + ], + should: [{ range: { idleTimeoutExpiration: { lte: now } } }], + minimum_should_match: 1, }, - ], - }, + }, + ], }, }, }, - { ignore: [409, 404] } - ); + }); expect(mockElasticsearchClient.bulk).toHaveBeenCalledTimes(1); - expect(mockElasticsearchClient.bulk).toHaveBeenCalledWith({ - index: indexName, - operations: [{ delete: { _id: sessionValue._id } }], - }); + expect(mockElasticsearchClient.bulk).toHaveBeenCalledWith( + { + index: indexName, + operations: [{ delete: { _id: sessionValue._id } }], + }, + { + ignore: [409, 404], + } + ); }); it('when only `idleTimeout` is configured', async () => { @@ -417,64 +424,67 @@ describe('Session index', () => { await sessionIndex.cleanUp(); expect(mockElasticsearchClient.search).toHaveBeenCalledTimes(1); - expect(mockElasticsearchClient.search).toHaveBeenCalledWith( - { - index: indexName, - _source_includes: 'usernameHash,provider', - body: { - query: { - bool: { - should: [ - // All expired sessions based on the lifespan, no matter which provider they belong to. - { range: { lifespanExpiration: { lte: now } } }, - // All sessions that belong to the providers that aren't configured. - { - bool: { - must_not: { - bool: { - should: [ - { - bool: { - must: [ - { term: { 'provider.type': 'basic' } }, - { term: { 'provider.name': 'basic' } }, - ], - }, + expect(mockElasticsearchClient.search).toHaveBeenCalledWith({ + index: indexName, + _source_includes: 'usernameHash,provider', + size: 1_000, + body: { + query: { + bool: { + should: [ + // All expired sessions based on the lifespan, no matter which provider they belong to. + { range: { lifespanExpiration: { lte: now } } }, + // All sessions that belong to the providers that aren't configured. + { + bool: { + must_not: { + bool: { + should: [ + { + bool: { + must: [ + { term: { 'provider.type': 'basic' } }, + { term: { 'provider.name': 'basic' } }, + ], }, - ], - minimum_should_match: 1, - }, + }, + ], + minimum_should_match: 1, }, }, }, - // The sessions that belong to a particular provider that are either expired based on the idle timeout - // or don't have it configured at all. - { - bool: { - must: [ - { term: { 'provider.type': 'basic' } }, - { term: { 'provider.name': 'basic' } }, - ], - should: [ - { range: { idleTimeoutExpiration: { lte: now - 3 * idleTimeout } } }, - { bool: { must_not: { exists: { field: 'idleTimeoutExpiration' } } } }, - ], - minimum_should_match: 1, - }, + }, + // The sessions that belong to a particular provider that are either expired based on the idle timeout + // or don't have it configured at all. + { + bool: { + must: [ + { term: { 'provider.type': 'basic' } }, + { term: { 'provider.name': 'basic' } }, + ], + should: [ + { range: { idleTimeoutExpiration: { lte: now - 3 * idleTimeout } } }, + { bool: { must_not: { exists: { field: 'idleTimeoutExpiration' } } } }, + ], + minimum_should_match: 1, }, - ], - }, + }, + ], }, }, }, - { ignore: [409, 404] } - ); + }); expect(mockElasticsearchClient.bulk).toHaveBeenCalledTimes(1); - expect(mockElasticsearchClient.bulk).toHaveBeenCalledWith({ - index: indexName, - operations: [{ delete: { _id: sessionValue._id } }], - }); + expect(mockElasticsearchClient.bulk).toHaveBeenCalledWith( + { + index: indexName, + operations: [{ delete: { _id: sessionValue._id } }], + }, + { + ignore: [409, 404], + } + ); }); it('when both `lifespan` and `idleTimeout` are configured', async () => { @@ -494,74 +504,77 @@ describe('Session index', () => { await sessionIndex.cleanUp(); expect(mockElasticsearchClient.search).toHaveBeenCalledTimes(1); - expect(mockElasticsearchClient.search).toHaveBeenCalledWith( - { - index: indexName, - _source_includes: 'usernameHash,provider', - body: { - query: { - bool: { - should: [ - // All expired sessions based on the lifespan, no matter which provider they belong to. - { range: { lifespanExpiration: { lte: now } } }, - // All sessions that belong to the providers that aren't configured. - { - bool: { - must_not: { - bool: { - should: [ - { - bool: { - must: [ - { term: { 'provider.type': 'basic' } }, - { term: { 'provider.name': 'basic' } }, - ], - }, + expect(mockElasticsearchClient.search).toHaveBeenCalledWith({ + index: indexName, + _source_includes: 'usernameHash,provider', + size: 1_000, + body: { + query: { + bool: { + should: [ + // All expired sessions based on the lifespan, no matter which provider they belong to. + { range: { lifespanExpiration: { lte: now } } }, + // All sessions that belong to the providers that aren't configured. + { + bool: { + must_not: { + bool: { + should: [ + { + bool: { + must: [ + { term: { 'provider.type': 'basic' } }, + { term: { 'provider.name': 'basic' } }, + ], }, - ], - minimum_should_match: 1, - }, + }, + ], + minimum_should_match: 1, }, }, }, - // The sessions that belong to a particular provider but don't have a configured lifespan. - { - bool: { - must: [ - { term: { 'provider.type': 'basic' } }, - { term: { 'provider.name': 'basic' } }, - ], - must_not: { exists: { field: 'lifespanExpiration' } }, - }, + }, + // The sessions that belong to a particular provider but don't have a configured lifespan. + { + bool: { + must: [ + { term: { 'provider.type': 'basic' } }, + { term: { 'provider.name': 'basic' } }, + ], + must_not: { exists: { field: 'lifespanExpiration' } }, }, - // The sessions that belong to a particular provider that are either expired based on the idle timeout - // or don't have it configured at all. - { - bool: { - must: [ - { term: { 'provider.type': 'basic' } }, - { term: { 'provider.name': 'basic' } }, - ], - should: [ - { range: { idleTimeoutExpiration: { lte: now - 3 * idleTimeout } } }, - { bool: { must_not: { exists: { field: 'idleTimeoutExpiration' } } } }, - ], - minimum_should_match: 1, - }, + }, + // The sessions that belong to a particular provider that are either expired based on the idle timeout + // or don't have it configured at all. + { + bool: { + must: [ + { term: { 'provider.type': 'basic' } }, + { term: { 'provider.name': 'basic' } }, + ], + should: [ + { range: { idleTimeoutExpiration: { lte: now - 3 * idleTimeout } } }, + { bool: { must_not: { exists: { field: 'idleTimeoutExpiration' } } } }, + ], + minimum_should_match: 1, }, - ], - }, + }, + ], }, }, }, - { ignore: [409, 404] } - ); + }); expect(mockElasticsearchClient.bulk).toHaveBeenCalledTimes(1); - expect(mockElasticsearchClient.bulk).toHaveBeenCalledWith({ - index: indexName, - operations: [{ delete: { _id: sessionValue._id } }], - }); + expect(mockElasticsearchClient.bulk).toHaveBeenCalledWith( + { + index: indexName, + operations: [{ delete: { _id: sessionValue._id } }], + }, + { + ignore: [409, 404], + } + ); }); it('when both `lifespan` and `idleTimeout` are configured and multiple providers are enabled', async () => { @@ -596,107 +609,127 @@ describe('Session index', () => { await sessionIndex.cleanUp(); expect(mockElasticsearchClient.search).toHaveBeenCalledTimes(1); - expect(mockElasticsearchClient.search).toHaveBeenCalledWith( - { - index: indexName, - _source_includes: 'usernameHash,provider', - body: { - query: { - bool: { - should: [ - // All expired sessions based on the lifespan, no matter which provider they belong to. - { range: { lifespanExpiration: { lte: now } } }, - // All sessions that belong to the providers that aren't configured. - { - bool: { - must_not: { - bool: { - should: [ - { - bool: { - must: [ - { term: { 'provider.type': 'basic' } }, - { term: { 'provider.name': 'basic1' } }, - ], - }, + expect(mockElasticsearchClient.search).toHaveBeenCalledWith({ + index: indexName, + _source_includes: 'usernameHash,provider', + size: 1_000, + body: { + query: { + bool: { + should: [ + // All expired sessions based on the lifespan, no matter which provider they belong to. + { range: { lifespanExpiration: { lte: now } } }, + // All sessions that belong to the providers that aren't configured. + { + bool: { + must_not: { + bool: { + should: [ + { + bool: { + must: [ + { term: { 'provider.type': 'basic' } }, + { term: { 'provider.name': 'basic1' } }, + ], }, - { - bool: { - must: [ - { term: { 'provider.type': 'saml' } }, - { term: { 'provider.name': 'saml1' } }, - ], - }, + }, + { + bool: { + must: [ + { term: { 'provider.type': 'saml' } }, + { term: { 'provider.name': 'saml1' } }, + ], }, - ], - minimum_should_match: 1, - }, + }, + ], + minimum_should_match: 1, }, }, }, - // The sessions that belong to a Basic provider but don't have a configured lifespan. - { - bool: { - must: [ - { term: { 'provider.type': 'basic' } }, - { term: { 'provider.name': 'basic1' } }, - ], - must_not: { exists: { field: 'lifespanExpiration' } }, - }, + }, + // The sessions that belong to a Basic provider but don't have a configured lifespan. + { + bool: { + must: [ + { term: { 'provider.type': 'basic' } }, + { term: { 'provider.name': 'basic1' } }, + ], + must_not: { exists: { field: 'lifespanExpiration' } }, }, - // The sessions that belong to a Basic provider that are either expired based on the idle timeout - // or don't have it configured at all. - { - bool: { - must: [ - { term: { 'provider.type': 'basic' } }, - { term: { 'provider.name': 'basic1' } }, - ], - should: [ - { range: { idleTimeoutExpiration: { lte: now - 3 * globalIdleTimeout } } }, - { bool: { must_not: { exists: { field: 'idleTimeoutExpiration' } } } }, - ], - minimum_should_match: 1, - }, + }, + // The sessions that belong to a Basic provider that are either expired based on the idle timeout + // or don't have it configured at all. + { + bool: { + must: [ + { term: { 'provider.type': 'basic' } }, + { term: { 'provider.name': 'basic1' } }, + ], + should: [ + { range: { idleTimeoutExpiration: { lte: now - 3 * globalIdleTimeout } } }, + { bool: { must_not: { exists: { field: 'idleTimeoutExpiration' } } } }, + ], + minimum_should_match: 1, }, - // The sessions that belong to a SAML provider but don't have a configured lifespan. - { - bool: { - must: [ - { term: { 'provider.type': 'saml' } }, - { term: { 'provider.name': 'saml1' } }, - ], - must_not: { exists: { field: 'lifespanExpiration' } }, - }, + }, + // The sessions that belong to a SAML provider but don't have a configured lifespan. + { + bool: { + must: [ + { term: { 'provider.type': 'saml' } }, + { term: { 'provider.name': 'saml1' } }, + ], + must_not: { exists: { field: 'lifespanExpiration' } }, }, - // The sessions that belong to a SAML provider that are either expired based on the idle timeout - // or don't have it configured at all. - { - bool: { - must: [ - { term: { 'provider.type': 'saml' } }, - { term: { 'provider.name': 'saml1' } }, - ], - should: [ - { range: { idleTimeoutExpiration: { lte: now - 3 * samlIdleTimeout } } }, - { bool: { must_not: { exists: { field: 'idleTimeoutExpiration' } } } }, - ], - minimum_should_match: 1, - }, + }, + // The sessions that belong to a SAML provider that are either expired based on the idle timeout + // or don't have it configured at all. + { + bool: { + must: [ + { term: { 'provider.type': 'saml' } }, + { term: { 'provider.name': 'saml1' } }, + ], + should: [ + { range: { idleTimeoutExpiration: { lte: now - 3 * samlIdleTimeout } } }, + { bool: { must_not: { exists: { field: 'idleTimeoutExpiration' } } } }, + ], + minimum_should_match: 1, }, - ], - }, + }, + ], }, }, }, - { ignore: [409, 404] } - ); + }); expect(mockElasticsearchClient.bulk).toHaveBeenCalledTimes(1); - expect(mockElasticsearchClient.bulk).toHaveBeenCalledWith({ - index: indexName, - operations: [{ delete: { _id: sessionValue._id } }], - }); + expect(mockElasticsearchClient.bulk).toHaveBeenCalledWith( + { + index: indexName, + operations: [{ delete: { _id: sessionValue._id } }], + }, + { + ignore: [409, 404], + } + ); + }); + + it('should clean up sessions in batches of 1000', async () => { + for (const value of [2500, 1500, 500]) { + mockElasticsearchClient.search.mockResolvedValueOnce( + securityMock.createApiResponse({ + body: { + hits: { total: { value, relation: 'eq' }, hits: [sessionValue] }, + } as SearchResponse, + }) + ); + } + + await sessionIndex.cleanUp(); + + expect(mockElasticsearchClient.search).toHaveBeenCalledTimes(3); + expect(mockElasticsearchClient.bulk).toHaveBeenCalledTimes(3); }); it('should log audit event', async () => { diff --git a/x-pack/plugins/security/server/session_management/session_index.ts b/x-pack/plugins/security/server/session_management/session_index.ts index ef04f5704b040..df3fa30ffb8aa 100644 --- a/x-pack/plugins/security/server/session_management/session_index.ts +++ b/x-pack/plugins/security/server/session_management/session_index.ts @@ -5,7 +5,10 @@ * 2.0. */ -import type { BulkOperationContainer } from '@elastic/elasticsearch/lib/api/typesWithBodyKey'; +import type { + BulkOperationContainer, + SearchTotalHits, +} from '@elastic/elasticsearch/lib/api/typesWithBodyKey'; import type { ElasticsearchClient, Logger } from 'src/core/server'; @@ -39,6 +42,11 @@ export type InvalidateSessionsFilter = */ const SESSION_INDEX_TEMPLATE_VERSION = 1; +/** + * Number of sessions to remove per batch during cleanup. Must be below 10,000 (maximum pagination size). + */ +const SESSION_INDEX_CLEANUP_BATCH_SIZE = 1_000; + /** * Returns index template that is used for the current version of the session index. */ @@ -490,16 +498,16 @@ export class SessionIndex { } const operations: Array>> = []; + let total = 0; try { const { body: searchResponse } = - await this.options.elasticsearchClient.search( - { - index: this.indexName, - body: { query: { bool: { should: deleteQueries } } }, - _source_includes: 'usernameHash,provider', - }, - { ignore: [409, 404] } - ); + await this.options.elasticsearchClient.search({ + index: this.indexName, + body: { query: { bool: { should: deleteQueries } } }, + _source_includes: 'usernameHash,provider', + size: SESSION_INDEX_CLEANUP_BATCH_SIZE, + }); + total = (searchResponse.hits.total as SearchTotalHits).value; searchResponse.hits.hits.forEach(({ _id, _source }) => { const { usernameHash, provider } = _source!; this.options.auditLogger.log( @@ -514,10 +522,14 @@ export class SessionIndex { if (operations.length > 0) { try { - const { body: deleteResponse } = await this.options.elasticsearchClient.bulk({ - index: this.indexName, - operations, - }); + const { body: deleteResponse } = await this.options.elasticsearchClient.bulk( + { + index: this.indexName, + operations, + refresh: false, + }, + { ignore: [409, 404] } + ); if (deleteResponse.errors) { const errorCount = deleteResponse.items.reduce( (count, item) => (item.delete!.error ? count + 1 : count), @@ -542,5 +554,9 @@ export class SessionIndex { throw err; } } + + if (total > SESSION_INDEX_CLEANUP_BATCH_SIZE) { + await this.cleanUp(); + } } } From 5ed9bfb8db01bfbbb0116e955af5a51062079c94 Mon Sep 17 00:00:00 2001 From: Thom Heymann Date: Tue, 11 Jan 2022 22:58:40 +0000 Subject: [PATCH 4/5] Clean up sessions in batches --- .../session_management/session_index.test.ts | 552 +++++++++--------- .../session_management/session_index.ts | 132 +++-- 2 files changed, 367 insertions(+), 317 deletions(-) diff --git a/x-pack/plugins/security/server/session_management/session_index.test.ts b/x-pack/plugins/security/server/session_management/session_index.test.ts index e7cb34be0278f..93f9edd15fc0c 100644 --- a/x-pack/plugins/security/server/session_management/session_index.test.ts +++ b/x-pack/plugins/security/server/session_management/session_index.test.ts @@ -6,7 +6,11 @@ */ import { errors } from '@elastic/elasticsearch'; -import type { BulkResponse, SearchResponse } from '@elastic/elasticsearch/lib/api/types'; +import type { + BulkResponse, + OpenPointInTimeResponse, + SearchResponse, +} from '@elastic/elasticsearch/lib/api/types'; import type { DeeplyMockedKeys } from '@kbn/utility-types/jest'; import type { ElasticsearchClient } from 'src/core/server'; @@ -226,11 +230,19 @@ describe('Session index', () => { const sessionValue = { _id: 'SESSION_ID', _source: { usernameHash: 'USERNAME_HASH', provider: { name: 'basic1', type: 'basic' } }, + sort: [0], }; beforeEach(() => { + mockElasticsearchClient.openPointInTime.mockResolvedValue( + securityMock.createApiResponse({ + body: { id: 'PIT_ID' } as OpenPointInTimeResponse, + }) + ); mockElasticsearchClient.search.mockResolvedValue( securityMock.createApiResponse({ - body: { hits: { total: 1, hits: [sessionValue] } } as SearchResponse, + body: { + hits: { total: { value: 1, relation: 'eq' }, hits: [sessionValue] }, + } as SearchResponse, }) ); mockElasticsearchClient.bulk.mockResolvedValue( @@ -264,48 +276,51 @@ describe('Session index', () => { expect(mockElasticsearchClient.search).toHaveBeenCalledTimes(1); expect(mockElasticsearchClient.search).toHaveBeenCalledWith({ - index: indexName, _source_includes: 'usernameHash,provider', - size: 1_000, - body: { - query: { - bool: { - should: [ - // All expired sessions based on the lifespan, no matter which provider they belong to. - { range: { lifespanExpiration: { lte: now } } }, - // All sessions that belong to the providers that aren't configured. - { - bool: { - must_not: { - bool: { - should: [ - { - bool: { - must: [ - { term: { 'provider.type': 'basic' } }, - { term: { 'provider.name': 'basic' } }, - ], - }, + sort: '_shard_doc', + search_after: undefined, + size: 10_000, + pit: { + id: 'PIT_ID', + keep_alive: '5m', + }, + query: { + bool: { + should: [ + // All expired sessions based on the lifespan, no matter which provider they belong to. + { range: { lifespanExpiration: { lte: now } } }, + // All sessions that belong to the providers that aren't configured. + { + bool: { + must_not: { + bool: { + should: [ + { + bool: { + must: [ + { term: { 'provider.type': 'basic' } }, + { term: { 'provider.name': 'basic' } }, + ], }, - ], - minimum_should_match: 1, - }, + }, + ], + minimum_should_match: 1, }, }, }, - // The sessions that belong to a particular provider that are expired based on the idle timeout. - { - bool: { - must: [ - { term: { 'provider.type': 'basic' } }, - { term: { 'provider.name': 'basic' } }, - ], - should: [{ range: { idleTimeoutExpiration: { lte: now } } }], - minimum_should_match: 1, - }, + }, + // The sessions that belong to a particular provider that are expired based on the idle timeout. + { + bool: { + must: [ + { term: { 'provider.type': 'basic' } }, + { term: { 'provider.name': 'basic' } }, + ], + should: [{ range: { idleTimeoutExpiration: { lte: now } } }], + minimum_should_match: 1, }, - ], - }, + }, + ], }, }, }); @@ -315,6 +330,7 @@ describe('Session index', () => { { index: indexName, operations: [{ delete: { _id: sessionValue._id } }], + refresh: false, }, { ignore: [409, 404], @@ -339,58 +355,61 @@ describe('Session index', () => { expect(mockElasticsearchClient.search).toHaveBeenCalledTimes(1); expect(mockElasticsearchClient.search).toHaveBeenCalledWith({ - index: indexName, _source_includes: 'usernameHash,provider', - size: 1_000, - body: { - query: { - bool: { - should: [ - // All expired sessions based on the lifespan, no matter which provider they belong to. - { range: { lifespanExpiration: { lte: now } } }, - // All sessions that belong to the providers that aren't configured. - { - bool: { - must_not: { - bool: { - should: [ - { - bool: { - must: [ - { term: { 'provider.type': 'basic' } }, - { term: { 'provider.name': 'basic' } }, - ], - }, + sort: '_shard_doc', + search_after: undefined, + size: 10_000, + pit: { + id: 'PIT_ID', + keep_alive: '5m', + }, + query: { + bool: { + should: [ + // All expired sessions based on the lifespan, no matter which provider they belong to. + { range: { lifespanExpiration: { lte: now } } }, + // All sessions that belong to the providers that aren't configured. + { + bool: { + must_not: { + bool: { + should: [ + { + bool: { + must: [ + { term: { 'provider.type': 'basic' } }, + { term: { 'provider.name': 'basic' } }, + ], }, - ], - minimum_should_match: 1, - }, + }, + ], + minimum_should_match: 1, }, }, }, - // The sessions that belong to a particular provider but don't have a configured lifespan. - { - bool: { - must: [ - { term: { 'provider.type': 'basic' } }, - { term: { 'provider.name': 'basic' } }, - ], - must_not: { exists: { field: 'lifespanExpiration' } }, - }, + }, + // The sessions that belong to a particular provider but don't have a configured lifespan. + { + bool: { + must: [ + { term: { 'provider.type': 'basic' } }, + { term: { 'provider.name': 'basic' } }, + ], + must_not: { exists: { field: 'lifespanExpiration' } }, }, - // The sessions that belong to a particular provider that are expired based on the idle timeout. - { - bool: { - must: [ - { term: { 'provider.type': 'basic' } }, - { term: { 'provider.name': 'basic' } }, - ], - should: [{ range: { idleTimeoutExpiration: { lte: now } } }], - minimum_should_match: 1, - }, + }, + // The sessions that belong to a particular provider that are expired based on the idle timeout. + { + bool: { + must: [ + { term: { 'provider.type': 'basic' } }, + { term: { 'provider.name': 'basic' } }, + ], + should: [{ range: { idleTimeoutExpiration: { lte: now } } }], + minimum_should_match: 1, }, - ], - }, + }, + ], }, }, }); @@ -400,6 +419,7 @@ describe('Session index', () => { { index: indexName, operations: [{ delete: { _id: sessionValue._id } }], + refresh: false, }, { ignore: [409, 404], @@ -425,52 +445,55 @@ describe('Session index', () => { expect(mockElasticsearchClient.search).toHaveBeenCalledTimes(1); expect(mockElasticsearchClient.search).toHaveBeenCalledWith({ - index: indexName, _source_includes: 'usernameHash,provider', - size: 1_000, - body: { - query: { - bool: { - should: [ - // All expired sessions based on the lifespan, no matter which provider they belong to. - { range: { lifespanExpiration: { lte: now } } }, - // All sessions that belong to the providers that aren't configured. - { - bool: { - must_not: { - bool: { - should: [ - { - bool: { - must: [ - { term: { 'provider.type': 'basic' } }, - { term: { 'provider.name': 'basic' } }, - ], - }, + sort: '_shard_doc', + search_after: undefined, + size: 10_000, + pit: { + id: 'PIT_ID', + keep_alive: '5m', + }, + query: { + bool: { + should: [ + // All expired sessions based on the lifespan, no matter which provider they belong to. + { range: { lifespanExpiration: { lte: now } } }, + // All sessions that belong to the providers that aren't configured. + { + bool: { + must_not: { + bool: { + should: [ + { + bool: { + must: [ + { term: { 'provider.type': 'basic' } }, + { term: { 'provider.name': 'basic' } }, + ], }, - ], - minimum_should_match: 1, - }, + }, + ], + minimum_should_match: 1, }, }, }, - // The sessions that belong to a particular provider that are either expired based on the idle timeout - // or don't have it configured at all. - { - bool: { - must: [ - { term: { 'provider.type': 'basic' } }, - { term: { 'provider.name': 'basic' } }, - ], - should: [ - { range: { idleTimeoutExpiration: { lte: now - 3 * idleTimeout } } }, - { bool: { must_not: { exists: { field: 'idleTimeoutExpiration' } } } }, - ], - minimum_should_match: 1, - }, + }, + // The sessions that belong to a particular provider that are either expired based on the idle timeout + // or don't have it configured at all. + { + bool: { + must: [ + { term: { 'provider.type': 'basic' } }, + { term: { 'provider.name': 'basic' } }, + ], + should: [ + { range: { idleTimeoutExpiration: { lte: now - 3 * idleTimeout } } }, + { bool: { must_not: { exists: { field: 'idleTimeoutExpiration' } } } }, + ], + minimum_should_match: 1, }, - ], - }, + }, + ], }, }, }); @@ -480,6 +503,7 @@ describe('Session index', () => { { index: indexName, operations: [{ delete: { _id: sessionValue._id } }], + refresh: false, }, { ignore: [409, 404], @@ -505,62 +529,65 @@ describe('Session index', () => { expect(mockElasticsearchClient.search).toHaveBeenCalledTimes(1); expect(mockElasticsearchClient.search).toHaveBeenCalledWith({ - index: indexName, _source_includes: 'usernameHash,provider', - size: 1_000, - body: { - query: { - bool: { - should: [ - // All expired sessions based on the lifespan, no matter which provider they belong to. - { range: { lifespanExpiration: { lte: now } } }, - // All sessions that belong to the providers that aren't configured. - { - bool: { - must_not: { - bool: { - should: [ - { - bool: { - must: [ - { term: { 'provider.type': 'basic' } }, - { term: { 'provider.name': 'basic' } }, - ], - }, + sort: '_shard_doc', + search_after: undefined, + size: 10_000, + pit: { + id: 'PIT_ID', + keep_alive: '5m', + }, + query: { + bool: { + should: [ + // All expired sessions based on the lifespan, no matter which provider they belong to. + { range: { lifespanExpiration: { lte: now } } }, + // All sessions that belong to the providers that aren't configured. + { + bool: { + must_not: { + bool: { + should: [ + { + bool: { + must: [ + { term: { 'provider.type': 'basic' } }, + { term: { 'provider.name': 'basic' } }, + ], }, - ], - minimum_should_match: 1, - }, + }, + ], + minimum_should_match: 1, }, }, }, - // The sessions that belong to a particular provider but don't have a configured lifespan. - { - bool: { - must: [ - { term: { 'provider.type': 'basic' } }, - { term: { 'provider.name': 'basic' } }, - ], - must_not: { exists: { field: 'lifespanExpiration' } }, - }, + }, + // The sessions that belong to a particular provider but don't have a configured lifespan. + { + bool: { + must: [ + { term: { 'provider.type': 'basic' } }, + { term: { 'provider.name': 'basic' } }, + ], + must_not: { exists: { field: 'lifespanExpiration' } }, }, - // The sessions that belong to a particular provider that are either expired based on the idle timeout - // or don't have it configured at all. - { - bool: { - must: [ - { term: { 'provider.type': 'basic' } }, - { term: { 'provider.name': 'basic' } }, - ], - should: [ - { range: { idleTimeoutExpiration: { lte: now - 3 * idleTimeout } } }, - { bool: { must_not: { exists: { field: 'idleTimeoutExpiration' } } } }, - ], - minimum_should_match: 1, - }, + }, + // The sessions that belong to a particular provider that are either expired based on the idle timeout + // or don't have it configured at all. + { + bool: { + must: [ + { term: { 'provider.type': 'basic' } }, + { term: { 'provider.name': 'basic' } }, + ], + should: [ + { range: { idleTimeoutExpiration: { lte: now - 3 * idleTimeout } } }, + { bool: { must_not: { exists: { field: 'idleTimeoutExpiration' } } } }, + ], + minimum_should_match: 1, }, - ], - }, + }, + ], }, }, }); @@ -570,6 +597,7 @@ describe('Session index', () => { { index: indexName, operations: [{ delete: { _id: sessionValue._id } }], + refresh: false, }, { ignore: [409, 404], @@ -610,95 +638,98 @@ describe('Session index', () => { expect(mockElasticsearchClient.search).toHaveBeenCalledTimes(1); expect(mockElasticsearchClient.search).toHaveBeenCalledWith({ - index: indexName, _source_includes: 'usernameHash,provider', - size: 1_000, - body: { - query: { - bool: { - should: [ - // All expired sessions based on the lifespan, no matter which provider they belong to. - { range: { lifespanExpiration: { lte: now } } }, - // All sessions that belong to the providers that aren't configured. - { - bool: { - must_not: { - bool: { - should: [ - { - bool: { - must: [ - { term: { 'provider.type': 'basic' } }, - { term: { 'provider.name': 'basic1' } }, - ], - }, + sort: '_shard_doc', + search_after: undefined, + size: 10_000, + pit: { + id: 'PIT_ID', + keep_alive: '5m', + }, + query: { + bool: { + should: [ + // All expired sessions based on the lifespan, no matter which provider they belong to. + { range: { lifespanExpiration: { lte: now } } }, + // All sessions that belong to the providers that aren't configured. + { + bool: { + must_not: { + bool: { + should: [ + { + bool: { + must: [ + { term: { 'provider.type': 'basic' } }, + { term: { 'provider.name': 'basic1' } }, + ], }, - { - bool: { - must: [ - { term: { 'provider.type': 'saml' } }, - { term: { 'provider.name': 'saml1' } }, - ], - }, + }, + { + bool: { + must: [ + { term: { 'provider.type': 'saml' } }, + { term: { 'provider.name': 'saml1' } }, + ], }, - ], - minimum_should_match: 1, - }, + }, + ], + minimum_should_match: 1, }, }, }, - // The sessions that belong to a Basic provider but don't have a configured lifespan. - { - bool: { - must: [ - { term: { 'provider.type': 'basic' } }, - { term: { 'provider.name': 'basic1' } }, - ], - must_not: { exists: { field: 'lifespanExpiration' } }, - }, + }, + // The sessions that belong to a Basic provider but don't have a configured lifespan. + { + bool: { + must: [ + { term: { 'provider.type': 'basic' } }, + { term: { 'provider.name': 'basic1' } }, + ], + must_not: { exists: { field: 'lifespanExpiration' } }, }, - // The sessions that belong to a Basic provider that are either expired based on the idle timeout - // or don't have it configured at all. - { - bool: { - must: [ - { term: { 'provider.type': 'basic' } }, - { term: { 'provider.name': 'basic1' } }, - ], - should: [ - { range: { idleTimeoutExpiration: { lte: now - 3 * globalIdleTimeout } } }, - { bool: { must_not: { exists: { field: 'idleTimeoutExpiration' } } } }, - ], - minimum_should_match: 1, - }, + }, + // The sessions that belong to a Basic provider that are either expired based on the idle timeout + // or don't have it configured at all. + { + bool: { + must: [ + { term: { 'provider.type': 'basic' } }, + { term: { 'provider.name': 'basic1' } }, + ], + should: [ + { range: { idleTimeoutExpiration: { lte: now - 3 * globalIdleTimeout } } }, + { bool: { must_not: { exists: { field: 'idleTimeoutExpiration' } } } }, + ], + minimum_should_match: 1, }, - // The sessions that belong to a SAML provider but don't have a configured lifespan. - { - bool: { - must: [ - { term: { 'provider.type': 'saml' } }, - { term: { 'provider.name': 'saml1' } }, - ], - must_not: { exists: { field: 'lifespanExpiration' } }, - }, + }, + // The sessions that belong to a SAML provider but don't have a configured lifespan. + { + bool: { + must: [ + { term: { 'provider.type': 'saml' } }, + { term: { 'provider.name': 'saml1' } }, + ], + must_not: { exists: { field: 'lifespanExpiration' } }, }, - // The sessions that belong to a SAML provider that are either expired based on the idle timeout - // or don't have it configured at all. - { - bool: { - must: [ - { term: { 'provider.type': 'saml' } }, - { term: { 'provider.name': 'saml1' } }, - ], - should: [ - { range: { idleTimeoutExpiration: { lte: now - 3 * samlIdleTimeout } } }, - { bool: { must_not: { exists: { field: 'idleTimeoutExpiration' } } } }, - ], - minimum_should_match: 1, - }, + }, + // The sessions that belong to a SAML provider that are either expired based on the idle timeout + // or don't have it configured at all. + { + bool: { + must: [ + { term: { 'provider.type': 'saml' } }, + { term: { 'provider.name': 'saml1' } }, + ], + should: [ + { range: { idleTimeoutExpiration: { lte: now - 3 * samlIdleTimeout } } }, + { bool: { must_not: { exists: { field: 'idleTimeoutExpiration' } } } }, + ], + minimum_should_match: 1, }, - ], - }, + }, + ], }, }, }); @@ -708,6 +739,7 @@ describe('Session index', () => { { index: indexName, operations: [{ delete: { _id: sessionValue._id } }], + refresh: false, }, { ignore: [409, 404], @@ -715,16 +747,14 @@ describe('Session index', () => { ); }); - it('should clean up sessions in batches of 1000', async () => { - for (const value of [2500, 1500, 500]) { - mockElasticsearchClient.search.mockResolvedValueOnce( - securityMock.createApiResponse({ - body: { - hits: { total: { value, relation: 'eq' }, hits: [sessionValue] }, - } as SearchResponse, - }) - ); - } + it('should clean up sessions in batches of 10,000', async () => { + mockElasticsearchClient.search.mockResolvedValue( + securityMock.createApiResponse({ + body: { + hits: { total: { value: 3 * 10_000, relation: 'eq' }, hits: [sessionValue] }, + } as SearchResponse, + }) + ); await sessionIndex.cleanUp(); diff --git a/x-pack/plugins/security/server/session_management/session_index.ts b/x-pack/plugins/security/server/session_management/session_index.ts index df3fa30ffb8aa..037c47ca79104 100644 --- a/x-pack/plugins/security/server/session_management/session_index.ts +++ b/x-pack/plugins/security/server/session_management/session_index.ts @@ -8,6 +8,7 @@ import type { BulkOperationContainer, SearchTotalHits, + SortResults, } from '@elastic/elasticsearch/lib/api/typesWithBodyKey'; import type { ElasticsearchClient, Logger } from 'src/core/server'; @@ -43,9 +44,14 @@ export type InvalidateSessionsFilter = const SESSION_INDEX_TEMPLATE_VERSION = 1; /** - * Number of sessions to remove per batch during cleanup. Must be below 10,000 (maximum pagination size). + * Number of sessions to remove per batch during cleanup. */ -const SESSION_INDEX_CLEANUP_BATCH_SIZE = 1_000; +const SESSION_INDEX_CLEANUP_BATCH_SIZE = 10_000; + +/** + * Number of sessions to remove per batch during cleanup. + */ +const SESSION_INDEX_CLEANUP_KEEP_ALIVE = '5m'; /** * Returns index template that is used for the current version of the session index. @@ -438,6 +444,56 @@ export class SessionIndex { async cleanUp() { this.options.logger.debug(`Running cleanup routine.`); + try { + for await (const sessionValues of this.getSessionValuesInBatches()) { + const operations: Array>> = []; + sessionValues.forEach(({ _id, _source }) => { + const { usernameHash, provider } = _source!; + this.options.auditLogger.log( + sessionCleanupEvent({ sessionId: _id, usernameHash, provider }) + ); + operations.push({ delete: { _id } }); + }); + if (operations.length > 0) { + const { body: bulkResponse } = await this.options.elasticsearchClient.bulk( + { + index: this.indexName, + operations, + refresh: false, + }, + { ignore: [409, 404] } + ); + if (bulkResponse.errors) { + const errorCount = bulkResponse.items.reduce( + (count, item) => (item.delete!.error ? count + 1 : count), + 0 + ); + if (errorCount < bulkResponse.items.length) { + this.options.logger.warn( + `Failed to clean up ${errorCount} of ${bulkResponse.items.length} invalid or expired sessions. The remaining sessions were cleaned up successfully.` + ); + } else { + this.options.logger.error( + `Failed to clean up ${bulkResponse.items.length} invalid or expired sessions.` + ); + } + } else { + this.options.logger.debug( + `Cleaned up ${bulkResponse.items.length} invalid or expired sessions.` + ); + } + } + } + } catch (err) { + this.options.logger.error(`Failed to clean up sessions: ${err.message}`); + throw err; + } + } + + /** + * Fetches session values from session index in batches of 10,000. + */ + private async *getSessionValuesInBatches() { const now = Date.now(); const providersSessionConfig = this.options.config.authc.sortedProviders.map((provider) => { return { @@ -497,66 +553,30 @@ export class SessionIndex { }); } - const operations: Array>> = []; - let total = 0; - try { + // Create point in time snapshot to paginate through sessions + const { body: pitResponse } = await this.options.elasticsearchClient.openPointInTime({ + index: this.indexName, + keep_alive: SESSION_INDEX_CLEANUP_KEEP_ALIVE, + }); + + // We don't know the total number of sessions until we fetched the first batch so assume we have at least one session to clean. + let total = 1; + let searchAfter: SortResults | undefined; + for (let i = 0; i < total / SESSION_INDEX_CLEANUP_BATCH_SIZE; i++) { const { body: searchResponse } = await this.options.elasticsearchClient.search({ - index: this.indexName, - body: { query: { bool: { should: deleteQueries } } }, + pit: { id: pitResponse.id, keep_alive: SESSION_INDEX_CLEANUP_KEEP_ALIVE }, _source_includes: 'usernameHash,provider', + query: { bool: { should: deleteQueries } }, + search_after: searchAfter, size: SESSION_INDEX_CLEANUP_BATCH_SIZE, + sort: '_shard_doc', }); - total = (searchResponse.hits.total as SearchTotalHits).value; - searchResponse.hits.hits.forEach(({ _id, _source }) => { - const { usernameHash, provider } = _source!; - this.options.auditLogger.log( - sessionCleanupEvent({ sessionId: _id, usernameHash, provider }) - ); - operations.push({ delete: { _id } }); - }); - } catch (err) { - this.options.logger.error(`Failed to look up invalid or expired sessions: ${err.message}`); - throw err; - } - - if (operations.length > 0) { - try { - const { body: deleteResponse } = await this.options.elasticsearchClient.bulk( - { - index: this.indexName, - operations, - refresh: false, - }, - { ignore: [409, 404] } - ); - if (deleteResponse.errors) { - const errorCount = deleteResponse.items.reduce( - (count, item) => (item.delete!.error ? count + 1 : count), - 0 - ); - if (errorCount < deleteResponse.items.length) { - this.options.logger.warn( - `Failed to clean up ${errorCount} of ${deleteResponse.items.length} invalid or expired sessions. The remaining sessions were cleaned up successfully.` - ); - } else { - this.options.logger.error( - `Failed to clean up ${deleteResponse.items.length} invalid or expired sessions.` - ); - } - } else { - this.options.logger.debug( - `Cleaned up ${deleteResponse.items.length} invalid or expired sessions.` - ); - } - } catch (err) { - this.options.logger.error(`Failed to clean up sessions: ${err.message}`); - throw err; + if (searchResponse.hits.hits.length > 0) { + yield searchResponse.hits.hits; + total = (searchResponse.hits.total as SearchTotalHits).value; + searchAfter = searchResponse.hits.hits[searchResponse.hits.hits.length - 1].sort; } } - - if (total > SESSION_INDEX_CLEANUP_BATCH_SIZE) { - await this.cleanUp(); - } } } From 52d61cd35f174fb4604a3f2b0fc813457651c4c0 Mon Sep 17 00:00:00 2001 From: Thom Heymann Date: Wed, 12 Jan 2022 19:36:38 +0000 Subject: [PATCH 5/5] Added suggestions form code review --- .../session_management/session_index.test.ts | 63 ++++++++++++++++--- .../session_management/session_index.ts | 54 +++++++++------- 2 files changed, 87 insertions(+), 30 deletions(-) diff --git a/x-pack/plugins/security/server/session_management/session_index.test.ts b/x-pack/plugins/security/server/session_management/session_index.test.ts index 93f9edd15fc0c..45ce865de5635 100644 --- a/x-pack/plugins/security/server/session_management/session_index.test.ts +++ b/x-pack/plugins/security/server/session_management/session_index.test.ts @@ -8,6 +8,7 @@ import { errors } from '@elastic/elasticsearch'; import type { BulkResponse, + ClosePointInTimeResponse, OpenPointInTimeResponse, SearchResponse, } from '@elastic/elasticsearch/lib/api/types'; @@ -238,10 +239,15 @@ describe('Session index', () => { body: { id: 'PIT_ID' } as OpenPointInTimeResponse, }) ); + mockElasticsearchClient.closePointInTime.mockResolvedValue( + securityMock.createApiResponse({ + body: { succeeded: true, num_freed: 1 } as ClosePointInTimeResponse, + }) + ); mockElasticsearchClient.search.mockResolvedValue( securityMock.createApiResponse({ body: { - hits: { total: { value: 1, relation: 'eq' }, hits: [sessionValue] }, + hits: { hits: [sessionValue] }, } as SearchResponse, }) ); @@ -260,6 +266,10 @@ describe('Session index', () => { mockElasticsearchClient.search.mockRejectedValue(failureReason); await expect(sessionIndex.cleanUp()).rejects.toBe(failureReason); + expect(mockElasticsearchClient.openPointInTime).toHaveBeenCalledTimes(1); + expect(mockElasticsearchClient.search).toHaveBeenCalledTimes(1); + expect(mockElasticsearchClient.bulk).not.toHaveBeenCalled(); + expect(mockElasticsearchClient.closePointInTime).toHaveBeenCalledTimes(1); }); it('throws if bulk delete call to Elasticsearch fails', async () => { @@ -269,15 +279,21 @@ describe('Session index', () => { mockElasticsearchClient.bulk.mockRejectedValue(failureReason); await expect(sessionIndex.cleanUp()).rejects.toBe(failureReason); + expect(mockElasticsearchClient.openPointInTime).toHaveBeenCalledTimes(1); + expect(mockElasticsearchClient.search).toHaveBeenCalledTimes(1); + expect(mockElasticsearchClient.bulk).toHaveBeenCalledTimes(1); + expect(mockElasticsearchClient.closePointInTime).toHaveBeenCalledTimes(1); }); it('when neither `lifespan` nor `idleTimeout` is configured', async () => { await sessionIndex.cleanUp(); + expect(mockElasticsearchClient.openPointInTime).toHaveBeenCalledTimes(1); expect(mockElasticsearchClient.search).toHaveBeenCalledTimes(1); expect(mockElasticsearchClient.search).toHaveBeenCalledWith({ _source_includes: 'usernameHash,provider', sort: '_shard_doc', + track_total_hits: false, search_after: undefined, size: 10_000, pit: { @@ -324,7 +340,6 @@ describe('Session index', () => { }, }, }); - expect(mockElasticsearchClient.bulk).toHaveBeenCalledTimes(1); expect(mockElasticsearchClient.bulk).toHaveBeenCalledWith( { @@ -336,6 +351,7 @@ describe('Session index', () => { ignore: [409, 404], } ); + expect(mockElasticsearchClient.closePointInTime).toHaveBeenCalledTimes(1); }); it('when only `lifespan` is configured', async () => { @@ -353,10 +369,12 @@ describe('Session index', () => { await sessionIndex.cleanUp(); + expect(mockElasticsearchClient.openPointInTime).toHaveBeenCalledTimes(1); expect(mockElasticsearchClient.search).toHaveBeenCalledTimes(1); expect(mockElasticsearchClient.search).toHaveBeenCalledWith({ _source_includes: 'usernameHash,provider', sort: '_shard_doc', + track_total_hits: false, search_after: undefined, size: 10_000, pit: { @@ -413,7 +431,6 @@ describe('Session index', () => { }, }, }); - expect(mockElasticsearchClient.bulk).toHaveBeenCalledTimes(1); expect(mockElasticsearchClient.bulk).toHaveBeenCalledWith( { @@ -425,6 +442,7 @@ describe('Session index', () => { ignore: [409, 404], } ); + expect(mockElasticsearchClient.closePointInTime).toHaveBeenCalledTimes(1); }); it('when only `idleTimeout` is configured', async () => { @@ -443,10 +461,12 @@ describe('Session index', () => { await sessionIndex.cleanUp(); + expect(mockElasticsearchClient.openPointInTime).toHaveBeenCalledTimes(1); expect(mockElasticsearchClient.search).toHaveBeenCalledTimes(1); expect(mockElasticsearchClient.search).toHaveBeenCalledWith({ _source_includes: 'usernameHash,provider', sort: '_shard_doc', + track_total_hits: false, search_after: undefined, size: 10_000, pit: { @@ -497,7 +517,6 @@ describe('Session index', () => { }, }, }); - expect(mockElasticsearchClient.bulk).toHaveBeenCalledTimes(1); expect(mockElasticsearchClient.bulk).toHaveBeenCalledWith( { @@ -509,6 +528,7 @@ describe('Session index', () => { ignore: [409, 404], } ); + expect(mockElasticsearchClient.closePointInTime).toHaveBeenCalledTimes(1); }); it('when both `lifespan` and `idleTimeout` are configured', async () => { @@ -527,10 +547,12 @@ describe('Session index', () => { await sessionIndex.cleanUp(); + expect(mockElasticsearchClient.openPointInTime).toHaveBeenCalledTimes(1); expect(mockElasticsearchClient.search).toHaveBeenCalledTimes(1); expect(mockElasticsearchClient.search).toHaveBeenCalledWith({ _source_includes: 'usernameHash,provider', sort: '_shard_doc', + track_total_hits: false, search_after: undefined, size: 10_000, pit: { @@ -591,7 +613,6 @@ describe('Session index', () => { }, }, }); - expect(mockElasticsearchClient.bulk).toHaveBeenCalledTimes(1); expect(mockElasticsearchClient.bulk).toHaveBeenCalledWith( { @@ -603,6 +624,7 @@ describe('Session index', () => { ignore: [409, 404], } ); + expect(mockElasticsearchClient.closePointInTime).toHaveBeenCalledTimes(1); }); it('when both `lifespan` and `idleTimeout` are configured and multiple providers are enabled', async () => { @@ -636,10 +658,12 @@ describe('Session index', () => { await sessionIndex.cleanUp(); + expect(mockElasticsearchClient.openPointInTime).toHaveBeenCalledTimes(1); expect(mockElasticsearchClient.search).toHaveBeenCalledTimes(1); expect(mockElasticsearchClient.search).toHaveBeenCalledWith({ _source_includes: 'usernameHash,provider', sort: '_shard_doc', + track_total_hits: false, search_after: undefined, size: 10_000, pit: { @@ -733,7 +757,6 @@ describe('Session index', () => { }, }, }); - expect(mockElasticsearchClient.bulk).toHaveBeenCalledTimes(1); expect(mockElasticsearchClient.bulk).toHaveBeenCalledWith( { @@ -745,21 +768,43 @@ describe('Session index', () => { ignore: [409, 404], } ); + expect(mockElasticsearchClient.closePointInTime).toHaveBeenCalledTimes(1); }); it('should clean up sessions in batches of 10,000', async () => { + for (const count of [10_000, 1]) { + mockElasticsearchClient.search.mockResolvedValueOnce( + securityMock.createApiResponse({ + body: { + hits: { hits: new Array(count).fill(sessionValue, 0) }, + } as SearchResponse, + }) + ); + } + + await sessionIndex.cleanUp(); + + expect(mockElasticsearchClient.openPointInTime).toHaveBeenCalledTimes(1); + expect(mockElasticsearchClient.search).toHaveBeenCalledTimes(2); + expect(mockElasticsearchClient.bulk).toHaveBeenCalledTimes(2); + expect(mockElasticsearchClient.closePointInTime).toHaveBeenCalledTimes(1); + }); + + it('should limit number of batches to 10', async () => { mockElasticsearchClient.search.mockResolvedValue( securityMock.createApiResponse({ body: { - hits: { total: { value: 3 * 10_000, relation: 'eq' }, hits: [sessionValue] }, + hits: { hits: new Array(10_000).fill(sessionValue, 0) }, } as SearchResponse, }) ); await sessionIndex.cleanUp(); - expect(mockElasticsearchClient.search).toHaveBeenCalledTimes(3); - expect(mockElasticsearchClient.bulk).toHaveBeenCalledTimes(3); + expect(mockElasticsearchClient.openPointInTime).toHaveBeenCalledTimes(1); + expect(mockElasticsearchClient.search).toHaveBeenCalledTimes(10); + expect(mockElasticsearchClient.bulk).toHaveBeenCalledTimes(10); + expect(mockElasticsearchClient.closePointInTime).toHaveBeenCalledTimes(1); }); it('should log audit event', async () => { diff --git a/x-pack/plugins/security/server/session_management/session_index.ts b/x-pack/plugins/security/server/session_management/session_index.ts index 037c47ca79104..e064a735bc031 100644 --- a/x-pack/plugins/security/server/session_management/session_index.ts +++ b/x-pack/plugins/security/server/session_management/session_index.ts @@ -7,7 +7,6 @@ import type { BulkOperationContainer, - SearchTotalHits, SortResults, } from '@elastic/elasticsearch/lib/api/typesWithBodyKey'; @@ -49,7 +48,13 @@ const SESSION_INDEX_TEMPLATE_VERSION = 1; const SESSION_INDEX_CLEANUP_BATCH_SIZE = 10_000; /** - * Number of sessions to remove per batch during cleanup. + * Maximum number of batches per cleanup. + * If the batch size is 10,000 and this limit is 10, then Kibana will remove up to 100k sessions per cleanup. + */ +const SESSION_INDEX_CLEANUP_BATCH_LIMIT = 10; + +/** + * How long the session cleanup search point-in-time should be kept alive. */ const SESSION_INDEX_CLEANUP_KEEP_ALIVE = '5m'; @@ -553,30 +558,37 @@ export class SessionIndex { }); } - // Create point in time snapshot to paginate through sessions - const { body: pitResponse } = await this.options.elasticsearchClient.openPointInTime({ + const { body: openPitResponse } = await this.options.elasticsearchClient.openPointInTime({ index: this.indexName, keep_alive: SESSION_INDEX_CLEANUP_KEEP_ALIVE, }); - // We don't know the total number of sessions until we fetched the first batch so assume we have at least one session to clean. - let total = 1; - let searchAfter: SortResults | undefined; - for (let i = 0; i < total / SESSION_INDEX_CLEANUP_BATCH_SIZE; i++) { - const { body: searchResponse } = - await this.options.elasticsearchClient.search({ - pit: { id: pitResponse.id, keep_alive: SESSION_INDEX_CLEANUP_KEEP_ALIVE }, - _source_includes: 'usernameHash,provider', - query: { bool: { should: deleteQueries } }, - search_after: searchAfter, - size: SESSION_INDEX_CLEANUP_BATCH_SIZE, - sort: '_shard_doc', - }); - if (searchResponse.hits.hits.length > 0) { - yield searchResponse.hits.hits; - total = (searchResponse.hits.total as SearchTotalHits).value; - searchAfter = searchResponse.hits.hits[searchResponse.hits.hits.length - 1].sort; + try { + let searchAfter: SortResults | undefined; + for (let i = 0; i < SESSION_INDEX_CLEANUP_BATCH_LIMIT; i++) { + const { body: searchResponse } = + await this.options.elasticsearchClient.search({ + pit: { id: openPitResponse.id, keep_alive: SESSION_INDEX_CLEANUP_KEEP_ALIVE }, + _source_includes: 'usernameHash,provider', + query: { bool: { should: deleteQueries } }, + search_after: searchAfter, + size: SESSION_INDEX_CLEANUP_BATCH_SIZE, + sort: '_shard_doc', + track_total_hits: false, // for performance + }); + const { hits } = searchResponse.hits; + if (hits.length > 0) { + yield hits; + searchAfter = hits[hits.length - 1].sort; + } + if (hits.length < SESSION_INDEX_CLEANUP_BATCH_SIZE) { + break; + } } + } finally { + await this.options.elasticsearchClient.closePointInTime({ + id: openPitResponse.id, + }); } } }