[Security Solution][Endpoint] Upload response action create API #156303

paul-tavares · 2023-05-01T20:51:59Z

Summary

New api to create upload response action request. API route mounted at: /api/endpoint/action/upload
New server config value - maxUploadResponseActionFileBytes - that controls how large uploaded files can be. Defaults to 25MB. Max is 100MB
New feature flag to control upload response action: responseActionUploadEnabled. Must be set to true in kibana yaml in order to access this new API

API Request body (MUST BE SENT AS multipart/form-data). JSON representation of it below:

{
  file: FileBlob
  endpoint_ids: Array<string>
  /* other optional action params */
}

File Created (in .fleet-files-endpoint index)

      {
        "_index": ".fleet-files-endpoint",
        "_id": "f4369611-09ce-4a4f-851c-49542b70fac1",
        "_score": 1,
        "_source": {
          "file": {
            "created": "2023-05-02T15:05:28.529Z",
            "Status": "READY",
            "Updated": "2023-05-02T15:05:28.529Z",
            "name": "olm-6308-endpoint-agent-status.gif",
            "mime_type": "image/gif",
            "Meta": {
              "target_agents": [
                "63e7a9cf-5945-417d-89b8-027294b7a4f0"
              ],
              "action_id": "3c8c3b3e-1f18-4ab0-96e4-dd0b6c176621"
            },
            "extension": "gif",
            "FileKind": "none",
            "size": 410104,
            "hash": {
              "sha256": "231b8d932da84849f8d16a96712d434aa21c72cbf1d83845e34a0b8a54883b5a"
            }
          }
        }
      }

Checklist

Unit or functional tests were updated or added to match the most common scenarios

… config

…d-response-action-api

…e Action

…tup()`

…fix'

…data

…load` parameters

…File`

…tion-api' into task/olm-5369-upload-response-action-api

…d-response-action-api # Conflicts: # x-pack/plugins/security_solution/tsconfig.json

elasticmachine · 2023-05-03T18:59:31Z

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

…d-response-action-api

…d-response-action-api # Conflicts: # x-pack/plugins/security_solution/server/endpoint/services/actions/create/index.ts

xcrzx

Rules Area changes LGTM

…ontext()` incorrectly

kibana-ci · 2023-05-04T20:19:50Z

💚 Build Succeeded

Buildkite Build
Commit: b15e022

Metrics [docs]

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.1MB	+278.0B

Page load bundle

Size of the bundles that are downloaded on every page load. Target size is below 100kb

id	before	after	diff
`securitySolution`	61.6KB	61.6KB	+31.0B

Unknown metric groups

ESLint disabled line counts

id	before	after	diff
`enterpriseSearch`	19	21	+2
`securitySolution`	398	402	+4
total			+6

Total ESLint disabled count

id	before	after	diff
`enterpriseSearch`	20	22	+2
`securitySolution`	478	482	+4
total			+6

History

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

cc @paul-tavares

tomsonpl

LGTM, left a few questions 👍

tomsonpl · 2023-05-08T13:16:56Z

src/plugins/files/server/mocks.ts

@@ -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);


Just out of curiosity: why clone?

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

tomsonpl · 2023-05-08T13:17:32Z

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

@@ -239,6 +241,24 @@ export class EndpointActionGenerator extends BaseDataGenerator {
      }
    }

+    if (command === 'upload') {


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

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

tomsonpl · 2023-05-08T13:17:49Z

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

+    if (command === 'upload') {
+      if (!details.parameters) {
+        (
+          details as ActionDetails<


Do we need the cast?

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.

tomsonpl · 2023-05-08T13:21:14Z

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

+    });
+
+    it('should NOT register route if feature flag is false', () => {
+      // @ts-expect-error


any specific error here?

Yeah, assignment to a Readonly property. I'll have to adjust the test render mock at some point to perhaps not set these to readonly so that we don't have to do this.

tomsonpl · 2023-05-08T13:24:31Z

x-pack/plugins/security_solution/server/endpoint/services/actions/action_files.ts

+  await file.update({
+    meta: {
+      // eslint-disable-next-line @typescript-eslint/no-non-null-assertion
+      ...file.data.meta!,


can we get rid of the bang somehow?

Let me see in the next PR is I can somehow narrow the type. The issue here is that meta is optional in the Files plugin interface, but required in our use case. Will look into it.

tomsonpl · 2023-05-08T13:25:48Z

x-pack/plugins/security_solution/server/endpoint/services/actions/create/index.ts

@@ -77,10 +78,13 @@ export const actionCreateService = (
    return createAction({ ...payload }, { minimumLicenseRequired: 'enterprise' });
  };

-  const createAction = async (
+  const createAction = async <
+    TOutputContent extends object = object,


TILT: Could you explain the syntax? what is this = object? A default type if the type was not provided or anything else? Thanks!

Yes, exactly. This is the definition of a generic where they generic types have default values, thus you are not required to set them if attempting to use the method, but could be set if you want to narrow down the ActionDetails type that is returned.

More about generics here.

tomsonpl

LGTM, left a few questions 👍

ashokaditya

I've a few nits and questions, but otherwise it is looking good to 🚢

ashokaditya · 2023-05-08T13:26:49Z

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

+      fileStream = createHapiReadableStreamMock();
+    });
+
+    it('should not error if `override` parameter is not defined', () => {


nit: overwrite parameter

Will correct in next PR. 👍

ashokaditya · 2023-05-08T13:27:02Z

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

+      }).not.toThrow();
+    });
+
+    it('should allow `override` parameter', () => {


nit: overwrite

same here. Thanks for catching this

ashokaditya · 2023-05-08T13:27:25Z

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

+        UploadActionRequestSchema.body.validate({
+          endpoint_ids: ['endpoint_id'],
+          parameters: {
+            override: true,


Suggested change

override: true,

overwrite: true,

😱
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.

ashokaditya · 2023-05-08T13:31:16Z

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

+    ...BaseActionRequestSchema,
+
+    parameters: schema.object({
+      overwrite: schema.maybe(schema.boolean()),


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

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

ashokaditya · 2023-05-08T13:32:47Z

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

@@ -74,6 +77,7 @@ export const commandToRBACMap: Record<ConsoleResponseActionCommands, ResponseCon
    processes: 'writeProcessOperations',
    'get-file': 'writeFileOperations',
    execute: 'writeExecuteOperations',
+    upload: 'writeFileOperations',


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

ashokaditya · 2023-05-08T13:34:21Z

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

+ * 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
+ */


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

I don't think it is necessary. One can just go look at the API parameters type/schema

ashokaditya · 2023-05-08T14:19:17Z

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

+
+      uploadParameters.file.file_id = createdFile.file.id;
+      uploadParameters.file.file_name = createdFile.file.name;
+      uploadParameters.file.sha256 = createdFile.file.hash?.sha256;


Suggested change

uploadParameters.file.sha256 = createdFile.file.hash?.sha256;

uploadParameters.file.sha256 = createdFile.file.hash.sha256;

file.hash is an optional attribute.

Oh I see. I saw it typed as a required string in the PR so I suggested the change.

ashokaditya · 2023-05-08T14:24:50Z

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

+          { casesClient }
+        );
+
+      await setFileActionId(esClient, logger, data);


Q: What happens if the setFileActionId fails?

Then we fail the API request. But - thats a good point that I will need to think through. At this point in the code, a failure is unlikely, but perhaps if it happens to fail here, we just catch it and log it, but allow the api call to succeed, since the action has already been "sent" to the Endpoint. Failing here as is right now will cause the file to be deleted (in the catch() below)` which would in turn cause the endpoint to fail to down the file.

Good catch @ashokaditya . Will correct it in next PR.

Thanks for the explanation. Sounds good with later updates. 🚀

…PI (#156303)" This reverts commit cc781e6.

jbudz · 2023-05-09T14:57:32Z

This was reverted with c75d1c5 due to a type failure - https://buildkite.com/elastic/kibana-on-merge/builds/30174#018800e0-d266-4ec4-ac05-cd972bfb9e12/229-232

…create API (elastic#156303)"" This reverts commit c75d1c5.

paul-tavares · 2023-05-09T15:51:45Z

Trying again:

#157182

🤞

…ommit) (#157182) ## Summary This is a re-commit of PR #156303 which was reverted due to a type error that slipped passed the last PR.

…om PR elastic#156303)

commit c899f1d Author: Paul Tavares <paul.tavares@elastic.co> Date: Tue May 9 17:30:04 2023 -0400 Fix execute mapped authz property commit 4c460c5 Author: Paul Tavares <paul.tavares@elastic.co> Date: Tue May 9 16:07:39 2023 -0400 fix sending `undefined` for `comment` commit 02f9d07 Author: Paul Tavares <paul.tavares@elastic.co> Date: Tue May 9 11:47:03 2023 -0400 Upload API: log failure to file's action ID and still allow API response to return success commit 5d6990c Author: Paul Tavares <paul.tavares@elastic.co> Date: Tue May 9 11:40:44 2023 -0400 add default value to `overwrite` param of upload command (feedback from PR elastic#156303) commit a44b08b Author: Paul Tavares <paul.tavares@elastic.co> Date: Tue May 9 11:39:33 2023 -0400 fix tests (feedback from PR elastic#156303) commit c221c56 Author: Paul Tavares <paul.tavares@elastic.co> Date: Tue May 9 10:02:42 2023 -0400 tests for upload result component commit 955c706 Author: Paul Tavares <paul.tavares@elastic.co> Date: Mon May 8 17:44:33 2023 -0400 tests for upload action handler component commit f5a0bae Author: Paul Tavares <paul.tavares@elastic.co> Date: Mon May 8 15:37:55 2023 -0400 tests for argument `mustHaveValue` validators commit b9d9525 Author: Paul Tavares <paul.tavares@elastic.co> Date: Mon May 8 09:43:31 2023 -0400 test scaffold for `mustHaveValue` console argument value validators commit e6df2cf Author: Paul Tavares <paul.tavares@elastic.co> Date: Fri May 5 17:01:54 2023 -0400 fix type error commit fb574e1 Author: Paul Tavares <paul.tavares@elastic.co> Date: Fri May 5 16:46:53 2023 -0400 Support upload output for actions sent to multiple agents commit d578647 Author: Paul Tavares <paul.tavares@elastic.co> Date: Fri May 5 16:45:38 2023 -0400 Fix style in Console to ensure that whitespace is preserved commit 9c436d2 Author: Paul Tavares <paul.tavares@elastic.co> Date: Fri May 5 13:44:32 2023 -0400 add command `upload` result to the UI commit 2f9c07f Author: Paul Tavares <paul.tavares@elastic.co> Date: Fri May 5 13:42:48 2023 -0400 Added `upload` output data to endpint generator when generating repsonse commit ca6596a Author: Paul Tavares <paul.tavares@elastic.co> Date: Fri May 5 10:40:20 2023 -0400 Add `truthy` validation to the `file` argument commit 0f43c54 Author: Paul Tavares <paul.tavares@elastic.co> Date: Fri May 5 10:31:00 2023 -0400 Console: add new `mustHaveValue` argument value validation of `truthy` commit 0044d5c Author: Paul Tavares <paul.tavares@elastic.co> Date: Fri May 5 09:51:58 2023 -0400 UI: hook to build and send request + command sends action to server commit ee91d9d Author: Paul Tavares <paul.tavares@elastic.co> Date: Thu May 4 17:42:39 2023 -0400 Add `upload` command to list of console commands commit d8adbf1 Author: Paul Tavares <paul.tavares@elastic.co> Date: Thu May 4 16:55:52 2023 -0400 Removed `commandToCapabilitiesPrivilegesMap` and started using const's

paul-tavares added 11 commits May 1, 2023 11:15

Added responseActionUploadEnabled feature flag

8cd5527

Add API route and define maxUploadResponseActionFileBytes in server…

0dfb681

… config

Use max uplaod size from config

eb51dad

refactor: move HapiReadableStream to top-level types for reuse

59f12ec

Initial setup of Route handler and createFile() service method

38420d7

Merge remote-tracking branch 'upstream/main' into task/olm-5369-uploa…

a2887e9

…d-response-action-api

action file service for crateFile() and deleteFile()

2dfe9ba

Create the file in route handler and then pass data long to create th…

3a02739

…e Action

move FF check to the register function

82859a7

updated metadata generator to include upload capability value

ad57cf7

refactored how server config is passed down to route setup during `se…

e0b154d

…tup()`

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 1, 2023

paul-tavares self-assigned this May 1, 2023

kibanamachine and others added 15 commits May 1, 2023 20:58

[CI] Auto-commit changed files from 'node scripts/lint_ts_projects --…

086d286

…fix'

Types for Upload response content and params

11e9590

ActionCreateService: converted createAction to a generic function

d905916

Save action id to File uploaded

6f1736b

Updated createFileServiceMock() toJSON method to return fileMock.…

8ffb88b

…data

Updated EndpointActionGenerator.generateActionDetails() to return `up…

e35196d

…load` parameters

Action Files test for new createFile, setFileActionId and `delete…

7135355

…File`

Tests for UploadActionRequestSchema

25c353a

Tests for Upload API route registration

ad0e3dc

Merge remote-tracking branch 'origin/task/olm-5369-upload-response-ac…

71118bf

…tion-api' into task/olm-5369-upload-response-action-api

Upload api handler tests

e331bcc

Merge remote-tracking branch 'upstream/main' into task/olm-5369-uploa…

fff1737

…d-response-action-api # Conflicts: # x-pack/plugins/security_solution/tsconfig.json

adjust response action list UI to only show upload if FF is true

4d5bd65

Fix mocks

a7586bb

Fix test unused variables

5ed98c0

paul-tavares requested review from pzl and tomsonpl May 3, 2023 18:59

paul-tavares requested a review from xcrzx May 3, 2023 18:59

paul-tavares added 2 commits May 3, 2023 15:47

Merge remote-tracking branch 'upstream/main' into task/olm-5369-uploa…

9969053

…d-response-action-api

update failing test for action log

098ab0e

sebelga approved these changes May 4, 2023

View reviewed changes

Merge remote-tracking branch 'upstream/main' into task/olm-5369-uploa…

1f3bd9b

…d-response-action-api # Conflicts: # x-pack/plugins/security_solution/server/endpoint/services/actions/create/index.ts

xcrzx approved these changes May 4, 2023

View reviewed changes

paul-tavares and others added 3 commits May 4, 2023 11:23

fix test failures due to earlier commit using `createMockEndpointAppC…

e32193f

…ontext()` incorrectly

Fix tests after merge from main

3fb73fe

Merge branch 'main' into task/olm-5369-upload-response-action-api

b15e022

tomsonpl approved these changes May 8, 2023

View reviewed changes

ashokaditya approved these changes May 8, 2023

View reviewed changes

angorayc approved these changes May 9, 2023

View reviewed changes

paul-tavares merged commit cc781e6 into elastic:main May 9, 2023

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

paul-tavares deleted the task/olm-5369-upload-response-action-api branch May 9, 2023 13:39

jbudz added a commit that referenced this pull request May 9, 2023

Revert "[Security Solution][Endpoint] Upload response action create A…

c75d1c5

…PI (#156303)" This reverts commit cc781e6.

jbudz added the reverted label May 9, 2023

paul-tavares added a commit to paul-tavares/kibana that referenced this pull request May 9, 2023

Revert "Revert "[Security Solution][Endpoint] Upload response action …

7617cda

…create API (elastic#156303)"" This reverts commit c75d1c5.

paul-tavares mentioned this pull request May 9, 2023

[Security Solution][Endpoint] Upload response action create API (re-commit) #157182

Merged

paul-tavares added a commit that referenced this pull request May 9, 2023

[Security Solution][Endpoint] Upload response action create API (re-c…

a5ac5b6

…ommit) (#157182) ## Summary This is a re-commit of PR #156303 which was reverted due to a type error that slipped passed the last PR.

paul-tavares added a commit to paul-tavares/kibana that referenced this pull request May 9, 2023

fix tests (feedback from PR elastic#156303)

a44b08b

paul-tavares added a commit to paul-tavares/kibana that referenced this pull request May 9, 2023

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

5d6990c

…om PR elastic#156303)

	uploadParameters.file.sha256 = createdFile.file.hash?.sha256;
	uploadParameters.file.sha256 = createdFile.file.hash.sha256;

[Security Solution][Endpoint] Upload response action create API #156303

[Security Solution][Endpoint] Upload response action create API #156303

Conversation

paul-tavares commented May 1, 2023 • edited Loading

Summary

Checklist

elasticmachine commented May 3, 2023

xcrzx left a comment

Choose a reason for hiding this comment

kibana-ci commented May 4, 2023

💚 Build Succeeded

Metrics [docs]

Async chunks

Page load bundle

ESLint disabled line counts

Total ESLint disabled count

History

tomsonpl left a 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

tomsonpl left a comment

Choose a reason for hiding this comment

ashokaditya left a 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

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

jbudz commented May 9, 2023

paul-tavares commented May 9, 2023

paul-tavares commented May 1, 2023 •

edited

Loading