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.

Sign up for GitHub

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

Jump to bottom

EMT-451: add ability to filter endpoint metadata based on presence of unenrolled events #69708

Merged
merged 15 commits into from
Jun 25, 2020
Merged

EMT-451: add ability to filter endpoint metadata based on presence of unenrolled events #69708

Show file tree
Hide file tree
Changes from 6 commits
Commits
Show all changes
15 commits
Select commit Hold shift + click to select a range
e30aba8
EMT-451: check i
nnamdifrankie Jun 21, 2020
1148c47
Merge branch 'master' into EMT-451_add_endpoint_enrolled_status_filte…
nnamdifrankie Jun 23, 2020
6862ae2
Merge branch 'master' into EMT-451_add_endpoint_enrolled_status_filte…
elasticmachine Jun 23, 2020
f7298fc
EMT-451: clean up
nnamdifrankie Jun 23, 2020
69d4b70
Merge branch 'EMT-451_add_endpoint_enrolled_status_filtering' of gith…
nnamdifrankie Jun 23, 2020
ce9f7bf
EMT-451: document the endpoint status, and make it strongly typed.
nnamdifrankie Jun 24, 2020
0dfd093
Merge branch 'master' into EMT-451_add_endpoint_enrolled_status_filte…
elasticmachine Jun 24, 2020
c5244e3
EMT-451: update case
nnamdifrankie Jun 24, 2020
e84be27
Merge branch 'EMT-451_add_endpoint_enrolled_status_filtering' of gith…
nnamdifrankie Jun 24, 2020
728d229
EMT-451: add import
nnamdifrankie Jun 24, 2020
3649efa
EMT-451: fix tests
nnamdifrankie Jun 24, 2020
825ebf0
EMT-451: review comments
nnamdifrankie Jun 24, 2020
866b3e2
EMT-451: clean up
nnamdifrankie Jun 24, 2020
b310f92
EMT-451: some more clean up
nnamdifrankie Jun 25, 2020
46d2130
EMT-451: use loop, remove await
nnamdifrankie Jun 25, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions x-pack/plugins/security_solution/common/endpoint/constants.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,5 +7,6 @@
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-*';
Copy link
Contributor

Choose a reason for hiding this comment

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

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

export const policyIndexPattern = 'metrics-endpoint.policy-*';
export const telemetryIndexPattern = 'metrics-endpoint.telemetry-*';
5 changes: 4 additions & 1 deletion x-pack/plugins/security_solution/common/endpoint/generate_data.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@ import {
HostPolicyResponse,
HostPolicyResponseActionStatus,
PolicyData,
EndpointStatus,
} from './types';
import { factory as policyFactory } from './models/policy_config';

Expand Down Expand Up @@ -209,6 +210,7 @@ interface HostInfo {
};
host: Host;
Endpoint: {
status: EndpointStatus;
policy: {
applied: {
id: string;
Expand Down Expand Up @@ -305,7 +307,7 @@ export class EndpointDocGenerator {
* Creates new random policy id for the host to simulate new policy application
*/
public updatePolicyId() {
this.commonInfo.Endpoint.policy.applied = this.randomChoice(APPLIED_POLICIES);
this.commonInfo.Endpoint.policy.applied.id = this.randomChoice(APPLIED_POLICIES).id;
this.commonInfo.Endpoint.policy.applied.status = this.randomChoice([
HostPolicyResponseActionStatus.success,
HostPolicyResponseActionStatus.failure,
Expand Down Expand Up @@ -333,6 +335,7 @@ export class EndpointDocGenerator {
os: this.randomChoice(OS),
},
Endpoint: {
status: EndpointStatus.ENROLLED,
policy: {
applied: this.randomChoice(APPLIED_POLICIES),
},
Expand Down
20 changes: 19 additions & 1 deletion x-pack/plugins/security_solution/common/endpoint/types.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -276,6 +276,7 @@ export interface AlertEvent {
type: string;
};
Endpoint: {
status: EndpointStatus;
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not really

policy: {
applied: {
id: string;
Expand Down Expand Up @@ -350,7 +351,23 @@ export interface AlertEvent {
}

/**
* The status of the host
* The status of the Endpoint Agent as reported by the Agent or the
* Security solution app using events from fleet.
*/
export enum EndpointStatus {
/**
* Agent is enrolled with Fleet
*/
ENROLLED = 'enrolled',
Copy link
Contributor

Choose a reason for hiding this comment

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

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.


/**
* Agent is unenrrolled from Fleet
*/
UNENROLLED = 'unenrolled',
}

/**
* The status of the host, which is mapped to the Elastic Agent status in Fleet
*/
export enum HostStatus {
/**
Expand Down Expand Up @@ -386,6 +403,7 @@ export type HostMetadata = Immutable<{
};
};
Endpoint: {
status: EndpointStatus;
policy: {
applied: {
id: string;
Expand Down
24 changes: 23 additions & 1 deletion x-pack/plugins/security_solution/server/endpoint/routes/metadata/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ import { IRouter, Logger, RequestHandlerContext } from 'kibana/server';
import { SearchResponse } from 'elasticsearch';
import { schema } from '@kbn/config-schema';

import Boom from 'boom';
import { metadataIndexPattern } from '../../../../common/endpoint/constants';
import { getESQueryHostMetadataByID, kibanaRequestToMetadataListESQuery } from './query_builders';
import {
Expand All @@ -18,6 +19,7 @@ import {
} from '../../../../common/endpoint/types';
import { EndpointAppContext } from '../../types';
import { AgentStatus } from '../../../../../ingest_manager/common/types/models';
import { findAllUnenrolledHostIds, findUnenrolledHostByHostId, HostId } from './support/unenroll';

interface HitSource {
_source: HostMetadata;
Expand Down Expand Up @@ -68,10 +70,17 @@ export function registerEndpointRoutes(router: IRouter, endpointAppContext: Endp
},
async (context, req, res) => {
try {
const unenrolledHostIds = await findAllUnenrolledHostIds(
context.core.elasticsearch.legacy.client
);

const queryParams = await kibanaRequestToMetadataListESQuery(
req,
endpointAppContext,
metadataIndexPattern
metadataIndexPattern,
{
unenrolledHostIds: unenrolledHostIds.map((host: HostId) => host.host.id),
}
);
const response = (await context.core.elasticsearch.legacy.client.callAsCurrentUser(
'search',
Expand Down Expand Up @@ -113,6 +122,12 @@ export function registerEndpointRoutes(router: IRouter, endpointAppContext: Endp
return res.notFound({ body: 'Endpoint Not Found' });
Copy link
Contributor

Choose a reason for hiding this comment

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

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?)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

} catch (err) {
logger.warn(JSON.stringify(err, null, 2));
if (err.isBoom) {
return res.customError({
statusCode: err.output.statusCode,
body: { message: err.message },
});
}
return res.internalError({ body: err });
}
}
Expand All @@ -123,6 +138,13 @@ export async function getHostData(
metadataRequestContext: MetadataRequestContext,
id: string
): Promise<HostInfo | undefined> {
const unenrolledHostId = await findUnenrolledHostByHostId(
metadataRequestContext.requestHandlerContext.core.elasticsearch.legacy.client,
id
);
if (unenrolledHostId) {
throw Boom.badRequest(`the requested endpoint is unerolled`);
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: unerolled -> unenrolled

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

Copy link
Contributor

Choose a reason for hiding this comment

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

Same here - should the message be i18n?

}
const query = getESQueryHostMetadataByID(id, metadataIndexPattern);
const response = (await metadataRequestContext.requestHandlerContext.core.elasticsearch.legacy.client.callAsCurrentUser(
'search',
Expand Down
130 changes: 100 additions & 30 deletions x-pack/plugins/security_solution/server/endpoint/routes/metadata/metadata.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,7 @@ import Boom from 'boom';
import { EndpointAppContextService } from '../../endpoint_app_context_services';
import { createMockConfig } from '../../../lib/detection_engine/routes/__mocks__';
import { EndpointDocGenerator } from '../../../../common/endpoint/generate_data';
import { HostId } from './support/unenroll';

describe('test endpoint route', () => {
let routerMock: jest.Mocked<IRouter>;
Expand All @@ -46,6 +47,12 @@ describe('test endpoint route', () => {
let routeConfig: RouteConfig<any, any, any, any>;
let mockAgentService: jest.Mocked<AgentService>;
let endpointAppContextService: EndpointAppContextService;
const noUnenrolledEndpoint = () =>
Promise.resolve(({
hits: {
hits: [],
},
} as unknown) as SearchResponse<HostId>);

beforeEach(() => {
mockClusterClient = elasticsearchServiceMock.createClusterClient() as jest.Mocked<
Expand Down Expand Up @@ -74,7 +81,9 @@ describe('test endpoint route', () => {
it('test find the latest of all endpoints', async () => {
const mockRequest = httpServerMock.createKibanaRequest({});
const response = createSearchResponse(new EndpointDocGenerator().generateHostMetadata());
mockScopedClient.callAsCurrentUser.mockImplementationOnce(() => Promise.resolve(response));
mockScopedClient.callAsCurrentUser
.mockImplementationOnce(noUnenrolledEndpoint)
.mockImplementationOnce(() => Promise.resolve(response));
[routeConfig, routeHandler] = routerMock.post.mock.calls.find(([{ path }]) =>
path.startsWith('/api/endpoint/metadata')
)!;
Expand All @@ -85,7 +94,7 @@ describe('test endpoint route', () => {
mockResponse
);

expect(mockScopedClient.callAsCurrentUser).toBeCalled();
expect(mockScopedClient.callAsCurrentUser).toHaveBeenCalledTimes(2);
expect(routeConfig.options).toEqual({ authRequired: true });
expect(mockResponse.ok).toBeCalled();
const endpointResultList = mockResponse.ok.mock.calls[0][0]?.body as HostResultList;
Expand All @@ -110,9 +119,11 @@ describe('test endpoint route', () => {
});

mockAgentService.getAgentStatusById = jest.fn().mockReturnValue('error');
mockScopedClient.callAsCurrentUser.mockImplementationOnce(() =>
Promise.resolve(createSearchResponse(new EndpointDocGenerator().generateHostMetadata()))
);
mockScopedClient.callAsCurrentUser
.mockImplementationOnce(noUnenrolledEndpoint)
.mockImplementationOnce(() =>
Promise.resolve(createSearchResponse(new EndpointDocGenerator().generateHostMetadata()))
);
[routeConfig, routeHandler] = routerMock.post.mock.calls.find(([{ path }]) =>
path.startsWith('/api/endpoint/metadata')
)!;
Expand All @@ -123,8 +134,8 @@ describe('test endpoint route', () => {
mockResponse
);

expect(mockScopedClient.callAsCurrentUser).toBeCalled();
expect(mockScopedClient.callAsCurrentUser.mock.calls[0][1]?.body?.query).toEqual({
expect(mockScopedClient.callAsCurrentUser).toHaveBeenCalledTimes(2);
expect(mockScopedClient.callAsCurrentUser.mock.calls[1][1]?.body?.query).toEqual({
match_all: {},
});
expect(routeConfig.options).toEqual({ authRequired: true });
Expand Down Expand Up @@ -153,9 +164,11 @@ describe('test endpoint route', () => {
});

mockAgentService.getAgentStatusById = jest.fn().mockReturnValue('error');
mockScopedClient.callAsCurrentUser.mockImplementationOnce(() =>
Promise.resolve(createSearchResponse(new EndpointDocGenerator().generateHostMetadata()))
);
mockScopedClient.callAsCurrentUser
.mockImplementationOnce(noUnenrolledEndpoint)
.mockImplementationOnce(() =>
Promise.resolve(createSearchResponse(new EndpointDocGenerator().generateHostMetadata()))
);
[routeConfig, routeHandler] = routerMock.post.mock.calls.find(([{ path }]) =>
path.startsWith('/api/endpoint/metadata')
)!;
Expand All @@ -167,20 +180,26 @@ describe('test endpoint route', () => {
);

expect(mockScopedClient.callAsCurrentUser).toBeCalled();
expect(mockScopedClient.callAsCurrentUser.mock.calls[0][1]?.body?.query).toEqual({
expect(mockScopedClient.callAsCurrentUser.mock.calls[1][1]?.body?.query).toEqual({
bool: {
must_not: {
bool: {
minimum_should_match: 1,
should: [
{
match: {
'host.ip': '10.140.73.246',
must: [
{
bool: {
must_not: {
bool: {
minimum_should_match: 1,
should: [
{
match: {
'host.ip': '10.140.73.246',
},
},
],
},
},
],
},
},
},
],
},
});
expect(routeConfig.options).toEqual({ authRequired: true });
Expand All @@ -196,9 +215,10 @@ describe('test endpoint route', () => {
it('should return 404 on no results', async () => {
const mockRequest = httpServerMock.createKibanaRequest({ params: { id: 'BADID' } });

mockScopedClient.callAsCurrentUser.mockImplementationOnce(() =>
Promise.resolve(createSearchResponse())
);
mockScopedClient.callAsCurrentUser
.mockImplementationOnce(noUnenrolledEndpoint)
.mockImplementationOnce(() => Promise.resolve(createSearchResponse()));

mockAgentService.getAgentStatusById = jest.fn().mockReturnValue('error');
[routeConfig, routeHandler] = routerMock.get.mock.calls.find(([{ path }]) =>
path.startsWith('/api/endpoint/metadata')
Expand All @@ -209,7 +229,7 @@ describe('test endpoint route', () => {
mockResponse
);

expect(mockScopedClient.callAsCurrentUser).toBeCalled();
expect(mockScopedClient.callAsCurrentUser).toHaveBeenCalledTimes(2);
expect(routeConfig.options).toEqual({ authRequired: true });
expect(mockResponse.notFound).toBeCalled();
const message = mockResponse.notFound.mock.calls[0][0]?.body;
Expand All @@ -221,8 +241,12 @@ describe('test endpoint route', () => {
const mockRequest = httpServerMock.createKibanaRequest({
params: { id: response.hits.hits[0]._id },
});

mockAgentService.getAgentStatusById = jest.fn().mockReturnValue('online');
mockScopedClient.callAsCurrentUser.mockImplementationOnce(() => Promise.resolve(response));
mockScopedClient.callAsCurrentUser
.mockImplementationOnce(noUnenrolledEndpoint)
.mockImplementationOnce(() => Promise.resolve(response));

[routeConfig, routeHandler] = routerMock.get.mock.calls.find(([{ path }]) =>
path.startsWith('/api/endpoint/metadata')
)!;
Expand All @@ -233,7 +257,7 @@ describe('test endpoint route', () => {
mockResponse
);

expect(mockScopedClient.callAsCurrentUser).toBeCalled();
expect(mockScopedClient.callAsCurrentUser).toHaveBeenCalledTimes(2);
expect(routeConfig.options).toEqual({ authRequired: true });
expect(mockResponse.ok).toBeCalled();
const result = mockResponse.ok.mock.calls[0][0]?.body as HostInfo;
Expand All @@ -251,7 +275,11 @@ describe('test endpoint route', () => {
mockAgentService.getAgentStatusById = jest.fn().mockImplementation(() => {
throw Boom.notFound('Agent not found');
});
mockScopedClient.callAsCurrentUser.mockImplementationOnce(() => Promise.resolve(response));

mockScopedClient.callAsCurrentUser
.mockImplementationOnce(noUnenrolledEndpoint)
.mockImplementationOnce(() => Promise.resolve(response));

[routeConfig, routeHandler] = routerMock.get.mock.calls.find(([{ path }]) =>
path.startsWith('/api/endpoint/metadata')
)!;
Expand All @@ -262,7 +290,7 @@ describe('test endpoint route', () => {
mockResponse
);

expect(mockScopedClient.callAsCurrentUser).toBeCalled();
expect(mockScopedClient.callAsCurrentUser).toHaveBeenCalledTimes(2);
expect(routeConfig.options).toEqual({ authRequired: true });
expect(mockResponse.ok).toBeCalled();
const result = mockResponse.ok.mock.calls[0][0]?.body as HostInfo;
Expand All @@ -277,7 +305,11 @@ describe('test endpoint route', () => {
});

mockAgentService.getAgentStatusById = jest.fn().mockReturnValue('warning');
mockScopedClient.callAsCurrentUser.mockImplementationOnce(() => Promise.resolve(response));

mockScopedClient.callAsCurrentUser
.mockImplementationOnce(noUnenrolledEndpoint)
.mockImplementationOnce(() => Promise.resolve(response));

[routeConfig, routeHandler] = routerMock.get.mock.calls.find(([{ path }]) =>
path.startsWith('/api/endpoint/metadata')
)!;
Expand All @@ -288,12 +320,50 @@ describe('test endpoint route', () => {
mockResponse
);

expect(mockScopedClient.callAsCurrentUser).toBeCalled();
expect(mockScopedClient.callAsCurrentUser).toHaveBeenCalledTimes(2);
expect(routeConfig.options).toEqual({ authRequired: true });
expect(mockResponse.ok).toBeCalled();
const result = mockResponse.ok.mock.calls[0][0]?.body as HostInfo;
expect(result.host_status).toEqual(HostStatus.ERROR);
});

it('should throw error when endpoint is unenrolled', async () => {
const mockRequest = httpServerMock.createKibanaRequest({
params: { id: 'hostId' },
});

mockScopedClient.callAsCurrentUser.mockImplementationOnce(() =>
Promise.resolve(({
hits: {
hits: [
{
_index: 'metrics-endpoint.metadata_mirror-default-1',
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: remove the trailing -1 from the index name

_id: 'S5M1yHIBLSMVtiLw6Wpr',
_score: 0.0,
_source: {
host: {
id: 'hostId',
},
},
},
],
},
} as unknown) as SearchResponse<HostId>)
);

[routeConfig, routeHandler] = routerMock.get.mock.calls.find(([{ path }]) =>
path.startsWith('/api/endpoint/metadata')
)!;

await routeHandler(
createRouteHandlerContext(mockScopedClient, mockSavedObjectClient),
mockRequest,
mockResponse
);

expect(mockScopedClient.callAsCurrentUser).toHaveBeenCalledTimes(1);
expect(mockResponse.customError).toBeCalled();
});
});
});

Expand Down
Loading