Open banderror opened 2 years ago
Pinging @elastic/security-detections-response (Team:Detections and Resp)
Pinging @elastic/security-solution (Team: SecuritySolution)
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 https://github.com/elastic/kibana/pull/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
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?
is this something that we still would like to implement, given changes in https://github.com/elastic/kibana/pull/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:
index
or data_view_id
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.
https://github.com/elastic/kibana/pull/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:
validateMutatedParams
for the rest of alerting framework methods, to enforce it similarly as for bulk edit API++ 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.
@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:
index
to an empty array when data_view_id
is not blankI'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.
@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.
Related to: https://github.com/elastic/kibana/issues/136006 Docs: https://github.com/elastic/security-docs/issues/1832#issuecomment-1208411526
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 invalidateMutatedParams
is not entirely correct and not full. Also, this does not correspond to how we're going to document theoverwrite_data_views
parameter in https://github.com/elastic/security-docs/issues/1832#issuecomment-1208411526:This is not how it works in the code right now, and you can verify this with the following steps:
logs-*
data view.foo-*
andlogs-*
index patterns to it withoverwrite_data_views = false
.logs-*
data view and the['foo-*', 'logs-*']
index patterns. This does not correspond to the statement thatIf overwrite_data_views is set to false - no updates or changes will be made to rules using data views
logs-*
index pattern from the rule.If you have selected rules which depend on a data view this action will not have any effect on those rules.
.foo-*
index pattern from the rule.Mutated params invalid: Index patterns can't be empty
error although this is a rule with a data view and we saidIf 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:
I think we need to enforce the following invariants in the code:
overwrite_data_views: true
).The following places in the codebase should be updated:
applyBulkActionEditToRuleParams
functionvalidateMutatedParams
of all our rule types:validateIndexPatterns(mutatedRuleParams.index)
should be changed to something likevalidateDataSource(mutatedRuleParams.index, mutatedRuleParams.dataViewId)