[Security Solution][Detections] Set default indicator path to reduce friction with new filebeat modules #92081

rylnd · 2021-02-19T20:11:43Z

Summary

We were previously conflating the path to retrieve indicator fields with
the path to persist indicator fields, since they were the same value.

To reduce friction in use with the new filebeat modules, we've decided
to make the default source path threatintel.indicator. However, we still
want to persist to threat.indicator, so we add a new constant, here.

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
Unit or functional tests were updated or added to match the most common scenarios

For maintainers

This was checked for breaking API changes and was labeled appropriately

We were previously conflating the path to retrieve indicator fields with the path to persist indicator fields, since they were the same value. To reduce friction in use with the new filebeat modules, we've decided to make the default source path threatintel.indicator. However, we still want to persist to threat.indicator, so we add a new constant, here.

These tests were assuming a default path of threat.indicator. Since that is the ECS standard, we're not going to rewrite the tests but instead just add this rule override. In the future if the default changes, this parameter might be unnecessary.

elasticmachine · 2021-02-19T23:51:34Z

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

rylnd · 2021-02-22T01:55:48Z

@elasticmachine merge upstream

ecezalp

as a part of this change, DEFAULT_INDICATOR_PATH has been renamed to INDICATOR_DESTINATION_PATH, and a new DEFAULT_INDICATOR_SOURCE_PATH has been introduced, and tests have been updated accordingly. There is a few instances where threat.indicator is hardcoded, which could be replaced with the const INDICATOR_DESTINATION_PATH. Approved

Unit or functional tests were updated or added to match the most common scenarios

this doesn't exactly apply as there are no UI changes, it's safe to say that guidelines have not been violated

Any text added follows EUI's writing guidelines, uses sentence case text and includes i18n support

ecezalp · 2021-02-22T15:56:42Z

...urity_solution/server/lib/detection_engine/signals/threat_mapping/build_threat_enrichment.ts

@@ -52,7 +52,9 @@ export const buildThreatEnrichment = ({
    return threatResponse.hits.hits;
  };

-  const defaultedIndicatorPath = threatIndicatorPath ? threatIndicatorPath : DEFAULT_INDICATOR_PATH;
+  const defaultedIndicatorPath = threatIndicatorPath


this could be simplified as threatIndicatorPath || DEFAULT_INDICATOR_SOURCE_PATH

Good catch, thanks!

ecezalp · 2021-02-22T16:04:35Z

...tion/server/lib/detection_engine/signals/threat_mapping/enrich_signal_threat_matches.test.ts

@@ -94,7 +94,7 @@ describe('buildMatchedIndicator', () => {
    const indicators = buildMatchedIndicator({
      queries: [],
      threats,
-      indicatorPath: DEFAULT_INDICATOR_PATH,
+      indicatorPath: 'threat.indicator',


if the intention is to keep this explicit, no need for modification - although it looks like we could declare an indicatorPath variable and assign it in thebeforeEach block with the value threat.indicator or INDICATOR_DESTINATION_PATH so that we can use shorthand. In the same vein, it looks like there are a couple of tests that build the matched indicators in the same manner, so maybe it could make sense to declare a new var called defaultIndicators in the same beforeEach block where the defaultIndicators would be buildMatchedIndicator({queries, threats, indicatorPath})

Agreed on DRYing this up, I think I'll do that here 👍. The intention with hardcoding the path here here was to make the tests robust to default values changing; since defaulting happens outside of these functions I only want those integration/UI tests to change if those defaults do.

ecezalp · 2021-02-22T16:07:35Z

..._solution/server/lib/detection_engine/signals/threat_mapping/enrich_signal_threat_matches.ts

@@ -92,7 +91,7 @@ export const enrichSignalThreatMatches = async (
    if (!isObject(threat)) {
      throw new Error(`Expected threat field to be an object, but found: ${threat}`);
    }
-    const existingIndicatorValue = get(signalHit._source, DEFAULT_INDICATOR_PATH) ?? [];
+    const existingIndicatorValue = get(signalHit._source, 'threat.indicator') ?? [];


we could use the previously declared INDICATOR_DESTINATION_PATH here instead of the hardcoded value, as this is not a test file

I'm inclined to leave this hardcoded string for now, as the check above and the object generation below this line both make assumptions about this string in different ways.

rylnd · 2021-02-23T22:53:53Z

this doesn't exactly apply as there are no UI changes, it's safe to say that guidelines have not been violated

Any text added follows EUI's writing guidelines, uses sentence case text and includes i18n support

There was a minor copy change here where part of the field's description was deleted. Trivial, but I left this in there to call that out.

If/when that constant changes, I imagine this will be useful context.

…_path

kibanamachine · 2021-02-24T21:36:00Z

💛 Build succeeded, but was flaky

continuous-integration/kibana-ci/pull-request
Commit: d5c24d4
Storybooks Preview
Flaky suites:
- xpack-securitySolutionCypressChrome

Test Failures

Kibana Pipeline / general / Deletes and recovers more than one rule.Deleting prebuilt rules Deletes and recovers more than one rule

Link to Jenkins

Stack Trace

Failed Tests Reporter:
  - Test has not failed recently on tracked branches

AssertionError: Timed out retrying after 60000ms: expected '<button.euiButton.euiButton--primary>' to have text 'Install 2 Elastic prebuilt rules ', but the text was 'Install 1 Elastic prebuilt rule '
    at Context.eval (http://localhost:6151/__cypress/tests?p=cypress/integration/detection_rules/prebuilt_rules.spec.ts:20253:68)

Metrics [docs]

Async chunks

Total size of all lazy-loaded chunks that will be downloaded as the user navigates the app

id	before	after	diff
`securitySolution`	7.7MB	7.7MB	-57.0B

Page load bundle

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

id	before	after	diff
`securitySolution`	234.8KB	234.8KB	+71.0B

History

💚 Build #108244 succeeded c9ec203
💚 Build #108195 succeeded d35a8fc
💔 Build #108156 failed 7c90377

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

cc @rylnd

…friction with new filebeat modules (elastic#92081) * Distinguish source and destination config for indicator matches We were previously conflating the path to retrieve indicator fields with the path to persist indicator fields, since they were the same value. To reduce friction in use with the new filebeat modules, we've decided to make the default source path threatintel.indicator. However, we still want to persist to threat.indicator, so we add a new constant, here. * Update our integration tests following change of default These tests were assuming a default path of threat.indicator. Since that is the ECS standard, we're not going to rewrite the tests but instead just add this rule override. In the future if the default changes, this parameter might be unnecessary. * DRY up unit tests a bit * Add a note for future devs If/when that constant changes, I imagine this will be useful context. Co-authored-by: Kibana Machine <[email protected]>

…friction with new filebeat modules (#92081) (#92751) * Distinguish source and destination config for indicator matches We were previously conflating the path to retrieve indicator fields with the path to persist indicator fields, since they were the same value. To reduce friction in use with the new filebeat modules, we've decided to make the default source path threatintel.indicator. However, we still want to persist to threat.indicator, so we add a new constant, here. * Update our integration tests following change of default These tests were assuming a default path of threat.indicator. Since that is the ECS standard, we're not going to rewrite the tests but instead just add this rule override. In the future if the default changes, this parameter might be unnecessary. * DRY up unit tests a bit * Add a note for future devs If/when that constant changes, I imagine this will be useful context. Co-authored-by: Kibana Machine <[email protected]> Co-authored-by: Kibana Machine <[email protected]>

* master: (38 commits) Fixes Cypress flake by adding pipe, click, and should (elastic#92762) [Discover] Fix filtering selected sidebar fields (elastic#91828) [ML] Fixes positions of calendar arrow buttons in start datafeed modal (elastic#92625) [dev/build_ts_refs] check that commit in outDirs matches mergeBase (elastic#92513) add dep on `@kbn/config` so it is built first [Expressions] [Lens] Add id and copyMetaFrom arg to mapColumn fn + add configurable onError argument to math fn (elastic#90481) [Lens] Fix Workspace hidden when using Safari (elastic#92616) [Lens] Fixes vertical alignment validation messages (elastic#91878) forbid x-elastic-product-origin header in elasticsearch configuration (elastic#92359) [Security Solution][Detections] Set default indicator path to reduce friction with new filebeat modules (elastic#92081) [ILM][Accessibility] Added A11y test for ILM new policy form. (elastic#92570) [Security Solution][Exceptions] - Fixes exceptions builder UI where invalid values can cause overwrites of other values (elastic#90634) Automatically generated Api documentation (elastic#86232) Increase index pattern select limit to 1000 (elastic#92093) [core.logging] Add RewriteAppender for filtering LogMeta. (elastic#91492) [Security Solution][Detection Rules] Update prebuilt rule threats to match schema (elastic#92281) [Security Solutions][Detection Engine] Fixes bug with not being able to duplicate indicator matches (elastic#92565) [Dashboard] Export appropriate references from byValue panels (elastic#91567) [Upgrade Assistant] Align code between branches (elastic#91862) [Security Solution][Case] Fix alerts push (elastic#91638) ...

…friction with new filebeat modules (#92081) (#92752) * Distinguish source and destination config for indicator matches We were previously conflating the path to retrieve indicator fields with the path to persist indicator fields, since they were the same value. To reduce friction in use with the new filebeat modules, we've decided to make the default source path threatintel.indicator. However, we still want to persist to threat.indicator, so we add a new constant, here. * Update our integration tests following change of default These tests were assuming a default path of threat.indicator. Since that is the ECS standard, we're not going to rewrite the tests but instead just add this rule override. In the future if the default changes, this parameter might be unnecessary. * DRY up unit tests a bit * Add a note for future devs If/when that constant changes, I imagine this will be useful context. Co-authored-by: Kibana Machine <[email protected]> Co-authored-by: Kibana Machine <[email protected]>

rylnd added the Team: SecuritySolution Security Solutions Team working on SIEM, Endpoint, Timeline, Resolver, etc. label Feb 19, 2021

rylnd self-assigned this Feb 19, 2021

rylnd added v7.12.0 v8.0.0 release_note:skip Skip the PR/issue when compiling release notes labels Feb 19, 2021

rylnd changed the title ~~Distinguish source and destination config for indicator matches~~ [Security Solution][Detections] Set default indicator path to reduce friction with new filebeat modules Feb 19, 2021

rylnd marked this pull request as ready for review February 19, 2021 23:51

rylnd requested a review from a team as a code owner February 19, 2021 23:51

rylnd mentioned this pull request Feb 20, 2021

[DOCS] Updates Create Rule API docs for Threshold and Indicator Match Rules elastic/security-docs#522

Merged

Merge branch 'master' into default_indicator_path

c9ec203

ecezalp approved these changes Feb 22, 2021

View reviewed changes

rylnd added 3 commits February 24, 2021 13:03

DRY up unit tests a bit

40c280b

Add a note for future devs

4790415

If/when that constant changes, I imagine this will be useful context.

Merge remote-tracking branch 'upstream/master' into default_indicator…

d5c24d4

…_path

rylnd merged commit f67fef9 into elastic:master Feb 25, 2021

rylnd deleted the default_indicator_path branch February 25, 2021 02:45

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Security Solution][Detections] Set default indicator path to reduce friction with new filebeat modules #92081

[Security Solution][Detections] Set default indicator path to reduce friction with new filebeat modules #92081

rylnd commented Feb 19, 2021 •

edited by ecezalp

Loading

elasticmachine commented Feb 19, 2021

rylnd commented Feb 22, 2021

ecezalp left a comment

ecezalp Feb 22, 2021

rylnd Feb 23, 2021

ecezalp Feb 22, 2021

rylnd Feb 23, 2021

ecezalp Feb 22, 2021

rylnd Feb 24, 2021

rylnd commented Feb 23, 2021

kibanamachine commented Feb 24, 2021

Stack Trace

[Security Solution][Detections] Set default indicator path to reduce friction with new filebeat modules #92081

[Security Solution][Detections] Set default indicator path to reduce friction with new filebeat modules #92081

Conversation

rylnd commented Feb 19, 2021 • edited by ecezalp Loading

Summary

Checklist

For maintainers

elasticmachine commented Feb 19, 2021

rylnd commented Feb 22, 2021

ecezalp left a comment

Choose a reason for hiding this comment

ecezalp Feb 22, 2021

Choose a reason for hiding this comment

rylnd Feb 23, 2021

Choose a reason for hiding this comment

ecezalp Feb 22, 2021

Choose a reason for hiding this comment

rylnd Feb 23, 2021

Choose a reason for hiding this comment

ecezalp Feb 22, 2021

Choose a reason for hiding this comment

rylnd Feb 24, 2021

Choose a reason for hiding this comment

rylnd commented Feb 23, 2021

kibanamachine commented Feb 24, 2021

💛 Build succeeded, but was flaky

Test Failures

Stack Trace

Metrics [docs]

Async chunks

Page load bundle

History

rylnd commented Feb 19, 2021 •

edited by ecezalp

Loading