Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

[Security Solution] Populates threat.indicator.event with _source.event (#951) #95697

Merged
merged 3 commits into from
Mar 31, 2021
Merged

[Security Solution] Populates threat.indicator.event with _source.event (#951) #95697

merged 3 commits into from
Mar 31, 2021

Conversation

ecezalp
Copy link
Contributor

@ecezalp ecezalp commented Mar 29, 2021 •
edited
Loading

Summary

This change copies all event details from the source event to the alert under threat.indicator.event.

Closes elastic/security-team#951.
Relates to elastic/security-team#946.

Images

Screen Shot 2021-03-29 at 3 34 07 PM

Notes

I am not entirely sure if the changes to x-pack/plugins/security_solution/server/lib/detection_engine/routes/index/indicator_mapping.json and x-pack/plugins/security_solution/server/lib/detection_engine/routes/index/get_signals_template.ts were necessary, please let me know if they should be reverted to their original state.

Checklist

Delete any items that are not applicable to this PR.

@ecezalp ecezalp added release_note:feature Makes this part of the condensed release notes 7.13 candidate labels Mar 29, 2021
@ecezalp ecezalp requested a review from rylnd March 29, 2021 19:40
@ecezalp ecezalp requested a review from a team as a code owner March 29, 2021 19:40
@ecezalp ecezalp self-assigned this Mar 29, 2021
@ecezalp ecezalp added v7.13.0 Team: SecuritySolution Security Solutions Team working on SIEM, Endpoint, Timeline, Resolver, etc. labels Mar 29, 2021
@elasticmachine
Copy link
Contributor

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

rylnd
rylnd requested changes Mar 29, 2021
View reviewed changes
Copy link
Contributor

@rylnd rylnd left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks great, just a small change to the mappings and this should be good to go.

It would be great if you could add/update an integration test to account for this new behavior, as well.

...k/plugins/security_solution/server/lib/detection_engine/routes/index/get_signals_template.ts Outdated
...ecsMapping.mappings.properties.threat,
properties: {
...ecsMapping.mappings.properties.threat.properties,
...indicatorMapping,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think this is correct; we want these to be part of the nested threat.indicator objects, e.g. threat.indicator[].event.*, and this looks to generate threat.event.* mappings.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hey thanks for the catch

..._solution/server/lib/detection_engine/signals/threat_mapping/enrich_signal_threat_matches.ts
@@ -57,9 +57,11 @@ export const buildMatchedIndicator = ({
}
const atomic = get(matchedThreat?._source, query.value) as unknown;
const type = get(indicator, 'type') as unknown;
const event = get(matchedThreat?._source, 'event') as unknown;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 this logic is correct, it's just the mappings that are off.

@ecezalp
Copy link
Contributor Author

ecezalp commented Mar 30, 2021 •
edited
Loading

latest changes

  • fixed mappings
  • (cypress) updated JSON with added event data in x-pack/plugins/security_solution/cypress/integration/detection_rules/indicator_match_rule.spec.ts
  • (ftr) updated tests in x-pack/test/detection_engine_api_integration/security_and_spaces/tests/create_threat_matching.ts

@ecezalp ecezalp requested a review from rylnd March 30, 2021 22:09
rylnd
rylnd approved these changes Mar 31, 2021
View reviewed changes
Copy link
Contributor

@rylnd rylnd left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great! Glad we have those integration and cypress tests for coverage here, and appreciate the added unit tests as well 👍

...k/plugins/security_solution/server/lib/detection_engine/routes/index/get_signals_template.ts Outdated
@@ -7,6 +7,7 @@

import signalsMapping from './signals_mapping.json';
import ecsMapping from './ecs_mapping.json';
import indicatorMapping from './indicator_mapping.json';
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since this file is just currently "ECS event mappings," what do you think about using ecsMapping.mappings.properties.event instead of this new file? I expect we'll want these indicator mappings to be in sync with the latest from ECS, so that might be the best way to accomplish this.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

excellent point!

...tion/server/lib/detection_engine/signals/threat_mapping/enrich_signal_threat_matches.test.ts
@@ -117,6 +118,16 @@ describe('buildMatchedIndicator', () => {
expect(get(indicator, 'matched.atomic')).toEqual('domain_1');
});

it('returns event values as a part of threat', () => {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

@kibanamachine
Copy link
Contributor

💚 Build Succeeded

Metrics [docs]

✅ unchanged

History

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

cc @ecezalp

@ecezalp ecezalp merged commit 178c2de into elastic:master Mar 31, 2021
@ecezalp ecezalp deleted the indicator-event branch March 31, 2021 15:37
ecezalp added a commit to ecezalp/kibana that referenced this pull request Mar 31, 2021
@ecezalp
[Security Solution] Populates threat.indicator.event with _source.eve…
1457a60 
…nt (elastic#951) (elastic#95697)

* [Security Solution] Add event data to threat.indicator (elastic/security_team/elastic#951)

* fixes mappings, updates tests

* refactor mappings
@ecezalp ecezalp mentioned this pull request Mar 31, 2021
Merged
ecezalp added a commit that referenced this pull request Mar 31, 2021
@ecezalp
[Security Solution] Populates threat.indicator.event with _source.eve…
49da182 
…nt (#951) (#95697) (#95955)

* [Security Solution] Add event data to threat.indicator (elastic/security_team/#951)

* fixes mappings, updates tests

* refactor mappings
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@rylnd rylnd rylnd approved these changes

Assignees

@ecezalp ecezalp

Labels
release_note:feature Makes this part of the condensed release notes Team: SecuritySolution Security Solutions Team working on SIEM, Endpoint, Timeline, Resolver, etc. v7.13.0
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@ecezalp @elasticmachine @kibanamachine @rylnd