[Security Solution][Endpoint] add `upload` console response action #157208

paul-tavares · 2023-05-09T19:23:19Z

Summary

Add the upload response action to the endpoint console

NOTE:

functionality is currently behind feature flag: xpack.securitySolution.enableExperimental.responseActionUploadEnabled: true
The Endpoint has not yet implemented support for upload. In order to test this locally, run the endpoint agent emulator dev. tool:

node x-pack/plugins/security_solution/scripts/endpoint/endpoint_agent_emulator.js --asSuperuser

Checklist

Any text added follows EUI's writing guidelines, uses sentence case text and includes i18n support
Unit or functional tests were updated or added to match the most common scenarios

…om PR elastic#156303)

…nse to return success

elasticmachine · 2023-05-09T19:23:22Z

Pinging @elastic/security-defend-workflows (Team:Defend Workflows)

ashokaditya

This is looking awesome. Thanks for all the changes that we talked about in the last PR. I mostly have nits and some questions.

I took it for a spin and I noticed three things:

The history input with the up arrow key shows the file name that was used for upload but it doesn't actually take that file when I enter it from the history. It's empty. Either we should make that work or maybe show the history item with --file="". See the clip.
Another thing I noticed is that for all commands or anything typed into the console input, I could do a "select all" (cmd + a) and hit delete/backspace and the text is deleted. This doesn't work for upload. Hitting delete/backspace when everything is selected only deletes the last character at the end. You can see this in the clip.
I see a uploadAction log in the kibana console on every upload action execute when it goes through on the console . Not sure if this is just for testing. Might bloat the log for every upload action.

[2023-05-10T15:50:16.607+02:00][INFO ][plugins.securitySolution.uploadAction] {
 "action": "create",
 "outcome": "success"
}

ashokaditya · 2023-05-10T12:38:23Z

x-pack/plugins/security_solution/common/endpoint/schema/actions.ts

+/** Type used by the server's API for `upload` action */
+export type UploadActionApiRequestBody = TypeOf<typeof UploadActionRequestSchema.body>;
+
+/**
+ * Type used on the UI side. The `file` definition is different on the UI side, thus the
+ * need for a separate type.
+ */
+export type UploadActionUIRequestBody = Omit<UploadActionApiRequestBody, 'file'> & {
+  file: File;
+};


ashokaditya · 2023-05-10T12:38:52Z

x-pack/plugins/security_solution/common/endpoint/service/response_actions/constants.ts

+ * maps the console command to the RBAC control (kibana feature control) that is required to access it via console
 */
-export const commandToRBACMap: Record<ConsoleResponseActionCommands, ResponseConsoleRbacControls> =
-  Object.freeze({
-    isolate: 'writeHostIsolation',
-    release: 'writeHostIsolation',
-    'kill-process': 'writeProcessOperations',
-    'suspend-process': 'writeProcessOperations',
-    processes: 'writeProcessOperations',
-    'get-file': 'writeFileOperations',
-    execute: 'writeExecuteOperations',
-    upload: 'writeFileOperations',
-  });
+export const RESPONSE_CONSOLE_ACTION_COMMANDS_TO_RBAC_FEATURE_CONTROL: Record<
+  ConsoleResponseActionCommands,
+  ResponseConsoleRbacControls
+> = Object.freeze({
+  isolate: 'writeHostIsolation',
+  release: 'writeHostIsolation',
+  'kill-process': 'writeProcessOperations',
+  'suspend-process': 'writeProcessOperations',
+  processes: 'writeProcessOperations',
+  'get-file': 'writeFileOperations',
+  execute: 'writeExecuteOperations',
+  upload: 'writeFileOperations',
+});


ashokaditya · 2023-05-10T12:39:57Z

x-pack/plugins/security_solution/common/endpoint/service/response_actions/constants.ts

+export const CONSOLE_RESPONSE_ACTION_COMMANDS_TO_ENDPOINT_CAPABILITY = Object.freeze<
+  Record<ConsoleResponseActionCommands, EndpointCapabilities>
+>({
+  isolate: 'isolation',
+  release: 'isolation',
+  execute: 'execute',
+  'get-file': 'get_file',
+  processes: 'running_processes',
+  'kill-process': 'kill_process',
+  'suspend-process': 'suspend_process',
+  upload: 'upload_file',
+});
+
+/**
+ * The list of console commands mapped to the required EndpointAuthz to access that command
+ */
+export const RESPONSE_CONSOLE_ACTION_COMMANDS_TO_REQUIRED_AUTHZ = Object.freeze<
+  Record<ConsoleResponseActionCommands, EndpointAuthzKeyList[number]>
+>({
+  isolate: 'canIsolateHost',
+  release: 'canUnIsolateHost',
+  execute: 'canWriteExecuteOperations',
+  'get-file': 'canWriteFileOperations',
+  upload: 'canWriteFileOperations',
+  processes: 'canGetRunningProcesses',
+  'kill-process': 'canKillProcess',
+  'suspend-process': 'canSuspendProcess',
+});


🔥 Thanks for adding these!

ashokaditya · 2023-05-10T12:41:52Z

x-pack/plugins/security_solution/common/endpoint/service/response_actions/constants.ts

@@ -93,6 +97,35 @@ export const RESPONSE_ACTION_API_COMMANDS_TO_CONSOLE_COMMAND_MAP = Object.freeze
  upload: 'upload',
 });

+export const CONSOLE_RESPONSE_ACTION_COMMANDS_TO_ENDPOINT_CAPABILITY = Object.freeze<


nit: Consider naming this RESPONSE_CONSOLE_ACTION_COMMANDS_TO_ENDPOINT_CAPABILITY so that we've consistent name prefix for these constants here.

ashokaditya · 2023-05-10T12:59:03Z

x-pack/plugins/security_solution/common/endpoint/service/response_actions/constants.ts

+  'kill-process': 'canKillProcess',
+  'suspend-process': 'canSuspendProcess',
+});
+


We could combine all these into a single object perhaps. Easier to read values from just one variable then. Also unifies all these different values in a single place. What do you think?

Something like:

{ 'isolate': { authz: 'canIsolateHost', capability: 'isolation', rbac: 'writeHostIsolation', }, ... 'execute': { authz: 'canExecuteFileOperations', capability: 'execute', rbac: 'executeFileOperations', }, 'get-file': { authz: 'canWriteFileOperations' capability: 'get-file' rbac: 'writeFileOperations' }, ... }

yes, I been thinking about this as well. Having one unified deeply nested object and then derive the simple maps from it. something we can refactor in the future.

ashokaditya · 2023-05-10T13:01:05Z

x-pack/plugins/security_solution/common/endpoint/schema/actions.test.ts

@@ -659,7 +659,7 @@ describe('actions schemas', () => {
      }).not.toThrow();
    });

-    it('should allow `override` parameter', () => {
+    it('should allow `overwrite` parameter', () => {


Thanks for updating this.

ashokaditya · 2023-05-10T13:10:00Z

x-pack/plugins/security_solution/common/endpoint/data_generators/endpoint_action_generator.ts

+        uploadOutput = {
+          type: 'json',
+          content: {
+            code: 'ra_upload_success',


I think this should be ra_upload_file-success. Unless they are two different codes.

ahhh.. Yes, it will likely be the value you stated. We're still waiting for the codes to be provided by the endpoint, but I'll update it.

Same as the one that's a bit below in the code.

x-pack/plugins/security_solution/common/endpoint/data_generators/endpoint_action_generator.ts

ashokaditya · 2023-05-10T13:18:26Z

x-pack/plugins/security_solution/public/management/components/console/types.ts

+   * - `truthy`: The argument must have a value and the values must be "truthy"
+   *   (no `null` or `undefined`). Note that `0` (zero) will be considered `truthy`


nit: Maybe simpler to say that the argument must have some primitive value other than null or undefined.

hmm.. not sure about that. How would boolean false or '' (empty string). Will need to think about that a little more.

What I understood from the comment is false/'' are also truthy same as 0.

hmmm. no, the implemenation looks like this:

if (typeof argValue !== 'number' && !argValue) {

So number of 0 is truthy, but everything else is not. so false, '' would fail validation.

I might have perhaps been too explicit on this thruthy validator. Maybe I should just remove the typeof argValue !== 'number and just test that value evaluates to a boolean true?

if someone wants to ensure its not an empty string, then they could use mustHaveValue: 'non-empty-string'. And if they want to ensure the number zero can be used, then they would set the mustHaveValue: 'number'.

What do you think?

Oh indeed. Yeah, that makes sense. Let's go with verifying that that it is in fact true.

ashokaditya · 2023-05-10T13:33:02Z

x-pack/plugins/security_solution/server/endpoint/routes/actions/file_upload_handler.ts

+      // Update the file meta to include the action id, and if any errors (unlikely),
+      // then just log them and still allow api to return success since the action has
+      // already been created and potentially dispatched to Endpoint. Action ID is not
+      // need by the Endpoint or fleet-server's API, so no need to fail here


nit: // needed...

paul-tavares · 2023-05-10T14:48:07Z

Thanks for the feedback @ashokaditya . I have made just about all of the suggestions you provided. Here are responses to your overall comments:

➡️ re:

The history input with the up arrow key shows the file name that was used for upload but it doesn't actually take that file when I enter it from the history. It's empty. Either we should make that work or maybe show the history item with --file="".

This is by design. The only thing we can show in the up history is plain text, so I feel like having the file name that was used for upload there would be more helpful to the user than having an empty string.

Unfortunately (but in the larger picture - fortunately) we can not select files from a user's system programatically. A user has to explicitly do it through he browser's file picking functionality, so we cant just re-select the file when we populate the command to the input area.

➡️ re:

Another thing I noticed is that for all commands or anything typed into the console input, I could do a "select all" (cmd + a) and hit delete/backspace and the text is deleted. This doesn't work for upload. Hitting delete/backspace when everything is selected only deletes the last character at the end. You can see this in the clip.

This is in fact a current limitation in the console when using argument value selector components (like the file picker). See the description of this PR for more on that: #148693

➡️ re:

I see a uploadAction log in the kibana console on every upload action execute when it goes through on the console . Not sure if this is just for testing. Might bloat the log for every upload action.

This is being done by the Files plugin. We pass in the Logger to that service and it seems to be logging this everytime a file is uploaded.

kibana-ci · 2023-05-10T16:23:20Z

💛 Build succeeded, but was flaky

Buildkite Build
Commit: 2c34523

Failed CI Steps

FTR Configs #23

Test Failures

[job] [logs] FTR Configs #23 / spaces api without security _update_objects_spaces targeting the space_2 space "before all" hook for "should return 200 {objects: [default_only,space_1_only,space_2_only,default_and_space_1,default_and_space_2,space_1_and_space_2,each_space,all_spaces,does_not_exist], spacesToAdd: [some-space-id], spacesToRemove: [space_2]}"

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id	before	after	diff
`securitySolution`	3867	3873	+6

Async chunks

Total size of all lazy-loaded chunks that will be downloaded as the user navigates the app

id	before	after	diff
`securitySolution`	9.1MB	9.2MB	+24.4KB

Unknown metric groups

ESLint disabled line counts

id	before	after	diff
`enterpriseSearch`	19	21	+2
`securitySolution`	400	404	+4
total			+6

Total ESLint disabled count

id	before	after	diff
`enterpriseSearch`	20	22	+2
`securitySolution`	480	484	+4
total			+6

History

💛 Build #126712 was flaky c899f1d
💔 Build #126702 failed 4c460c5

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

cc @paul-tavares

dasansol92

Awesome stuff here. LGTM! 🔥

paul-tavares added 17 commits May 9, 2023 15:07

Removed commandToCapabilitiesPrivilegesMap and started using const's

d8adbf1

Add upload command to list of console commands

ee91d9d

UI: hook to build and send request + command sends action to server

0044d5c

Console: add new mustHaveValue argument value validation of truthy

0f43c54

Add truthy validation to the file argument

ca6596a

Added upload output data to endpint generator when generating repsonse

2f9c07f

add command upload result to the UI

9c436d2

Fix style in Console to ensure that whitespace is preserved

d578647

Support upload output for actions sent to multiple agents

fb574e1

fix type error

e6df2cf

test scaffold for mustHaveValue console argument value validators

b9d9525

tests for argument mustHaveValue validators

f5a0bae

tests for upload action handler component

955c706

tests for upload result component

c221c56

fix tests (feedback from PR elastic#156303)

a44b08b

add default value to overwrite param of upload command (feedback fr…

5d6990c

…om PR elastic#156303)

Upload API: log failure to file's action ID and still allow API respo…

02f9d07

…nse to return success

paul-tavares added release_note:skip Skip the PR/issue when compiling release notes Team:Defend Workflows “EDR Workflows” sub-team of Security Solution v8.9.0 labels May 9, 2023

paul-tavares self-assigned this May 9, 2023

paul-tavares requested a review from a team as a code owner May 9, 2023 19:23

paul-tavares requested review from dasansol92 and parkiino May 9, 2023 19:23

paul-tavares requested review from ashokaditya and removed request for parkiino May 9, 2023 19:23

paul-tavares added 2 commits May 9, 2023 16:07

fix sending undefined for comment

4c460c5

Fix execute mapped authz property

c899f1d

ashokaditya approved these changes May 10, 2023

View reviewed changes

paul-tavares added 3 commits May 10, 2023 10:19

Rename const

7cf1123

Feedback from code review (ash)

16678e6

simplified implemenation of console's truthy validator

2c34523

dasansol92 approved these changes May 11, 2023

View reviewed changes

paul-tavares merged commit f9f4c1a into elastic:main May 11, 2023

kibanamachine added the backport:skip This commit does not require backporting label May 11, 2023

paul-tavares deleted the task/olm-5368-upload-response-action-console-command branch May 11, 2023 13:10

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Security Solution][Endpoint] add `upload` console response action #157208

[Security Solution][Endpoint] add `upload` console response action #157208

paul-tavares commented May 9, 2023 •

edited

Loading

elasticmachine commented May 9, 2023

ashokaditya left a comment •

edited

Loading

ashokaditya May 10, 2023

ashokaditya May 10, 2023

ashokaditya May 10, 2023

ashokaditya May 10, 2023

ashokaditya May 10, 2023

paul-tavares May 10, 2023

ashokaditya May 10, 2023

ashokaditya May 10, 2023

paul-tavares May 10, 2023

ashokaditya May 10, 2023

ashokaditya May 10, 2023

paul-tavares May 10, 2023

ashokaditya May 10, 2023

paul-tavares May 10, 2023

ashokaditya May 10, 2023 •

edited

Loading

ashokaditya May 10, 2023

paul-tavares commented May 10, 2023

kibana-ci commented May 10, 2023

ESLint disabled line counts

Total ESLint disabled count

dasansol92 left a comment

		* - `truthy`: The argument must have a value and the values must be "truthy"
		* (no `null` or `undefined`). Note that `0` (zero) will be considered `truthy`

[Security Solution][Endpoint] add upload console response action #157208

[Security Solution][Endpoint] add upload console response action #157208

Conversation

paul-tavares commented May 9, 2023 • edited Loading

Summary

Checklist

elasticmachine commented May 9, 2023

ashokaditya left a comment • edited Loading

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

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

ashokaditya May 10, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

paul-tavares commented May 10, 2023

kibana-ci commented May 10, 2023

💛 Build succeeded, but was flaky

Failed CI Steps

Test Failures

Metrics [docs]

Module Count

Async chunks

ESLint disabled line counts

Total ESLint disabled count

History

dasansol92 left a comment

Choose a reason for hiding this comment

[Security Solution][Endpoint] add `upload` console response action #157208

[Security Solution][Endpoint] add `upload` console response action #157208

paul-tavares commented May 9, 2023 •

edited

Loading

ashokaditya left a comment •

edited

Loading

ashokaditya May 10, 2023 •

edited

Loading