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] Upload response action create API #156303

Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
32 commits
Select commit Hold shift + click to select a range
8cd5527
Added `responseActionUploadEnabled` feature flag
paul-tavares May 1, 2023
0dfb681
Add API route and define `maxUploadResponseActionFileBytes` in server…
paul-tavares May 1, 2023
eb51dad
Use max uplaod size from config
paul-tavares May 1, 2023
59f12ec
refactor: move `HapiReadableStream` to top-level types for reuse
paul-tavares May 1, 2023
38420d7
Initial setup of Route handler and `createFile()` service method
paul-tavares May 1, 2023
a2887e9
Merge remote-tracking branch 'upstream/main' into task/olm-5369-uploa…
paul-tavares May 1, 2023
2dfe9ba
action file service for `crateFile()` and `deleteFile()`
paul-tavares May 1, 2023
3a02739
Create the file in route handler and then pass data long to create th…
paul-tavares May 1, 2023
82859a7
move FF check to the `register` function
paul-tavares May 1, 2023
ad57cf7
updated metadata generator to include `upload` capability value
paul-tavares May 1, 2023
e0b154d
refactored how server config is passed down to route setup during `se…
paul-tavares May 1, 2023
086d286
[CI] Auto-commit changed files from 'node scripts/lint_ts_projects --…
kibanamachine May 1, 2023
11e9590
Types for Upload response content and params
paul-tavares May 2, 2023
d905916
ActionCreateService: converted `createAction` to a generic function
paul-tavares May 2, 2023
6f1736b
Save action id to File uploaded
paul-tavares May 2, 2023
8ffb88b
Updated `createFileServiceMock()` `toJSON` method to return fileMock.…
paul-tavares May 2, 2023
e35196d
Updated EndpointActionGenerator.generateActionDetails() to return `up…
paul-tavares May 2, 2023
7135355
Action Files test for new `createFile`, `setFileActionId` and `delete…
paul-tavares May 2, 2023
25c353a
Tests for `UploadActionRequestSchema`
paul-tavares May 2, 2023
ad0e3dc
Tests for Upload API route registration
paul-tavares May 2, 2023
71118bf
Merge remote-tracking branch 'origin/task/olm-5369-upload-response-ac…
paul-tavares May 2, 2023
e331bcc
Upload api handler tests
paul-tavares May 3, 2023
fff1737
Merge remote-tracking branch 'upstream/main' into task/olm-5369-uploa…
paul-tavares May 3, 2023
4d5bd65
adjust response action list UI to only show `upload` if FF is true
paul-tavares May 3, 2023
a7586bb
Fix mocks
paul-tavares May 3, 2023
5ed98c0
Fix test unused variables
paul-tavares May 3, 2023
9969053
Merge remote-tracking branch 'upstream/main' into task/olm-5369-uploa…
paul-tavares May 3, 2023
098ab0e
update failing test for action log
paul-tavares May 3, 2023
1f3bd9b
Merge remote-tracking branch 'upstream/main' into task/olm-5369-uploa…
paul-tavares May 4, 2023
e32193f
fix test failures due to earlier commit using `createMockEndpointAppC…
paul-tavares May 4, 2023
3fb73fe
Fix tests after merge from `main`
paul-tavares May 4, 2023
b15e022
Merge branch 'main' into task/olm-5369-upload-response-action-api
paul-tavares May 4, 2023
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
5 changes: 4 additions & 1 deletion src/plugins/files/server/mocks.ts
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@
import { KibanaRequest } from '@kbn/core/server';
import { DeeplyMockedKeys } from '@kbn/utility-types-jest';
import * as stream from 'stream';
import { clone } from 'lodash';
import { File } from '../common';
import { FileClient, FileServiceFactory, FileServiceStart, FilesSetup } from '.';

Expand Down Expand Up @@ -56,7 +57,9 @@ export const createFileMock = (): DeeplyMockedKeys<File> => {
share: jest.fn(),
listShares: jest.fn(),
unshare: jest.fn(),
toJSON: jest.fn(),
toJSON: jest.fn(() => {
return clone(fileMock.data);
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: why clone?

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 used clone() so that the data being return by this mock is not a reference to the data stored in the instance. Thus, the data returned can be Mutated (in a tests) without impacting the state of the File class instance

}),
};

fileMock.update.mockResolvedValue(fileMock);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -82,6 +82,7 @@ export const KILL_PROCESS_ROUTE = `${BASE_ENDPOINT_ACTION_ROUTE}/kill_process`;
export const SUSPEND_PROCESS_ROUTE = `${BASE_ENDPOINT_ACTION_ROUTE}/suspend_process`;
export const GET_FILE_ROUTE = `${BASE_ENDPOINT_ACTION_ROUTE}/get_file`;
export const EXECUTE_ROUTE = `${BASE_ENDPOINT_ACTION_ROUTE}/execute`;
export const UPLOAD_ROUTE = `${BASE_ENDPOINT_ACTION_ROUTE}/upload`;

/** Endpoint Actions Routes */
export const ENDPOINT_ACTION_LOG_ROUTE = `${BASE_ENDPOINT_ROUTE}/action_log/{agent_id}`;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,8 @@ import type {
ResponseActionGetFileParameters,
ResponseActionsExecuteParameters,
ResponseActionExecuteOutputContent,
ResponseActionUploadOutputContent,
ResponseActionUploadParameters,
} from '../types';
import { ActivityLogItemTypes } from '../types';
import {
Expand Down Expand Up @@ -239,6 +241,24 @@ export class EndpointActionGenerator extends BaseDataGenerator {
}
}

if (command === 'upload') {
Copy link
Contributor

Choose a reason for hiding this comment

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

Why don't you prepare an enum with all command names?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Don't really like using Enums from TS. They are a bit strange to use and I often find my self casting the type of a string to match the Enum definition when attempting to look up a value in the Enum

if (!details.parameters) {
(
details as ActionDetails<
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 cast?

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 because in order to set the parameters below, the ActionDetails must be an upload action details, so the cast here using the output content and parameters type does that.

ResponseActionUploadOutputContent,
ResponseActionUploadParameters
>
).parameters = {
file: {
file_id: 'file-x-y-z',
file_name: 'foo.txt',
size: 1234,
sha256: 'file-hash-sha-256',
},
};
}
}

return merge(details, overrides as ActionDetails) as unknown as ActionDetails<
TOutputType,
TParameters
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -129,6 +129,11 @@ export class EndpointMetadataGenerator extends BaseDataGenerator {
capabilities.push('execute');
}

// v8.9 introduced `upload` capability
if (gte(agentVersion, '8.9.0')) {
capabilities.push('upload_file');
}

const hostMetadataDoc: HostMetadataInterface = {
'@timestamp': ts,
event: {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,10 @@ import {
NoParametersRequestSchema,
KillOrSuspendProcessRequestSchema,
ExecuteActionRequestSchema,
UploadActionRequestSchema,
} from './actions';
import { createHapiReadableStreamMock } from '../../../server/endpoint/services/actions/mocks';
import type { HapiReadableStream } from '../../../server/types';

describe('actions schemas', () => {
describe('Endpoint action list API Schema', () => {
Expand Down Expand Up @@ -639,4 +642,56 @@ describe('actions schemas', () => {
}).not.toThrow();
});
});

describe(`UploadActionRequestSchema`, () => {
let fileStream: HapiReadableStream;

beforeEach(() => {
fileStream = createHapiReadableStreamMock();
});

it('should not error if `override` parameter is not defined', () => {
Copy link
Member

Choose a reason for hiding this comment

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

nit: overwrite parameter

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Will correct in next PR. 👍

expect(() => {
UploadActionRequestSchema.body.validate({
endpoint_ids: ['endpoint_id'],
file: fileStream,
});
}).not.toThrow();
});

it('should allow `override` parameter', () => {
Copy link
Member

Choose a reason for hiding this comment

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

nit: overwrite

Copy link
Contributor Author

Choose a reason for hiding this comment

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

same here. Thanks for catching this

expect(() => {
UploadActionRequestSchema.body.validate({
endpoint_ids: ['endpoint_id'],
parameters: {
overwrite: true,
},
file: fileStream,
});
}).not.toThrow();
});

it('should error if `file` is not defined', () => {
expect(() => {
UploadActionRequestSchema.body.validate({
endpoint_ids: ['endpoint_id'],
parameters: {
override: true,
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
override: true,
overwrite: true,

Copy link
Contributor Author

Choose a reason for hiding this comment

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

😱
Good catch. this was actually probably throw'ing an incorrect error for what the test is attempting to validate. Will correct it in next PR.

},
});
}).toThrow();
});

it('should error if `file` is not a Stream', () => {
expect(() => {
UploadActionRequestSchema.body.validate({
endpoint_ids: ['endpoint_id'],
parameters: {
overwrite: true,
},
file: {},
});
}).toThrow();
});
});
});
14 changes: 14 additions & 0 deletions x-pack/plugins/security_solution/common/endpoint/schema/actions.ts
Original file line number Diff line number Diff line change
Expand Up @@ -239,3 +239,17 @@ export const ResponseActionBodySchema = schema.oneOf([
EndpointActionGetFileSchema.body,
ExecuteActionRequestSchema.body,
]);

export const UploadActionRequestSchema = {
body: schema.object({
...BaseActionRequestSchema,

parameters: schema.object({
overwrite: schema.maybe(schema.boolean()),
Copy link
Member

Choose a reason for hiding this comment

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

There should be a default value of false for overwrite here. No?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good point. I can set it a default in next PR

}),

file: schema.stream(),
}),
};

export type UploadActionRequestBody = TypeOf<typeof UploadActionRequestSchema.body>;
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@ export const RESPONSE_ACTION_API_COMMANDS_NAMES = [
'running-processes',
'get-file',
'execute',
'upload',
] as const;

export type ResponseActionsApiCommandNames = typeof RESPONSE_ACTION_API_COMMANDS_NAMES[number];
Expand All @@ -36,6 +37,7 @@ export const ENDPOINT_CAPABILITIES = [
'running_processes',
'get_file',
'execute',
'upload_file',
] as const;

export type EndpointCapabilities = typeof ENDPOINT_CAPABILITIES[number];
Expand All @@ -52,6 +54,7 @@ export const CONSOLE_RESPONSE_ACTION_COMMANDS = [
'processes',
'get-file',
'execute',
'upload',
] as const;

export type ConsoleResponseActionCommands = typeof CONSOLE_RESPONSE_ACTION_COMMANDS[number];
Expand All @@ -74,6 +77,7 @@ export const commandToRBACMap: Record<ConsoleResponseActionCommands, ResponseCon
processes: 'writeProcessOperations',
'get-file': 'writeFileOperations',
execute: 'writeExecuteOperations',
upload: 'writeFileOperations',
Copy link
Member

Choose a reason for hiding this comment

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

Maybe this should be right after get-file just so that all writeFileOperations are bunched together.

});

export const RESPONSE_ACTION_API_COMMANDS_TO_CONSOLE_COMMAND_MAP = Object.freeze<
Expand All @@ -86,6 +90,7 @@ export const RESPONSE_ACTION_API_COMMANDS_TO_CONSOLE_COMMAND_MAP = Object.freeze
'running-processes': 'processes',
'kill-process': 'kill-process',
'suspend-process': 'suspend-process',
upload: 'upload',
});

// 4 hrs in seconds
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@ import type {
NoParametersRequestSchema,
ResponseActionBodySchema,
KillOrSuspendProcessRequestSchema,
UploadActionRequestBody,
} from '../schema/actions';
import type {
ResponseActionStatus,
Expand Down Expand Up @@ -176,7 +177,8 @@ export type EndpointActionDataParameterTypes =
| undefined
| ResponseActionParametersWithPidOrEntityId
| ResponseActionsExecuteParameters
| ResponseActionGetFileParameters;
| ResponseActionGetFileParameters
| ResponseActionUploadParameters;

export interface EndpointActionData<
TParameters extends EndpointActionDataParameterTypes = EndpointActionDataParameterTypes,
Expand Down Expand Up @@ -468,3 +470,25 @@ export type UploadedFileInfo = Pick<
export interface ActionFileInfoApiResponse {
data: UploadedFileInfo;
}

/**
* The parameters that are sent to the Endpoint.
*
* NOTE: Most of the parameters below are NOT accepted via the API. They are inserted into
* the action's parameters via the API route handler
*/
Comment on lines +477 to +479
Copy link
Member

Choose a reason for hiding this comment

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

🔥

Copy link
Member

Choose a reason for hiding this comment

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

Should we also mention in the note above, which parameters are provided in the API?

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 don't think it is necessary. One can just go look at the API parameters type/schema

export type ResponseActionUploadParameters = UploadActionRequestBody['parameters'] & {
file: {
sha256: string;
size: number;
file_name: string;
file_id: string;
};
};

export interface ResponseActionUploadOutputContent {
/** Full path to the file on the host machine where it was saved */
path: string;
/** The free space available (after saving the file) of the drive where the file was saved to, In Bytes */
disk_free_space: number;
}
Original file line number Diff line number Diff line change
Expand Up @@ -98,6 +98,11 @@ export const allowedExperimentalValues = Object.freeze({
*/
responseActionExecuteEnabled: true,

/**
* Enables the `upload` endpoint response action
*/
responseActionUploadEnabled: false,

/**
* Enables top charts on Alerts Page
*/
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -234,20 +234,21 @@ export const useActionsLogFilter = ({
: isHostsFilter
? []
: RESPONSE_ACTION_API_COMMANDS_NAMES.filter((commandName) => {
const featureFlags = ExperimentalFeaturesService.get();

// `get-file` is currently behind FF
if (
commandName === 'get-file' &&
!ExperimentalFeaturesService.get().responseActionGetFileEnabled
) {
if (commandName === 'get-file' && !featureFlags.responseActionGetFileEnabled) {
return false;
}

// TODO: remove this when `execute` is no longer behind FF
// planned for 8.8
if (
commandName === 'execute' &&
!ExperimentalFeaturesService.get().responseActionExecuteEnabled
) {
if (commandName === 'execute' && !featureFlags.responseActionExecuteEnabled) {
return false;
}

// upload - v8.9
if (commandName === 'upload' && !featureFlags.responseActionUploadEnabled) {
return false;
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -118,8 +118,6 @@ jest.mock('@kbn/kibana-react-plugin/public', () => {

jest.mock('../../../hooks/endpoint/use_get_endpoints_list');

jest.mock('../../../../common/experimental_features_service');

jest.mock('../../../../common/components/user_privileges');
const useUserPrivilegesMock = _useUserPrivileges as jest.Mock;

Expand Down Expand Up @@ -917,6 +915,7 @@ describe('Response actions history', () => {
});

it('should show a list of actions when opened', () => {
mockedContext.setExperimentalFlag({ responseActionUploadEnabled: true });
render();
const { getByTestId, getAllByTestId } = renderResult;

Expand All @@ -934,6 +933,7 @@ describe('Response actions history', () => {
'processes',
'get-file',
'execute',
'upload',
]);
});

Expand Down
2 changes: 2 additions & 0 deletions x-pack/plugins/security_solution/server/config.mock.ts
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@ export const createMockConfig = (): ConfigType => {
'responseActionGetFileEnabled',
// remove property below once `execute` FF is enabled or removed
'responseActionExecuteEnabled',
'responseActionUploadEnabled',
];

return {
Expand All @@ -29,6 +30,7 @@ export const createMockConfig = (): ConfigType => {
prebuiltRulesPackageVersion: '',
alertMergeStrategy: 'missingFields',
alertIgnoreFields: [],
maxUploadResponseActionFileBytes: 26214400,

experimentalFeatures: parseExperimentalConfigValue(enableExperimental),
enabled: true,
Expand Down
8 changes: 8 additions & 0 deletions x-pack/plugins/security_solution/server/config.ts
Original file line number Diff line number Diff line change
Expand Up @@ -123,6 +123,14 @@ export const configSchema = schema.object({
*/
prebuiltRulesPackageVersion: schema.maybe(schema.string()),
enabled: schema.boolean({ defaultValue: true }),

/**
* The Max number of Bytes allowed for the `upload` endpoint response action
*/
maxUploadResponseActionFileBytes: schema.number({
defaultValue: 26214400, // 25MB,
max: 104857600, // 100MB,
}),
});

export type ConfigSchema = TypeOf<typeof configSchema>;
Expand Down
Loading