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

fix for detectors with sigma aggregation rules #1372

Merged
merged 2 commits into from
Oct 23, 2024
Merged

fix for detectors with sigma aggregation rules #1372

merged 2 commits into from
Oct 23, 2024

Conversation

sbcd90
Copy link
Collaborator

@sbcd90 sbcd90 commented Oct 22, 2024

Description

fix for detectors with sigma aggregation rules

Related Issues

Resolves #[Issue number to be closed when this PR is merged]

Check List

  • New functionality includes testing.
  • New functionality has been documented.
  • API changes companion pull request created.
  • Commits are signed per the DCO using --signoff.
  • Public documentation issue/PR created.

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.

eirsep
eirsep reviewed Oct 23, 2024
View reviewed changes
src/test/java/org/opensearch/securityanalytics/resthandler/DetectorMonitorRestApiIT.java Outdated
@@ -2153,6 +2157,109 @@ public void testCreateDetectorWithCloudtrailAggrRuleWithEcsFields() throws IOExc
assertEquals(1, getFindingsBody.get("total_findings"));
}

@SuppressWarnings("unchecked")
public void testCreateDetectorWithCloudtrailAggrRuleWithRolloverIndexAliases() throws IOException, InterruptedException {
updateClusterSetting("plugins.security_analytics.enable_detectors_with_dedicated_query_indices", "false");
Copy link
Member

Choose a reason for hiding this comment

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

why do we turn off this setting?

Copy link
Member

Choose a reason for hiding this comment

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

plz change to true as that's the new default behaviour

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

yes. sorry. leftover from some experiments.

eirsep
eirsep reviewed Oct 23, 2024
View reviewed changes
src/test/java/org/opensearch/securityanalytics/resthandler/DetectorMonitorRestApiIT.java
assertEquals(1, getFindingsBody.get("total_findings"));

doRollover("ocsf_ct");
Thread.sleep(90000);
Copy link
Member

Choose a reason for hiding this comment

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

why should we do sleep?
after rollover is the template not applied to new index directly?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The Sigma Aggregation Rule we're using has timeframe set to 1m(

security-analytics/src/test/java/org/opensearch/securityanalytics/TestHelpers.java

Line 1162 in 622d26e

" timeframe: 1m\n" +
). So, the sleep ensures that the rule only considers docs from the rolled over new write index. If this fix is not present, the alerting workflow run doesn't generate the new finding.

security-analytics/src/test/java/org/opensearch/securityanalytics/resthandler/DetectorMonitorRestApiIT.java

Line 2251 in 622d26e

executeAlertingWorkflow(workflowId, Collections.emptyMap());

eirsep
eirsep reviewed Oct 23, 2024
View reviewed changes
src/test/java/org/opensearch/securityanalytics/resthandler/DetectorMonitorRestApiIT.java
doRollover("ocsf_ct");
Thread.sleep(90000);

indexDoc("ocsf_ct", "4", randomCloudtrailOcsfDoc());
Copy link
Member

Choose a reason for hiding this comment

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

before this should we not verify mappings of new write index?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

mappings of the first write index are verified with the first workflow run.

security-analytics/src/test/java/org/opensearch/securityanalytics/resthandler/DetectorMonitorRestApiIT.java

Line 2235 in 622d26e

executeAlertingWorkflow(workflowId, Collections.emptyMap());
.If alias mappings are not applied, findings will not be generated.

Copy link
Member

Choose a reason for hiding this comment

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

But i thought the bug was that AFTER rollover the template is not getting applied?

eirsep
eirsep reviewed Oct 23, 2024
View reviewed changes
Copy link
Member

@eirsep eirsep left a comment

Choose a reason for hiding this comment

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

when is template applied? not at creation time? why is there sleep 90 s in test?

@sbcd90
Copy link
Collaborator Author

sbcd90 commented Oct 23, 2024

when is template applied? not at creation time? why is there sleep 90 s in test?

Yes. Template is applied at creation time with the Create Mapping api call.

@sbcd90
fix comment
70c6f09 
Signed-off-by: Subhobrata Dey <[email protected]>
riysaxen-amzn
riysaxen-amzn reviewed Oct 23, 2024
View reviewed changes
src/main/java/org/opensearch/securityanalytics/mapper/IndexTemplateManager.java
@@ -115,13 +115,22 @@ public void upsertIndexTemplateWithAliasMappings(

upsertComponentTemplateStepListener.whenComplete( acknowledgedResponse -> {

// Find template which matches input index best
// Find template which matches input index best. starts by directly matching with input index and
Copy link
Collaborator

Choose a reason for hiding this comment

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

let's use doc comments for these lines instead -> https://www.oracle.com/technical-resources/articles/java/javadoc-tool.html

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

will address this in a follow-up pr.

@sbcd90 sbcd90 merged commit 6f543b5 into opensearch-project:main Oct 23, 2024
8 of 12 checks passed
opensearch-trigger-bot bot pushed a commit that referenced this pull request Oct 23, 2024
@github-actions
fix for detectors with sigma aggregation rules (#1372)
f99a107 
Signed-off-by: Subhobrata Dey <[email protected]>
(cherry picked from commit 6f543b5)
Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
@opensearch-trigger-bot opensearch-trigger-bot bot mentioned this pull request Oct 23, 2024
Merged
opensearch-trigger-bot bot pushed a commit that referenced this pull request Oct 23, 2024
@github-actions
fix for detectors with sigma aggregation rules (#1372)
48fcca8 
Signed-off-by: Subhobrata Dey <[email protected]>
(cherry picked from commit 6f543b5)
Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
@opensearch-trigger-bot opensearch-trigger-bot bot mentioned this pull request Oct 23, 2024
Merged
opensearch-trigger-bot bot pushed a commit that referenced this pull request Oct 23, 2024
@github-actions
fix for detectors with sigma aggregation rules (#1372)
a5580d6 
Signed-off-by: Subhobrata Dey <[email protected]>
(cherry picked from commit 6f543b5)
Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
@opensearch-trigger-bot opensearch-trigger-bot bot mentioned this pull request Oct 23, 2024
Merged
sbcd90 pushed a commit that referenced this pull request Oct 23, 2024
@opensearch-trigger-bot @github-actions
fix for detectors with sigma aggregation rules (#1372) (#1376)
c15d54e 
(cherry picked from commit 6f543b5)

Signed-off-by: Subhobrata Dey <[email protected]>
Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
sbcd90 pushed a commit that referenced this pull request Oct 23, 2024
@opensearch-trigger-bot @github-actions
fix for detectors with sigma aggregation rules (#1372) (#1375)
bf831f3 
(cherry picked from commit 6f543b5)

Signed-off-by: Subhobrata Dey <[email protected]>
Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
sbcd90 pushed a commit that referenced this pull request Oct 23, 2024
@opensearch-trigger-bot @github-actions
fix for detectors with sigma aggregation rules (#1372) (#1374)
94562be 
(cherry picked from commit 6f543b5)

Signed-off-by: Subhobrata Dey <[email protected]>
Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@eirsep eirsep eirsep approved these changes

@riysaxen-amzn riysaxen-amzn riysaxen-amzn approved these changes

@amsiglan amsiglan Awaiting requested review from amsiglan amsiglan is a code owner

@AWSHurneyt AWSHurneyt Awaiting requested review from AWSHurneyt AWSHurneyt is a code owner

@getsaurabh02 getsaurabh02 Awaiting requested review from getsaurabh02 getsaurabh02 is a code owner

@lezzago lezzago Awaiting requested review from lezzago lezzago is a code owner

@praveensameneni praveensameneni Awaiting requested review from praveensameneni praveensameneni is a code owner

@jowg-amazon jowg-amazon Awaiting requested review from jowg-amazon jowg-amazon is a code owner

@engechas engechas Awaiting requested review from engechas engechas is a code owner

@goyamegh goyamegh Awaiting requested review from goyamegh goyamegh is a code owner

Assignees
No one assigned
Labels
backport 2.x backport 2.15 backport 2.17
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@sbcd90 @eirsep @riysaxen-amzn