[Security Solution] Converge detection engine on single schema representation #96186

marshallmain · 2021-04-03T00:37:10Z

Summary

In this PR we:

Migrate siem.signals alerts to convert null values to undefined and also normalize threshold.field in the SO so we don't have to normalize it at runtime
Delete signalParamsSchema in favor of io-ts based schemas that we already use for the API, making it easier to maintain parity between API types and internal SO/detection engine types
Remove most usage of RuleTypeParams interface, which was a pseudo-union of all 6 rule types but typed any rule-type specific fields as optional (and also had to be manually maintained, rather than being generated from the schema)
- As part of this removal, functions that used RuleTypeParams and fields from the rule SO were converted to take the rule SO as an entire object instead (see e.g. buildBulkBody)
Added more mock data closer to the schema that defines the data structure in rule_schemas.mock.ts and started reducing mock data duplication

Future work:
[ ] If/when #95843 is addressed, we can convert from using the rule SO structure to using the Alert structure
[ ] We need to convert the import and addPrepackaged routes to use rule-type specific validation

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
Any UI touched in this PR is usable by keyboard only (learn more about keyboard accessibility)
Any UI touched in this PR does not create any new axe failures (run axe in browser: FF, Chrome)
If a plugin configuration key changed, check if it needs to be allowlisted in the cloud and added to the docker list
This renders correctly on smaller devices using a responsive layout. (You can test this in your browser)
This was checked for cross-browser compatibility

For maintainers

This was checked for breaking API changes and was labeled appropriately

…the same

x-pack/plugins/security_solution/common/detection_engine/schemas/common/schemas.ts

marshallmain · 2021-04-08T03:17:23Z

x-pack/plugins/security_solution/common/detection_engine/schemas/request/rule_schemas.ts

@@ -159,12 +159,11 @@ const commonParams = {
    tags,
    interval,
    enabled,
-    throttle: throttleOrNull,
+    throttle,


Minor API change here, no longer allowing null to be sent in as a value for throttle. We already use throttle: 'no_actions' in the UI and API tests to represent the same concept that null represents here, and null is not listed as an allowed value in the API docs.

The driving reason for making this change is that the existing manually created response schema defines throttle as an optional string (not null), and this newer schema builds both the request and response schema using the same structure to ensure parity between them. If we leave this field as throttleOrNull then the new response schema will allow null and therefore will be incompatible with the existing response schema. So the best way to get consistency between the existing response and the new response is to simply disallow null as an input in this schema, a slight change from existing behavior but still compatible with our public docs.

marshallmain · 2021-04-08T03:25:22Z

x-pack/plugins/security_solution/common/detection_engine/schemas/request/rule_schemas.ts

  created_at,
-  created_by: createdByOrNull,
+  created_by,


Similar to above with throttleOrNull being changed to throttle, in this case updated_by and created_by are no longer allowed to be null in the response to get consistency with the existing response schema.

madirey · 2021-04-12T15:32:07Z

x-pack/plugins/alerting/server/saved_objects/migrations.ts

+          typeof params.threshold === 'object' &&
+          !Array.isArray(params.threshold)
+            ? {
+                field: Array.isArray(params.threshold.field)


This is fun 😂

madirey · 2021-04-12T15:42:59Z

Threshold work looks good 👍

marshallmain · 2021-04-12T16:20:26Z

@elasticmachine merge upstream

marshallmain · 2021-04-13T23:23:36Z

x-pack/plugins/alerting/server/saved_objects/migrations.ts

+            ? params.exceptions_list
+            : params.lists != null
+            ? params.lists
+            : [],


@FrankHassanabad Is there any other conversion that needs to be done on these fields or is the rename enough?

rylnd

Marshall walked me through this code earlier. We uncovered one missing normalization and a few more non-essential improvements that he's working on right now. I called a few of them out in review here, but I trust CI and Marshall's diligence for the rest.

Overall, WOW. This is an incredible payment toward the detection engine tech debt, as evidenced by the diff stats here 😉 . The fact that this will enable further cleanup is even more exciting!

Thank you for making the time to clean this up, it is very much appreciated.

rylnd · 2021-04-14T00:34:15Z

x-pack/plugins/security_solution/cypress/objects/rule.ts

@@ -332,5 +332,5 @@ export const editedRule = {
 export const expectedExportedRule = (ruleResponse: Cypress.Response) => {
  const jsonrule = ruleResponse.body;

-  return `{"author":[],"actions":[],"created_at":"${jsonrule.created_at}","updated_at":"${jsonrule.updated_at}","created_by":"elastic","description":"${jsonrule.description}","enabled":false,"false_positives":[],"from":"now-17520h","id":"${jsonrule.id}","immutable":false,"index":["exceptions-*"],"interval":"10s","rule_id":"rule_testing","language":"kuery","output_index":".siem-signals-default","max_signals":100,"risk_score":${jsonrule.risk_score},"risk_score_mapping":[],"name":"${jsonrule.name}","query":"${jsonrule.query}","references":[],"severity":"${jsonrule.severity}","severity_mapping":[],"updated_by":"elastic","tags":[],"to":"now","type":"query","threat":[],"throttle":"no_actions","version":1,"exceptions_list":[]}\n{"exported_count":1,"missing_rules":[],"missing_rules_count":0}\n`;
+  return `{"id":"${jsonrule.id}","updated_at":"${jsonrule.updated_at}","updated_by":"elastic","created_at":"${jsonrule.created_at}","created_by":"elastic","name":"${jsonrule.name}","tags":[],"interval":"10s","enabled":false,"description":"${jsonrule.description}","risk_score":${jsonrule.risk_score},"severity":"${jsonrule.severity}","output_index":".siem-signals-default","author":[],"false_positives":[],"from":"now-17520h","rule_id":"rule_testing","max_signals":100,"risk_score_mapping":[],"severity_mapping":[],"threat":[],"to":"now","references":[],"version":1,"exceptions_list":[],"immutable":false,"type":"query","language":"kuery","index":["exceptions-*"],"query":"${jsonrule.query}","throttle":"no_actions","actions":[]}\n{"exported_count":1,"missing_rules":[],"missing_rules_count":0}\n`;


As discussed offline: the ordering here changed due to the object now being build piecemeal from:

base fields

type-specific fields

action sidecar fields

rylnd · 2021-04-14T00:35:30Z

...security_solution/server/lib/detection_engine/notifications/rules_notification_alert_type.ts

@@ -14,7 +14,7 @@ import {
 } from '../../../../common/constants';

 import { NotificationAlertTypeDefinition } from './types';
-import { RuleAlertAttributes } from '../signals/types';
+import { AlertAttributes } from '../signals/types';


nit: we talked about potentially removing the AlertAttributes type in favor of the analogous Alert type from alerting (which we're already using below to type response mocks).

I tried removing this briefly and found that Alert in the alerting framework does some processing on the raw SO before it becomes an Alert: it at least converts the created_at and updated_at fields from string to Date, I didn't look further than that. We don't want to duplicate or couple to that conversion process, so it's probably best to leave AlertAttributes as a completely raw representation of the SO until we get access to the full Alert in the executor through a formal alerting framework parameter. At that point we should be able to remove the entire codepath where we sidestep the alerting framework and directly access the SO, and remove AlertAttributes with it.

rylnd · 2021-04-14T00:36:28Z

...lugins/security_solution/server/lib/detection_engine/routes/rules/create_rules_bulk_route.ts

@@ -101,12 +99,9 @@ export const createRulesBulkRoute = (
                });
              }

-              /**
-               * TODO: Remove this use of `as` by utilizing the proper type


x-pack/plugins/security_solution/server/lib/detection_engine/routes/rules/create_rules_route.ts

x-pack/plugins/security_solution/server/lib/detection_engine/notifications/types.test.ts

rylnd · 2021-04-14T00:59:45Z

x-pack/plugins/security_solution/server/lib/detection_engine/signals/single_bulk_create.ts

 }: SingleBulkCreateParams): Promise<SingleBulkCreateResponse> => {
+  const ruleParams = ruleSO.attributes.params;


rylnd · 2021-04-14T01:00:59Z

x-pack/plugins/security_solution/server/lib/detection_engine/signals/executors/threshold.ts

@@ -83,7 +77,7 @@ export const thresholdExecutor = async ({
      services,
      logger,
      ruleId: ruleParams.ruleId,
-      bucketByFields: normalizeThresholdField(ruleParams.threshold.field),


Sanity check: this is no longer necessary due to the migration, correct?

Right, the migration will convert all threshold.field values to string[]. By this point in the executor we've also runtime-validated with thresholdSpecificRuleParams so the compiler knows ruleParams.threshold must be of type ThresholdNormalized

rylnd · 2021-04-14T01:06:31Z

x-pack/plugins/security_solution/server/lib/detection_engine/signals/__mocks__/es_results.ts

-});
-
-export const sampleRuleSO = (): SavedObject<RuleAlertAttributes> => {
+export const sampleRuleSO = <T extends RuleParams>(params: T): SavedObject<AlertAttributes<T>> => {


nit: this looks to share a lot of functionality with getResult; could we perhaps rewrite this in terms of getResult?

Great question, I spent some time while refactoring trying to combine these which lead to creating #95843. Summary: once we have access to the alerting framework Alert object in the executor, we can further unify these types and mocks.

Expanded: similar to above with removing AlertAttributes, this mock is used for functions that expect a raw Alert SO as opposed to the processed Alert that the alerting framework client provides. These functions are called from the detection engine executor function where we don't have access to the alerts client so we (currently) have to sidestep the alerts client and get the SO directly using the SO client. getResult is used as a mock for functions that take an Alert as a parameter. Those functions are called from the API routes since in those routes we have access to an alerts client.

x-pack/plugins/security_solution/server/lib/detection_engine/signals/signal_rule_alert_type.ts

rylnd · 2021-04-14T01:12:41Z

...solution/server/lib/detection_engine/signals/threshold/bulk_create_threshold_signals.test.ts

 import { sampleDocNoSortId, sampleDocSearchResultsNoSortId } from '../__mocks__/es_results';
 import { sampleThresholdSignalHistory } from '../__mocks__/threshold_signal_history.mock';
 import { calculateThresholdSignalUuid } from '../utils';
 import { transformThresholdResultsToEcs } from './bulk_create_threshold_signals';

 describe('transformThresholdNormalizedResultsToEcs', () => {
-  it('should return transformed threshold results for pre-7.12 rules', () => {


Are these tests no longer relevant due to the migration?

Yes, we shouldn't need to worry about pre-7.12 rules anymore. Since transformThresholdResultsToEcs takes a normalized threshold parameter even before these changes, these tests seem to be more about testing the conversion from Threshold to ThresholdNormalized rather than testing transformThresholdResultsToEcs with different inputs. The conversion has test coverage in utils.test.ts so I don't think we're losing significant coverage here.

…nitions

marshallmain · 2021-04-14T04:00:16Z

@elasticmachine merge upstream

…nto one-schema-to-rule

kibanamachine · 2021-04-14T07:55:15Z

💚 Build Succeeded

Metrics [docs]

Page load bundle

Size of the bundles that are downloaded on every page load. Target size is below 100kb

id	before	after	diff
`securitySolution`	215.6KB	215.4KB	-168.0B

History

💔 Build #119497 failed 5b28455
💔 Build #119496 failed e0c969c
💔 Build #118819 failed 6da57c1
💛 Build #118087 was flaky 05237b3
💔 Build #118018 failed aaae517

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

pmuellr

LGTM

…entation (elastic#96186) * Replace validation function in signal executor * Remove more RuleTypeParams usage * Add security solution rules migration to alerting plugin * Handle and test null value in threshold.field * Remove runtime normalization of threshold field * Remove signalParamsSchema Co-authored-by: Davis Plumlee <[email protected]> Co-authored-by: Kibana Machine <[email protected]>

kibanamachine · 2021-04-14T16:56:24Z

💚 Backport successful

Status	Branch	Result
✅	7.x

This backport PR will be merged automatically after passing CI.

…entation (#96186) (#97152) * Replace validation function in signal executor * Remove more RuleTypeParams usage * Add security solution rules migration to alerting plugin * Handle and test null value in threshold.field * Remove runtime normalization of threshold field * Remove signalParamsSchema Co-authored-by: Davis Plumlee <[email protected]> Co-authored-by: Kibana Machine <[email protected]> Co-authored-by: Marshall Main <[email protected]> Co-authored-by: Davis Plumlee <[email protected]>

marshallmain and others added 17 commits April 2, 2021 17:30

Replace validation function in signal executor

303f4e7

fix types

93afc0e

about to do something maybe stupid

baf0837

not stupid totally worked

5933786

Remove more RuleTypeParams usage

3c9be2e

Remove more RuleTypeParams usage

c154260

Fix imports and tests

c079ac0

Update more tests

44ea270

fixes jest tests

b1a3359

fixes rest of tests

caaa052

reverts unneeded output index change

1c803e0

Add security solution rules migration to alerting plugin

ec85c8d

Remove extra function

79ba987

Handle and test null value in threshold.field

d1c2aec

Limit threshold schema changes to internal representation, leave API …

bfbff0a

…the same

Remove runtime normalization of threshold field

42367e3

Fix unit tests

184e0dc

marshallmain force-pushed the one-schema-to-rule branch from a72bff4 to 184e0dc Compare April 6, 2021 21:21

marshallmain added 3 commits April 6, 2021 15:34

Allow threshold.cardinality to be undefined in internal schema

046bca1

Remove signalParamsSchema

b6d60a4

Fix comment

40c80da

marshallmain changed the title ~~[Draft][Security Solution] Converge detection engine on single schema representation~~ [Security Solution] Converge detection engine on single schema representation Apr 7, 2021

marshallmain marked this pull request as ready for review April 7, 2021 07:12

marshallmain requested review from a team as code owners April 7, 2021 07:12

marshallmain added release_note:skip Skip the PR/issue when compiling release notes Team: SecuritySolution Security Solutions Team working on SIEM, Endpoint, Timeline, Resolver, etc. Team:Detections and Resp Security Detection Response Team v7.13.0 v8.0.0 labels Apr 7, 2021

Merge branch 'master' into one-schema-to-rule

05237b3

marshallmain commented Apr 8, 2021

View reviewed changes

x-pack/plugins/security_solution/common/detection_engine/schemas/common/schemas.ts Show resolved Hide resolved

marshallmain commented Apr 8, 2021

View reviewed changes

madirey reviewed Apr 12, 2021

View reviewed changes

spong requested a review from a team April 12, 2021 15:51

Merge branch 'master' into one-schema-to-rule

6da57c1

marshallmain commented Apr 13, 2021

View reviewed changes

Remove dead code around findRulesSchema response

8b7c0f7

rylnd approved these changes Apr 14, 2021

View reviewed changes

marshallmain added 2 commits April 13, 2021 20:27

Normalize threshold field on routes that use legacy style schema defi…

411b0da

…nitions

Remove superfluous comments

e0c969c

Merge branch 'master' into one-schema-to-rule

5b28455

rylnd mentioned this pull request Apr 14, 2021

[Security Solution][Detections] ML Rules accept multiple ML Job IDs #97073

Merged

14 tasks

marshallmain added 3 commits April 13, 2021 22:35

Delete references to deleted schema

2c89b80

More descriptive function name: getResult to getAlertMock

306af37

Merge branch 'one-schema-to-rule' of github.com:marshallmain/kibana i…

c72fff5

…nto one-schema-to-rule

pmuellr approved these changes Apr 14, 2021

View reviewed changes

marshallmain merged commit ff7c533 into elastic:master Apr 14, 2021

kibanamachine mentioned this pull request Apr 14, 2021

[7.x] [Security Solution] Converge detection engine on single schema representation (#96186) #97152

Merged

banderror mentioned this pull request May 16, 2022

[Detections][Discuss] Detection engine tech debt/refactor opportunities #80792

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Security Solution] Converge detection engine on single schema representation #96186

[Security Solution] Converge detection engine on single schema representation #96186

marshallmain commented Apr 3, 2021 •

edited

Loading

marshallmain Apr 8, 2021 •

edited

Loading

marshallmain Apr 8, 2021

madirey Apr 12, 2021

madirey commented Apr 12, 2021

marshallmain commented Apr 12, 2021

marshallmain Apr 13, 2021

rylnd left a comment

rylnd Apr 14, 2021

rylnd Apr 14, 2021

marshallmain Apr 14, 2021

rylnd Apr 14, 2021

rylnd Apr 14, 2021

rylnd Apr 14, 2021

marshallmain Apr 14, 2021 •

edited

Loading

rylnd Apr 14, 2021

marshallmain Apr 14, 2021

rylnd Apr 14, 2021

marshallmain Apr 14, 2021

marshallmain commented Apr 14, 2021

kibanamachine commented Apr 14, 2021

pmuellr left a comment

kibanamachine commented Apr 14, 2021

		}: SingleBulkCreateParams): Promise<SingleBulkCreateResponse> => {
		const ruleParams = ruleSO.attributes.params;

[Security Solution] Converge detection engine on single schema representation #96186

[Security Solution] Converge detection engine on single schema representation #96186

Conversation

marshallmain commented Apr 3, 2021 • edited Loading

Summary

Checklist

For maintainers

marshallmain Apr 8, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

madirey commented Apr 12, 2021

marshallmain commented Apr 12, 2021

Choose a reason for hiding this comment

rylnd left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

marshallmain Apr 14, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

marshallmain commented Apr 14, 2021

kibanamachine commented Apr 14, 2021

💚 Build Succeeded

Metrics [docs]

Page load bundle

History

pmuellr left a comment

Choose a reason for hiding this comment

kibanamachine commented Apr 14, 2021

💚 Backport successful

marshallmain commented Apr 3, 2021 •

edited

Loading

marshallmain Apr 8, 2021 •

edited

Loading

marshallmain Apr 14, 2021 •

edited

Loading