Skip to content

Commit 7653767

Browse files
authored
[Security Solution][Endpoint] Fix automated response actions when spaces is enabled and action can not be sent (#226043)
## Summary PR fixes the following issue when spaces is enabled: - When automated response actions are processed, if unable to determine the policy associated with the agent the action is being sent to, we should still create the failed action request (as we did before spaces was enabled) - Although this fix creates the action request in failed state, it will not be visible in the UI - both under the alert that triggered it and the action history log - because we are not able to determine access to the action due to not having policy information for the agent. - Action failure will tagged as "orphan" and thus it will be displayed in the action history log in the space that is configured to show orphan actions
1 parent cec1a92 commit 7653767

File tree

8 files changed

+137
-16
lines changed

8 files changed

+137
-16
lines changed

x-pack/solutions/security/plugins/security_solution/server/endpoint/services/actions/action_details_by_id.test.ts

Lines changed: 15 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -179,4 +179,19 @@ describe('When using `getActionDetailsById()', () => {
179179
})
180180
);
181181
});
182+
183+
it('should not validate against spaces when `bypassSpaceValidation` is `true`', async () => {
184+
// @ts-expect-error
185+
endpointAppContextService.experimentalFeatures.endpointManagementSpaceAwarenessEnabled = true;
186+
(
187+
endpointAppContextService.getInternalFleetServices().ensureInCurrentSpace as jest.Mock
188+
).mockResolvedValue(undefined);
189+
await getActionDetailsById(endpointAppContextService, 'default', '123', {
190+
bypassSpaceValidation: true,
191+
});
192+
193+
expect(
194+
endpointAppContextService.getInternalFleetServices().ensureInCurrentSpace
195+
).not.toHaveBeenCalled();
196+
});
182197
});

x-pack/solutions/security/plugins/security_solution/server/endpoint/services/actions/action_details_by_id.ts

Lines changed: 11 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -27,7 +27,16 @@ import { NotFoundError } from '../../errors';
2727
export const getActionDetailsById = async <T extends ActionDetails = ActionDetails>(
2828
endpointService: EndpointAppContextService,
2929
spaceId: string,
30-
actionId: string
30+
actionId: string,
31+
{
32+
bypassSpaceValidation = false,
33+
}: Partial<{
34+
/**
35+
* if `true`, then no space validations will be done on the action retrieved. Default is `false`.
36+
* USE IT CAREFULLY!
37+
*/
38+
bypassSpaceValidation: boolean;
39+
}> = {}
3140
): Promise<T> => {
3241
let normalizedActionRequest: ReturnType<typeof mapToNormalizedActionRequest> | undefined;
3342
let actionResponses: FetchActionResponsesResult;
@@ -36,7 +45,7 @@ export const getActionDetailsById = async <T extends ActionDetails = ActionDetai
3645
// Get both the Action Request(s) and action Response(s)
3746
const [actionRequestEsDoc, actionResponseResult] = await Promise.all([
3847
// Get the action request(s)
39-
fetchActionRequestById(endpointService, spaceId, actionId),
48+
fetchActionRequestById(endpointService, spaceId, actionId, { bypassSpaceValidation }),
4049

4150
// Get all responses
4251
fetchActionResponses({

x-pack/solutions/security/plugins/security_solution/server/endpoint/services/actions/clients/endpoint/endpoint_actions_client.test.ts

Lines changed: 48 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -21,6 +21,7 @@ import { Readable } from 'stream';
2121
import { EndpointActionGenerator } from '../../../../../../common/endpoint/data_generators/endpoint_action_generator';
2222
import type { ResponseActionsRequestBody } from '../../../../../../common/api/endpoint';
2323
import { AgentNotFoundError } from '@kbn/fleet-plugin/server';
24+
import { ALLOWED_ACTION_REQUEST_TAGS } from '../../constants';
2425

2526
jest.mock('../../action_details_by_id', () => {
2627
const originalMod = jest.requireActual('../../action_details_by_id');
@@ -596,5 +597,52 @@ describe('EndpointActionsClient', () => {
596597
).not.toHaveBeenCalled();
597598
}
598599
);
600+
601+
it('should create failed action request for automated response actions', async () => {
602+
classConstructorOptions.isAutomated = true;
603+
// @ts-expect-error mocking this for testing purposes
604+
endpointActionsClient.checkAgentIds = jest.fn().mockResolvedValueOnce({
605+
isValid: false,
606+
valid: [],
607+
invalid: ['invalid-id'],
608+
hosts: [{ agent: { id: 'invalid-id', name: '' }, host: { hostname: '' } }],
609+
});
610+
611+
await endpointActionsClient.isolate(
612+
responseActionsClientMock.createIsolateOptions(getCommonResponseActionOptions())
613+
);
614+
615+
expect(classConstructorOptions.esClient.index).toHaveBeenCalledWith(
616+
expect.objectContaining({
617+
document: expect.objectContaining({
618+
agent: { id: [], policy: [] },
619+
tags: [ALLOWED_ACTION_REQUEST_TAGS.integrationPolicyDeleted],
620+
}),
621+
}),
622+
expect.anything()
623+
);
624+
});
625+
626+
it('should return action details for failed automated response actions even when no valid agents', async () => {
627+
classConstructorOptions.isAutomated = true;
628+
// @ts-expect-error mocking this for testing purposes
629+
endpointActionsClient.checkAgentIds = jest.fn().mockResolvedValueOnce({
630+
isValid: false,
631+
valid: [],
632+
invalid: ['invalid-id'],
633+
hosts: [{ agent: { id: 'invalid-id', name: '' }, host: { hostname: '' } }],
634+
});
635+
636+
await endpointActionsClient.isolate(
637+
responseActionsClientMock.createIsolateOptions(getCommonResponseActionOptions())
638+
);
639+
640+
expect(getActionDetailsByIdMock).toHaveBeenCalledWith(
641+
expect.anything(),
642+
expect.anything(),
643+
expect.anything(),
644+
{ bypassSpaceValidation: true }
645+
);
646+
});
599647
});
600648
});

x-pack/solutions/security/plugins/security_solution/server/endpoint/services/actions/clients/endpoint/endpoint_actions_client.ts

Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -195,7 +195,10 @@ export class EndpointActionsClient extends ResponseActionsClientImpl {
195195
}),
196196
});
197197

198-
return this.fetchActionDetails<TResponse>(actionId);
198+
// We bypass space validation when retrieving the action details to ensure that if a failed
199+
// action was created, and it did not contain the agent policy information (and space is enabled)
200+
// we don't trigger an error.
201+
return this.fetchActionDetails<TResponse>(actionId, true);
199202
}
200203

201204
private async dispatchActionViaFleet({

x-pack/solutions/security/plugins/security_solution/server/endpoint/services/actions/clients/lib/base_response_actions_client.test.ts

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -293,7 +293,8 @@ describe('ResponseActionsClientImpl base class', () => {
293293
expect(getActionDetailsByIdMock).toHaveBeenCalledWith(
294294
expect.anything(),
295295
expect.anything(),
296-
'one'
296+
'one',
297+
{ bypassSpaceValidation: false }
297298
);
298299
});
299300
});

x-pack/solutions/security/plugins/security_solution/server/endpoint/services/actions/clients/lib/base_response_actions_client.ts

Lines changed: 30 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -16,6 +16,7 @@ import type { QueryDslQueryContainer } from '@elastic/elasticsearch/lib/api/type
1616
import type { PackagePolicy } from '@kbn/fleet-plugin/common';
1717
import { PACKAGE_POLICY_SAVED_OBJECT_TYPE } from '@kbn/fleet-plugin/common';
1818
import type { ResponseActionRequestTag } from '../../constants';
19+
import { ALLOWED_ACTION_REQUEST_TAGS } from '../../constants';
1920
import { getUnExpiredActionsEsQuery } from '../../utils/fetch_space_ids_with_maybe_pending_actions';
2021
import { catchAndWrapError } from '../../../../utils';
2122
import {
@@ -452,12 +453,20 @@ export abstract class ResponseActionsClientImpl implements ResponseActionsClient
452453
/**
453454
* Returns the action details for a given response action id
454455
* @param actionId
456+
* @param bypassSpaceValidation
455457
* @protected
456458
*/
457459
protected async fetchActionDetails<T extends ActionDetails = ActionDetails>(
458-
actionId: string
460+
actionId: string,
461+
/**
462+
* if `true`, then no space validations will be done on the action retrieved. Default is `false`.
463+
* USE IT CAREFULLY!
464+
*/
465+
bypassSpaceValidation: boolean = false
459466
): Promise<T> {
460-
return getActionDetailsById(this.options.endpointService, this.options.spaceId, actionId);
467+
return getActionDetailsById(this.options.endpointService, this.options.spaceId, actionId, {
468+
bypassSpaceValidation,
469+
});
461470
}
462471

463472
/**
@@ -611,29 +620,41 @@ export abstract class ResponseActionsClientImpl implements ResponseActionsClient
611620

612621
this.notifyUsage(actionRequest.command);
613622

623+
const actionId = actionRequest.actionId || uuidv4();
624+
const tags = actionRequest.tags ?? [];
625+
626+
// With automated response action, it's possible to reach this point and not have any `endpoint_ids`
627+
// defined in the action. That's because with automated response actions we always create an
628+
// action request, even when there is a failure - like if the agent was un-enrolled in between
629+
// the event sent and the detection engine processing that event.
630+
const agentPolicyInfo =
631+
isSpacesEnabled && actionRequest.endpoint_ids.length
632+
? await this.fetchAgentPolicyInfo(actionRequest.endpoint_ids)
633+
: [];
634+
635+
if (isSpacesEnabled && agentPolicyInfo.length === 0) {
636+
tags.push(ALLOWED_ACTION_REQUEST_TAGS.integrationPolicyDeleted);
637+
}
638+
614639
const doc: LogsEndpointAction<TParameters, TOutputContent, TMeta> = {
615640
'@timestamp': new Date().toISOString(),
616641

617642
// Add the `originSpaceId` property to the document if spaces is enabled
618643
...(isSpacesEnabled ? { originSpaceId: this.options.spaceId } : {}),
619644

620645
// Add `tags` property to the document if spaces is enabled
621-
...(isSpacesEnabled ? { tags: actionRequest.tags ?? [] } : {}),
646+
...(isSpacesEnabled ? { tags } : {}),
622647

623648
// Need to suppress this TS error around `agent.policy` not supporting `undefined`.
624649
// It will be removed once we enable the feature and delete the feature flag checks.
625650
// @ts-expect-error
626651
agent: {
627652
id: actionRequest.endpoint_ids,
628653
// add the `policy` info if space awareness is enabled
629-
...(isSpacesEnabled
630-
? {
631-
policy: await this.fetchAgentPolicyInfo(actionRequest.endpoint_ids),
632-
}
633-
: {}),
654+
...(isSpacesEnabled ? { policy: agentPolicyInfo } : {}),
634655
},
635656
EndpointActions: {
636-
action_id: actionRequest.actionId || uuidv4(),
657+
action_id: actionId,
637658
expiration: getActionRequestExpiration(),
638659
type: 'INPUT_ACTION',
639660
input_type: this.agentType,

x-pack/solutions/security/plugins/security_solution/server/endpoint/services/actions/utils/fetch_action_request_by_id.test.ts

Lines changed: 13 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -117,5 +117,18 @@ describe('fetchActionRequestById() utility', () => {
117117
'Action [123] not found'
118118
);
119119
});
120+
121+
it('should not validate action against spaces if `bypassSpaceValidation` is true', async () => {
122+
(
123+
endpointServiceMock.getInternalFleetServices().ensureInCurrentSpace as jest.Mock
124+
).mockResolvedValue(undefined);
125+
await fetchActionRequestById(endpointServiceMock, 'default', '123', {
126+
bypassSpaceValidation: true,
127+
});
128+
129+
expect(
130+
endpointServiceMock.getInternalFleetServices().ensureInCurrentSpace as jest.Mock
131+
).not.toHaveBeenCalled();
132+
});
120133
});
121134
});

x-pack/solutions/security/plugins/security_solution/server/endpoint/services/actions/utils/fetch_action_request_by_id.ts

Lines changed: 14 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -25,7 +25,6 @@ import { REF_DATA_KEYS } from '../../../lib/reference_data';
2525
* @param endpointService
2626
* @param spaceId
2727
* @param actionId
28-
*
2928
* @throws
3029
*/
3130
export const fetchActionRequestById = async <
@@ -35,7 +34,16 @@ export const fetchActionRequestById = async <
3534
>(
3635
endpointService: EndpointAppContextService,
3736
spaceId: string,
38-
actionId: string
37+
actionId: string,
38+
{
39+
bypassSpaceValidation = false,
40+
}: Partial<{
41+
/**
42+
* if `true`, then no space validations will be done on the action retrieved. Default is `false`.
43+
* USE IT CAREFULLY!
44+
*/
45+
bypassSpaceValidation: boolean;
46+
}> = {}
3947
): Promise<LogsEndpointAction<TParameters, TOutputContent, TMeta>> => {
4048
const logger = endpointService.createLogger('fetchActionRequestById');
4149
const searchResponse = await endpointService
@@ -54,7 +62,10 @@ export const fetchActionRequestById = async <
5462

5563
if (!actionRequest) {
5664
throw new NotFoundError(`Action with id '${actionId}' not found.`);
57-
} else if (endpointService.experimentalFeatures.endpointManagementSpaceAwarenessEnabled) {
65+
} else if (
66+
endpointService.experimentalFeatures.endpointManagementSpaceAwarenessEnabled &&
67+
!bypassSpaceValidation
68+
) {
5869
if (!actionRequest.agent.policy || actionRequest.agent.policy.length === 0) {
5970
const message = `Response action [${actionId}] missing 'agent.policy' information - unable to determine if response action is accessible for space [${spaceId}]`;
6071

0 commit comments

Comments
 (0)