-
Notifications
You must be signed in to change notification settings - Fork 8.3k
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] [CTI] Fixes bug that caused Threshold and Indicator Match rules to ignore custom rule filters if a saved query was used in the rule definition. #109253
Conversation
Pinging @elastic/security-solution (Team: SecuritySolution) |
hello 👋 added some labels to ensure that the PR passes all pipeline checks. Just to quickly go over those, as for the contents of the PR - I wasn't able to find a ticket with a clear description of the desired solution / acceptance criteria, the bug ticket had a problem statement which was helpful.
reading this, my understanding was that the presence of the saved query bypasses the indicator matching pieces of the rule execution for the threat_match rule, which results in more alerts. In other words, some events that are grabbed by that query that do NOT have a corresponding threat match still trigger alerts when then shouldn't (as per the matching information on the indicator match rule). It's almost like the indicator match rule just becomes a custom query rule, which is not good! it looks like the solution implemented in this PR is to remove the saved query functionality, which is arguably one way to solve this problem. That being said, I would have assumed that we still want to keep the concept of saved queries around, but need to ensure that appropriate threat matching happens for the indicator match rule even when there is a saved query present. Would love to get some confirmation from @rylnd on this one. |
I was able to check out the code and confirm that the bug is gone, and the rule creation flow with a saved query selected from in the query bar works as expected for indicator match rule as well as custom query rule. It is also mentioned here that the saved_id params are no longer necessary. A quick look at the QueryBar ⬇️ kibana/x-pack/plugins/security_solution/public/detections/components/rules/query_bar/index.tsx Lines 183 to 185 in 4a54188
seems to show that the filters and the query is grabbed from the saved query during the rule creation step, which seems to remove the need for the usage of saved_id during the rule execution. ✅ I was able to see that with the changes in this PR, alert.rule.saved_id is present on the alert kibana/x-pack/plugins/security_solution/common/detection_engine/schemas/request/rule_schemas.ts Lines 212 to 221 in 4a54188
I was wondering if these (saved_id from optional rule params) can / should be removed altogether as a part of this change as well. |
I agree that we should remove as much of |
73fd7c1
to
2fb24e9
Compare
I agree that ideally, we should remove
If we want to get rid of |
interesting question - I am not particularly familiar with |
There are very few differences between the two query rule types and most logic is already shared; For 7.x I think we can remove the ability to create
Second Edit: Removing support for |
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.
Requesting changes as per the PR comments; please re-request review when saved_query
rules can no longer be created!
@elasticmachine merge upstream |
70b1cbf
to
3f9d3f0
Compare
@elasticmachine merge upstream |
03e7750
to
7f45d6f
Compare
@marshallmain @rylnd Thank you for your latest comments. I updated this PR, and make very simple small changes, only to address bugs with Indicator Match and Threshold rule. These rules will simply ignore saved_id and just use query. Also, I didn't update any schema or types, I think it makes sense to do when we will completely remove save_id functionality |
@elasticmachine merge upstream |
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.
Reviewed the code and verified that the regression tests failed without the change.
@nkhristinin we have the release_note:fix
label here already; just be sure to follow up with the docs team to ensure an appropriate note has been generated for this issue so that affected users will be aware of the fix; before merging I would update the PR title to describe the bug(s) being fixed.
x-pack/plugins/security_solution/server/lib/detection_engine/signals/get_filter.test.ts
Outdated
Show resolved
Hide resolved
x-pack/plugins/security_solution/server/lib/detection_engine/signals/get_filter.test.ts
Outdated
Show resolved
Hide resolved
I think this should also go into 7.15.1; I've added the appropriate label so the backport bot should take care of the rest, but ping me if you have questions. |
…or Match rules to ignore custom rule filters if a saved query was used in the rule definition. (elastic#109253) * Ignore saved_id for Threat match and threshold rules Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>
…or Match rules to ignore custom rule filters if a saved query was used in the rule definition. (elastic#109253) * Ignore saved_id for Threat match and threshold rules Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>
If we're holding off on this until 7.15.1 then we need to cancel the backport to the 7.15 branch for now, right? I disabled auto-merge on the 7.15 backport PR to make sure we have a chance to confirm what the desired outcome is here. |
@marshallmain good catch; even with bugfixes we need to be mindful of the current release status. @nkhristinin the |
…eporting-to-v2 * 'master' of github.com:elastic/kibana: (65 commits) Move to vis_types folder part 2 (elastic#110574) [SOR] use initialNamespaces when checking for conflict for `create` and `bulkCreate` (elastic#111023) [Discover] Remove export* syntax (elastic#110934) [Event log][7.x] Updated event log client to search across legacy IDs (elastic#109365) [Security Solution][Detection Rules] Changes 'activated' text on rule details page (elastic#111044) [Metrics UI] Filter out APM nodes from the inventory view (elastic#110300) [package testing] Update logging and pid configuration (elastic#111059) [Dashboard] Read App State from URL on Soft Refresh (elastic#109354) Add correct roles to test user for functional tests in dashboard (elastic#110880) [DOCS] Adds Lens Inspector and minor edits (elastic#109736) [DOCS] Updates Spaces page (elastic#111005) normalize initialNamespaces (elastic#110936) [Reporting] Clean up `any` usage, reorganize server route files (elastic#110740) [Security Solution] [CTI] Fixes bug that caused Threshold and Indicator Match rules to ignore custom rule filters if a saved query was used in the rule definition. (elastic#109253) skip flaky suites: elastic#111001, elastic#111022 [Security Solution][RAC] - Update reason field text (elastic#110308) [RAC][Security Solution] Make analyzer work with EuiDataGrid full screen (elastic#110913) [Metrics UI] Add integration tests for Metric Threshold Rule and refactor to fire correctly (elastic#109971) [DOCS] Updates Discover docs (elastic#110346) [RAC] Persistent timeline fields fix (elastic#110685) ...
💔 Build Failed
Failed CI Steps
Test FailuresKibana Pipeline / general / Chrome X-Pack UI Functional Tests.x-pack/test/functional/apps/transform/starting·ts.transform starting continuous transform with pivot configuration start transformStandard Out
Stack Trace
Kibana Pipeline / general / Chrome X-Pack UI Functional Tests.x-pack/test/functional/apps/transform/starting·ts.transform starting "after all" hook in "starting"Standard Out
Stack Trace
Kibana Pipeline / general / Chrome X-Pack UI Functional Tests.x-pack/test/functional/apps/transform.transform "after all" hook in "transform"Standard Out
Stack Trace
and 2 more failures, only showing the first 3. Metrics [docs]
History
To update your PR or re-run it, just comment with: cc @nkhristinin |
Looks like this PR has backport PRs but they still haven't been merged. Please merge them ASAP to keep the branches relatively in sync. |
1 similar comment
Looks like this PR has backport PRs but they still haven't been merged. Please merge them ASAP to keep the branches relatively in sync. |
…or Match rules to ignore custom rule filters if a saved query was used in the rule definition. (#109253) (#111024) * Ignore saved_id for Threat match and threshold rules Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com> Co-authored-by: Khristinin Nikita <nikita.khristinin@elastic.co>
Looks like this PR has backport PRs but they still haven't been merged. Please merge them ASAP to keep the branches relatively in sync. |
Removing the |
Looks like this PR has backport PRs but they still haven't been merged. Please merge them ASAP to keep the branches relatively in sync. |
Closed the 7.15 backport for now since it appears the bot doesn't like it staying open. |
Summary
If the rule configuration has Saved Query, it's ignored specific filter logic for Indicator match and Threshold rules. Here described how to reproduce it.
In this PR, we will ignore savedQuery for Threshold and Indicator Match, and just use the custom query from rule configuration.
Checklist
Delete any items that are not applicable to this PR.
Risk Matrix
Delete this section if it is not applicable to this PR.
Before closing this PR, invite QA, stakeholders, and other developers to identify risks that should be tested prior to the change/feature release.
When forming the risk matrix, consider some of the following examples and how they may potentially impact the change:
For maintainers