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

[Defend Workflows] Common response actions tab in alert Flyout #155362

Merged
merged 68 commits into from
May 24, 2023

Conversation

tomsonpl
Copy link
Contributor

@tomsonpl tomsonpl commented Apr 20, 2023

This PR combines 2 Tabs (Osquery + Endpoint) Response Actions in Event Details Flyout into one, and present results in a unified way.

AC:

  • keep osquery tab if endpointResponseActionsEnabled is set to false
  • present osquery + endpoint response actions in one unified tab (only if endpointResponseActionsEnabled FF is set to true) and hide osquery tab
  • fetch all actions together in order to get them in historical order
  • data fetching for endpoint actions through search strategy
  • refactor types of securitySolutionSearchStrategyProvider so we can have a StrategyParseResponseType
  • calculate endpoint actions status based on docs count and statuses instead of in javascript
  • show Empty prompts for Endpoint and Osquery if user is missing RBAC permissions
  • fix issue with Endpoint Automated filter in Flyout
  • change e2e tets to look up for new design (not included in CI for now)
  • add hostname (even the unenrolled one for automated actions) to action details

We are aware of the new Extended Flyout that is prepared to be released soon. We're going to migrate to it after this PR gets merged.

Zrzut ekranu 2023-04-20 o 12 18 17

When analyst doesn't have RBAC permissions to see osquery / endpoint results.
Zrzut ekranu 2023-05-11 o 09 57 18

When there is an agent that is not enrolled (we also display hostname from now on)
Zrzut ekranu 2023-05-18 o 08 28 30

@tomsonpl tomsonpl self-assigned this Apr 20, 2023
@tomsonpl tomsonpl added Team:Defend Workflows “EDR Workflows” sub-team of Security Solution Feature:Osquery Security Solution Osquery feature v8.8.0 release_note:feature Makes this part of the condensed release notes labels Apr 20, 2023
@tomsonpl tomsonpl added v8.9.0 and removed v8.8.0 labels May 9, 2023
@@ -37,7 +38,9 @@ export const securitySolutionSearchStrategyProvider = <T extends FactoryQueryTyp
getSpaceId?: (request: KibanaRequest) => string,
ruleDataClient?: IRuleDataClient | null
): ISearchStrategy<StrategyRequestType<T>, StrategyResponseType<T>> => {
const es = data.search.getSearchStrategy(ENHANCED_ES_SEARCH_STRATEGY);
const es = data.search.getSearchStrategy(
Copy link
Contributor

Choose a reason for hiding this comment

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

why does the inferred return type no longer work here?

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 needed to enable using a type in between request and response, I called it StrategyParseResponseType which default to IEsSearchResponse because in my case the response getting into parse() and whatever parse() returns are 2 different things, couldn't just use StrategyResponseType.

This enabled us to add some more logic to search_strategy, instead of doing that on frontend queries.

How does this sound?

<Suspense fallback={null}>
<OsqueryResult services={services} {...props} />
</Suspense>
);
Copy link
Contributor

Choose a reason for hiding this comment

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

nit:

Suggested change
);
);
getLazyOsqueryResult.displayName = 'LazyOsQueryResult';

Copy link
Contributor

@kqualters-elastic kqualters-elastic left a comment

Choose a reason for hiding this comment

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

lgtm 👍

patrykkopycinski and others added 3 commits May 21, 2023 14:40
…s-common-tab

# Conflicts:
#	x-pack/plugins/translations/translations/fr-FR.json
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.

Looks good! I've tested it out again for hostnames. I've a few small suggestions and a couple of questions. I do strongly feel that the hosts/hostnames info should go into EndpointActions.data if it is in the actions doc and in the EndpointActions.data.output if it is in the response. Let me know what you think or need help with anything.

@@ -45,6 +45,7 @@ export class EndpointActionGenerator extends BaseDataGenerator {
agent: {
id: [this.seededUUIDv4()],
},
hosts: undefined,
Copy link
Member

Choose a reason for hiding this comment

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

This should be part of EndpointActions.data. See my comment below.

import type { LogsEndpointActionWithHosts } from '../../../endpoint/types';
import type { ResponseActionsQueries } from '.';

export enum Direction {
Copy link
Member

Choose a reason for hiding this comment

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

Consider naming this SortOrder

expiration: string;
actionId: string;
sort: {
direction: Direction;
Copy link
Member

Choose a reason for hiding this comment

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

Consider renaming this so that sort has sorting order

...
sort : {
  order: SortOrder;
  filed: string;
}
...

@@ -0,0 +1,29 @@
/*
* Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one
Copy link
Member

Choose a reason for hiding this comment

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

Consider renaming the file to action.ts for naming consistency.

alertIds: string[];
agentId?: string;
sort: {
direction: Direction;
Copy link
Member

Choose a reason for hiding this comment

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

order: SortOrder

{
alertIds: [alertId],
},
{ skip: shouldEarlyReturn }
Copy link
Member

Choose a reason for hiding this comment

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

I feel this skip name is confusing! Also, in the useGetAutomatedActionList hook it is inverted. I would suggest that you do this instead. I also now notice that there's also a skip with useGetAutomatedActionResponseList. Consider changing that also similar to my suggestion here.

Suggested change
{ skip: shouldEarlyReturn }
{ enabled: !shouldEarlyReturn }

You could also rename shouldEarlyReturn to doNothing.

@@ -71,7 +71,7 @@ export const ResponseActionsLog = memo<
statuses: [],
userIds: [],
withOutputs: [],
withAutomatedActions: true,
withAutomatedActions: false,
Copy link
Member

Choose a reason for hiding this comment

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

Please also update some of the tests that have withAutomatedActions as true for default.

@@ -143,6 +144,7 @@ export const actionCreateService = (
agent: {
id: payload.endpoint_ids,
},
hosts: payload.hosts,
Copy link
Member

Choose a reason for hiding this comment

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

Either this should be in the response output or it should be within EndpointActions.data. See my earlier comment.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Move to EndpointActions.data the change makes sense to me after giving it more thoughts. Thanks @ashokaditya 👍

import type { ActionRequestOptions } from '../../../../../../common/search_strategy/security_solution/response_actions';
import { ENDPOINT_ACTIONS_INDEX } from '../../../../../../common/endpoint/constants';

export const buildActionsQuery = ({
Copy link
Member

Choose a reason for hiding this comment

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

Maybe name this buildResponseActionsQuery.

response.rawResponse?.aggregations?.aggs.responses_by_action_id?.responses.buckets;
const successful = aggsBuckets?.find((bucket) => bucket.key === 'success')?.doc_count ?? 0;

const wasSuccessful = responded === successful;
Copy link
Member

Choose a reason for hiding this comment

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

Q: So here it looks like unless all responses are received it is not successful. Have we reached a consensus on showing a partially successful status based on a set of completed/successful responses 🤔

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 question 👍 The change for partially successful has been abandoned. But in my opinion it would be nice to get back to it.

@tomsonpl tomsonpl requested a review from ashokaditya May 23, 2023 13:22
@@ -71,7 +71,7 @@ export const allowedExperimentalValues = Object.freeze({
/**
* Enables the automated endpoint response action in rule + alerts
*/
endpointResponseActionsEnabled: false,
endpointResponseActionsEnabled: true,
Copy link
Member

Choose a reason for hiding this comment

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

this should be false to start with. The only way to enable this would be via setting it on kibana.dev.yml or with local running env vars

@@ -31,7 +31,7 @@ export const EndpointResponseActionResults = ({ action }: EndpointResponseAction
const canReadEndpoint = canReadActionsLogManagement && canAccessEndpointActionsLogManagement;
const { data: expandedAction } = useGetAutomatedActionResponseList(
{ actionId, expiration, agent },
{ skip: !canReadEndpoint, action, isLive }
{ enabled: canReadEndpoint, action, isLive }
Copy link
Member

Choose a reason for hiding this comment

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

This makes sense when you read it quickly! Thanks for changing this.

@@ -51,7 +51,7 @@ describe('useGetEndpointActionList hook', () => {
pageSize: 20,
startDate: 'now-5d',
endDate: 'now',
withAutomatedActions: true,
withAutomatedActions: false,
Copy link
Member

Choose a reason for hiding this comment

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

🙇

@@ -25,12 +25,12 @@ import type {
} from '../../../../common/endpoint/schema/automated_actions';

interface GetAutomatedActionsListOptions {
skip?: boolean;
enabled: boolean;
Copy link
Member

Choose a reason for hiding this comment

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

🙇

Comment on lines 19 to 22
// export interface SortField<Field = string> {
// field: Field;
// order: SortOrder;
// }
Copy link
Member

Choose a reason for hiding this comment

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

Maybe remove this if not needed.

@tomsonpl tomsonpl requested a review from ashokaditya May 23, 2023 18:17
@@ -152,6 +153,7 @@ export const actionCreateService = (
command: payload.command,
comment: payload.comment ?? undefined,
...(payload.alert_ids ? { alert_id: payload.alert_ids } : {}),
...(payload.hosts ? { hosts: payload.hosts } : {}),
Copy link
Member

Choose a reason for hiding this comment

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

🚀

@@ -29,6 +29,7 @@ import type {
} from '../../../../common/endpoint/types';
import { ActivityLogItemTypes } from '../../../../common/endpoint/types';
import type { EndpointMetadataService } from '../metadata';

Copy link
Member

Choose a reason for hiding this comment

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

This was unintentional I presume

export const useGetAutomatedActionList = (
query: EndpointAutomatedActionListRequestQuery,
{ enabled }: GetAutomatedActionsListOptions
) => {
Copy link
Member

Choose a reason for hiding this comment

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

Sorry I missed this earlier, but I'd encourage you to add return types to all these query hooks.

setUrlWithAutomatedActions(!withAutomatedActionsUrlParam);
}
}, [isFlyout, setUrlWithAutomatedActions, withAutomatedActionsUrlParam]);
setUrlWithAutomatedActions(!withAutomatedActionsUrlParam);
Copy link
Member

Choose a reason for hiding this comment

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

So this would update the URL params on the flyout view as well. We don't want to do that. We want the response actions log query params to be updated on the URL only when in the page view.

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, therefore I would have to pass event handlers through the whole thing again ;p Could you elaborate on why we don't want to pass URL params in flyout?

Copy link
Member

Choose a reason for hiding this comment

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

Not sure what you mean by passing even handlers through...?

We don't want to change the URL params on the flyout as we have a flyout on the endpoint details and responder view. We don't want to drive/control the response actions log or the endpoint details via URL params. Also, changing URL in this view will break bookmarks. Hope that helps.

Screenshot 2023-05-24 at 10 25 34
Screenshot 2023-05-24 at 10 25 24

@@ -182,7 +182,7 @@ export const ActionsLogExpandedTray = memo<{
}>(({ action, 'data-test-subj': dataTestSubj }) => {
const getTestId = useTestIdGenerator(dataTestSubj);

const { startedAt, completedAt, command: _command, comment, parameters } = action;
const { hosts, agents, startedAt, completedAt, command: _command, comment, parameters } = action;
Copy link
Member

Choose a reason for hiding this comment

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

Just a note, this hosts "name" is not the same as the one we've added in this PR under EndpointActions.data for automated actions from alerts.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is hosts from ActionDetails that could be created in two ways:

  1. For Response Actions Results tab: it comes from EndpointActions.data
  2. For Normal Actions List it's calculated in getActionDetailsList

Is this ok with you?

Copy link
Member

@ashokaditya ashokaditya May 24, 2023

Choose a reason for hiding this comment

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

That is right. But I don't see that change on the server side to handle that case for 1. That would go in mapToNormalizedActionRequest util function. And then on the list handler for action list you need to handle the hosts info being populated for each action based on whether it is automated or user initiated.

So here you're using the user-initiated action detail data and the hostname is populated via the agent metadata

Copy link
Member

Choose a reason for hiding this comment

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

We talked offline about this, and this is okay here. This hostname is indeed the one from normal actions and the change here to have consistent host names in the details.

@@ -223,13 +223,17 @@ export const ActionsLogExpandedTray = memo<{
title: OUTPUT_MESSAGES.expandSection.comment,
description: comment ? comment : emptyValue,
},
{
title: OUTPUT_MESSAGES.expandSection.hostname,
description: hosts?.[agents?.[0]]?.name || emptyValue,
Copy link
Member

Choose a reason for hiding this comment

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

hosts and agents are always present in action details.

Suggested change
description: hosts?.[agents?.[0]]?.name || emptyValue,
description: hosts[agents[0]]name,

Copy link
Member

Choose a reason for hiding this comment

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

Now to think of it, this should also handle showing multiple hostnames on the page view. Right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hey, you're right the hosts and agents are always there 👍 However, sometimes name === '' so I would leave || emptyValue to display - as with the rest. What do you think?

Copy link
Member

Choose a reason for hiding this comment

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

Oh yeah the name could be '' for unenrolled agents

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Changed this to display multiple hostnames :)

const eventText = getCommentText(action.EndpointActions.data.command);

const hostName = useMemo(
() => expandedAction?.hosts?.[expandedAction.agents?.[0]]?.name,
Copy link
Member

Choose a reason for hiding this comment

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

This is correct where the hosts is the one we need from the automated actions

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, this is the host that we previously saved in Endpointdata.data. However this const is just used for data-test-subj here.

} = useUserPrivileges();

const [isLive, setIsLive] = useState(true);
const canReadEndpoint = canReadActionsLogManagement && canAccessEndpointActionsLogManagement;
Copy link
Member

@ashokaditya ashokaditya May 24, 2023

Choose a reason for hiding this comment

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

Sorry, I missed this earlier as well. This condition boils down to having READ RBAC access to Actions Log management and having platinum and enterprise licenses. So this won't work if you're not superuser.

We should decide if we want to give this to enterprise or platinum users. One can't have two licenses at the same time. For platinum and upwards, canReadActionsLogManagement will do, and for enterprise canAccessEndpointActionsLogManagement should be sufficient.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hmm thanks for raising this Ash 👍 I misread how withEndpointAuthz works and assumed that these 2 are necessary. Please correct me if I am wrong, but you're saying that we should have one or another right? So it should be like this?
const canReadEndpoint = canReadActionsLogManagement || canAccessEndpointActionsLogManagement;

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, this is also okay.

But I understand that, if you gate this for platinum users then it will work for both platinum and enterprise users. Enterprise includes everything platinum has. So in essence, just canReadActionsLogManagement will do.

You can also add a test for these license checks if you like.

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.

One minor change is needed with multiple hostnames. I'm approving this now. Feel free to do this in a new PR along with missing tests. I can take another look if you happen to fix this in the current PR.

Thanks for your patience with the review. 🙇 And many thanks for doing this huge PR. 👏

@@ -225,15 +226,15 @@ export const ActionsLogExpandedTray = memo<{
},
{
title: OUTPUT_MESSAGES.expandSection.hostname,
description: hosts?.[agents?.[0]]?.name || emptyValue,
description: map(hosts, (host) => host.name).join(', ') || emptyValue,
Copy link
Member

Choose a reason for hiding this comment

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

This should be for non-flyout(page view). For flyout the previous version was okay. You would also need to pass the isFlyout as a prop to ActionsLogExpandedTray now. Another thing is to filter out '' names. Something like,

description:
            (!isFlyout
              ? Object.values(hosts)
                  .reduce<string[]>((acc, name) => {
                    if (name.name.length) {
                      acc.push(name.name);
                    }
                    return acc;
                  }, [])
                  .join(', ')
              : hosts[agents[0]].name) || emptyValue,

We've also had the logic for this here if you want to pull it out into a new function in x-pack/plugins/security_solution/public/management/components/endpoint_response_actions_list/components/hooks.tsx and then import and use it from there.

@kibana-ci
Copy link
Collaborator

💚 Build Succeeded

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id before after diff
osquery 265 268 +3
securitySolution 3873 3879 +6
total +9

Public APIs missing comments

Total count of every public API that lacks a comment. Target amount is 0. Run node scripts/build_api_docs --plugin [yourplugin] --stats comments for more detailed information.

id before after diff
osquery 22 24 +2

Async chunks

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

id before after diff
osquery 1.1MB 1.1MB +3.7KB
securitySolution 9.2MB 9.2MB +7.1KB
total +10.7KB

Public APIs missing exports

Total count of every type that is part of your API that should be exported but is not. This will cause broken links in the API documentation system. Target amount is 0. Run node scripts/build_api_docs --plugin [yourplugin] --stats exports for more detailed information.

id before after diff
osquery 6 7 +1

Page load bundle

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

id before after diff
osquery 50.1KB 50.3KB +185.0B
Unknown metric groups

API count

id before after diff
osquery 22 24 +2

async chunk count

id before after diff
osquery 11 12 +1

ESLint disabled line counts

id before after diff
enterpriseSearch 19 21 +2
osquery 108 110 +2
securitySolution 399 402 +3
total +7

Total ESLint disabled count

id before after diff
enterpriseSearch 20 22 +2
osquery 109 111 +2
securitySolution 479 482 +3
total +7

History

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

cc @tomsonpl

@tomsonpl tomsonpl merged commit 8f004fd into elastic:main May 24, 2023
@kibanamachine kibanamachine added the backport:skip This commit does not require backporting label May 24, 2023
MadameSheema pushed a commit to MadameSheema/kibana that referenced this pull request May 24, 2023
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 Feature:Osquery Security Solution Osquery feature release_note:feature Makes this part of the condensed release notes Team:Defend Workflows “EDR Workflows” sub-team of Security Solution v8.9.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants