From 5cad3fa8a3d10e25ec7879ed1cb1313515d46442 Mon Sep 17 00:00:00 2001 From: "Devin W. Hurley" Date: Tue, 14 Jul 2020 20:26:24 -0400 Subject: [PATCH] [7.x] [Security Solution] [DETECTIONS] Set rule status to failure only on large gaps (#71549) (#71782) Only display rule error status if a large gap is present --- .../signals/signal_rule_alert_type.test.ts | 36 ++- .../signals/signal_rule_alert_type.ts | 36 ++- .../lib/detection_engine/signals/types.ts | 5 + .../detection_engine/signals/utils.test.ts | 47 ++++ .../lib/detection_engine/signals/utils.ts | 218 ++++++++++++------ 5 files changed, 258 insertions(+), 84 deletions(-) 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 5832b4075a40b..b0c855afa8be9 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 @@ -10,7 +10,13 @@ import { getResult, getMlResult } from '../routes/__mocks__/request_responses'; import { signalRulesAlertType } from './signal_rule_alert_type'; import { alertsMock, AlertServicesMock } from '../../../../../alerts/server/mocks'; import { ruleStatusServiceFactory } from './rule_status_service'; -import { getGapBetweenRuns, getListsClient, getExceptions, sortExceptionItems } from './utils'; +import { + getGapBetweenRuns, + getGapMaxCatchupRatio, + getListsClient, + getExceptions, + sortExceptionItems, +} from './utils'; import { RuleExecutorOptions } from './types'; import { searchAfterAndBulkCreate } from './search_after_bulk_create'; import { scheduleNotificationActions } from '../notifications/schedule_notification_actions'; @@ -97,6 +103,7 @@ describe('rules_notification_alert_type', () => { exceptionsWithValueLists: [], }); (searchAfterAndBulkCreate as jest.Mock).mockClear(); + (getGapMaxCatchupRatio as jest.Mock).mockClear(); (searchAfterAndBulkCreate as jest.Mock).mockResolvedValue({ success: true, searchAfterTimes: [], @@ -126,22 +133,39 @@ describe('rules_notification_alert_type', () => { }); describe('executor', () => { - it('should warn about the gap between runs', async () => { - (getGapBetweenRuns as jest.Mock).mockReturnValue(moment.duration(1000)); + it('should warn about the gap between runs if gap is very large', async () => { + (getGapBetweenRuns as jest.Mock).mockReturnValue(moment.duration(100, 'm')); + (getGapMaxCatchupRatio as jest.Mock).mockReturnValue({ + maxCatchup: 4, + ratio: 20, + gapDiffInUnits: 95, + }); await alert.executor(payload); expect(logger.warn).toHaveBeenCalled(); expect(logger.warn.mock.calls[0][0]).toContain( - 'a few seconds (1000ms) has passed since last rule execution, and signals may have been missed.' + '2 hours (6000000ms) has passed since last rule execution, and signals may have been missed.' ); expect(ruleStatusService.error).toHaveBeenCalled(); expect(ruleStatusService.error.mock.calls[0][0]).toContain( - 'a few seconds (1000ms) has passed since last rule execution, and signals may have been missed.' + '2 hours (6000000ms) has passed since last rule execution, and signals may have been missed.' ); expect(ruleStatusService.error.mock.calls[0][1]).toEqual({ - gap: 'a few seconds', + gap: '2 hours', }); }); + it('should NOT warn about the gap between runs if gap small', async () => { + (getGapBetweenRuns as jest.Mock).mockReturnValue(moment.duration(1, 'm')); + (getGapMaxCatchupRatio as jest.Mock).mockReturnValue({ + maxCatchup: 1, + ratio: 1, + gapDiffInUnits: 1, + }); + await alert.executor(payload); + expect(logger.warn).toHaveBeenCalledTimes(0); + expect(ruleStatusService.error).toHaveBeenCalledTimes(0); + }); + it("should set refresh to 'wait_for' when actions are present", async () => { const ruleAlert = getResult(); ruleAlert.actions = [ 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 49efc30b9704d..0e859ecef31c6 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 @@ -22,7 +22,14 @@ import { } from './search_after_bulk_create'; import { getFilter } from './get_filter'; import { SignalRuleAlertTypeDefinition, RuleAlertAttributes } from './types'; -import { getGapBetweenRuns, parseScheduleDates, getListsClient, getExceptions } from './utils'; +import { + getGapBetweenRuns, + parseScheduleDates, + getListsClient, + getExceptions, + getGapMaxCatchupRatio, + MAX_RULE_GAP_RATIO, +} from './utils'; import { signalParamsSchema } from './signal_params_schema'; import { siemRuleActionGroups } from './siem_rule_action_groups'; import { findMlSignals } from './find_ml_signals'; @@ -130,15 +137,26 @@ export const signalRulesAlertType = ({ const gap = getGapBetweenRuns({ previousStartedAt, interval, from, to }); if (gap != null && gap.asMilliseconds() > 0) { - const gapString = gap.humanize(); - const gapMessage = buildRuleMessage( - `${gapString} (${gap.asMilliseconds()}ms) has passed since last rule execution, and signals may have been missed.`, - 'Consider increasing your look behind time or adding more Kibana instances.' - ); - logger.warn(gapMessage); + const fromUnit = from[from.length - 1]; + const { ratio } = getGapMaxCatchupRatio({ + logger, + buildRuleMessage, + previousStartedAt, + ruleParamsFrom: from, + interval, + unit: fromUnit, + }); + if (ratio && ratio >= MAX_RULE_GAP_RATIO) { + const gapString = gap.humanize(); + const gapMessage = buildRuleMessage( + `${gapString} (${gap.asMilliseconds()}ms) has passed since last rule execution, and signals may have been missed.`, + 'Consider increasing your look behind time or adding more Kibana instances.' + ); + logger.warn(gapMessage); - hasError = true; - await ruleStatusService.error(gapMessage, { gap: gapString }); + hasError = true; + await ruleStatusService.error(gapMessage, { gap: gapString }); + } } try { const { listClient, exceptionsClient } = await getListsClient({ diff --git a/x-pack/plugins/security_solution/server/lib/detection_engine/signals/types.ts b/x-pack/plugins/security_solution/server/lib/detection_engine/signals/types.ts index 5d6bafc5a6d09..bfc72a169566e 100644 --- a/x-pack/plugins/security_solution/server/lib/detection_engine/signals/types.ts +++ b/x-pack/plugins/security_solution/server/lib/detection_engine/signals/types.ts @@ -11,6 +11,11 @@ import { RuleAlertAction } from '../../../../common/detection_engine/types'; import { RuleTypeParams } from '../types'; import { SearchResponse } from '../../types'; +// used for gap detection code +export type unitType = 's' | 'm' | 'h'; +export const isValidUnit = (unitParam: string): unitParam is unitType => + ['s', 'm', 'h'].includes(unitParam); + export interface SignalsParams { signalIds: string[] | undefined | null; query: object | undefined | null; diff --git a/x-pack/plugins/security_solution/server/lib/detection_engine/signals/utils.test.ts b/x-pack/plugins/security_solution/server/lib/detection_engine/signals/utils.test.ts index 0cc3ca092a4dc..a6130a20f9c52 100644 --- a/x-pack/plugins/security_solution/server/lib/detection_engine/signals/utils.test.ts +++ b/x-pack/plugins/security_solution/server/lib/detection_engine/signals/utils.test.ts @@ -21,6 +21,7 @@ import { parseScheduleDates, getDriftTolerance, getGapBetweenRuns, + getGapMaxCatchupRatio, errorAggregator, getListsClient, hasLargeValueList, @@ -716,6 +717,52 @@ describe('utils', () => { }); }); + describe('getMaxCatchupRatio', () => { + test('should return null if rule has never run before', () => { + const { maxCatchup, ratio, gapDiffInUnits } = getGapMaxCatchupRatio({ + logger: mockLogger, + previousStartedAt: null, + interval: '30s', + ruleParamsFrom: 'now-30s', + buildRuleMessage, + unit: 's', + }); + expect(maxCatchup).toBeNull(); + expect(ratio).toBeNull(); + expect(gapDiffInUnits).toBeNull(); + }); + + test('should should have non-null values when gap is present', () => { + const { maxCatchup, ratio, gapDiffInUnits } = getGapMaxCatchupRatio({ + logger: mockLogger, + previousStartedAt: moment().subtract(65, 's').toDate(), + interval: '50s', + ruleParamsFrom: 'now-55s', + buildRuleMessage, + unit: 's', + }); + expect(maxCatchup).toEqual(0.2); + expect(ratio).toEqual(0.2); + expect(gapDiffInUnits).toEqual(10); + }); + + // when a rule runs sooner than expected we don't + // consider that a gap as that is a very rare circumstance + test('should return null when given a negative gap (rule ran sooner than expected)', () => { + const { maxCatchup, ratio, gapDiffInUnits } = getGapMaxCatchupRatio({ + logger: mockLogger, + previousStartedAt: moment().subtract(-15, 's').toDate(), + interval: '10s', + ruleParamsFrom: 'now-13s', + buildRuleMessage, + unit: 's', + }); + expect(maxCatchup).toBeNull(); + expect(ratio).toBeNull(); + expect(gapDiffInUnits).toBeNull(); + }); + }); + describe('#getExceptions', () => { test('it successfully returns array of exception list items', async () => { const client = listMock.getExceptionListClient(); diff --git a/x-pack/plugins/security_solution/server/lib/detection_engine/signals/utils.ts b/x-pack/plugins/security_solution/server/lib/detection_engine/signals/utils.ts index 0016765b9dbe9..0b95ff6786b01 100644 --- a/x-pack/plugins/security_solution/server/lib/detection_engine/signals/utils.ts +++ b/x-pack/plugins/security_solution/server/lib/detection_engine/signals/utils.ts @@ -12,7 +12,7 @@ import { AlertServices, parseDuration } from '../../../../../alerts/server'; import { ExceptionListClient, ListClient, ListPluginSetup } from '../../../../../lists/server'; import { EntriesArray, ExceptionListItemSchema } from '../../../../../lists/common/schemas'; import { ListArrayOrUndefined } from '../../../../common/detection_engine/schemas/types/lists'; -import { BulkResponse, BulkResponseErrorAggregation } from './types'; +import { BulkResponse, BulkResponseErrorAggregation, isValidUnit } from './types'; import { BuildRuleMessage } from './rule_messages'; interface SortExceptionsReturn { @@ -20,6 +20,101 @@ interface SortExceptionsReturn { exceptionsWithoutValueLists: ExceptionListItemSchema[]; } +export const MAX_RULE_GAP_RATIO = 4; + +export const shorthandMap = { + s: { + momentString: 'seconds', + asFn: (duration: moment.Duration) => duration.asSeconds(), + }, + m: { + momentString: 'minutes', + asFn: (duration: moment.Duration) => duration.asMinutes(), + }, + h: { + momentString: 'hours', + asFn: (duration: moment.Duration) => duration.asHours(), + }, +}; + +export const getGapMaxCatchupRatio = ({ + logger, + previousStartedAt, + unit, + buildRuleMessage, + ruleParamsFrom, + interval, +}: { + logger: Logger; + ruleParamsFrom: string; + previousStartedAt: Date | null | undefined; + interval: string; + buildRuleMessage: BuildRuleMessage; + unit: string; +}): { + maxCatchup: number | null; + ratio: number | null; + gapDiffInUnits: number | null; +} => { + if (previousStartedAt == null) { + return { + maxCatchup: null, + ratio: null, + gapDiffInUnits: null, + }; + } + if (!isValidUnit(unit)) { + logger.error(buildRuleMessage(`unit: ${unit} failed isValidUnit check`)); + return { + maxCatchup: null, + ratio: null, + gapDiffInUnits: null, + }; + } + /* + we need the total duration from now until the last time the rule ran. + the next few lines can be summed up as calculating + "how many second | minutes | hours have passed since the last time this ran?" + */ + const nowToGapDiff = moment.duration(moment().diff(previousStartedAt)); + // rule ran early, no gap + if (shorthandMap[unit].asFn(nowToGapDiff) < 0) { + // rule ran early, no gap + return { + maxCatchup: null, + ratio: null, + gapDiffInUnits: null, + }; + } + const calculatedFrom = `now-${ + parseInt(shorthandMap[unit].asFn(nowToGapDiff).toString(), 10) + unit + }`; + logger.debug(buildRuleMessage(`calculatedFrom: ${calculatedFrom}`)); + + const intervalMoment = moment.duration(parseInt(interval, 10), unit); + logger.debug(buildRuleMessage(`intervalMoment: ${shorthandMap[unit].asFn(intervalMoment)}`)); + const calculatedFromAsMoment = dateMath.parse(calculatedFrom); + const dateMathRuleParamsFrom = dateMath.parse(ruleParamsFrom); + if (dateMathRuleParamsFrom != null && intervalMoment != null) { + const momentUnit = shorthandMap[unit].momentString as moment.DurationInputArg2; + const gapDiffInUnits = dateMathRuleParamsFrom.diff(calculatedFromAsMoment, momentUnit); + + const ratio = gapDiffInUnits / shorthandMap[unit].asFn(intervalMoment); + + // maxCatchup is to ensure we are not trying to catch up too far back. + // This allows for a maximum of 4 consecutive rule execution misses + // to be included in the number of signals generated. + const maxCatchup = ratio < MAX_RULE_GAP_RATIO ? ratio : MAX_RULE_GAP_RATIO; + return { maxCatchup, ratio, gapDiffInUnits }; + } + logger.error(buildRuleMessage('failed to parse calculatedFrom and intervalMoment')); + return { + maxCatchup: null, + ratio: null, + gapDiffInUnits: null, + }; +}; + export const getListsClient = async ({ lists, spaceId, @@ -294,8 +389,6 @@ export const getSignalTimeTuples = ({ from: moment.Moment | undefined; maxSignals: number; }> => { - type unitType = 's' | 'm' | 'h'; - const isValidUnit = (unit: string): unit is unitType => ['s', 'm', 'h'].includes(unit); let totalToFromTuples: Array<{ to: moment.Moment | undefined; from: moment.Moment | undefined; @@ -305,20 +398,6 @@ export const getSignalTimeTuples = ({ const fromUnit = ruleParamsFrom[ruleParamsFrom.length - 1]; if (isValidUnit(fromUnit)) { const unit = fromUnit; // only seconds (s), minutes (m) or hours (h) - const shorthandMap = { - s: { - momentString: 'seconds', - asFn: (duration: moment.Duration) => duration.asSeconds(), - }, - m: { - momentString: 'minutes', - asFn: (duration: moment.Duration) => duration.asMinutes(), - }, - h: { - momentString: 'hours', - asFn: (duration: moment.Duration) => duration.asHours(), - }, - }; /* we need the total duration from now until the last time the rule ran. @@ -333,62 +412,63 @@ export const getSignalTimeTuples = ({ const intervalMoment = moment.duration(parseInt(interval, 10), unit); logger.debug(buildRuleMessage(`intervalMoment: ${shorthandMap[unit].asFn(intervalMoment)}`)); - const calculatedFromAsMoment = dateMath.parse(calculatedFrom); - if (calculatedFromAsMoment != null && intervalMoment != null) { - const dateMathRuleParamsFrom = dateMath.parse(ruleParamsFrom); - const momentUnit = shorthandMap[unit].momentString as moment.DurationInputArg2; - const gapDiffInUnits = calculatedFromAsMoment.diff(dateMathRuleParamsFrom, momentUnit); - - const ratio = Math.abs(gapDiffInUnits / shorthandMap[unit].asFn(intervalMoment)); - - // maxCatchup is to ensure we are not trying to catch up too far back. - // This allows for a maximum of 4 consecutive rule execution misses - // to be included in the number of signals generated. - const maxCatchup = ratio < 4 ? ratio : 4; - logger.debug(buildRuleMessage(`maxCatchup: ${ratio}`)); + const momentUnit = shorthandMap[unit].momentString as moment.DurationInputArg2; + // maxCatchup is to ensure we are not trying to catch up too far back. + // This allows for a maximum of 4 consecutive rule execution misses + // to be included in the number of signals generated. + const { maxCatchup, ratio, gapDiffInUnits } = getGapMaxCatchupRatio({ + logger, + buildRuleMessage, + previousStartedAt, + unit, + ruleParamsFrom, + interval, + }); + logger.debug(buildRuleMessage(`maxCatchup: ${maxCatchup}, ratio: ${ratio}`)); + if (maxCatchup == null || ratio == null || gapDiffInUnits == null) { + throw new Error( + buildRuleMessage('failed to calculate maxCatchup, ratio, or gapDiffInUnits') + ); + } + let tempTo = dateMath.parse(ruleParamsFrom); + if (tempTo == null) { + // return an error + throw new Error(buildRuleMessage('dateMath parse failed')); + } - let tempTo = dateMath.parse(ruleParamsFrom); - if (tempTo == null) { - // return an error - throw new Error('dateMath parse failed'); + let beforeMutatedFrom: moment.Moment | undefined; + while (totalToFromTuples.length < maxCatchup) { + // if maxCatchup is less than 1, we calculate the 'from' differently + // and maxSignals becomes some less amount of maxSignals + // in order to maintain maxSignals per full rule interval. + if (maxCatchup > 0 && maxCatchup < 1) { + totalToFromTuples.push({ + to: tempTo.clone(), + from: tempTo.clone().subtract(gapDiffInUnits, momentUnit), + maxSignals: ruleParamsMaxSignals * maxCatchup, + }); + break; } + const beforeMutatedTo = tempTo.clone(); - let beforeMutatedFrom: moment.Moment | undefined; - while (totalToFromTuples.length < maxCatchup) { - // if maxCatchup is less than 1, we calculate the 'from' differently - // and maxSignals becomes some less amount of maxSignals - // in order to maintain maxSignals per full rule interval. - if (maxCatchup > 0 && maxCatchup < 1) { - totalToFromTuples.push({ - to: tempTo.clone(), - from: tempTo.clone().subtract(Math.abs(gapDiffInUnits), momentUnit), - maxSignals: ruleParamsMaxSignals * maxCatchup, - }); - break; - } - const beforeMutatedTo = tempTo.clone(); - - // moment.subtract mutates the moment so we need to clone again.. - beforeMutatedFrom = tempTo.clone().subtract(intervalMoment, momentUnit); - const tuple = { - to: beforeMutatedTo, - from: beforeMutatedFrom, - maxSignals: ruleParamsMaxSignals, - }; - totalToFromTuples = [...totalToFromTuples, tuple]; - tempTo = beforeMutatedFrom; - } - totalToFromTuples = [ - { - to: dateMath.parse(ruleParamsTo), - from: dateMath.parse(ruleParamsFrom), - maxSignals: ruleParamsMaxSignals, - }, - ...totalToFromTuples, - ]; - } else { - logger.debug(buildRuleMessage('calculatedFromMoment was null or intervalMoment was null')); + // moment.subtract mutates the moment so we need to clone again.. + beforeMutatedFrom = tempTo.clone().subtract(intervalMoment, momentUnit); + const tuple = { + to: beforeMutatedTo, + from: beforeMutatedFrom, + maxSignals: ruleParamsMaxSignals, + }; + totalToFromTuples = [...totalToFromTuples, tuple]; + tempTo = beforeMutatedFrom; } + totalToFromTuples = [ + { + to: dateMath.parse(ruleParamsTo), + from: dateMath.parse(ruleParamsFrom), + maxSignals: ruleParamsMaxSignals, + }, + ...totalToFromTuples, + ]; } } else { totalToFromTuples = [