Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

[FR] Add Support for ES|QL Rule Type and Remote Validation #3281

Merged
merged 17 commits into from
Dec 8, 2023
Merged

[FR] Add Support for ES|QL Rule Type and Remote Validation #3281

merged 17 commits into from
Dec 8, 2023

Conversation

Mikaayenson
Copy link
Contributor

@Mikaayenson Mikaayenson commented Nov 20, 2023 •
edited
Loading

Issues

Resolves #3275

Summary

  • DO NOT merge with the example rule
  • Adds support for the ES|QL Rule type based on the kibana implementation
  • Adds a stub unit test to test all ES|QL rules
  • Adds a singleton elasticsearch client since validation occurs per rule when loading all
  • Adds base logic to validate esql rules based on an existing stack.

Testing

  • Core logic was migrated from the on-week POC. See a previous run for example testing.
  • Run the ES|QL unit tests, which has examples for valid and invalid tests.

Additional Information

Increased Scope

We've decided to spend more cycles designing and implementing the remote validation component prior to working on the ECP/persistent stack POCs. The initial design to fully support remote validation is here and will be worked by @brokensound77.

@Mikaayenson Mikaayenson self-assigned this Nov 20, 2023
@Mikaayenson Mikaayenson linked an issue Nov 20, 2023 that may be closed by this pull request
[FR] ES|QL Remote Validation POC #3275
Closed
@botelastic botelastic bot added Domain: Endpoint OS: Windows windows related rules python Internal python for the repository schema labels Nov 20, 2023
Mikaayenson
Mikaayenson commented Nov 20, 2023
View reviewed changes
detection_rules/rule.py Outdated Show resolved Hide resolved
Mikaayenson
Mikaayenson commented Nov 20, 2023
View reviewed changes
detection_rules/rule.py Outdated Show resolved Hide resolved
Mikaayenson
Mikaayenson commented Nov 20, 2023
View reviewed changes
tests/test_all_rules.py Outdated Show resolved Hide resolved
@Mikaayenson Mikaayenson changed the title [FR] Add Support for ES|QL Rule Type WIP [FR] Add Support for ES|QL Rule Type Nov 20, 2023
@terrancedejesus
Copy link
Contributor

Aside from the small comments made, this looks relatively straight forward. @Mikaayenson will there be a second PR to add the github workflow action?

Also, I built a package to ensure the release package workflow does not break with any of the changes in this PR.

@Mikaayenson Mikaayenson changed the title [FR] Add Support for ES|QL Rule Type [FR] Add Support for ES|QL Rule Type and Remote Validation Nov 28, 2023
@Mikaayenson Mikaayenson mentioned this pull request Nov 28, 2023
Closed
@terrancedejesus terrancedejesus mentioned this pull request Nov 29, 2023
Merged
@brokensound77
Merge remote-tracking branch 'upstream/main' into 3275-fr-esql-rule-t…
0fa2786 
…ype-and-core-validation

# Conflicts:
#	detection_rules/rule.py
#	detection_rules/rule_validators.py
#	detection_rules/schemas/definitions.py
#	tests/test_all_rules.py
@brokensound77
resolve conflicts from upstream
25f122c
@brokensound77
set clients in RemoteConnector from auth methods
63401ea
@brokensound77
thread remote rules; add engine test
a43d6bf
@brokensound77
Copy link
Contributor

Slight modifications to the previous diagram

image

This now takes TOMLRuleContents vs the rule data objects.

This currently will skip the unit test until we add a config file (intentionally). This adds capability to remote validate ES|QL and EQL only. If an index doesn't exist on the stack for the rule, it will raise an exception.

Mikaayenson
Mikaayenson commented Dec 7, 2023
View reviewed changes
Copy link
Contributor Author

@Mikaayenson Mikaayenson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Did you ever resolve the intermitted bug with remote eql queries?

detection_rules/misc.py Show resolved Hide resolved
detection_rules/remote_validation.py Show resolved Hide resolved
detection_rules/remote_validation.py Show resolved Hide resolved
detection_rules/remote_validation.py Show resolved Hide resolved
detection_rules/remote_validation.py
Comment on lines +121 to +122
query_results = method(contents)
engine_results = self.engine_preview(contents)
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do both?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is this saying that we validate via the esql api and the rules engine api?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

kind of - definitely the ESQL api, whereas the engine preview api would need further inspection to determine "validation" - parsing and interpreting the results

detection_rules/rule.py Show resolved Hide resolved
detection_rules/rule_validators.py Outdated Show resolved Hide resolved
detection_rules/rule_validators.py Show resolved Hide resolved
tests/test_rules_remote.py
from detection_rules.remote_validation import RemoteValidator


@unittest.skipIf(get_default_config() is None, 'Skipping remote validation due to missing config')
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we should probably skipIf env var is available vs config for CI purposes.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So what I think makes the most sense is that I am goign to just comment this test out for now and in a future PR, we can add the functionality to trigger via a label in a PR

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

#3306

tests/test_specific_rules.py
f"new terms fields values are not unique - {rule.contents.data.new_terms.value}"


class TestESQLRules(BaseRuleTest):
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

note: in the esql library pr, I moved these to a test_esql_rules

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, thanks. I think we should leave these here and consolidate all tests which focus on a subset of rules (vs all) to this file to avoid making a test file per rule type

@brokensound77 brokensound77 mentioned this pull request Dec 8, 2023
Open
@brokensound77
Copy link
Contributor

Delete this file before merging!

removed: 44209c7

brokensound77
brokensound77 approved these changes Dec 8, 2023
View reviewed changes
Copy link
Contributor

@brokensound77 brokensound77 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🚀

terrancedejesus
terrancedejesus approved these changes Dec 8, 2023
View reviewed changes
Copy link
Contributor

@terrancedejesus terrancedejesus left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. I had a couple questions that were already resolved. Functionality appears sound.

eric-forte-elastic
eric-forte-elastic approved these changes Dec 8, 2023
View reviewed changes
Copy link
Contributor

@eric-forte-elastic eric-forte-elastic left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, minor questions all addressed 👍

@brokensound77 brokensound77 merged commit 7514c0a into main Dec 8, 2023
12 checks passed
@brokensound77 brokensound77 deleted the 3275-fr-esql-rule-type-and-core-validation branch December 8, 2023 19:46
protectionsmachine pushed a commit that referenced this pull request Dec 8, 2023
@Mikaayenson @github-actions
[FR] Add Support for ES|QL Rule Type and Remote Validation (#3281)
7bf95fc 
* add suuport for esql type
* add unit tests
* set clients in RemoteConnector from auth methods
* thread remote rules; add engine test
* Add versions to remote validation results

---------

Co-authored-by: Terrance DeJesus <[email protected]>
Co-authored-by: brokensound77 <[email protected]>
Co-authored-by: Justin Ibarra <[email protected]>

(cherry picked from commit 7514c0a)
protectionsmachine pushed a commit that referenced this pull request Dec 8, 2023
@Mikaayenson @github-actions
[FR] Add Support for ES|QL Rule Type and Remote Validation (#3281)
b0c8ae1 
* add suuport for esql type
* add unit tests
* set clients in RemoteConnector from auth methods
* thread remote rules; add engine test
* Add versions to remote validation results

---------

Co-authored-by: Terrance DeJesus <[email protected]>
Co-authored-by: brokensound77 <[email protected]>
Co-authored-by: Justin Ibarra <[email protected]>

(cherry picked from commit 7514c0a)
protectionsmachine pushed a commit that referenced this pull request Dec 8, 2023
@Mikaayenson @github-actions
[FR] Add Support for ES|QL Rule Type and Remote Validation (#3281)
1d53949 
* add suuport for esql type
* add unit tests
* set clients in RemoteConnector from auth methods
* thread remote rules; add engine test
* Add versions to remote validation results

---------

Co-authored-by: Terrance DeJesus <[email protected]>
Co-authored-by: brokensound77 <[email protected]>
Co-authored-by: Justin Ibarra <[email protected]>

(cherry picked from commit 7514c0a)
protectionsmachine pushed a commit that referenced this pull request Dec 8, 2023
@Mikaayenson @github-actions
[FR] Add Support for ES|QL Rule Type and Remote Validation (#3281)
5e3e5ca 
* add suuport for esql type
* add unit tests
* set clients in RemoteConnector from auth methods
* thread remote rules; add engine test
* Add versions to remote validation results

---------

Co-authored-by: Terrance DeJesus <[email protected]>
Co-authored-by: brokensound77 <[email protected]>
Co-authored-by: Justin Ibarra <[email protected]>

(cherry picked from commit 7514c0a)
protectionsmachine pushed a commit that referenced this pull request Dec 8, 2023
@Mikaayenson @github-actions
[FR] Add Support for ES|QL Rule Type and Remote Validation (#3281)
256955e 
* add suuport for esql type
* add unit tests
* set clients in RemoteConnector from auth methods
* thread remote rules; add engine test
* Add versions to remote validation results

---------

Co-authored-by: Terrance DeJesus <[email protected]>
Co-authored-by: brokensound77 <[email protected]>
Co-authored-by: Justin Ibarra <[email protected]>

(cherry picked from commit 7514c0a)
protectionsmachine pushed a commit that referenced this pull request Dec 8, 2023
@Mikaayenson @github-actions
[FR] Add Support for ES|QL Rule Type and Remote Validation (#3281)
2932d33 
* add suuport for esql type
* add unit tests
* set clients in RemoteConnector from auth methods
* thread remote rules; add engine test
* Add versions to remote validation results

---------

Co-authored-by: Terrance DeJesus <[email protected]>
Co-authored-by: brokensound77 <[email protected]>
Co-authored-by: Justin Ibarra <[email protected]>

(cherry picked from commit 7514c0a)
protectionsmachine pushed a commit that referenced this pull request Dec 8, 2023
@Mikaayenson @github-actions
[FR] Add Support for ES|QL Rule Type and Remote Validation (#3281)
eaa2a72 
* add suuport for esql type
* add unit tests
* set clients in RemoteConnector from auth methods
* thread remote rules; add engine test
* Add versions to remote validation results

---------

Co-authored-by: Terrance DeJesus <[email protected]>
Co-authored-by: brokensound77 <[email protected]>
Co-authored-by: Justin Ibarra <[email protected]>

(cherry picked from commit 7514c0a)
protectionsmachine pushed a commit that referenced this pull request Dec 8, 2023
@Mikaayenson @github-actions
[FR] Add Support for ES|QL Rule Type and Remote Validation (#3281)
111ce46 
* add suuport for esql type
* add unit tests
* set clients in RemoteConnector from auth methods
* thread remote rules; add engine test
* Add versions to remote validation results

---------

Co-authored-by: Terrance DeJesus <[email protected]>
Co-authored-by: brokensound77 <[email protected]>
Co-authored-by: Justin Ibarra <[email protected]>

(cherry picked from commit 7514c0a)
protectionsmachine pushed a commit that referenced this pull request Dec 8, 2023
@Mikaayenson @github-actions
[FR] Add Support for ES|QL Rule Type and Remote Validation (#3281)
53f924d 
* add suuport for esql type
* add unit tests
* set clients in RemoteConnector from auth methods
* thread remote rules; add engine test
* Add versions to remote validation results

---------

Co-authored-by: Terrance DeJesus <[email protected]>
Co-authored-by: brokensound77 <[email protected]>
Co-authored-by: Justin Ibarra <[email protected]>

(cherry picked from commit 7514c0a)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@brokensound77 brokensound77 brokensound77 approved these changes

@eric-forte-elastic eric-forte-elastic eric-forte-elastic approved these changes

@terrancedejesus terrancedejesus terrancedejesus approved these changes

Assignees

@brokensound77 brokensound77

Labels
backport: auto Domain: Endpoint enhancement New feature or request esql ES|QL OS: Windows windows related rules python Internal python for the repository schema Team: TRADE
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[FR] ES|QL Remote Validation POC
4 participants
@Mikaayenson @terrancedejesus @brokensound77 @eric-forte-elastic