[Security Solution] Add coverage overview dashboard API contract #159993

maximpn · 2023-06-20T11:02:49Z

Addresses: #158202

Summary

This PR defines Coverage Overview Dashboard API's request and response type definitions and adds UI domain models.

elasticmachine · 2023-06-20T11:12:14Z

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

elasticmachine · 2023-06-20T11:12:16Z

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

xcrzx

Leaving a couple of minor comments, but overall LGTM 👍

xcrzx · 2023-06-21T09:52:03Z

...solution/public/detection_engine/rule_management/logic/coverage_overview/models/rule_data.ts

+export interface CoverageOverviewRuleData {
+  id: string; // rule SO's ids (not ruleId)
+  name: string;
+}


There are two structures in the PR named CoverageOverviewRuleData what's the difference between them?

One defined in response_schema.ts is a DTO and another one here in rule_data.ts is a domain model.

xcrzx · 2023-06-21T09:55:15Z

...solution/public/detection_engine/rule_management/logic/coverage_overview/models/rule_data.ts

+ */
+
+export interface CoverageOverviewRuleData {
+  id: string; // rule SO's ids (not ruleId)


Can we use the RuleObjectId alias here?

xcrzx · 2023-06-21T09:58:05Z

...ution/public/detection_engine/rule_management/logic/coverage_overview/models/mitre_tactic.ts

+
+export interface CoverageOverviewMitreTactic {
+  name: string;
+  reference: string;


Some comments along the structure would be greatly appreciated. For example, it is not completely clear what reference means in this context.

Yes, reference looks too generic. I used the same naming as in mitre_tactics_techniques.ts.

As it causes confusion I've added comments to some fields.

banderror · 2023-06-21T11:44:08Z

@maximpn I'd like to shortly check this PR too if you don't mind waiting for a little bit.

banderror

Most of the comments from my side are relatively minor, but it would be nice to address them before merging this PR.

...lution/common/detection_engine/rule_management/api/rules/coverage_overview/request_schema.ts

...ution/common/detection_engine/rule_management/api/rules/coverage_overview/response_schema.ts

...ution/public/detection_engine/rule_management/logic/coverage_overview/models/mitre_tactic.ts

banderror · 2023-06-21T12:53:16Z

...ution/public/detection_engine/rule_management/logic/coverage_overview/models/mitre_tactic.ts

+
+export interface CoverageOverviewMitreTactic {
+  name: string;
+  reference: string;


banderror · 2023-06-21T12:55:24Z

...solution/public/detection_engine/rule_management/logic/coverage_overview/models/rule_data.ts

+ * 2.0.
+ */
+
+export interface CoverageOverviewRuleData {


~~Could we reuse the CoverageOverviewRuleData from the common folder?~~ Probably not if we want to have an id in the FE's model.

One defined in common/.../response_schema.ts is a DTO and another one here in rule_data.ts is a domain model.

Generally speaking we can reuse it but it will lead to excess id added in the API response which I'd like to avoid.

Great, maybe then we could rename the FE model to disambiguate from the DTO? E.g.

DTO: CoverageOverviewRuleAttributes
Model: CoverageOverviewRule

...ution/public/detection_engine/rule_management/logic/coverage_overview/models/mitre_tactic.ts

banderror · 2023-06-21T16:58:05Z

...lution/common/detection_engine/rule_management/api/rules/coverage_overview/request_schema.ts

+export enum CoverageOverviewRuleActivity {
+  Enabled = 'enabled',
+  Disabled = 'disabled',
+  Available = 'available',
+}
+export const CoverageOverviewRuleActivitySchema = enumeration(
+  'CoverageOverviewRuleActivity',
+  CoverageOverviewRuleActivity
+);
+
+export enum CoverageOverviewRuleSource {
+  Prebuilt = 'prebuilt',
+  Custom = 'custom',
+  Customized = 'customized',
+}
+export const CoverageOverviewRuleSourceSchema = enumeration(
+  'CoverageOverviewRuleSource',
+  CoverageOverviewRuleSource
+);


Nit: commenting on each individual enum option + enums themselves could be helpful too for someone who's out of context. It could be not obvious what, for example, available means, because this word is ambiguous.

banderror · 2023-06-21T16:58:53Z

...lution/common/detection_engine/rule_management/api/rules/coverage_overview/request_schema.ts

+  /**
+   * A search term to filter the response by rule name, index pattern, MITRE ATT&CK tactic or technique
+   */
+  search_term: NonEmptyString,


Nit: adding one or a few @example blah-blah could be helpful.

banderror · 2023-06-21T17:07:42Z

...ty_solution/public/detection_engine/rule_management/models/coverage_overview/mitre_tactic.ts

Nit: for consistency with the other existing subdomains having this folder, let's call it model - in a singular form. It was assumed to mean "domain model" as a whole :)

banderror · 2023-06-21T17:11:35Z

...solution/public/detection_engine/rule_management/models/coverage_overview/mitre_technique.ts

+   */
+  reference: string;
+  /**
+   * A number of covered subtechniques (having as minimum one rule enabled)


Nit: as minimum -> at least

banderror · 2023-06-21T17:12:27Z

...solution/public/detection_engine/rule_management/models/coverage_overview/mitre_technique.ts

+  /**
+   * A number of covered subtechniques (having as minimum one rule enabled)
+   */
+  numOfCoveredSubtechniques: number;


Do we want to treat "covered" as "having at least one installed rule associated with it AND enabled" or just "having at least one installed rule associated with it" regardless of the enabled/disabled state?

Logically speaking having at least one installed rule associated with it AND enabled is the right answer.

I agree, but we should double-check it with @approksiu

banderror

Posted two minor comments:

[Security Solution] Add coverage overview dashboard API contract #159993 (comment)
[Security Solution] Add coverage overview dashboard API contract #159993 (comment)

and a bunch of nits.

Looking good @maximpn, thank you for the fixes 👍 I'm gonna approve it so you don't have to wait for another cycle of review -- please address the rest of the comments at will and merge the PR! 🚀

kibana-ci · 2023-06-22T11:16:35Z

💛 Build succeeded, but was flaky

Buildkite Build
Commit: 73374a6

Failed CI Steps

Security Solution Tests #6

Test Failures

[job] [logs] Security Solution Tests #6 / Detections : Page Filters Alert list is updated when the alerts are updated
[job] [logs] Security Solution Tests #6 / Detections : Page Filters Impact of inputs should recover from invalide kql Query result

Metrics [docs]

Unknown metric groups

ESLint disabled line counts

id	before	after	diff
`enterpriseSearch`	13	15	+2
`securitySolution`	416	420	+4
total			+6

Total ESLint disabled count

id	before	after	diff
`enterpriseSearch`	14	16	+2
`securitySolution`	497	501	+4
total			+6

History

💔 Build #137023 failed 94a7f1cc24c7f948d04bcaf4f4a4bcf5baf0de7b
💛 Build #136459 was flaky b17f5bc1d837fb5d4863651a37d4f4c7447a9b48

To update your PR or re-run it, just comment with:
@elasticmachine merge upstream

cc @maximpn

maximpn · 2023-06-22T12:31:19Z

@banderror thank you for the through review and useful comments. I've addressed all of them so merging back the PR.

**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 flag `detectionsCoverageOverview`. 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 ```json { "coverage": { "TA001": ["e2c9ee90-12d6-11ee-a0ab-c95a1fc4921d"], "T001": ["e2c9ee90-12d6-11ee-a0ab-c95a1fc4921d"], "T001.001": ["e2c9ee90-12d6-11ee-a0ab-c95a1fc4921d"], "TA002": ["e2f459f0-12d6-11ee-a0ab-c95a1fc4921d"], "T002": ["e2f459f0-12d6-11ee-a0ab-c95a1fc4921d"], "T002.002": ["e2f459f0-12d6-11ee-a0ab-c95a1fc4921d"], }, "unmapped_rule_ids": [], "rules_data": { "e2c9ee90-12d6-11ee-a0ab-c95a1fc4921d": { "name": "Some rule", "activity": "disabled" }, "e2f459f0-12d6-11ee-a0ab-c95a1fc4921d": { "name": "Another rule", "activity": "enabled" }, }, } ``` ### How to access the endpoint? Make sure a feature `detectionsCoverageOverview` flag is set in `config/kibana.dev.yml` like ```yaml xpack.securitySolution.enableExperimental: - detectionsCoverageOverview ``` Then access the API via an HTTP client for example `curl` - an empty filter ```sh curl -X POST --user elastic:changeme -H 'Content-Type: application/json' -H 'kbn-xsrf: 123' -d '{}' http://localhost:5601/kbn/internal/detection_engine/rules/_coverage_overview ``` - filter by rule name ```sh curl -X POST --user elastic:changeme -H 'Content-Type: application/json' -H 'kbn-xsrf: 123' -d '{"filter":{"search_term": "rule name"}}' http://localhost:5601/kbn/internal/detection_engine/rules/_coverage_overview ``` - filter by enabled rules ```sh curl -X POST --user elastic:changeme -H 'Content-Type: application/json' -H 'kbn-xsrf: 123' -d '{"filter":{"activity": ["enabled"]}}' http://localhost:5601/kbn/internal/detection_engine/rules/_coverage_overview ``` - filter by prebuilt rules ```sh curl -X POST --user elastic:changeme -H 'Content-Type: application/json' -H 'kbn-xsrf: 123' -d '{"filter":{"source": ["prebuilt"]}}' http://localhost:5601/kbn/internal/detection_engine/rules/_coverage_overview ``` ## Known problems - <del>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.</del> UPD: leave as is for now - <del>Some of the implementation details are similar to `find_rules` endpoint. The difference is that `find_rules` accepts `query` parameter which is a KQL query while `coverage_overview` accepts filter fields and builds up a KQL query under the hood. Passing a prepared KQL query to `coverage_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.</del> UPD: Solved - <del>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 of `io-ts` schemas used to request query validation. It's handled by `buildRouteValidation()` 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 to `buildRouteValidation()`</del> ```ts { "filter": '{"search_term": "rule 1"}' } ``` <del>as you may notice `'{"search_term": "rule 1"}'` is a string so the `io-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](https://github.com/elastic/kibana/blob/main/x-pack/plugins/alerting/server/routes/find_rules.ts#L100C16-L100C27) for `internal/alerting/rules/_find` endpoint, `fields` query parameter can be a JSON array and validation handles it correctly.</del> UPD: discussed with the team and decided that HTTP POST is more convenient for complex filters. - During FTR tests implementation I've noticed the server fails if the second page (10000 - 20000 rules) is requested with an error ``` illegal_argument_exception: Result window is too large, from + size must be less than or equal to: [10000] but was [20000]. See the scroll api for a more efficient way to request large data sets. This limit can be set by changing the [index.max_result_window] index level setting. ``` 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 - [x] Move KQL utility functions into common folder to be shared between FE and BE (done) - Implement stricter filtering to return only searched tactic, technique and sub-technique (leave as is for now) - Use HTTP GET instead of HTTP POST (discussed with the team and decided that HTTP POST is more convenient for complex filters) - Make sure pages above 10000 rules are handled properly (will be addresses in #160698) ### Checklist - [ ] [Documentation](https://www.elastic.co/guide/en/kibana/master/development-documentation.html) was added for features that require explanation or tutorials - [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 scenario

maximpn self-assigned this Jun 20, 2023

maximpn marked this pull request as ready for review June 20, 2023 11:12

maximpn requested a review from a team as a code owner June 20, 2023 11:12

maximpn requested a review from dplumlee June 20, 2023 11:12

maximpn requested a review from xcrzx June 20, 2023 11:12

xcrzx approved these changes Jun 21, 2023

View reviewed changes

banderror self-requested a review June 21, 2023 11:43

banderror requested changes Jun 21, 2023

View reviewed changes

banderror reviewed Jun 21, 2023

View reviewed changes

...ution/public/detection_engine/rule_management/logic/coverage_overview/models/mitre_tactic.ts Outdated Show resolved Hide resolved

maximpn force-pushed the mitre-dashboard-api-contract branch from 80b2bac to 585b307 Compare June 21, 2023 15:22

banderror reviewed Jun 21, 2023

View reviewed changes

banderror approved these changes Jun 21, 2023

View reviewed changes

maximpn added 8 commits June 22, 2023 11:15

add request and response schemas

fa37fde

add mitre dashboard UI models

f49e84d

reorganize coverage overview API typings

3d6de3e

reorganize UI domain models

1589b72

rename a response field

e731059

remove unnecessary files

c49ebb9

get rid of mitre mentions in response type

b2ca1e6

update UI models naming

833b0f3

maximpn added 16 commits June 22, 2023 11:15

convert coverage overview filters to enums

0099f36

allow to specify an array of filters

ab7d65f

use only underscore case

ff54677

update response schema

f965ad5

use more specific name

5ef7cb0

use RuleObjectId

08d7f7c

add explanation comments

2811e98

add explanation comments

cb330fa

add comments to request and response schemas

0772808

move UI domain models level up

708234e

add a type for representing the whole coverage data for the page

e552144

disambiguate DTO and domain model naming

ff92758

rename model folder

0b1f50b

add detailed filter comments

8fa834c

fix wording

002df4b

add a search term example

73374a6

maximpn force-pushed the mitre-dashboard-api-contract branch from 94a7f1c to 73374a6 Compare June 22, 2023 09:15

maximpn merged commit 7186684 into elastic:main Jun 22, 2023

maximpn deleted the mitre-dashboard-api-contract branch June 22, 2023 12:31

kibanamachine added the v8.9.0 label Jun 22, 2023

banderror mentioned this pull request Jun 23, 2023

[Security Solution] Design and implement the API contract #158202

Closed

5 tasks

maximpn mentioned this pull request Jun 25, 2023

[Security Solution] Implement coverage overview dashboard API #160480

Merged

3 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Security Solution] Add coverage overview dashboard API contract #159993

[Security Solution] Add coverage overview dashboard API contract #159993

maximpn commented Jun 20, 2023 •

edited

Loading

elasticmachine commented Jun 20, 2023

elasticmachine commented Jun 20, 2023

xcrzx left a comment

xcrzx Jun 21, 2023

maximpn Jun 21, 2023

xcrzx Jun 21, 2023

xcrzx Jun 21, 2023

banderror Jun 21, 2023

maximpn Jun 21, 2023

banderror commented Jun 21, 2023

banderror left a comment

banderror Jun 21, 2023

banderror Jun 21, 2023 •

edited

Loading

maximpn Jun 21, 2023

banderror Jun 21, 2023 •

edited

Loading

banderror Jun 21, 2023

banderror Jun 21, 2023

banderror Jun 21, 2023

banderror Jun 21, 2023

banderror Jun 21, 2023

maximpn Jun 22, 2023

banderror Jun 22, 2023

banderror left a comment

kibana-ci commented Jun 22, 2023

ESLint disabled line counts

Total ESLint disabled count

maximpn commented Jun 22, 2023

[Security Solution] Add coverage overview dashboard API contract #159993

[Security Solution] Add coverage overview dashboard API contract #159993

Conversation

maximpn commented Jun 20, 2023 • edited Loading

Summary

elasticmachine commented Jun 20, 2023

elasticmachine commented Jun 20, 2023

xcrzx left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

banderror commented Jun 21, 2023

banderror left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

banderror Jun 21, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

banderror Jun 21, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

banderror left a comment

Choose a reason for hiding this comment

kibana-ci commented Jun 22, 2023

💛 Build succeeded, but was flaky

Failed CI Steps

Test Failures

Metrics [docs]

ESLint disabled line counts

Total ESLint disabled count

History

maximpn commented Jun 22, 2023

maximpn commented Jun 20, 2023 •

edited

Loading

banderror Jun 21, 2023 •

edited

Loading

banderror Jun 21, 2023 •

edited

Loading