[Security Solution] Use safe type in resolver backend #76969

jonathan-buttner · 2020-09-08T18:44:05Z

This PR refactors the backend to use the SafeResolverEvent type to enforce checking for arrays and single values in ECS fields.

A couple places in front end tests had to be switched to casting the type back to ResolverEvent. This is because the resolver generator is now returning the safe type and the front end doesn't use it in all places yet. Once the front end is switched we can remove the casts.

jonathan-buttner · 2020-09-08T18:51:53Z

x-pack/plugins/security_solution/common/endpoint/models/ecs_safety_helpers.ts

@@ -46,12 +46,12 @@ export function values<T>(valueOrCollection: ECSField<T>): T[] {
  if (Array.isArray(valueOrCollection)) {
    const nonNullValues: T[] = [];
    for (const value of valueOrCollection) {
-      if (value !== null) {
+      if (value !== null && value !== undefined) {


Avoid having [undefined]

jonathan-buttner · 2020-09-08T18:55:26Z

x-pack/plugins/security_solution/common/endpoint/models/event.ts

  );
 }

-export function timestampSafeVersion(event: SafeResolverEvent): string | undefined | number {
-  return isLegacyEventSafeVersion(event)
-    ? firstNonNullValue(event.endgame?.timestamp_utc)


I can't seem to find references to event.endgame.timestamp_utc in the endgame test data we have in es archiver or the endgame schema. evrelay populates @timestamp, so I think it'd be simpler to keep these consistent between the legacy events and endpoint events. If we end up needing to use event.created we can create a new function to retrieve that and do the legacy check there.

jonathan-buttner · 2020-09-08T18:55:45Z

x-pack/plugins/security_solution/common/endpoint/models/event.ts

@@ -75,11 +67,7 @@ export function timestampAsDateSafeVersion(event: SafeResolverEvent): Date | und
 }

 export function eventTimestamp(event: ResolverEvent): string | undefined | number {
-  if (isLegacyEvent(event)) {
-    return event.endgame.timestamp_utc;


Same as above.

jonathan-buttner · 2020-09-08T18:59:55Z

x-pack/plugins/security_solution/common/endpoint/models/event.ts

@@ -105,14 +93,7 @@ export function eventId(event: ResolverEvent): number | undefined | string {
  return event.event.id;
 }

-export function eventSequence(event: ResolverEvent): number | undefined {


The backend was the only one using this function so I just removed it and switched the name of the safe version.

jonathan-buttner · 2020-09-08T19:01:13Z

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

@@ -112,6 +112,27 @@ export interface ResolverChildNode extends ResolverLifecycleNode {
  nextChild?: string | null;
 }

+/**


For now I'm duplicating all the types that are used between the frontend and backend. The backend uses the safe versions and the frontend will use the unsafe ones for now.

jonathan-buttner · 2020-09-08T19:01:39Z

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

@@ -198,7 +281,7 @@ export interface SafeResolverRelatedEvents {
 */
 export interface ResolverRelatedAlerts {
  entityID: string;
-  alerts: ResolverEvent[];
+  alerts: SafeResolverEvent[];


The frontend isn't referencing the related alerts yet so switching it over to use the safe version.

jonathan-buttner · 2020-09-08T19:02:30Z

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

-    executable: string;
-    start: number;
-    thread?: ThreadFields[];
+export type AlertEvent = Partial<{


Wrapping everything in ECSField and removing the duplication between this type and SafeEndpointEvent since we'll just combine them

jonathan-buttner · 2020-09-08T19:16:34Z

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

@@ -578,7 +642,7 @@ export type ResolverEvent = EndpointEvent | LegacyEndpointEvent;
 * All mappings in Elasticsearch support arrays. They can also return null values or be missing. For example, a `keyword` mapping could return `null` or `[null]` or `[]` or `'hi'`, or `['hi', 'there']`. We need to handle these cases in order to avoid throwing an error.
 * When dealing with an value that comes from ES, wrap the underlying type in `ECSField`. For example, if you have a `keyword` or `text` value coming from ES, cast it to `ECSField<string>`.
 */
-export type ECSField<T> = T | null | Array<T | null>;
+export type ECSField<T> = T | null | undefined | Array<T | null>;


Needed to add undefined here even though ES won't ever return that. This avoids types errors that TS was giving me.

Thanks. This is safe since we want to account for the case of missing fields.

jonathan-buttner · 2020-09-08T20:12:06Z

...public/resolver/models/indexed_process_tree/__snapshots__/isometric_taxi_layout.test.ts.snap

@@ -212,6 +212,10 @@ Object {
    },
    Object {
      "metadata": Object {
+        "elapsedTime": Object {


These changed because the timestampSafeVersion now retrieves the @timestamp field on the legacy event type as well. Previously, we were the isometric code was receiving an empty string which resulted in null be used for this field so it was excluded from the metadata.

elasticmachine · 2020-09-08T20:12:55Z

Pinging @elastic/endpoint-data-visibility-team (Team:Endpoint Data Visibility)

oatkiller

Looks great. Thank you so much.

My only request is to have any TODO comments removed before merge. I only noticed one and I commented on it.

oatkiller · 2020-09-10T14:25:59Z

x-pack/plugins/security_solution/common/endpoint/generate_data.test.ts

@@ -420,7 +434,7 @@ describe('data generator', () => {
      }
    }
    // The root node must be first in the array or this fails
-    return tree[events[0].process.entity_id];
+    return tree[entityIDSafeVersion(events[0]) ?? ''];


I don't think '' would be correct here. Maybe this instead?

Suggested change

return tree[entityIDSafeVersion(events[0]) ?? ''];

const entityID = entityIDSafeVersion(events[0])

if (entityID === undefined) {

// this should never happen.

throw new Error()

}

return tree[entityIDSafeVersion(events[0])];

Or this?

Suggested change

return tree[entityIDSafeVersion(events[0]) ?? ''];

return tree[entityIDSafeVersion(events[0])!];

I'm not that familiar with the code.

Yeah good point, I'll throw an error like you suggested 👍

oatkiller · 2020-09-10T14:29:53Z

x-pack/plugins/security_solution/common/endpoint/models/event.test.ts


 describe('Generated documents', () => {
  let generator: EndpointDocGenerator;
  beforeEach(() => {
    generator = new EndpointDocGenerator('seed');
  });

+  // TODO what should I do about the cast? We can create a new safe descriptive name but
+  // the front end won't use that until the rest of it is converted


The cast is fine. This code doesn't belong in the model anyway.

Could you remove this comment before merging? ty

Oh oops, thanks for catching that.

oatkiller · 2020-09-10T14:32:17Z

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

@@ -578,7 +642,7 @@ export type ResolverEvent = EndpointEvent | LegacyEndpointEvent;
 * All mappings in Elasticsearch support arrays. They can also return null values or be missing. For example, a `keyword` mapping could return `null` or `[null]` or `[]` or `'hi'`, or `['hi', 'there']`. We need to handle these cases in order to avoid throwing an error.
 * When dealing with an value that comes from ES, wrap the underlying type in `ECSField`. For example, if you have a `keyword` or `text` value coming from ES, cast it to `ECSField<string>`.
 */
-export type ECSField<T> = T | null | Array<T | null>;
+export type ECSField<T> = T | null | undefined | Array<T | null>;


Thanks. This is safe since we want to account for the case of missing fields.

michaelolo24 · 2020-09-10T15:21:44Z

x-pack/plugins/security_solution/public/resolver/store/data/reducer.test.ts

@@ -40,7 +41,9 @@ describe('Resolver Data Middleware', () => {
      // Generate a 'tree' using the Resolver generator code. This structure isn't the same as what the API returns.
      const baseTree = generateBaseTree();
      const tree = mockResolverTree({
-        events: baseTree.allEvents,
+        // Casting here because the generator returns the SafeResolverEvent type which isn't yet compatible with


👍 Created this issue to handle the front end side of things: https://github.com/elastic/security-team/issues/239

Oh nice, thanks Mike!

jonathan-buttner · 2020-09-10T16:50:59Z

@elasticmachine merge upstream

kibanamachine · 2020-09-10T18:25:49Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request
Commit: 7908e51

Build metrics

async chunks size

id	value	diff	baseline
securitySolution	10.0MB	-330.0B	10.0MB

History

💔 Build #73728 failed b14cb3b
💚 Build #73223 succeeded f2909b1
💔 Build #73209 failed c06ba93
💔 Build #73198 failed 04a588b

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

* Moving generator to safe type version * Finished generator and alert * Gzipping again * Finishing type conversions for backend * Trying to cast front end tests back to unsafe type for now * Working reducer tests * Adding more comments and fixing alert type * Restoring resolver test data * Updating snapshot with timestamp info * Removing todo and fixing test Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com>

* Moving generator to safe type version * Finished generator and alert * Gzipping again * Finishing type conversions for backend * Trying to cast front end tests back to unsafe type for now * Working reducer tests * Adding more comments and fixing alert type * Restoring resolver test data * Updating snapshot with timestamp info * Removing todo and fixing test Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com> Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com>

…s-for-710 * 'master' of github.com:elastic/kibana: (65 commits) Separate url forwarding logic and legacy services (elastic#76892) Bump yargs-parser to v13.1.2+ (elastic#77009) [Ingest Manager] Shared Fleet agent policy action (elastic#76013) [Search] Re-add support for aborting when a connection is closed (elastic#76470) [Search] Remove long-running query pop-up (elastic#75385) [Monitoring] Fix UI error when alerting is not available (elastic#77179) do not log plugin id format warning in dist mode (elastic#77134) [ML] Improving client side error handling (elastic#76743) [Alerting][Connectors] Refactor IBM Resilient: Generic Implementation (phase one) (elastic#74357) [Docs] some basic searchsource api docs (elastic#77038) add cGroupOverrides to the legacy config (elastic#77180) Change saved object bulkUpdate to work across multiple namespaces (elastic#75478) [Security Solution][Resolver] Replace Selectable popover with badges (elastic#76997) Removing ml-state index from archive (elastic#77143) [Security Solution] Add unit tests for histograms (elastic#77081) [Lens] Filters aggregation (elastic#75635) [Enterprise Search] Update WS Overview logic to use new config data (elastic#77122) Cleanup type output before building new types (elastic#77211) [Security Solution] Use safe type in resolver backend (elastic#76969) Use proper lodash syntax (elastic#77105) ... # Conflicts: # x-pack/plugins/index_lifecycle_management/public/application/sections/edit_policy/components/node_allocation.tsx

jonathan-buttner added 7 commits September 3, 2020 17:44

Moving generator to safe type version

f9f3921

Finished generator and alert

3216705

Gzipping again

d2d5f4e

Finishing type conversions for backend

304335a

Trying to cast front end tests back to unsafe type for now

4ef0b21

Working reducer tests

910fe24

Fixing merge conflicts

04a588b

jonathan-buttner added v8.0.0 release_note:skip Skip the PR/issue when compiling release notes Team:Endpoint Data Visibility Team managing the endpoint resolver v7.10.0 labels Sep 8, 2020

jonathan-buttner requested review from oatkiller, bkimmel, michaelolo24 and kqualters-elastic September 8, 2020 18:44

jonathan-buttner commented Sep 8, 2020

View reviewed changes

jonathan-buttner added 3 commits September 8, 2020 15:25

Adding more comments and fixing alert type

c06ba93

Restoring resolver test data

25fa083

Updating snapshot with timestamp info

f2909b1

jonathan-buttner commented Sep 8, 2020

View reviewed changes

jonathan-buttner marked this pull request as ready for review September 8, 2020 20:12

jonathan-buttner requested review from a team as code owners September 8, 2020 20:12

oatkiller approved these changes Sep 10, 2020

View reviewed changes

Removing todo and fixing test

b14cb3b

michaelolo24 reviewed Sep 10, 2020

View reviewed changes

michaelolo24 approved these changes Sep 10, 2020

View reviewed changes

Merge branch 'master' into use-safe-type

7908e51

jonathan-buttner merged commit e7b02d0 into elastic:master Sep 10, 2020

jonathan-buttner deleted the use-safe-type branch September 10, 2020 18:26

jonathan-buttner mentioned this pull request Sep 10, 2020

[7.x] [Security Solution] Use safe type in resolver backend (#76969) #77230

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Security Solution] Use safe type in resolver backend #76969

[Security Solution] Use safe type in resolver backend #76969

jonathan-buttner commented Sep 8, 2020

jonathan-buttner Sep 8, 2020

jonathan-buttner Sep 8, 2020

jonathan-buttner Sep 8, 2020

jonathan-buttner Sep 8, 2020

jonathan-buttner Sep 8, 2020

jonathan-buttner Sep 8, 2020

jonathan-buttner Sep 8, 2020

jonathan-buttner Sep 8, 2020

oatkiller Sep 10, 2020

jonathan-buttner Sep 8, 2020

elasticmachine commented Sep 8, 2020

oatkiller left a comment

oatkiller Sep 10, 2020

jonathan-buttner Sep 10, 2020

oatkiller Sep 10, 2020

oatkiller Sep 10, 2020

jonathan-buttner Sep 10, 2020

oatkiller Sep 10, 2020

michaelolo24 Sep 10, 2020

jonathan-buttner Sep 10, 2020

jonathan-buttner commented Sep 10, 2020

kibanamachine commented Sep 10, 2020

-    return tree[entityIDSafeVersion(events[0]) ?? ''];
+    const entityID = entityIDSafeVersion(events[0])
+    if (entityID === undefined) {
+      // this should never happen.
+      throw new Error()
+    }
+    return tree[entityIDSafeVersion(events[0])];

	return tree[entityIDSafeVersion(events[0]) ?? ''];
	return tree[entityIDSafeVersion(events[0])!];

[Security Solution] Use safe type in resolver backend #76969

[Security Solution] Use safe type in resolver backend #76969

Conversation

jonathan-buttner commented Sep 8, 2020

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

elasticmachine commented Sep 8, 2020

oatkiller 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jonathan-buttner commented Sep 10, 2020

kibanamachine commented Sep 10, 2020

💚 Build Succeeded

Build metrics

async chunks size

History