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][Detections] Rule type changes on update are no longer supported #123859

Closed
spong opened this issue Jan 26, 2022 · 6 comments · Fixed by #128283
Closed

[Security Solution][Detections] Rule type changes on update are no longer supported #123859

spong opened this issue Jan 26, 2022 · 6 comments · Fixed by #128283
Assignees
Labels
8.2 candidate considered, but not committed, for 8.2 release bug Fixes for quality problems that affect the customer experience Feature:Detection Rules Security Solution rules and Detection Engine Feature:Rule Management Security Solution Detection Rule Management area impact:high Addressing this issue will have a high level of impact on the quality/strength of our product. Team:Detection Alerts Security Detection Alerts Area Team 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.

Comments

@spong
Copy link
Member

spong commented Jan 26, 2022

Uncovered in testing 8.0.0 rule updates (#123786 (comment)), rule type changes on-update are no longer supported since changing from a single alerting rule type (siem.signals) to separate dedicated types for each security rule, e.g. siem.queryRule and siem.eqlRule.

As @marshallmain detailed:

There are 2 issues at play here: (1) the error that's displaying is coming from the patchRules function, which rejects rule type changes. (2) The changes to the rule types in 8.0 mean that rule type changes on an existing rule won't work, as Garrett pointed out on the issue, because the rule executor logic is now split into separate rule types at the alerting framework level. The result is that even if the type of a rule changes (by using the update_rules_route ) the rule will execute using the old executor function still, and on execution the rule parameter validation will fail so the rule will fail.

We should also update the update_rules_route to reject type changes or handle them in a way that won't cause rule failures on execution.

Proposed change to add back support would likely be "delete the old rule then make a new one with the same rule_id" rather than modifying the existing rule.

Note: Will need to ensure migration of actions/exceptions. See this issue for history around how unsupported exceptions are handled when types are migrated.


Impact

If a type change is performed, the UX is as follows:

Looks like because of the language/type change this rule is failing to update, and must be deleted before it can be updated/re-installed. The tricky thing here is that the error doesn't tell you it's the Interactive Terminal Spawned via Python rule that is failing the update, so it's not clear to the user how they can fix it and the update 1 rule callout will persist and can't be dismissed.

@spong spong added bug Fixes for quality problems that affect the customer experience triage_needed Feature:Detection Rules Security Solution rules and Detection Engine Team:Detections and Resp Security Detection Response Team Team: SecuritySolution Security Solutions Team working on SIEM, Endpoint, Timeline, Resolver, etc. Feature:Rule Management Security Solution Detection Rule Management area Team:Detection Rule Management Security Detection Rule Management Team Team:Detection Alerts Security Detection Alerts Area Team labels Jan 26, 2022
@elasticmachine
Copy link
Contributor

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

@elasticmachine
Copy link
Contributor

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

@marshallmain
Copy link
Contributor

On the Alerts area side we've discussed the implications of allowing rule type changes vs requiring a new rule in order to change the type. The 2 options can be summarized as:

  • Support rule type changes by automatically deleting old rule and replace with new rule with same ID, rule ID, and exceptions (and any other settings that are common to all rule types)
  • Don't support rule type changes at all. Require a new rule to be made with the desired rule type.

Initially I was leaning towards not supporting rule type changes, as it seemed simpler to require a new rule. There's a possibility that some Detection Engine features (e.g. value list exceptions) now and in the future may be supported by some rule types but not others, so allowing arbitrary rule type changes could complicate those features. In addition, rule state like previousStartedAt won't be carried over to the updated rule so gap detection logic won't be able to detect gaps across the time when the type is updated.

However, since users could upgrade from any previous version of prepackaged detection rules to the current version, prepackaged rule type changes that shipped in the past may still need to be applied by future versions of Kibana. In addition, the automated process of "create a new rule with the new type then delete the old one" doesn't seem too risky overall. In the individual rule type executors we can check to see if features not supported by the rule type are being used and display warnings if unsupported features are found on the rule configuration - EQL rules already have a check like this for value list exceptions.

So to summarize, my recommendation would be support rule type changes by updating any rules routes that could change the type of a rule (import, update, patch, update_bulk, patch_bulk, and add_prepackaged?) to check if the rule type is changing and, if so, automatically create a new rule with the new type before deleting the old rule.

@brokensound77
Copy link
Contributor

display warnings if unsupported features are found on the rule configuration - EQL rules already have a check like this for value list exceptions.

Can you reference these - we could probably push some constraints to the rules repo.

Also, would this be something specific that should also be exposed via API

@marshallmain
Copy link
Contributor

marshallmain commented Mar 1, 2022

The example that could happen right now would be user defined value list exceptions that are supported for KQL rules but not for EQL. When the rule gets converted, the value list exceptions would no longer be evaluated for the rule and on execution users would get a warning that the rule has unsupported exceptions. Since the exceptions are user defined I don't know if we'd be able to implement a check on the rule definition itself. Value list exceptions are the only case that I can think of right now that would transfer between rules but have different levels of support on different rule types - but it's possible that other cases could pop up in the future.

Actions, for example, are user defined - right now the same actions are supported across all rule types so it's not an issue, but if a new action is introduced in the future that only supports some rule types that could be an issue if we expect the actions to be moved seamlessly. Similarly in that case, though, at rule execution time we could check to see if unsupported actions are attached to the rule and warn users.

Also, would this be something specific that should also be exposed via API

I think it should be transparent to users as much as possible, so the API would make it appear as though the rule has been updated in place. It's just behind the scenes that the rule was deleted and replaced with a rule that has the same IDs but a different type.

@brokensound77
Copy link
Contributor

awesome, thanks for the update. Doesn't look there would be anything to tighten up within the rules repo wrt that scenario 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
8.2 candidate considered, but not committed, for 8.2 release bug Fixes for quality problems that affect the customer experience Feature:Detection Rules Security Solution rules and Detection Engine Feature:Rule Management Security Solution Detection Rule Management area impact:high Addressing this issue will have a high level of impact on the quality/strength of our product. Team:Detection Alerts Security Detection Alerts Area Team 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.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

7 participants