[Security Solution][Detections]Update detection alert mappings to ECS 1.9#97573
Merged
rylnd merged 7 commits intoelastic:masterfrom Apr 20, 2021
Merged
[Security Solution][Detections]Update detection alert mappings to ECS 1.9#97573rylnd merged 7 commits intoelastic:masterfrom
rylnd merged 7 commits intoelastic:masterfrom
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.
rylnd
added
release_note:enhancement
Team:Detections and Resp
Contributor
|
Pinging @elastic/security-solution (Team: SecuritySolution) |
Contributor
|
Pinging @elastic/security-detections-response (Team:Detections and Resp) |
Contributor
Author
|
@MikePaquette If you missed it above, you can see the full mappings changes (which are purely additive) here |
rylnd
commented
Apr 20, 2021
| ], | ||
| "mappings": { | ||
| "dynamic": false, | ||
| "_meta": { |
Contributor
Author
There was a problem hiding this comment.
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
Contributor
Author
There was a problem hiding this comment.
and if we did accidentally start using them, the snapshot test would tell us so 😉
FrankHassanabad
approved these changes
Apr 20, 2021
Contributor
FrankHassanabad
left a comment
FrankHassanabad
left a comment
There was a problem hiding this comment.
LGTM! Looks great. Thanks for the updating of the mappings
| { | ||
| "index_patterns": [ | ||
| "try-ecs-*" | ||
| ], |
Contributor
There was a problem hiding this comment.
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.
Contributor
There was a problem hiding this comment.
I see the comment below, I am good with this.
Contributor
Author
There was a problem hiding this comment.
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.
rylnd
added
the
auto-backport
Contributor
marshallmain
left a comment
marshallmain
left a comment
There was a problem hiding this comment.
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": { |
Contributor
There was a problem hiding this comment.
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?
Contributor
Author
There was a problem hiding this comment.
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
Contributor
There was a problem hiding this comment.
Sounds good - I went ahead and 👍'd since any of the 3 approaches sound reasonable to me and the code changes look good.
Contributor
Author
There was a problem hiding this comment.
Confirmed the above needs/rationale with @MikePaquette and @peasead. Merging for now!
Contributor
💛 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 OutStack TraceMetrics [docs]
History
To update your PR or re-run it, just comment with: |
marshallmain
approved these changes
Apr 20, 2021
kibanamachine
pushed a commit
to kibanamachine/kibana
that referenced
this pull request
Apr 20, 2021
… 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>
Contributor
💚 Backport successful
This backport PR will be merged automatically after passing CI. |
kibanamachine
added a commit
that referenced
this pull request
Apr 20, 2021
… 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>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
7 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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