Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Security Solution][Resolver] Handle disabled process collection #73592

20 changes: 10 additions & 10 deletions x-pack/plugins/security_solution/common/endpoint/schema/resolver.ts
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@ import { schema } from '@kbn/config-schema';
* Used to validate GET requests for a complete resolver tree.
*/
export const validateTree = {
params: schema.object({ id: schema.string() }),
params: schema.object({ id: schema.string({ minLength: 1 }) }),
query: schema.object({
children: schema.number({ defaultValue: 200, min: 0, max: 10000 }),
ancestors: schema.number({ defaultValue: 200, min: 0, max: 10000 }),
Expand All @@ -19,54 +19,54 @@ export const validateTree = {
afterEvent: schema.maybe(schema.string()),
afterAlert: schema.maybe(schema.string()),
afterChild: schema.maybe(schema.string()),
legacyEndpointID: schema.maybe(schema.string()),
legacyEndpointID: schema.maybe(schema.string({ minLength: 1 })),
}),
};

/**
* Used to validate GET requests for non process events for a specific event.
*/
export const validateEvents = {
params: schema.object({ id: schema.string() }),
params: schema.object({ id: schema.string({ minLength: 1 }) }),
query: schema.object({
events: schema.number({ defaultValue: 1000, min: 1, max: 10000 }),
afterEvent: schema.maybe(schema.string()),
legacyEndpointID: schema.maybe(schema.string()),
legacyEndpointID: schema.maybe(schema.string({ minLength: 1 })),
}),
};

/**
* Used to validate GET requests for alerts for a specific process.
*/
export const validateAlerts = {
params: schema.object({ id: schema.string() }),
params: schema.object({ id: schema.string({ minLength: 1 }) }),
query: schema.object({
alerts: schema.number({ defaultValue: 1000, min: 1, max: 10000 }),
afterAlert: schema.maybe(schema.string()),
legacyEndpointID: schema.maybe(schema.string()),
legacyEndpointID: schema.maybe(schema.string({ minLength: 1 })),
}),
};

/**
* Used to validate GET requests for the ancestors of a process event.
*/
export const validateAncestry = {
params: schema.object({ id: schema.string() }),
params: schema.object({ id: schema.string({ minLength: 1 }) }),
query: schema.object({
ancestors: schema.number({ defaultValue: 200, min: 0, max: 10000 }),
legacyEndpointID: schema.maybe(schema.string()),
legacyEndpointID: schema.maybe(schema.string({ minLength: 1 })),
}),
};

/**
* Used to validate GET requests for children of a specified process event.
*/
export const validateChildren = {
params: schema.object({ id: schema.string() }),
params: schema.object({ id: schema.string({ minLength: 1 }) }),
query: schema.object({
children: schema.number({ defaultValue: 200, min: 1, max: 10000 }),
afterChild: schema.maybe(schema.string()),
legacyEndpointID: schema.maybe(schema.string()),
legacyEndpointID: schema.maybe(schema.string({ minLength: 1 })),
}),
};

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,13 @@

import { Ecs } from '../../../../graphql/types';

import { eventHasNotes, eventIsPinned, getPinTooltip, stringifyEvent } from './helpers';
import {
eventHasNotes,
eventIsPinned,
getPinTooltip,
stringifyEvent,
isInvestigateInResolverActionEnabled,
} from './helpers';
import { TimelineType } from '../../../../../common/types/timeline';

describe('helpers', () => {
Expand Down Expand Up @@ -242,4 +248,60 @@ describe('helpers', () => {
expect(eventIsPinned({ eventId, pinnedEventIds })).toEqual(false);
});
});

describe('isInvestigateInResolverActionEnabled', () => {
it('returns false if agent.type does not equal endpoint', () => {
const data: Ecs = { _id: '1', agent: { type: ['blah'] } };

expect(isInvestigateInResolverActionEnabled(data)).toBeFalsy();
});

it('returns false if agent.type does not have endpoint in first array index', () => {
const data: Ecs = { _id: '1', agent: { type: ['blah', 'endpoint'] } };

expect(isInvestigateInResolverActionEnabled(data)).toBeFalsy();
});

it('returns false if process.entity_id is not defined', () => {
const data: Ecs = { _id: '1', agent: { type: ['endpoint'] } };

expect(isInvestigateInResolverActionEnabled(data)).toBeFalsy();
});

it('returns false if process.entity_id is an empty array', () => {
const data: Ecs = { _id: '1', agent: { type: ['endpoint'] } };
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fyi, this is missing the empty array

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah thanks. I actually check that case below. Must be an accidental copy paste. I'll remove it.


expect(isInvestigateInResolverActionEnabled(data)).toBeFalsy();
});

it('returns true if agent.type has endpoint in first array index', () => {
const data: Ecs = {
_id: '1',
agent: { type: ['endpoint', 'blah'] },
process: { entity_id: ['5'] },
};

expect(isInvestigateInResolverActionEnabled(data)).toBeTruthy();
});

it('returns false if multiple entity_ids', () => {
const data: Ecs = {
_id: '1',
agent: { type: ['endpoint', 'blah'] },
process: { entity_id: ['5', '10'] },
};

expect(isInvestigateInResolverActionEnabled(data)).toBeFalsy();
});

it('returns false if entity_id is an empty string', () => {
const data: Ecs = {
_id: '1',
agent: { type: ['endpoint', 'blah'] },
process: { entity_id: [''] },
};

expect(isInvestigateInResolverActionEnabled(data)).toBeFalsy();
});
});
});
Original file line number Diff line number Diff line change
Expand Up @@ -106,7 +106,8 @@ export const getEventType = (event: Ecs): Omit<EventType, 'all'> => {
export const isInvestigateInResolverActionEnabled = (ecsData?: Ecs) => {
return (
get(['agent', 'type', 0], ecsData) === 'endpoint' &&
get(['process', 'entity_id'], ecsData)?.length > 0
get(['process', 'entity_id'], ecsData)?.length === 1 &&
get(['process', 'entity_id', 0], ecsData) !== ''
);
};

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -70,6 +70,13 @@ export function handleEntities(): RequestHandler<unknown, TypeOf<typeof validate
field: 'process.entity_id',
},
},
{
bool: {
must_not: {
term: { 'process.entity_id': '' },
},
},
},
],
},
},
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -37,7 +37,8 @@ export abstract class ResolverQuery<T, R = ResolverEvent> implements MSearchQuer
}

private buildQuery(ids: string | string[]): { query: JsonObject; index: string | string[] } {
const idsArray = ResolverQuery.createIdsArray(ids);
// only accept queries for entity_ids that are not an empty string
const idsArray = ResolverQuery.createIdsArray(ids).filter((id) => id !== '');
if (this.endpointID) {
return { query: this.legacyQuery(this.endpointID, idsArray), index: legacyEventIndexPattern };
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -74,6 +74,18 @@ export class ChildrenQuery extends ResolverQuery<ResolverEvent[]> {
],
},
},
{
exists: {
field: 'process.entity_id',
},
},
{
bool: {
must_not: {
term: { 'process.entity_id': '' },
},
},
},
{
term: { 'event.category': 'process' },
},
Expand Down
3 changes: 2 additions & 1 deletion x-pack/test/security_solution_endpoint_api_int/apis/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,8 @@ export default function endpointAPIIntegrationTests(providerContext: FtrProvider
before(async () => {
await ingestManager.setup();
});
loadTestFile(require.resolve('./resolver'));
loadTestFile(require.resolve('./resolver/entity_id'));
loadTestFile(require.resolve('./resolver/tree'));
loadTestFile(require.resolve('./metadata'));
loadTestFile(require.resolve('./policy'));
loadTestFile(require.resolve('./artifacts'));
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,156 @@
/*
* Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one
* or more contributor license agreements. Licensed under the Elastic License;
* you may not use this file except in compliance with the Elastic License.
*/
import expect from '@kbn/expect';
import { SearchResponse } from 'elasticsearch';
import { eventsIndexPattern } from '../../../../plugins/security_solution/common/endpoint/constants';
import {
ResolverTree,
ResolverEntityIndex,
} from '../../../../plugins/security_solution/common/endpoint/types';
import { FtrProviderContext } from '../../ftr_provider_context';
import {
EndpointDocGenerator,
Event,
} from '../../../../plugins/security_solution/common/endpoint/generate_data';
import { GeneratedEvents } from '../../services/resolver';

export default function resolverAPIIntegrationTests({ getService }: FtrProviderContext) {
const supertest = getService('supertest');
const resolver = getService('resolverGenerator');
const es = getService('es');
const generator = new EndpointDocGenerator('resolver');

describe('Resolver handling of entity ids', () => {
describe('entity api', () => {
let origin: Event;
let genData: GeneratedEvents;
before(async () => {
origin = generator.generateEvent({ parentEntityID: 'a' });
origin.process.entity_id = '';
genData = await resolver.insertEvents([origin]);
});

after(async () => {
await resolver.deleteData(genData);
});

it('excludes events that have an empty entity_id field', async () => {
// first lets get the _id of the document using the parent.process.entity_id
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nothing to block over, but can't you set _id when you insert a document?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah good point. I'm using a helper function that inserts an array of documents so it's doing a bulk request. Later we can refactor it so it specifies the _id of the document so we don't have to do the search.

// then we'll use the API to search for that specific document
const res = await es.search<SearchResponse<Event>>({
index: genData.indices[0],
body: {
query: {
bool: {
filter: [
{
term: { 'process.parent.entity_id': origin.process.parent!.entity_id },
},
],
},
},
},
});
const { body }: { body: ResolverEntityIndex } = await supertest.get(
// using the same indices value here twice to force the query parameter to be an array
// for some reason using supertest's query() function doesn't construct a parsable array
`/api/endpoint/resolver/entity?_id=${res.body.hits.hits[0]._id}&indices=${eventsIndexPattern}&indices=${eventsIndexPattern}`
);
expect(body).to.be.empty();
});
});

describe('children', () => {
let origin: Event;
let childNoEntityID: Event;
let childWithEntityID: Event;
let events: Event[];
let genData: GeneratedEvents;

before(async () => {
// construct a tree with an origin and two direct children. One child will not have an entity_id. That child
// should not be returned by the backend.
origin = generator.generateEvent({ entityID: 'a' });
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we should add tests of this kind to the UI too eventually

childNoEntityID = generator.generateEvent({
parentEntityID: origin.process.entity_id,
ancestry: [origin.process.entity_id],
});
// force it to be empty
childNoEntityID.process.entity_id = '';

childWithEntityID = generator.generateEvent({
entityID: 'b',
parentEntityID: origin.process.entity_id,
ancestry: [origin.process.entity_id],
});
events = [origin, childNoEntityID, childWithEntityID];
genData = await resolver.insertEvents(events);
});

after(async () => {
await resolver.deleteData(genData);
});

it('does not find children without a process entity_id', async () => {
const { body }: { body: ResolverTree } = await supertest
.get(`/api/endpoint/resolver/${origin.process.entity_id}`)
.expect(200);
expect(body.children.childNodes.length).to.be(1);
expect(body.children.childNodes[0].entityID).to.be(childWithEntityID.process.entity_id);
});
});

describe('ancestors', () => {
let origin: Event;
let ancestor1: Event;
let ancestor2: Event;
let ancestorNoEntityID: Event;
let events: Event[];
let genData: GeneratedEvents;

before(async () => {
// construct a tree with an origin that has two ancestors. The origin will have an empty string as one of the
// entity_ids in the ancestry array. This is to make sure that the backend will not query for that event.
ancestor2 = generator.generateEvent({
entityID: '2',
});
ancestor1 = generator.generateEvent({
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

having trouble following what ancestor1 is for. but i'm tired. maybe you can explain it to me next week :)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah it was more of a sanity check. The origin node in that test has its process.parent.entity_id set to ancestor1's process.entity_id, but the backend should be using the ancestry array to determine the ancestors if it exists. So since ancestor1 was not returned in the request we can be assured that the backend is properly using the ancestry array.

entityID: '1',
parentEntityID: ancestor2.process.entity_id,
ancestry: [ancestor2.process.entity_id],
});

// we'll insert an event that doesn't have an entity id so if the backend does search for it, it should be
// returned and our test should fail
ancestorNoEntityID = generator.generateEvent({
ancestry: [ancestor2.process.entity_id],
});
ancestorNoEntityID.process.entity_id = '';

origin = generator.generateEvent({
entityID: 'a',
parentEntityID: ancestor1.process.entity_id,
ancestry: ['', ancestor2.process.entity_id],
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

you could just make the first one here ancestorNoEntityID.process.entity_id too. Took me a sec to figure that's what was being referenced. Nothing to restart CI over though

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah good point. I added that late. I wanted to make sure that the backend wouldn't return the event even if ES actually did have a document with entity_id as an empty string

});

events = [origin, ancestor1, ancestor2, ancestorNoEntityID];
genData = await resolver.insertEvents(events);
});

after(async () => {
await resolver.deleteData(genData);
});

it('does not query for ancestors that have an empty string for the entity_id', async () => {
const { body }: { body: ResolverTree } = await supertest
.get(`/api/endpoint/resolver/${origin.process.entity_id}`)
.expect(200);
expect(body.ancestry.ancestors.length).to.be(1);
expect(body.ancestry.ancestors[0].entityID).to.be(ancestor2.process.entity_id);
});
});
});
}
Loading