[Security Solution][Detections] Discuss: EQL search strategy by rylnd · Pull Request #78107 · elastic/kibana

rylnd · 2020-09-22T01:01:32Z

This adds a basic implementation for an EQL search strategy. This is meant to start a conversation; there are still several issues to address, which I've annotated inline below.

General issues:

Since EQL is part of the Basic license, where should the strategy live? The data_enhanced plugin, a new x-pack plugin, or do we keep it in security_solution for now?
The current search strategies all assume normal ES queries and are typed as such, but EQL sends responses that are incompatible with the underlying type SearchResponse provided by elasticsearch.

Checklist

Any text added follows EUI's writing guidelines, uses sentence case text and includes i18n support
Documentation was added for features that require explanation or tutorials
Unit or functional tests were updated or added to match the most common scenarios
This was checked for keyboard-only and screenreader accessibility
This renders correctly on smaller devices using a responsive layout. (You can test this in your browser
This was checked for cross-browser compatibility

For maintainers

This was checked for breaking API changes and was labeled appropriately

* Attempts to extend ISearchStrategy to allow EQL requests and responses * Since the ES search response is incompatible with the EQL Search response, we can either try to merge them into one generic type, or allow two paths for responses. We're currently on track for the latter. There are several issues here, the most pressing of which is licensing.

rylnd · 2020-09-22T01:08:38Z

src/plugins/data/common/search/es_search/types.ts

 export const isEsResponse = (response: any): response is IEsSearchResponse =>
  response && response.rawResponse;
+
+export interface EqlSearchResponse<T> {


I don't believe that this type exists in the elasticsearch package, or at least I couldn't find it: the EQL client is currently typed to return an ApiResponse which makes no assumptions about the response body.

rylnd · 2020-09-22T01:11:41Z

src/plugins/data/common/search/es_search/types.ts

-  params?: ISearchRequestParams;
+export interface IEsSearchRequest<T = Record<string, any>, P = ISearchRequestParams<T>>
+  extends IKibanaSearchRequest {
+  params?: P;


This allows us to override params for EqlSearchStrategyRequest below to be those required by the EQL search endpoint.

rylnd · 2020-09-22T01:14:18Z

x-pack/plugins/security_solution/server/lib/types.ts

 import { Timeline } from './timeline/saved_object';
 import { TLS } from './tls';
 import { MatrixHistogram } from './matrix_histogram';
+import { SearchTypes } from './detection_engine/signals/types';


Ignore this file's changes; these new EQL typings are temporarily duplicated in both the security_solution and data plugins.

rylnd · 2020-09-22T01:16:46Z

x-pack/plugins/security_solution/server/search_strategy/eql/base.ts

+};
+
+export const securityEqlSearchStrategyProvider = (
+  logger: Logger


We're not yet pulling configuration here as I wasn't sure which options (if any) were relevant to EQL queries.

kibanamachine · 2020-09-22T01:20:21Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request
Commit: 5d1a927
Pipeline Steps (look for red circles / failed steps)
Interpreting CI Failures

Failed CI Steps

Test Failures

X-Pack APM API integration tests (basic).x-pack/test/apm_api_integration/basic/tests/metrics_charts/metrics_charts·ts.APM specs (basic) Metrics when data is loaded for opbeans-node returns metrics data CPU usage has correct series overall values

Link to Jenkins

Standard Out

Failed Tests Reporter:
  - Test has failed 4 times on tracked branches: https://github.com/elastic/kibana/issues/77870

[00:00:00]       │
[00:00:00]         └-: APM specs (basic)
[00:00:00]           └-> "before all" hook
[00:01:44]           └-: Metrics
[00:01:44]             └-> "before all" hook
[00:01:44]             └-: when data is loaded
[00:01:44]               └-> "before all" hook
[00:01:44]               └-> "before all" hook
[00:01:44]                 │ info [metrics_8.0.0] Loading "mappings.json"
[00:01:44]                 │ info [metrics_8.0.0] Loading "data.json.gz"
[00:01:44]                 │ info [o.e.c.m.MetadataCreateIndexService] [kibana-ci-immutable-ubuntu-18-tests-xxl-1600736549507444443] [apm-8.0.0-metric-000001] creating index, cause [api], templates [], shards [1]/[0]
[00:01:45]                 │ info [o.e.c.r.a.AllocationService] [kibana-ci-immutable-ubuntu-18-tests-xxl-1600736549507444443] current.health="GREEN" message="Cluster health status changed from [YELLOW] to [GREEN] (reason: [shards started [[apm-8.0.0-metric-000001][0]]])." previous.health="YELLOW" reason="shards started [[apm-8.0.0-metric-000001][0]]"
[00:01:45]                 │ info [metrics_8.0.0] Created index "apm-8.0.0-metric-000001"
[00:01:45]                 │ debg [metrics_8.0.0] "apm-8.0.0-metric-000001" settings {"index":{"codec":"best_compression","lifecycle":{"name":"apm-rollover-30-days","rollover_alias":"apm-8.0.0-metric"},"mapping":{"total_fields":{"limit":"2000"}},"max_docvalue_fields_search":"200","number_of_replicas":"0","number_of_shards":"1","priority":"100","refresh_interval":"1ms"}}
[00:01:45]                 │ info [metrics_8.0.0] Indexed 2323 docs into "apm-8.0.0-metric-000001"
[00:01:45]               └-: for opbeans-node
[00:01:45]                 └-> "before all" hook
[00:01:45]                 └-: returns metrics data
[00:01:45]                   └-> "before all" hook
[00:01:45]                   └-> "before all" hook
[00:01:46]                   └-> contains CPU usage and System memory usage chart data
[00:01:46]                     └-> "before each" hook: global before each
[00:01:46]                     └-> "before each" hook
[00:01:46]                     └- ✓ pass  (1ms) "APM specs (basic) Metrics when data is loaded for opbeans-node returns metrics data contains CPU usage and System memory usage chart data"
[00:01:46]                   └-> "after each" hook
[00:01:46]                   └-: CPU usage
[00:01:46]                     └-> "before all" hook
[00:01:46]                     └-> "before all" hook
[00:01:46]                     └-> has correct series
[00:01:46]                       └-> "before each" hook: global before each
[00:01:46]                       └-> "before each" hook
[00:01:46]                       └- ✓ pass  (1ms) "APM specs (basic) Metrics when data is loaded for opbeans-node returns metrics data CPU usage has correct series"
[00:01:46]                     └-> "after each" hook
[00:01:46]                     └-> has correct series overall values
[00:01:46]                       └-> "before each" hook: global before each
[00:01:46]                       └-> "before each" hook
[00:01:46]                       └- ✖ fail: APM specs (basic) Metrics when data is loaded for opbeans-node returns metrics data CPU usage has correct series overall values
[00:01:46]                       │       Error: expect(received).toMatchInlineSnapshot(snapshot)
[00:01:46]                       │ 
[00:01:46]                       │ Snapshot name: `when data is loaded for opbeans-node returns metrics data CPU usage has correct series overall values 1`
[00:01:46]                       │ 
[00:01:46]                       │ - Snapshot  - 1
[00:01:46]                       │ + Received  + 1
[00:01:46]                       │ 
[00:01:46]                       │   Array [
[00:01:46]                       │     0.714,
[00:01:46]                       │ -   0.38770000000000004,
[00:01:46]                       │ +   0.3877,
[00:01:46]                       │     0.75,
[00:01:46]                       │     0.2543,
[00:01:46]                       │   ]
[00:01:46]                       │       + expected - actual
[00:01:46]                       │ 
[00:01:46]                       │       -false
[00:01:46]                       │       +true
[00:01:46]                       │       
[00:01:46]                       │       at Assertion.assert (/dev/shm/workspace/parallel/24/kibana/packages/kbn-expect/expect.js:100:11)
[00:01:46]                       │       at Assertion.toMatchInline [as eql] (/dev/shm/workspace/parallel/24/kibana/packages/kbn-expect/expect.js:244:8)
[00:01:46]                       │       at Context.apply (test/apm_api_integration/basic/tests/metrics_charts/metrics_charts.ts:69:16)
[00:01:46]                       │       at Object.apply (/dev/shm/workspace/parallel/24/kibana/packages/kbn-test/src/functional_test_runner/lib/mocha/wrap_function.js:84:30)
[00:01:46]                       │ 
[00:01:46]                       │

Stack Trace

{ Error: expect(received).toMatchInlineSnapshot(snapshot)

Snapshot name: `when data is loaded for opbeans-node returns metrics data CPU usage has correct series overall values 1`

- Snapshot  - 1
+ Received  + 1

  Array [
    0.714,
-   0.38770000000000004,
+   0.3877,
    0.75,
    0.2543,
  ]
    at Assertion.assert (/dev/shm/workspace/parallel/24/kibana/packages/kbn-expect/expect.js:100:11)
    at Assertion.toMatchInline [as eql] (/dev/shm/workspace/parallel/24/kibana/packages/kbn-expect/expect.js:244:8)
    at Context.apply (test/apm_api_integration/basic/tests/metrics_charts/metrics_charts.ts:69:16)
    at Object.apply (/dev/shm/workspace/parallel/24/kibana/packages/kbn-test/src/functional_test_runner/lib/mocha/wrap_function.js:84:30) actual: 'false', expected: 'true', showDiff: true }

Build metrics

distributable file count

id	value	diff	baseline
default	45944	+4	45940

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

rylnd · 2020-09-22T01:42:11Z

x-pack/plugins/security_solution/server/search_strategy/eql/base.ts

+          rawResponse: {
+            ...response.body,
+            ...dummyResponseFields,
+          },
+          rawEqlResponse: response.body,


Suggested change

rawResponse: {

...response.body,

...dummyResponseFields,

},

rawEqlResponse: response.body,

rawResponse: response.body

Applying the above change should reveal the incompatibilities between the expected SearchResponse type and our (currently unofficial) EqlSearchResponse type. To name a few, EQL responses are missing _shards, and hits has no max_score. Additionally, hits does not have its own hits field, but instead has either events or sequences depending on the query's syntax.

I've temporarily satisfied typescript by adding these dummy fields to satisfy IEsSearchResponse['rawResponse'], and added another field to hold our "true" EQL response.

We could address this in a few ways: loosen IEsSearchResponse to require the minimal subset of fields (took, timed_out, etc.), give it a conditional type, or diverge from ISearchStrategy entirely. Guidance on a path forward here would be greatly appreciated.

rylnd · 2020-09-22T01:50:44Z

x-pack/plugins/security_solution/server/search_strategy/eql/base.ts

+
+export const securityEqlSearchStrategyProvider = (
+  logger: Logger
+): ISearchStrategy<EqlSearchStrategyRequest, EqlSearchStrategyResponse> => {


I think this is just an oversight, but one other thing I noticed is that while registerSearchStrategy is generic, getSearchStrategy is not and so consumers of divergent search strategies would have to assert their strategy's typings, lest they get a IEsSearchStrategy<any>

lizozom · 2020-09-23T08:46:21Z

src/plugins/data/common/search/es_search/types.ts

   */
  isPartial?: boolean;
-  rawResponse: SearchResponse<Source>;
+  rawResponse: SearchResponse<T>;


The only response arguments the search infrastructure cares about are:

id - the async search ID provided by elasticsearch

isRunning and isPartial which are indicators of the search's execution in ES.

took and total_hits for telemetry and potentially any additional meta data about the request's execution.

Otherwise, the response can be completely generic, so we could create a new type for rawResponse (EsSearch response is specific to the ES strategy).

lizozom · 2020-09-23T08:47:28Z

x-pack/plugins/security_solution/server/plugin.ts

        depsStart.data
      );
+
+      plugins.data.search.registerSearchStrategy(SECURITY_EQL_SEARCH_STRATEGY, eqlSearchStrategy);


If the strategy is owned by the data plugin, it should be registered there too

@lizozom since EQL is only available in x-pack, should this strategy live in the data_enhanced plugin for now?

lizozom · 2020-09-23T10:40:01Z

@rylnd

I've made this PR to make the search types more generic.

It enables you to define

interface IEqlSearchRequest extends IKibanaSearchRequest<IEqlRequestParams> {
  ...
}

type IEqlSearchResponse = IKibanaSearchResponse<EqlResponse>;

I also added generic typing to both search and getStrategy functions of the search service.

Please let me know if it helps. Maybe you want to rebase this PR on top of that one and give it a try?

rylnd · 2020-09-23T22:37:15Z

@lizozom thanks so much for these improvements, they're working great.

I'm probably going to just close this PR since it'll need to be rebased on both your work and my own outstanding PR (#77493).

In the meantime, I have a working branch that integrates all three pieces on my fork: rylnd#5. It's currently targeting (a local copy of) your branch, so you can see what the implementation on top of that looks like right now.

Since the implementation is pretty straightforward I don't I need any more cursory review at this point (but correct me if you disagree). While I finalize the async search workflow and add the default search configuration, there's one outstanding question we need to answer:

Since EQL is only supported in x-pack, can/should this strategy live in the data plugin? If not, would the data_enhanced plugin be a more appropriate place for it, or should it live somewhere else entirely?

lizozom · 2020-09-24T12:29:21Z

@rylnd
Glad it helped!
I think it would be a good idea to merge the strategy in a separate PR, for documentation and release notes purposes.

Regarding your question: data_enhanced/server is definitely right the place.

rylnd · 2020-09-28T20:48:04Z

Closing this in favor of #78645.

rylnd added discuss Team:SIEM Team:AppArch labels Sep 22, 2020

rylnd requested review from XavierM, lizozom and yctercero September 22, 2020 01:01

rylnd self-assigned this Sep 22, 2020

rylnd commented Sep 22, 2020

View reviewed changes

rylnd mentioned this pull request Sep 22, 2020

[Security Solution][Detections] EQL Validation #77493

Merged

4 tasks

lizozom reviewed Sep 23, 2020

View reviewed changes

lizozom mentioned this pull request Sep 23, 2020

[Search] Generic search request and response types #78268

Merged

7 tasks

lukeelmers mentioned this pull request Sep 24, 2020

[Discuss] Removing RequestHandlerContext from the low-level data.search API. #78461

Closed

rylnd closed this Sep 28, 2020

rylnd deleted the just_eql_strategy branch September 28, 2020 20:48

MindyRS added the Team: SecuritySolution Security Solutions Team working on SIEM, Endpoint, Timeline, Resolver, etc. label Sep 23, 2021

Conversation

rylnd commented Sep 22, 2020

Checklist

For maintainers

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rylnd Sep 22, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kibanamachine commented Sep 22, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

💔 Build Failed

Failed CI Steps

Standard Out

Stack Trace

Build metrics

distributable file count

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lizozom commented Sep 23, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rylnd commented Sep 23, 2020

Uh oh!

lizozom commented Sep 24, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rylnd commented Sep 28, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

rylnd Sep 22, 2020 •

edited

Loading

kibanamachine commented Sep 22, 2020 •

edited

Loading

lizozom commented Sep 23, 2020 •

edited

Loading

lizozom commented Sep 24, 2020 •

edited

Loading