-
Notifications
You must be signed in to change notification settings - Fork 8.1k
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
[SecuritySolution][Endpoint] Add scan
response action API
#184437
[SecuritySolution][Endpoint] Add scan
response action API
#184437
Conversation
/ci |
4e78e38
to
b27d2a8
Compare
/ci |
c339810
to
ae882df
Compare
/ci |
Pinging @elastic/security-defend-workflows (Team:Defend Workflows) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great - thank you.
I left a few minor comments, but am 👍 it. Note that i did not run it locally - only did code review
@@ -759,4 +759,6 @@ describe('actions schemas', () => { | |||
}).toThrow('[file]: expected value of type [Stream] but got [Object]'); | |||
}); | |||
}); | |||
|
|||
describe('ScanActionRequestSchema', () => {}); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
still working on these?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh yeah. I need to add these 😅
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done c44b3e6
@@ -47,7 +47,10 @@ describe( | |||
}); | |||
} | |||
|
|||
for (const actionName of RESPONSE_ACTION_API_COMMANDS_NAMES) { | |||
// TODO: update tests when `scan` is included in PLIs |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Remind me: we have an issue tracking that, correct?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not a separate ticket but the same API ticket has an item in it.
@@ -261,6 +261,8 @@ describe('EndpointActionsClient', () => { | |||
execute: responseActionsClientMock.createExecuteOptions(getCommonResponseActionOptions()), | |||
|
|||
upload: responseActionsClientMock.createUploadOptions(getCommonResponseActionOptions()), | |||
|
|||
scan: responseActionsClientMock.createUploadOptions(getCommonResponseActionOptions()), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
using incorrect mock here. Should create one that is specific for scan
(even if the same options as upload are returned). Will prevent issues if we ever enhance one command to add more parameters, but not for the others that maybe reusing the same props
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sheesh! Yeah I'll update this as well. Thanks for catching this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
FYI: I think that for isolate
/release
I created two separate functions out of responseActionsClientMock
, but internally In that mock file) its actually using the same payload for both. This ensure that consumers of the mock still use the appropriate mock creation function and if we need to ever change them to differ from each other, then only the mock provider internal logic needs to be changed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done 881a8bb
review changes
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Lgtm 👍 added some comments, thanks for considering them :)
@@ -30,7 +30,6 @@ components: | |||
- type: object | |||
required: | |||
- parameters | |||
- file |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Was this removed on purpose?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes. The file
param is redundant. The schema needs a parameters
only.
(apiName) => apiName !== 'unisolate' | ||
)) { | ||
(apiName) => apiName !== 'scan' | ||
).filter((apiName) => apiName !== 'unisolate')) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't understand the double .filter()
here, what do you think about the following? :
for (const actionName of RESPONSE_ACTION_API_COMMANDS_NAMES.filter(
(apiName) => apiName !== 'unisolate' && apiName !== 'scan'
)) {
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah that works the same way. I added another filter function just so that it is easier to delete later and not confuse with the logic for excluding release
action from tests
@@ -95,6 +95,7 @@ describe('When using `getActionList()', () => { | |||
agentType: 'endpoint', | |||
hosts: { 'agent-a': { name: 'Host-agent-a' } }, | |||
command: 'kill-process', | |||
alertIds: undefined, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do we need these changes?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The test needed to be updated as the API command list is now changed resulting in the endpoint action generator (that uses that) now picks the execute
command for the tests instead.
💚 Build Succeeded
Metrics [docs]Async chunks
Unknown metric groupsESLint disabled in files
Total ESLint disabled count
History
To update your PR or re-run it, just comment with: cc @ashokaditya |
…184437) ## Summary Adds a `scan` action response route and related server side logic to handle `scan` action response. Note: A lot of the changes in the PR are due to test updates that resulted out of adding `scan` command to list of API commands. ### Testing 1. Add `responseActionScanEnabled` feature flag to `xpack.securitySolution.enableExperimental` 2. Run ES/Kibana 3. Run `node x-pack/plugins/security_solution/scripts/endpoint/run_endpoint_agent.js` to start a VM with Elastic Defend installed. 4. Visit `app/security/administration/endpoints` and click on the endpoint on the endpoint list. 5. Copy the endpoint id (`selected_endpopint`) from the URL. 6. Use `curl` to send out a `scan` request to the endpoint with `elastic` user. Use the curl command below: <details><summary>curl</summary> <code> curl --location 'http://localhost:5601/api/endpoint/action/scan' \ --header 'kbn-xsrf: test-xsrf' \ --header 'Content-Type: application/json' \ --header 'Authorization: Basic ZWxhc3RpYzpjaGFuZ2VtZQ==' \ --data '{ "endpoint_ids": [ "copied_endpoint_id" ], "parameters": { "path": "/home/ubuntu" } }' </code> </details> 7. You should see the action created on the response actions history for the endpoint. 8. Using any other non existing file path will result in a failed action. UX work to address this will follow. 9. Disabling/removing `responseActionScanEnabled` feature flag should give you a not found error when accessing the API. ### Checklist - [x] Any text added follows [EUI's writing guidelines](https://elastic.github.io/eui/#/guidelines/writing), uses sentence case text and includes [i18n support](https://github.com/elastic/kibana/blob/main/packages/kbn-i18n/README.md) - [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 - [ ] [Flaky Test Runner](https://ci-stats.kibana.dev/trigger_flaky_test_runner/1) was used on any tests changed --------- Co-authored-by: kibanamachine <42973632+kibanamachine@users.noreply.github.com>
Summary
Adds a
scan
action response route and related server side logic to handlescan
action response.Note: A lot of the changes in the PR are due to test updates that resulted out of adding
scan
command to list of API commands.Testing
responseActionScanEnabled
feature flag toxpack.securitySolution.enableExperimental
node x-pack/plugins/security_solution/scripts/endpoint/run_endpoint_agent.js
to start a VM with Elastic Defend installed.app/security/administration/endpoints
and click on the endpoint on the endpoint list.selected_endpopint
) from the URL.curl
to send out ascan
request to the endpoint withelastic
user. Use the curl command below:curl
curl --location 'http://localhost:5601/api/endpoint/action/scan' \ --header 'kbn-xsrf: test-xsrf' \ --header 'Content-Type: application/json' \ --header 'Authorization: Basic ZWxhc3RpYzpjaGFuZ2VtZQ==' \ --data '{ "endpoint_ids": [ "copied_endpoint_id" ], "parameters": { "path": "/home/ubuntu" } }'
responseActionScanEnabled
feature flag should give you a not found error when accessing the API.Checklist