[Defend workflows] Validate Isolate RBAC on rule creation/update #157401

tomsonpl · 2023-05-11T14:59:49Z

This PR adds a validation on rule creation/update checking if user has permissions to change response_actions.

AC:

throws 401 error when user tries to add/change/delete any of the endpoint response_actions without specific permission per command, eg. if the isolate command is changed, we validate for canIsolateHost
validation happens only for queryTypeRule
validation happens only if endpointResponseActionsEnabled flag is set to True
validation happens only if either existing rule or the update rule contains response_actions
should be ready to validate more commands than just the current isolate
throws error when there is a different than isolate command provided (schema requirement)
basic unit tests provided

Rules Team: would you consider adding this validation to rule create/update flow? I was trying to make sure that this is as least problematic as possible, but would love to use a better approach if you have any suggestions.

Thanks!

# Conflicts: # x-pack/plugins/security_solution/public/management/components/endpoint_responder/lib/console_commands_definition.ts

elasticmachine · 2023-05-12T08:18:52Z

Pinging @elastic/security-defend-workflows (Team:Defend Workflows)

paul-tavares · 2023-05-12T15:07:09Z

x-pack/plugins/security_solution/common/endpoint/utils/commands.ts

+import { RESPONSE_CONSOLE_ACTION_COMMANDS_TO_REQUIRED_AUTHZ } from '../service/response_actions/constants';
+import type { EndpointPrivileges } from '../types';
+
+export const getRbacControl = ({


This is a UI specific construct. EndpointPrivileges has some additional ui specific property (like loading). On the server side we use EndpointAuthz.

I would not recommend trying to make this reusable with the server side.

paul-tavares · 2023-05-12T15:08:57Z

x-pack/plugins/security_solution/common/endpoint/utils/commands.ts

+ * unisolate -> release
+ * running-processes -> processes
+ */
+export const getUiCommand = (


We have a const that does this mapping here:

kibana/x-pack/plugins/security_solution/common/endpoint/service/response_actions/constants.ts

Lines 87 to 98 in f9f4c1a

export const RESPONSE_ACTION_API_COMMANDS_TO_CONSOLE_COMMAND_MAP = Object.freeze<

Record<ResponseActionsApiCommandNames, ConsoleResponseActionCommands>

>({

isolate: 'isolate',

unisolate: 'release',

execute: 'execute',

'get-file': 'get-file',

'running-processes': 'processes',

'kill-process': 'kill-process',

'suspend-process': 'suspend-process',

upload: 'upload',

});

That being said, and following from my earlier comment, I don't think you need this for the server side.

paul-tavares · 2023-05-12T15:10:18Z

x-pack/plugins/security_solution/server/lib/detection_engine/rule_management/utils/validate.ts

+  securitySolution: SecuritySolutionApiRequestHandlerContext,
+  ruleUpdate: RuleCreateProps | RuleUpdateProps,
+  existingRule?: RuleAlertType | null
+) => {


can you add a return type to this function.

paul-tavares · 2023-05-12T15:18:23Z

x-pack/plugins/security_solution/server/lib/detection_engine/rule_management/utils/validate.ts

+    );
+
+    difference.forEach((action) => {
+      if ('command' in action?.params) {


I don't see the API restricting this to only isolate. Did you forget to make that check here?

I know you are trying to build it in a way that it will just work with other aciton as they are introduced, but the API needs to be locked down to only the actions we support currently.

paul-tavares · 2023-05-12T15:25:34Z

x-pack/plugins/security_solution/server/lib/detection_engine/rule_management/utils/validate.ts

+        const isInvalid = !getRbacControl({
+          commandName: getUiCommand(action.params.command),
+          privileges: { ...endpointAuthz, loading: false },
+        });
+


I would suggest not using getRbackControl() since that is working with a UI specific construct as I mentioned earlier.

If you keep this generic as is now, then I would suggest something like:

const authzPropName = RESPONSE_CONSOLE_ACTION_COMMANDS_TO_REQUIRED_AUTHZ[ RESPONSE_ACTION_API_COMMANDS_TO_CONSOLE_COMMAND_MAP[action.params.command] ]; const isInvalid = endpiontAuthz[authzPropName];

You could also create a const that includes the mapping of RESPONSE_ACTION_API_COMMANDS_TO_REQUIRED_AUTHZ if that is clearer to understand.

(FYI: @ashokaditya and I have talked about refactoring all of const (there are many for actions) into a nested object that can be use by both the server and UI)

👍 Great hint, thank you!

paul-tavares · 2023-05-12T15:31:41Z

x-pack/plugins/security_solution/common/endpoint/utils/commands.ts

@@ -0,0 +1,57 @@
+/*


if you endup keeping these (see other comments below) I think they should go into common/endpoint/service/response_actions instead, since they are all actions specific

paul-tavares · 2023-05-12T15:32:41Z

x-pack/plugins/security_solution/common/endpoint/utils/commands.ts

+  if (command === 'unisolate') {
+    return 'release';
+  } else if (command === 'running-processes') {
+    return 'processes';
+  } else {
+    return command;
+  }
+};


we have a const that holds this mapping

tomsonpl · 2023-05-12T17:44:57Z

Thank you @paul-tavares, indeed the constants that you created recently made all my changes to management redundant 🚀 thanks!
I also added the schema restriction for isolate command as suggested :)

paul-tavares

Looks good from an Endpoint management standpoint.

maximpn

@tomsonpl there are nice changes to validate permissions 👍

Rules Team: would you consider adding this validation to rule create/update flow? I was trying to make sure that this is as least problematic as possible, but would love to use a better approach if you have any suggestions.

It'd be nice to have this logic somewhere in validate section, so outside the route's business logic. Unfortunately it's impossible without refactoring and this definitely goes outside PR's scope. Having validateResponseActionsPermissions() next to the other validate functions is a good choice.

I left some comments, mostly related to the clarity of the changes.

maximpn · 2023-05-15T13:06:39Z

x-pack/plugins/security_solution/server/lib/detection_engine/rule_management/utils/validate.ts

+  if (payload.response_actions?.length || existingPayload?.params?.responseActions?.length) {
+    const endpointAuthz = await securitySolution.getEndpointAuthz();
+
+    const difference = findDifferenceInArrays<ResponseAction, RuleResponseAction>(


What's the point to define a wrapper function while it can be handled here right in place by

const differences = _.differenceWith<ResponseAction, RuleResponseAction>( payload.response_actions, existingPayload?.params?.responseActions ?? [], _.isEqual );

nit: the result is rather differences than difference 😅

Well, I had to support empty arrays, because it was not finding it as differences. However now the tests are still passing when I removed those So... I will have to give it another try and test it. :) Thanks!

Yes I tested and it doesn't find differences correctly when we eg. had 1 response action in existingRule, and remove it to end up with empty array. I added a test to validate that.

Good point! I've checked the implementation in lodash and it seems considering comparing to an empty array as there are no differences. Looks like a bug IMHO.

maximpn · 2023-05-15T13:14:39Z

x-pack/plugins/security_solution/server/lib/detection_engine/rule_management/utils/validate.ts

+  const payload = ruleUpdate as QueryRule;
+  const existingPayload = existingRule as Rule<UnifiedQueryRuleParams>;
+
+  if (payload.response_actions?.length || existingPayload?.params?.responseActions?.length) {


It's a good candidate for a type guard as ruleUpdate as QueryRule makes it harder to understand what's going on below as payload.response_actions isn't optional in the TS type.

Something like the following type guard should work for QueryRule

function isQueryRulePayload(rule: SharedResponseProps): rule is QueryRule { return 'response_actions' in rule; }

And the same is applicable to existingRule. So the result code could be

if (!isQueryRulePayload(payload) && (!existingPayload || !isQueryRuleObject(existingPayload)) { return; } if (payload.response_actions.length === 0 && existingPayload.params?.responseActions.length === 0) { return; }

maximpn · 2023-05-15T14:01:11Z

x-pack/plugins/security_solution/server/lib/detection_engine/rule_management/utils/validate.ts

+    );
+
+    difference.forEach((action) => {
+      if ('command' in action?.params) {


nit: early exit would be nice here as well

maximpn · 2023-05-15T14:07:51Z

...ity_solution/server/lib/detection_engine/rule_management/api/rules/update_rule/route.test.ts

@@ -180,4 +183,69 @@ describe('Update rule route', () => {
      expect(result.badRequest).toHaveBeenCalledWith('Failed to parse "from" on rule param');
    });
  });
+  describe('rule containing response actions', () => {
+    beforeEach(() => {
+      // @ts-expect-error


A comment here why @ts-expect-error is needed would be nice.

maximpn · 2023-05-15T14:09:59Z

...ity_solution/server/lib/detection_engine/rule_management/api/rules/create_rule/route.test.ts

@@ -176,4 +176,69 @@ describe('Create rule route', () => {
      expect(result.badRequest).toHaveBeenCalledWith('Failed to parse "from" on rule param');
    });
  });
+  describe('rule containing response actions', () => {
+    beforeEach(() => {
+      // @ts-expect-error


A comment here why @ts-expect-error is needed would be nice.

maximpn · 2023-05-15T14:17:25Z

.../plugins/security_solution/common/detection_engine/rule_response_actions/schemas/endpoint.ts


 // to enable using RESPONSE_ACTION_API_COMMANDS_NAMES as a type
 function keyObject<T extends readonly string[]>(arr: T): { [K in T[number]]: null } {
  return Object.fromEntries(arr.map((v) => [v, null])) as never;
 }

 export const EndpointParams = t.type({
-  command: t.keyof(keyObject(RESPONSE_ACTION_API_COMMANDS_NAMES)),
+  command: t.keyof(keyObject(ENABLED_AUTOMATED_RESPONSE_ACTION_COMMANDS)),


Could you clarify what's impact of this change? As far as I see it doesn't affect create/update rule endpoints or I just got lost in the files 😅

Hey, that's a great question. What we agreed on within the team is that we want to support just isolate command so far. This change, makes sure we get a schema error when user wants to force using another command. I am saying force, because in the UI it's limited to just that one command.

…ndpoint-rule-validation

tomsonpl · 2023-05-15T18:57:44Z

Thanks @maximpn, appreciate the feedback 👍 I've applied your comments, could you take another look please? :)

ashokaditya · 2023-05-16T09:08:48Z

x-pack/plugins/security_solution/server/lib/detection_engine/rule_management/utils/utils.ts

@@ -384,3 +385,17 @@ export const convertAlertSuppressionToSnake = (
        missing_fields_strategy: input.missingFieldsStrategy,
      }
    : undefined;
+
+export const findDifferenceInArrays = <T1, T2>(


Looks like you're finding elements that are not common in either array. If yes, then consider renaming this to findSymmetricDifferenceInArrays

#157966 changed the approach in here, hope you like this one better @ashokaditya :)

maximpn

@tomsonpl thank you for addressing all my comments 👍 There is one minor change left. Anyway it's not critical so approving in advance.

maximpn · 2023-05-16T10:50:45Z

x-pack/plugins/security_solution/server/lib/detection_engine/rule_management/utils/validate.ts

+    return;
+  }
+
+  const payload = ruleUpdate as QueryRule;


Type casting as QueryRule and as Rule<UnifiedQueryRuleParams isn't necessary anymore as type guards are used.

Fixed as discussed offline, thank you!

…ndpoint-rule-validation

kibana-ci · 2023-05-16T13:10:58Z

💚 Build Succeeded

Buildkite Build
Commit: 39bd49c

Metrics [docs]

Unknown metric groups

ESLint disabled line counts

id	before	after	diff
`enterpriseSearch`	19	21	+2
`securitySolution`	400	404	+4
total			+6

Total ESLint disabled count

id	before	after	diff
`enterpriseSearch`	20	22	+2
`securitySolution`	480	484	+4
total			+6

History

💚 Build #128089 succeeded 35bd581
💚 Build #128011 succeeded ac2db58
💚 Build #127614 succeeded 6b30126
💔 Build #127542 failed e846281
💔 Build #127523 failed 4953ea1
💚 Build #127400 succeeded 5b5b0d5

To update your PR or re-run it, just comment with:
@elasticmachine merge upstream

cc @tomsonpl

…7401)

tomsonpl added 2 commits May 11, 2023 15:23

initial version for only isolate

b34a5d0

fix types for validation isolate

e15a698

tomsonpl added release_note:skip Skip the PR/issue when compiling release notes Team:Defend Workflows “EDR Workflows” sub-team of Security Solution v8.9.0 labels May 11, 2023

tomsonpl self-assigned this May 11, 2023

tomsonpl added 4 commits May 11, 2023 21:21

make validation more generic

18006f6

Merge branch 'main' into endpoint-rule-validation

1f9bc65

# Conflicts: # x-pack/plugins/security_solution/public/management/components/endpoint_responder/lib/console_commands_definition.ts

fix after merge

e70bf8c

add feature flag check and move isQueryRule check

de6facb

tomsonpl marked this pull request as ready for review May 12, 2023 08:18

tomsonpl requested review from a team as code owners May 12, 2023 08:18

tomsonpl requested review from pzl and parkiino May 12, 2023 08:18

tomsonpl requested a review from maximpn May 12, 2023 08:18

tomsonpl added 2 commits May 12, 2023 12:02

remove redundant console loge

fd13f6b

add tests

e9be499

tomsonpl requested a review from a team as a code owner May 12, 2023 11:26

tomsonpl requested review from paul-tavares and removed request for parkiino May 12, 2023 11:32

change flag just for 2 tests

92a4943

tomsonpl removed the request for review from a team May 12, 2023 12:02

test response error text

5b5b0d5

paul-tavares reviewed May 12, 2023

View reviewed changes

tomsonpl added 2 commits May 12, 2023 19:11

revert changes in management and dont use rbacControl in rule

7fb1f6c

change validation login and test + change schema to accept only isolate

aa4dec8

tomsonpl requested a review from a team as a code owner May 12, 2023 17:38

tomsonpl requested a review from paul-tavares May 12, 2023 17:39

remove helper function

4953ea1

tomsonpl removed the request for review from a team May 12, 2023 18:29

paul-tavares approved these changes May 12, 2023

View reviewed changes

tomsonpl added 2 commits May 12, 2023 21:25

Merge branch 'main' into endpoint-rule-validation

e846281

Merge branch 'main' into endpoint-rule-validation

6b30126

maximpn reviewed May 15, 2023

View reviewed changes

tomsonpl added 2 commits May 15, 2023 20:56

apply comments

7de02c3

Merge remote-tracking branch 'origin/endpoint-rule-validation' into e…

d02a804

…ndpoint-rule-validation

revert change to check arr empty and add test to check this

ac2db58

tomsonpl requested a review from maximpn May 16, 2023 06:24

tomsonpl and others added 2 commits May 16, 2023 09:58

change 401 to 403 response error

a64c396

Merge branch 'main' into endpoint-rule-validation

35bd581

ashokaditya approved these changes May 16, 2023

View reviewed changes

tomsonpl removed the request for review from pzl May 16, 2023 09:22

maximpn approved these changes May 16, 2023

View reviewed changes

tomsonpl added 2 commits May 16, 2023 13:23

remove casting and improve typeguard + logic

79bdab3

Merge remote-tracking branch 'origin/endpoint-rule-validation' into e…

39bd49c

…ndpoint-rule-validation

tomsonpl merged commit e607283 into elastic:main May 16, 2023

kibanamachine added the backport:skip This commit does not require backporting label May 16, 2023

jasonrhodes pushed a commit that referenced this pull request May 17, 2023

[Defend workflows] Validate Isolate RBAC on rule creation/update (#15…

430219a

…7401)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Defend workflows] Validate Isolate RBAC on rule creation/update #157401

[Defend workflows] Validate Isolate RBAC on rule creation/update #157401

tomsonpl commented May 11, 2023 •

edited

Loading

elasticmachine commented May 12, 2023

paul-tavares May 12, 2023

paul-tavares May 12, 2023

paul-tavares May 12, 2023

paul-tavares May 12, 2023

paul-tavares May 12, 2023

tomsonpl May 12, 2023

paul-tavares May 12, 2023

paul-tavares May 12, 2023

tomsonpl commented May 12, 2023

paul-tavares left a comment

maximpn left a comment

maximpn May 15, 2023

tomsonpl May 15, 2023

tomsonpl May 15, 2023

maximpn May 16, 2023

maximpn May 15, 2023

maximpn May 15, 2023

maximpn May 15, 2023

maximpn May 15, 2023

maximpn May 15, 2023

tomsonpl May 15, 2023

tomsonpl commented May 15, 2023

ashokaditya May 16, 2023

tomsonpl May 17, 2023

maximpn left a comment

maximpn May 16, 2023 •

edited

Loading

tomsonpl May 16, 2023

kibana-ci commented May 16, 2023

ESLint disabled line counts

Total ESLint disabled count

	export const RESPONSE_ACTION_API_COMMANDS_TO_CONSOLE_COMMAND_MAP = Object.freeze<
	Record<ResponseActionsApiCommandNames, ConsoleResponseActionCommands>
	>({
	isolate: 'isolate',
	unisolate: 'release',
	execute: 'execute',
	'get-file': 'get-file',
	'running-processes': 'processes',
	'kill-process': 'kill-process',
	'suspend-process': 'suspend-process',
	upload: 'upload',
	});

[Defend workflows] Validate Isolate RBAC on rule creation/update #157401

[Defend workflows] Validate Isolate RBAC on rule creation/update #157401

Conversation

tomsonpl commented May 11, 2023 • edited Loading

elasticmachine commented May 12, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tomsonpl commented May 12, 2023

paul-tavares left a comment

Choose a reason for hiding this comment

maximpn left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tomsonpl commented May 15, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

maximpn left a comment

Choose a reason for hiding this comment

maximpn May 16, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kibana-ci commented May 16, 2023

💚 Build Succeeded

Metrics [docs]

ESLint disabled line counts

Total ESLint disabled count

History

tomsonpl commented May 11, 2023 •

edited

Loading

maximpn May 16, 2023 •

edited

Loading