[Security Solution] Split rule executor by rule type and validate type specific rule params #94857

marshallmain · 2021-03-17T20:22:45Z

Summary

This PR is copy paste with some refactoring thrown in to support the copy/paste.

Refactor changes include:

added warning boolean flag to the "search after return type" that each executor returns. This was a single variable that was shared across all rule types before, so to split the rule executors it has to be returned as part of the result object.
Each executor gets a SavedObject that has already been validated to ensure it matches the schema for the rule type. The validation is non-exact, meaning if a rule has fields we don't expect then the validation still passes. However, validation fails if required fields are missing - e.g. if query is missing from a rule that requires a query, or if required threat_match fields are missing. This allows us to rely entirely on the type specific schemas for validation and remove if statements checking if certain required fields exist. For optional fields, e.g. timestamp override, of course we still have to check if the field exists when using it.
reduced the number of places we cast filters as Filter[] from unknown. Instead, filters and threatFilters are typed as unknown until we have to cast them as params to buildEsQuery since we don't have validation for filters yet.

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

marshallmain · 2021-03-17T20:44:52Z

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

+    );
+    logger.warn(errorMessage);
+    result.warning = true;
+    // TODO: change this to partialFailure since we don't immediately exit rule function and still do actions at the end?


This is a new TODO: noticed this behavior when adding the warning logic to result. I think this error was added before we created partial errors, would it fit better as a partial error?

marshallmain · 2021-03-17T20:46:07Z

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

+    tags: rule.attributes.tags,
+    buildRuleMessage,
+  });
+  // The legacy ES client does not define failures when it can be present on the structure, hence why I have the & { failures: [] }


Now that we use the new ES client, we can revisit this

This reverts commit b1dd59e.

marshallmain · 2021-03-23T15:27:00Z

@elasticmachine merge upstream

elasticmachine · 2021-03-23T22:02:19Z

Pinging @elastic/security-solution (Team: SecuritySolution)

elasticmachine · 2021-03-23T22:02:19Z

Pinging @elastic/security-detections-response (Team:Detections and Resp)

dhurley14 · 2021-03-24T19:29:48Z

...ecurity_solution/server/lib/detection_engine/signals/threat_mapping/create_threat_signals.ts

  let results: SearchAfterAndBulkCreateReturnType = {
    success: true,
+    warning: false,
    bulkCreateTimes: [],
    searchAfterTimes: [],
    lastLookBackDate: null,


Could we initialize this with the same createSearchAfterReturnType() function used in the other executors?

We can, I just didn't do it in this PR to limit scope creep

kibanamachine · 2021-03-25T17:03:35Z

💚 Build Succeeded

Metrics [docs]

✅ unchanged

History

💛 Build #114988 was flaky 0ccaddc
💔 Build #114614 failed 004491b
💔 Build #114477 failed e6bf4fc
💔 Build #114458 failed 4aff9f7
💔 Build #114454 failed 63b0640

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

yctercero

LGTM! I'm not sure I saw why we needed to change the typed partial filters to unknown but that was about it. I pulled down and created different rules to test out that it's all still as expected. That executor file is looking so much better! Thanks for all the work!

yctercero · 2021-03-26T18:36:27Z

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

@@ -22,7 +22,7 @@ import { Query, Language, Index, TimestampOverrideOrUndefined } from './schemas/
 export const getQueryFilter = (
  query: Query,
  language: Language,
-  filters: Array<Partial<Filter>>,
+  filters: unknown,


Why do we turn this into an unknown? It seems that everywhere we're using it we're passing in filters typed as Filter[].

The source of the problem here is that we don't have validation for the Filter type on the server side yet, so in attempting to make the server types more accurate the type of filters in RuleTypeParams became unknown. Then since we had a cast in here on line 51 already I thought we could just do the unsafe cast once at the end instead of passing in a partial then casting to unknown and then to Filter[].

I think you're right though that we should pass in Filter[] and function callers should be responsible for casting the parameter if necessary. As part of the follow up updating server types I'll revisit this - want to get this merged before it conflicts with other work.

yctercero · 2021-03-26T18:57:44Z

x-pack/plugins/security_solution/server/lib/detection_engine/schemas/rule_schemas.ts

@@ -153,12 +161,19 @@ const thresholdSpecificRuleParams = t.type({
  savedId: savedIdOrUndefined,
  threshold,
 });
+export const thresholdRuleParams = t.intersection([baseRuleParams, thresholdSpecificRuleParams]);


I like this! Reads easy

yctercero · 2021-03-26T19:28:30Z

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

+  } catch (err) {
+    if (err.statusCode === 403) {
+      throw new Error(
+        `EQL based rules require the user that created it to have the view_index_metadata, read, and write permissions for index: ${ruleParams.outputIndex}`


Not sure if this is the place to do it. But would it be possible to guide our users more with our error messages. Like where to view these permissions for example.

yctercero · 2021-03-26T19:29:28Z

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

+    ruleParams.eventCategoryOverride
+  );
+  const eqlSignalSearchStart = performance.now();
+  // TODO: fix this later


Is this fix referring to the as?

ah yeah this is referring to the use of transport.request still, which was necessary with the legacy ES client because the EQL apis weren't in it - but the EQL api is in the new client, so we can switch over.

yctercero · 2021-03-26T19:30:52Z

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

+  const eqlSignalSearchEnd = performance.now();
+  const eqlSearchDuration = makeFloatString(eqlSignalSearchEnd - eqlSignalSearchStart);
+  result.searchAfterTimes = [eqlSearchDuration];
+  let newSignals: WrappedSignalHit[] | undefined;


Might be cleaner to break this out into a function we can test and also then remove the let just a const newSignals = determineNewSignals()

Good idea, that would be cleaner 👍 will do in follow up

…e specific rule params (elastic#94857) * Split rule executors into different files * Pass type-specific rule SOs to rule executor functions * Genericize function to narrow ruleSO type * Remove undefined return type from getExceptions * Remove unintentional change to SIGNALS_TEMPLATE_VERSION * Remove extra validation now covered by schemas * Remove extra validation from ML rule executor * Fix types * syncs schemas * Revert "syncs schemas" This reverts commit b1dd59e. * Fix api test and move threshold executor test * kinda adds eql test * Refactor and fix unit tests * fixes marshalls mistake Co-authored-by: Davis Plumlee <davis.plumlee@elastic.co> Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>

kibanamachine · 2021-03-27T02:51:56Z

💚 Backport successful

✅ 7.x / #95607

This backport PR will be merged automatically after passing CI.

…e specific rule params (#94857) (#95607) * Split rule executors into different files * Pass type-specific rule SOs to rule executor functions * Genericize function to narrow ruleSO type * Remove undefined return type from getExceptions * Remove unintentional change to SIGNALS_TEMPLATE_VERSION * Remove extra validation now covered by schemas * Remove extra validation from ML rule executor * Fix types * syncs schemas * Revert "syncs schemas" This reverts commit b1dd59e. * Fix api test and move threshold executor test * kinda adds eql test * Refactor and fix unit tests * fixes marshalls mistake Co-authored-by: Davis Plumlee <davis.plumlee@elastic.co> Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com> Co-authored-by: Marshall Main <55718608+marshallmain@users.noreply.github.com> Co-authored-by: Davis Plumlee <davis.plumlee@elastic.co>

marshallmain added 6 commits March 17, 2021 13:18

Split rule executors into different files

4eb00c3

Pass type-specific rule SOs to rule executor functions

4ed40a6

Genericize function to narrow ruleSO type

c426433

Remove undefined return type from getExceptions

2d2b4ba

Remove unintentional change to SIGNALS_TEMPLATE_VERSION

93b3780

Remove extra validation now covered by schemas

254e254

marshallmain commented Mar 17, 2021

View reviewed changes

marshallmain and others added 7 commits March 17, 2021 14:01

Remove extra validation from ML rule executor

118cbd3

Fix types

8bc5c4e

syncs schemas

b1dd59e

Revert "syncs schemas"

652ff47

This reverts commit b1dd59e.

Fix api test and move threshold executor test

63b0640

kinda adds eql test

4aff9f7

Refactor and fix unit tests

e6bf4fc

Merge branch 'master' into split-rule-executors-rebased

004491b

marshallmain marked this pull request as ready for review March 23, 2021 22:00

marshallmain requested a review from a team as a code owner March 23, 2021 22:00

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 Mar 23, 2021

fixes marshalls mistake

0ccaddc

dplumlee mentioned this pull request Mar 24, 2021

[Security Solution][Detections][Meta] Modularize the Detection Engine #93550

Closed

13 tasks

dhurley14 reviewed Mar 24, 2021

View reviewed changes

Merge master into split executors branch

d92a043

yctercero approved these changes Mar 26, 2021

View reviewed changes

marshallmain merged commit 533a7bb into elastic:master Mar 27, 2021

marshallmain added the auto-backport Deprecated - use backport:version if exact versions are needed label Mar 27, 2021

kibanamachine mentioned this pull request Mar 27, 2021

[7.x] [Security Solution] Split rule executor by rule type and validate type specific rule params (#94857) #95607

Merged

marshallmain deleted the split-rule-executors-rebased branch March 27, 2021 03:04

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Security Solution] Split rule executor by rule type and validate type specific rule params #94857

[Security Solution] Split rule executor by rule type and validate type specific rule params #94857

marshallmain commented Mar 17, 2021 •

edited

Loading

marshallmain Mar 17, 2021

marshallmain Mar 17, 2021

marshallmain commented Mar 23, 2021

elasticmachine commented Mar 23, 2021

elasticmachine commented Mar 23, 2021

dhurley14 Mar 24, 2021

marshallmain Mar 24, 2021

kibanamachine commented Mar 25, 2021

yctercero left a comment

yctercero Mar 26, 2021

marshallmain Mar 27, 2021 •

edited

Loading

yctercero Mar 26, 2021

yctercero Mar 26, 2021

yctercero Mar 26, 2021

marshallmain Mar 27, 2021

yctercero Mar 26, 2021

marshallmain Mar 27, 2021

kibanamachine commented Mar 27, 2021

[Security Solution] Split rule executor by rule type and validate type specific rule params #94857

[Security Solution] Split rule executor by rule type and validate type specific rule params #94857

Conversation

marshallmain commented Mar 17, 2021 • edited Loading

Summary

Checklist

For maintainers

Choose a reason for hiding this comment

Choose a reason for hiding this comment

marshallmain commented Mar 23, 2021

elasticmachine commented Mar 23, 2021

elasticmachine commented Mar 23, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kibanamachine commented Mar 25, 2021

💚 Build Succeeded

Metrics [docs]

History

yctercero left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

marshallmain Mar 27, 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kibanamachine commented Mar 27, 2021

💚 Backport successful

marshallmain commented Mar 17, 2021 •

edited

Loading

marshallmain Mar 27, 2021 •

edited

Loading