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] server-side standard interface for response actions clients #171755

Conversation

paul-tavares
Copy link
Contributor

@paul-tavares paul-tavares commented Nov 22, 2023

Summary

PR introduces a standard interface for Response Actions clients - currently only Endpoint, but in the near future, other clients will be introduced like SentinelOne. This PR is in preperation for that feature in a post v8.12 release.

Changes include:

  • Introduction of EndpointActionsClient class (first Actions client using new standard interface)
  • Changed Response Actions API handler to:
    • use new EndpointActionsClient for processing response actions
    • added support for handling file upload response action (previously a separate handler)
    • now handles all errors using the common HTTP error handler
  • Deleted upload specific API HTTP handler - no longer needed as common handler will now also process upload response actions

NOTE: No changes in functionality as a result of this PR. Just preparation work needed to support Bi-Directional Response Actions.

@paul-tavares 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.12.0 labels Nov 22, 2023
@paul-tavares paul-tavares self-assigned this Nov 22, 2023
@paul-tavares paul-tavares marked this pull request as ready for review November 22, 2023 20:31
@paul-tavares paul-tavares requested a review from a team as a code owner November 22, 2023 20:31
@elasticmachine
Copy link
Contributor

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

@paul-tavares paul-tavares changed the title [Security Solution][Endpoint] Generic server-side response actions client interface [Security Solution][Endpoint] Standard server-side response actions client interface Nov 22, 2023
@paul-tavares paul-tavares changed the title [Security Solution][Endpoint] Standard server-side response actions client interface [Security Solution][Endpoint] server-side standard interface for response actions clients Nov 22, 2023
@kibana-ci
Copy link
Collaborator

💚 Build Succeeded

Metrics [docs]

✅ unchanged

History

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

cc @paul-tavares

ImmutableObject,
} from '../../../../../common/endpoint/types';

export class EndpointActionsClient extends BaseResponseActionsClient {
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: testing of this client is covered by existing tests for both the Route handler for response actions and the individual service functions under services/actions/*

Copy link
Member

@ashokaditya ashokaditya left a comment

Choose a reason for hiding this comment

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

Thanks for this. I've a couple suggestions, please feel free to address them in a later PR.

file_size: 0,
};

const createdFile = await fleetFiles.create(fileStream, actionPayload.endpoint_ids);
Copy link
Member

Choose a reason for hiding this comment

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

We should handle errors for create file failures.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There is really nothing we have to do if this fails here, so we just would be re-throwing the error again. The catch() block below attempts to delete the file (since they can be large) if there is a failure after it is created/stored (ex. a failure while indexing the documents for the action request).

@@ -17,7 +16,7 @@ import type { ResponseActionsApiCommandNames } from '../../../../../common/endpo

export type CreateActionPayload = TypeOf<typeof ResponseActionBodySchema> & {
command: ResponseActionsApiCommandNames;
user?: ReturnType<AuthenticationServiceStart['getCurrentUser']>;
user?: { username: string } | null | undefined;
Copy link
Member

Choose a reason for hiding this comment

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

Q: Should we just change user to be not optional for automated actions.

Suggested change
user?: { username: string } | null | undefined;
user: { username: string } | undefined;

else just optional user should be okay?

Suggested change
user?: { username: string } | null | undefined;
user?: { username: string } ;

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 not sure what the impact would be. for now, i'm thinking that we just leave it as is since I was not trying to change much about the existing logic. are you ok with that?

Copy link
Member

Choose a reason for hiding this comment

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

Yeah that sounds okay about not changing too much. We can clean it up as we go later.

Copy link
Contributor

@patrykkopycinski patrykkopycinski left a comment

Choose a reason for hiding this comment

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

🙇

@paul-tavares paul-tavares merged commit 0a72738 into elastic:main Nov 28, 2023
@kibanamachine kibanamachine added the backport:skip This commit does not require backporting label Nov 28, 2023
@paul-tavares paul-tavares deleted the task/olm-bi-directional-response-actions-data-persist branch November 28, 2023 14:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport:skip This commit does not require backporting release_note:skip Skip the PR/issue when compiling release notes Team:Defend Workflows “EDR Workflows” sub-team of Security Solution v8.12.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants