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] Bulk update on index pattern for ML rules #124918

Closed
cybersecdiva opened this issue Feb 8, 2022 · 20 comments
Closed

[Security Solution] Bulk update on index pattern for ML rules #124918

cybersecdiva opened this issue Feb 8, 2022 · 20 comments
Assignees
Labels
8.4 candidate bug Fixes for quality problems that affect the customer experience Feature:Rule Management Security Solution Detection Rule Management area fixed impact:high Addressing this issue will have a high level of impact on the quality/strength of our product. QA:Validated Issue has been validated by QA SecuritySolution:QAAssist Part of QA testing process for release Team:Detection Rule Management Security Detection Rule Management Team Team:Detections and Resp Security Detection Response Team Team: SecuritySolution Security Solutions Team working on SIEM, Endpoint, Timeline, Resolver, etc. v8.4.0

Comments

@cybersecdiva
Copy link

cybersecdiva commented Feb 8, 2022

Describe the bug:
ML rules doesn't have index patterns. Users should not be able to modify and update and add index patterns

Kibana/Elasticsearch Stack version:
8.1 BC1

Steps to reproduce:

  1. Create a new ML rule

Go to Rules overview and search Ml and select Mltest rules

  • In this scenario I have a total of 3 Ml test rules and used ml* to list all ML rules
  1. Select the Ml test rules ----> Bulk actions ----> Index patterns ----> Add index patterns

( apm-*-transaction, traces-apm*, auditbeat-*, endgame-*, filebeat-*, logs-*, packetbeat-*, winlogbeat-*)

Note: Behavior was also tested with the selection of a single index pattern, and the same result produced and allowed bulk update on index pattern for ML rule

Current behavior:
Rules - Kibana

Expected behavior:
The selection for modification add/delete index patterns for ML rules should prompt an error message

@cybersecdiva cybersecdiva added bug Fixes for quality problems that affect the customer experience triage_needed Team: SecuritySolution Security Solutions Team working on SIEM, Endpoint, Timeline, Resolver, etc. v8.1.0 labels Feb 8, 2022
@elasticmachine
Copy link
Contributor

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

@cybersecdiva
Copy link
Author

@MadameSheema FYI

@cybersecdiva
Copy link
Author

@MadameSheema MadameSheema added the Team:Detection Rule Management Security Detection Rule Management Team label Feb 8, 2022
@banderror banderror added Feature:Rule Management Security Solution Detection Rule Management area Team:Detections and Resp Security Detection Response Team labels Feb 8, 2022
@elasticmachine
Copy link
Contributor

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

@banderror banderror removed their assignment Feb 8, 2022
@banderror banderror added the impact:high Addressing this issue will have a high level of impact on the quality/strength of our product. label Feb 8, 2022
@vitaliidm
Copy link
Contributor

vitaliidm commented Feb 8, 2022

@yiyangliu9286 , @jethr0null , can you please look at this issue.
We need to come up with solution how to tell user that ML rules can't be edited for index patterns

  1. Should we prevent user from proceeding with editing ML rules(like with Elastic ones)? We need to make sure in this case we don't display too many modal windows, like first one for Elastic rules, second for ML rules.
    For this solution there 2 cases:
  • User selects ML rules on page. This way we know which are ML and can notify user if needed
  • User selects all existing rules. This way we don't know if there are any ML rules, we would need to make HTTP request to determine it(similarly to elastic rule)
  1. Or should we partially fail bulk edit for ML rule?
  2. Or maybe display message in toast that some of rules were skipped because they are ML?

Or maybe we can combine approaches

@banderror
Copy link
Contributor

banderror commented Feb 9, 2022

Synced with @vitaliidm on the current behavior of the bulk actions endpoint.

So if the user tries to edit (add, remove, overwrite) index patterns of a set of rules, and there is one or a few ML rules among them, the endpoint will silently skip applying this modification to the ML rules. However, in the endpoint response, these ML rules will be represented as successfully updated, which is not true.

The server-side logic is going to be fixed in #124525 in such a way that the endpoint will return an error for every attempt to edit index patterns of an ML rule.

@yiyangliu9286
Copy link

Thanks for the update and the proposed solution @vitaliidm @banderror!

I think from the frond-end UI perceptive we'd like to notify users about ML rules cannot be edited (as well as the existing prebuilt rules which also cannot be edited atm), here are 4 scenarios:

  • If users select rules which contain prebuilt rules + ML rules + custom rules:

Security Rules_duplicate rule modal

  • If users select rules which contain ML rules + custom rules:

Security Rules_duplicate rule modal-1

  • If users select rules which contain prebuilt rules + ML rules (block action):

Security Rules_block workflow modal

  • If users select rules which contain ML rules (block action):

Security Rules_block workflow modal-1

@vitaliidm
Copy link
Contributor

@yiyangliu9286 , I'm thinking this approach can be too complex to scale

Right now we would have 7 scenarios:

  1. Elastic rules
  2. ML rules for index patterns
  3. Custom
  4. Elastic rules + ML
  5. Elastic + Custom
  6. Custom + ML
  7. Elastic + Custom + ML

If there would be yet another one uneditable rule type: we will end up with ~ 12 scenarios and so on and so on.

Plus:

  • Modal title will be too large, it would consist more and more data.
  • Localization also become difficult to handle, due to a lot of scenarios.
  • Maybe we would need to give user some details, for example why ML rule is no getting updated with Index Pattern

Instead, what if would have just common 3 scenarios:

  1. No custom rules, action is blocked
    We than display in modal window list with number of failed rules and reason

  2. Custom rules + uneditable rules
    We say in title how many rule we can edit.
    And then list of rules that can't be

  3. All custom - no modal at all, as before

This way, if there would be another type of not applicable action, we would just add another item in a list.
Without a need to handle multiple crossing scenarios
Here is a rough draft of my proposal
Screenshot 2022-02-09 at 20 35 05

@banderror
Copy link
Contributor

banderror commented Feb 11, 2022

I agree with @vitaliidm - let's keep the title as simple as possible without permutations and combinations, and represent types of rules that can't be edited in the form of a list.

I'd maybe suggest

The action will only be applied to 3 out of 176 rules you've selected
===========

This action can't be applied to the following rules:

- 170 prebuilt Elastic rules (editing prebuilt rules is not supported)
- 3 custom Machine Learning rules (these rules don't have index patterns)

                     Cancel     Edit 3 rules

vitaliidm added a commit that referenced this issue Feb 11, 2022
…stions and increases test coverage (#124525)

Issue: #125223
## Summary

- removes try/catch for bulk edit operation
- removes isElasticRule in bulk route API, replaces with rule.params.immutable check(as isElasticRule is used within telemetry)
- adds Cypress tests
- fixes case when index pattern action applied to ML rule. As ML rules don't have index pattern, so we will throw error if there is an attempt to run this action. It partially addresses #124918
- now checks if updated index patterns array is empty, if it is: throws error(#125223)
kibanamachine pushed a commit to kibanamachine/kibana that referenced this issue Feb 11, 2022
…stions and increases test coverage (elastic#124525)

Issue: elastic#125223
## Summary

- removes try/catch for bulk edit operation
- removes isElasticRule in bulk route API, replaces with rule.params.immutable check(as isElasticRule is used within telemetry)
- adds Cypress tests
- fixes case when index pattern action applied to ML rule. As ML rules don't have index pattern, so we will throw error if there is an attempt to run this action. It partially addresses elastic#124918
- now checks if updated index patterns array is empty, if it is: throws error(elastic#125223)

(cherry picked from commit ae51f81)
kibanamachine added a commit that referenced this issue Feb 11, 2022
…stions and increases test coverage (#124525) (#125437)

Issue: #125223
## Summary

- removes try/catch for bulk edit operation
- removes isElasticRule in bulk route API, replaces with rule.params.immutable check(as isElasticRule is used within telemetry)
- adds Cypress tests
- fixes case when index pattern action applied to ML rule. As ML rules don't have index pattern, so we will throw error if there is an attempt to run this action. It partially addresses #124918
- now checks if updated index patterns array is empty, if it is: throws error(#125223)

(cherry picked from commit ae51f81)

Co-authored-by: Vitalii Dmyterko <92328789+vitaliidm@users.noreply.github.com>
@banderror
Copy link
Contributor

Okie, so the server-side logic has been fixed in #124525 and will be released in 8.1.0. I think we can work on the UI improvements as part of the 8.2 dev cycle (release in 8.2.0 and maybe 8.1.1+).

@yiyangliu9286
Copy link

@yiyangliu9286 , I'm thinking this approach can be too complex to scale

Right now we would have 7 scenarios:

  1. Elastic rules
  2. ML rules for index patterns
  3. Custom
  4. Elastic rules + ML
  5. Elastic + Custom
  6. Custom + ML
  7. Elastic + Custom + ML

If there would be yet another one uneditable rule type: we will end up with ~ 12 scenarios and so on and so on.

Plus:

  • Modal title will be too large, it would consist more and more data.
  • Localization also become difficult to handle, due to a lot of scenarios.
  • Maybe we would need to give user some details, for example why ML rule is no getting updated with Index Pattern

Instead, what if would have just common 3 scenarios:

  1. No custom rules, action is blocked
    We than display in modal window list with number of failed rules and reason
  2. Custom rules + uneditable rules
    We say in title how many rule we can edit.
    And then list of rules that can't be
  3. All custom - no modal at all, as before

This way, if there would be another type of not applicable action, we would just add another item in a list. Without a need to handle multiple crossing scenarios Here is a rough draft of my proposal Screenshot 2022-02-09 at 20 35 05

Hi @vitaliidm and @banderror! Thanks for the suggested path and providing your thoughts on how to improve and better resolve this issue by considering the current and future use cases! I think your proposal would be the most simplified and intuitive way to address this - we wouldn't want to create a lot of complicated use cases to potentially create users confusion, so let's proceed to fix this issue to align your thoughts here.

Let's limit users options for those 3 scenarios you've mentioned above (I think it makes sense to to able to explain to users why those rules aren't editable) Here are the updated designs:

  1. No custom rules, action is blocked (explain why those rules cannot be edited for different reasons + rules count, and block users to proceed to the next step):

Security Rules_block workflow modal

  1. Custom rules + uneditable rules (address the number of Custom rules to be able to edit in the title, and in the paragraph, explain why those rules cannot be edited + rules count):

Security Rules_duplicate rule modal

  1. All custom - no modal at all, as before

@banderror banderror added the 8.2 candidate considered, but not committed, for 8.2 release label Feb 15, 2022
@cybersecdiva
Copy link
Author

@banderror For clarification, will we be providing a fix for this bug in 8.2 to prevent the user from adding ML rules using bulk actions for index patterns and tags?

@banderror
Copy link
Contributor

@cybersecdiva Yup, it's in our checklist for 8.2.
Small correction though: no need for preventing editing tags of ML rules, only index patterns.

@banderror banderror added the SecuritySolution:QAAssist Part of QA testing process for release label Mar 10, 2022
@banderror banderror added 8.3 candidate and removed 8.2 candidate considered, but not committed, for 8.2 release v8.1.0 v8.2.0 labels Apr 11, 2022
@banderror
Copy link
Contributor

Just an update that the team thinks that we should address #125512 before fixing the remainder of this bug. I'll push it to the 8.4 scope.

@vitaliidm
Copy link
Contributor

vitaliidm commented Jun 24, 2022

Hey @yiyangliu9286
I also added a default option if any error happens during the check, it will be displayed as such

[Rules number[ rules can't be edited due to error: [Error message goes here]

Screenshot 2022-06-24 at 11 42 03

Please, let me know if you are fine with this implementation

@yiyangliu9286
Copy link

Hey @yiyangliu9286 I also added a default option if any error happens during the check, it will be displayed as such

[Rules number[ rules can't be edited due to error: [Error message goes here]

Screenshot 2022-06-24 at 11 42 03

Please, let me know if you are fine with this implementation

Thanks for showing this @vitaliidm! This looks good to me. The only thing I'd say is to make the format of the two bullet points consistent:

This action can't be applied to followings rules:
- 640 Prebuilt Elastic rules (editing prebuilt rules is not supported)
- 101 [%rule_type%, if they are one type e.g. Machine Learning] rules can't be edited (the current user is not a machine learning administrator)

vitaliidm added a commit that referenced this issue Jul 18, 2022
…dle for bulk edit of ML rule index (#134664)

## Summary
Addresses:
- #125512
- #124918

Implemented
- Adds new search query parameter `dry_run` to _bulk_action API, that allows simulate bulk action without actually updating rules. More details in the [ticket](#125512 (comment))
- Changes the way, modal window is displaying before applying bulk edit action(index patterns, timeline, tags). 
   Before displaying modal window, dry_run bulk action request will be send to get information how many rules can be edited. Based on this result, modal window displays how many rules can be edited and how many can't(with displaying error of validation). Users then can proceed to edit rules, that haven't failed during dry run.
Validation errors include at this point
   - immutable rules
   - index pattern action on ML rule
   - default error message handle, that displays error message, with which rule failed
 
### Before
Error displayed when user tried to apply index pattern action to ML rule

https://user-images.githubusercontent.com/92328789/175342440-39ede444-d90e-4294-a68f-b9f3c83a81d1.mov

<img width="2011" alt="Screenshot 2022-06-23 at 16 55 52" src="https://user-images.githubusercontent.com/92328789/175342592-a271e2c3-ccf7-43ee-a579-02bc7cc71264.png">


### After
Modal window is displayed with message that index pattern action can't be applied to ML rule

https://user-images.githubusercontent.com/92328789/174799973-8dc28c14-4413-4837-adf9-644bf4c81297.mov

<img width="1535" alt="Screenshot 2022-06-21 at 13 34 27" src="https://user-images.githubusercontent.com/92328789/174800195-184f9233-e552-411d-a815-5950dbb44cf8.png">

### Follow-ups:
Once merged, create PR for Security Solution API documentation

### 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
- [x] Any UI touched in this PR is usable by keyboard only (learn more about [keyboard accessibility](https://webaim.org/techniques/keyboard/))
- [x] Any UI touched in this PR does not create any new axe failures (run axe in browser: [FF](https://addons.mozilla.org/en-US/firefox/addon/axe-devtools/), [Chrome](https://chrome.google.com/webstore/detail/axe-web-accessibility-tes/lhdoppojpmngadmnindnejefpokejbdd?hl=en-US))
- [x] This renders correctly on smaller devices using a responsive layout. (You can test this [in your browser](https://www.browserstack.com/guide/responsive-testing-on-local-server))
- [x] This was checked for [cross-browser compatibility](https://www.elastic.co/support/matrix#matrix_browsers)

### Release note
- adds dry run mode for /detection_engine/rules/_bulk_action API
- displays warning modal when user will try to apply index pattern bulk edit action to Machine Learning rule
@vitaliidm vitaliidm added QA:Ready for Testing Code is merged and ready for QA to validate fixed labels Jul 18, 2022
@vitaliidm vitaliidm assigned cybersecdiva and unassigned vitaliidm Jul 18, 2022
@vitaliidm
Copy link
Contributor

@cybersecdiva, issue has been addressed in #134664

@banderror
Copy link
Contributor

cc @MadameSheema

@ghost
Copy link

ghost commented Jul 29, 2022

Hi Team,

We have validated above issue on 8.4.0 BC1 and it's working fine. 🟢

Build Details

VERSION: 8.4.0 BC1
BUILD: 54999
COMMIT: 58f7eaf0f8dc3c43cbfcd393e587f155e97b3d0d

Screenshots:
NO custom Rules

image

Custom Rules and Non actionable rules

image

Custom Rules

image

Please let us know if we need to cover any other scenarios or we are good to close this issue.

Thanks !

CC: @MadameSheema

@banderror
Copy link
Contributor

I think we've addressed all the issues described in this ticket and can close it @karanverma-qasource. Thank you.

@ghost ghost closed this as completed Jul 29, 2022
@ghost ghost added QA:Validated Issue has been validated by QA and removed QA:Ready for Testing Code is merged and ready for QA to validate labels Jul 29, 2022
This issue was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
8.4 candidate bug Fixes for quality problems that affect the customer experience Feature:Rule Management Security Solution Detection Rule Management area fixed impact:high Addressing this issue will have a high level of impact on the quality/strength of our product. QA:Validated Issue has been validated by QA SecuritySolution:QAAssist Part of QA testing process for release Team:Detection Rule Management Security Detection Rule Management Team Team:Detections and Resp Security Detection Response Team Team: SecuritySolution Security Solutions Team working on SIEM, Endpoint, Timeline, Resolver, etc. v8.4.0
Projects
None yet
Development

No branches or pull requests

6 participants