[SECURITY_SOLUTION] 145: Advanced Policy UI by jmiller263 · Pull Request #80390 · elastic/kibana

jmiller263 · 2020-10-13T18:00:11Z

Summary

This PR adds a UI for Advanced Policy:

Checklist

Delete any items that are not applicable to this PR.

Any text added follows EUI's writing guidelines, uses sentence case text and includes i18n support
Documentation was added for features that require explanation or tutorials
Unit or functional tests were updated or added to match the most common scenarios
Any UI touched in this PR is usable by keyboard only (learn more about keyboard accessibility)
Any UI touched in this PR does not create any new axe failures (run axe in browser: FF, Chrome)
This renders correctly on smaller devices using a responsive layout. (You can test this in your browser)
This was checked for cross-browser compatibility

For maintainers

This was checked for breaking API changes and was labeled appropriately

…olicy

…ps://github.com/paul-tavares/kibana into task/145_advanced_policy_poc

…_policy_poc

...ck/plugins/security_solution/public/management/pages/policy/models/advanced_policy_schema.ts

kevinlog · 2020-10-22T15:45:48Z

x-pack/plugins/security_solution/common/endpoint/types/index.ts

 */
 export interface PolicyConfig {
  windows: {
-    advanced?: AdvancedFields; // will be introduced in 7.11+


For the type failure - you can actually keep this, but have it accept any object since the advanced section can dynamically accept a set of integrations.

Something like this should work:

... windows { advanced?: {}; ... } ...

@paul-tavares @parkiino let me know your thoughts

yeah that makes sense to me

kevinlog · 2020-10-22T15:45:59Z

x-pack/plugins/security_solution/common/endpoint/types/index.ts

  };
 }

-export interface AdvancedFields {


This can remain deleted

kevinlog · 2020-10-22T15:46:12Z

x-pack/plugins/security_solution/common/endpoint/types/index.ts

  /**
   * Windows-specific policy configuration that is supported via the UI
   */
-  windows: Pick<PolicyConfig['windows'], 'events' | 'malware' | 'popup' | 'advanced'>;


and then add these sections back in

kevinlog · 2020-10-22T15:47:38Z

@jmiller263 - apologies, I miscommunicated on deleting the AdvancedFields type. You can still delete that type, but the Policy option can still exist in the overall config.

See my comments here: 8bd1ef6

paul-tavares

@jmiller263 awsome job on this. I provided some feedback through the code and have the following additional comments:

For the Advanced section items - will the "description" be shown once we know what that is? if so, where? (I assume you will use the "helpText" from the FormRow, just checking)
For the layout design I wonder if using the "Described Form Groups" would work best - it would display the option label + description to the left and the input field on the right. ref: https://elastic.github.io/eui/#/forms/form-layouts#described-form-groups
I think this one might be the more critical one - once I add an advanced option value and save it, the actual advanced section and associated leafKey will never be removed from the policy, even when the user comes later and remove the value from the input field. I guess the question really is: what does the endpoint do in this case?
My quick test was:
1. pull up a policy for edit - ensure that the advanced section does not exist in the API data.
2. in the policy UI, add value to one of the fields and save. This created the advanced section in the policy API data and sets the value (all good)
3. bring up the policy again for edit, and remove the value from the option (no other option had values) and save. This does set the value to an empty string, but the advanced section along with the leafKey remains there.

Also,
Can we write up some tests for the work done on this PR? (can be a separate issue for tracking)

x-pack/plugins/security_solution/common/endpoint/types/index.ts

...ck/plugins/security_solution/public/management/pages/policy/models/advanced_policy_schema.ts

x-pack/plugins/security_solution/public/management/pages/policy/models/policy_details_config.ts

paul-tavares · 2020-10-22T18:18:54Z

x-pack/plugins/security_solution/public/management/pages/policy/view/policy_advanced.tsx

+        if (policyDetailsConfig) {
+          const newPayload = cloneDeep(policyDetailsConfig);
+          setValue(
+            (newPayload as unknown) as Record<string, unknown>,


We should open a tech debt issue to see if we can remove all of these type castings, as they essentially turn off the benefits of the type system

paul-tavares · 2020-10-22T18:21:36Z

x-pack/plugins/security_solution/public/management/pages/policy/view/policy_advanced.tsx

+  newPolicyConfig[path[path.length - 1]] = value;
+}
+
+function getValue(obj: Record<string, unknown>, path: string[]) {


you shoudl add a return type to this function (string right?)

yes for the return in L37 it'll be a string. is it still ok to return undefined in L34 if the return type is string?

you can add both types to the function's return signature:

Suggested change

function getValue(obj: Record<string, unknown>, path: string[]) {

function getValue(obj: Record<string, unknown>, path: string[]): string | undefined {

actually since currentPolicyConfig is type Record<string, unknown>, L37 is returning type unknown; should i be casting to a string? how does the return type help here?

@paul-tavares

paul-tavares · 2020-10-22T18:22:15Z

x-pack/plugins/security_solution/public/management/pages/policy/view/policy_advanced.tsx

+            </EuiText>
+          }
+        >
+          <EuiFieldText fullWidth value={value as string} onChange={onChange} />


I think this casting can be avoiced if you change value above to:

const value = policyDetailsConfig ? getValue((policyDetailsConfig as unknown) as Record<string, unknown>, configPath) : '';

x-pack/plugins/security_solution/public/management/pages/policy/view/policy_details.tsx

x-pack/plugins/security_solution/public/management/pages/policy/view/policy_advanced.tsx

paul-tavares

Awesome job 🤩

👍 🚢 🚀

paul-tavares · 2020-10-27T12:39:08Z

x-pack/plugins/security_solution/public/management/pages/policy/view/policy_advanced.tsx

+  newPolicyConfig[path[path.length - 1]] = value;
+}
+
+function getValue(obj: Record<string, unknown>, path: string[]) {


you can add both types to the function's return signature:

Suggested change

function getValue(obj: Record<string, unknown>, path: string[]) {

function getValue(obj: Record<string, unknown>, path: string[]): string | undefined {

…_policy_poc

parkiino

lgtm 👍

…_policy_poc

kibanamachine · 2020-10-28T15:59:33Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request
Commit: a55447a

Metrics [docs]

@kbn/optimizer bundle module count

id	before	after	diff
`securitySolution`	2063	2065	+2

async chunks size

id	before	after	diff
`securitySolution`	8.1MB	8.1MB	+5.6KB

History

💔 Build #84106 failed 946dbc1
💚 Build #83473 succeeded c7054c6
💚 Build #83406 succeeded 37cc6a4
💚 Build #83391 succeeded 06c3b4d
💔 Build #83363 failed 8bd1ef6

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

ferullo

Options LGTM. Endpoint will have to full audit them again and add/remove things before this ships, as we'll always have to do.

ferullo · 2020-10-28T16:09:12Z

...ck/plugins/security_solution/public/management/pages/policy/models/advanced_policy_schema.ts

+    key: 'mac.advanced.kernel.harden',
+    first_supported_version: '7.11',
+    documentation: '',
+  },


@bjmcnic @willyu-elastic do we want this (mac.advanced.kernel.harden) to remain the advanced option or should we make it something like mac.advanced.harden.enabled so we can add other options nested under harden and it's not conceptually nested under "kernel"?

I would prefer to move it to mac.advanced.harden.enabled

ferullo · 2020-10-28T16:09:38Z

...ck/plugins/security_solution/public/management/pages/policy/models/advanced_policy_schema.ts

+    documentation: '',
+  },
+  {
+    key: 'windows.advanced.kernel.harden',


@bjmcnic @willyu-elastic same question

I agree this should move out of kernel given the direction we're going. My concern with something like windows.advanced.harden.enabled would be that what that really refers to is a a parallel technique for protecting our process that will remain useful on Windows 7 and 8.0 if we wish to use it, and while in some ways redundant, would still function even while PPL'd. Perhaps windows.advanced.harden.selfprotectprocess would resolve the ambiguity of simply enabled? @gabriellandau @ferullo @willyu-elastic

that really refers to is a a parallel technique for protecting our process that will remain useful on Windows 7 and 8.0 if we wish to use it

I don't want to support this. I would, however, like a way to opt out of PPL for debugging/troubleshooting purposes. Is that feasible?

I mean, while anything is possible in software, it'd be significant change to have our install/uninstall logic parse and respect the config. Additionally, dynamic reconfigs that toggled PPL would force service restarts. As such, I don't believe having PPL as opt-in/out in the config makes sense. However, it wouldn't be difficult to allow our executable to install without PPL, or to modify an existing PPL install to un-PPL it and restart it.

In regards to this issue though, are you suggesting we remove the current harden option then? And we could add new options (password protected uninstall, etc.) in a later release?

This is carried forward from Endgame. If we had crashes or hangs before whitelists were applied, we would crash and WER wouldn't be able to dump us. We couldn't attach a debugger. Trusted Apps are applied as part of whitelists, so they're ineffective until whitelists are installed. This feature gave us a kill switch for such situations, though it was cumbersome to enable (modifying files on the SMP and generating new deployment profiles).

bDisableHardening is a parameter we have to pass when handshaking with the driver. When I added it for Elastic, hardening was enabled by default. Rather than hard-coding it to false, I exposed it as an opt-out advanced config option like we did for Endgame. We disabled legacy hardening on Elastic almost immediately thereafter (before that release was made), so it's an unused feature.

are you suggesting we remove the current harden option then

Yes, unless we clearly and conspicuously indicate that it is an untested and unsupported feature. I worry some users will turn it on anyway and contact us when something doesn't work as expected.

I mean, while anything is possible in software, it'd be significant change to have our install/uninstall logic parse and respect the config

Understood. You've put more thought into this than me. I'm just wondering if there's a way to force the Win7 install behavior on Win8.1+ as a troubleshooting mechanism. It doesn't have to be through policy, and doesn't have to be toggle-able. If, for example, we crash on Win8.1 and earlier builds of Win10/Server, WER will encrypt our dumps, making them useless to us. This doesn't have to be in the initial push.

I shouldn't have pinged on this topic here. Let's move this to an endpoint-dev issue discussion so we don't co-opt Jane's PR. This PR is just mirroring the Endpoint's advanced configuration. We'll revisit what is in this document before it is released to make sure it is in sync with the options Endpoint actually supports.

thanks all for the feedback. merging this, and we can open a follow-on PR to make sure the schema is exactly right.

kibanamachine · 2020-11-02T18:14:40Z

Friendly reminder: Looks like this PR hasn’t been backported yet.
To create backports run node scripts/backport --pr 80390 or prevent reminders by adding the backport:skip label.

jmiller263 · 2020-11-02T18:19:56Z

@kevinlog should i backport this?

* Create Policies for each generated host * Refactor Ingest setup to also setup Fleet * Rename prop name * Add generic response type to KbnClient.request + support for headers * first attempt at adding fleet agent registration * a little closer with fleet integration * SUCCESS. Able to enroll agent and set it to online * update names to be policy * policy generator has advanced types in endpoint confit * linting * flesh out callback * add submit button for verify_peer * add verify hostname field * 145 generalize cb * 145 fix setAgain and getValue * 145 merge conflict * 145 add verify_hostname back, start loop for form * 145 remove OS trick * 145 make AdvancedPolicyForms its own component * 145 grid partially working * 145 back to basics * 145 back to basics * 145 rolled back grid * 145 flex table working * 145 undo accidental change * 145 remove extra schema file * 145 remove unused variable * 145 kevin's PR feedback * 145 fix type check and jest * 145 EuiFlexGroups * 145 use simple EuiFormRow and add show/hide buttons * 145 move all advanced policy code to advanced file; remove unnec test code * 145 fix IDs * 145 take out unnecessary stuff * 145 removed a couple more lines * 145 add some fields back in * 145 add spacer Co-authored-by: Paul Tavares <paul.tavares@elastic.co> Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com> Co-authored-by: kevinlog <kevin.logan@elastic.co> Co-authored-by: Candace Park <candace.park@elastic.co> Co-authored-by: Paul Tavares <paul.tavares@elastic.co> Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com> Co-authored-by: kevinlog <kevin.logan@elastic.co> Co-authored-by: Candace Park <candace.park@elastic.co>

paul-tavares and others added 30 commits August 3, 2020 13:36

Create Policies for each generated host

d4af7e4

Refactor Ingest setup to also setup Fleet

9b1ff83

Rename prop name

915c910

Add generic response type to KbnClient.request + support for headers

9e53c33

first attempt at adding fleet agent registration

3797983

a little closer with fleet integration

67425be

SUCCESS. Able to enroll agent and set it to online

3dc453c

Merge branch 'master' into task/endpoint-host-data-generator-create-p…

766821b

…olicy

update names to be policy

63f4d1c

policy generator has advanced types in endpoint confit

a2297b3

linting

0889459

Merge branch 'task/endpoint-host-data-generator-create-policy' of htt…

ffd46ef

…ps://github.com/paul-tavares/kibana into task/145_advanced_policy_poc

flesh out callback

e868b25

add submit button for verify_peer

41cfac0

Merge remote-tracking branch 'upstream/master' into task/145_advanced…

ba3b90e

…_policy_poc

add verify hostname field

67b12d2

145 generalize cb

638dc5f

145 fix setAgain and getValue

59c8170

145 merge conflict

98601b2

145 merge conflict

ff425c8

145 add verify_hostname back, start loop for form

3726588

145 remove OS trick

0b98114

145 make AdvancedPolicyForms its own component

201f433

Merge remote-tracking branch 'upstream/master' into task/145_advanced…

749c3f3

…_policy_poc

145 grid partially working

e534356

145 grid partially working

2c6530e

Merge remote-tracking branch 'upstream/master' into task/145_advanced…

bda9b8c

…_policy_poc

145 back to basics

aa94cda

145 back to basics

6074d82

Merge remote-tracking branch 'upstream/master' into task/145_advanced…

38574c6

…_policy_poc

jmiller263 added 3 commits October 22, 2020 10:26

145 take out unnecessary stuff

8bd1ef6

Merge remote-tracking branch 'upstream/master' into task/145_advanced…

9a6afa0

…_policy_poc

145 removed a couple more lines

06c3b4d

jmiller263 requested a review from parkiino October 22, 2020 15:43

jmiller263 commented Oct 22, 2020

View reviewed changes

...ck/plugins/security_solution/public/management/pages/policy/models/advanced_policy_schema.ts Show resolved Hide resolved

kevinlog reviewed Oct 22, 2020

View reviewed changes

145 add some fields back in

37cc6a4

paul-tavares reviewed Oct 22, 2020

View reviewed changes

145 add spacer

c7054c6

paul-tavares approved these changes Oct 27, 2020

View reviewed changes

MindyRS added the Team: SecuritySolution Security Solutions Team working on SIEM, Endpoint, Timeline, Resolver, etc. label Oct 27, 2020

Merge remote-tracking branch 'upstream/master' into task/145_advanced…

946dbc1

…_policy_poc

parkiino approved these changes Oct 27, 2020

View reviewed changes

Merge remote-tracking branch 'upstream/master' into task/145_advanced…

a55447a

…_policy_poc

ferullo reviewed Oct 28, 2020

View reviewed changes

MindyRS added Team:Defend Workflows “EDR Workflows” sub-team of Security Solution and removed Team:Endpoint Management labels Oct 29, 2020

jmiller263 merged commit ee7ce25 into elastic:master Oct 29, 2020

jmiller263 deleted the task/145_advanced_policy_poc branch October 29, 2020 18:09

kibanamachine added the backport missing Added to PRs automatically when the are determined to be missing a backport. label Nov 2, 2020

jmiller263 mentioned this pull request Nov 2, 2020

[7.x] [SECURITY_SOLUTION] 145: Advanced Policy UI (#80390) #82363

Merged

kibanamachine removed the backport missing Added to PRs automatically when the are determined to be missing a backport. label Nov 3, 2020

jmiller263 mentioned this pull request Nov 9, 2020

[SECURITY_SOLUTION] 145: Advanced Policy Tests #82898

Merged

8 tasks

	function getValue(obj: Record<string, unknown>, path: string[]) {
	function getValue(obj: Record<string, unknown>, path: string[]): string \| undefined {

Conversation

jmiller263 commented Oct 13, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Checklist

For maintainers

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kevinlog commented Oct 22, 2020

Uh oh!

paul-tavares left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

paul-tavares left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

parkiino left a comment

Choose a reason for hiding this comment

Uh oh!

kibanamachine commented Oct 28, 2020

💚 Build Succeeded

Metrics [docs]

@kbn/optimizer bundle module count

async chunks size

History

Uh oh!

ferullo left a comment

Choose a reason for hiding this comment

Uh oh!

ferullo Oct 28, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gabriellandau Oct 29, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

jmiller263 commented Oct 13, 2020 •

edited

Loading

paul-tavares left a comment •

edited

Loading

ferullo Oct 28, 2020 •

edited

Loading

gabriellandau Oct 29, 2020 •

edited

Loading