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.

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][Detection Engine] fixes ES|QL ECS multifiefields issue #167769

Merged

Conversation

vitaliidm
Copy link
Contributor

@vitaliidm vitaliidm commented Oct 2, 2023

Summary

  • fixes https://github.com/elastic/security-team/issues/7741 by replacing ecsMap from hardcoded @kbn/rule-registry-plugin to actual mapping for alerts indices from @kbn/alerts-as-data-utils
  • when converting ES|QL row table results to object, null values skipped, since its results consists of all existing mappings in searched indices, if fields in query are not filtered

Checklist

Delete any items that are not applicable to this PR.

@vitaliidm vitaliidm self-assigned this Oct 2, 2023
@vitaliidm vitaliidm added release_note:skip Skip the PR/issue when compiling release notes Team:Detections and Resp Security Detection Response Team Team: SecuritySolution Security Solutions Team working on SIEM, Endpoint, Timeline, Resolver, etc. Team:Detection Engine Security Solution Detection Engine Area v8.11.0 labels Oct 2, 2023
@vitaliidm vitaliidm changed the title [Security Solution][Detection Engine] fixes ES|QL EXS multifiefields issue [Security Solution][Detection Engine] fixes ES|QL ECS multifiefields issue Oct 3, 2023
@vitaliidm vitaliidm marked this pull request as ready for review October 3, 2023 12:57
@vitaliidm vitaliidm requested review from a team as code owners October 3, 2023 12:57
@elasticmachine
Copy link
Contributor

Pinging @elastic/security-detections-response (Team:Detections and Resp)

@elasticmachine
Copy link
Contributor

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

@rylnd
Copy link
Contributor

rylnd commented Oct 3, 2023

@vitaliidm to explicitly validate that this will fix the issue described in https://github.com/elastic/sdh-security-team/issues/736, we can add a regression unit test:

diff --git a/x-pack/plugins/security_solution/server/lib/detection_engine/rule_types/factories/utils/strip_non_ecs_fields.test.ts b/x-pack/plugins/security_solution/server/lib/detection_engine/rule_types/factories/utils/strip_non_ecs_fields.test.ts
index 9c10a317ee1..fe66c695f63 100644
--- a/x-pack/plugins/security_solution/server/lib/detection_engine/rule_types/factories/utils/strip_non_ecs_fields.test.ts
+++ b/x-pack/plugins/security_solution/server/lib/detection_engine/rule_types/factories/utils/strip_non_ecs_fields.test.ts
@@ -71,6 +71,26 @@ describe('stripNonEcsFields', () => {
     ]);
   });
 
+  describe('with an object field that exists in the alerts mapping but not our local ECS definition', () => {
+    it('strips that field if it is supplied as a keyword', () => {
+      const { result, removed } = stripNonEcsFields({
+        device: 'test',
+        message: 'test message',
+      });
+
+      expect(result).toEqual({
+        message: 'test message',
+      });
+
+      expect(removed).toEqual([
+        {
+          key: 'device',
+          value: 'test',
+        },
+      ]);
+    });
+  });
+
   describe('array fields', () => {
     it('should not strip arrays of objects when an object is expected', () => {
       const { result, removed } = stripNonEcsFields({

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.

Changes look good to me!

I had a few questions about unit tests changing, but the integration tests answered my questions. Thanks!

@vitaliidm vitaliidm added v8.12.0 backport:prev-minor Backport to (8.x) the previous minor version (i.e. one version back from main) labels Oct 6, 2023
@vitaliidm vitaliidm enabled auto-merge (squash) October 6, 2023 09:19
@kibana-ci
Copy link
Collaborator

💛 Build succeeded, but was flaky

Failed CI Steps

Test Failures

  • [job] [logs] Defend Workflows Cypress Tests #3 / Artifact pages Trusted applications should update Endpoint Policy on Endpoint when adding Trusted application name should update Endpoint Policy on Endpoint when adding Trusted application name
  • [job] [logs] Investigations - Security Solution Cypress Tests #7 / Detections : Page Filters Impact of inputs "after each" hook for "should recover from invalid kql Query result" "after each" hook for "should recover from invalid kql Query result"
  • [job] [logs] Investigations - Security Solution Cypress Tests #7 / Detections : Page Filters Impact of inputs should recover from invalid kql Query result should recover from invalid kql Query result

Metrics [docs]

✅ unchanged

History

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

cc @vitaliidm

@vitaliidm vitaliidm merged commit 4ebe45d into elastic:main Oct 6, 2023
4 checks passed
kibanamachine pushed a commit to kibanamachine/kibana that referenced this pull request Oct 6, 2023
…issue (elastic#167769)

## Summary

- fixes elastic/security-team#7741 by
replacing `ecsMap` from hardcoded `@kbn/rule-registry-plugin` to actual
mapping for alerts indices from `@kbn/alerts-as-data-utils`
- when converting ES|QL row table results to object, `null` values
skipped, since its results consists of all existing mappings in searched
indices, if fields in query are not filtered

### Checklist

Delete any items that are not applicable to this PR.

- [x] [Unit or functional
tests](https://www.elastic.co/guide/en/kibana/master/development-tests.html)
were updated or added to match the most common scenarios

(cherry picked from commit 4ebe45d)
@kibanamachine
Copy link
Contributor

💚 All backports created successfully

Status Branch Result
8.11

Note: Successful backport PRs will be merged automatically after passing CI.

Questions ?

Please refer to the Backport tool documentation

kibanamachine added a commit that referenced this pull request Oct 6, 2023
…fields issue (#167769) (#168206)

# Backport

This will backport the following commits from `main` to `8.11`:
- [[Security Solution][Detection Engine] fixes ES|QL ECS multifiefields
issue (#167769)](#167769)

<!--- Backport version: 8.9.7 -->

### Questions ?
Please refer to the [Backport tool
documentation](https://github.com/sqren/backport)

<!--BACKPORT [{"author":{"name":"Vitalii
Dmyterko","email":"[email protected]"},"sourceCommit":{"committedDate":"2023-10-06T10:59:41Z","message":"[Security
Solution][Detection Engine] fixes ES|QL ECS multifiefields issue
(#167769)\n\n## Summary\r\n\r\n- fixes
elastic/security-team#7741 by\r\nreplacing
`ecsMap` from hardcoded `@kbn/rule-registry-plugin` to actual\r\nmapping
for alerts indices from `@kbn/alerts-as-data-utils`\r\n- when converting
ES|QL row table results to object, `null` values\r\nskipped, since its
results consists of all existing mappings in searched\r\nindices, if
fields in query are not filtered\r\n\r\n### Checklist\r\n\r\nDelete any
items that are not applicable to this PR.\r\n\r\n\r\n- [x] [Unit or
functional\r\ntests](https://www.elastic.co/guide/en/kibana/master/development-tests.html)\r\nwere
updated or added to match the most common
scenarios","sha":"4ebe45d77ee46c2b502c87aee0f89b73f0d3e40f","branchLabelMapping":{"^v8.12.0$":"main","^v(\\d+).(\\d+).\\d+$":"$1.$2"}},"sourcePullRequest":{"labels":["release_note:skip","Team:Detections
and Resp","Team: SecuritySolution","backport:prev-minor","Team:Detection
Engine","v8.11.0","v8.12.0"],"number":167769,"url":"https://github.com/elastic/kibana/pull/167769","mergeCommit":{"message":"[Security
Solution][Detection Engine] fixes ES|QL ECS multifiefields issue
(#167769)\n\n## Summary\r\n\r\n- fixes
elastic/security-team#7741 by\r\nreplacing
`ecsMap` from hardcoded `@kbn/rule-registry-plugin` to actual\r\nmapping
for alerts indices from `@kbn/alerts-as-data-utils`\r\n- when converting
ES|QL row table results to object, `null` values\r\nskipped, since its
results consists of all existing mappings in searched\r\nindices, if
fields in query are not filtered\r\n\r\n### Checklist\r\n\r\nDelete any
items that are not applicable to this PR.\r\n\r\n\r\n- [x] [Unit or
functional\r\ntests](https://www.elastic.co/guide/en/kibana/master/development-tests.html)\r\nwere
updated or added to match the most common
scenarios","sha":"4ebe45d77ee46c2b502c87aee0f89b73f0d3e40f"}},"sourceBranch":"main","suggestedTargetBranches":["8.11"],"targetPullRequestStates":[{"branch":"8.11","label":"v8.11.0","labelRegex":"^v(\\d+).(\\d+).\\d+$","isSourceBranch":false,"state":"NOT_CREATED"},{"branch":"main","label":"v8.12.0","labelRegex":"^v8.12.0$","isSourceBranch":true,"state":"MERGED","url":"https://github.com/elastic/kibana/pull/167769","number":167769,"mergeCommit":{"message":"[Security
Solution][Detection Engine] fixes ES|QL ECS multifiefields issue
(#167769)\n\n## Summary\r\n\r\n- fixes
elastic/security-team#7741 by\r\nreplacing
`ecsMap` from hardcoded `@kbn/rule-registry-plugin` to actual\r\nmapping
for alerts indices from `@kbn/alerts-as-data-utils`\r\n- when converting
ES|QL row table results to object, `null` values\r\nskipped, since its
results consists of all existing mappings in searched\r\nindices, if
fields in query are not filtered\r\n\r\n### Checklist\r\n\r\nDelete any
items that are not applicable to this PR.\r\n\r\n\r\n- [x] [Unit or
functional\r\ntests](https://www.elastic.co/guide/en/kibana/master/development-tests.html)\r\nwere
updated or added to match the most common
scenarios","sha":"4ebe45d77ee46c2b502c87aee0f89b73f0d3e40f"}}]}]
BACKPORT-->

Co-authored-by: Vitalii Dmyterko <[email protected]>
dej611 pushed a commit to dej611/kibana that referenced this pull request Oct 17, 2023
…issue (elastic#167769)

## Summary

- fixes elastic/security-team#7741 by
replacing `ecsMap` from hardcoded `@kbn/rule-registry-plugin` to actual
mapping for alerts indices from `@kbn/alerts-as-data-utils`
- when converting ES|QL row table results to object, `null` values
skipped, since its results consists of all existing mappings in searched
indices, if fields in query are not filtered

### Checklist

Delete any items that are not applicable to this PR.


- [x] [Unit or functional
tests](https://www.elastic.co/guide/en/kibana/master/development-tests.html)
were updated or added to match the most common scenarios
@vitaliidm vitaliidm deleted the detection-engine/esql-ecs-multifields branch March 4, 2024 17:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport:prev-minor Backport to (8.x) the previous minor version (i.e. one version back from main) release_note:skip Skip the PR/issue when compiling release notes Team:Detection Engine Security Solution Detection Engine Area Team:Detections and Resp Security Detection Response Team Team: SecuritySolution Security Solutions Team working on SIEM, Endpoint, Timeline, Resolver, etc. v8.11.0 v8.12.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants