Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[RAM][Security Solution][Alerts] Support the ability to trigger a rule action per alert generated (#153611) #155384

Merged
merged 34 commits into from
Apr 26, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
34 commits
Select commit Hold shift + click to select a range
77a53e0
[RAM][Security Solution][Alerts] Support the ability to trigger a rul…
e40pud Apr 20, 2023
7d7f8b7
Fix types
e40pud Apr 20, 2023
afc00be
Fix types
e40pud Apr 20, 2023
f305058
Fix tests
e40pud Apr 20, 2023
5d3de3e
Merge branch 'main' into security/feature/per-alert-actions
kibanamachine Apr 21, 2023
04f5167
Merge branch 'main' into security/feature/per-alert-actions
kibanamachine Apr 21, 2023
a1441e3
Merge branch 'main' into security/feature/per-alert-actions
kibanamachine Apr 21, 2023
5d66c2a
Merge branch 'main' into security/feature/per-alert-actions
kibanamachine Apr 22, 2023
bb45080
Merge branch 'main' into security/feature/per-alert-actions
e40pud Apr 23, 2023
d65d992
Update default “for each alert” body message
e40pud Apr 23, 2023
e243d00
Fix linting
e40pud Apr 23, 2023
2be474e
Migrate 8.7 SIEM rules
e40pud Apr 24, 2023
7f2bbf3
Using UUID for alert summary filter
ymao1 Apr 24, 2023
6c94f85
UUID or ID
ymao1 Apr 24, 2023
15fcadf
migrate security solution frequency
XavierM Apr 24, 2023
5182466
Merge branch 'security/feature/per-alert-actions' of github.com:e40pu…
XavierM Apr 24, 2023
aa6b8a1
uncomment tests
XavierM Apr 24, 2023
806d0f8
Review feedback: add “results_link” and “alerts” to per-alert action
e40pud Apr 25, 2023
32e4e79
Review feedback
e40pud Apr 25, 2023
8ce3ac4
Fix typos
e40pud Apr 25, 2023
ec1279c
Review feedback
e40pud Apr 25, 2023
e439fe1
Fixing jest test
ymao1 Apr 25, 2023
27faf3f
Fix non existing schedul.interval error
e40pud Apr 25, 2023
c4385ae
Set `results_link` to URL which opens alerts table in per-alert actio…
e40pud Apr 25, 2023
629acfd
Merge branch 'main' into security/feature/per-alert-actions
kibanamachine Apr 25, 2023
51f3e3c
Pass `params` to build results link
e40pud Apr 25, 2023
08af15d
Merge branch 'main' into security/feature/per-alert-actions
kibanamachine Apr 25, 2023
b2832bd
Update x-pack/test/alerting_api_integration/spaces_only/tests/alertin…
e40pud Apr 25, 2023
62a6b77
Call formatAlert before passing alert to “for each alert” context
e40pud Apr 25, 2023
30b1c72
Merge branch 'main' into security/feature/per-alert-actions
kibanamachine Apr 25, 2023
6948d0c
Revert "Call formatAlert before passing alert to “for each alert” con…
e40pud Apr 25, 2023
8567d91
Merge branch 'main' into security/feature/per-alert-actions
kibanamachine Apr 26, 2023
696a454
Merge branch 'main' into security/feature/per-alert-actions
kibanamachine Apr 26, 2023
4507d23
Merge branch 'main' into security/feature/per-alert-actions
kibanamachine Apr 26, 2023
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
20 changes: 13 additions & 7 deletions x-pack/plugins/alerting/server/alert/alert.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -661,27 +661,33 @@ describe('resetPendingRecoveredCount', () => {

describe('isFilteredOut', () => {
const summarizedAlerts = {
all: { count: 1, data: [{ kibana: { alert: { instance: { id: 1 } } } }] },
all: { count: 1, data: [{ kibana: { alert: { uuid: '1' } } }] },
new: { count: 0, data: [] },
ongoing: { count: 0, data: [] },
recovered: { count: 0, data: [] },
};

test('returns false if summarizedAlerts is null', () => {
const alert = new Alert<AlertInstanceState, AlertInstanceContext, DefaultActionGroupId>('1', {
meta: { pendingRecoveredCount: 3 },
meta: { pendingRecoveredCount: 3, uuid: '1' },
});
expect(alert.isFilteredOut(null)).toBe(false);
});
test('returns false if the alert is in summarizedAlerts', () => {
test('returns false if the alert with same ID is in summarizedAlerts', () => {
const alert = new Alert<AlertInstanceState, AlertInstanceContext, DefaultActionGroupId>('1', {
meta: { pendingRecoveredCount: 3 },
meta: { pendingRecoveredCount: 3, uuid: 'no' },
});
expect(alert.isFilteredOut(null)).toBe(false);
expect(alert.isFilteredOut(summarizedAlerts)).toBe(false);
});
test('returns true if the alert is not in summarizedAlerts', () => {
test('returns false if the alert with same UUID is in summarizedAlerts', () => {
const alert = new Alert<AlertInstanceState, AlertInstanceContext, DefaultActionGroupId>('2', {
meta: { pendingRecoveredCount: 3 },
meta: { pendingRecoveredCount: 3, uuid: '1' },
});
expect(alert.isFilteredOut(summarizedAlerts)).toBe(false);
});
test('returns true if the alert with same UUID or ID is not in summarizedAlerts', () => {
const alert = new Alert<AlertInstanceState, AlertInstanceContext, DefaultActionGroupId>('2', {
meta: { pendingRecoveredCount: 3, uuid: '3' },
});
expect(alert.isFilteredOut(summarizedAlerts)).toBe(true);
});
Expand Down
20 changes: 17 additions & 3 deletions x-pack/plugins/alerting/server/alert/alert.ts
spong marked this conversation as resolved.
Show resolved Hide resolved
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@

import { v4 as uuidV4 } from 'uuid';
import { get, isEmpty } from 'lodash';
import { ALERT_INSTANCE_ID, ALERT_RULE_UUID } from '@kbn/rule-data-utils';
import { ALERT_UUID } from '@kbn/rule-data-utils';
import { CombinedSummarizedAlerts } from '../types';
import {
AlertInstanceMeta,
Expand Down Expand Up @@ -271,15 +271,29 @@ export class Alert<
this.meta.pendingRecoveredCount = 0;
}

/**
* Checks whether this alert exists in the given alert summary
*/
isFilteredOut(summarizedAlerts: CombinedSummarizedAlerts | null) {
if (summarizedAlerts === null) {
return false;
}

// We check the alert UUID against both the alert ID and the UUID here
// The framework generates a UUID for each new reported alert.
// For lifecycle rule types, this UUID is written out in the ALERT_UUID field
// so we can compare ALERT_UUID to getUuid()
// For persistence rule types, the executor generates its own UUID which is a SHA
// of the alert data and stores it in the ALERT_UUID and uses it as the alert ID
// before reporting the alert back to the framework. The framework then generates
// another UUID that is never persisted. For these alerts, we want to compare
// ALERT_UUID to getId()
//
// Related issue: https://github.com/elastic/kibana/issues/144862

return !summarizedAlerts.all.data.some(
(alert) =>
get(alert, ALERT_INSTANCE_ID) === this.getId() ||
get(alert, ALERT_RULE_UUID) === this.getId()
get(alert, ALERT_UUID) === this.getId() || get(alert, ALERT_UUID) === this.getUuid()
ymao1 marked this conversation as resolved.
Show resolved Hide resolved
);
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -165,8 +165,7 @@ export function getPartialRuleFromRaw<Params extends RuleTypeParams>(
// Need the `rule` object to build a URL
if (!excludeFromPublicApi) {
const viewInAppRelativeUrl =
ruleType.getViewInAppRelativeUrl &&
ruleType.getViewInAppRelativeUrl({ rule: rule as Rule<Params> });
ruleType.getViewInAppRelativeUrl && ruleType.getViewInAppRelativeUrl({ rule });
if (viewInAppRelativeUrl) {
rule.viewInAppRelativeUrl = viewInAppRelativeUrl;
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,8 @@ import { EncryptedSavedObjectsPluginSetup } from '@kbn/encrypted-saved-objects-p
import { v4 as uuidv4 } from 'uuid';
import { createEsoMigration, isDetectionEngineAADRuleType, pipeMigrations } from '../utils';
import { RawRule } from '../../../types';
import { transformToAlertThrottle } from '../../../rules_client/lib/siem_legacy_actions/transform_to_alert_throttle';
import { transformToNotifyWhen } from '../../../rules_client/lib/siem_legacy_actions/transform_to_notify_when';

function addRevision(doc: SavedObjectUnsanitizedDoc<RawRule>): SavedObjectUnsanitizedDoc<RawRule> {
return {
Expand Down Expand Up @@ -42,9 +44,38 @@ function addActionUuid(
};
}

function addSecuritySolutionActionsFrequency(
doc: SavedObjectUnsanitizedDoc<RawRule>
): SavedObjectUnsanitizedDoc<RawRule> {
if (isDetectionEngineAADRuleType(doc)) {
const {
attributes: { throttle, actions },
} = doc;

return {
...doc,
attributes: {
...doc.attributes,
actions: actions
? actions.map((action) => ({
...action,
// Till now SIEM worked without action level frequencies. Instead rule level `throttle` and `notifyWhen` used
frequency: action.frequency ?? {
summary: true,
notifyWhen: transformToNotifyWhen(throttle) ?? 'onActiveAlert',
throttle: transformToAlertThrottle(throttle),
},
}))
: [],
},
};
}
return doc;
}

export const getMigrations880 = (encryptedSavedObjects: EncryptedSavedObjectsPluginSetup) =>
createEsoMigration(
encryptedSavedObjects,
(doc: SavedObjectUnsanitizedDoc<RawRule>): doc is SavedObjectUnsanitizedDoc<RawRule> => true,
pipeMigrations(addActionUuid, addRevision)
pipeMigrations(addActionUuid, addRevision, addSecuritySolutionActionsFrequency)
);
Original file line number Diff line number Diff line change
Expand Up @@ -2668,6 +2668,52 @@ describe('successful migrations', () => {
const migratedAlert880 = migration880(rule, migrationContext);
expect(migratedAlert880.attributes.revision).toEqual(2);
});

describe('migrate actions frequency for Security Solution ', () => {
test('Add frequency when throttle is null', () => {
const migration880 = getMigrations(encryptedSavedObjectsSetup, {}, isPreconfigured)[
'8.8.0'
];

const rule = getMockData({ alertTypeId: ruleTypeMappings.eql });
const migratedAlert880 = migration880(rule, migrationContext);
expect(migratedAlert880.attributes.actions[0].frequency.summary).toEqual(true);
expect(migratedAlert880.attributes.actions[0].frequency.notifyWhen).toEqual(
'onActiveAlert'
);
expect(migratedAlert880.attributes.actions[0].frequency.throttle).toEqual(null);
});

test('Add frequency when throttle is 1h', () => {
const migration880 = getMigrations(encryptedSavedObjectsSetup, {}, isPreconfigured)[
'8.8.0'
];

const rule = getMockData({ alertTypeId: ruleTypeMappings.eql, throttle: '1h' });
const migratedAlert880 = migration880(rule, migrationContext);
expect(migratedAlert880.attributes.actions[0].frequency.summary).toEqual(true);
expect(migratedAlert880.attributes.actions[0].frequency.notifyWhen).toEqual(
'onThrottleInterval'
);
expect(migratedAlert880.attributes.actions[0].frequency.throttle).toEqual('1h');
});

test('Do not migrate action when alert does NOT belong to security solution', () => {
const migration880 = getMigrations(encryptedSavedObjectsSetup, {}, isPreconfigured)[
'8.8.0'
];

const rule = getMockData();
const migratedAlert880 = migration880(rule, migrationContext);
const updatedActions = migratedAlert880.attributes.actions.map(
(action: { [x: string]: unknown; uuid: unknown }) => {
const { uuid, ...updatedAction } = action;
return updatedAction;
}
);
expect(updatedActions).toEqual(rule.attributes.actions);
});
});
});

describe('Metrics Inventory Threshold rule', () => {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -1366,7 +1366,7 @@ describe('Execution Handler', () => {
getSummarizedAlertsMock.mockResolvedValue({
new: {
count: 1,
data: [{ ...mockAAD, kibana: { alert: { instance: { id: '1' } } } }],
data: [{ ...mockAAD, kibana: { alert: { uuid: '1' } } }],
},
ongoing: {
count: 0,
Expand Down
3 changes: 2 additions & 1 deletion x-pack/plugins/alerting/server/types.ts
Original file line number Diff line number Diff line change
Expand Up @@ -169,7 +169,8 @@ export interface CombinedSummarizedAlerts extends SummarizedAlerts {
}
export type GetSummarizedAlertsFn = (opts: GetSummarizedAlertsFnOpts) => Promise<SummarizedAlerts>;
export interface GetViewInAppRelativeUrlFnOpts<Params extends RuleTypeParams> {
rule: Omit<SanitizedRule<Params>, 'viewInAppRelativeUrl'>;
rule: Pick<SanitizedRule<Params>, 'id'> &
Omit<Partial<SanitizedRule<Params>>, 'viewInAppRelativeUrl'>;
// Optional time bounds
start?: number;
end?: number;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -12,19 +12,24 @@ import { chunk, partition } from 'lodash';
import {
ALERT_INSTANCE_ID,
ALERT_LAST_DETECTED,
ALERT_NAMESPACE,
ALERT_START,
ALERT_SUPPRESSION_DOCS_COUNT,
ALERT_SUPPRESSION_END,
ALERT_UUID,
ALERT_WORKFLOW_STATUS,
TIMESTAMP,
VERSION,
} from '@kbn/rule-data-utils';
import { mapKeys, snakeCase } from 'lodash/fp';
import { getCommonAlertFields } from './get_common_alert_fields';
import { CreatePersistenceRuleTypeWrapper } from './persistence_types';
import { errorAggregator } from './utils';
import { createGetSummarizedAlertsFn } from './create_get_summarized_alerts_fn';
import { AlertDocument, createGetSummarizedAlertsFn } from './create_get_summarized_alerts_fn';
import { AlertWithSuppressionFields870 } from '../../common/schemas/8.7.0';

export const ALERT_GROUP_INDEX = `${ALERT_NAMESPACE}.group.index` as const;

const augmentAlerts = <T>({
alerts,
options,
Expand Down Expand Up @@ -56,7 +61,7 @@ const mapAlertsToBulkCreate = <T>(alerts: Array<{ _id: string; _source: T }>) =>
};

export const createPersistenceRuleTypeWrapper: CreatePersistenceRuleTypeWrapper =
({ logger, ruleDataClient }) =>
({ logger, ruleDataClient, formatAlert }) =>
(type) => {
return {
...type,
Expand Down Expand Up @@ -160,17 +165,40 @@ export const createPersistenceRuleTypeWrapper: CreatePersistenceRuleTypeWrapper
return { createdAlerts: [], errors: {}, alertsWereTruncated };
}

return {
createdAlerts: augmentedAlerts
.map((alert, idx) => {
const responseItem = response.body.items[idx].create;
return {
_id: responseItem?._id ?? '',
_index: responseItem?._index ?? '',
...alert._source,
};
const createdAlerts = augmentedAlerts
.map((alert, idx) => {
const responseItem = response.body.items[idx].create;
return {
_id: responseItem?._id ?? '',
_index: responseItem?._index ?? '',
...alert._source,
};
})
.filter((_, idx) => response.body.items[idx].create?.status === 201)
// Security solution's EQL rule consists of building block alerts which should be filtered out.
// Building block alerts have additional "kibana.alert.group.index" attribute which is absent for the root alert.
.filter((alert) => !Object.keys(alert).includes(ALERT_GROUP_INDEX));
e40pud marked this conversation as resolved.
Show resolved Hide resolved

createdAlerts.forEach((alert) =>
options.services.alertFactory
.create(alert._id)
.scheduleActions(type.defaultActionGroupId, {
rule: mapKeys(snakeCase, {
...options.params,
name: options.rule.name,
id: options.rule.id,
Copy link
Contributor

Choose a reason for hiding this comment

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

do we want to make the alert available in the context variable? or a result_link?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah I'm not seeing alert data available in contxext.alerts for the per alert actions - do you have to use a different mustache variable for per alert actions to get the alert data? If not, we should probably add it in context.alerts.

Copy link
Contributor Author

@e40pud e40pud Apr 25, 2023

Choose a reason for hiding this comment

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

I intentionally removed context.alerts for per alert actions here. I was not sure whether we need it for the case when we have one alert only. I will add it back and make sure we pass alerts for single alert case as well, which will be just alerts: [alert].

Also, as discussed earlier with Ying, I will add results_link which will be a alert[ALERT_URL] for per alert action case.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@ymao1 @marshallmain I added results_link: alert[ALERT_URL] for per alert action. According to these changes alert[ALERT_URL] can be undefined. What would be a good alternative for the results_link in this case?

Copy link
Contributor

Choose a reason for hiding this comment

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

Hey @e40pud it can be undefined because it's based on the server.publicBaseUrl being set. We expect users to have that configured as connectors apparently won't work without it, but there's a chance they may not always have it configured. I'm not sure about what a smart alternative default would be

Copy link
Member

Choose a reason for hiding this comment

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

Ya, there is no good alternative. In similar cases in alerting, we use undefined. It's possible to "if" this condition (falsy) in mustache, so customers can "mustache" it in or out, if it's there or not.

}),
results_link: type.getViewInAppRelativeUrl?.({
rule: { ...options.rule, params: options.params },
start: Date.parse(alert[TIMESTAMP]),
end: Date.parse(alert[TIMESTAMP]),
}),
alerts: [alert],
Copy link
Contributor

Choose a reason for hiding this comment

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

Note for future improvement: we can include the EQL building block alerts in the alerts context here so that the action-per-alert context for EQL sequences has the entire sequence

Copy link
Contributor

Choose a reason for hiding this comment

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

Also we should call formatAlert on the alerts here before passing them into the actions context.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Here is the ticket for this #155748

Copy link
Contributor

Choose a reason for hiding this comment

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

Followup issue for formatAlert #155812

})
.filter((_, idx) => response.body.items[idx].create?.status === 201),
);

return {
createdAlerts,
errors: errorAggregator(response.body, [409]),
alertsWereTruncated,
};
Expand Down Expand Up @@ -325,21 +353,44 @@ export const createPersistenceRuleTypeWrapper: CreatePersistenceRuleTypeWrapper
return { createdAlerts: [], errors: {} };
}

return {
createdAlerts: augmentedAlerts
.map((alert, idx) => {
const responseItem =
bulkResponse.body.items[idx + duplicateAlerts.length].create;
return {
_id: responseItem?._id ?? '',
_index: responseItem?._index ?? '',
...alert._source,
};
const createdAlerts = augmentedAlerts
.map((alert, idx) => {
const responseItem =
bulkResponse.body.items[idx + duplicateAlerts.length].create;
return {
_id: responseItem?._id ?? '',
_index: responseItem?._index ?? '',
...alert._source,
};
})
.filter(
(_, idx) =>
bulkResponse.body.items[idx + duplicateAlerts.length].create?.status === 201
)
// Security solution's EQL rule consists of building block alerts which should be filtered out.
// Building block alerts have additional "kibana.alert.group.index" attribute which is absent for the root alert.
.filter((alert) => !Object.keys(alert).includes(ALERT_GROUP_INDEX));

createdAlerts.forEach((alert) =>
options.services.alertFactory
.create(alert._id)
.scheduleActions(type.defaultActionGroupId, {
rule: mapKeys(snakeCase, {
...options.params,
name: options.rule.name,
id: options.rule.id,
}),
results_link: type.getViewInAppRelativeUrl?.({
rule: { ...options.rule, params: options.params },
start: Date.parse(alert[TIMESTAMP]),
end: Date.parse(alert[TIMESTAMP]),
}),
alerts: [alert],
})
.filter(
(_, idx) =>
bulkResponse.body.items[idx + duplicateAlerts.length].create?.status === 201
),
);

return {
createdAlerts,
errors: errorAggregator(bulkResponse.body, [409]),
};
} else {
Expand All @@ -356,6 +407,7 @@ export const createPersistenceRuleTypeWrapper: CreatePersistenceRuleTypeWrapper
ruleDataClient,
useNamespace: true,
isLifecycleAlert: false,
formatAlert: formatAlert as (alert: AlertDocument) => AlertDocument,
})(),
};
};
12 changes: 4 additions & 8 deletions x-pack/plugins/rule_registry/server/utils/persistence_types.ts
Original file line number Diff line number Diff line change
Expand Up @@ -90,11 +90,7 @@ export type PersistenceAlertType<
export type CreatePersistenceRuleTypeWrapper = (options: {
ruleDataClient: IRuleDataClient;
logger: Logger;
}) => <
TParams extends RuleTypeParams,
TState extends RuleTypeState,
TInstanceContext extends AlertInstanceContext = {},
TActionGroupIds extends string = never
>(
type: PersistenceAlertType<TParams, TState, TInstanceContext, TActionGroupIds>
) => RuleType<TParams, TParams, TState, AlertInstanceState, TInstanceContext, TActionGroupIds>;
formatAlert?: (alert: unknown) => unknown;
}) => <TParams extends RuleTypeParams, TState extends RuleTypeState>(
type: PersistenceAlertType<TParams, TState, AlertInstanceContext, 'default'>
) => RuleType<TParams, TParams, TState, AlertInstanceState, AlertInstanceContext, 'default'>;
Original file line number Diff line number Diff line change
Expand Up @@ -565,6 +565,7 @@ const CreateRulePageComponent: React.FC = () => {
setForm={setFormHook}
onSubmit={() => submitStep(RuleStep.ruleActions)}
actionMessageParams={actionMessageParams}
summaryActionMessageParams={actionMessageParams}
// We need a key to make this component remount when edit/view mode is toggled
// https://github.com/elastic/kibana/pull/132834#discussion_r881705566
key={isShouldRerenderStep(RuleStep.ruleActions, activeStep)}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -310,6 +310,7 @@ const EditRulePageComponent: FC = () => {
defaultValues={actionsStep.data}
setForm={setFormHook}
actionMessageParams={actionMessageParams}
summaryActionMessageParams={actionMessageParams}
ruleType={rule?.type}
/>
)}
Expand Down
Loading