[Security Solution][Detections] Support arrays in event fields for Severity/Risk overrides #83723

banderror · 2020-11-18T21:23:18Z

Addresses: #82384

Summary

This PR changes the behavior of severity and risk score overrides in two ways:

adds support for arrays in the mapped event fields (so a rule can be triggered by an event where e.g. event.custom_severity has a value like [45, 70, 90])
makes the logic of overrides more flexible, resilient to the incoming values (filters out junk, extracts meaningful values, does its best to find a value that would fit the mapping)

TODO:

Risc score mapping: support arrays
Risc score mapping: support arbitrary floats within the range [0;100] according to ECS
Severity: support arrays
Severity mapping: support numbers and strings in arbitrary (non-ECS) fields
Add unit tests
Add FTR integration tests for BE API

Useful info

You might find useful some background info on severity and risk score overrides, as well as fields from ECS corresponding to them.

Docs on creating rules.
Spec for ECS fields, look for
- event.risk_score
- event.risk_score_norm
- event.severity

Testing

Frank clearly described how to reproduce the bug in the parent issue: #82384
This instruction can be used to do manual QA and make sure the bug is fixed.

Also, I added a few FTR integration tests for the overrides:

x-pack/test/detection_engine_api_integration/security_and_spaces/tests/generating_signals.ts
x-pack/test/functional/es_archives/signals/severity_risk_overrides

You can run them with:

node scripts/functional_tests --config x-pack/test/detection_engine_api_integration/security_and_spaces/config.ts --include x-pack/test/detection_engine_api_integration/security_and_spaces/tests/generating_signals.ts

Tests implementation is based on #83764.

Checklist

Delete any items that are not applicable to this PR.

Documentation was added for features that require explanation or tutorials
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

rylnd

Did a cursory code review; most of my comments are nits, FYI.

The additional logic looks good to me, but I'm not going to explicitly approve because I haven't touched/am not familiar with this code. I'm happy to do another pass here when this is out of draft, though!

rylnd · 2020-11-23T18:43:38Z

..._solution/server/lib/detection_engine/signals/mappings/build_risk_score_from_mapping.test.ts

+  expected: BuildRiskScoreFromMappingReturn;
+}
+
+function testIt({ fieldValue, scoreDefault, scoreMapping, expected }: TestCase) {


nit: we tend to prefer arrow functions

nit: I would personally do away with the testIt function as I find it makes the tests a bit too convoluted, but that's totally subjective.

Thanks, I briefly went through these docs:

https://www.elastic.co/guide/en/kibana/master/pr-review.html

https://github.com/elastic/kibana/blob/master/STYLEGUIDE.md

https://github.com/elastic/kibana/blob/master/TYPESCRIPT.md

On functions, arrows vs fn expressions vs fn declarations. Normally, unless there's a different coding standard/guideline, to vertically structure the code I prefer using the "newspaper metaphor" coined by Uncle Bob in his Clean Code book.

Newspaper Metaphor. The source file should be organized like a newspaper article, with the highest level summary at the top, and more and more details further down. Functions called from the top function come directly below it, and so on down to the lowest level and most detailed functions at the bottom. This is a good way to organize the source code, even though IDE:s make the location of functions less important, since it is so easy to navigate in and out of them.

It's a top-down approach, from general things to details, and it's possible with function declarations. With arrows and fn expressions it's possible to use the opposite bottow-up approach, where you first have details, helper functions etc, and at the bottom of the file there's let's say the main function.

Normally I use arrows in all other cases: inline calls, closures to an outer this, just a bunch of small independent functions in a file (e.g. selectors), etc.

Of course I wouldn't go against our guidelines, whether they are documented or not, and against the typical style of the codebase. For the sake of consistency.

Hopefully I managed to explain my motivation. Let me know what you think. Although the styleguide uses fn declarations, it's a general one. If this style looks too foreign for the security_solution plugin's codebase, I'll change it to the one based on arrows.

On the testIt helper. Yea, it definitely has pros and cons. It reduces duplication and makes the test suite shorter, but abstracts away some details. One can perceive it as either good or bad thing in terms of readability and easiness to understand the tests. I personally find that as soon as you got what testIt does, it's easier and faster to read all the tests. On the other hand, it can be just a bit harder to start reading the tests once you opened the file.

If you don't have tests written in such a way and/or don't like it (yes, even subjectively), I'll refactor. But maybe there are people who are ok with that?

Any opinions, clarifications, comments or just votes are super-welcome. This helps me a lot with learning the codebase faster.

"Newspaper metaphor" with fn declarations?

testIt helper?

rylnd · 2020-11-23T18:56:50Z

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

+  return doc;
+};
+
+export const sampleDocRiskScore = (riskScore?: unknown): SignalSourceHit => ({


Nit: we're starting to add a lot of indirection to these tests, and I question the utility of such a method over something like:

const testEventWithDescriptiveName = { ...buildSpecificEventType(), [fieldName]: overriddenValue, };

For context: my perception has been that we typically prefer repetitive, inline test logic as opposed to shared helpers, with the exceptions of (normalized) data generation and ad hoc test helpers within the test file.

Discussed with @rylnd. We talked about creating test data factories and all that patterns in general, agreed on a few ideas, but no action required for this PR.

rylnd · 2020-11-23T18:59:06Z

...urity_solution/server/lib/detection_engine/signals/mappings/build_risk_score_from_mapping.ts

+}
+
+function getMaxOf(array: number[]) {
+  // NOTE: It's safer to use reduce rather than Math.max(...array). The latter won't handle large input.


..._solution/server/lib/detection_engine/signals/mappings/build_risk_score_from_mapping.test.ts

...urity_solution/server/lib/detection_engine/signals/mappings/build_risk_score_from_mapping.ts

...ecurity_solution/server/lib/detection_engine/signals/mappings/build_severity_from_mapping.ts

elasticmachine · 2020-11-25T16:08:34Z

Pinging @elastic/siem (Team:SIEM)

…[0;100] according to ECS

…fields

kibanamachine · 2020-11-30T11:56:17Z

💚 Build Succeeded

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

Metrics [docs]

✅ unchanged

History

💚 Build #90498 succeeded 713134b
💚 Build #90270 succeeded 22b227f
💔 Build #89939 failed bbb3f75
💚 Build #89586 succeeded 56f0f7f
💚 Build #89475 succeeded 80503d9

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

peluja1012

LGTM. Tested functionality with various severity and risk score combinations, including array values, and the generated signals respected the mappings and yielded expected results. Thanks for the fix and for the added FTR test coverage as well!

banderror · 2020-12-01T13:11:18Z

Just for the record, here's the data I used to test the fix manually:

###### Testing mapping as described in the GitHub issue (slightly extended)

PUT delme-test-overrides
{
  "mappings": {
    "dynamic": "strict",
    "properties": {
      "@timestamp": {
        "type": "date"
      },
      "severity_override": {
        "type": "keyword"
      },
      "risk_override": {
        "type": "integer"
      }
    }
  }
}

PUT delme-test-overrides/_doc/1
{
 "@timestamp": "2020-11-24T13:00:01.000Z",
 "severity_override" : "sev_90",
 "risk_override": 3.14
}

PUT delme-test-overrides/_doc/2
{
 "@timestamp": "2020-11-24T13:00:02.000Z",
 "severity_override": ["sev_90", "sev_max"],
 "risk_override": [3.14]
}

PUT delme-test-overrides/_doc/3
{
 "@timestamp": "2020-11-24T13:00:03.000Z",
 "severity_override": ["sev_max", "sev_90"],
 "risk_override": "3.14"
}

PUT delme-test-overrides/_doc/4
{
 "@timestamp": "2020-11-24T13:00:04.000Z",
 "severity_override": "sev_max",
 "risk_override": [1.23, "3.14"]
}

PUT delme-test-overrides/_doc/5
{
 "@timestamp": "2020-11-24T13:00:05.000Z",
 "severity_override": 99
}


###### Testing mapping to integer non-ECS severity field

PUT delme-test-overrides-sev-int
{
  "mappings": {
    "dynamic": "strict",
    "properties": {
      "@timestamp": {
        "type": "date"
      },
      "severity_override": {
        "type": "long"
      }
    }
  }
}

PUT delme-test-overrides-sev-int/_doc/1
{
 "@timestamp": "2020-11-29T13:00:01.000Z",
 "severity_override": 60
}

PUT delme-test-overrides-sev-int/_doc/2
{
 "@timestamp": "2020-11-29T13:00:02.000Z",
 "severity_override": 70
}

PUT delme-test-overrides-sev-int/_doc/3
{
 "@timestamp": "2020-11-29T13:00:03.000Z",
 "severity_override": 80
}

PUT delme-test-overrides-sev-int/_doc/4
{
 "@timestamp": "2020-11-29T13:00:04.000Z",
 "severity_override": 90
}


###### Testing mapping to integer event.severity field from ECS

PUT delme-test-overrides-sev-ecs
{
  "mappings": {
    "dynamic": "strict",
    "properties": {
      "@timestamp": {
        "type": "date"
      },
      "event": {
        "properties": {
          "severity": {
            "type": "long"
          }
        }
      }
    }
  }
}

PUT delme-test-overrides-sev-ecs/_doc/1
{
 "@timestamp": "2020-11-30T13:00:01.000Z",
 "event": {
   "severity": 60
 }
}

PUT delme-test-overrides-sev-ecs/_doc/2
{
 "@timestamp": "2020-11-30T13:00:02.000Z",
 "event": {
   "severity": 70
 }
}

PUT delme-test-overrides-sev-ecs/_doc/3
{
 "@timestamp": "2020-11-30T13:00:03.000Z",
 "event": {
   "severity": 80
 }
}

PUT delme-test-overrides-sev-ecs/_doc/4
{
 "@timestamp": "2020-11-30T13:00:04.000Z",
 "event": {
   "severity": 90
 }
}


###### Testing mapping support in EQL rules

PUT delme-test-overrides-sev-eql
{
  "mappings": {
    "dynamic": "strict",
    "properties": {
      "@timestamp": {
        "type": "date"
      },
      "event": {
        "properties": {
          "category": {
            "type": "keyword"
          }
        }
      },
      "params": {
        "properties": {
          "severity_override": {
            "type": "keyword"
          }
        }
      }
    }
  }
}

PUT delme-test-overrides-sev-eql/_doc/1
{
 "@timestamp": "2020-12-01T08:00:01.000Z",
 "event": {
   "category": "authentication"
 },
 "params": {
   "severity_override": "sev_90"
 }
}

PUT delme-test-overrides-sev-eql/_doc/2
{
 "@timestamp": "2020-12-01T08:00:02.000Z",
 "event": {
   "category": "authentication"
 },
 "params": {
   "severity_override": "sev_max"
 }
}


#############################

GET _cat/shards/delme-test-overrides*?v&s=shard,prirep

GET delme-test-overrides/_search
GET delme-test-overrides-sev-int/_search
GET delme-test-overrides-sev-ecs/_search
GET delme-test-overrides-sev-eql/_search

DELETE delme-test-overrides*

…verity/Risk overrides (elastic#83723) This PR changes the behavior of severity and risk score overrides in two ways: - adds support for arrays in the mapped event fields (so a rule can be triggered by an event where e.g. `event.custom_severity` has a value like `[45, 70, 90]`) - makes the logic of overrides more flexible, resilient to the incoming values (filters out junk, extracts meaningful values, does its best to find a value that would fit the mapping)

…verity/Risk overrides (#83723) (#84643) This PR changes the behavior of severity and risk score overrides in two ways: - adds support for arrays in the mapped event fields (so a rule can be triggered by an event where e.g. `event.custom_severity` has a value like `[45, 70, 90]`) - makes the logic of overrides more flexible, resilient to the incoming values (filters out junk, extracts meaningful values, does its best to find a value that would fit the mapping)

* master: (63 commits) Revert the Revert of "[Alerting] renames Resolved action group to Recovered (elastic#84123)" (elastic#84662) declare kbn/monaco dependency on kbn/i18n explicitly (elastic#84660) Remove unscripted fields from sample data index-pattern saved objects (elastic#84659) [ML] Fix unnecessary trigger of wildcard field type search for ML plugin routes. (elastic#84605) Update create.asciidoc (elastic#84046) [Security Solution][Detections] Fix labels and issue with mandatory fields (elastic#84525) Fix flaky test suite (elastic#84602) [Security Solution] [Detections] Create a 'partial failure' status for rules (elastic#84293) Revert "[Alerting] renames Resolved action group to Recovered (elastic#84123)" Update code-comments describing babel plugins (elastic#84622) [Security Solution] [Cases] Cypress for case connector selector options (elastic#80745) [Discover] Unskip doc table tests (elastic#84564) [Lens] (Accessibility) Improve landmarks in Lens (elastic#84511) [Lens] (Accessibility) Focus mistakenly stops on righthand form (elastic#84519) Return early when parallel install process detected (elastic#84190) [Security Solution][Detections] Support arrays in event fields for Severity/Risk overrides (elastic#83723) [Security Solution][Detections] Fix grammatical error in validation message for threshold field in "Create new rule" -> "Define rule" (elastic#84490) [Fleet] Update agent details page (elastic#84434) adding documentation of use of NODE_EXTRA_CA_CERTS env var (elastic#84578) [Search] Integrate "Send to background" UI with session service (elastic#83073) ...

banderror force-pushed the severity-risk-overrides-arrays branch 4 times, most recently from 568f46c to 75ad0ca Compare November 19, 2020 18:50

peluja1012 added Team:SIEM Team:Detections and Resp Security Detection Response Team labels Nov 20, 2020

banderror force-pushed the severity-risk-overrides-arrays branch 3 times, most recently from 7c96731 to 80503d9 Compare November 22, 2020 15:15

banderror requested review from spong and peluja1012 November 22, 2020 15:16

banderror self-assigned this Nov 22, 2020

banderror force-pushed the severity-risk-overrides-arrays branch from 80503d9 to 56f0f7f Compare November 23, 2020 12:38

spong requested a review from a team November 23, 2020 17:57

rylnd reviewed Nov 23, 2020

View reviewed changes

banderror force-pushed the severity-risk-overrides-arrays branch from bbb3f75 to 22b227f Compare November 25, 2020 16:00

banderror changed the title ~~[Security Solution] Severity/Risk override does not work with arrays~~ [Security Solution][Detections] Support arrays in event fields for Severity/Risk overrides Nov 25, 2020

banderror added Team: SecuritySolution Security Solutions Team working on SIEM, Endpoint, Timeline, Resolver, etc. release_note:fix v8.0.0 v7.11.0 labels Nov 25, 2020

banderror marked this pull request as ready for review November 25, 2020 16:08

banderror requested review from a team as code owners November 25, 2020 16:08

banderror added the Feature:Detection Rules Anything related to Security Solution's Detection Rules label Nov 25, 2020

banderror force-pushed the severity-risk-overrides-arrays branch from 22b227f to 713134b Compare November 26, 2020 19:12

banderror requested a review from a team November 26, 2020 19:31

banderror added 2 commits November 30, 2020 11:16

Risc score mapping: support arrays and cover with tests

7f366ba

Severity mapping: support arrays and cover with tests

220b291

banderror added 4 commits November 30, 2020 11:16

Risk score mapping: support arbitrary float numbers within the range …

3d840b8

…[0;100] according to ECS

Severity mapping: support numbers and strings in arbitrary (non-ECS) …

9dbee12

…fields

Address comments of @rylnd

c47a47d

Add FTR integration tests

153063c

banderror force-pushed the severity-risk-overrides-arrays branch from 713134b to 153063c Compare November 30, 2020 10:17

banderror requested a review from rylnd November 30, 2020 10:36

peluja1012 approved these changes Dec 1, 2020

View reviewed changes

banderror merged commit 80e88ce into elastic:master Dec 1, 2020

This was referenced Dec 1, 2020

[7.x] [Security Solution][Detections] Support arrays in event fields for Severity/Risk overrides (#83723) #84643

Merged

[Security Solution] Severity/Risk override does not work with arrays #82384

Closed

banderror deleted the severity-risk-overrides-arrays branch December 1, 2020 14:06

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Security Solution][Detections] Support arrays in event fields for Severity/Risk overrides #83723

[Security Solution][Detections] Support arrays in event fields for Severity/Risk overrides #83723

banderror commented Nov 18, 2020 •

edited

Loading

rylnd left a comment

rylnd Nov 23, 2020

rylnd Nov 23, 2020

banderror Nov 24, 2020

banderror Nov 24, 2020

banderror Nov 24, 2020 •

edited

Loading

banderror Nov 24, 2020

banderror Nov 24, 2020

banderror Nov 24, 2020

rylnd Nov 23, 2020

banderror Nov 24, 2020

rylnd Nov 23, 2020

elasticmachine commented Nov 25, 2020

kibanamachine commented Nov 30, 2020

peluja1012 left a comment

banderror commented Dec 1, 2020

[Security Solution][Detections] Support arrays in event fields for Severity/Risk overrides #83723

[Security Solution][Detections] Support arrays in event fields for Severity/Risk overrides #83723

Conversation

banderror commented Nov 18, 2020 • edited Loading

Summary

Useful info

Testing

Checklist

For maintainers

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

banderror Nov 24, 2020 • 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

elasticmachine commented Nov 25, 2020

kibanamachine commented Nov 30, 2020

💚 Build Succeeded

Metrics [docs]

History

peluja1012 left a comment

Choose a reason for hiding this comment

banderror commented Dec 1, 2020

banderror commented Nov 18, 2020 •

edited

Loading

banderror Nov 24, 2020 •

edited

Loading