[Security Solution] Remaining bug in bulk editing index patterns of rules with a data view

[banderror]

Related to: #136006
Docs: elastic/security-docs#1832 (comment)

Summary

There's a leftover bug in bulk editing index patterns of rules with a data view, and the logic of applyBulkActionEditToRuleParams and validation that we do in validateMutatedParams is not entirely correct and not full. Also, this does not correspond to how we're going to document the overwrite_data_views parameter in elastic/security-docs#1832 (comment):

If overwrite_data_views is set to true - even rules using data views will be updated to include the index changes and the data_view_id will be removed
If overwrite_data_views is set to false - no updates or changes will be made to rules using data views

This is not how it works in the code right now, and you can verify this with the following steps:

1. Create a rule with a logs-* data view.
2. Bulk add foo-* and logs-* index patterns to it with overwrite_data_views = false.
3. Notice that now the rule has both the logs-* data view and the ['foo-*', 'logs-*'] index patterns. This does not correspond to the statement that If overwrite_data_views is set to false - no updates or changes will be made to rules using data views
4. Now, bulk delete the logs-* index pattern from the rule.
5. Notice that it gets deleted despite the fact that in the bulk deletion form we say If you have selected rules which depend on a data view this action will not have any effect on those rules..
6. Finally, bulk delete the remaining foo-* index pattern from the rule.
7. You will get a Mutated params invalid: Index patterns can't be empty error although this is a rule with a data view and we said If you have selected rules which depend on a data view this action will not have any effect on those rules..

Proposed fix

The behavior described above is caused by the fact that a rule Saved Object can have both a non-empty data view and a non-empty array of index patterns at the same time, for instance:

{
  data_view_id: "logs-*",
  index: ["foo-*", "logs-*"],
  // other rule params...
}

I think we need to enforce the following invariants in the code:

• Every rule must have either a data view or index patterns. No rule can have both.
• Every modification to a rule (such as saving or bulk editing) ensures that the above invariant is maintained.
• By default, bulk editing index patterns has no effect on rules with data views whatsoever OR returns a descriptive error. Unless the user explicitly requests to apply changes to rules with data views (overwrite_data_views: true).

The following places in the codebase should be updated:

• applyBulkActionEditToRuleParams function
• validateMutatedParams of all our rule types: validateIndexPatterns(mutatedRuleParams.index) should be changed to something like validateDataSource(mutatedRuleParams.index, mutatedRuleParams.dataViewId)

[elasticmachine]

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

[elasticmachine]

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

[vitaliidm]

validateMutatedParams of all our rule types: validateIndexPatterns(mutatedRuleParams.index) should be changed to something like validateDataSource(mutatedRuleParams.index, mutatedRuleParams.dataViewId)

@banderror , @yctercero, @dhurley14 is this something that we still would like to implement, given changes in #138448, where in some of the tests, rules have both index patterns and data view ?

If we still would like to enforce invariant on index and data view, I think it should be enforced somewhere on rule schema level if possible. So, that would prevent, creating rule with both index and data view in the first place, or update rule in a way: it would have both properties

[yctercero]

No objections to having added XOR validations. However, I think this would be considered a breaking change, so it may require review?

We unfortunately don't have resources to do this work. So may be up to @peluja1012 to prioritize?

[banderror]

is this something that we still would like to implement, given changes in #138448, where in some of the tests, rules have both index patterns and data view ?

@vitaliidm Great catch. We have this test, but there shouldn't be rules containing both index patterns and data_view_id, because:

• when a rule is created or edited, it is saved with either index or data_view_id
• [Security Solution][Bulk actions]- Fix bulk actions data views behavior #138448 added code to the bulk action endpoint that prevents the user from updating a rule with both index and data_view_id

If that is true (@yctercero please correct me if it's not), users should not end up having any rules with both index and data_view_id. This means doing this should be safe and shouldn't be a breaking change:

validateMutatedParams of all our rule types: validateIndexPatterns(mutatedRuleParams.index) should be changed to something like validateDataSource(mutatedRuleParams.index, mutatedRuleParams.dataViewId)

Enforcing the same invariant at the schema level would probably require much more effort. If we want to go that path, I think we should open a separate ticket for that.

[vitaliidm]

#138448 added code to the bulk action endpoint that prevents the user from updating a rule with both index and data_view_id

changes in this PR prevents edit that creates both index and data view properties by calling bulk API.

when a rule is created or edited, it is saved with either index or data_view_id

But, it still possible to have rule with both properties by using create/update API. Here is a simple example(similarly you can see it in test itself)

curl --location --request POST 'http://localhost:5601/kbn/api/detection_engine/rules' \
--header 'kbn-xsrf: kibana' \
--header 'Authorization: Basic ZWxhc3RpYzpjaGFuZ2VtZQ==' \
--header 'Content-Type: application/json' \
--data-raw '{
    "type": "query",
    "filters": [],
    "language": "kuery",
    "query": "*:*",
    "data_view_id": "logs-*",
    "author": [],
    "false_positives": [],
    "references": [],
    "risk_score": 21,
    "risk_score_mapping": [],
    "severity": "low",
    "severity_mapping": [],
    "threat": [],
    "name": "test-123",
    "description": "test-123",
    "tags": [],
    "license": "",
    "interval": "5m",
    "from": "now-360s",
    "to": "now",
    "meta": {
        "from": "1m",
        "kibana_siem_app_url": "http://localhost:5601/kbn/app/security"
    },
    "actions": [],
    "enabled": false,
    "throttle": "no_actions",
    "index": ["test-*"]
}'

So, the proper fix would be to change update/create API, so it won't allow having 2 properties at the same time. As bulk edit already doesn't allow you to do it, unless you had these properties prior to editing. See the same test I mentioned, where rule is created with both properties.
Just adding this check in bulk edit won't prevent it.

I think we need to enforce it for all APIs, if it's not considered a breaking change.
There might be few ways to do it:

1. Enforce on schema level
2. Set usage of validateMutatedParams for the rest of alerting framework methods, to enforce it similarly as for bulk edit API

[yctercero]

++ what @vitaliidm mentioned. There is nothing API side preventing a user from creating a rule with both data view and index pattern. Making the change, we'd have to file it as breaking unfortunately as it breaks the existing contract in that a user submitting a rule with both (who I'm not sure why they would be doing this) would now start getting an error back.

[banderror]

@vitaliidm I generally agree with your comments, that makes sense to me 👍

The first step would be to disallow creating a rule with both index and data_view_id fields. I imagine this could be done in different ways. When both fields are passed in request parameters to our rule CRUD endpoints, we could:

• 1st option: return an error
• 2nd option: reset index to an empty array when data_view_id is not blank

I'm leaning towards the 2nd option. Users in theory could have already created some rules with both index and data_view_id if they used the API directly. If we go with the 1st option and don't migrate their rules, they would not be able to edit the affected rules. And AFAIK there's no way to migrate rules at the plugin setup or start phases - which means we'd need to do it like it was done with migrating legacy rule actions, which is conceptually similar to the 2nd option (migrate-on-change). @vitaliidm please correct me if that assumption about rule migration is not correct.

If this is done, we could additionally call validateMutatedParams from the rest of the RulesClient methods, to enforce the invariant. @vitaliidm would there be any potential problems or risks with doing that?

Enforcing it further in the schema (e.g. in the rule "read" schema) might be not an easy thing to do -- I'd wait with making decisions on that until the issues above are resolved.

Making the change, we'd have to file it as breaking unfortunately as it breaks the existing contract in that a user submitting a rule with both (who I'm not sure why they would be doing this) would now start getting an error back.

@yctercero I'm not sure I'm on board with that. Firstly, if we go with the 2nd option above, the API won't start returning errors. Secondly, even if it will, have we explicitly stated in the docs that the user can create a rule with both index and data_view_id, and explained why and what would it mean? I couldn't find the data_view_id field mentioned in https://www.elastic.co/guide/en/security/current/rules-api-create.html at all. I don't think it's a contract until it's documented and explained. We're just going to fix a bug in the API behavior.

[yctercero]

@banderror I'm going off of what core considers to be breaking here - https://docs.elastic.dev/kibana-dev-docs/standards#what-constitutes-a-breaking-change . Reading that, I would consider it breaking as the same inputs would result in a different output now. 🤷‍♀️

I don't know if there's any consideration given to whether or not it was documented or how "fresh" the change is. I think prior to core efforts to be more strict on breaking changes, this wouldn't of mattered, but if we're going by the book, it might be.