EMT-451: add ability to filter endpoint metadata based on presence of unenrolled events by nnamdifrankie · Pull Request #69708 · elastic/kibana

nnamdifrankie · 2020-06-23T16:04:01Z

Summary

Issue: https://github.com/elastic/endpoint-app-team/issues/451

add mirror index that will hold events that do not originate from the endpoint agent. Events in this index will be triggered by state changes in the Fleet system.
filter metadata list and endpoint details based on unenrolled events in the mirror index.
update tests and add more tests to validate that endpoints are removed when there is an unenrolled endpoint in the mirror index.

Checklist

Delete any items that are not applicable to this PR.

Unit or functional tests were updated or added to match the most common scenarios

…ring

nnamdifrankie · 2020-06-23T19:32:02Z

@elasticmachine merge upstream

…ring

…ub.com:nnamdifrankie/kibana into EMT-451_add_endpoint_enrolled_status_filtering

nnamdifrankie · 2020-06-24T13:43:03Z

@elasticmachine merge upstream

…ring

…ub.com:nnamdifrankie/kibana into EMT-451_add_endpoint_enrolled_status_filtering

kevinlog · 2020-06-24T13:53:37Z

Looks good to me, I wanna make sure @ferullo is aware of the adjustment to the schema https://github.com/elastic/endpoint-package/blob/master/package/endpoint/dataset/metadata/fields/fields.yml#L52

would also like a look from @jonathan-buttner

x-pack/plugins/security_solution/common/endpoint/types.ts

paul-tavares · 2020-06-24T15:34:19Z

x-pack/plugins/security_solution/common/endpoint/types.ts

+  /**
+   * Agent is enrolled with Fleet
+   */
+  ENROLLED = 'enrolled',


can we have the enum keys in lower case and match the value?

When (if) try to use this on the front-end it makes it easier to work with when we are working with the value.

paul-tavares · 2020-06-24T15:39:38Z

x-pack/plugins/security_solution/server/endpoint/routes/metadata/index.ts

@@ -113,6 +122,12 @@ export function registerEndpointRoutes(router: IRouter, endpointAppContext: Endp
        return res.notFound({ body: 'Endpoint Not Found' });


I have not been picking up on this, but I think all returned static error messages should be i18n'ed
(maybe we have discussed this already?)

I have not been picking up on this, but I think all returned static error messages should be i18n'ed
(maybe we have discussed this already?)

I do not think this applies to server side code, usually the approach is error code. This is the pattern system wide. But we will certainly review that approach.

paul-tavares · 2020-06-24T15:40:01Z

x-pack/plugins/security_solution/server/endpoint/routes/metadata/index.ts

+    id
+  );
+  if (unenrolledHostId) {
+    throw Boom.badRequest(`the requested endpoint is unerolled`);


Same here - should the message be i18n?

paul-tavares · 2020-06-24T15:44:52Z

x-pack/plugins/security_solution/common/endpoint/constants.ts

 export const eventsIndexPattern = 'logs-endpoint.events.*';
 export const alertsIndexPattern = 'logs-endpoint.alerts-*';
 export const metadataIndexPattern = 'metrics-endpoint.metadata-*';
+export const metadataMirrorIndexPattern = 'metrics-endpoint.metadata_mirror-*';


Sorry - I have not followed the discussions as close as I probably should.

What is the new index used for? just to keep track of unregistered endpoint and used by the "callback" from ingest to get updated when agent unenrolls or endpoint integration is removed from an agent config?

It will be used to store events that is generated from Security Solution application. We do not want to pollute the index used by the endpoint, and it also give us the ability to reverse course without messing up the state of the application.

jonathan-buttner

There's a couple changes I think we should make:

Address the alert schema difference between the type defined here and the schema
Leverage search_after instead of scroll
Leverage the filter context instead of query to improve performance.

jonathan-buttner · 2020-06-24T15:24:34Z

x-pack/plugins/security_solution/common/endpoint/types.ts

    type: string;
  };
  Endpoint: {
+    status: EndpointStatus;


Do we need the endpoint status in the alert event? This doesn't match with what we have in the schema:

https://github.com/elastic/endpoint-package/blob/master/custom_subsets/elastic_endpoint/alerts/malware_event.yaml#L29

jonathan-buttner · 2020-06-24T15:30:31Z

x-pack/plugins/security_solution/server/endpoint/routes/metadata/index.ts

+    id
+  );
+  if (unenrolledHostId) {
+    throw Boom.badRequest(`the requested endpoint is unerolled`);


nit: unerolled -> unenrolled

Would we ever want to show information about an unenrolled host?

jonathan-buttner · 2020-06-24T15:31:54Z

x-pack/plugins/security_solution/server/endpoint/routes/metadata/metadata.test.ts

+          hits: {
+            hits: [
+              {
+                _index: 'metrics-endpoint.metadata_mirror-default-1',


nit: remove the trailing -1 from the index name

x-pack/plugins/security_solution/server/endpoint/routes/metadata/query_builders.test.ts

jonathan-buttner · 2020-06-24T16:14:44Z

x-pack/plugins/security_solution/server/endpoint/routes/metadata/support/unenroll.ts

+
+  if (newHits.length > 0) {
+    const hostIds = newHits
+      .flatMap((data) => data as HitSource)


I think I'm missing why we need to flatten the map and then iterate over it again? I don't think we could just do a single map() call right? to retrieve the _source.

jonathan-buttner · 2020-06-24T16:15:27Z

x-pack/plugins/security_solution/server/endpoint/routes/metadata/support/unenroll.ts

+    index: metadataMirrorIndexPattern,
+    scroll: KEEPALIVE,
+    body: {
+      size: 100,


Why limit it to 100? I think we'll want to reduce the number of API requests to ES and balance that with the amount of data we get back in a single request. I wonder if we should bump this up to over 1k?

jonathan-buttner · 2020-06-24T16:20:30Z

x-pack/plugins/security_solution/server/endpoint/routes/metadata/support/unenroll.ts

+      .map((hitSource: HitSource) => hitSource._source);
+    hits.push(...hostIds);
+
+    const innerResponse = await client('scroll', {


Hmm, I don't think we want to use scroll here. The docs say:

Scrolling is not intended for real time user requests, but rather for processing large amounts of data, e.g. in order to reindex the contents of one index into a new index with a different configuration.

https://www.elastic.co/guide/en/elasticsearch/reference/7.8/search-request-body.html#request-body-search-scroll

I think we want to use search_after instead: https://www.elastic.co/guide/en/elasticsearch/reference/7.8/search-request-body.html#request-body-search-search-after

We discussed this offline. The function here is to retrieve all the results (only the ids) using a cursor based approach and does not involve any user interaction or pagination. This approach was taken in the event that the number of unerolled endpoints are greater than 10000, which is the default max size of a page. We are also returning only the host ids, so we are very minimal. Only returning the IDs means faster transmission and processing time, and small space requirement . We also keep the previous search context for 10s to 30s. I increased to 30s with the increase in page size, to allow for processing.

jonathan-buttner · 2020-06-24T16:21:58Z

x-pack/plugins/security_solution/server/endpoint/routes/metadata/support/unenroll.ts

+
+  if (newHits.length > 0) {
+    const hostIds: HostId[] = newHits
+      .flatMap((data) => data as HitSource)


I think we can remove this call.

x-pack/plugins/security_solution/server/endpoint/routes/metadata/query_builders.ts

ferullo · 2020-06-24T19:20:29Z

Here's the Endpoint PR to set this in it's metadata documents https://github.com/elastic/endpoint-dev/pull/7236

jonathan-buttner · 2020-06-25T12:28:29Z

x-pack/plugins/security_solution/server/endpoint/routes/metadata/support/unenroll.ts

+  >;
+
+  // eslint-disable-next-line no-return-await
+  return await fetchAllUnenrolledHostIdsWithScroll(response, client.callAsCurrentUser);


nit: I think you can remove the await since the caller of this function is doing an await right? Unless we're doing this for call stack debugging?

jonathan-buttner · 2020-06-25T12:28:44Z

x-pack/plugins/security_solution/server/endpoint/routes/metadata/support/unenroll.ts

+    });
+
+    // eslint-disable-next-line no-return-await
+    return await fetchAllUnenrolledHostIdsWithScroll(innerResponse, client, hits);


nit: I think you can remove the await since the caller of this function is doing an await right? Unless we're doing this for call stack debugging?

kibanamachine · 2020-06-25T15:28:22Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request
Commit: 46d2130

Build metrics

✅ unchanged

History

💚 Build #56252 succeeded b310f92
💚 Build #56130 succeeded 866b3e2
💚 Build #55971 succeeded 3649efa
💔 Build #55926 failed 728d229
💔 Build #55901 failed e84be27

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

…resence of unenrolled events (elastic#69708) [Endpoint]EMT-451: add ability to filter endpoint metadata based on presence of unenrolled events

* master: (90 commits) [Encrypted Saved Objects] Adds support for migrations in ESO (elastic#69513) [SIEM] Replace WithSource with useWithSource hook (elastic#68722) [Endpoint]EMT-451: add ability to filter endpoint metadata based on presence of unenrolled events (elastic#69708) rename old siem kibana config to securitySolution (elastic#69874) Remove unused Resolver code (elastic#69914) [Observability] Fixing dynamic return type based on the appName (elastic#69894) [SECURITY SOLUTION][INGEST] Task/endpoint list tests (elastic#69419) Fixes special clicks and 3rd party icon sizes in nav (elastic#69767) [APM] Catch annotations index permission error and log warning (elastic#69881) [Endpoint][Ingest Manager] minor code cleanup (elastic#69844) [Logs UI] Logs ui context menu (elastic#69915) Index pattern serialize and de-serialize (elastic#68844) [QA] Unskip functional tests (elastic#69760) [SIEM][Detection Engine] - Update DE to work with new exceptions schema (elastic#69715) Fixes elastic#69639: Ignore url.url fields above 2048 characters (elastic#69863) PR: Provide limit warnings to user when API limits are reached. (elastic#69590) [Maps] Remove broken button (elastic#69853) Makes usage collection methods available on start (elastic#69836) [SIEM][CASE] Improve Jira's labelling (elastic#69892) [Logs UI] Access ML via the programmatic plugin API (elastic#68905) ...

…resence of unenrolled events (#69708) (#69971) [Endpoint]EMT-451: add ability to filter endpoint metadata based on presence of unenrolled events

* master: [Encrypted Saved Objects] Adds support for migrations in ESO (elastic#69513) [SIEM] Replace WithSource with useWithSource hook (elastic#68722) [Endpoint]EMT-451: add ability to filter endpoint metadata based on presence of unenrolled events (elastic#69708) rename old siem kibana config to securitySolution (elastic#69874) Remove unused Resolver code (elastic#69914) [Observability] Fixing dynamic return type based on the appName (elastic#69894) [SECURITY SOLUTION][INGEST] Task/endpoint list tests (elastic#69419) Fixes special clicks and 3rd party icon sizes in nav (elastic#69767) [APM] Catch annotations index permission error and log warning (elastic#69881) [Endpoint][Ingest Manager] minor code cleanup (elastic#69844) [Logs UI] Logs ui context menu (elastic#69915) Index pattern serialize and de-serialize (elastic#68844)

EMT-451: check i

e30aba8

nnamdifrankie added v7.9.0 v8.0.0 release_note:skip Skip the PR/issue when compiling release notes labels Jun 23, 2020

Merge branch 'master' into EMT-451_add_endpoint_enrolled_status_filte…

1148c47

…ring

nnamdifrankie requested a review from jonathan-buttner June 23, 2020 19:10

nnamdifrankie marked this pull request as ready for review June 23, 2020 19:31

nnamdifrankie requested review from a team as code owners June 23, 2020 19:31

elasticmachine and others added 4 commits June 23, 2020 13:32

Merge branch 'master' into EMT-451_add_endpoint_enrolled_status_filte…

6862ae2

…ring

EMT-451: clean up

f7298fc

Merge branch 'EMT-451_add_endpoint_enrolled_status_filtering' of gith…

69d4b70

…ub.com:nnamdifrankie/kibana into EMT-451_add_endpoint_enrolled_status_filtering

EMT-451: document the endpoint status, and make it strongly typed.

ce9f7bf

elasticmachine and others added 3 commits June 24, 2020 07:43

Merge branch 'master' into EMT-451_add_endpoint_enrolled_status_filte…

0dfd093

…ring

EMT-451: update case

c5244e3

Merge branch 'EMT-451_add_endpoint_enrolled_status_filtering' of gith…

e84be27

…ub.com:nnamdifrankie/kibana into EMT-451_add_endpoint_enrolled_status_filtering

kevinlog approved these changes Jun 24, 2020

View reviewed changes

pzl reviewed Jun 24, 2020

View reviewed changes

x-pack/plugins/security_solution/common/endpoint/types.ts Show resolved Hide resolved

EMT-451: add import

728d229

pzl approved these changes Jun 24, 2020

View reviewed changes

paul-tavares reviewed Jun 24, 2020

View reviewed changes

EMT-451: fix tests

3649efa

jonathan-buttner requested changes Jun 24, 2020

View reviewed changes

nnamdifrankie added 3 commits June 24, 2020 19:07

EMT-451: review comments

825ebf0

EMT-451: clean up

866b3e2

EMT-451: some more clean up

b310f92

paul-tavares approved these changes Jun 25, 2020

View reviewed changes

jonathan-buttner approved these changes Jun 25, 2020

View reviewed changes

EMT-451: use loop, remove await

46d2130

nnamdifrankie merged commit a854067 into elastic:master Jun 25, 2020

nnamdifrankie deleted the EMT-451_add_endpoint_enrolled_status_filtering branch June 25, 2020 15:51

nnamdifrankie mentioned this pull request Jun 25, 2020

[7.x] [Endpoint]EMT-451: add ability to filter endpoint metadata based on presence of unenrolled events (#69708) #69971

Merged

		@@ -113,6 +122,12 @@ export function registerEndpointRoutes(router: IRouter, endpointAppContext: Endp
		return res.notFound({ body: 'Endpoint Not Found' });

Conversation

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

Uh oh!

Summary

Checklist

Uh oh!

nnamdifrankie commented Jun 23, 2020

Uh oh!

nnamdifrankie commented Jun 24, 2020

Uh oh!

kevinlog commented Jun 24, 2020

Uh oh!

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!

Choose a reason for hiding this comment

Uh oh!

jonathan-buttner left a comment

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!

Uh oh!

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!

Uh oh!

ferullo commented Jun 24, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kibanamachine commented Jun 25, 2020

💚 Build Succeeded

Build metrics

History

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

nnamdifrankie commented Jun 23, 2020 •

edited

Loading