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] Allow users to edit max_signals field for custom rules #173593

Closed
15 of 16 tasks
Tracked by #174168
approksiu opened this issue Dec 19, 2023 · 17 comments · Fixed by #179680
Closed
15 of 16 tasks
Tracked by #174168

[Security Solution] Allow users to edit max_signals field for custom rules #173593

approksiu opened this issue Dec 19, 2023 · 17 comments · Fixed by #179680
Assignees
Labels
8.15 candidate enhancement New value added to drive a business result Feature:Rule Creation Security Solution Detection Rule Creation workflow Feature:Rule Details Security Solution Detection Rule Details page Feature:Rule Edit Security Solution Detection Rule Editing workflow 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.15.0

Comments

@approksiu
Copy link

approksiu commented Dec 19, 2023

Epics: https://github.com/elastic/security-team/issues/1974 (internal), #174168
Related bug: #164234

Summary

We need to expose the max signals field in the rule details, and rule edit page.
Check the validity of the field (NOTE: To avoid rule failures, do not set the max_signals value higher than the value of xpack.alerting.rules.run.alerts.max.
Provide default value (100 - according to the docs).

Background

We want to allow users more customisation options for their custom rules.
Max_signals field sets the maximum number of alerts that will be created during a single rule execution.

User story

  • As a user, I want to be able to specify max_signals fields for my custom rules.

Acceptance criteria

  • Rule edit page shows the max_signals field in the About Rule section under advanced settings.
  • User can edit the max_signals value on the rule edit page.
  • On the rule edit page, there is a tooltip explaining what this field does, (and a link to documentation?).
  • Rule details page shows the max_signals field in the About Rule section.
  • When creating a new rule, the max_signals filed default value is set to 100.
  • If the user sets value higher than xpack.alerting.rules.run.alerts.max - a warning is shown, but the rule can be saved.
  • If the user sets value higher than xpack.alerting.rules.run.alerts.max - a warning is thrown during rule execution and the lower value of the two will be used for an upper limit.

Release progress

  • Initial implementation is done. We're going to release this enhancement in a single PR, so it's going to be open until everything is ready for release. (PR)
  • Feature is covered with automated tests. (PR)
  • Feature is fully implemented and considered by the development team as ready to be released.
  • Acceptance testing is done and the feature is approved by @approksiu and @ARWNightingale.
  • Exploratory testing is done and the feature is approved by @vgomez-el.
  • Documentation is written for ESS and Serverless by @joepeeples. Two docs PRs are approved and ready to be merged. (ticket)
  • Feature is released in Serverless.
  • Follow-up fixes are merged and released in Serverless. (PR) We'll open separate tickets for follow-up fixes, when/if needed.

Planned release date in Serverless: Mon, May 13, 2024.
Planned release date in ESS: TBD (v8.15.0).

@botelastic botelastic bot added the needs-team Issues missing a team label label Dec 19, 2023
@approksiu approksiu added the Team:Detection Rule Management Security Detection Rule Management Team label Dec 19, 2023
@botelastic botelastic bot removed the needs-team Issues missing a team label label Dec 19, 2023
@jpdjere
Copy link
Contributor

jpdjere commented Jan 5, 2024

TODO:

  • find out how (and if it is possible) to expose the xpack.alerting.rules.run.alerts.max setting to the frontend. Alternatively, consider throwing an error/warning only onSave

Update: This value is exposed here and has to be accesible in the rules client by adding it to:

  1. As a new parameter in the plugin initialization, similarly to how it's done for other props like minimumScheduleInterval: https://github.com/elastic/kibana/blob/main/x-pack/plugins/alerting/server/plugin.ts#L508
  2. In the RuleClient factory, added to the initialize and create methods (where the RulesClient class is instantiated): https://github.com/elastic/kibana/blob/main/x-pack/plugins/alerting/server/rules_client_factory.ts
  3. In the RuleClient class, passed to the context in the constructor: https://github.com/elastic/kibana/blob/main/x-pack/plugins/alerting/server/rules_client/rules_client.ts#L116

And that will make it available in the context object within methods such as update.

@ARWNightingale ARWNightingale self-assigned this Jan 8, 2024
@jpdjere
Copy link
Contributor

jpdjere commented Jan 10, 2024

@approksiu @ARWNightingale

I don't think we really need designs for this (maybe for link to docs, and explanation in tooltip?), but I think we can agree on using the Number field from EUI:

image

@banderror banderror changed the title [Security Solution] Allow users to edit max_signals field for rules [Security Solution] Allow users to edit max_signals field for custom rules Mar 4, 2024
@dplumlee dplumlee self-assigned this Mar 4, 2024
@banderror banderror added enhancement New value added to drive a business result Team:Detections and Resp Security Detection Response Team Team: SecuritySolution Security Solutions Team working on SIEM, Endpoint, Timeline, Resolver, etc. Feature:Rule Creation Security Solution Detection Rule Creation workflow 8.14 candidate labels Mar 7, 2024
@elasticmachine
Copy link
Contributor

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

@elasticmachine
Copy link
Contributor

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

@dplumlee
Copy link
Contributor

dplumlee commented Apr 3, 2024

Deployment links have been added to the PR in the latest build message for acceptance and exploratory testing. cc @vgomez-el @approksiu

@dplumlee
Copy link
Contributor

dplumlee commented Apr 16, 2024

All of our logic changes are now (hopefully) complete and the tickets/acceptance criteria have been updated to match. The new deployment links should be added to the latest build message. Would appreciate some eyes when you have a chance, thanks! cc: @vgomez-el @approksiu @ARWNightingale

@dplumlee
Copy link
Contributor

dplumlee commented Apr 26, 2024

The latest deployment links are in the most recent build message. Would appreciate some eyes for acceptance/exploratory testing when everyone has a chance, thanks! cc: @vgomez-el @approksiu @ARWNightingale

@banderror
Copy link
Contributor

banderror commented Apr 26, 2024

@dplumlee I did a quick testing in the ESS Cloud deployment and found a few issues:


First, an attempt to reset max_signals to an empty value (undefined) on the Rule Editing page leads to a 400 error:

Screenshot 2024-04-26 at 12 21 35 Screenshot 2024-04-26 at 12 21 45

Request body sent to the PUT /api/detection_engine/rules endpoint:

{"type":"query","index":["apm-*-transaction*","auditbeat-*","endgame-*","filebeat-*","logs-*","packetbeat-*","traces-apm*","winlogbeat-*","-*elastic-cloud-logs-*"],"filters":[],"language":"kuery","query":"*","author":[],"exceptions_list":[],"false_positives":[],"references":[],"risk_score":21,"risk_score_mapping":[],"severity":"low","severity_mapping":[],"threat":[],"max_signals":"","name":"Georgii's test","description":"-","tags":[],"setup":"","license":"","interval":"5m","from":"now-360s","to":"now","meta":{"from":"1m","kibana_siem_app_url":"https://kibana-pr-179680.kb.us-west2.gcp.elastic-cloud.com:9243/app/security"},"actions":[],"enabled":false,"id":"929893cf-5021-44cb-8b99-01dea17108d3"}
"max_signals":""

Steps to reproduce:

  1. Create a new rule with default settings, leave max_signals equal to 100 in the form.
  2. On the Rule Details page, click "Edit rule settings".
  3. In About -> Advanced settings, clear the "Max alerts per run" input.
  4. Click "Save changes".

Same eventually happens on rule creation, but the form allows to proceed from the About rule step to the next steps without showing any validation errors:

  1. Open the Rule Creation page.
  2. Fill in Custom query, click Continue.
  3. In the About rule step, fill in Name and Description, expand Advanced settings.
  4. Clear the "Max alerts per run" input, click Continue.
  5. The page allows you to proceed to the next step, click Continue, and click Save.
  6. You see the same 400 error.

I'd expect the field to be optional in the UI, and defaultable on the BE side (if max_signals is undefined in the request body, we set it to 100).


Secondly, in the AC we have:

On the rule edit page, there is a tooltip explaining what this field does, (and a link to documentation?).

Have we decided to not add this tooltip?


Screenshot 2024-04-26 at 12 39 11

Nit: In this warning, we should probably say The rule will only write a maximum of 1000 alerts per rule run..

Also not sure about using the max_signals field name in the text, but I don't know if we use any other field names in our status messages or logs. We could replace "max_signals value" with "Max alerts per run setting".

@dplumlee
Copy link
Contributor

@banderror This has been fixed in the most recent build, a leftover remnant from refactoring logic.

Secondly, in the AC we have:

On the rule edit page, there is a tooltip explaining what this field does, (and a link to documentation?).

Have we decided to not add this tooltip?

I didn't see that in the AC before, but I would say the description we have underneath the field in the form would suffice for this component. It's pretty straightforward and will have been documented externally - I'm struggling to think of what more we would even put in there. @approksiu might have a different opinion

@dplumlee
Copy link
Contributor

dplumlee commented May 2, 2024

@ARWNightingale @vgomez-el Any update on acceptance/exploratory testing for this component? Hoping to merge before the next serverless branch on monday.

@jpdjere
Copy link
Contributor

jpdjere commented May 3, 2024

@ARWNightingale @vgomez-el cc @dplumlee

Here are the credentials so that you can access ESS and Serverless environments for testing:

ESS:
Env Url: https://kibana-pr-179680.kb.us-west2.gcp.elastic-cloud.com:9243/
User and password: https://p.elstc.co/paste/s7aUuzef#3dFyPMsGXJ50ecNnK5kDBUkiy3CGEOV8JQ+UJN6P5Dx

Serverless:
Env URL: https://kibana-pr-179680-security-af6bdf.kb.eu-west-1.aws.qa.elastic.cloud/login
User and password: https://p.elstc.co/paste/w7k0D-wH#QvDSl6IaCj7MLlcyW6oy-lYFU7XwlOYH+p6njH9SRqr

Remember to select the option Login with Elasticsearch to input the user and passwords retrieved from the links above.

@ARWNightingale
Copy link

Looks good to me, That environment has just gone down on me but one small issue is when I put 0 into the input I get an error saying " Max alerts must be greater than 0". I can still save as it just reverts it too the previous value. I think we should add that to the warning. " Max alerts must be greater than 0. The value will be set to default when saved, unless changed" or something similar.

Apart from that, nice work!

@jpdjere
Copy link
Contributor

jpdjere commented May 3, 2024

Agree with @ARWNightingale comment above, but I think the validation should fail on the API request, same as when the the value is negative. Left a comment in the PR on how to fix that.

@vgomez-el
Copy link

vgomez-el commented May 3, 2024

Feature LGTM, but I also have a couple of comments:

image
  • Field is catching nice rare values, But the error message is unrefined and shows that [request body] String in the message that looks a bit ugly ( And I was editing a rule, not adding a new one)
image image

jpdjere added a commit that referenced this issue May 3, 2024
…rules (#179680)

**Resolves: #173593
**Fixes: #164234

## Summary

Adds a number component in the create and edit rule forms so that users
are able to customize the `max_signals` value for custom rules from the
UI. Also adds validations to the rule API's for invalid values being
passed in.

This PR also exposes the `xpack.alerting.rules.run.alerts.max` config
setting from the alerting framework to the frontend and backend so that
we can validate against it as it supersedes our own `max_signals` value.

[Flaky test run (internal)

](https://buildkite.com/elastic/kibana-flaky-test-suite-runner/builds/5601)

### Screenshots
**Form component**
<p align="center">
<img width="887" alt="Screenshot 2024-04-08 at 11 02 12 PM"
src="https://github.com/elastic/kibana/assets/56367316/58cd2f6d-61b6-4343-8025-ff867c050dd7">
</p>

**Details Page**
<p align="center">
<img width="595" alt="Screenshot 2024-04-08 at 11 04 04 PM"
src="https://github.com/elastic/kibana/assets/56367316/d2c61593-3d35-408e-b047-b4d1f68898f8">
</p>

**Error state**
<p align="center">
<img width="857" alt="Screenshot 2024-04-08 at 11 01 55 PM"
src="https://github.com/elastic/kibana/assets/56367316/86e64280-7b81-46f2-b223-fde8c20066c8">
</p>

**Warning state**
<p align="center">
<img width="601" alt="Screenshot 2024-04-16 at 3 20 00 PM"
src="https://github.com/elastic/kibana/assets/56367316/eab07d62-3d3e-4c85-8468-36c3e56c5a99">
</p>

### Checklist

Delete any items that are not applicable to this PR.

- [x] Any text added follows [EUI's writing
guidelines](https://elastic.github.io/eui/#/guidelines/writing), uses
sentence case text and includes [i18n
support](https://github.com/elastic/kibana/blob/main/packages/kbn-i18n/README.md)
- [x]
[Documentation](https://www.elastic.co/guide/en/kibana/master/development-documentation.html)
was added for features that require explanation or tutorials
- [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] [Flaky Test
Runner](https://ci-stats.kibana.dev/trigger_flaky_test_runner/1) was
used on any tests changed
- [x] Any UI touched in this PR is usable by keyboard only (learn more
about [keyboard accessibility](https://webaim.org/techniques/keyboard/))


### For maintainers

- [ ] This was checked for breaking API changes and was [labeled
appropriately](https://www.elastic.co/guide/en/kibana/master/contributing.html#kibana-release-notes-process)

---------

Co-authored-by: Juan Pablo Djeredjian <[email protected]>
@banderror banderror reopened this May 3, 2024
@banderror
Copy link
Contributor

The feature has been live in Serverless for a while.

We also had a discussion on what should we do with 18 prebuilt rules that have max_signals equal to 10000 (context). Summary of the discussion:

  • TRaDE needs this 10k setting in those rules because those rules are "promotion" rules, and should be able to generate as many detection alerts from external alerts as possible. Users will be able to achieve that via tweaking the xpack.alerting.rules.run.alerts.max system setting. Updating max_signals to a lower value is not an option.
  • We agreed that it would be worth documenting that those rules that have max_signals > xpack.alerting.rules.run.alerts.max will be generating at most xpack.alerting.rules.run.alerts.max alerts per execution (1000 by default).
  • We agreed that @approksiu will collect some telemetry on rules hitting the max_signals circuit breaker.
  • We also discussed, with no particular action items:
    • That we could show a dismissable "FYI" callout on the Rule Details page when max_signals of the rule is higher than the system setting.
    • That severity of the Warning status might be too high. @approksiu disagreed with that.
    • That we should give a heads up to the user before they enable or maybe even install a prebuilt rule regarding what needs to be done to make this rule work (setup guide, caveats). This would be a separate UX epic to work on, and @approksiu had some thoughts on that.
    • That we can ask ResponseOps if we could force max_signals and override the system setting with it, for certain rules.
    • That users can override the xpack.alerting.rules.run.alerts.max setting in Cloud ESS.

@dplumlee Let's keep the behavior of the Warning status as is. My concerns did not get support from product, but I'm totally fine with that. I don't think we have any other immediate action items related to this feature (we can always revisit later), so I'll close the ticket. cc @jpdjere

Thanks to everyone involved! We'll track elastic/security-docs#5029 separately.

@approksiu
Copy link
Author

Here's the evidence about rules hitting max_signals and not saving all alerts, based on cloud telemetry. ( I am looking to find the data for on-prem for this issues.) In the past 30 days, we had about 10-12 clusters on average reporting errors when hitting max signals, of which about 5-9 clusters reporting this issues for prebuilt promotion rules. In this data we track > 3200 clusters that have reported data with rule mentioned. The most common rule is External Alerts.

@approksiu
Copy link
Author

After more research here're the latest numbers: doc.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
8.15 candidate enhancement New value added to drive a business result Feature:Rule Creation Security Solution Detection Rule Creation workflow Feature:Rule Details Security Solution Detection Rule Details page Feature:Rule Edit Security Solution Detection Rule Editing workflow 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.15.0
Projects
None yet
Development

Successfully merging a pull request may close this issue.

7 participants