add field based rules support in correlation engine by sbcd90 · Pull Request #737 · opensearch-project/security-analytics

sbcd90 · 2023-11-28T04:30:43Z

Description

add field based correlation rules support in correlation engine.

e.g., sample correlation rule with group by field

POST /_plugins/_security_analytics/correlation/rules
{
  "correlate": [
    {
      "index": "windows5",
      "field": "SourceIp",
      "category": "windows"
    },
    {
      "index": "vpc_flow1",
      "category": "vpcflow",
      "field": "netflow.srcaddr"
    }
  ],
  "name": "vpcflow to windows",
  "time_window": 300000
}

sample correlation rule with group by field & filter query

{
  "correlate": [
    {
      "index": "cloudtrail-5",
      "query": "eventName:CreateUser",
      "field": "requestParameters.userName",
      "category": "cloudtrail"
    },
    {
      "index": "cloudtrail-5",
      "category": "cloudtrail",
      "query": "eventName:DeleteUser",
      "field": "requestParameters.userName"
    }
  ],
  "name": "cloudtrail create to delete account",
  "time_window": 600000
}

Issues Resolved

[List any issues this PR will resolve]

Check List

New functionality includes testing.
- All tests pass
New functionality has been documented.
- New functionality has javadoc added
Commits are signed per the DCO using --signoff

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.

codecov · 2023-11-28T09:01:44Z

Codecov Report

Attention: 80 lines in your changes are missing coverage. Please review.

Comparison is base (c0f7bd9) 24.73% compared to head (dec7296) 24.62%.

Files	Patch %	Lines
...arch/securityanalytics/correlation/JoinEngine.java	0.00%	52 Missing ⚠️
...arch/securityanalytics/model/CorrelationQuery.java	0.00%	15 Missing ⚠️
...earch/securityanalytics/model/CorrelationRule.java	0.00%	10 Missing ⚠️
...ics/transport/TransportCorrelateFindingAction.java	0.00%	3 Missing ⚠️

Additional details and impacted files

@@             Coverage Diff              @@
##               main     #737      +/-   ##
============================================
- Coverage     24.73%   24.62%   -0.12%     
+ Complexity     1021     1018       -3     
============================================
  Files           275      275              
  Lines         12662    12711      +49     
  Branches       1389     1399      +10     
============================================
- Hits           3132     3130       -2     
- Misses         9264     9314      +50     
- Partials        266      267       +1

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

eirsep · 2023-11-29T18:41:22Z

src/main/java/org/opensearch/securityanalytics/correlation/JoinEngine.java

+            if (enableAutoCorrelations) {
+                generateAutoCorrelations(detector, finding);
+            } else {
+                onAutoCorrelations(detector, finding, Map.of());


this method seems to be search on correlation index

if auto-correlation is disabled why are we invoking this method?

this is the callback when auto correlations complete. so, if auto-correlations are disabled, we directly call the callback without generating auto correlations.

eirsep · 2023-11-29T21:40:54Z

src/main/java/org/opensearch/securityanalytics/correlation/JoinEngine.java

-                    for (CorrelationRule rule: filteredCorrelationRules) {
-                        List<CorrelationQuery> queries = rule.getCorrelationQueries();
+                    Map<String, Long> categoryToTimeWindowMap = new HashMap<>();
+                    for (Triple<CorrelationRule, SearchHit[], String> rule: filteredCorrelationRules) {


nit: why are we not creating pojos. we have increased from list to triple?

eirsep · 2023-11-29T22:55:04Z

src/main/java/org/opensearch/securityanalytics/settings/SecurityAnalyticsSettings.java

    );

+    public static final Setting<Boolean> ENABLE_AUTO_CORRELATIONS = Setting.boolSetting(
+            "plugins.security_analytics.enable_auto_correlations",


rename this to plugins.security_analytics.auto_correlations_enabled

eirsep · 2023-11-29T22:55:11Z

src/main/java/org/opensearch/securityanalytics/settings/SecurityAnalyticsSettings.java


+    public static final Setting<Boolean> ENABLE_AUTO_CORRELATIONS = Setting.boolSetting(
+            "plugins.security_analytics.enable_auto_correlations",
+            false,


why is auto correlations disabled by default?

auto correlations are disabled because they may generate too many irrelevant correlations. we're trying to further filter down auto correlations so that meaningful results are shown.

eirsep · 2023-11-29T23:00:14Z

src/test/java/org/opensearch/securityanalytics/TestHelpers.java

                "level: high";
    }

+    public static String randomRuleForCorrelations(String value) {


nit: rename to randomCloudtrailRuleForCorrelations

eirsep · 2023-11-29T23:04:15Z

src/test/java/org/opensearch/securityanalytics/correlation/CorrelationEngineRestApiIT.java

+        }
+    }
+
+    public void testBasicCorrelationEngineWorkflowWithFieldBasedRulesOnMultipleLogTypes() throws IOException, InterruptedException {


do we support correlation for custom log types?

if yes, can we add a tests for correlation on (custom log types + log types)

added an integ test for it.

eirsep · 2023-11-29T23:04:58Z

src/test/java/org/opensearch/securityanalytics/correlation/CorrelationEngineRestApiIT.java

+                Map<String, Object> responseMap = entityAsMap(response);
+                List<Object> results = (List<Object>) responseMap.get("findings");
+                if (results.size() == 1) {
+                    Assert.assertTrue(true);


why are we not validating field values like finding id or field name etc.

we do not need to in this test as only 1 result is expected. 0 correlations mean error.

eirsep · 2023-11-29T23:05:59Z

Plz elaborate PR description for better understanding of the feature.. typically a github issue with the elaborate description tagged with PR is best practice and good for understanding and tracking

Describe settings introduced and description of setting

Add javadocs for settings and critical sections of code
plz add details in PR description with example of correlation finding document and if there is any change after field based correlation.

eirsep · 2023-11-29T23:09:13Z

src/test/java/org/opensearch/securityanalytics/findings/FindingIT.java

        getFindingsBody = entityAsMap(getFindingsResponse);
        Assert.assertEquals(1, getFindingsBody.get("total_findings"));
    }
-    @Ignore


why did we uncomment
is this test flakiness fixed?

flakiness was fixed by adding this https://github.com/opensearch-project/security-analytics/pull/737/files#diff-20d6f1670be34a4f479880ddef06426f6dc20ff28a153a9225e829eaa325a7f7

eirsep · 2023-11-29T23:13:00Z

src/main/java/org/opensearch/securityanalytics/correlation/JoinEngine.java

-import java.util.function.Predicate;
 import java.util.stream.Collectors;




why is JoinEngine not a "component" injected from createComponents() in Plugin definition? We shouldn't have to parse a setting in transport action and pass it to correlation engine

JoinEngine needs an instance of TransportCorrelateFindingAction. hence, it cannot be part of createComponents().

Signed-off-by: Subhobrata Dey <sbcd90@gmail.com>

eirsep · 2023-12-06T00:15:59Z

src/main/java/org/opensearch/securityanalytics/correlation/JoinEngine.java


-import kotlin.Pair;
+import org.apache.commons.lang3.tuple.Pair;
+import org.apache.commons.lang3.tuple.Triple;


is this import used at all?

eirsep · 2023-12-06T00:29:02Z

src/main/resources/mappings/finding_mapping.json

            }
          }
+        },
+        "fields": {


do we support multiple fields?

opensearch-trigger-bot · 2023-12-06T01:10:07Z

The backport to 2.11-with-features failed:

The process '/usr/bin/git' failed with exit code 1

To backport manually, run these commands in your terminal:

# Fetch latest updates from GitHub
git fetch
# Create a new working tree
git worktree add .worktrees/backport-2.11-with-features 2.11-with-features
# Navigate to the new working tree
cd .worktrees/backport-2.11-with-features
# Create a new branch
git switch --create backport/backport-737-to-2.11-with-features
# Cherry-pick the merged commit of this pull request and resolve the conflicts
git cherry-pick -x --mainline 1 01facfc90ad56f9c7d0a63025c980297b5d352dd
# Push it to GitHub
git push --set-upstream origin backport/backport-737-to-2.11-with-features
# Go back to the original working tree
cd ../..
# Delete the working tree
git worktree remove .worktrees/backport-2.11-with-features

Then, create a pull request where the base branch is 2.11-with-features and the compare/head branch is backport/backport-737-to-2.11-with-features.

opensearch-trigger-bot · 2023-12-06T01:14:59Z

The backport to 2.x failed:

The process '/usr/bin/git' failed with exit code 1

To backport manually, run these commands in your terminal:

# Fetch latest updates from GitHub
git fetch
# Create a new working tree
git worktree add .worktrees/backport-2.x 2.x
# Navigate to the new working tree
cd .worktrees/backport-2.x
# Create a new branch
git switch --create backport/backport-737-to-2.x
# Cherry-pick the merged commit of this pull request and resolve the conflicts
git cherry-pick -x --mainline 1 01facfc90ad56f9c7d0a63025c980297b5d352dd
# Push it to GitHub
git push --set-upstream origin backport/backport-737-to-2.x
# Go back to the original working tree
cd ../..
# Delete the working tree
git worktree remove .worktrees/backport-2.x

Then, create a pull request where the base branch is 2.x and the compare/head branch is backport/backport-737-to-2.x.

…ct#737) * add field based rules support in correlation engine Signed-off-by: Subhobrata Dey <sbcd90@gmail.com> * add field based rules support in correlation engine Signed-off-by: Subhobrata Dey <sbcd90@gmail.com> --------- Signed-off-by: Subhobrata Dey <sbcd90@gmail.com>

* add field based rules support in correlation engine Signed-off-by: Subhobrata Dey <sbcd90@gmail.com> Co-authored-by: Subhobrata Dey <sbcd90@gmail.com>

…ct#737) * add field based rules support in correlation engine Signed-off-by: Subhobrata Dey <sbcd90@gmail.com> * add field based rules support in correlation engine Signed-off-by: Subhobrata Dey <sbcd90@gmail.com> --------- Signed-off-by: Subhobrata Dey <sbcd90@gmail.com>

Signed-off-by: Joanne Wang <jowg@amazon.com>

…ch-project#728) (opensearch-project#737) Signed-off-by: Petar Dzepina <petar.dzepina@gmail.com>

sbcd90 requested review from AWSHurneyt, amsiglan, eirsep, getsaurabh02, lezzago and praveensameneni as code owners November 28, 2023 04:30

sbcd90 marked this pull request as draft November 29, 2023 16:34

eirsep reviewed Nov 29, 2023

View reviewed changes

sbcd90 force-pushed the test456 branch 2 times, most recently from 15d461a to 5d5d6dd Compare November 29, 2023 20:33

sbcd90 marked this pull request as ready for review November 29, 2023 20:58

sbcd90 requested a review from jowg-amazon as a code owner November 29, 2023 20:58

eirsep reviewed Nov 29, 2023

View reviewed changes

sbcd90 mentioned this pull request Nov 29, 2023

add field based rules support in correlation engine #740

Closed

5 tasks

eirsep reviewed Nov 29, 2023

View reviewed changes

eirsep requested changes Nov 29, 2023

View reviewed changes

sbcd90 force-pushed the test456 branch from 45a650e to 27f320f Compare November 30, 2023 02:56

add field based rules support in correlation engine

e2c628a

Signed-off-by: Subhobrata Dey <sbcd90@gmail.com>

sbcd90 force-pushed the test456 branch from 27f320f to b77c67c Compare November 30, 2023 03:30

sbcd90 mentioned this pull request Nov 30, 2023

add field based rules support in correlation engine #754

Merged

5 tasks

eirsep previously approved these changes Nov 30, 2023

View reviewed changes

add field based rules support in correlation engine

dec7296

Signed-off-by: Subhobrata Dey <sbcd90@gmail.com>

sbcd90 dismissed eirsep’s stale review via dec7296 November 30, 2023 22:32

sbcd90 force-pushed the test456 branch from b77c67c to dec7296 Compare November 30, 2023 22:32

praveensameneni approved these changes Dec 6, 2023

View reviewed changes

eirsep reviewed Dec 6, 2023

View reviewed changes

src/main/resources/mappings/finding_mapping.json

}

}

},

"fields": {

Copy link
Copy Markdown

Member

eirsep Dec 6, 2023

Choose a reason for hiding this comment

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

do we support multiple fields?

eirsep approved these changes Dec 6, 2023

View reviewed changes

eirsep merged commit 01facfc into opensearch-project:main Dec 6, 2023

eirsep added the backport 2.11-with-features label Dec 6, 2023

jowg-amazon added the backport 2.x label Dec 6, 2023

jowg-amazon mentioned this pull request Dec 6, 2023

Backport #737 to 2-11-with-features #770

Merged

5 tasks

jowg-amazon mentioned this pull request Jan 18, 2024

[Backport 2.x] #737 add field based rules support in correlation engine #812

Merged

5 tasks

sbcd90 pushed a commit that referenced this pull request Feb 6, 2024

add field based rules support in correlation engine (#737) (#812)

1cf3743

Signed-off-by: Joanne Wang <jowg@amazon.com>

riysaxen-amzn pushed a commit to riysaxen-amzn/security-analytics that referenced this pull request Mar 25, 2024

Added unwrapping exception from core; added more debug logs (opensear…

dc20ba1

…ch-project#728) (opensearch-project#737) Signed-off-by: Petar Dzepina <petar.dzepina@gmail.com>

		import java.util.function.Predicate;
		import java.util.stream.Collectors;

Conversation

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

Uh oh!

Description

Issues Resolved

Check List

Uh oh!

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

Uh oh!

Codecov Report

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!

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!

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

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!

opensearch-trigger-bot bot commented Dec 6, 2023

Uh oh!

opensearch-trigger-bot bot commented Dec 6, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

sbcd90 commented Nov 28, 2023 •

edited

Loading

codecov bot commented Nov 28, 2023 •

edited

Loading

eirsep commented Nov 29, 2023 •

edited

Loading