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

[EDR Workflows] Add RunScript CS Command - UI #202012

Merged
merged 46 commits into from
Dec 10, 2024

Conversation

tomsonpl
Copy link
Contributor

@tomsonpl tomsonpl commented Nov 27, 2024

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 new crowdstrikeRunScriptEnabled feature flag for controlled rollout. Some aspects are temporary and will be refined in future PRs.

Zrzut ekranu 2024-12-10 o 11 46 16

Key Changes

  • Added runscript Command:

    • Implements the runscript command to allow execution of scripts via CrowdStrike RTR.
  • Added helpUsage to Command Definition:

    • Introduced an optional helpUsage field that will overwrite exampleUsage in --help command if provided
  • Parameter 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:

    • A dedicated component for runscript results will be introduced in a separate PR.
  • API Route:

    • The API route for executing the runscript command will be added in a subsequent PR.
  • Feature Flag:

    • Hidden behind the new crowdstrikeRunScriptEnabled feature flag to ensure incremental adoption and testing.

Future Work

  • Replace null with a dedicated component for displaying runscript results.
  • Add API route for executing the runscript command.
  • Expand support for additional RTR commands and enhance error handling.

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.

@tomsonpl tomsonpl self-assigned this Nov 27, 2024
@tomsonpl tomsonpl added Team:Defend Workflows “EDR Workflows” sub-team of Security Solution release_note:feature Makes this part of the condensed release notes v8.18.0 labels Nov 27, 2024
@tomsonpl
Copy link
Contributor Author

/ci

@tomsonpl
Copy link
Contributor Author

/ci

@tomsonpl
Copy link
Contributor Author

/ci

@tomsonpl
Copy link
Contributor Author

/ci

@tomsonpl
Copy link
Contributor Author

/ci

@tomsonpl
Copy link
Contributor Author

/ci

@tomsonpl tomsonpl added the backport:version Backport to applied version labels label Nov 28, 2024
@tomsonpl
Copy link
Contributor Author

/ci

@tomsonpl
Copy link
Contributor Author

/ci

@tomsonpl
Copy link
Contributor Author

/ci

@tomsonpl
Copy link
Contributor Author

/ci

1 similar comment
@tomsonpl
Copy link
Contributor Author

/ci

@tomsonpl
Copy link
Contributor Author

/ci

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.

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',
Copy link
Member

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()

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 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',
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
defaultMessage: 'Absolute or relative path of script on host machine',
defaultMessage: 'Absolute or relative path of script file on host machine',

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 was taken 1:1 from CrowdStrike console - although I am ok with adjusting if you prefer

Comment on lines 542 to 549
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`,
Copy link
Member

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.
Screenshot 2024-12-09 at 13 52 12

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.

Copy link
Contributor Author

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',
Copy link
Member

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?

Suggested change
defaultMessage: 'Script name in cloud storage',
defaultMessage: 'Script file name in cloud storage',

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. 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',
Copy link
Member

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?

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 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',
Copy link
Member

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,
Copy link
Member

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?

Comment on lines 1548 to 1550
// 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);
Copy link
Member

@ashokaditya ashokaditya Dec 9, 2024

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 😓

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 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? :)

Copy link
Contributor Author

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!

Copy link
Member

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Applied, thanks a lot 👍

@tomsonpl
Copy link
Contributor Author

tomsonpl commented Dec 9, 2024

/ci

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.

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 });
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 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.

Copy link
Contributor

@paul-tavares paul-tavares left a 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:

  1. the help command output does not show the runscript 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) image



  2. There are argument validations in place. I just entered runscript --comment and it was accepted image

@@ -33,6 +33,13 @@ export const CommandInputUsage = memo<Pick<CommandUsageProps, 'commandDef'>>(({
});
}, [commandDef]);

const helpExample = useMemo(() => {
Copy link
Contributor

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?

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, that was the reason 👍

privileges: endpointPrivileges,
},
exampleUsage: `runscript -Raw=\`\`\`Get-ChildItem .\`\`\` -CommandLine=""`,
helpUsage: `
Copy link
Contributor

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=""`,
Copy link
Contributor

Choose a reason for hiding this comment

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

This does not seem correct.

  1. you seem to be using single - instead of double --
  2. Why is the example using back-tick (`) ? The helpUsage below shows the examples enclosed in double quotes image

Copy link
Contributor Author

Choose a reason for hiding this comment

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

  1. Changing - into -- thanks!
  2. 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',
Copy link
Contributor

Choose a reason for hiding this comment

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

This setting's value (non-empty-string) correct ? I thought timeout was a number.

image

allowMultiples: false,
about: CROWDSTRIKE_CONSOLE_COMMANDS.runscript.args.hostPath.about,
mustHaveValue: 'non-empty-string',
},
Copy link
Contributor

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?

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 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.

Copy link
Contributor

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 to false
  • 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(
Copy link
Contributor

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.

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 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;
Copy link
Contributor

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.

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 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?

Copy link
Contributor

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'
Copy link
Contributor

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)

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 temporary. This PR doesn't include API changes. The next PR will include it.

@tomsonpl
Copy link
Contributor Author

@paul-tavares thank's for the review. i was confused about exclusiveOr because it lacked the description, but in the end it's a great out of the box functionality that is perfect for our usage here 👍

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 👍

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

Metrics [docs]

Async chunks

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

id before after diff
securitySolution 14.6MB 14.7MB +10.4KB

Page load bundle

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

id before after diff
securitySolution 87.8KB 87.9KB +31.0B

History

cc @tomsonpl

Copy link
Contributor

@paul-tavares paul-tavares left a 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=""`,
Copy link
Contributor

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.

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, we want --, I mistakenly omitted the -CommandLine - will adjust

Copy link
Contributor

@paul-tavares paul-tavares Dec 10, 2024

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 .?

Copy link
Contributor Author

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',
},
Copy link
Contributor

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 to false
  • 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;
Copy link
Contributor

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.

@tomsonpl tomsonpl merged commit 9b27804 into elastic:main Dec 10, 2024
8 checks passed
@kibanamachine
Copy link
Contributor

Starting backport for target branches: 8.x

https://github.com/elastic/kibana/actions/runs/12257730007

@tomsonpl
Copy link
Contributor Author

tomsonpl commented Dec 10, 2024

Merged, the following will be coming in a separate PR:

  • fix triple backticks in the example
  • change -CommandLine single dash into --
  • think whether it's needed to provide all 3 arguments at a time - maybe it's just quite loose validation on CS that we don't really need to follow.

kibanamachine pushed a commit to kibanamachine/kibana that referenced this pull request Dec 10, 2024
@kibanamachine
Copy link
Contributor

💚 All backports created successfully

Status Branch Result
8.x

Note: Successful backport PRs will be merged automatically after passing CI.

Questions ?

Please refer to the Backport tool documentation

kibanamachine added a commit that referenced this pull request Dec 10, 2024
# 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]>
CAWilson94 pushed a commit to CAWilson94/kibana that referenced this pull request Dec 12, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport:version Backport to applied version labels release_note:feature Makes this part of the condensed release notes Team:Defend Workflows “EDR Workflows” sub-team of Security Solution v8.18.0 v9.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants