Skip to content

Commit

Permalink
[Security Solution][Endpoint] Use "valid" agents for creating action …
Browse files Browse the repository at this point in the history 
…requests and updating cases (#187145)

## Summary

This PR corrects an API we were able to create multi-agent action for
duplicate/non-existing agent ids. The changes in the PR fix that
1. by not including invalid agent ids (that do not have Elastic Defend
installed) and
2. adds a warning info in the action request comment so that the user is
aware of discarded agents



This action when errored shows multi agent action Host/Error grouping.

**Before** (notice the single `Hostname` above outputs)
You could do this:
```json5
POST /api/endpoint/action/suspend_process
{
    "endpoint_ids": [
        "1d793b46-7b6a-4dd4-a6b7-4c75718e04be",
        "1d793b46-7b6a-4dd4-a6b7-4c75718e04be"
    ],
    "parameters": {
        "pid": "42424242"
    }
}
``` 
and get a response that looked like (notice the duplicate agent ids in
`agents`)
```json5
{
    "data": {
        "id": "ff66dc36-5be9-4b12-a94f-09421baed8da",
        "agentType": "endpoint",
        "agents": [
            "1d793b46-7b6a-4dd4-a6b7-4c75718e04be",
             1d793b46-7b6a-4dd4-a6b7-4c75718e04be
        ],
        "hosts": {
            "1d793b46-7b6a-4dd4-a6b7-4c75718e04be": {
                "name": "ashokaditya-dev-7131"
            }
        },
        "command": "suspend-process",
        "startedAt": "2024-06-28T13:56:40.524Z",
        "isCompleted": false,
        "wasSuccessful": false,
        "isExpired": false,
        "status": "pending",
        "outputs": {},
        "agentState": {
            "1d793b46-7b6a-4dd4-a6b7-4c75718e04be": {
                "isCompleted": false,
                "wasSuccessful": false
            }
        },
        "createdBy": "elastic",
        "parameters": {
            "pid": 4242424242
        }
    }
}
```

**after**
With this change when you try and send a request such as
```json5
POST /api/endpoint/action/suspend_process
{
    "endpoint_ids": [
        "1d793b46-7b6a-4dd4-a6b7-4c75718e04be",
        "1d793b46-7b6a-4dd4-a6b7-4c75718e04be",
        "no-dice"
    ]
    ,
    "parameters": {
        "pid": 4242424242
    }
}
```

you get a response that looks like (notice neither the duplicate agent
id nor the invalid agent id in `agents`, also additional WARNING in the
`comment` field)
```json5
{
    "data": {
        "id": "9bf67816-4d58-4843-bc7a-776f6017b06b",
        "agentType": "endpoint",
        "agents": [
            "1d793b46-7b6a-4dd4-a6b7-4c75718e04be"
        ],
        "hosts": {
            "1d793b46-7b6a-4dd4-a6b7-4c75718e04be": {
                "name": "ashokaditya-dev-7131"
            }
        },
        "command": "suspend-process",
        "startedAt": "2024-06-28T15:13:20.138Z",
        "isCompleted": false,
        "wasSuccessful": false,
        "isExpired": false,
        "status": "pending",
        "outputs": {},
        "agentState": {
            "1d793b46-7b6a-4dd4-a6b7-4c75718e04be": {
                "isCompleted": false,
                "wasSuccessful": false
            }
        },
        "createdBy": "elastic",
        "comment": "undefined; WARNING: The following agent ids are not valid: [\"no-dice\"] and would not be included in action request.",
        "parameters": {
            "pid": 4242424242
        }
    }
}
```
follow up of /pull/186284

### Checklist
- [x] [Unit or functional
tests](https://www.elastic.co/guide/en/kibana/master/development-tests.html)
were updated or added to match the most common scenarios

### Risk Matrix

Delete this section if it is not applicable to this PR.

Before closing this PR, invite QA, stakeholders, and other developers to
identify risks that should be tested prior to the change/feature
release.

When forming the risk matrix, consider some of the following examples
and how they may potentially impact the change:

| Risk | Probability | Severity | Mitigation/Notes |

|---------------------------|-------------|----------|-------------------------|
| Multiple Spaces—unexpected behavior in non-default Kibana Space.
| Low | High | Integration tests will verify that all features are still
supported in non-default Kibana Space and when user switches between
spaces. |
| Multiple nodes—Elasticsearch polling might have race conditions
when multiple Kibana nodes are polling for the same tasks. | High | Low
| Tasks are idempotent, so executing them multiple times will not result
in logical error, but will degrade performance. To test for this case we
add plenty of unit tests around this logic and document manual testing
procedure. |
| Code should gracefully handle cases when feature X or plugin Y are
disabled. | Medium | High | Unit tests will verify that any feature flag
or plugin combination still results in our service operational. |
| [See more potential risk
examples](https://github.com/elastic/kibana/blob/main/RISK_MATRIX.mdx) |


### For maintainers

- [ ] This was checked for breaking API changes and was [labeled
appropriately](https://www.elastic.co/guide/en/kibana/master/contributing.html#kibana-release-notes-process)
  • Loading branch information
@ashokaditya
ashokaditya authored Jul 1, 2024
1 parent 79f71b7 commit cbe6b7b
Show file tree
Hide file tree
Showing 2 changed files with 144 additions and 17 deletions.
126 changes: 117 additions & 9 deletions ...olution/server/endpoint/services/actions/clients/endpoint/endpoint_actions_client.test.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,7 @@ describe('EndpointActionsClient', () => {
'endpoint_ids' | 'case_ids'
> => {
return {
endpoint_ids: ['1-2-3', 'invalid-id'],
endpoint_ids: ['1-2-3', 'invalid-id', '1-2-3'],
case_ids: ['case-a'],
};
};
Expand All @@ -42,8 +42,8 @@ describe('EndpointActionsClient', () => {
responseActionsClientMock.createIsolateOptions(getCommonResponseActionOptions())
);

expect(classConstructorOptions.endpointService.createLogger().debug).toHaveBeenCalledWith(
'The following agent ids are not valid: ["invalid-id"]'
expect(classConstructorOptions.endpointService.createLogger().warn).toHaveBeenCalledWith(
'The following agent ids are not valid: ["invalid-id"] and will not be included in action request'
);
});

Expand Down Expand Up @@ -85,11 +85,87 @@ describe('EndpointActionsClient', () => {
});
});

it('should write action request document', async () => {
it('should write action request document to endpoint action request index with given set of valid/invalid agent ids', async () => {
await endpointActionsClient.isolate(
responseActionsClientMock.createIsolateOptions(getCommonResponseActionOptions())
);

expect(classConstructorOptions.esClient.index).toHaveBeenCalledWith(
{
index: ENDPOINT_ACTIONS_INDEX,
document: {
'@timestamp': expect.any(String),
EndpointActions: {
action_id: expect.any(String),
data: {
command: 'isolate',
comment:
'test comment. (WARNING: The following agent ids are not valid: ["invalid-id"] and will not be included in action request)',
parameters: undefined,
},
expiration: expect.any(String),
input_type: 'endpoint',
type: 'INPUT_ACTION',
},
agent: {
id: ['1-2-3'],
},
user: {
id: 'foo',
},
},
refresh: 'wait_for',
},
expect.anything()
);
});

it('should write correct comment when invalid agent ids', async () => {
await endpointActionsClient.isolate(
responseActionsClientMock.createIsolateOptions({
...getCommonResponseActionOptions(),
comment: '',
})
);

expect(classConstructorOptions.esClient.index).toHaveBeenCalledWith(
{
index: ENDPOINT_ACTIONS_INDEX,
document: {
'@timestamp': expect.any(String),
EndpointActions: {
action_id: expect.any(String),
data: {
command: 'isolate',
comment:
'(WARNING: The following agent ids are not valid: ["invalid-id"] and will not be included in action request)',
parameters: undefined,
},
expiration: expect.any(String),
input_type: 'endpoint',
type: 'INPUT_ACTION',
},
agent: {
id: ['1-2-3'],
},
user: {
id: 'foo',
},
},
refresh: 'wait_for',
},
expect.anything()
);
});

it('should write action request document to endpoint action request index with given valid agent ids', async () => {
await endpointActionsClient.isolate(
responseActionsClientMock.createIsolateOptions({
endpoint_ids: ['1-2-3'],
case_ids: ['case-a'],
})
);

expect(classConstructorOptions.esClient.index).toHaveBeenCalledWith(
{
index: ENDPOINT_ACTIONS_INDEX,
Expand All @@ -107,7 +183,7 @@ describe('EndpointActionsClient', () => {
type: 'INPUT_ACTION',
},
agent: {
id: ['1-2-3', 'invalid-id'],
id: ['1-2-3'],
},
user: {
id: 'foo',
Expand All @@ -119,9 +195,12 @@ describe('EndpointActionsClient', () => {
);
});

it('should update cases', async () => {
it('should update cases for valid agent ids', async () => {
await endpointActionsClient.isolate(
responseActionsClientMock.createIsolateOptions(getCommonResponseActionOptions())
responseActionsClientMock.createIsolateOptions({
endpoint_ids: ['1-2-3'],
case_ids: ['case-a'],
})
);

expect(classConstructorOptions.casesClient?.attachments.bulkCreate).toHaveBeenCalledWith({
Expand All @@ -138,10 +217,38 @@ describe('EndpointActionsClient', () => {
endpointId: '1-2-3',
hostname: 'Host-ku5jy6j0pw',
},
],
},
externalReferenceStorage: {
type: 'elasticSearchDoc',
},
owner: 'securitySolution',
type: 'externalReference',
},
],
caseId: 'case-a',
});
});

it('should update cases for valid/invalid agent ids', async () => {
await endpointActionsClient.isolate(
responseActionsClientMock.createIsolateOptions(getCommonResponseActionOptions())
);

expect(classConstructorOptions.casesClient?.attachments.bulkCreate).toHaveBeenCalledWith({
attachments: [
{
externalReferenceAttachmentTypeId: 'endpoint',
externalReferenceId: expect.any(String),
externalReferenceMetadata: {
command: 'isolate',
comment:
'test comment. (WARNING: The following agent ids are not valid: ["invalid-id"] and will not be included in action request)',
targets: [
{
agentType: 'endpoint',
endpointId: 'invalid-id',
hostname: '',
endpointId: '1-2-3',
hostname: 'Host-ku5jy6j0pw',
},
],
},
Expand Down Expand Up @@ -175,6 +282,7 @@ describe('EndpointActionsClient', () => {
{ meta: true }
);
});

it('should create an action with error when agents are invalid', async () => {
// @ts-expect-error mocking this for testing purposes
endpointActionsClient.checkAgentIds = jest.fn().mockResolvedValueOnce({
Expand Down
35 changes: 27 additions & 8 deletions ...ity_solution/server/endpoint/services/actions/clients/endpoint/endpoint_actions_client.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -52,6 +52,13 @@ import type {
} from '../lib/types';
import { DEFAULT_EXECUTE_ACTION_TIMEOUT } from '../../../../../../common/endpoint/service/response_actions/constants';

const getInvalidAgentsWarning = (invalidAgents: string[]) =>
invalidAgents.length
? `The following agent ids are not valid: ${JSON.stringify(
invalidAgents
)} and will not be included in action request`
: '';

export class EndpointActionsClient extends ResponseActionsClientImpl {
protected readonly agentType: ResponseActionAgentType = 'endpoint';

Expand All @@ -61,14 +68,15 @@ export class EndpointActionsClient extends ResponseActionsClientImpl {
allValid: boolean;
hosts: HostMetadata[];
}> {
const uniqueIds = [...new Set(ids)];
const foundEndpointHosts = await this.options.endpointService
.getEndpointMetadataService()
.getMetadataForEndpoints(this.options.esClient, [...new Set(ids)]);
.getMetadataForEndpoints(this.options.esClient, uniqueIds);
const validIds = foundEndpointHosts.map((endpoint: HostMetadata) => endpoint.elastic.agent.id);
const invalidIds = ids.filter((id) => !validIds.includes(id));

if (invalidIds.length) {
this.log.debug(`The following agent ids are not valid: ${JSON.stringify(invalidIds)}`);
this.log.warn(getInvalidAgentsWarning(invalidIds));
}

return {
Expand All @@ -88,12 +96,12 @@ export class EndpointActionsClient extends ResponseActionsClientImpl {
actionReq: TOptions,
options?: TMethodOptions
): Promise<TResponse> {
const agentIds = await this.checkAgentIds(actionReq.endpoint_ids);
const validatedAgents = await this.checkAgentIds(actionReq.endpoint_ids);
const actionId = uuidv4();
const { error: validationError } = await this.validateRequest({
...actionReq,
command,
endpoint_ids: agentIds.valid || [],
endpoint_ids: validatedAgents.valid || [],
});

const { hosts, ruleName, ruleId, error } = this.getMethodOptions<TMethodOptions>(options);
Expand All @@ -104,7 +112,7 @@ export class EndpointActionsClient extends ResponseActionsClientImpl {
try {
await this.dispatchActionViaFleet({
actionId,
agents: agentIds.valid,
agents: validatedAgents.valid,
data: {
command,
comment: actionReq.comment,
Expand All @@ -122,29 +130,40 @@ export class EndpointActionsClient extends ResponseActionsClientImpl {
}
}

// Append warning message to comment if there are invalid agents
const commentMessage = actionReq.comment ? actionReq.comment : '';
const warningMessage = `(WARNING: ${getInvalidAgentsWarning(validatedAgents.invalid)})`;
const comment = validatedAgents.invalid.length
? commentMessage
? `${commentMessage}. ${warningMessage}`
: warningMessage
: actionReq.comment;

// Write action to endpoint index
await this.writeActionRequestToEndpointIndex({
...actionReq,
endpoint_ids: validatedAgents.valid,
error: actionError,
ruleId,
ruleName,
hosts,
actionId,
command,
comment,
});

// Update cases
await this.updateCases({
command,
actionId,
comment: actionReq.comment,
comment,
caseIds: actionReq.case_ids,
alertIds: actionReq.alert_ids,
hosts: actionReq.endpoint_ids.map((hostId) => {
hosts: validatedAgents.valid.map((hostId) => {
return {
hostId,
hostname:
agentIds.hosts.find((host) => host.agent.id === hostId)?.host.hostname ??
validatedAgents.hosts.find((host) => host.agent.id === hostId)?.host.hostname ??
hosts?.[hostId].name ??
'',
};
Expand Down

0 comments on commit cbe6b7b

Please sign in to comment.