-
Notifications
You must be signed in to change notification settings - Fork 8.3k
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
[EDR Workflows] Add RunScript CS Command - UI #202012
Conversation
/ci |
/ci |
/ci |
/ci |
/ci |
/ci |
/ci |
/ci |
/ci |
/ci |
1 similar comment
/ci |
/ci |
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.
Thanks for the changes. I have left some questions and suggestions that I would like to hear before I approve. Also, you're missing a ff check for action type filter in https://github.com/elastic/kibana/blob/main/x-pack/plugins/security_solution/public/management/components/endpoint_response_actions_list/components/hooks.tsx
output = { | ||
type: 'json', | ||
content: { | ||
code: 'ra_runscript_success_done', |
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.
Do you also have a list of error codes? Then you should add those below in a function as well.
randomRunscriptFailureCode()
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 have a list unfortunately, I noted a few codes though so when I conitnue working on CrowdStrike connector and results - I'll adjust the CS errors list 👍
about: i18n.translate( | ||
'xpack.securitySolution.crowdStrikeConsoleCommands.runscript.args.hostPath.about', | ||
{ | ||
defaultMessage: 'Absolute or relative path of script on host machine', |
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.
defaultMessage: 'Absolute or relative path of script on host machine', | |
defaultMessage: 'Absolute or relative path of script file on host machine', |
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.
This was taken 1:1 from CrowdStrike console - although I am ok with adjusting if you prefer
helpUsage: `runscript -CloudFile="CloudScript1.ps1" -CommandLine="-Verbose true" | ||
Run a script saved to the CrowdStrike cloud with the specified command line arguments | ||
runscript -CloudFile="CloudScript1.ps1" -CommandLine="-Verbose true" -Timeout=180 | ||
Run a script saved to the CrowdStrike cloud with the specified command line arguments and 180 seconds timeout | ||
runscript -Raw=\`\`\`Get-ChildItem .\`\`\` -CommandLine="" | ||
Run a raw script whose entire contents are provided in the "-Raw=" flag | ||
runscript -HostPath="C:\\temp\\LocalScript.ps1" -CommandLine="-Verbose true" | ||
Run a script from a path on the remote host with the specified command line arguments`, |
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.
In the screenshot, I notice the first line is indented separately from the rest of the lines.
Also, I feel the set of example help text here is a bit too dense to read on the screen. Consider visually separating the help title/text from the actual example command. Why not have the help description on top of the help syntax?
Something like
-- Run a script saved to the CrowdStrike cloud with the specified command line arguments
runscript -CloudFile="CloudScript1.ps1" -CommandLine="-Verbose true"
--Run a script saved to the CrowdStrike cloud with the specified command line arguments and 180 seconds timeout
runscript -CloudFile="CloudScript1.ps1" -CommandLine="-Verbose true" -Timeout=180
...
...
etc.
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.
Applied some changes - hope it looks better :) I might create a component to render it nicer but I'd prefer to do it at the later stage if we still have time before 8.18 👍
about: i18n.translate( | ||
'xpack.securitySolution.crowdStrikeConsoleCommands.runscript.args.cloudFile.about', | ||
{ | ||
defaultMessage: 'Script name in cloud storage', |
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.
Q: Should be file name I think? Unless script name and file name are the same?
defaultMessage: 'Script name in cloud storage', | |
defaultMessage: 'Script file name in cloud storage', |
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.
Good question. So scripts are on the Cloud, files are on a host. Again I copied this over from CrowdStrike console. But I am happy to adjust if you prefer
@@ -22,7 +22,7 @@ describe( | |||
// how to enable experimental features in the Cypress tests. | |||
// kbnServerArgs: [ | |||
// `--xpack.securitySolution.enableExperimental=${JSON.stringify([ | |||
// 'featureFlagName', | |||
// 'crowdstrikeRunScriptEnabled', |
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.
Do you need this change?
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.
Good catch. This can be reverted for the purposes of this PR, will be needed for another one.
@@ -28,7 +28,7 @@ describe( | |||
// how to enable experimental features in the Cypress tests. | |||
// kbnServerArgs: [ | |||
// `--xpack.securitySolution.enableExperimental=${JSON.stringify([ | |||
// 'featureFlagName', | |||
// 'crowdstrikeRunScriptEnabled', |
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.
also here
runscript: { | ||
endpoint: false, | ||
sentinel_one: false, | ||
crowdstrike: true, |
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.
Q: Looks like this is yet to be updated to false
?
// TODO: RUNSCRIPT does not have a corresponding action in the API yet | ||
const TEMPORARY_LENGTH = RESPONSE_ACTION_API_COMMANDS_NAMES.length - 1; | ||
expect(getAllByTestId(`${filterPrefix}-option`).length).toEqual(TEMPORARY_LENGTH); |
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.
This is an issue with the EUI filter popover component. The popover list shows 9 items at a time on the default height. We need to collect the options using querySelectorAll
or something like that. I think worth noting this glitch in a comment here 😓
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 tried adjusting according to your suggestions but nothing helped. I'd take a note though and try to address this in another PR if this is ok with you? :)
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.
Ok @ashokaditya showed me on zoom, that this was indeed the case. I was adjusting wrong components height 😅 Thanks!
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 found a workaround and sent you the diff.
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.
Applied, thanks a lot 👍
/ci |
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.
Thanks for the changes.
it('should show a list of actions (with `scan`) when opened', async () => { | ||
render(); | ||
it('should show a list of actions (with `runscript`) when opened', async () => { | ||
render({ 'data-test-height': 350 }); |
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.
Just a note that we would need to have larger heights here as the number of commands increases. We might want to add a note somewhere here about that.
...olution/public/management/components/endpoint_response_actions_list/response_actions_log.tsx
Outdated
Show resolved
Hide resolved
…endpoint_response_actions_list/response_actions_log.tsx Co-authored-by: Ash <[email protected]>
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.
2nd round of feedback provided - I still see several issues. Let me know if you have any questions.
also, I checked out the PR and test it manually... here is what I found:
- the
help
command output does not show therunscript
correctly (note the--
. I think this is due to the fact that you have the command configured to require arguments, but tehre are no required arguments defined. I think its due to the fact the fact that you did not define the arguments correctly for the mutually exclusive arguments (see my code comment) - There are argument validations in place. I just entered
runscript --comment
and it was accepted
@@ -33,6 +33,13 @@ export const CommandInputUsage = memo<Pick<CommandUsageProps, 'commandDef'>>(({ | |||
}); | |||
}, [commandDef]); | |||
|
|||
const helpExample = useMemo(() => { |
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 did we need to add a helpUsage
property if exampleUsage
already existed? Is it because you wanted to have the input hint be different from what --help
displays?
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, that was the reason 👍
x-pack/plugins/security_solution/public/management/components/console/types.ts
Show resolved
Hide resolved
privileges: endpointPrivileges, | ||
}, | ||
exampleUsage: `runscript -Raw=\`\`\`Get-ChildItem .\`\`\` -CommandLine=""`, | ||
helpUsage: ` |
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.
This needs to be i18n
.
also - examples below are showing the argument with a single dash -
instead of two --
capabilities: endpointCapabilities, | ||
privileges: endpointPrivileges, | ||
}, | ||
exampleUsage: `runscript -Raw=\`\`\`Get-ChildItem .\`\`\` -CommandLine=""`, |
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.
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.
- Changing
-
into--
thanks! - I'd say it could support both, or even more:
runscript --Raw=ls
|runscript --Raw="ls"
|runscript --Raw='ls'
| 'runscript --Raw=ls
'
required: false, | ||
allowMultiples: false, | ||
about: CROWDSTRIKE_CONSOLE_COMMANDS.runscript.args.timeout.about, | ||
mustHaveValue: 'non-empty-string', |
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.
allowMultiples: false, | ||
about: CROWDSTRIKE_CONSOLE_COMMANDS.runscript.args.hostPath.about, | ||
mustHaveValue: 'non-empty-string', | ||
}, |
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 thought you said that 1 of these tree needed to be used... so should have the exclusiveOr
property set to true, no?
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.
This is a bit tricky, since CrowdStrike lets you add multiple of these arguments, and then the strange behavior I explained during the meeting happened. But I agree with you that our approach would be to use exclusiveOr
and just let them use one at a time.
Added exclusiveOr
👍 Thank you, I also added a description to exclusiveOr type.
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.
Ok. Sorry about this - I do now remember that we discussed that they can use multiples, but at least one of them must be present. In that case, you are correct in that we can't use exclusiveOr
because that limits the options for the user to having to only pick one.
The main issue I saw when testing this is that there was no validations in place. Realizing now that all three could be used, and that at least 1 must be defined, I think the perhaps the command should be setup as:
- Remove
exclusiveOr
from the arguments - Set the command's
mustHaveArgs
tofalse
- Add a command level
validate
callback and add the validation there (that at least one if being used)
Removing the mustHaveArgs
(or setting it to false
) will also address the visual issue where the help was showing runscript --
|
||
// Dev: | ||
// runscript success/competed | ||
ra_runscript_success_done: i18n.translate( |
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.
You should not be using these code for Crowdstrike. This are Elastic Defend specific and IMO should remain as such.
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 did it to reflect the behavior in tests, but I am ok with removing it 👍
}: { | ||
filterName: ActionsLogPopupFilters; | ||
typesFilters?: TypesFilters; | ||
isFlyout: boolean; | ||
onChangeFilterOptions?: (selectedOptions: string[]) => void; | ||
'data-test-subj'?: string; | ||
'data-test-height'?: number; |
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.
can you explain what this property is? I see it used in a few places below and I'm just not getting it. Also - the naming seems strange,since data-
are normally the naming convention use if the property needs to be pass all the way though the HTML element.
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.
This is a workaround to make our tests work with more than 9 elements on the filters list in history log. Apparently EUI component has dynamically loaded elements and without increased height we weren't able to render all 10 elements.
I wouldn't be really worried about this prop, what do you think?
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.
Ahhhhh. Ok - so used only for testing so that we don't have to trigger a "scroll" of the popup's content. Got it. I was initially thinking this was a fix that would be visible in the UI and that concerned me because it would not scale as we add more items.
@@ -63,7 +63,7 @@ describe( | |||
|
|||
// No access to response actions (except `unisolate`) | |||
for (const actionName of RESPONSE_ACTION_API_COMMANDS_NAMES.filter( | |||
(apiName) => apiName !== 'unisolate' | |||
(apiName) => apiName !== 'unisolate' && apiName !== 'runscript' |
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.
Is this temporary? if not, then please update the JSDoc comment above (it mentions only 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.
Yes, this is temporary. This PR doesn't include API changes. The next PR will include it.
@paul-tavares thank's for the review. i was confused about I think I applied all the suggestions, also included an updated screenshot in the description. Also - thanks for the review and help with the strange EUI issue @ashokaditya, appreciate the help 👍 |
💚 Build Succeeded
Metrics [docs]Async chunks
Page load bundle
History
cc @tomsonpl |
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.
Thanks for all the follow ups. Left some more comments, but am 👍 it. You can address it later if needed.
capabilities: endpointCapabilities, | ||
privileges: endpointPrivileges, | ||
}, | ||
exampleUsage: `runscript --Raw=\`\`\`Get-ChildItem .\`\`\` -CommandLine=""`, |
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 still see the use of ` here for the --Raw
value. I'm guessing thats intentional.
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, we want --, I mistakenly omitted the -CommandLine - will adjust
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 sure you understood my comment (although, I did not notice the single -
the CommandLine
so good call).
Look at the value of --Raw
.... Why is it using 3 backtick characters (`) to enclose the value of Get-ChildItem .
?
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.
Right... well, apparently an example I copied over from CS. But indeed it makes no sense, will remove! thx
allowMultiples: false, | ||
about: CROWDSTRIKE_CONSOLE_COMMANDS.runscript.args.hostPath.about, | ||
mustHaveValue: 'non-empty-string', | ||
}, |
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.
Ok. Sorry about this - I do now remember that we discussed that they can use multiples, but at least one of them must be present. In that case, you are correct in that we can't use exclusiveOr
because that limits the options for the user to having to only pick one.
The main issue I saw when testing this is that there was no validations in place. Realizing now that all three could be used, and that at least 1 must be defined, I think the perhaps the command should be setup as:
- Remove
exclusiveOr
from the arguments - Set the command's
mustHaveArgs
tofalse
- Add a command level
validate
callback and add the validation there (that at least one if being used)
Removing the mustHaveArgs
(or setting it to false
) will also address the visual issue where the help was showing runscript --
}: { | ||
filterName: ActionsLogPopupFilters; | ||
typesFilters?: TypesFilters; | ||
isFlyout: boolean; | ||
onChangeFilterOptions?: (selectedOptions: string[]) => void; | ||
'data-test-subj'?: string; | ||
'data-test-height'?: number; |
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.
Ahhhhh. Ok - so used only for testing so that we don't have to trigger a "scroll" of the popup's content. Got it. I was initially thinking this was a fix that would be visible in the UI and that concerned me because it would not scale as we add more items.
Starting backport for target branches: 8.x https://github.com/elastic/kibana/actions/runs/12257730007 |
Merged, the following will be coming in a separate PR:
|
(cherry picked from commit 9b27804)
💚 All backports created successfully
Note: Successful backport PRs will be merged automatically after passing CI. Questions ?Please refer to the Backport tool documentation |
# Backport This will backport the following commits from `main` to `8.x`: - [[EDR Workflows] Add RunScript CS Command - UI (#202012)](#202012) <!--- Backport version: 9.4.3 --> ### Questions ? Please refer to the [Backport tool documentation](https://github.com/sqren/backport) <!--BACKPORT [{"author":{"name":"Tomasz Ciecierski","email":"[email protected]"},"sourceCommit":{"committedDate":"2024-12-10T14:02:12Z","message":"[EDR Workflows] Add RunScript CS Command - UI (#202012)","sha":"9b27804a9b4f90a48f68c1f543270373d7bab6db","branchLabelMapping":{"^v9.0.0$":"main","^v8.18.0$":"8.x","^v(\\d+).(\\d+).\\d+$":"$1.$2"}},"sourcePullRequest":{"labels":["v9.0.0","Team:Defend Workflows","release_note:feature","backport:version","v8.18.0"],"title":"[EDR Workflows] Add RunScript CS Command - UI","number":202012,"url":"https://github.com/elastic/kibana/pull/202012","mergeCommit":{"message":"[EDR Workflows] Add RunScript CS Command - UI (#202012)","sha":"9b27804a9b4f90a48f68c1f543270373d7bab6db"}},"sourceBranch":"main","suggestedTargetBranches":["8.x"],"targetPullRequestStates":[{"branch":"main","label":"v9.0.0","branchLabelMappingKey":"^v9.0.0$","isSourceBranch":true,"state":"MERGED","url":"https://github.com/elastic/kibana/pull/202012","number":202012,"mergeCommit":{"message":"[EDR Workflows] Add RunScript CS Command - UI (#202012)","sha":"9b27804a9b4f90a48f68c1f543270373d7bab6db"}},{"branch":"8.x","label":"v8.18.0","branchLabelMappingKey":"^v8.18.0$","isSourceBranch":false,"state":"NOT_CREATED"}]}] BACKPORT--> Co-authored-by: Tomasz Ciecierski <[email protected]>
Summary
This PR introduces the
runscript
command for CrowdStrike RTR and adds parameter validation to align with the CrowdStrike API. The functionality is currently hidden behind a newcrowdstrikeRunScriptEnabled
feature flag for controlled rollout. Some aspects are temporary and will be refined in future PRs.Key Changes
Added
runscript
Command:runscript
command to allow execution of scripts via CrowdStrike RTR.Added
helpUsage
to Command Definition:helpUsage
field that will overwriteexampleUsage
in--help
command if providedParameter Validation:
Added validation for the following arguments -
choose one
, as defined by the CrowdStrike API:--Raw
--HostPath
--CloudFile
Optional arguments:
--CommandLine
--Timeout
*Temporary return of Null:
runscript
results will be introduced in a separate PR.API Route:
runscript
command will be added in a subsequent PR.Feature Flag:
crowdstrikeRunScriptEnabled
feature flag to ensure incremental adoption and testing.Future Work
runscript
results.runscript
command.Why is this needed?
The
runscript
command is a critical RTR feature that enables script execution on target hosts. Adding this functionality brings Elastic closer to full-featured integration with CrowdStrike RTR, providing greater flexibility and utility for users.