[Security Solution][Admin][Policy][Event Filters] Update event filters creation to include more match options by parkiino · Pull Request #170495 · elastic/kibana

parkiino · 2023-11-03T08:15:51Z

Summary

Adds a matches and does not match operator option to all eligible event filter creation entry fields that support matches
Updates the existing warning to only appear if that is the file.path.text entry field is selected
Generalize the warning for wildcard usage if operator is matches and a wildcard is used in the value
Updates wildcard warning tooltip to include "Creating event filters with both matches and does not match operators may significantly decrease performance."
Unit tests

Screenshots

Warning about wildcards affecting Endpoint performance

Event Filter & Artifact

LINUX

linux artifact entry

{
  "entries": [
    {
      "type": "simple",
      "entries": [
        {
          "field": "event.category",
          "operator": "included",
          "type": "wildcard_cased",
          "value": "network"
        },
        {
          "field": "process.name",
          "operator": "included",
          "type": "exact_cased",
          "value": "network"
        }
      ]
    }
  ]
}

WINDOWS

windows artifact entry

{
  "entries": [
    {
      "type": "simple",
      "entries": [
        {
          "field": "event.kind",
          "operator": "included",
          "type": "wildcard_cased",
          "value": "event"
        },
        {
          "field": "process.name",
          "operator": "included",
          "type": "exact_caseless",
          "value": "event"
        },
        {
          "field": "event.category",
          "operator": "included",
          "type": "wildcard_cased",
          "value": "authentication"
        },
        {
          "field": "process.name",
          "operator": "included",
          "type": "exact_caseless",
          "value": "authentication"
        }
      ]
    }
  ]
}

MAC

mac artifact entry

{
  "entries": [
    {
      "type": "simple",
      "entries": [
        {
          "field": "event.id",
          "operator": "included",
          "type": "wildcard_cased",
          "value": "071e1cfc-8333-4c6c-965a-00678c7b1d61"
        },
        {
          "field": "process.name",
          "operator": "included",
          "type": "exact_caseless",
          "value": "071e1cfc-8333-4c6c-965a-00678c7b1d61"
        },
        {
          "field": "file.path",
          "operator": "included",
          "type": "wildcard_cased",
          "value": "C:\\My Documents\\business\\January\\processName"
        },
        {
          "field": "process.name",
          "operator": "included",
          "type": "exact_caseless",
          "value": "C:\\My Documents\\business\\January\\processName"
        }
      ]
    }
  ]
}

…matches

elasticmachine · 2023-11-08T20:37:26Z

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

rylnd

Detection engine changes for non-endpoint exceptions are trivial and LGTM.

I had a broader question about the consequence of opening up the wildcard operator to such a broad number of fields, but I suppose that's the entire point of this PR 🤷 . I'll defer to @ashokaditya for the endpoint aspect of this.

davismcphee

Code-only review. Data view mock changes LGTM 👍

parkiino · 2023-11-09T15:41:25Z

@rylnd We understand that adding more matches can cause slower performance on Endpoint, but we show a warning in the UI to the notify the users of this. We worked with the Endpoint team on this and the Endpoint users are asking for this flexibility. @ferullo @roxana-gheorghe

ferullo · 2023-11-09T16:47:44Z

Updates the existing warning to only appear if that is the file.path.text entry field is selected

Why is the warning based on selecting file.path.text rather than use of the matches operator?

kevinlog · 2023-11-09T17:02:46Z

@parkiino

Why is the warning based on selecting file.path.text rather than use of the matches operator?

Agreed on this point from @ferullo , we should make this message generic and show it for any field using matches.

Can we make it show up for any field using matching and have the text say the following?

Using wildcards in value fields can impact Endpoint performance

For the tooltip:

We can reword the tooltip text like this by just remove the work "path":

To make a more efficient event filter, use multiple conditions and make them as specific as possible when using wildcards in the values. For instance, adding a process.name or file.name field.

…matches

parkiino · 2023-11-09T20:57:48Z

@ferullo @kevinlog adjusted the wildcard warning to be more generic

ashokaditya

Thanks for expanding matches to more fields.

I tested out the generated artifacts, and they are as expected. I tested out the UX and I have some comments and suggestions for improvements. Please let me know if you have questions.

ashokaditya · 2023-11-10T16:05:13Z

packages/kbn-securitysolution-utils/src/path_validations/index.ts

 export type TrustedAppEntryTypes = Extract<EntryTypes, 'match' | 'wildcard'>;

-export const validateFilePathInput = ({
+export const validatePotentialWildcardInput = ({


We now see the FILEPATH_WARNING for any field when selecting for matches operator.

ashokaditya · 2023-11-13T09:51:16Z

packages/kbn-securitysolution-utils/src/path_validations/index.ts


-export const validateFilePathInput = ({
+export const validatePotentialWildcardInput = ({
+  fieldName = 'file.path.text',


This param should be similar to the value param. field = ''. You would also need to update the tests for validatePotentialWildcardInput for win/unix path validation.

ashokaditya · 2023-11-13T09:53:40Z

packages/kbn-securitysolution-utils/src/path_validations/index.ts

  os,
  value = '',
 }: {
+  fieldName?: string;


Suggested change

fieldName?: string;

field?: string;

ashokaditya · 2023-11-13T09:53:53Z

packages/kbn-securitysolution-utils/src/path_validations/index.ts


-export const validateFilePathInput = ({
+export const validatePotentialWildcardInput = ({
+  fieldName = 'file.path.text',


Suggested change

fieldName = 'file.path.text',

field = '',

ashokaditya · 2023-11-13T09:56:08Z

packages/kbn-securitysolution-utils/src/path_validations/index.ts

+    isFieldFilePath &&
+    isPathValid({
+      os,
+      field: 'file.path.text',


Since we're already checking that this is a file.path.text or isFiledFilePath, the field value should be a is:

Suggested change

field: 'file.path.text',

field: filedName,

or if you rename the fieldName function param to field

Suggested change

field: 'file.path.text',

field,

ashokaditya · 2023-11-13T09:56:48Z

packages/kbn-securitysolution-utils/src/path_validations/index.ts

+  if (
+    (isFieldFilePath && isValidFilePath) ||
+    (!isFieldFilePath && hasSimpleFileName !== undefined)
+  ) {


isValidFilePath already checks for isFieldFilePath. You can simplify this boolean:

Suggested change

if (

(isFieldFilePath && isValidFilePath) ||

(!isFieldFilePath && hasSimpleFileName !== undefined)

) {

if (isValidFilePath || !isFieldFilePath) {

if (hasSimpleFileName !== undefined && !hasSimpleFileName) {

Also, looks like with this new if condition the else block is unreachable. I'd suggest we refactor the two if blocks in this function to something like this instead, that is simpler to reason with.

// for file.path.text if (isFieldFilePath && !(isValidFilePath && textInput.length)) { return FILEPATH_WARNING; } // for all other fields if (hasSimpleFileName !== undefined && !hasSimpleFileName) { return WILDCARD_WARNING; }

A better approach would be to create a new function that uses the existing valideFilePathInput function when field is file.path.text and uses a different function to show WILDCARD_WARNING when field is not file.path.text (and only when there is a wildcard * or ? in the value). That way you would need to do minimal changes and write tests only for new functions. Also the logic is easier to follow. Something like:

const validateWildcardInput = ({ field = '', os, value = '', }: { field?: string; os: OperatingSystem; value?: string; }) => { if (field === 'file.path.text') { return validateFilePathInput({ os, value }); } return validatePotentialWildcard(value); };

export const validatePotentialWildcard = (value?: string): string | undefined => { if (/\*|\?/.test(value ?? '')) { return WILDCARD_WARNING; } };

and then in the entryRenderer you would use the new function

validateWildcardInput( { field: entry.field?.name, os, value: wildcardValue })

Thanks for the suggestion! I agree this is a lot easier to read as well

x-pack/plugins/lists/public/exceptions/components/builder/entry_renderer.tsx

ashokaditya · 2023-11-13T09:57:54Z

x-pack/plugins/lists/public/exceptions/components/builder/entry_renderer.tsx

          }
-          const warning = validateFilePathInput({ os, value: wildcardValue });
+          const warning = validatePotentialWildcardInput({
+            fieldName: entry.field?.name ?? '',


We should just let this be undefined similar to what we do for value argument.

Suggested change

fieldName: entry.field?.name ?? '',

field: entry.field?.name,

ashokaditya · 2023-11-13T10:44:56Z

x-pack/plugins/lists/public/exceptions/components/builder/helpers.test.ts

      });

-      test('it returns all fields unfiletered if "item.nested" is not "child" or "parent"', () => {
+      test('it returns all fields unfiltered if "item.nested" is not "child" or "parent"', () => {


🔥 thanks for fixing type

ashokaditya

Thanks for the change @parkiino. 🔥 I've a few more suggestions. Also I think the old tests should stay and we should add new ones for the new functions we add in this PR. That way all the composable functions have unit test coverage of their own.

ashokaditya · 2023-11-14T08:08:06Z

packages/kbn-securitysolution-utils/src/path_validations/index.ts

  if (isValidFilePath) {
    if (hasSimpleFileName !== undefined && !hasSimpleFileName) {
-      return FILENAME_WILDCARD_WARNING;
+      return WILDCARD_WARNING;


I think I would expect to see only FILEPATH_WARNING in this function as it only deals with file.path.text field. What would make this simpler is if we combine the two if blocks in this function as

if ( !isValidFilePath || !value.length || (hasSimpleFileName !== undefined && !hasSimpleFileName) ) { return FILEPATH_WARNING; }

and that's all the function should return.

The FILEPATH_WARNING warns about a malformed path whereas the WILDCARD_WARNING warns the user that a wildcard will affect performance. We actually still want the WILDCARD_WARNING in this case, given that the path is valid.

Oh yeah you're absolutely right! Perhaps the following may be simpler to reason with:

if (!isValidFilePath || !value.length) { return FILEPATH_WARNING; } if (hasSimpleFileName !== undefined && !hasSimpleFileName) { return WILDCARD_WARNING; }

packages/kbn-securitysolution-utils/src/path_validations/index.test.ts

parkiino · 2023-11-14T15:38:33Z

@defensivedepth

linux wildcards will be case-sensitive and windows / mac wildcards will be case-insensitive

ashokaditya

Looks great. Thanks for all the changes. I've a small suggestion for improving readability, but this is good to go.

ashokaditya · 2023-11-14T16:42:28Z

packages/kbn-securitysolution-utils/src/path_validations/index.ts

  if (isValidFilePath) {
    if (hasSimpleFileName !== undefined && !hasSimpleFileName) {
-      return FILENAME_WILDCARD_WARNING;
+      return WILDCARD_WARNING;


Oh yeah you're absolutely right! Perhaps the following may be simpler to reason with:

if (!isValidFilePath || !value.length) { return FILEPATH_WARNING; } if (hasSimpleFileName !== undefined && !hasSimpleFileName) { return WILDCARD_WARNING; }

ashokaditya · 2023-11-14T16:43:25Z

packages/kbn-securitysolution-utils/src/path_validations/index.ts

 };

+const validatePotentialWildcardInput = (value?: string): string | undefined => {
+  if (/\*|\?/.test(value ?? '')) {


my editor says that using single character substituion is not that great after all. Using a set is better. 😅

Suggested change

if (/\*|\?/.test(value ?? '')) {

if (/[*?].test(value ?? '')) {

…matches

This reverts commit 75f1a7d.

…matches

kibana-ci · 2023-11-27T07:39:45Z

💚 Build Succeeded

Buildkite Build
Commit: c6c27e5

Metrics [docs]

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
`@kbn/securitysolution-utils`	32	39	+7

Async chunks

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

id	before	after	diff
`lists`	148.1KB	148.3KB	+265.0B
`securitySolution`	12.8MB	12.8MB	+68.0B
total			+333.0B

Unknown metric groups

API count

id	before	after	diff
`@kbn/securitysolution-utils`	37	44	+7

History

💛 Build #177994 was flaky bd4c109
💔 Build #176688 failed 75f1a7d
💚 Build #176580 succeeded c5a1ced
💔 Build #176418 failed 173de02
💔 Build #175819 failed 787c441
💔 Build #175556 failed 65c75fb

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

…on (#166002) ## Summary This PR adds `matches` (`wildcard include`) and `does not match` (`wildcard exclude`) to fields which support them when creating an Endpoint exception. For backwards compatibility with Endpoints < 8.2.0, Manifest Manager adds the following entry to Endpoint Exceptions containing _only_ wildcards: ```json { "field": "event.module", "operator": "included", "type": "exact_cased", "value": "endpoint" } ``` > [!Note] > Warnings for wrongly formatted wildcards don't seem to work correctly at the moment. #170495 will bring some changes in the related functions, so this PR is waiting on that to be merged. <img width="1465" alt="image" src="https://github.com/elastic/kibana/assets/39014407/db04fe0b-4cb3-4cba-a6d7-622a2239f059"> ## Sample manifests ### Linux ⚠️ On Linux, the type is always `wildcard_cased`, see the following comment for details: #120349 (comment) ```json { "entries": [ { "type": "simple", "entries": [ { "field": "file.path", "operator": "included", "type": "wildcard_cased", "value": "*/test/*" }, { "field": "event.module", "operator": "included", "type": "exact_cased", "value": "endpoint" } ] } ] } ``` ### Windows ```json { "entries": [ { "type": "simple", "entries": [ { "field": "file.path", "operator": "included", "type": "wildcard_caseless", "value": "*/test/*" }, { "field": "event.module", "operator": "included", "type": "exact_cased", "value": "endpoint" } ] } ] } ``` ### Checklist Delete any items that are not applicable to this PR. - [ ] Any text added follows [EUI's writing guidelines](https://elastic.github.io/eui/#/guidelines/writing), uses sentence case text and includes [i18n support](https://github.com/elastic/kibana/blob/main/packages/kbn-i18n/README.md) - [ ] [Documentation](https://www.elastic.co/guide/en/kibana/master/development-documentation.html) was added for features that require explanation or tutorials - [ ] [Unit or functional tests](https://www.elastic.co/guide/en/kibana/master/development-tests.html) 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](https://webaim.org/techniques/keyboard/)) - [ ] Any UI touched in this PR does not create any new axe failures (run axe in browser: [FF](https://addons.mozilla.org/en-US/firefox/addon/axe-devtools/), [Chrome](https://chrome.google.com/webstore/detail/axe-web-accessibility-tes/lhdoppojpmngadmnindnejefpokejbdd?hl=en-US)) - [ ] If a plugin configuration key changed, check if it needs to be allowlisted in the cloud and added to the [docker list](https://github.com/elastic/kibana/blob/main/src/dev/build/tasks/os_packages/docker_generator/resources/base/bin/kibana-docker) - [ ] This renders correctly on smaller devices using a responsive layout. (You can test this [in your browser](https://www.browserstack.com/guide/responsive-testing-on-local-server)) - [ ] This was checked for [cross-browser compatibility](https://www.elastic.co/support/matrix#matrix_browsers)

parkiino added 7 commits November 3, 2023 03:34

update event filters to include more match options

d8a2be1

make file path warning specific to file path

ca3dea7

fix test

cdf7dae

remove console.log

0fc5496

Merge remote-tracking branch 'upstream/main' into task/event-filters-…

667e55e

…matches

fix test

ed5ab97

Merge remote-tracking branch 'upstream/main' into task/event-filters-…

02825c0

…matches

parkiino marked this pull request as ready for review November 8, 2023 03:50

parkiino requested review from a team as code owners November 8, 2023 03:50

parkiino requested a review from rylnd November 8, 2023 03:50

Merge remote-tracking branch 'upstream/main' into task/event-filters-…

f0000a3

…matches

parkiino requested review from ashokaditya and dasansol92 November 8, 2023 16:51

parkiino added release_note:skip Skip the PR/issue when compiling release notes backport:skip This PR does not require backporting Team:Defend Workflows “EDR Workflows” sub-team of Security Solution v8.12.0 labels Nov 8, 2023

rylnd approved these changes Nov 8, 2023

View reviewed changes

davismcphee approved these changes Nov 9, 2023

View reviewed changes

parkiino added 3 commits November 9, 2023 15:40

generalize wildcard warning

e1a5144

adjust text, add simple unit tests

dea8dc1

Merge remote-tracking branch 'upstream/main' into task/event-filters-…

b441f5c

…matches

ferullo mentioned this pull request Nov 10, 2023

Feature Request: Additional Filtering Options for Elastic Defend #168478

Open

fix improt

18d3e33

remove unnecessary tests and translations

7cece88

ashokaditya reviewed Nov 13, 2023

View reviewed changes

gergoabraham mentioned this pull request Nov 13, 2023

[EDR Workflows] Add matches capabilities to Endpoint Exception creation #166002

Merged

8 tasks

add ashs suggestion

65c75fb

parkiino requested review from ashokaditya and dasansol92 November 14, 2023 05:50

ashokaditya reviewed Nov 14, 2023

View reviewed changes

rename functions, add more unit tests'

787c441

parkiino requested a review from ashokaditya November 14, 2023 16:41

ashokaditya approved these changes Nov 14, 2023

View reviewed changes

parkiino added 3 commits November 14, 2023 14:11

update regex

99d6a87

Merge remote-tracking branch 'upstream/main' into task/event-filters-…

0a6f34d

…matches

add does not match wildcard

0d642c0

parkiino requested a review from a team as a code owner November 16, 2023 05:32

parkiino added 2 commits November 16, 2023 00:45

fix function import

173de02

fix broken tests

c5a1ced

parkiino changed the title ~~[Security Solution][Admin][Policy][Event FIlters] Update event filters creation to include more match options~~ [Security Solution][Admin][Policy][Event Filters] Update event filters creation to include more match options Nov 16, 2023

parkiino and others added 6 commits November 16, 2023 15:51

remove does not match for now

75f1a7d

Revert "remove does not match for now"

0461dae

This reverts commit 75f1a7d.

Merge remote-tracking branch 'upstream/main' into task/event-filters-…

cb569a7

…matches

Merge remote-tracking branch 'upstream/main' into task/event-filters-…

fa71a65

…matches

update warning messaging

bd4c109

Merge branch 'main' into task/event-filters-matches

c6c27e5

parkiino enabled auto-merge (squash) November 27, 2023 06:34

parkiino merged commit 69b2cd2 into elastic:main Nov 27, 2023

parkiino deleted the task/event-filters-matches branch November 27, 2023 14:04

joepeeples mentioned this pull request Jan 4, 2024

Add matches capabilities to event filter creation elastic/security-docs#4574

Closed

	fieldName: entry.field?.name ?? '',
	field: entry.field?.name,

	if (/\*\|\?/.test(value ?? '')) {
	if (/[*?].test(value ?? '')) {

Conversation

parkiino commented Nov 3, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Screenshots

Event Filter & Artifact

Uh oh!

elasticmachine commented Nov 8, 2023

Uh oh!

rylnd left a comment

Choose a reason for hiding this comment

Uh oh!

davismcphee left a comment

Choose a reason for hiding this comment

Uh oh!

parkiino commented Nov 9, 2023

Uh oh!

ferullo commented Nov 9, 2023

Uh oh!

kevinlog commented Nov 9, 2023

Uh oh!

parkiino commented Nov 9, 2023

Uh oh!

ashokaditya left a comment

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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ashokaditya 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!

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!

Uh oh!

parkiino commented Nov 14, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ashokaditya left a comment

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!

kibana-ci commented Nov 27, 2023

💚 Build Succeeded

Metrics [docs]

Public APIs missing comments

Async chunks

API count

History

Uh oh!

Reviewers

Assignees

parkiino commented Nov 3, 2023 •

edited

Loading

ashokaditya left a comment •

edited

Loading

parkiino commented Nov 14, 2023 •

edited

Loading