-
Notifications
You must be signed in to change notification settings - Fork 8.3k
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] Implement coverage overview dashboard API #160480
Conversation
c5f7f7a
to
5a82ce1
Compare
Pinging @elastic/security-detections-response (Team:Detections and Resp) |
Pinging @elastic/security-solution (Team: SecuritySolution) |
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.
Thanks for your implementation, @maximpn! 🎉 I've tested the PR locally and the expected path works as it should. However, I noticed that Kibana crashes when there are over 10,000 detection rules. I think we should address this issue as it appears quite critical. The overall implementation seems sound, with my suggestions primarily relating to reducing code duplication.
...ction_engine/rule_management/api/rules/coverage_overview/handle_coverage_overview_request.ts
Outdated
Show resolved
Hide resolved
...ty_solution/server/lib/detection_engine/rule_management/api/rules/coverage_overview/route.ts
Outdated
Show resolved
Hide resolved
.../detection_engine/rule_management/api/rules/coverage_overview/utils/convert_source_to_kql.ts
Outdated
Show resolved
Hide resolved
...ction_engine/rule_management/api/rules/coverage_overview/utils/convert_search_term_to_kql.ts
Outdated
Show resolved
Hide resolved
x-pack/test/detection_engine_api_integration/basic/tests/coverage_overview.ts
Outdated
Show resolved
Hide resolved
x-pack/test/detection_engine_api_integration/basic/tests/coverage_overview.ts
Outdated
Show resolved
Hide resolved
x-pack/test/detection_engine_api_integration/basic/tests/coverage_overview.ts
Outdated
Show resolved
Hide resolved
.../detection_engine/rule_management/api/rules/coverage_overview/utils/convert_filter_to_kql.ts
Outdated
Show resolved
Hide resolved
...ty_solution/server/lib/detection_engine/rule_management/api/rules/coverage_overview/route.ts
Show resolved
Hide resolved
..._engine/rule_management/api/rules/coverage_overview/handle_coverage_overview_request.test.ts
Outdated
Show resolved
Hide resolved
x-pack/test/detection_engine_api_integration/basic/tests/coverage_overview.ts
Outdated
Show resolved
Hide resolved
5a82ce1
to
9af1945
Compare
@xcrzx thank you for the thorough review 🙏 I've addressed all your comments and the PR is ready for the second review. |
💛 Build succeeded, but was flaky
Failed CI StepsTest Failures
Metrics [docs]Module Count
Async chunks
Unknown metric groupsESLint disabled line counts
Total ESLint disabled count
History
To update your PR or re-run it, just comment with: cc @maximpn |
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.
Thank you for addressing my comments and the additional cleanup, @maximpn 👍
Just a side note - we've discussed how we might inform users that the returned data is capped at 10,000 rules. If they have more than 10,000 rules, they'll need to narrow down their search request to view complete coverage. So we might need to return the total count of matched rules from the API. However, this seems like a minor enhancement that can be implemented at a later stage.
…aracters (#163767) **Fixes: #163692 ## Summary This PR fixes rules table filtering for tags with special characters like colon `category:tag`. We didn't have this bug in `8.9` it was introduced by #160480 in `8.10` scope. ## Details Coverage Overview Dashboard required very similar rule filtering functionality as rule table has so #160480 moved the common logic to `common` folder and started using `escapeKuery` from `@kbn/es-query` instead of custom `escapeKuery` defined in Security Solution in `x-pack/plugins/security_solution/public/common/lib/kuery/index.ts`. The difference is the first function escapes all the special characters `\():<>"*` but the second escapes only quotes but the value must be enclosed in quotes to be passed on. Both ways are correct according to [docs](https://www.elastic.co/guide/en/kibana/current/kuery-query.html). So escaping all the special characters and enclosing the value in quotes breaks rules search. ### How was it fixed? Escaping related code in `x-pack/plugins/security_solution/common/utils/kql.ts` (`convertRulesFilterToKQL` in particular) was moved to `x-pack/plugins/security_solution/common/detection_engine/rule_management/rule_filtering.ts` as Coverage Overview Dashboard API endpoint and rules table UI share the same KQL helpers and refactored along the way to be much more transparent. ### Checklist - [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
…aracters (elastic#163767) **Fixes: elastic#163692 ## Summary This PR fixes rules table filtering for tags with special characters like colon `category:tag`. We didn't have this bug in `8.9` it was introduced by elastic#160480 in `8.10` scope. ## Details Coverage Overview Dashboard required very similar rule filtering functionality as rule table has so elastic#160480 moved the common logic to `common` folder and started using `escapeKuery` from `@kbn/es-query` instead of custom `escapeKuery` defined in Security Solution in `x-pack/plugins/security_solution/public/common/lib/kuery/index.ts`. The difference is the first function escapes all the special characters `\():<>"*` but the second escapes only quotes but the value must be enclosed in quotes to be passed on. Both ways are correct according to [docs](https://www.elastic.co/guide/en/kibana/current/kuery-query.html). So escaping all the special characters and enclosing the value in quotes breaks rules search. ### How was it fixed? Escaping related code in `x-pack/plugins/security_solution/common/utils/kql.ts` (`convertRulesFilterToKQL` in particular) was moved to `x-pack/plugins/security_solution/common/detection_engine/rule_management/rule_filtering.ts` as Coverage Overview Dashboard API endpoint and rules table UI share the same KQL helpers and refactored along the way to be much more transparent. ### Checklist - [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 (cherry picked from commit 92ff716) # Conflicts: # x-pack/plugins/security_solution/public/detection_engine/rule_management/api/api.test.ts
…cial characters (#163767) (#164956) # Backport This will backport the following commits from `main` to `8.10`: - [[Security Solution] Fix rules table filtering by tags with special characters (#163767)](#163767) <!--- Backport version: 8.9.8 --> ### Questions ? Please refer to the [Backport tool documentation](https://github.com/sqren/backport) <!--BACKPORT [{"author":{"name":"Maxim Palenov","email":"maxim.palenov@elastic.co"},"sourceCommit":{"committedDate":"2023-08-28T09:20:31Z","message":"[Security Solution] Fix rules table filtering by tags with special characters (#163767)\n\n**Fixes: https://github.com/elastic/kibana/issues/163692**\r\n\r\n## Summary\r\n\r\nThis PR fixes rules table filtering for tags with special characters like colon `category:tag`.\r\n\r\nWe didn't have this bug in `8.9` it was introduced by #160480 in `8.10` scope.\r\n\r\n## Details\r\n\r\nCoverage Overview Dashboard required very similar rule filtering functionality as rule table has so #160480 moved the common logic to `common` folder and started using `escapeKuery` from `@kbn/es-query` instead of custom `escapeKuery` defined in Security Solution in `x-pack/plugins/security_solution/public/common/lib/kuery/index.ts`. The difference is the first function escapes all the special characters `\\():<>\"*` but the second escapes only quotes but the value must be enclosed in quotes to be passed on. Both ways are correct according to [docs](https://www.elastic.co/guide/en/kibana/current/kuery-query.html). So escaping all the special characters and enclosing the value in quotes breaks rules search.\r\n\r\n### How was it fixed?\r\n\r\nEscaping related code in `x-pack/plugins/security_solution/common/utils/kql.ts` (`convertRulesFilterToKQL` in particular) was moved to `x-pack/plugins/security_solution/common/detection_engine/rule_management/rule_filtering.ts` as Coverage Overview Dashboard API endpoint and rules table UI share the same KQL helpers and refactored along the way to be much more transparent.\r\n\r\n### Checklist\r\n\r\n- [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","sha":"92ff716b750c2a47bf55ff280e0621e553f1f019","branchLabelMapping":{"^v8.11.0$":"main","^v(\\d+).(\\d+).\\d+$":"$1.$2"}},"sourcePullRequest":{"labels":["bug","release_note:skip","impact:high","Team:Detections and Resp","Team: SecuritySolution","Feature:Rule Management","Team:Detection Rule Management","v8.10.0","v8.11.0"],"number":163767,"url":"https://github.com/elastic/kibana/pull/163767","mergeCommit":{"message":"[Security Solution] Fix rules table filtering by tags with special characters (#163767)\n\n**Fixes: https://github.com/elastic/kibana/issues/163692**\r\n\r\n## Summary\r\n\r\nThis PR fixes rules table filtering for tags with special characters like colon `category:tag`.\r\n\r\nWe didn't have this bug in `8.9` it was introduced by #160480 in `8.10` scope.\r\n\r\n## Details\r\n\r\nCoverage Overview Dashboard required very similar rule filtering functionality as rule table has so #160480 moved the common logic to `common` folder and started using `escapeKuery` from `@kbn/es-query` instead of custom `escapeKuery` defined in Security Solution in `x-pack/plugins/security_solution/public/common/lib/kuery/index.ts`. The difference is the first function escapes all the special characters `\\():<>\"*` but the second escapes only quotes but the value must be enclosed in quotes to be passed on. Both ways are correct according to [docs](https://www.elastic.co/guide/en/kibana/current/kuery-query.html). So escaping all the special characters and enclosing the value in quotes breaks rules search.\r\n\r\n### How was it fixed?\r\n\r\nEscaping related code in `x-pack/plugins/security_solution/common/utils/kql.ts` (`convertRulesFilterToKQL` in particular) was moved to `x-pack/plugins/security_solution/common/detection_engine/rule_management/rule_filtering.ts` as Coverage Overview Dashboard API endpoint and rules table UI share the same KQL helpers and refactored along the way to be much more transparent.\r\n\r\n### Checklist\r\n\r\n- [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","sha":"92ff716b750c2a47bf55ff280e0621e553f1f019"}},"sourceBranch":"main","suggestedTargetBranches":["8.10"],"targetPullRequestStates":[{"branch":"8.10","label":"v8.10.0","labelRegex":"^v(\\d+).(\\d+).\\d+$","isSourceBranch":false,"state":"NOT_CREATED"},{"branch":"main","label":"v8.11.0","labelRegex":"^v8.11.0$","isSourceBranch":true,"state":"MERGED","url":"https://github.com/elastic/kibana/pull/163767","number":163767,"mergeCommit":{"message":"[Security Solution] Fix rules table filtering by tags with special characters (#163767)\n\n**Fixes: https://github.com/elastic/kibana/issues/163692**\r\n\r\n## Summary\r\n\r\nThis PR fixes rules table filtering for tags with special characters like colon `category:tag`.\r\n\r\nWe didn't have this bug in `8.9` it was introduced by #160480 in `8.10` scope.\r\n\r\n## Details\r\n\r\nCoverage Overview Dashboard required very similar rule filtering functionality as rule table has so #160480 moved the common logic to `common` folder and started using `escapeKuery` from `@kbn/es-query` instead of custom `escapeKuery` defined in Security Solution in `x-pack/plugins/security_solution/public/common/lib/kuery/index.ts`. The difference is the first function escapes all the special characters `\\():<>\"*` but the second escapes only quotes but the value must be enclosed in quotes to be passed on. Both ways are correct according to [docs](https://www.elastic.co/guide/en/kibana/current/kuery-query.html). So escaping all the special characters and enclosing the value in quotes breaks rules search.\r\n\r\n### How was it fixed?\r\n\r\nEscaping related code in `x-pack/plugins/security_solution/common/utils/kql.ts` (`convertRulesFilterToKQL` in particular) was moved to `x-pack/plugins/security_solution/common/detection_engine/rule_management/rule_filtering.ts` as Coverage Overview Dashboard API endpoint and rules table UI share the same KQL helpers and refactored along the way to be much more transparent.\r\n\r\n### Checklist\r\n\r\n- [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","sha":"92ff716b750c2a47bf55ff280e0621e553f1f019"}}]}] BACKPORT-->
Addresses: #158238
Summary
This PR adds Coverage Overview API endpoint necessary to implement Coverage Overview Dashboard.
Details
The Coverage Overview API implementation is represented by one HTTP POST internal route
/internal/detection_engine/rules/_coverage_overview
hidden by a feature flagdetectionsCoverageOverview
. It returns response in the format defined in #159993.Implementation is done in a quite simple way. It basically just fetches all the rules in chunks and adds them to appropriate MITRE ATT&CK category buckets depending on the assigned categories. The chunk size has been chosen to be
10000
as it's the default limit.At the current stage the API doesn't handle available rules which means it doesn't return available rules in the response.
Sample response containing two rules looks like
How to access the endpoint?
Make sure a feature
detectionsCoverageOverview
flag is set inconfig/kibana.dev.yml
likeThen access the API via an HTTP client for example
curl
Known problems
Filtering by a tactic name doesn't guarantee the other tactics, techniques and sub-techniques will be filtered out. As one rule may be assigned to more than one tactic, technique and sub-technique filtering such rules by one tactic will lead to only rules assigned to the desired tactic be processed. But the result will include all the tactics, techniques and sub-techniques assigned to that rules.UPD: leave as is for now
Some of the implementation details are similar tofind_rules
endpoint. The difference is thatfind_rules
acceptsquery
parameter which is a KQL query whilecoverage_overview
accepts filter fields and builds up a KQL query under the hood. Passing a prepared KQL query tocoverage_overview
looks too permissive and can lead to undesired filtering results. Some of KQL query building code is common and can be reused between FE and BE.UPD: Solved
One may ask why using an HTTP POST request instead of HTTP GET. In fact HTTP POST is needed only for convenience to send a JSON request query in the request body similar to GraphQL approach but it looks rather an overkill. One of the main reasons why HTTP POST is used is the limitation ofio-ts
schemas used to request query validation. It's handled bybuildRouteValidation()
which doesn't parse input parameters. For example there is a request with a query string/internal/detection_engine/rules/_coverage_overview?filter={"search_term": "rule 1"}
, it's handled and the following object gets passed tobuildRouteValidation()
as you may notice'{"search_term": "rule 1"}'
is a string so theio-ts
schema validation fails while the request looks correct. In contrast a similar@kbn/config-schema
schema used instead for the request query validation handles it correctly. As the reference it works here forinternal/alerting/rules/_find
endpoint,fields
query parameter can be a JSON array and validation handles it correctly.UPD: discussed with the team and decided that HTTP POST is more convenient for complex filters.
There is a chance it can fail with the same error in prod.
UPD: The problem in reproducible in prod. To avoid a server crash the endpoint doesn't handle more than 10k rules. The problem will be addressed in #160698.
Posible improvements
Checklist