-
Notifications
You must be signed in to change notification settings - Fork 8.2k
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.
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
[Security Solution][Detections]Update detection alert mappings to ECS 1.9 #97573
Conversation
* Adds a wrapping "mappings.properties" around our extra mappings * Spreads our other mappings similarly to ECS mappings * Moves dynamic: false out of ECS mappings and into our main template * Ensures we include 'threat.properties.indicator', since that's where our 'type: nested' declaration resides
This updated snapshot reflects the mappings changes that one will receive when migrating/rolling over to a 7.13 alerts index.
The last released mappings update was elastic#92928, which bumped from 24 -> 25. The few unreleased updates since then have increased this by 1, but since these changes are going out with 7.13 we are bumping by 10 _since the last release_, in order to give "room" for minor releases.
Pinging @elastic/security-solution (Team: SecuritySolution) |
Pinging @elastic/security-detections-response (Team:Detections and Resp) |
@MikePaquette If you missed it above, you can see the full mappings changes (which are purely additive) here |
"mappings": { | ||
"dynamic": false, | ||
"_meta": { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I opted to leave these top-level fields because:
- we're not using anything but
.mappings.properties
- it makes ECS updates easier as it's just a copy/paste from the generated mappings template.json
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
and if we did accidentally start using them, the snapshot test would tell us so 😉
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM! Looks great. Thanks for the updating of the mappings
@@ -1,12 +1,37 @@ | |||
{ | |||
"index_patterns": [ | |||
"try-ecs-*" | |||
], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This seems weird, but if you're fine with it. I don't think that is going to mess things up, but it's odd to see it in the mapping file like a template would have them.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see the comment below, I am good with this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I went back and forth on this; the main motivation here was that it's easy to copy/paste the generated ECS mappings into this file, and the _meta here also gives readers some indication of where the mappings came from.
We have the snapshot test to ensure that we're only pulling what we want/need from this for now, and RAC should make most of this code unnecessary in the near future 👍
This magic number represents "the number of mapped fields that begin with 'host.geo.c' and, because this PR adds a mapping for host.geo.continent_code, the test needed to be updated.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like the snapshot idea, I think having a single place to compare the full mapping is very valuable especially now that we've added more different components that get combined. It's like an integration test for the mapping so we can change implementation behind how we build the mapping and verify that the output hasn't subtly changed.
}, | ||
"threat": { | ||
"properties": { | ||
"indicator": { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How are the experimental fields included here chosen? It looks like there are a couple fieldsets in the experimental generated JSON for threat.indicator that don't show up here, notably threat.indicator.file.*
, .hash.*
, and .registry.*
- are those purposely left out?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
CTI has, to date, only been pulling in what's needed in order to minimize the likelihood of incompatible mappings in the case where the experimental fields change. However, thank you for calling this out as we really need to formalize this process, or at the very least get product's opinion here.
@MikePaquette As part of 7.13 we have already added the event
fieldset to both the alert mappings and the enrichment logic. However, as Marshall identified, the current indicator mappings are missing the nested file
, hash
, and registry
fieldsets as currently specified in the RFC. Should we:
- Do nothing
- Add these mappings but leave them off of enrichment
- Add both the mappings and the corresponding enrichment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sounds good - I went ahead and 👍'd since any of the 3 approaches sound reasonable to me and the code changes look good.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Confirmed the above needs/rationale with @MikePaquette and @peasead. Merging for now!
💛 Build succeeded, but was flaky
Test FailuresKibana Pipeline / general / Chrome X-Pack UI Functional Tests.x-pack/test/functional/apps/dashboard/sync_colors·ts.dashboard sync colors should sync colors on dashboard by defaultStandard Out
Stack Trace
Metrics [docs]
History
To update your PR or re-run it, just comment with: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, thanks @rylnd
… 1.9 (elastic#97573) * adds snapshot test for getSignalsTemplate * [CTI] Extracts non-ecs, non-signal mappings to separate file * adds updated ECS mappings * Normalize/clean up various mappings files * Adds a wrapping "mappings.properties" around our extra mappings * Spreads our other mappings similarly to ECS mappings * Moves dynamic: false out of ECS mappings and into our main template * Ensures we include 'threat.properties.indicator', since that's where our 'type: nested' declaration resides * Update ECS mappings snapshot post-1.9 updates This updated snapshot reflects the mappings changes that one will receive when migrating/rolling over to a 7.13 alerts index. * Update signals template version as per guidelines. The last released mappings update was elastic#92928, which bumped from 24 -> 25. The few unreleased updates since then have increased this by 1, but since these changes are going out with 7.13 we are bumping by 10 _since the last release_, in order to give "room" for minor releases. * Fix cypress test failure due to updated mappings This magic number represents "the number of mapped fields that begin with 'host.geo.c' and, because this PR adds a mapping for host.geo.continent_code, the test needed to be updated. Co-authored-by: Ece Ozalp <ozale272@newschool.edu>
💚 Backport successful
This backport PR will be merged automatically after passing CI. |
… 1.9 (#97573) (#97682) * adds snapshot test for getSignalsTemplate * [CTI] Extracts non-ecs, non-signal mappings to separate file * adds updated ECS mappings * Normalize/clean up various mappings files * Adds a wrapping "mappings.properties" around our extra mappings * Spreads our other mappings similarly to ECS mappings * Moves dynamic: false out of ECS mappings and into our main template * Ensures we include 'threat.properties.indicator', since that's where our 'type: nested' declaration resides * Update ECS mappings snapshot post-1.9 updates This updated snapshot reflects the mappings changes that one will receive when migrating/rolling over to a 7.13 alerts index. * Update signals template version as per guidelines. The last released mappings update was #92928, which bumped from 24 -> 25. The few unreleased updates since then have increased this by 1, but since these changes are going out with 7.13 we are bumping by 10 _since the last release_, in order to give "room" for minor releases. * Fix cypress test failure due to updated mappings This magic number represents "the number of mapped fields that begin with 'host.geo.c' and, because this PR adds a mapping for host.geo.continent_code, the test needed to be updated. Co-authored-by: Ece Ozalp <ozale272@newschool.edu> Co-authored-by: Ryland Herrick <ryalnd@gmail.com> Co-authored-by: Ece Ozalp <ozale272@newschool.edu>
… 1.9 (elastic#97573) * adds snapshot test for getSignalsTemplate * [CTI] Extracts non-ecs, non-signal mappings to separate file * adds updated ECS mappings * Normalize/clean up various mappings files * Adds a wrapping "mappings.properties" around our extra mappings * Spreads our other mappings similarly to ECS mappings * Moves dynamic: false out of ECS mappings and into our main template * Ensures we include 'threat.properties.indicator', since that's where our 'type: nested' declaration resides * Update ECS mappings snapshot post-1.9 updates This updated snapshot reflects the mappings changes that one will receive when migrating/rolling over to a 7.13 alerts index. * Update signals template version as per guidelines. The last released mappings update was elastic#92928, which bumped from 24 -> 25. The few unreleased updates since then have increased this by 1, but since these changes are going out with 7.13 we are bumping by 10 _since the last release_, in order to give "room" for minor releases. * Fix cypress test failure due to updated mappings This magic number represents "the number of mapped fields that begin with 'host.geo.c' and, because this PR adds a mapping for host.geo.continent_code, the test needed to be updated. Co-authored-by: Ece Ozalp <ozale272@newschool.edu>
Summary
This PR does two main things:
Because it's really hard to observe the changes that one is making to those mappings, and to force such changes to be expected and deliberate, we've also added a snapshot test around our mappings-generating function. The full set of changes to our mappings, then, can be seen in this diff.
Checklist
For maintainers