[Security Solution] Change query builder so N exception items don't nest N levels deep #72224

marshallmain · 2020-07-17T05:02:26Z

Summary

Currently, the usage of buildEsQuery with a single large KQL string causes a deeply nested Elasticsearch query to be generated. The builder generates an AST from a boolean expression of the form A OR B OR C OR D ..., and rather than detecting that A, B, C, and D can be put in an array in a single should block, it creates a set of nested should blocks as deep as the number of expressions being OR'd together. In our case, the number of expressions being OR'd is the number of exception items. This eventually causes Elasticsearch to fail on the search when the nesting becomes too deep.

This PR changes the logic so that every exception item being OR'd together will be in the same array without unnecessary nesting. A further improvement would be using similar logic on the entries within an exception, which can suffer from the same problem with the AST, however the number of entries per exception has a much lower practical limit.

Additional refactoring for next PR:

Remove unnecessary language option from buildExceptionListQueries
Extract building of exceptionFilter to separate function
Handle potential error case if maxClauseCount is below 1024 for some reason
Potentially increase the exception list size limit from 5000 https://github.com/elastic/kibana/blob/master/x-pack/plugins/security_solution/server/lib/detection_engine/signals/utils.ts#L186

Manual test process:

Add ~4000 endpoint exception list items by modifying this script
Create a detection alert
Add an exception that allowlists the detection alert
Observe that no more detection alerts are created that match the new exception

For maintainers

This was checked for breaking API changes and was labeled appropriately

madirey · 2020-07-19T16:13:22Z

x-pack/plugins/security_solution/common/detection_engine/build_exceptions_query.test.ts

    });

-    describe('when "exclude" is false', () => {


Removed because exclude is no longer used. 👍

madirey · 2020-07-19T16:47:37Z

x-pack/plugins/security_solution/common/detection_engine/get_query_filter.ts

+    dateFormatTZ: 'Zulu',
+  };
+
+  const enabledFilters = ((filters as unknown) as Filter[]).filter((f) => !isFilterDisabled(f));


It might be good to have some extra validation here. We could be explicit about which fields we need Filter to contain, since it's so loosely defined atm. At this point, enabledFilters could really be anything and we won't have a good idea of why things break, if they do. I know this PR is just modifying existing code, but maybe could look at this as a follow-up.

Yeah that would be better. I know getQueryFilter is used in a few different places, I haven't looked into why it needs to pass in Partial<Filter> instead of a complete Filter`, but it seems like something to fix.

madirey · 2020-07-19T16:51:43Z

x-pack/plugins/security_solution/common/detection_engine/get_query_filter.ts

-
-  const enabledFilters = ((filters as unknown) as Filter[]).filter((f) => !isFilterDisabled(f));
-  return buildEsQuery(indexPattern, queries, enabledFilters, config);
+  return buildEsQuery(indexPattern, initialQuery, enabledFilters, config);


This logic seems sound 👍

marshallmain · 2020-07-20T18:37:49Z

@elasticmachine merge upstream

FrankHassanabad

LGTM (But I'm not an expert in this area or been that involved so caveat emptor with my 👍 )

madirey · 2020-07-22T00:36:53Z

x-pack/plugins/security_solution/common/detection_engine/get_query_filter.ts

@@ -48,27 +49,76 @@ export const getQueryFilter = (
   */
  const exceptionQueries = buildExceptionListQueries({ language: 'kuery', lists });
  if (exceptionQueries.length > 0) {
+    // Assume that `indices.query.bool.max_clause_count` is at least 1024 (the default value),


nice comment 👍

yctercero

LGTM! Focused mainly on the changes to the exceptions build query file. Was gonna say to remove the language param used to build the exceptions since we should just always build them in KQL (since it supports nested), but you already have that in a TODO. Thanks so much for the work here!

yctercero · 2020-07-22T00:53:05Z

x-pack/plugins/security_solution/common/detection_engine/build_exceptions_query.ts

  language: Language;
 }): string => {
-  const { field, entries } = item;
+  const { field, entries: subentries } = entry;


I like the subentries will look to use that in future PRs for clarity on what it refers to.

dhurley14 · 2020-07-22T15:05:41Z

x-pack/plugins/security_solution/common/detection_engine/get_query_filter.ts

+    for (let index = 0; index < exceptionQueries.length; index += chunkSize) {
+      const exceptionQueriesChunk = exceptionQueries.slice(index, index + chunkSize);
+      const esQueryChunk = buildEsQuery(indexPattern, exceptionQueriesChunk, [], config);
+      const filterChunk: Filter = {
+        meta: {
+          alias: null,
+          negate: false,
+          disabled: false,
+        },
+        query: {
+          bool: {
+            should: esQueryChunk.bool.filter,
+          },
+        },
+      };
+      chunkedFilters.push(filterChunk);
+    }


Is it possible to rewrite this with a map? Tendency to prefer a functional style for situations like this.

dhurley14 · 2020-07-22T15:05:58Z

x-pack/plugins/security_solution/common/detection_engine/get_query_filter.test.ts

+        });
+      });
+
+      test('it should work with a list with multiple items', () => {


nit: I think this test case could have a more descriptive title. Something like, "exception list with multiple exception items with the same entry".

dhurley14 · 2020-07-22T15:06:04Z

x-pack/plugins/security_solution/common/detection_engine/get_query_filter.test.ts

+      });
+    });
+
+    test('it should work with a list with multiple items', () => {


nit: I think this test case could have a more descriptive title. Something like, "exception list with multiple exception items with the same entry".

dhurley14 · 2020-07-22T15:06:10Z

x-pack/plugins/security_solution/common/detection_engine/build_exceptions_query.ts

-      {
-        query: formattedQuery,
+    return exceptionItems.map((exceptionItem) => {
+      return {


nit: we can probably remove this return statement.

dhurley14 · 2020-07-22T15:06:14Z

x-pack/plugins/security_solution/common/detection_engine/build_exceptions_query.ts

+  const exceptionItemEntries = entries.map((entry) => {
+    return buildEntry({ entry, language });
+  });


nit: I think we can remove the return statement here and rewrite as

const exceptionItemEntries = entries.map((entry) => buildEntry({ entry, language }));

…est N levels deep (elastic#72224) * Change query builder so N exceptions don't nest N levels deep * Fix tests and clarify function naming * Rename evaluateEntry to buildEntry for consistency * Remove duplicate tests * more test fixes * Add tests with multiple exception list items * Chunk exception list items in query to support up to 1000000 Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com>

…est N levels deep (#72224) (#72867) * Change query builder so N exceptions don't nest N levels deep * Fix tests and clarify function naming * Rename evaluateEntry to buildEntry for consistency * Remove duplicate tests * more test fixes * Add tests with multiple exception list items * Chunk exception list items in query to support up to 1000000 Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com> Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com>

…est N levels deep (#72224) (#72868) * Change query builder so N exceptions don't nest N levels deep * Fix tests and clarify function naming * Rename evaluateEntry to buildEntry for consistency * Remove duplicate tests * more test fixes * Add tests with multiple exception list items * Chunk exception list items in query to support up to 1000000 Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com> Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com>

kibanamachine · 2020-07-24T14:24:44Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request
Commit: 41d3d79

Build metrics

async chunks size

id	value	diff	baseline
securitySolution	7.3MB	-883.0B	7.3MB

History

💚 Build #63587 succeeded 41d3d79
💚 Build #63466 succeeded 93243bf
💚 Build #63084 succeeded 99fa3c4
💔 Build #62747 failed 35ebd1f
💔 Build #62734 failed 0b6dddb

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

Change query builder so N exceptions don't nest N levels deep

5e8061d

marshallmain added v8.0.0 release_note:skip Skip the PR/issue when compiling release notes v7.10.0 v7.9.0 labels Jul 17, 2020

marshallmain added 5 commits July 17, 2020 11:38

Fix tests and clarify function naming

673b164

Rename evaluateEntry to buildEntry for consistency

1998e19

Remove duplicate tests

94fffc7

more test fixes

0b6dddb

Merge branch 'master' into improve-query-building

35ebd1f

madirey reviewed Jul 19, 2020

View reviewed changes

Add tests with multiple exception list items

9db6acf

Merge branch 'master' into improve-query-building

99fa3c4

marshallmain marked this pull request as ready for review July 20, 2020 18:38

marshallmain requested review from a team as code owners July 20, 2020 18:38

marshallmain changed the title ~~Change query builder so N exception items don't nest N levels deep~~ [Security Solution] Change query builder so N exception items don't nest N levels deep Jul 20, 2020

Chunk exception list items in query to support up to 1000000

93243bf

FrankHassanabad approved these changes Jul 21, 2020

View reviewed changes

madirey reviewed Jul 22, 2020

View reviewed changes

madirey approved these changes Jul 22, 2020

View reviewed changes

yctercero approved these changes Jul 22, 2020

View reviewed changes

Merge branch 'master' into improve-query-building

41d3d79

marshallmain merged commit 0bab771 into elastic:master Jul 22, 2020

marshallmain deleted the improve-query-building branch July 22, 2020 15:04

dhurley14 reviewed Jul 22, 2020

View reviewed changes

epixa mentioned this pull request Jul 22, 2020

Remove the optimizer from builds #7322

Closed

9 tasks

marshallmain mentioned this pull request Jul 22, 2020

[7.x] [Security Solution] Change query builder so N exception items don't nest N levels deep (#72224) #72867

Merged

marshallmain mentioned this pull request Jul 22, 2020

[7.9] [Security Solution] Change query builder so N exception items don't nest N levels deep (#72224) #72868

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Security Solution] Change query builder so N exception items don't nest N levels deep #72224

[Security Solution] Change query builder so N exception items don't nest N levels deep #72224

marshallmain commented Jul 17, 2020 •

edited by peluja1012

Loading

madirey Jul 19, 2020

madirey Jul 19, 2020

marshallmain Jul 20, 2020

madirey Jul 19, 2020

marshallmain commented Jul 20, 2020

FrankHassanabad left a comment

madirey Jul 22, 2020

yctercero left a comment

yctercero Jul 22, 2020

dhurley14 Jul 22, 2020

dhurley14 Jul 22, 2020

dhurley14 Jul 22, 2020

dhurley14 Jul 22, 2020

dhurley14 Jul 22, 2020

kibanamachine commented Jul 24, 2020

[Security Solution] Change query builder so N exception items don't nest N levels deep #72224

[Security Solution] Change query builder so N exception items don't nest N levels deep #72224

Conversation

marshallmain commented Jul 17, 2020 • edited by peluja1012 Loading

Summary

For maintainers

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

marshallmain commented Jul 20, 2020

FrankHassanabad left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yctercero 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

kibanamachine commented Jul 24, 2020

💚 Build Succeeded

Build metrics

async chunks size

History

marshallmain commented Jul 17, 2020 •

edited by peluja1012

Loading