[Osquery] Add Detection action #133279

tomsonpl · 2022-06-01T09:05:07Z

Hi, this PR aims to resolve this issue https://github.com/elastic/security-team/issues/1103.

After a meeting happening sometime in between June/July where it was suggested that we shouldn't make Osquery a connector, but instead keep our data in the rule itself. Therefore responseActions in the rule, specifically in the rule params, more specifically in QueryRuleParams.

This PR will contain two major pieces:

rule and repsonseActions
osquery tab with results in Alert details.

TODO:

write more tests
Feedback appreciated re: validation of responseActions within step_rule_actions.

https://watch.screencastify.com/v/gWN8anJcbYamvT1hkipl

# Conflicts: # x-pack/plugins/osquery/public/live_queries/form/index.tsx

tomsonpl · 2022-06-13T07:31:51Z

@elasticmachine merge upstream

…squery-detection-action

tomsonpl · 2022-06-19T18:14:51Z

@elasticmachine merge upstream

…squery-detection-action

This reverts commit 12370cf.

jpdjere · 2022-09-19T12:34:52Z

.../server/lib/detection_engine/rule_response_actions/schedule_notification_response_actions.ts

+}
+
+interface ScheduleNotificationActions {
+  signals: unknown[];


Nit: any reason why not call this alerts instead of the deprecated signals?

Hmm, good question. For the purpose of this component, I think we could call it alerts, but there are some other places where these are signals, so I didn't want to stand out ;p
eg. x-pack/plugins/security_solution/server/lib/detection_engine/rule_types/create_security_rule_type_wrapper.ts

jpdjere · 2022-09-19T12:58:41Z

x-pack/plugins/osquery/public/components/accordion.tsx

+import styled from 'styled-components';
+import { EuiAccordion } from '@elastic/eui';
+
+export const StyledEuiAccordion = styled(EuiAccordion)`


Look like this StyledEuiAccordion was extracted out but wasn't used for replacing any of its current uses?

Good catch, there was quite a big refactor in the meanwhile, I'll remove this file 👍

banderror · 2022-09-19T16:23:49Z

about a major one - I'll get rid of the connection between responseActions and throttle.

We can take it out of the ''throttle'' and just show always, if you think it's a better approach

@tomsonpl Yes! Now when you did that, and response actions are decoupled from regular actions and their frequency (throttle), it:

fixed the UI/UX issues I mentioned above
fixed bulk editing rule actions behavior: it cannot affect response actions anymore, which is what we want

Tested all of that locally. Works great 👍

…dion file

banderror · 2022-09-19T16:25:38Z

"packId": "216a9df0-37bf-11ed-81a4-03e432e474ec" - is this a "foreign key" to a saved object?

"savedQueryId": "osquery_manager-a08d7320-1823-11ed-89c6-331eb0db6d01" - is this a "foreign key" to a saved object?

"id": "b0d25568-c6da-41ff-b999-d9949dbf9cef" and the other 2 response action ids - what are they, do they refer to any entities in the system?

Not sure what the 'foreigh key'' is, @patrykkopycinski could you help, please?

By foreign key, I mean a reference to any other Kibana entity (for example, a saved object) by id.

The ids have different purposes. The one above (in params is an ID for the query - that we later on use to fetch osquery results in the EventDetails tab. The Id in queries array is a query name of each query in the pack.

Ok, so I guess these are needed.

Do you actually need the packId and savedQueryId?

The problem is that you mixed references with copies of data here, and ideally it should be either full copy or only a reference. If that makes sense. You can chat with @marshallmain on that because we had a similar issue with Saved Query rules and it was fixed in #140064 by storing either a reference to a saved query, or its copy at the time of rule creation/editing.

tomsonpl · 2022-09-19T16:28:07Z

Oh, then yes, these are foreign keys, but we do not make use of them yet, there is nothing pushed to references: []. We are planning to make this connection by 8.6.

For now, as you mentioned, we keep both a 'copy' and a reference Id. However, it's not a real copy, because we just store a small piece of data, just query + ecs_mapping, mostly. I think we could get rid of reference id (packId, savedQueryId) - but that would trigger some UI changes in the form that I would like to avoid at this late stage. Do you think it would be problematic to keep both for 8.5, and then move to references at 8.6 ?

banderror · 2022-09-19T16:39:01Z

@banderror I validated these 2 issues with ''no saved query'' and ''no saved pack'' meaning if kibana user import rule but doesnt have SO.

Saved query - there is just input of the dropdown that is missing value right? In this scenario we just use just 'query' - saved query field is just a convenient way to use your actual saved queries, but there is no connection in the functionality (no reference) between response action and savedQueryId. I believe this functionality works fine.

Saved Packs - there was a bug hidden indeed, again big thanks for catching this. We have no reference between response_action and packId, meaning when somebody edits/deletes pack - it doesnt update response_action. We are planning to implement this in the next iteration (8.6) But for now, we just keep a saved version of queries from the pack and send it in response_action. There was a tiny bug that was cleaning packId and queries after editing ''empty pack'' repsonse_action - I fixed it and now the values persist. However, there is still a wrong behaviour of ''not showing'' these details if the user has no pack. We'll have to thing about a nice UI workaround for this until we create a connection between packs and response_actions (references).

Do you think it would be acceptable to skip having 'references' in 8.5 and update this in 8.6? For 8.5 we'll (after FF) add a UI explaining what happened if actual user has no pack.

It's hard for me to tell for sure if it would be acceptable to skip having 'references' in 8.5 and update this in 8.6. I don't think that writing SO references will help to fix that by itself.

The two ref integrity issues I mentioned above require support for exporting and importing the referenced objects, in this case osquery packs and single queries, in the Security rule export and import endpoints. This is not something I'm familiar with - @yctercero could you please comment on that?

UPD: FWIW we don't export and import connectors with rules, and this is what happens when you import a rule that references non-existent connectors:

So to me the current implementation doesn't seem too risky, but maybe I'm missing something.

banderror · 2022-09-19T16:44:02Z

I'll try to consolidate all the changes in

x-pack/plugins/security_solution/common/detection_engine/rule_response_actions
x-pack/plugins/security_solution/public/detection_engine/rule_response_actions
x-pack/plugins/security_solution/server/lib/detection_engine/rule_response_actions

as you suggested @banderror 👍

🙏 Checked the changes -- looks great! Thank you.

The most part of response_actions regarding the forms etc is already in x-pack/plugins/security_solution/public/detections/components/rule_response_actions/. What is a difference between detection_engine and detections? I feel like some code is used across these two. Should I move it out from detections to detection_engine?

Yes, please do it similar to how it was done for x-pack/plugins/security_solution/public/detection_engine/rule_exceptions and x-pack/plugins/security_solution/public/detection_engine/rule_monitoring. We are moving away from the x-pack/plugins/security_solution/public/detections folder towards more well-defined detection engine subdomains. More info here: #138600

The x-pack/plugins/security_solution/server/lib/detection_engine part - I feel like there is not much code in there that is just responsible for responseActions. I moved schedule_notification_response_actions.ts to rule_response_actions .

That LGTM, thank you 👍

banderror

I think most of my comments have been addressed by now, and it LGTM.

I don't feel that the current implementation imposes any significant risk for the future and I'm ok with merging it as is and then improving in the next cycle. I'd still consult with @marshallmain and @yctercero - maybe they have any other thoughts on the data model, referential integrity, or export/import.

Would be still good to know answers to some of the general questions I posted above.

Thank you @tomsonpl for all your efforts, quick responses and fixes!

banderror · 2022-09-19T14:55:10Z

x-pack/plugins/security_solution/common/detection_engine/schemas/common/schemas.ts

@@ -101,6 +101,8 @@ export const data_view_id = t.string;

 export const dataViewIdOrUndefined = t.union([data_view_id, t.undefined]);

+export const action_action_type_id = NonEmptyString;


Looks like this one is an unused leftover

…engine

marshallmain

Detection engine changes LGTM. Now that the response actions are decoupled from the throttle, I don't see this feature interfering with existing detection engine features. The implementation in the rule executors also looks low risk to existing detection engine users - if a user has no response actions on the rule, there is no change to rule behavior at all, and if response actions are defined the only additional tasks are a couple of bulk requests that don't block the rule from completing. 👍

There are some more updates I'd like to see in the OsqueryParams schema to keep a consistent convention of using snake case in the HTTP API and camelCase internally. Essentially we should follow the same convention we have so far with the rule schemas and have separate versions of OsqueryParams for the HTTP API and the internal rule representation with a conversion function between them.

tomsonpl · 2022-09-19T20:27:50Z

@elasticmachine merge upstream

kqualters-elastic · 2022-09-19T22:46:08Z

...ns/security_solution/public/detection_engine/rule_response_actions/response_actions_list.tsx

+const GhostFormField = () => <></>;
+
+// eslint-disable-next-line react/display-name
+export const ResponseActionsList = React.memo(


to get rid of this eslint disable, you can just declare the const here, set ResponseActionsList.displayName = 'ResponseActionsList' below the component definition, and then export ResponseActionsList i know it might seem tedious but display name can help debugging sometimes

kqualters-elastic · 2022-09-19T22:49:55Z

x-pack/plugins/security_solution/public/detections/pages/detection_engine/rules/helpers.tsx

 ): ActionsStepRule => {
-  const { enabled, throttle, meta, actions = [] } = rule;
+  // eslint-disable-next-line @typescript-eslint/naming-convention
+  const { enabled, throttle, meta, actions = [], response_actions = [] } = rule;


Suggested change

const { enabled, throttle, meta, actions = [], response_actions = [] } = rule;

const { enabled, throttle, meta, actions = [], response_actions: responseActions = [] } = rule;

kqualters-elastic · 2022-09-19T22:53:58Z

x-pack/plugins/security_solution/public/common/components/event_details/osquery_tab.tsx

+  }
+
+  const { OsqueryResults } = osquery;
+  const parameters = rawEventData.fields['kibana.alert.rule.parameters'];


would be easier to work with everything in rawEventData if you used the helper expandDottedObject https://github.com/elastic/kibana/blob/main/x-pack/plugins/security_solution/public/common/utils/alerts.ts#L82

I'll be creating a small PR later today with a small fix, do you mind If I attach this change there too? :) Thanks!

And yessss, this is much easier, thank you :)

kqualters-elastic

few small nitpicks but overall this feature is awesome and threat hunting changes lgtm!

kqualters-elastic · 2022-09-19T23:09:00Z

x-pack/plugins/osquery/public/shared_components/osquery_response_action_type/index.tsx

+        setValue('packId', [packId]);
+      }
+    }
+    /* eslint-disable-next-line react-hooks/exhaustive-deps */


I get why you disable this to only run once, but this line is terrifying and often causes bugs, there are a few other ways to accomplish the run once by refactoring the hook to be something like

const mungeData = useCallback(() => { if (defaultParams && defaultParams.id) { const { packId, ecs_mapping: ecsMapping, ...restParams } = defaultParams; map(restParams, (value, key: keyof OsqueryResponseActionsParamsFormFields) => { if (!isEmpty(value)) { setValue(key, value); } }); if (ecsMapping) { const converted = convertECSMappingToFormValue(ecsMapping); setValue('ecs_mapping', converted); } if (!isEmpty(packId)) { setValue('packId', [packId]); } } }, [defaultParams, setValue]); useEffect(() => { mungeData(); }, [mungeData]);

the callback only runs when the reference in the useEffect changes, which won't happen just from render cycles

Changed it into useEffectOnce from react-use/lib/useEffectOnce , hope this is better :)

# Conflicts: # x-pack/plugins/security_solution/server/lib/detection_engine/rule_types/query/create_query_alert_type.ts # x-pack/plugins/security_solution/server/lib/detection_engine/rule_types/saved_query/create_saved_query_alert_type.ts # x-pack/plugins/security_solution/server/lib/detection_engine/signals/executors/query.ts

…squery-detection-action

kibana-ci · 2022-09-20T06:24:02Z

💚 Build Succeeded

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id	before	after	diff
`osquery`	295	308	+13
`securitySolution`	3108	3124	+16
total			+29

Public APIs missing comments

Total count of every public API that lacks a comment. Target amount is 0. Run node scripts/build_api_docs --plugin [yourplugin] --stats comments for more detailed information.

id	before	after	diff
`osquery`	14	21	+7
`securitySolution`	53	54	+1
total			+8

Async chunks

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

id	before	after	diff
`osquery`	1009.8KB	1.0MB	+18.4KB
`securitySolution`	6.4MB	6.5MB	+18.3KB
total			+36.7KB

Public APIs missing exports

Total count of every type that is part of your API that should be exported but is not. This will cause broken links in the API documentation system. Target amount is 0. Run node scripts/build_api_docs --plugin [yourplugin] --stats exports for more detailed information.

id	before	after	diff
`osquery`	2	3	+1
`securitySolution`	22	23	+1
total			+2

Page load bundle

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

id	before	after	diff
`osquery`	90.0KB	91.3KB	+1.3KB
`securitySolution`	262.6KB	262.6KB	+26.0B
total			+1.3KB

Unknown metric groups

API count

id	before	after	diff
`osquery`	14	21	+7
`securitySolution`	54	55	+1
total			+8

async chunk count

id	before	after	diff
`osquery`	9	13	+4

ESLint disabled line counts

id	before	after	diff
`osquery`	106	111	+5
`securitySolution`	407	408	+1
total			+6

References to deprecated APIs

id	before	after	diff
`securitySolution`	66	67	+1

Total ESLint disabled count

id	before	after	diff
`osquery`	108	113	+5
`securitySolution`	479	480	+1
total			+6

History

💚 Build #73640 succeeded 6056245
💔 Build #73616 failed 6e37ecb
💚 Build #73501 succeeded cfa1143
💚 Build #73336 succeeded a5fafee
💛 Build #73284 was flaky dcf299e
💔 Build #73241 failed 8a10094

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

cc @tomsonpl

tomsonpl · 2022-09-20T06:27:35Z

Thank you everyone that helped me get this PR ready, your review and feedback was enormous :elasticheart:

tomsonpl added 6 commits May 30, 2022 07:51

initial commit

c838b2f

add osquery form

8796824

add get_action_type

3017315

Merge branch 'main' into osquery-detection-action

e101277

# Conflicts: # x-pack/plugins/osquery/public/live_queries/form/index.tsx

add results in the flyout

f9a6243

small adjustments

29fffac

kibanamachine and others added 4 commits June 13, 2022 03:31

Merge branch 'main' into osquery-detection-action

407a07f

Merge remote-tracking branch 'origin/osquery-detection-action' into o…

d8087ab

…squery-detection-action

add infinite scroll, and intersection observer

b0361ce

Merge branch 'main' into osquery-detection-action

936db7b

kibanamachine and others added 14 commits June 19, 2022 14:14

Merge branch 'main' into osquery-detection-action

f4f571d

fix form problems

611d92b

Merge remote-tracking branch 'origin/osquery-detection-action' into o…

7e2f9bc

…squery-detection-action

fix import

154b7dc

remove code duplication

e659aae

fix translation path

68f0dde

WIP remove Form

12370cf

Revert "WIP remove Form"

2efec11

This reverts commit 12370cf.

fix types, fix infinite scroll, add feature flag

e494e35

type rawEventData

0d800cd

fix types

594a452

add additional feature flag in security_solution

d20abe2

more changes to types

588d0c0

wip - trying to fix tests

bfb9dad

tomsonpl marked this pull request as ready for review June 22, 2022 10:50

tomsonpl requested review from a team as code owners June 22, 2022 10:50

jpdjere reviewed Sep 19, 2022

View reviewed changes

change interface of pack queries in pack field, remove redundant acor…

cfa1143

…dion file

banderror approved these changes Sep 19, 2022

View reviewed changes

move rule response actions components out of detections to detection_…

26d9988

…engine

patrykkopycinski removed the request for review from a team September 19, 2022 17:24

patrykkopycinski approved these changes Sep 19, 2022

View reviewed changes

initial schema for osquery response actions

6e37ecb

marshallmain approved these changes Sep 19, 2022

View reviewed changes

Merge branch 'main' into osquery-detection-action

6056245

kqualters-elastic reviewed Sep 19, 2022

View reviewed changes

kqualters-elastic approved these changes Sep 19, 2022

View reviewed changes

kqualters-elastic reviewed Sep 19, 2022

View reviewed changes

tomsonpl added 3 commits September 20, 2022 07:11

remove eslint disable

a64ea7e

Merge remote-tracking branch 'origin/osquery-detection-action' into o…

768402f

…squery-detection-action

tomsonpl enabled auto-merge (squash) September 20, 2022 05:26

tomsonpl merged commit fe6060d into elastic:main Sep 20, 2022

kibanamachine added the backport:skip This commit does not require backporting label Sep 20, 2022

xcrzx mentioned this pull request Sep 21, 2022

[Security Solution] Unable to save rule while user duplicating the rule #141200

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Osquery] Add Detection action #133279

[Osquery] Add Detection action #133279

tomsonpl commented Jun 1, 2022 •

edited

Loading

tomsonpl commented Jun 13, 2022

tomsonpl commented Jun 19, 2022

jpdjere Sep 19, 2022

tomsonpl Sep 19, 2022

jpdjere Sep 19, 2022

tomsonpl Sep 19, 2022

banderror commented Sep 19, 2022

banderror commented Sep 19, 2022 •

edited

Loading

tomsonpl commented Sep 19, 2022 •

edited

Loading

banderror commented Sep 19, 2022 •

edited

Loading

banderror commented Sep 19, 2022

banderror left a comment

banderror Sep 19, 2022

marshallmain left a comment

tomsonpl commented Sep 19, 2022

kqualters-elastic Sep 19, 2022

tomsonpl Sep 20, 2022

kqualters-elastic Sep 19, 2022

tomsonpl Sep 20, 2022

kqualters-elastic Sep 19, 2022

tomsonpl Sep 20, 2022

tomsonpl Sep 20, 2022

kqualters-elastic left a comment

kqualters-elastic Sep 19, 2022

tomsonpl Sep 20, 2022

kibana-ci commented Sep 20, 2022

API count

async chunk count

ESLint disabled line counts

References to deprecated APIs

Total ESLint disabled count

tomsonpl commented Sep 20, 2022

		@@ -101,6 +101,8 @@ export const data_view_id = t.string;

		export const dataViewIdOrUndefined = t.union([data_view_id, t.undefined]);

		export const action_action_type_id = NonEmptyString;

	const { enabled, throttle, meta, actions = [], response_actions = [] } = rule;
	const { enabled, throttle, meta, actions = [], response_actions: responseActions = [] } = rule;

[Osquery] Add Detection action #133279

[Osquery] Add Detection action #133279

Conversation

tomsonpl commented Jun 1, 2022 • edited Loading

tomsonpl commented Jun 13, 2022

tomsonpl commented Jun 19, 2022

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

banderror commented Sep 19, 2022

banderror commented Sep 19, 2022 • edited Loading

tomsonpl commented Sep 19, 2022 • edited Loading

banderror commented Sep 19, 2022 • edited Loading

banderror commented Sep 19, 2022

banderror left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

marshallmain left a comment

Choose a reason for hiding this comment

tomsonpl commented Sep 19, 2022

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

kqualters-elastic left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kibana-ci commented Sep 20, 2022

💚 Build Succeeded

Metrics [docs]

Module Count

Public APIs missing comments

Async chunks

Public APIs missing exports

Page load bundle

API count

async chunk count

ESLint disabled line counts

References to deprecated APIs

Total ESLint disabled count

History

tomsonpl commented Sep 20, 2022

tomsonpl commented Jun 1, 2022 •

edited

Loading

banderror commented Sep 19, 2022 •

edited

Loading

tomsonpl commented Sep 19, 2022 •

edited

Loading

banderror commented Sep 19, 2022 •

edited

Loading