diff --git a/x-pack/plugins/security_solution/server/lib/detection_engine/notifications/schedule_throttle_notification_actions.test.ts b/x-pack/plugins/security_solution/server/lib/detection_engine/notifications/schedule_throttle_notification_actions.test.ts index 2e5e331b71b00..81f229c636bd8 100644 --- a/x-pack/plugins/security_solution/server/lib/detection_engine/notifications/schedule_throttle_notification_actions.test.ts +++ b/x-pack/plugins/security_solution/server/lib/detection_engine/notifications/schedule_throttle_notification_actions.test.ts @@ -5,7 +5,7 @@ * 2.0. */ -import { elasticsearchServiceMock } from 'src/core/server/mocks'; +import { elasticsearchServiceMock, loggingSystemMock } from 'src/core/server/mocks'; import { alertsMock } from '../../../../../alerting/server/mocks'; import { scheduleThrottledNotificationActions } from './schedule_throttle_notification_actions'; import { @@ -19,8 +19,10 @@ jest.mock('./schedule_notification_actions', () => ({ describe('schedule_throttle_notification_actions', () => { let notificationRuleParams: NotificationRuleTypeParams; + let logger: ReturnType; beforeEach(() => { + logger = loggingSystemMock.createLogger(); (scheduleNotificationActions as jest.Mock).mockReset(); notificationRuleParams = { author: ['123'], @@ -82,6 +84,38 @@ describe('schedule_throttle_notification_actions', () => { ), alertInstance: alertsMock.createAlertInstanceFactory(), notificationRuleParams, + logger, + signals: [], + }); + + expect(scheduleNotificationActions as jest.Mock).toHaveBeenCalled(); + }); + + it('should call "scheduleNotificationActions" if the signals length is 1 or greater', async () => { + await scheduleThrottledNotificationActions({ + throttle: '1d', + startedAt: new Date('2021-08-24T19:19:22.094Z'), + id: '123', + kibanaSiemAppUrl: 'http://www.example.com', + outputIndex: 'output-123', + ruleId: 'rule-123', + esClient: elasticsearchServiceMock.createElasticsearchClient( + elasticsearchServiceMock.createSuccessTransportRequestPromise({ + hits: { + hits: [], + total: 0, + }, + }) + ), + alertInstance: alertsMock.createAlertInstanceFactory(), + notificationRuleParams, + logger, + signals: [ + { + _id: '123', + index: '123', + }, + ], }); expect(scheduleNotificationActions as jest.Mock).toHaveBeenCalled(); @@ -105,6 +139,8 @@ describe('schedule_throttle_notification_actions', () => { ), alertInstance: alertsMock.createAlertInstanceFactory(), notificationRuleParams, + logger, + signals: [], }); expect(scheduleNotificationActions as jest.Mock).not.toHaveBeenCalled(); @@ -132,6 +168,8 @@ describe('schedule_throttle_notification_actions', () => { ), alertInstance: alertsMock.createAlertInstanceFactory(), notificationRuleParams, + logger, + signals: [], }); expect(scheduleNotificationActions as jest.Mock).not.toHaveBeenCalled(); @@ -161,6 +199,8 @@ describe('schedule_throttle_notification_actions', () => { ), alertInstance: alertsMock.createAlertInstanceFactory(), notificationRuleParams, + logger, + signals: [], }); expect((scheduleNotificationActions as jest.Mock).mock.calls[0][0].resultsLink).toMatch( @@ -174,4 +214,384 @@ describe('schedule_throttle_notification_actions', () => { }) ); }); + + it('should log debug information when passing through in expected format and no error messages', async () => { + await scheduleThrottledNotificationActions({ + throttle: '1d', + startedAt: new Date('2021-08-24T19:19:22.094Z'), + id: '123', + kibanaSiemAppUrl: 'http://www.example.com', + outputIndex: 'output-123', + ruleId: 'rule-123', + esClient: elasticsearchServiceMock.createElasticsearchClient( + elasticsearchServiceMock.createSuccessTransportRequestPromise({ + hits: { + hits: [ + { + _source: {}, + }, + ], + total: 1, + }, + }) + ), + alertInstance: alertsMock.createAlertInstanceFactory(), + notificationRuleParams, + logger, + signals: [], + }); + // We only test the first part since it has date math using math + expect(logger.debug.mock.calls[0][0]).toMatch( + /The notification throttle resultsLink created is/ + ); + expect(logger.debug.mock.calls[1][0]).toEqual( + 'The notification throttle query result size before deconflicting duplicates is: 1. The notification throttle passed in signals size before deconflicting duplicates is: 0. The deconflicted size and size of the signals sent into throttle notification is: 1. The signals count from results size is: 1. The final signals count being sent to the notification is: 1.' + ); + // error should not have been called in this case. + expect(logger.error).not.toHaveBeenCalled(); + }); + + it('should log error information if "throttle" is an invalid string', async () => { + await scheduleThrottledNotificationActions({ + throttle: 'invalid', + startedAt: new Date('2021-08-24T19:19:22.094Z'), + id: '123', + kibanaSiemAppUrl: 'http://www.example.com', + outputIndex: 'output-123', + ruleId: 'rule-123', + esClient: elasticsearchServiceMock.createElasticsearchClient( + elasticsearchServiceMock.createSuccessTransportRequestPromise({ + hits: { + hits: [ + { + _source: {}, + }, + ], + total: 1, + }, + }) + ), + alertInstance: alertsMock.createAlertInstanceFactory(), + notificationRuleParams, + logger, + signals: [], + }); + + expect(logger.error).toHaveBeenCalledWith( + 'The notification throttle "from" and/or "to" range values could not be constructed as valid. Tried to construct the values of "from": now-invalid "to": 2021-08-24T19:19:22.094Z. This will cause a reset of the notification throttle. Expect either missing alert notifications or alert notifications happening earlier than expected.' + ); + }); + + it('should count correctly if it does a deconflict', async () => { + await scheduleThrottledNotificationActions({ + throttle: '1d', + startedAt: new Date('2021-08-24T19:19:22.094Z'), + id: '123', + kibanaSiemAppUrl: 'http://www.example.com', + outputIndex: 'output-123', + ruleId: 'rule-123', + esClient: elasticsearchServiceMock.createElasticsearchClient( + elasticsearchServiceMock.createSuccessTransportRequestPromise({ + hits: { + hits: [ + { + _index: 'index-123', + _id: 'id-123', + _source: { + test: 123, + }, + }, + { + _index: 'index-456', + _id: 'id-456', + _source: { + test: 456, + }, + }, + ], + total: 2, + }, + }) + ), + alertInstance: alertsMock.createAlertInstanceFactory(), + notificationRuleParams, + logger, + signals: [ + { + _index: 'index-456', + _id: 'id-456', + test: 456, + }, + ], + }); + expect(scheduleNotificationActions).toHaveBeenCalledWith( + expect.objectContaining({ + signalsCount: 2, + signals: [ + { + _id: 'id-456', + _index: 'index-456', + test: 456, + }, + { + _id: 'id-123', + _index: 'index-123', + test: 123, + }, + ], + ruleParams: notificationRuleParams, + }) + ); + }); + + it('should count correctly if it does not do a deconflict', async () => { + await scheduleThrottledNotificationActions({ + throttle: '1d', + startedAt: new Date('2021-08-24T19:19:22.094Z'), + id: '123', + kibanaSiemAppUrl: 'http://www.example.com', + outputIndex: 'output-123', + ruleId: 'rule-123', + esClient: elasticsearchServiceMock.createElasticsearchClient( + elasticsearchServiceMock.createSuccessTransportRequestPromise({ + hits: { + hits: [ + { + _index: 'index-123', + _id: 'id-123', + _source: { + test: 123, + }, + }, + { + _index: 'index-456', + _id: 'id-456', + _source: { + test: 456, + }, + }, + ], + total: 2, + }, + }) + ), + alertInstance: alertsMock.createAlertInstanceFactory(), + notificationRuleParams, + logger, + signals: [ + { + _index: 'index-789', + _id: 'id-789', + test: 456, + }, + ], + }); + expect(scheduleNotificationActions).toHaveBeenCalledWith( + expect.objectContaining({ + signalsCount: 3, + signals: [ + { + _id: 'id-789', + _index: 'index-789', + test: 456, + }, + { + _id: 'id-123', + _index: 'index-123', + test: 123, + }, + { + _id: 'id-456', + _index: 'index-456', + test: 456, + }, + ], + ruleParams: notificationRuleParams, + }) + ); + }); + + it('should count total hit with extra total elements', async () => { + await scheduleThrottledNotificationActions({ + throttle: '1d', + startedAt: new Date('2021-08-24T19:19:22.094Z'), + id: '123', + kibanaSiemAppUrl: 'http://www.example.com', + outputIndex: 'output-123', + ruleId: 'rule-123', + esClient: elasticsearchServiceMock.createElasticsearchClient( + elasticsearchServiceMock.createSuccessTransportRequestPromise({ + hits: { + hits: [ + { + _index: 'index-123', + _id: 'id-123', + _source: { + test: 123, + }, + }, + ], + total: 20, // total can be different from the actual return values so we have to ensure we count these. + }, + }) + ), + alertInstance: alertsMock.createAlertInstanceFactory(), + notificationRuleParams, + logger, + signals: [ + { + _index: 'index-789', + _id: 'id-789', + test: 456, + }, + ], + }); + expect(scheduleNotificationActions).toHaveBeenCalledWith( + expect.objectContaining({ + signalsCount: 21, + signals: [ + { + _id: 'id-789', + _index: 'index-789', + test: 456, + }, + { + _id: 'id-123', + _index: 'index-123', + test: 123, + }, + ], + ruleParams: notificationRuleParams, + }) + ); + }); + + it('should count correctly if it does a deconflict and the total has extra values', async () => { + await scheduleThrottledNotificationActions({ + throttle: '1d', + startedAt: new Date('2021-08-24T19:19:22.094Z'), + id: '123', + kibanaSiemAppUrl: 'http://www.example.com', + outputIndex: 'output-123', + ruleId: 'rule-123', + esClient: elasticsearchServiceMock.createElasticsearchClient( + elasticsearchServiceMock.createSuccessTransportRequestPromise({ + hits: { + hits: [ + { + _index: 'index-123', + _id: 'id-123', + _source: { + test: 123, + }, + }, + { + _index: 'index-456', + _id: 'id-456', + _source: { + test: 456, + }, + }, + ], + total: 20, // total can be different from the actual return values so we have to ensure we count these. + }, + }) + ), + alertInstance: alertsMock.createAlertInstanceFactory(), + notificationRuleParams, + logger, + signals: [ + { + _index: 'index-456', + _id: 'id-456', + test: 456, + }, + ], + }); + expect(scheduleNotificationActions).toHaveBeenCalledWith( + expect.objectContaining({ + signalsCount: 20, + signals: [ + { + _id: 'id-456', + _index: 'index-456', + test: 456, + }, + { + _id: 'id-123', + _index: 'index-123', + test: 123, + }, + ], + ruleParams: notificationRuleParams, + }) + ); + }); + + it('should add extra count element if it has signals added', async () => { + await scheduleThrottledNotificationActions({ + throttle: '1d', + startedAt: new Date('2021-08-24T19:19:22.094Z'), + id: '123', + kibanaSiemAppUrl: 'http://www.example.com', + outputIndex: 'output-123', + ruleId: 'rule-123', + esClient: elasticsearchServiceMock.createElasticsearchClient( + elasticsearchServiceMock.createSuccessTransportRequestPromise({ + hits: { + hits: [ + { + _index: 'index-123', + _id: 'id-123', + _source: { + test: 123, + }, + }, + { + _index: 'index-456', + _id: 'id-456', + _source: { + test: 456, + }, + }, + ], + total: 20, // total can be different from the actual return values so we have to ensure we count these. + }, + }) + ), + alertInstance: alertsMock.createAlertInstanceFactory(), + notificationRuleParams, + logger, + signals: [ + { + _index: 'index-789', + _id: 'id-789', + test: 789, + }, + ], + }); + expect(scheduleNotificationActions).toHaveBeenCalledWith( + expect.objectContaining({ + signalsCount: 21, // should be 1 more than the total since we pushed in an extra signal + signals: [ + { + _id: 'id-789', + _index: 'index-789', + test: 789, + }, + { + _id: 'id-123', + _index: 'index-123', + test: 123, + }, + { + _id: 'id-456', + _index: 'index-456', + test: 456, + }, + ], + ruleParams: notificationRuleParams, + }) + ); + }); }); diff --git a/x-pack/plugins/security_solution/server/lib/detection_engine/notifications/schedule_throttle_notification_actions.ts b/x-pack/plugins/security_solution/server/lib/detection_engine/notifications/schedule_throttle_notification_actions.ts index 5dd583d47b403..5bf18496e6375 100644 --- a/x-pack/plugins/security_solution/server/lib/detection_engine/notifications/schedule_throttle_notification_actions.ts +++ b/x-pack/plugins/security_solution/server/lib/detection_engine/notifications/schedule_throttle_notification_actions.ts @@ -5,11 +5,11 @@ * 2.0. */ -import { ElasticsearchClient, SavedObject } from 'src/core/server'; +import { ElasticsearchClient, SavedObject, Logger } from 'src/core/server'; import { parseScheduleDates } from '@kbn/securitysolution-io-ts-utils'; import { AlertInstance } from '../../../../../alerting/server'; import { RuleParams } from '../schemas/rule_schemas'; -import { getNotificationResultsLink } from '../notifications/utils'; +import { deconflictSignalsAndResults, getNotificationResultsLink } from '../notifications/utils'; import { DEFAULT_RULE_NOTIFICATION_QUERY_SIZE } from '../../../../common/constants'; import { getSignals } from '../notifications/get_signals'; import { @@ -18,8 +18,25 @@ import { } from './schedule_notification_actions'; import { AlertAttributes } from '../signals/types'; +interface ScheduleThrottledNotificationActionsOptions { + id: SavedObject['id']; + startedAt: Date; + throttle: AlertAttributes['throttle']; + kibanaSiemAppUrl: string | undefined; + outputIndex: RuleParams['outputIndex']; + ruleId: RuleParams['ruleId']; + esClient: ElasticsearchClient; + alertInstance: AlertInstance; + notificationRuleParams: NotificationRuleTypeParams; + signals: unknown[]; + logger: Logger; +} + /** * Schedules a throttled notification action for executor rules. + * NOTE: It's important that since this is throttled that you call this in _ALL_ cases including error conditions or results being empty or not a success. + * If you do not call this within your rule executor then this will cause a "reset" and will stop "throttling" and the next call will cause an immediate action + * to be sent through the system. * @param throttle The throttle which is the alerting saved object throttle * @param startedAt When the executor started at * @param id The id the alert which caused the notifications @@ -40,17 +57,9 @@ export const scheduleThrottledNotificationActions = async ({ esClient, alertInstance, notificationRuleParams, -}: { - id: SavedObject['id']; - startedAt: Date; - throttle: AlertAttributes['throttle']; - kibanaSiemAppUrl: string | undefined; - outputIndex: RuleParams['outputIndex']; - ruleId: RuleParams['ruleId']; - esClient: ElasticsearchClient; - alertInstance: AlertInstance; - notificationRuleParams: NotificationRuleTypeParams; -}): Promise => { + signals, + logger, +}: ScheduleThrottledNotificationActionsOptions): Promise => { const fromInMs = parseScheduleDates(`now-${throttle}`); const toInMs = parseScheduleDates(startedAt.toISOString()); @@ -62,6 +71,22 @@ export const scheduleThrottledNotificationActions = async ({ kibanaSiemAppUrl, }); + logger.debug( + [ + `The notification throttle resultsLink created is: ${resultsLink}.`, + ' Notification throttle is querying the results using', + ` "from:" ${fromInMs.valueOf()}`, + ' "to":', + ` ${toInMs.valueOf()}`, + ' "size":', + ` ${DEFAULT_RULE_NOTIFICATION_QUERY_SIZE}`, + ' "index":', + ` ${outputIndex}`, + ' "ruleId":', + ` ${ruleId}`, + ].join('') + ); + const results = await getSignals({ from: `${fromInMs.valueOf()}`, to: `${toInMs.valueOf()}`, @@ -71,18 +96,56 @@ export const scheduleThrottledNotificationActions = async ({ esClient, }); - const signalsCount = + // This will give us counts up to the max of 10k from tracking total hits. + const signalsCountFromResults = typeof results.hits.total === 'number' ? results.hits.total : results.hits.total.value; - const signals = results.hits.hits.map((hit) => hit._source); - if (results.hits.hits.length !== 0) { + const resultsFlattened = results.hits.hits.map((hit) => { + return { + _id: hit._id, + _index: hit._index, + ...hit._source, + }; + }); + + const deconflicted = deconflictSignalsAndResults({ + logger, + signals, + querySignals: resultsFlattened, + }); + + // Difference of how many deconflicted results we have to subtract from our signals count. + const deconflictedDiff = resultsFlattened.length + signals.length - deconflicted.length; + + // Subtract any deconflicted differences from the total count. + const signalsCount = signalsCountFromResults + signals.length - deconflictedDiff; + logger.debug( + [ + `The notification throttle query result size before deconflicting duplicates is: ${resultsFlattened.length}.`, + ` The notification throttle passed in signals size before deconflicting duplicates is: ${signals.length}.`, + ` The deconflicted size and size of the signals sent into throttle notification is: ${deconflicted.length}.`, + ` The signals count from results size is: ${signalsCountFromResults}.`, + ` The final signals count being sent to the notification is: ${signalsCount}.`, + ].join('') + ); + + if (deconflicted.length !== 0) { scheduleNotificationActions({ alertInstance, signalsCount, - signals, + signals: deconflicted, resultsLink, ruleParams: notificationRuleParams, }); } + } else { + logger.error( + [ + 'The notification throttle "from" and/or "to" range values could not be constructed as valid. Tried to construct the values of', + ` "from": now-${throttle}`, + ` "to": ${startedAt.toISOString()}.`, + ' This will cause a reset of the notification throttle. Expect either missing alert notifications or alert notifications happening earlier than expected.', + ].join('') + ); } }; diff --git a/x-pack/plugins/security_solution/server/lib/detection_engine/notifications/utils.test.ts b/x-pack/plugins/security_solution/server/lib/detection_engine/notifications/utils.test.ts index 5a667616a9a39..2da7a0398bd3f 100644 --- a/x-pack/plugins/security_solution/server/lib/detection_engine/notifications/utils.test.ts +++ b/x-pack/plugins/security_solution/server/lib/detection_engine/notifications/utils.test.ts @@ -5,18 +5,384 @@ * 2.0. */ -import { getNotificationResultsLink } from './utils'; +import { SearchHit } from '@elastic/elasticsearch/api/types'; +import { loggingSystemMock } from 'src/core/server/mocks'; +import { SignalSource } from '../signals/types'; +import { deconflictSignalsAndResults, getNotificationResultsLink } from './utils'; describe('utils', () => { - it('getNotificationResultsLink', () => { - const resultLink = getNotificationResultsLink({ - kibanaSiemAppUrl: 'http://localhost:5601/app/security', - id: 'notification-id', - from: '00000', - to: '1111', - }); - expect(resultLink).toEqual( - `http://localhost:5601/app/security/detections/rules/id/notification-id?timerange=(global:(linkTo:!(timeline),timerange:(from:00000,kind:absolute,to:1111)),timeline:(linkTo:!(global),timerange:(from:00000,kind:absolute,to:1111)))` - ); + let logger = loggingSystemMock.create().get('security_solution'); + + beforeEach(() => { + logger = loggingSystemMock.create().get('security_solution'); + }); + + describe('getNotificationResultsLink', () => { + test('it returns expected link', () => { + const resultLink = getNotificationResultsLink({ + kibanaSiemAppUrl: 'http://localhost:5601/app/security', + id: 'notification-id', + from: '00000', + to: '1111', + }); + expect(resultLink).toEqual( + `http://localhost:5601/app/security/detections/rules/id/notification-id?timerange=(global:(linkTo:!(timeline),timerange:(from:00000,kind:absolute,to:1111)),timeline:(linkTo:!(global),timerange:(from:00000,kind:absolute,to:1111)))` + ); + }); + }); + + describe('deconflictSignalsAndResults', () => { + type FuncReturn = ReturnType; + + test('given no signals and no query results it returns an empty array', () => { + expect( + deconflictSignalsAndResults({ logger, querySignals: [], signals: [] }) + ).toEqual([]); + }); + + test('given an empty signal and a single query result it returns the query result in the array', () => { + const querySignals: Array> = [ + { + _id: 'id-123', + _index: 'index-123', + _source: { + test: '123', + }, + }, + ]; + expect( + deconflictSignalsAndResults({ logger, querySignals, signals: [] }) + ).toEqual(querySignals); + }); + + test('given a single signal and an empty query result it returns the query result in the array', () => { + const signals: Array> = [ + { + _id: 'id-123', + _index: 'index-123', + _source: { + test: '123', + }, + }, + ]; + expect( + deconflictSignalsAndResults({ logger, querySignals: [], signals }) + ).toEqual(signals); + }); + + test('given a signal and a different query result it returns both combined together', () => { + const querySignals: Array> = [ + { + _id: 'id-123', + _index: 'index-123', + _source: { + test: '123', + }, + }, + ]; + const signals: Array> = [ + { + _id: 'id-789', + _index: 'index-456', + _source: { + test: '456', + }, + }, + ]; + expect(deconflictSignalsAndResults({ logger, querySignals, signals })).toEqual([ + ...signals, + ...querySignals, + ]); + }); + + test('given a duplicate in querySignals it returns both combined together without the duplicate', () => { + const querySignals: Array> = [ + { + _id: 'id-123', + _index: 'index-123', // This should only show up once and not be duplicated twice + _source: { + test: '123', + }, + }, + { + _index: 'index-890', + _id: 'id-890', + _source: { + test: '890', + }, + }, + ]; + const signals: Array> = [ + { + _id: 'id-123', // This should only show up once and not be duplicated twice + _index: 'index-123', + _source: { + test: '123', + }, + }, + { + _id: 'id-789', + _index: 'index-456', + _source: { + test: '456', + }, + }, + ]; + expect(deconflictSignalsAndResults({ logger, querySignals, signals })).toEqual([ + { + _id: 'id-123', + _index: 'index-123', + _source: { + test: '123', + }, + }, + { + _id: 'id-789', + _index: 'index-456', + _source: { + test: '456', + }, + }, + { + _id: 'id-890', + _index: 'index-890', + _source: { + test: '890', + }, + }, + ]); + }); + + test('given a duplicate in signals it returns both combined together without the duplicate', () => { + const signals: Array> = [ + { + _id: 'id-123', + _index: 'index-123', // This should only show up once and not be duplicated twice + _source: { + test: '123', + }, + }, + { + _index: 'index-890', + _id: 'id-890', + _source: { + test: '890', + }, + }, + ]; + const querySignals: Array> = [ + { + _id: 'id-123', // This should only show up once and not be duplicated twice + _index: 'index-123', + _source: { + test: '123', + }, + }, + { + _id: 'id-789', + _index: 'index-456', + _source: { + test: '456', + }, + }, + ]; + expect(deconflictSignalsAndResults({ logger, querySignals, signals })).toEqual([ + { + _id: 'id-123', + _index: 'index-123', + _source: { test: '123' }, + }, + { + _id: 'id-890', + _index: 'index-890', + _source: { test: '890' }, + }, + { + _id: 'id-789', + _index: 'index-456', + _source: { test: '456' }, + }, + ]); + }); + + test('does not give a duplicate in signals if they are only different by their index', () => { + const signals: Array> = [ + { + _id: 'id-123', + _index: 'index-123-a', // This is only different by index + _source: { + test: '123', + }, + }, + { + _index: 'index-890', + _id: 'id-890', + _source: { + test: '890', + }, + }, + ]; + const querySignals: Array> = [ + { + _id: 'id-123', // This is only different by index + _index: 'index-123-b', + _source: { + test: '123', + }, + }, + { + _id: 'id-789', + _index: 'index-456', + _source: { + test: '456', + }, + }, + ]; + expect(deconflictSignalsAndResults({ logger, querySignals, signals })).toEqual([ + ...signals, + ...querySignals, + ]); + }); + + test('it logs a debug statement when it sees a duplicate and returns nothing if both are identical', () => { + const querySignals: Array> = [ + { + _id: 'id-123', + _index: 'index-123', + _source: { + test: '123', + }, + }, + ]; + const signals: Array> = [ + { + _id: 'id-123', + _index: 'index-123', + _source: { + test: '456', + }, + }, + ]; + expect(deconflictSignalsAndResults({ logger, querySignals, signals })).toEqual([ + { + _id: 'id-123', + _index: 'index-123', + _source: { + test: '456', + }, + }, + ]); + expect(logger.debug).toHaveBeenCalledWith( + 'Notification throttle removing duplicate signal and query result found of "_id": id-123, "_index": index-123' + ); + }); + + test('it logs an error statement if it sees a signal missing an "_id" for an uncommon reason and returns both documents', () => { + const querySignals: Array> = [ + { + _id: 'id-123', + _index: 'index-123', + _source: { + test: '123', + }, + }, + ]; + const signals: unknown[] = [ + { + _index: 'index-123', + _source: { + test: '456', + }, + }, + ]; + expect(deconflictSignalsAndResults({ logger, querySignals, signals })).toEqual([ + ...signals, + ...querySignals, + ]); + expect(logger.error).toHaveBeenCalledWith( + 'Notification throttle cannot determine if we can de-conflict as either the passed in signal or the results query has a null value for either "_id" or "_index". Expect possible duplications in your alerting actions. Passed in signals "_id": undefined. Passed in signals "_index": index-123. Passed in query "result._id": id-123. Passed in query "result._index": index-123.' + ); + }); + + test('it logs an error statement if it sees a signal missing a "_index" for an uncommon reason and returns both documents', () => { + const querySignals: Array> = [ + { + _id: 'id-123', + _index: 'index-123', + _source: { + test: '123', + }, + }, + ]; + const signals: unknown[] = [ + { + _id: 'id-123', + _source: { + test: '456', + }, + }, + ]; + expect(deconflictSignalsAndResults({ logger, querySignals, signals })).toEqual([ + ...signals, + ...querySignals, + ]); + expect(logger.error).toHaveBeenCalledWith( + 'Notification throttle cannot determine if we can de-conflict as either the passed in signal or the results query has a null value for either "_id" or "_index". Expect possible duplications in your alerting actions. Passed in signals "_id": id-123. Passed in signals "_index": undefined. Passed in query "result._id": id-123. Passed in query "result._index": index-123.' + ); + }); + + test('it logs an error statement if it sees a querySignals missing an "_id" for an uncommon reason and returns both documents', () => { + const querySignals: Array> = [ + { + _index: 'index-123', + _source: { + test: '123', + }, + }, + ] as unknown[] as Array>; + const signals: unknown[] = [ + { + _id: 'id-123', + _index: 'index-123', + _source: { + test: '456', + }, + }, + ]; + expect(deconflictSignalsAndResults({ logger, querySignals, signals })).toEqual([ + ...signals, + ...querySignals, + ]); + expect(logger.error).toHaveBeenCalledWith( + 'Notification throttle cannot determine if we can de-conflict as either the passed in signal or the results query has a null value for either "_id" or "_index". Expect possible duplications in your alerting actions. Passed in signals "_id": id-123. Passed in signals "_index": index-123. Passed in query "result._id": undefined. Passed in query "result._index": index-123.' + ); + }); + + test('it logs an error statement if it sees a querySignals missing a "_index" for an uncommon reason and returns both documents', () => { + const querySignals: Array> = [ + { + _id: 'id-123', + _source: { + test: '123', + }, + }, + ] as unknown[] as Array>; + const signals: unknown[] = [ + { + _id: 'id-123', + _index: 'index-123', + _source: { + test: '456', + }, + }, + ]; + expect(deconflictSignalsAndResults({ logger, querySignals, signals })).toEqual([ + ...signals, + ...querySignals, + ]); + expect(logger.error).toHaveBeenCalledWith( + 'Notification throttle cannot determine if we can de-conflict as either the passed in signal or the results query has a null value for either "_id" or "_index". Expect possible duplications in your alerting actions. Passed in signals "_id": id-123. Passed in signals "_index": index-123. Passed in query "result._id": id-123. Passed in query "result._index": undefined.' + ); + }); }); }); diff --git a/x-pack/plugins/security_solution/server/lib/detection_engine/notifications/utils.ts b/x-pack/plugins/security_solution/server/lib/detection_engine/notifications/utils.ts index 4c4bac7da6a62..c8fc6febe4d0f 100644 --- a/x-pack/plugins/security_solution/server/lib/detection_engine/notifications/utils.ts +++ b/x-pack/plugins/security_solution/server/lib/detection_engine/notifications/utils.ts @@ -5,7 +5,9 @@ * 2.0. */ +import { Logger } from 'src/core/server'; import { APP_PATH } from '../../../../common/constants'; +import { SignalSearchResponse } from '../signals/types'; export const getNotificationResultsLink = ({ kibanaSiemAppUrl = APP_PATH, @@ -22,3 +24,54 @@ export const getNotificationResultsLink = ({ return `${kibanaSiemAppUrl}/detections/rules/id/${id}?timerange=(global:(linkTo:!(timeline),timerange:(from:${from},kind:absolute,to:${to})),timeline:(linkTo:!(global),timerange:(from:${from},kind:absolute,to:${to})))`; }; + +interface DeconflictOptions { + signals: unknown[]; + querySignals: SignalSearchResponse['hits']['hits']; + logger: Logger; +} + +/** + * Given a signals array of unknown that at least has a '_id' and '_index' this will deconflict it with a results. + * @param signals The signals array to deconflict with results + * @param results The results to deconflict with the signals + * @param logger The logger to log results + */ +export const deconflictSignalsAndResults = ({ + signals, + querySignals, + logger, +}: DeconflictOptions): unknown[] => { + const querySignalsFiltered = querySignals.filter((result) => { + return !signals.find((signal) => { + const { _id, _index } = signal as { _id?: string; _index?: string }; + if (_id == null || _index == null || result._id == null || result._index == null) { + logger.error( + [ + 'Notification throttle cannot determine if we can de-conflict as either the passed in signal or the results query has a null value for either "_id" or "_index".', + ' Expect possible duplications in your alerting actions.', + ` Passed in signals "_id": ${_id}.`, + ` Passed in signals "_index": ${_index}.`, + ` Passed in query "result._id": ${result._id}.`, + ` Passed in query "result._index": ${result._index}.`, + ].join('') + ); + return false; + } else { + if (result._id === _id && result._index === _index) { + logger.debug( + [ + 'Notification throttle removing duplicate signal and query result found of', + ` "_id": ${_id},`, + ` "_index": ${_index}`, + ].join('') + ); + return true; + } else { + return false; + } + } + }); + }); + return [...signals, ...querySignalsFiltered]; +}; diff --git a/x-pack/plugins/security_solution/server/lib/detection_engine/rule_types/create_security_rule_type_wrapper.ts b/x-pack/plugins/security_solution/server/lib/detection_engine/rule_types/create_security_rule_type_wrapper.ts index 77981d92b2ba7..0ad416e86e31a 100644 --- a/x-pack/plugins/security_solution/server/lib/detection_engine/rule_types/create_security_rule_type_wrapper.ts +++ b/x-pack/plugins/security_solution/server/lib/detection_engine/rule_types/create_security_rule_type_wrapper.ts @@ -111,6 +111,12 @@ export const createSecurityRuleTypeWrapper: CreateSecurityRuleTypeWrapper = let result = createResultObject(state); + const notificationRuleParams: NotificationRuleTypeParams = { + ...params, + name: name as string, + id: ruleSO.id as string, + } as unknown as NotificationRuleTypeParams; + // check if rule has permissions to access given index pattern // move this collection of lines into a function in utils // so that we can use it in create rules route, bulk, etc. @@ -296,12 +302,6 @@ export const createSecurityRuleTypeWrapper: CreateSecurityRuleTypeWrapper = const createdSignalsCount = result.createdSignals.length; if (actions.length) { - const notificationRuleParams: NotificationRuleTypeParams = { - ...params, - name: name as string, - id: ruleSO.id as string, - } as unknown as NotificationRuleTypeParams; - const fromInMs = parseScheduleDates(`now-${interval}`)?.format('x'); const toInMs = parseScheduleDates('now')?.format('x'); const resultsLink = getNotificationResultsLink({ @@ -328,6 +328,8 @@ export const createSecurityRuleTypeWrapper: CreateSecurityRuleTypeWrapper = ruleId, esClient: services.scopedClusterClient.asCurrentUser, notificationRuleParams, + signals: result.createdSignals, + logger, }); } else if (createdSignalsCount) { const alertInstance = services.alertInstanceFactory(alertId); @@ -372,6 +374,21 @@ export const createSecurityRuleTypeWrapper: CreateSecurityRuleTypeWrapper = ) ); } else { + // NOTE: Since this is throttled we have to call it even on an error condition, otherwise it will "reset" the throttle and fire early + await scheduleThrottledNotificationActions({ + alertInstance: services.alertInstanceFactory(alertId), + throttle: ruleSO.attributes.throttle, + startedAt, + id: ruleSO.id, + kibanaSiemAppUrl: (meta as { kibana_siem_app_url?: string } | undefined) + ?.kibana_siem_app_url, + outputIndex: ruleDataClient.indexName, + ruleId, + esClient: services.scopedClusterClient.asCurrentUser, + notificationRuleParams, + signals: result.createdSignals, + logger, + }); const errorMessage = buildRuleMessage( 'Bulk Indexing of signals failed:', truncateMessageList(result.errors).join() @@ -389,6 +406,22 @@ export const createSecurityRuleTypeWrapper: CreateSecurityRuleTypeWrapper = }); } } catch (error) { + // NOTE: Since this is throttled we have to call it even on an error condition, otherwise it will "reset" the throttle and fire early + await scheduleThrottledNotificationActions({ + alertInstance: services.alertInstanceFactory(alertId), + throttle: ruleSO.attributes.throttle, + startedAt, + id: ruleSO.id, + kibanaSiemAppUrl: (meta as { kibana_siem_app_url?: string } | undefined) + ?.kibana_siem_app_url, + outputIndex: ruleDataClient.indexName, + ruleId, + esClient: services.scopedClusterClient.asCurrentUser, + notificationRuleParams, + signals: result.createdSignals, + logger, + }); + const errorMessage = error.message ?? '(no error message given)'; const message = buildRuleMessage( 'An error occurred during rule execution:', diff --git a/x-pack/plugins/security_solution/server/lib/detection_engine/signals/signal_rule_alert_type.test.ts b/x-pack/plugins/security_solution/server/lib/detection_engine/signals/signal_rule_alert_type.test.ts index c2923b566175e..88b276358a705 100644 --- a/x-pack/plugins/security_solution/server/lib/detection_engine/signals/signal_rule_alert_type.test.ts +++ b/x-pack/plugins/security_solution/server/lib/detection_engine/signals/signal_rule_alert_type.test.ts @@ -35,6 +35,7 @@ import { allowedExperimentalValues } from '../../../../common/experimental_featu import { scheduleNotificationActions } from '../notifications/schedule_notification_actions'; import { ruleExecutionLogClientMock } from '../rule_execution_log/__mocks__/rule_execution_log_client'; import { RuleExecutionStatus } from '../../../../common/detection_engine/schemas/common/schemas'; +import { scheduleThrottledNotificationActions } from '../notifications/schedule_throttle_notification_actions'; import { eventLogServiceMock } from '../../../../../event_log/server/mocks'; import { createMockConfig } from '../routes/__mocks__'; @@ -58,7 +59,7 @@ jest.mock('@kbn/securitysolution-io-ts-utils', () => { parseScheduleDates: jest.fn(), }; }); - +jest.mock('../notifications/schedule_throttle_notification_actions'); const mockRuleExecutionLogClient = ruleExecutionLogClientMock.create(); jest.mock('../rule_execution_log/rule_execution_log_client', () => ({ @@ -200,6 +201,7 @@ describe('signal_rule_alert_type', () => { }); mockRuleExecutionLogClient.logStatusChange.mockClear(); + (scheduleThrottledNotificationActions as jest.Mock).mockClear(); }); describe('executor', () => { @@ -520,5 +522,28 @@ describe('signal_rule_alert_type', () => { }) ); }); + + it('should call scheduleThrottledNotificationActions if result is false to prevent the throttle from being reset', async () => { + const result: SearchAfterAndBulkCreateReturnType = { + success: false, + warning: false, + searchAfterTimes: [], + bulkCreateTimes: [], + lastLookBackDate: null, + createdSignalsCount: 0, + createdSignals: [], + warningMessages: [], + errors: ['Error that bubbled up.'], + }; + (queryExecutor as jest.Mock).mockResolvedValue(result); + await alert.executor(payload); + expect(scheduleThrottledNotificationActions).toHaveBeenCalledTimes(1); + }); + + it('should call scheduleThrottledNotificationActions if an error was thrown to prevent the throttle from being reset', async () => { + (queryExecutor as jest.Mock).mockRejectedValue({}); + await alert.executor(payload); + expect(scheduleThrottledNotificationActions).toHaveBeenCalledTimes(1); + }); }); }); diff --git a/x-pack/plugins/security_solution/server/lib/detection_engine/signals/signal_rule_alert_type.ts b/x-pack/plugins/security_solution/server/lib/detection_engine/signals/signal_rule_alert_type.ts index 2094264cbf15f..4e98bee83aeb5 100644 --- a/x-pack/plugins/security_solution/server/lib/detection_engine/signals/signal_rule_alert_type.ts +++ b/x-pack/plugins/security_solution/server/lib/detection_engine/signals/signal_rule_alert_type.ts @@ -172,6 +172,12 @@ export const signalRulesAlertType = ({ newStatus: RuleExecutionStatus['going to run'], }); + const notificationRuleParams: NotificationRuleTypeParams = { + ...params, + name, + id: savedObject.id, + }; + // check if rule has permissions to access given index pattern // move this collection of lines into a function in utils // so that we can use it in create rules route, bulk, etc. @@ -396,12 +402,6 @@ export const signalRulesAlertType = ({ if (result.success) { if (actions.length) { - const notificationRuleParams: NotificationRuleTypeParams = { - ...params, - name, - id: savedObject.id, - }; - const fromInMs = parseScheduleDates(`now-${interval}`)?.format('x'); const toInMs = parseScheduleDates('now')?.format('x'); const resultsLink = getNotificationResultsLink({ @@ -426,8 +426,10 @@ export const signalRulesAlertType = ({ ?.kibana_siem_app_url, outputIndex, ruleId, + signals: result.createdSignals, esClient: services.scopedClusterClient.asCurrentUser, notificationRuleParams, + logger, }); } else if (result.createdSignalsCount) { const alertInstance = services.alertInstanceFactory(alertId); @@ -471,6 +473,22 @@ export const signalRulesAlertType = ({ ) ); } else { + // NOTE: Since this is throttled we have to call it even on an error condition, otherwise it will "reset" the throttle and fire early + await scheduleThrottledNotificationActions({ + alertInstance: services.alertInstanceFactory(alertId), + throttle: savedObject.attributes.throttle, + startedAt, + id: savedObject.id, + kibanaSiemAppUrl: (meta as { kibana_siem_app_url?: string } | undefined) + ?.kibana_siem_app_url, + outputIndex, + ruleId, + signals: result.createdSignals, + esClient: services.scopedClusterClient.asCurrentUser, + notificationRuleParams, + logger, + }); + const errorMessage = buildRuleMessage( 'Bulk Indexing of signals failed:', truncateMessageList(result.errors).join() @@ -488,6 +506,21 @@ export const signalRulesAlertType = ({ }); } } catch (error) { + // NOTE: Since this is throttled we have to call it even on an error condition, otherwise it will "reset" the throttle and fire early + await scheduleThrottledNotificationActions({ + alertInstance: services.alertInstanceFactory(alertId), + throttle: savedObject.attributes.throttle, + startedAt, + id: savedObject.id, + kibanaSiemAppUrl: (meta as { kibana_siem_app_url?: string } | undefined) + ?.kibana_siem_app_url, + outputIndex, + ruleId, + signals: result.createdSignals, + esClient: services.scopedClusterClient.asCurrentUser, + notificationRuleParams, + logger, + }); const errorMessage = error.message ?? '(no error message given)'; const message = buildRuleMessage( 'An error occurred during rule execution:',