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][ENDPOINT] Trusted Apps List API #75476

Merged
Merged
Show file tree
Hide file tree
Changes from 13 commits
Commits
Show all changes
17 commits
Select commit Hold shift + click to select a range
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
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,11 @@ import {
_VERSION,
} from '../../constants.mock';
import { ENDPOINT_LIST_ID } from '../..';
import {
ENDPOINT_TRUSTED_APPS_LIST_DESCRIPTION,
ENDPOINT_TRUSTED_APPS_LIST_ID,
ENDPOINT_TRUSTED_APPS_LIST_NAME,
} from '../../constants';

import { ExceptionListSchema } from './exception_list_schema';

Expand All @@ -42,6 +47,15 @@ export const getExceptionListSchemaMock = (): ExceptionListSchema => ({
version: VERSION,
});

export const getTrustedAppsListSchemaMock = (): ExceptionListSchema => {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@madirey FYI
Created this so that the client mock (below) can include it. I debated whether I should have created a sub-class of the Client under trusted apps and felt it belonged here more than in our feature code

return {
...getExceptionListSchemaMock(),
description: ENDPOINT_TRUSTED_APPS_LIST_DESCRIPTION,
list_id: ENDPOINT_TRUSTED_APPS_LIST_ID,
name: ENDPOINT_TRUSTED_APPS_LIST_NAME,
};
};

/**
* This is useful for end to end tests where we remove the auto generated parts for comparisons
* such as created_at, updated_at, and id.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,10 @@ import { savedObjectsClientMock } from 'src/core/server/mocks';
import { getFoundExceptionListSchemaMock } from '../../../common/schemas/response/found_exception_list_schema.mock';
import { getFoundExceptionListItemSchemaMock } from '../../../common/schemas/response/found_exception_list_item_schema.mock';
import { getExceptionListItemSchemaMock } from '../../../common/schemas/response/exception_list_item_schema.mock';
import { getExceptionListSchemaMock } from '../../../common/schemas/response/exception_list_schema.mock';
import {
getExceptionListSchemaMock,
getTrustedAppsListSchemaMock,
} from '../../../common/schemas/response/exception_list_schema.mock';

import { ExceptionListClient } from './exception_list_client';

Expand All @@ -24,6 +27,7 @@ export class ExceptionListClientMock extends ExceptionListClient {
public deleteExceptionListItem = jest.fn().mockResolvedValue(getExceptionListItemSchemaMock());
public findExceptionListItem = jest.fn().mockResolvedValue(getFoundExceptionListItemSchemaMock());
public findExceptionList = jest.fn().mockResolvedValue(getFoundExceptionListSchemaMock());
public createTrustedAppsList = jest.fn().mockResolvedValue(getTrustedAppsListSchemaMock());
}

export const getExceptionListClientMock = (): ExceptionListClient => {
Expand Down
2 changes: 2 additions & 0 deletions x-pack/plugins/security_solution/common/endpoint/constants.ts
Original file line number Diff line number Diff line change
Expand Up @@ -11,3 +11,5 @@ export const policyIndexPattern = 'metrics-endpoint.policy-*';
export const telemetryIndexPattern = 'metrics-endpoint.telemetry-*';
export const LIMITED_CONCURRENCY_ENDPOINT_ROUTE_TAG = 'endpoint:limited-concurrency';
export const LIMITED_CONCURRENCY_ENDPOINT_COUNT = 100;

export const TRUSTED_APPS_LIST_API = '/api/endpoint/trusted_apps';
Copy link
Contributor

Choose a reason for hiding this comment

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

why /endpoint?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

other endpoint apis are mounted at this location

Original file line number Diff line number Diff line change
@@ -0,0 +1,71 @@
/*
* 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 { GetTrustedAppsRequestSchema } from './trusted_apps';

describe('When invoking Trusted Apps Schema', () => {
describe('for GET List', () => {
const getListQueryParams = (page: unknown = 1, perPage: unknown = 20) => ({
page,
per_page: perPage,
});
const query = GetTrustedAppsRequestSchema.query;

describe('query param validation', () => {
it('should return query params if valid', () => {
expect(query.validate(getListQueryParams())).toEqual({
page: 1,
per_page: 20,
});
});

it('should use default values', () => {
expect(query.validate(getListQueryParams(undefined, undefined))).toEqual({
page: 1,
per_page: 20,
});
expect(query.validate(getListQueryParams(undefined, 100))).toEqual({
page: 1,
per_page: 100,
});
expect(query.validate(getListQueryParams(10, undefined))).toEqual({
page: 10,
per_page: 20,
});
});

it('should throw if `page` param is not a number', () => {
expect(() => {
query.validate(getListQueryParams('one'));
}).toThrowError();
});

it('should throw if `page` param is less than 1', () => {
expect(() => {
query.validate(getListQueryParams(0));
}).toThrowError();
expect(() => {
query.validate(getListQueryParams(-1));
}).toThrowError();
});

it('should throw if `per_page` param is not a number', () => {
expect(() => {
query.validate(getListQueryParams(1, 'twenty'));
}).toThrowError();
});

it('should throw if `per_page` param is less than 1', () => {
expect(() => {
query.validate(getListQueryParams(1, 0));
}).toThrowError();
expect(() => {
query.validate(getListQueryParams(1, -1));
}).toThrowError();
});
});
});
});
Original file line number Diff line number Diff line change
@@ -0,0 +1,14 @@
/*
* 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 { schema } from '@kbn/config-schema';

export const GetTrustedAppsRequestSchema = {
query: schema.object({
page: schema.maybe(schema.number({ defaultValue: 1, min: 1 })),
Copy link
Contributor

Choose a reason for hiding this comment

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

I always assumed 0 based is easier to implement, but maybe it's true that we need to show one based.

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'm of the opposite opinion - I find it 1 based to be clearer from a consumer (UI?) standpoint. I want page 1.
The service is 1 based and FYI - we have an issue opened to also refactor Endpoint hosts page to use 1 based pagination.

Copy link
Member

Choose a reason for hiding this comment

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

+1 to 1-based page numbers.

2020-08-25-123057_scrot

I consider the URL-bar to be human-facing as much as this

per_page: schema.maybe(schema.number({ defaultValue: 20, min: 1 })),
Copy link
Contributor

Choose a reason for hiding this comment

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

Just out of curiosity and not for bikesheding - why not page_size?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

mirror of lists API - they use these values. I also seem to remember that anything outside of code should use _ and only code vars/etc. should camelCase. I think Kibana even has an ESLint rule for it.

}),
};
Original file line number Diff line number Diff line change
Expand Up @@ -5,8 +5,10 @@
*/

import { ApplicationStart } from 'kibana/public';
import { NewPackagePolicy, PackagePolicy } from '../../../ingest_manager/common';
import { ManifestSchema } from './schema/manifest';
import { NewPackagePolicy, PackagePolicy } from '../../../../ingest_manager/common';
import { ManifestSchema } from '../schema/manifest';

export * from './trusted_apps';

/**
* Supported React-Router state for the Policy Details page
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,30 @@
/*
* 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 { TypeOf } from '@kbn/config-schema';
import { GetTrustedAppsRequestSchema } from '../schema/trusted_apps';
import { ExceptionListItemSchema } from '../../../../lists/common';
import { FoundExceptionListItemSchema } from '../../../../lists/common/schemas/response';

/** API request params for retrieving a list of Trusted Apps */
export type GetTrustedAppsListRequest = TypeOf<typeof GetTrustedAppsRequestSchema.query>;
export type GetTrustedListAppsResponse = Pick<
FoundExceptionListItemSchema,
'per_page' | 'page' | 'total'
> & {
data: TrustedApp[];
};

/** Type for a new Trusted App Entry */
export type NewTrustedApp = Pick<ExceptionListItemSchema, 'name' | 'description' | 'entries'> & {
os: string;
Copy link
Contributor

Choose a reason for hiding this comment

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

It feels like enum would be more appropriate.

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 will probably be an enum - just need to find the one that List is using (if one exists) and reuse it or come up with our own. FYI: this os field does not exist today on the data storage schema - the Trusted Apps API will populate it from the _tags[] array.

Copy link
Contributor

Choose a reason for hiding this comment

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

we could introduce an enum with with the Create flow and validation

Copy link
Contributor

Choose a reason for hiding this comment

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

We have a ticket to add the os to the exception list item schema in 7.10

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, we can create the Enum either with the UI changes or when we add the Create api.

@peluja1012 sounds good. Madi had mentioned that to me, so we're aware we will have to adjust our "translator" once that gets introduced.

};

/** A trusted app entry */
export type TrustedApp = NewTrustedApp &
Pick<ExceptionListItemSchema, 'created_at' | 'created_by'> & {
id: string;
};
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
#!/usr/bin/env node
/*
* 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.
*/

require('../../../../../src/setup_node_env');
require('./trusted_apps').cli();
Original file line number Diff line number Diff line change
@@ -0,0 +1,91 @@
/*
Copy link
Contributor Author

Choose a reason for hiding this comment

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

FYI - this utility is to assist with dev. of the Trusted Apps list until we have a create flow. I would expect this to be deleted once the create flow is merged.

* 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 { v4 as generateUUID } from 'uuid';
// @ts-ignore
import minimist from 'minimist';
import { KbnClient, ToolingLog } from '@kbn/dev-utils';
import { ENDPOINT_TRUSTED_APPS_LIST_ID } from '../../../../lists/common/constants';
import { TRUSTED_APPS_LIST_API } from '../../../common/endpoint/constants';
import { ExceptionListItemSchema } from '../../../../lists/common/schemas/response';

interface RunOptions {
count?: number;
}

const logger = new ToolingLog({ level: 'info', writeTo: process.stdout });
const separator = '----------------------------------------';

export const cli = async () => {
const options: RunOptions = minimist(process.argv.slice(2), {
default: {
count: 10,
},
});
logger.write(`${separator}
Loading ${options.count} Trusted App Entries`);
await run(options);
logger.write(`Done!
${separator}`);
};

export const run: (options?: RunOptions) => Promise<ExceptionListItemSchema[]> = async ({
count = 10,
}: RunOptions = {}) => {
const kbnClient = new KbnClient(logger, { url: 'http://elastic:changeme@localhost:5601' });

// touch the Trusted Apps List so it can be created
await kbnClient.request({
method: 'GET',
path: TRUSTED_APPS_LIST_API,
});

return Promise.all(
Array.from({ length: count }, () => {
return kbnClient
.request({
method: 'POST',
path: '/api/exception_lists/items',
body: generateTrustedAppEntry(),
})
.then<ExceptionListItemSchema>((item) => (item as unknown) as ExceptionListItemSchema);
})
);
};

interface GenerateTrustedAppEntryOptions {
os?: 'windows' | 'macos' | 'linux';
name?: string;
}

const generateTrustedAppEntry: (options?: GenerateTrustedAppEntryOptions) => object = ({
os = 'windows',
name = `Sample Endpoint Trusted App Entry ${Date.now()}`,
} = {}) => {
return {
list_id: ENDPOINT_TRUSTED_APPS_LIST_ID,
item_id: `generator_endpoint_trusted_apps_${generateUUID()}`,
_tags: ['endpoint', `os:${os}`],
tags: ['user added string for a tag', 'malware'],
type: 'simple',
description: 'This is a sample agnostic endpoint trusted app entry',
name,
namespace_type: 'agnostic',
entries: [
{
field: 'actingProcess.file.signer',
operator: 'included',
type: 'match',
value: 'Elastic, N.V.',
},
{
field: 'actingProcess.file.path',
operator: 'included',
type: 'match',
value: '/one/two/three',
},
],
};
};
Original file line number Diff line number Diff line change
Expand Up @@ -12,10 +12,12 @@ import {
import { AgentService, IngestManagerStartContract } from '../../../ingest_manager/server';
import { getPackagePolicyCreateCallback } from './ingest_integration';
import { ManifestManager } from './services/artifacts';
import { ExceptionListClient } from '../../../lists/server';

export type EndpointAppContextServiceStartContract = Partial<
Pick<IngestManagerStartContract, 'agentService'>
> & {
exceptionsListService: ExceptionListClient;
logger: Logger;
manifestManager?: ManifestManager;
registerIngestCallback?: IngestManagerStartContract['registerExternalCallback'];
Expand All @@ -30,9 +32,11 @@ export class EndpointAppContextService {
private agentService: AgentService | undefined;
private manifestManager: ManifestManager | undefined;
private savedObjectsStart: SavedObjectsServiceStart | undefined;
private exceptionsListService: ExceptionListClient | undefined;

public start(dependencies: EndpointAppContextServiceStartContract) {
this.agentService = dependencies.agentService;
this.exceptionsListService = dependencies.exceptionsListService;
this.manifestManager = dependencies.manifestManager;
this.savedObjectsStart = dependencies.savedObjectsStart;

Expand All @@ -50,6 +54,13 @@ export class EndpointAppContextService {
return this.agentService;
}

public getExceptionsList() {
if (!this.exceptionsListService) {
throw new Error('exceptionsListService not set');
}
return this.exceptionsListService;
}

public getManifestManager(): ManifestManager | undefined {
return this.manifestManager;
}
Expand Down
2 changes: 2 additions & 0 deletions x-pack/plugins/security_solution/server/endpoint/mocks.ts
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@ import {
import { ManifestManager } from './services/artifacts/manifest_manager/manifest_manager';
import { getManifestManagerMock } from './services/artifacts/manifest_manager/manifest_manager.mock';
import { EndpointAppContext } from './types';
import { listMock } from '../../../lists/server/mocks';

/**
* Creates a mocked EndpointAppContext.
Expand Down Expand Up @@ -58,6 +59,7 @@ export const createMockEndpointAppContextServiceStartContract = (): jest.Mocked<
> => {
return {
agentService: createMockAgentService(),
exceptionsListService: listMock.getExceptionListClient(),
logger: loggingSystemMock.create().get('mock_endpoint_app_context'),
savedObjectsStart: savedObjectsServiceMock.createStartContract(),
manifestManager: getManifestManagerMock(),
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,49 @@
/*
* 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 { RequestHandler } from 'kibana/server';
import {
GetTrustedAppsListRequest,
GetTrustedListAppsResponse,
} from '../../../../common/endpoint/types';
import { EndpointAppContext } from '../../types';
import { exceptionItemToTrustedAppItem } from './utils';
import { ENDPOINT_TRUSTED_APPS_LIST_ID } from '../../../../../lists/common/constants';

export const getTrustedAppsListRouteHandler = (
endpointAppContext: EndpointAppContext
): RequestHandler<undefined, GetTrustedAppsListRequest> => {
paul-tavares marked this conversation as resolved.
Show resolved Hide resolved
const logger = endpointAppContext.logFactory.get('trusted_apps');

return async (context, req, res) => {
const exceptionsListService = endpointAppContext.service.getExceptionsList();
const { page, per_page: perPage } = req.query;
Copy link
Contributor

Choose a reason for hiding this comment

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

This is why I'm wondering if it's a convention to follow "_" naming style for REST :)


try {
// Ensure list is created if it does not exist
await exceptionsListService?.createTrustedAppsList();
const results = await exceptionsListService.findExceptionListItem({
listId: ENDPOINT_TRUSTED_APPS_LIST_ID,
page,
perPage,
filter: undefined,
namespaceType: 'agnostic',
sortField: 'name',
sortOrder: 'asc',
});
const body: GetTrustedListAppsResponse = {
data: results?.data.map(exceptionItemToTrustedAppItem) ?? [],
total: results?.total ?? 0,
page: results?.page ?? 1,
per_page: perPage!,
Copy link
Contributor

Choose a reason for hiding this comment

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

No default?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

perPage default to 20

Copy link
Contributor

Choose a reason for hiding this comment

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

Also this value is already known to the client when making request, why send it back?

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 see other APIs "echo" the perPage count back. We can remove it if you feel strongly about it, but I was just trying to remain consistent

};
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure what @nnamdifrankie and @pzl have to say, but I would personally always introduce the service layer function to wrap the call to exceptionsListService. My rule of thumb is to have controller call service from the same logical domain, that scales better in terms of future evolvement.

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 can change the exceptionItemToTrustedAppItem() method to work on an array of ExceptionsListItemSchema and just pass that in. Of are you saying I should some a method that actually will define the entire HTTP response body?

return res.ok({ body });
} catch (error) {
logger.error(error);
return res.internalError({ body: error });
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure do we follow REST conventions for response codes? I can imagine there are errors that are client errors that would trigger 400 or 403.

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 think if you come here, we don't know if its a 400 (I belive the platform may handle 403). These I think these should be 500's.

Copy link
Member

Choose a reason for hiding this comment

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

if we can prove for sure it was a client error then it should definitely be 4xx error. And I believe a few of those cases are already caught (not authorized: 403, due to authRequired: true, though I'm not sure if we also need the tags: ['access:securitySolution']; and schema should catch bad parameters and give some 4xx status codes, probably 400).

If we can't prove it's a request fault, better to say 'excuse me' (5xx) than "I can't believe you've done this" (4xx)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We can revisit this later to see if it needs more interpretation. but given what is inside of the try {}, I'm not sure just how much logic we would need to decipher if the error that came in here in order to determine if a 4xx is appropriate. This block here is a "catch all" - maybe you guys see a missing try {} inside of the above code that I might be missing where its catch() should return a 4xx?

}
};
};
Loading