elastic / kibana

Your window into the Elastic Stack
https://www.elastic.co/products/kibana
Other
19.69k stars 8.12k forks source link

[Security Solution] Editing rules independently of source data #180407

Open banderror opened 5 months ago

banderror commented 5 months ago

Epics: https://github.com/elastic/security-team/issues/1974 (internal), https://github.com/elastic/kibana/issues/174168 Source of discussion: https://github.com/elastic/kibana/issues/178611

Summary

As part of the ongoing Prebuilt Rule Customization epic, the requirement to change the behaviour of rule validation on editing has come up.

Instead of blocking the editing of a rule when the rule's data source has not enough data for the query to work, the expected UX would only warn the user but continue to proceed with saving the rule.

However, such a change will have consequences on a number of features that depends on a rule's data source. We need to list them here, detail the consequences of such changes and find alternative behaviours where needed.

Please add any feature that might be impacted by this change, describing:

### Tasks
- [ ] https://github.com/elastic/security-team/issues/9282
- [ ] https://github.com/elastic/security-team/issues/10181
- [ ] https://github.com/elastic/security-team/issues/10215
- [ ] https://github.com/elastic/security-docs/issues/5758
- [ ] https://github.com/elastic/kibana/issues/191832
elasticmachine commented 5 months ago

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

elasticmachine commented 5 months ago

Pinging @elastic/security-detection-rule-management (Team:Detection Rule Management)

elasticmachine commented 5 months ago

Pinging @elastic/security-detection-engine (Team:Detection Engine)

elasticmachine commented 5 months ago

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

banderror commented 5 months ago

@approksiu Could you please have a PM look at this discussion ticket + at the description and the whole thread of comments in https://github.com/elastic/kibana/issues/178611 -- when you have time? This is not urgent at all, this is just FYI that we have this problem to discuss and might want to make a decision in the next few months. I will assign you to both tickets and add needs product.

We'd like to understand potential risks of allowing users to save rules (when creating new rules or editing existing ones) when their queries or filters are syntactically valid, but the data they rely upon is missing (e.g. concrete indices with source events are missing) or invalid (e.g. certain fields used in queries are missing or have incorrect mappings).

Having a strict validation that validates source data (what we have now) or having a looser validation but allowing users to customize prebuilt rules (what we are suggesting to have with @jpdjere) is a tradeoff, and we'd like to better understand what we're potentially going to "trade" for a better prebuilt rule customization UX.

@jpdjere @yctercero @rylnd @marshallmain Let's move the general discussion from https://github.com/elastic/kibana/issues/178611 to here, and continue discussing any EQL validation specifics in https://github.com/elastic/kibana/issues/178611. Thank you for all your comments!

banderror commented 5 months ago

@rylnd thank you, regarding your comments (one, two):

Required fields also doesn't (currently) allow this behavior, and I imagine other validations might also cause issue.

Actually, required fields are not currently editable in the app. @nikitaindik is working on adding support for editing them in https://github.com/elastic/kibana/issues/173594, this is work in progress.

In terms of this ticket: I think we need a little product/UX guidance on how we convey this situation to the user. It does look like the form hook lib has a concept of validation warnings, so we could potentially leverage that

@nikitaindik @dplumlee Could you please check that link?

As part of https://github.com/elastic/kibana/issues/173594 Nikita is working on implementing validation warnings for required fields instead of validation errors, and allowing the user to save a rule with required fields that don't necessarily exist. Also, @dplumlee is working on https://github.com/elastic/kibana/issues/173593 where we will also need some validation that generates warnings instead of errors.

marshallmain commented 5 months ago

Having a strict validation that validates source data (what we have now) or having a looser validation but allowing users to customize prebuilt rules (what we are suggesting to have with @jpdjere) is a tradeoff, and we'd like to better understand what we're potentially going to "trade" for a better prebuilt rule customization UX.

With something like a confirmation modal described at the end of https://github.com/elastic/kibana/issues/178611#issuecomment-2045930566, we could maybe get the best of both strict and loose validation. Users could save the rule with errors, but we would add an extra notice for users when they attempt to save the rule with errors or warnings that haven't been resolved.

e40pud commented 1 month ago

Per our discussion

  1. EQL and ES|QL query syntax errors will be blocking and won't allow user to save a rule with such query errors.
  2. ES|QL rule query requires metadata operator with the _id field and will be blocking condition for user to be able to save a rule.
  3. Missing data source and/or field won't be a blocking error and will allow user to save a rule

cc @approksiu

approksiu commented 1 month ago

Following up after additional research (details here)

  1. @e40pud please check that the other rule types (except ML) allow saving the rule with the edited data source.
  2. I want to allow users to change other data-source-dependent fields outside of the rule definition and save the rule independently of source data, I think this can be done later. Issue.
e40pud commented 1 week ago

@approksiu my changes are ready for review and I wanted to double check with you the wording inside confirmation modal that we are going to show to users:

Image

Also,

  1. Do we want different messages during rule creation and rule updating workflows? Right now same modal used.
  2. Do we want to cover this use case in documentation?
approksiu commented 1 week ago

@e40pud I suggest to make the error message more clear, something like "No index matching index pattern ["non-existent-index"] defined in the rule was found."

@nastasha-solomon could you please check the modal wording?

Re questions:

  1. The same modal is fine for rule creation and editing workflows.
  2. It is good to cover this case in the docs to specify that the rule will fail if run on the system that is missing specified index patterns, and user can proceed if they intend to use the rule on the other system where data is present, or will setup indexes later.

This rule is attempting to query data from Elasticsearch indices listed in the "Index patterns" section of the rule definition, however no index matching: ["non-existent-index"] was found. This warning will continue to appear until a matching index is created or this rule is disabled.'

approksiu commented 1 week ago

After further discussion, we agreed to simplify the modal to mention validation errors, but list them out, as there can be multiple errors.

e40pud commented 1 week ago

Here is the simplified version of the confirmation modal

Image