-
Notifications
You must be signed in to change notification settings - Fork 8.2k
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] Optimize rule fetching endpoints [WIP] #90639
[Security Solution][Detections] Optimize rule fetching endpoints [WIP] #90639
Conversation
const [ruleActions, ruleStatuses] = await Promise.all([ | ||
bulkFetchRuleActionsOfManyRules({ savedObjectsClient, ruleIds }), | ||
bulkFetchLatestStatusesOfManyRules(elasticsearchClient.asCurrentUser, ruleIds), | ||
]); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please review this from the security standpoint (access privileges, isolation (spaces), error handling, potential issues, etc.).
- Is it ok to query
.kibana
index viaelasticsearchClient.asCurrentUser
, or privileges to.kibana
are not guaranteed to all users by default? - I guess it would be better to query
asInternalUser
. Would it be safe in this particular case?
const bulkFetchLatestStatusesOfManyRules = async ( | ||
client: ElasticsearchClient, | ||
ruleIds: string[] | ||
): Promise<IRuleSavedAttributesSavedObjectAttributes[]> => { | ||
// This is not only a performance optimization. If ids is empty, the search | ||
// request to Elasticsearch using terms aggregation will throw a 400 error, | ||
// which is not what we want here. | ||
if (ruleIds.length === 0) { | ||
return []; | ||
} | ||
|
||
const response = await client.search({ | ||
index: '.kibana', | ||
ignore_unavailable: true, | ||
size: 0, | ||
body: { | ||
size: 0, | ||
query: { | ||
bool: { | ||
filter: [ | ||
{ term: { type: 'siem-detection-engine-rule-status' } }, | ||
{ terms: { 'siem-detection-engine-rule-status.alertId': ruleIds } }, | ||
], | ||
}, | ||
}, | ||
aggs: { | ||
rules: { | ||
terms: { | ||
field: 'siem-detection-engine-rule-status.alertId', | ||
size: ruleIds.length, | ||
}, | ||
aggs: { | ||
latest_status: { | ||
top_hits: { | ||
size: 1, | ||
sort: [{ 'siem-detection-engine-rule-status.statusDate': { order: 'desc' } }], | ||
}, | ||
}, | ||
}, | ||
}, | ||
}, | ||
}, | ||
}); | ||
|
||
const ruleBuckets = response.body.aggregations?.rules?.buckets ?? []; | ||
const statusHits = ruleBuckets.map((bucket: any) => { | ||
const [firstHit] = bucket?.latest_status?.hits?.hits ?? []; | ||
return firstHit ?? null; | ||
}); | ||
const statuses = statusHits.map((hit: any) => { | ||
return hit?._source?.['siem-detection-engine-rule-status'] ?? null; | ||
}); | ||
|
||
return statuses.filter(Boolean) as IRuleSavedAttributesSavedObjectAttributes[]; | ||
}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please review this from the security standpoint (access privileges, isolation (spaces), error handling, potential issues, etc.).
Note that:
- Detection rules are saved objects with
"type": "alert"
and"alert.alertTypeId": "siem.signals"
. - Rule statuses are saved objects with
"type": "siem-detection-engine-rule-status"
. - Rule statuses have "foreign keys" pointing to detections rules via their ids. If a rule has an
"_id": "alert:b921e020-6a13-11eb-bf61-5f56ac457c9b"
, its status object will contain a foreign key"alertId": "b921e020-6a13-11eb-bf61-5f56ac457c9b"
in its saved object attributes. - In this endpoint handler
ruleIds
are not coming from request parameters. We retrieve them via alerting client which uses saved objects client. Which should handle spaces and RBAC for us correctly (I guess?). - See more notes on our data model in the PR description.
} | ||
|
||
const response = await client.search({ | ||
index: '.kibana', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
TODO: Kibana index name should be retrieved from the config.
d57a937
to
471dab6
Compare
471dab6
to
a7836c6
Compare
💔 Build Failed
Failed CI StepsTest FailuresKibana Pipeline / general / "before all" hook for "should open a modal".Open timeline Open timeline modal "before all" hook for "should open a modal"Stack Trace
Metrics [docs]
History
To update your PR or re-run it, just comment with: |
We haven't got an approval from Kibana platform security team, unfortunately.
We decided to go a different way by changing the data model of our rule execution statuses. The idea is to avoid "joins", queries that require sorting by status date, aggregations - any of these - when we need to load the rule management table, and so fetch N rules with their N current statuses in 1-2, but not N queries to Elasticsearch. We'll start by evaluating Alerting's APIs for these needs, in particular the "event log" API. Closing this one. |
Needed for: #89877
Summary
In order to be able to implement in-memory rule management table, we need these endpoints to be fast and scalable in terms of the number of requested/existing rules:
api/detection_engine/rules/_find
api/detection_engine/rules/_find_statuses
api/detection_engine/rules/prepackaged/_status
This PR attempts to do that.
Details
Currently, the endpoints mentioned above generate ~2N requests to Elasticsearch. I guess, we did this partly because of current limitations of saved object client and alerting client APIs.
Instead, to be able to fetch for example N latest statuses of N rules, and do it in one query to Elasticsearch instead of N queries, we need to be able to do
term
+terms
queries and aggregations.In this PR I'm trying to use
ElasticsearchClient
directly. I'm building a custom query, executing it viaElasticsearchClient.search()
API and manually normalizing the response to a desired format of our domain entities. Search request directly queries.kibana
index.This naive hacky-dirty implementation works locally and reduces the
api/detection_engine/rules/_find
roundtrip time from 2-5 seconds down to 300 - 1100 ms when requesting 500 rules.Notes on data model
We have detections rules which are implemented as "alerts" in terms of alerting framework, and rule statuses and rule actions implemented as what we call "sidecar" saved objects.
"type": "alert"
and"alert.alertTypeId": "siem.signals"
."type": "siem-detection-engine-rule-status"
."type": "siem-detection-engine-rule-actions"
.Rule statuses and actions objects have "foreign keys" pointing to detections rules via their ids. E.g. if a rule has an
"_id": "alert:b921e020-6a13-11eb-bf61-5f56ac457c9b"
, its status object will contain a foreign key"alertId": "b921e020-6a13-11eb-bf61-5f56ac457c9b"
in its saved object attributes.Association types:
Example for rule with id
b921e020-6a13-11eb-bf61-5f56ac457c9b
:Checklist
Delete any items that are not applicable to this PR.
For maintainers