[Security Solution][Endpoint] Add API checks to Endpoint Policy create/update for checking `endpointPolicyProtections` is enabled #163429

paul-tavares · 2023-08-08T16:13:26Z

Summary

Adds checks to both the Policy Create and Policy Update APIs (Fleet API extension points) and turns off all protections if endpointPolicyProtections appFeature is disabled
Adds migration of policies to the Plugin start() that will check if endpointPolicyProtections is disabled and updates all existing policies (if necessary) to disable protections.

Checklist

Unit or functional tests were updated or added to match the most common scenarios

…o policy creation logic

…pi extension (logic not yet implemented)

…assing yet)

…d be disabled (logic in migration not done yet)

…licy

elasticmachine · 2023-08-08T16:13:29Z

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

joeypoon

🙆‍♂️

…ess-policy-protections

gergoabraham

looks great! added some thoughts and questions, please let me know what you think

gergoabraham · 2023-08-10T15:45:44Z

x-pack/plugins/security_solution/common/endpoint/models/policy_config_helpers.ts

+export const setPolicyToEventCollectionOnly = (policy: PolicyConfig): PolicyConfig => {
+  const updatedPolicy = disableProtections(policy);
+
+  set(updatedPolicy, 'windows.antivirus_registration.enabled', false);
+
+  return updatedPolicy;
+};


I'm wondering, if disableProtections() should have disabled windows.antivirus_registration.enabled in the first place. Its original goal was to prepare a policy for 'data collection only', see https://github.com/paul-tavares/kibana/blob/eff88cf671eaa498d2d894784e71ab0c795627cc/x-pack/plugins/security_solution/server/fleet_integration/handlers/create_default_policy.ts#L134

I tested it, just created a 'Data collection only' policy, and it has antivirus registration disabled, so I think I accidentally left it out from disableProtections().

what do you think? if you agree, could you add it there? and then there is no need for this function.

otherwise, I suggest another name for this function, because it does not set a policy to event collection only as it doesn't enable any event collection, just disabled protections and antivirus registration

yeah, I was not sure about that - thus why I created this one to set it explicitly. I am not familiar with what hte requirement might have been for your other PR, but I would check with Caitlyn and if in fact it should have been disabled in the first place, then we can just add it to disableProtections()

can we maybe follow up on this in a subsequent PR?

Re: name
Yeah you are right. The only reason I named it this way because the Antivirus entry is not a protection. I can change it to ensureOnlyEventCollectionIsAllowed()?? what do you think?

thanks for the rename, sounds good!

I've created a follow-up issue for this: https://github.com/elastic/security-team/issues/7323, so we can discuss it during grooming

x-pack/plugins/security_solution/common/endpoint/models/policy_config_helpers.test.ts

gergoabraham · 2023-08-10T16:05:54Z

x-pack/plugins/security_solution/common/endpoint/models/policy_config_helpers.ts

+      const isEnabled = currentValue !== disableValue;
+
+      if (isEnabled) {
+        message = `property [${fullKeyPathForOs}] is set to [${currentValue}]`;


q: do we want to maybe return all protections that are enabled, instead of the first one?

We could, but I was trying to just exit the function as soon as possible rather than to loop through all of them.

gergoabraham · 2023-08-10T16:11:35Z

x-pack/plugins/security_solution/server/endpoint/migrations/turn_off_policy_protections.ts

+    });
+
+    total = totalPolicies;
+    hasMoreData = items.length > 0;


this causes one additional call for packagePolicy.list() returning zero items. to avoid this, total could be used

update: I see you even have mocks prepared in the tests for the additional call. is it maybe intended?

Its just another way to ensure all policies are covered case policies are added as this function is running. I think I might have picked up this approach from code I saw in fleet.

sorry for nitpicking, but a small optimisation for keeping the startup time a bit lower is still an optimisation. I still don't think the extra fetch is needed, because if a new policy added at this point, it will go through the checks that ensure that no protections are enabled.

(what I don't know: is it even possible for policies to be added during Plugin.start()?)

in case it is possible and the checks are not in place when adding: I think the extra fetch still wouldn't help, only maybe for every N-thousand-and-first-some policies, while still possibly skipping the previous N-thousand minus-some policies (e.g. if the 3992...4016th policies were added after fetching the 4th page, 3992...4000th policies are skipped)

///

independently of the extra fetch, sortOrder: 'asc' and sortField: 'createdAt' should be added to the options for packagePolicy.list() as the default is descending by updated_at.

Good point. I'll make the change to ensure we don't make the additional API call

👍

x-pack/plugins/security_solution/server/plugin.ts

x-pack/plugins/security_solution/server/fleet_integration/handlers/create_default_policy.ts

from Gergo

…-for-serverless-policy-protections' into task/olm-7232-api-policy-watcher-for-serverless-policy-protections

gergoabraham

Thanks for the modifications!

I've extended my nitpicking, but other than that, looks great! 💯

…olicy-watcher-for-serverless-policy-protections

kibana-ci · 2023-08-11T13:32:26Z

💛 Build succeeded, but was flaky

Buildkite Build
Commit: abc633e

Failed CI Steps

FTR Configs #51

Test Failures

[job] [logs] FTR Configs #51 / Index Management app Index template wizard Mappings step "before each" hook for "clearing up the Numeric subtype dropdown doesn't break the page"

Metrics [docs]

Unknown metric groups

ESLint disabled in files

id	before	after	diff
`securitySolution`	63	64	+1

Total ESLint disabled count

id	before	after	diff
`securitySolution`	527	528	+1

History

💚 Build #149338 succeeded 8658745
💛 Build #148481 was flaky ba7a4c8
💔 Build #148440 failed d263339

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

cc @paul-tavares

* main: (64 commits) [ML] Transforms: Fix privileges check. (elastic#163687) [Log Explorer] Add test suite for Dataset Selector (elastic#163079) [Security Solution][Endpoint] Add API checks to Endpoint Policy create/update for checking `endpointPolicyProtections` is enabled (elastic#163429) [Security Solution] Fix flaky test: x-pack/test/detection_engine_api_integration/security_and_spaces/update_prebuilt_rules_package/update_prebuilt_rules_package·ts - update_prebuilt_rules_package should allow user to install prebuilt rules from scratch, then install new rules and upgrade existing rules from the new package (elastic#163241) [Security Solution] expandable flyout - replace feature flag with advanced settings toggle (elastic#161614) [DOCS] Adds the release notes for the 8.9.1 release. (elastic#163578) [FTR] Implement browser network condition utils (elastic#163633) [Security Solution] Unskip rules table auto-refresh Cypress tests (elastic#163451) [Security Solution] Re-enable fixed rule snoozing Cypress test (elastic#160037) [Flaky Test elastic#111821] Mock `moment` to avoid midnight TZ issues (elastic#163157) Document interactive setup (elastic#163619) [Lens] Align decoration color with text color for layer actions (elastic#163630) [Lens] Relax counter field checks for saved visualizations with unsupported operations (elastic#163515) [Security Solution][Endpoint] Removes pMap and uses a for loop instead (elastic#163509) [Enterprise Search] Update Workplace Search connectors doclink (elastic#163676) Update APM (main) (elastic#163623) [Serverless] Partially fix lens/maps/visualize breadcrumbs missing title (elastic#163476) [Flaky elastic#118272] Unskip tests (elastic#163319) [APM] Make service group saved objects exportable (elastic#163569) [Observability AI Assistant] Action menu item (elastic#163463) ...

paul-tavares added 20 commits August 8, 2023 12:01

changes for default policy

eb66569

add appFeatures to endpoint server side services and pass it down t…

6b199d2

…o policy creation logic

add disableAllPolicyProtections() and use it in policy create

cb8b985

refactor: use existing helpers to turn off protections

de9151c

add test for setPolicyToEventCollectionOnly()

b397446

fix test

ae96184

add call to setPolicyToEventCollectionOnly() to the policy update a…

b6f6e62

…pi extension (logic not yet implemented)

implementation of isPolicySetToEventCollectionOnly() + tests (not p…

d1e50cb

…assing yet)

Fix logic for isPolicySetToEventCollectionOnly() + add tests

96996b6

use AppFeatureSecurityKey enum

aeeaa16

utility to create internal SO client

3eabe35

Add migration step to the Plugin that will check if protections shoul…

f0cbc1c

…d be disabled (logic in migration not done yet)

Logic to update policies that have protections enabled

5867b74

Fix endpoint context mock + tests for fleet integrations

26700a1

Centralized mock for AppFeatures

e11b7ef

Tests for createDefaultPolicy() and use of appFeatures to adjust po…

117bc6b

…licy

Tests for Update integration policy fleet api extension

784764d

Added utility type: PromiseResolvedValue<>

213d13e

initial test for turnOffPolicyProtections()

9c7b859

tests for turnOffPolicyProtections()

d263339

paul-tavares added release_note:skip Skip the PR/issue when compiling release notes Team:Defend Workflows “EDR Workflows” sub-team of Security Solution v8.10.0 labels Aug 8, 2023

paul-tavares self-assigned this Aug 8, 2023

paul-tavares requested review from a team as code owners August 8, 2023 16:13

paul-tavares requested review from joeypoon and gergoabraham August 8, 2023 16:13

fix AppFeatures mock

ba7a4c8

joeypoon approved these changes Aug 9, 2023

View reviewed changes

paul-tavares added 2 commits August 10, 2023 09:44

Merge branch 'main' into task/olm-7232-api-policy-watcher-for-serverl…

717054b

…ess-policy-protections

Merge branch 'main' into task/olm-7232-api-policy-watcher-for-serverl…

eff88cf

…ess-policy-protections

gergoabraham reviewed Aug 10, 2023

View reviewed changes

paul-tavares added 2 commits August 10, 2023 12:56

code review updates

8fa8ec4

from Gergo

Merge remote-tracking branch 'origin/task/olm-7232-api-policy-watcher…

8658745

…-for-serverless-policy-protections' into task/olm-7232-api-policy-watcher-for-serverless-policy-protections

paul-tavares requested a review from gergoabraham August 10, 2023 16:56

gergoabraham approved these changes Aug 11, 2023

View reviewed changes

paul-tavares added 2 commits August 11, 2023 08:25

Adjust looping logic for adjust policies

f08dbb6

Merge remote-tracking branch 'upstream/main' into task/olm-7232-api-p…

abc633e

…olicy-watcher-for-serverless-policy-protections

paul-tavares enabled auto-merge (squash) August 11, 2023 12:26

paul-tavares merged commit 27c394c into elastic:main Aug 11, 2023

kibanamachine added the backport:skip This commit does not require backporting label Aug 11, 2023

paul-tavares deleted the task/olm-7232-api-policy-watcher-for-serverless-policy-protections branch August 11, 2023 13:33

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Security Solution][Endpoint] Add API checks to Endpoint Policy create/update for checking `endpointPolicyProtections` is enabled #163429

[Security Solution][Endpoint] Add API checks to Endpoint Policy create/update for checking `endpointPolicyProtections` is enabled #163429

paul-tavares commented Aug 8, 2023

elasticmachine commented Aug 8, 2023

joeypoon left a comment

gergoabraham left a comment

gergoabraham Aug 10, 2023

paul-tavares Aug 10, 2023

gergoabraham Aug 11, 2023

gergoabraham Aug 10, 2023

paul-tavares Aug 10, 2023

gergoabraham Aug 10, 2023

paul-tavares Aug 10, 2023

gergoabraham Aug 11, 2023

paul-tavares Aug 11, 2023

gergoabraham left a comment

kibana-ci commented Aug 11, 2023

ESLint disabled in files

Total ESLint disabled count

[Security Solution][Endpoint] Add API checks to Endpoint Policy create/update for checking endpointPolicyProtections is enabled #163429

[Security Solution][Endpoint] Add API checks to Endpoint Policy create/update for checking endpointPolicyProtections is enabled #163429

Conversation

paul-tavares commented Aug 8, 2023

Summary

Checklist

elasticmachine commented Aug 8, 2023

joeypoon left a comment

Choose a reason for hiding this comment

gergoabraham 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

gergoabraham left a comment

Choose a reason for hiding this comment

kibana-ci commented Aug 11, 2023

💛 Build succeeded, but was flaky

Failed CI Steps

Test Failures

Metrics [docs]

ESLint disabled in files

Total ESLint disabled count

History

[Security Solution][Endpoint] Add API checks to Endpoint Policy create/update for checking `endpointPolicyProtections` is enabled #163429

[Security Solution][Endpoint] Add API checks to Endpoint Policy create/update for checking `endpointPolicyProtections` is enabled #163429