[SIEM][Detection Engine] Add rule's notification alert type by patrykkopycinski · Pull Request #60832 · elastic/kibana

patrykkopycinski · 2020-03-20T22:30:21Z

Summary

Part of #59004

Adds Rule's Notification alert type that will be responsible for fetching signals count and scheduling actions
Adds support for Notifications in Rules routes
Adds support for scheduling Actions in SignalRuleAlertType
Adds unit tests coverage

Checklist

Delete any items that are not applicable to this PR.

Any text added follows EUI's writing guidelines, uses sentence case text and includes i18n support
Documentation was added for features that require explanation or tutorials
Unit or functional tests were updated or added to match the most common scenarios
This was checked for keyboard-only and screenreader accessibility
This renders correctly on smaller devices using a responsive layout. (You can test this in your browser
This was checked for cross-browser compatibility, including a check against IE11

For maintainers

This was checked for breaking API changes and was labeled appropriately

elasticmachine · 2020-03-23T12:42:03Z

Pinging @elastic/siem (Team:SIEM)

rylnd

Just a few minor comments, nothing seems blocking. One question about a new type and its purpose.

Thank you for adding tests, generalizing types, and just generally leaving things better than you found them!

rylnd · 2020-03-23T19:40:00Z

x-pack/legacy/plugins/siem/server/lib/detection_engine/notifications/read_notifications.test.ts

+
+class TestError extends Error {
+  constructor() {
+    // Pass remaining arguments (including vendor specific ones) to parent constructor


Is this a TODO?

rylnd · 2020-03-23T19:40:33Z

...plugins/siem/server/lib/detection_engine/notifications/rules_notification_alert_type.test.ts

+jest.mock('./build_signals_query');
+
+describe('rules_notification_alert_type', () => {
+  const savedObjectsClient = savedObjectsClientMock.create();


I would move these to a beforeEach if possible. The less shared state, the better.

rylnd · 2020-03-23T19:44:03Z

...gacy/plugins/siem/server/lib/detection_engine/notifications/schedule_notification_actions.ts

+  ruleParams,
+}: ScheduleNotificationActions): AlertInstance =>
+  alertInstance
+    .replaceState({


rylnd · 2020-03-23T19:56:46Z

x-pack/legacy/plugins/siem/server/lib/detection_engine/types.ts

  'name' | 'enabled' | 'interval' | 'tags' | 'actions' | 'throttle'
 >;

+export type RuleTypeParamsWithName = RuleTypeParams & {


Why the special casing of name, here? Is it possible to use it throughout our SIEM alerts, or do we expressly not want it in other alerts?

spong · 2020-03-23T20:53:17Z

...k/legacy/plugins/siem/server/lib/detection_engine/notifications/create_notifications.test.ts

+    const action = {
+      group: 'default',
+      id: '99403909-ca9b-49ba-9d7a-7e5320e68d05',
+      params: { message: 'Rule generated {{state.signalsCount}} singals' },


Suggested change

params: { message: 'Rule generated {{state.signalsCount}} singals' },

params: { message: 'Rule generated {{state.signalsCount}} signals' },

spong · 2020-03-23T20:53:35Z

...k/legacy/plugins/siem/server/lib/detection_engine/notifications/update_notifications.test.ts

+    const action = {
+      group: 'default',
+      id: '99403909-ca9b-49ba-9d7a-7e5320e68d05',
+      params: { message: 'Rule generated {{state.signalsCount}} singals' },


Suggested change

params: { message: 'Rule generated {{state.signalsCount}} singals' },

params: { message: 'Rule generated {{state.signalsCount}} signals' },

spong · 2020-03-23T20:53:49Z

x-pack/legacy/plugins/siem/server/lib/detection_engine/routes/__mocks__/request_responses.ts

+        {
+          group: 'default',
+          id: '99403909-ca9b-49ba-9d7a-7e5320e68d05',
+          params: { message: 'Rule generated {{state.signalsCount}} singals' },


Suggested change

params: { message: 'Rule generated {{state.signalsCount}} singals' },

params: { message: 'Rule generated {{state.signalsCount}} signals' },

spong · 2020-03-23T20:54:05Z

x-pack/legacy/plugins/siem/server/lib/detection_engine/routes/__mocks__/request_responses.ts

+    {
+      actionTypeId: '.slack',
+      params: {
+        message: 'Rule generated {{state.signalsCount}} singals\n\n{{rule.name}}\n{{resultsLink}}',


Suggested change

message: 'Rule generated {{state.signalsCount}} singals\n\n{{rule.name}}\n{{resultsLink}}',

message: 'Rule generated {{state.signalsCount}} signals\n\n{{rule.name}}\n{{resultsLink}}',

spong · 2020-03-23T22:23:13Z

...gacy/plugins/siem/server/lib/detection_engine/notifications/rules_notification_alert_type.ts

+      name: i18n.translate(
+        'xpack.siem.detectionEngine.ruleNotificationAlert.actionGroups.default',
+        {
+          defaultMessage: 'Default',
+        }
+      ),


I thought there wasn't any i18n on the server side -- are we able to do this now, and if so, do you know how the locale gets correctly set?

spong · 2020-03-23T23:08:08Z

...gacy/plugins/siem/server/lib/detection_engine/notifications/rules_notification_alert_type.ts

+      from: previousStartedAt ?? `now-${ruleParams.interval}`,
+      to: startedAt,


Not sure if this daterange is the issue or if it's timing from the signals being written, but I'm sometimes seeing mis-match signals counts in the server logs between signalsRulesAlertType and rulesNotificationsAlertType, and then the action is not fired.

e.g. here's the server log from when a single rule runs:

server log [17:00:04.302] [info][plugins][siem] Found 55 signals from the indexes of "[apm-*-transaction*, auditbeat-*, endgame-*, filebeat-*, packetbeat-*, winlogbeat-*]" using signal rule name: "Signal Generator 7k", id: "c0880b7e-0bbb-4ef2-9471-056d36741ba0", rule_id: "38bcddb9-8751-40b8-8244-04254c7a818c", pushing signals to index ".siem-signals-spong-default" server log [17:00:05.267] [info][plugins][siem] Found 0 signals using signal rule name: "Signal Generator 7k", id: "38bcddb9-8751-40b8-8244-04254c7a818c", rule_id: "38bcddb9-8751-40b8-8244-04254c7a818c" in ".siem-signals-spong-default" index

Which does not result in the rule's action being completed (slack message in this instance). No explicit repro steps yet, but I've noticed it mostly when editing an action after a rule has been created, and then this happens the next time the rule runs.

spong · 2020-03-23T23:10:09Z

x-pack/legacy/plugins/siem/server/plugin.ts

    if (plugins.alerting != null) {
-      const type = signalRulesAlertType({
+      const { host, port, protocol } = core.http.getServerInfo();
+      const kibanaUrl = `${protocol}://${host}:${port}`;


Not sure if this will work on cloud -- may want to touch base with the platform team to ensure this will be fine there.

spong · 2020-03-23T23:16:05Z

x-pack/legacy/plugins/siem/server/lib/detection_engine/rules/create_rules.ts

-export const createRules = ({
+export const createRules = async ({
  alertsClient,
  actionsClient, // TODO: Use this actionsClient once we have actions such as email, etc...


Is this TODO/prop still necessary?

I'm going to clean up actionsClient and the comments afterward :)

spong

Checked out, tested locally via UI provided in #59004, and performed a code review. Left a few nits but looks good to me! 👍 Was able to configure slack notifications and receive message when rules were run -- great work here @patrykkopycinski! 🙂🚀

…patrykkopycinski/kibana into feat/siem-rule-notifications-alert-type

kibanamachine · 2020-03-24T11:32:51Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request
Commit: 947911c

History

💚 Build #35367 succeeded f5a3eb4
💚 Build #35340 succeeded 0cea359
💔 Build #35112 failed e9cebc0

To update your PR or re-run it, just comment with:
@elasticmachine merge upstream

…60832)

* master: Updating our direct usage of https-proxy-agent to 5.0.0 (elastic#58296) allow users to unset the throttle of an alert (elastic#60964) [Lens] Fix bug in metric config panel (elastic#60982) [SearchProfiler] Minor fixes (elastic#60919) [ML] Renaming ML setup and start contracts (elastic#60980) introduce StartServicesAccessor type for `CoreSetup.getStartServices` (elastic#60748) [SIEM][Detection Engine] Add rule's notification alert type (elastic#60832) [APM] Re-revert "Collect telemetry about data/API performance" (elastic#61030) [NP] Graph: get rid of saved objects class wrapper (elastic#59917) [EPM] merge duplicate fields when creating index patterns (elastic#60957) [Uptime] Ml detection of duration anomalies (elastic#59785) [Alerting] removes unimplemented buttons from Alert Details page (elastic#60934) [skip-ci] Fix CODEOWNERS paths for the Pulse team (elastic#60944) [APM] Threshold alerts (elastic#59566) [ML] Add support for percentiles aggregation to Transform wizard (elastic#60763) Cahgen save object duplicate message (elastic#60901)

…61068)

## Summary Allow defining notifications that will trigger whenever the rule created new signals. Requires: - #58395 - #58964 - #60832 ![Screenshot 2020-03-02 at 10 19 18](https://user-images.githubusercontent.com/5188868/75662390-4fe8bf00-5c6f-11ea-943f-591367348b91.png) ![Screenshot 2020-03-02 at 10 13 00](https://user-images.githubusercontent.com/5188868/75662421-5e36db00-5c6f-11ea-9317-d158cddf4344.png) ### Checklist Delete any items that are not applicable to this PR. - [ ] Any text added follows [EUI's writing guidelines](https://elastic.github.io/eui/#/guidelines/writing), uses sentence case text and includes [i18n support](https://github.com/elastic/kibana/blob/master/packages/kbn-i18n/README.md) - [ ] [Documentation](https://github.com/elastic/kibana/blob/master/CONTRIBUTING.md#writing-documentation) was added for features that require explanation or tutorials - [ ] [Unit or functional tests](https://github.com/elastic/kibana/blob/master/CONTRIBUTING.md#cross-browser-compatibility) were updated or added to match the most common scenarios - [ ] This was checked for [keyboard-only and screenreader accessibility](https://developer.mozilla.org/en-US/docs/Learn/Tools_and_testing/Cross_browser_testing/Accessibility#Accessibility_testing_checklist) - [ ] This renders correctly on smaller devices using a responsive layout. (You can test this [in your browser](https://www.browserstack.com/guide/responsive-testing-on-local-server) - [ ] This was checked for cross-browser compatibility, [including a check against IE11](https://github.com/elastic/kibana/blob/master/CONTRIBUTING.md#cross-browser-compatibility) ### For maintainers - [ ] This was checked for breaking API changes and was [labeled appropriately](https://github.com/elastic/kibana/blob/master/CONTRIBUTING.md#release-notes-process)

## Summary Allow defining notifications that will trigger whenever the rule created new signals. Requires: - elastic#58395 - elastic#58964 - elastic#60832 ![Screenshot 2020-03-02 at 10 19 18](https://user-images.githubusercontent.com/5188868/75662390-4fe8bf00-5c6f-11ea-943f-591367348b91.png) ![Screenshot 2020-03-02 at 10 13 00](https://user-images.githubusercontent.com/5188868/75662421-5e36db00-5c6f-11ea-9317-d158cddf4344.png) ### Checklist Delete any items that are not applicable to this PR. - [ ] Any text added follows [EUI's writing guidelines](https://elastic.github.io/eui/#/guidelines/writing), uses sentence case text and includes [i18n support](https://github.com/elastic/kibana/blob/master/packages/kbn-i18n/README.md) - [ ] [Documentation](https://github.com/elastic/kibana/blob/master/CONTRIBUTING.md#writing-documentation) was added for features that require explanation or tutorials - [ ] [Unit or functional tests](https://github.com/elastic/kibana/blob/master/CONTRIBUTING.md#cross-browser-compatibility) were updated or added to match the most common scenarios - [ ] This was checked for [keyboard-only and screenreader accessibility](https://developer.mozilla.org/en-US/docs/Learn/Tools_and_testing/Cross_browser_testing/Accessibility#Accessibility_testing_checklist) - [ ] This renders correctly on smaller devices using a responsive layout. (You can test this [in your browser](https://www.browserstack.com/guide/responsive-testing-on-local-server) - [ ] This was checked for cross-browser compatibility, [including a check against IE11](https://github.com/elastic/kibana/blob/master/CONTRIBUTING.md#cross-browser-compatibility) ### For maintainers - [ ] This was checked for breaking API changes and was [labeled appropriately](https://github.com/elastic/kibana/blob/master/CONTRIBUTING.md#release-notes-process)

## Summary Allow defining notifications that will trigger whenever the rule created new signals. Requires: - #58395 - #58964 - #60832 ![Screenshot 2020-03-02 at 10 19 18](https://user-images.githubusercontent.com/5188868/75662390-4fe8bf00-5c6f-11ea-943f-591367348b91.png) ![Screenshot 2020-03-02 at 10 13 00](https://user-images.githubusercontent.com/5188868/75662421-5e36db00-5c6f-11ea-9317-d158cddf4344.png) ### Checklist Delete any items that are not applicable to this PR. - [ ] Any text added follows [EUI's writing guidelines](https://elastic.github.io/eui/#/guidelines/writing), uses sentence case text and includes [i18n support](https://github.com/elastic/kibana/blob/master/packages/kbn-i18n/README.md) - [ ] [Documentation](https://github.com/elastic/kibana/blob/master/CONTRIBUTING.md#writing-documentation) was added for features that require explanation or tutorials - [ ] [Unit or functional tests](https://github.com/elastic/kibana/blob/master/CONTRIBUTING.md#cross-browser-compatibility) were updated or added to match the most common scenarios - [ ] This was checked for [keyboard-only and screenreader accessibility](https://developer.mozilla.org/en-US/docs/Learn/Tools_and_testing/Cross_browser_testing/Accessibility#Accessibility_testing_checklist) - [ ] This renders correctly on smaller devices using a responsive layout. (You can test this [in your browser](https://www.browserstack.com/guide/responsive-testing-on-local-server) - [ ] This was checked for cross-browser compatibility, [including a check against IE11](https://github.com/elastic/kibana/blob/master/CONTRIBUTING.md#cross-browser-compatibility) ### For maintainers - [ ] This was checked for breaking API changes and was [labeled appropriately](https://github.com/elastic/kibana/blob/master/CONTRIBUTING.md#release-notes-process) Co-authored-by: patrykkopycinski <patryk.kopycinski@elastic.co>

patrykkopycinski added v8.0.0 release_note:skip Skip the PR/issue when compiling release notes v7.7.0 labels Mar 20, 2020

patrykkopycinski self-assigned this Mar 20, 2020

[SIEM] Add notification alert type

0cea359

patrykkopycinski force-pushed the feat/siem-rule-notifications-alert-type branch from e9cebc0 to 0cea359 Compare March 23, 2020 11:00

Update query.dsl.ts

f5a3eb4

patrykkopycinski added the Team:SIEM label Mar 23, 2020

patrykkopycinski requested review from FrankHassanabad and rylnd and removed request for rylnd March 23, 2020 12:42

patrykkopycinski requested review from dhurley14 and rylnd March 23, 2020 12:42

patrykkopycinski marked this pull request as ready for review March 23, 2020 12:42

patrykkopycinski requested a review from a team as a code owner March 23, 2020 12:42

patrykkopycinski mentioned this pull request Mar 23, 2020

[SIEM] Add rule notifications #59004

Merged

7 tasks

patrykkopycinski requested a review from yctercero March 23, 2020 15:51

rylnd reviewed Mar 23, 2020

View reviewed changes

spong reviewed Mar 23, 2020

View reviewed changes

spong approved these changes Mar 23, 2020

View reviewed changes

patrykkopycinski added 2 commits March 24, 2020 10:40

PR comments

8771446

Merge branch 'feat/siem-rule-notifications-alert-type' of github.com:…

947911c

…patrykkopycinski/kibana into feat/siem-rule-notifications-alert-type

patrykkopycinski merged commit 2106b69 into elastic:master Mar 24, 2020

patrykkopycinski deleted the feat/siem-rule-notifications-alert-type branch March 24, 2020 13:26

patrykkopycinski mentioned this pull request Mar 24, 2020

[7.x] [SIEM][Detection Engine] Add rule's notification alert type (#60832) #61068

Merged

patrykkopycinski added a commit to patrykkopycinski/kibana that referenced this pull request Mar 24, 2020

[SIEM][Detection Engine] Add rule's notification alert type (elastic#…

a6ca89a

…60832)

patrykkopycinski added a commit that referenced this pull request Mar 24, 2020

[SIEM][Detection Engine] Add rule's notification alert type (#60832) (#…

c2109b8

…61068)

	params: { message: 'Rule generated {{state.signalsCount}} singals' },
	params: { message: 'Rule generated {{state.signalsCount}} signals' },

	message: 'Rule generated {{state.signalsCount}} singals\n\n{{rule.name}}\n{{resultsLink}}',
	message: 'Rule generated {{state.signalsCount}} signals\n\n{{rule.name}}\n{{resultsLink}}',

		from: previousStartedAt ?? `now-${ruleParams.interval}`,
		to: startedAt,

Comments

Conversation

patrykkopycinski commented Mar 20, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Checklist

For maintainers

Uh oh!

elasticmachine commented Mar 23, 2020

Uh oh!

rylnd left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

spong left a comment

Choose a reason for hiding this comment

Uh oh!

kibanamachine commented Mar 24, 2020

💚 Build Succeeded

History

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

patrykkopycinski commented Mar 20, 2020 •

edited

Loading