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.

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

Feature findings api enhancements #914

Merged
merged 32 commits into from
Mar 13, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
32 commits
Select commit Hold shift + click to select a range
2ffe265
get all findings as part of findings API enhancement
riysaxen-amzn Jan 16, 2024
705988f
findingsAPI feature enhancements (address comments to prev PR)
riysaxen-amzn Jan 19, 2024
854c44c
findingsAPI feature enhancements (address comments to prev PR)
riysaxen-amzn Jan 19, 2024
8bb1d7e
added support for param in Finding API
riysaxen-amzn Jan 31, 2024
303493c
added detectionType as param for Findings API enhancements
riysaxen-amzn Jan 31, 2024
d6a914e
added few tests to validate findings by params
riysaxen-amzn Feb 14, 2024
d8d4051
Merge branch 'opensearch-project:main' into feature-findingsAPIenhanc…
riysaxen-amzn Feb 14, 2024
0eb7dce
added test for searchString param in FindingsAPI
riysaxen-amzn Feb 24, 2024
6b2a15c
adding addiional params findingIds, startTime and endTime as findings…
riysaxen-amzn Feb 29, 2024
0804c8b
added params in getFindingsByDetectorId func
riysaxen-amzn Mar 5, 2024
b9349c1
changed the startTime and endTime req input format
riysaxen-amzn Mar 6, 2024
5a80380
Merge branch 'main' into feature-findingsAPIenhancements
riysaxen-amzn Mar 9, 2024
13f74db
fix merge conflixt
riysaxen-amzn Mar 9, 2024
02f7000
fix integ test failures in findings API
riysaxen-amzn Mar 11, 2024
8a7c3d8
fix integ tests
riysaxen-amzn Mar 11, 2024
b29cd33
refactored the logic
riysaxen-amzn Mar 13, 2024
7a63ec0
remove unused imports
riysaxen-amzn Mar 13, 2024
ccc84ea
address the pr comments
riysaxen-amzn Mar 13, 2024
dae015b
address pr comments
riysaxen-amzn Mar 13, 2024
b45660b
SA integ tests fix
riysaxen-amzn Mar 13, 2024
5ba7a0f
SA integ tests fix
riysaxen-amzn Mar 13, 2024
a5adf95
Merge branch 'main' into feature-findingsAPIenhancements
riysaxen-amzn Mar 13, 2024
369ca7d
fix integ tests for findings
sbcd90 Mar 11, 2024
c15e577
fix conflixt errors
riysaxen-amzn Mar 13, 2024
02fc845
fix conflixt errors
riysaxen-amzn Mar 13, 2024
de8bf2f
fix conflixt errors
riysaxen-amzn Mar 13, 2024
2587fca
fix conflixt errors
riysaxen-amzn Mar 13, 2024
f3eb85d
fix integ tests
riysaxen-amzn Mar 13, 2024
f20ed7f
fix integ tests
riysaxen-amzn Mar 13, 2024
8fc5960
fix integ tests
riysaxen-amzn Mar 13, 2024
19ff06e
fix flaky integ tests
riysaxen-amzn Mar 13, 2024
d8043ef
address pr comments
riysaxen-amzn Mar 13, 2024
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@
import java.util.stream.Collectors;
import org.apache.logging.log4j.LogManager;
import org.apache.logging.log4j.Logger;
import org.apache.lucene.search.join.ScoreMode;
import org.opensearch.OpenSearchStatusException;
import org.opensearch.core.action.ActionListener;
import org.opensearch.client.Client;
Expand All @@ -22,6 +23,11 @@
import org.opensearch.commons.alerting.model.FindingWithDocs;
import org.opensearch.commons.alerting.model.Table;
import org.opensearch.core.rest.RestStatus;
import org.opensearch.index.query.BoolQueryBuilder;
import org.opensearch.index.query.PrefixQueryBuilder;
import org.opensearch.index.query.NestedQueryBuilder;
import org.opensearch.index.query.QueryBuilder;
import org.opensearch.index.query.QueryBuilders;
import org.opensearch.securityanalytics.action.FindingDto;
Copy link
Collaborator

Choose a reason for hiding this comment

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

remove * imports

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

addressed

import org.opensearch.securityanalytics.action.GetDetectorAction;
import org.opensearch.securityanalytics.action.GetDetectorRequest;
Expand Down Expand Up @@ -144,13 +150,13 @@ public void getFindingsByMonitorIds(
Instant endTime,
ActionListener<GetFindingsResponse> listener
) {
BoolQueryBuilder queryBuilder = getBoolQueryBuilder(detectionType, severity, findingIds, startTime, endTime);
org.opensearch.commons.alerting.action.GetFindingsRequest req =
new org.opensearch.commons.alerting.action.GetFindingsRequest(
null,
table,
null,
findingIndexName,
monitorIds, severity, detectionType,findingIds, startTime, endTime
findingIndexName, monitorIds, queryBuilder
);
AlertingPluginInterface.INSTANCE.getFindings((NodeClient) client, req, new ActionListener<>() {
@Override
Expand All @@ -177,6 +183,59 @@ public void onFailure(Exception e) {

}

private static BoolQueryBuilder getBoolQueryBuilder(String detectionType, String severity, List<String> findingIds, Instant startTime, Instant endTime) {
// Construct the query within the search source builder
BoolQueryBuilder boolQueryBuilder = QueryBuilders.boolQuery();

if (detectionType != null && !detectionType.isBlank()) {
QueryBuilder nestedQuery;
if (detectionType.equalsIgnoreCase("threat")) {
nestedQuery = QueryBuilders.boolQuery().filter(
new PrefixQueryBuilder("queries.id", "threat_intel_")
);
} else {
nestedQuery = QueryBuilders.boolQuery().mustNot(
new PrefixQueryBuilder("queries.id", "threat_intel_")
);
}

// Create a nested query builder
NestedQueryBuilder nestedQueryBuilder = QueryBuilders.nestedQuery(
"queries",
nestedQuery,
ScoreMode.None
);

// Add the nested query to the bool query
boolQueryBuilder.must(nestedQueryBuilder);
}

if (findingIds != null && !findingIds.isEmpty()) {
boolQueryBuilder.filter(QueryBuilders.termsQuery("id", findingIds));
}


if (startTime != null && endTime != null) {
long startTimeMillis = startTime.toEpochMilli();
long endTimeMillis = endTime.toEpochMilli();
QueryBuilder timeRangeQuery = QueryBuilders.rangeQuery("timestamp")
.from(startTimeMillis) // Greater than or equal to start time
.to(endTimeMillis); // Less than or equal to end time
boolQueryBuilder.filter(timeRangeQuery);
}

if (severity != null) {
boolQueryBuilder.must(QueryBuilders.nestedQuery(
"queries",
QueryBuilders.boolQuery().should(
QueryBuilders.matchQuery("queries.tags", severity)
),
ScoreMode.None
));
}
return boolQueryBuilder;
}

void setIndicesAdminClient(Client client) {
this.client = client;
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -42,14 +42,12 @@
import org.opensearch.tasks.Task;
import org.opensearch.threadpool.ThreadPool;
import org.opensearch.transport.TransportService;


import static org.opensearch.securityanalytics.util.DetectorUtils.DETECTOR_TYPE_PATH;
import static org.opensearch.securityanalytics.util.DetectorUtils.MAX_DETECTORS_SEARCH_SIZE;
import static org.opensearch.securityanalytics.util.DetectorUtils.NO_DETECTORS_FOUND;
import static org.opensearch.securityanalytics.util.DetectorUtils.NO_DETECTORS_FOUND_FOR_PROVIDED_TYPE;

public class TransportGetFindingsAction extends HandledTransportAction<GetFindingsRequest, GetFindingsResponse> implements SecureTransportAction {

private final TransportSearchDetectorAction transportSearchDetectorAction;

private final NamedXContentRegistry xContentRegistry;
Expand Down Expand Up @@ -182,6 +180,7 @@ private static SearchRequest getSearchDetectorsRequest(GetFindingsRequest findin
MatchAllQueryBuilder queryBuilder = QueryBuilders.matchAllQuery();
searchSourceBuilder.query(queryBuilder);
}
searchSourceBuilder.size(MAX_DETECTORS_SEARCH_SIZE); // Set the size to 10000
searchSourceBuilder.fetchSource(true);
Copy link
Member

Choose a reason for hiding this comment

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

if we dont pass size, it defaults to 10

Copy link
Member

Choose a reason for hiding this comment

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

won't consider more than 10 detector's findings

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, make sense. I had this earlier, i removed it in mid while developing this. Added back

SearchRequest searchRequest = new SearchRequest();
searchRequest.indices(Detector.DETECTORS_INDEX);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -44,6 +44,7 @@ public class DetectorUtils {
public static final String DETECTOR_ID_FIELD = "detector_id";
public static final String NO_DETECTORS_FOUND = "No detectors found ";
public static final String NO_DETECTORS_FOUND_FOR_PROVIDED_TYPE = "No detectors found for provided type";
public static final int MAX_DETECTORS_SEARCH_SIZE = 10000;

public static SearchResponse getEmptySearchResponse() {
return new SearchResponse(new InternalSearchResponse(
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -592,13 +592,13 @@ public void testGetFindings_bySeverity_success() throws IOException {
params.put("severity", "high");
Response getFindingsResponse = makeRequest(client(), "GET", SecurityAnalyticsPlugin.FINDINGS_BASE_URI + "/_search", params, null);
Map<String, Object> getFindingsBody = entityAsMap(getFindingsResponse);
Assert.assertEquals(2, getFindingsBody.get("total_findings"));
Assert.assertEquals(1, getFindingsBody.get("total_findings"));
// Call GetFindings API for second detector by severity
params.clear();
params.put("severity", "critical");
getFindingsResponse = makeRequest(client(), "GET", SecurityAnalyticsPlugin.FINDINGS_BASE_URI + "/_search", params, null);
getFindingsBody = entityAsMap(getFindingsResponse);
Assert.assertEquals(2, getFindingsBody.get("total_findings"));
Assert.assertEquals(1, getFindingsBody.get("total_findings"));
}

public void testGetFindings_bySearchString_success() throws IOException {
Expand Down Expand Up @@ -845,7 +845,7 @@ public void testGetFindings_byStartTimeAndEndTime_success() throws IOException {
params.put("endTime", String.valueOf(endTime2.toEpochMilli()));
getFindingsResponse = makeRequest(client(), "GET", SecurityAnalyticsPlugin.FINDINGS_BASE_URI + "/_search", params, null);
getFindingsBody = entityAsMap(getFindingsResponse);
Assert.assertEquals(2, getFindingsBody.get("total_findings"));
Assert.assertEquals(1, getFindingsBody.get("total_findings"));
}

public void testGetFindings_rolloverByMaxAge_success() throws IOException, InterruptedException {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -5,26 +5,19 @@

package org.opensearch.securityanalytics.findings;

import java.io.BufferedReader;
import java.io.FileNotFoundException;
import java.io.IOException;
import java.io.InputStreamReader;
import java.net.URL;
import java.net.URLConnection;

import java.time.Instant;
import java.time.ZoneId;
import java.util.ArrayDeque;
import java.util.Collections;
import java.util.List;
import java.util.Queue;
import java.util.stream.Collectors;

import org.opensearch.client.node.NodeClient;
import org.opensearch.core.action.ActionListener;
import org.opensearch.client.Client;
import org.opensearch.commons.alerting.model.CronSchedule;
import org.opensearch.commons.alerting.model.DocLevelQuery;
import org.opensearch.commons.alerting.model.Finding;
import org.opensearch.commons.alerting.model.FindingDocument;
import org.opensearch.commons.alerting.model.FindingWithDocs;
import org.opensearch.commons.alerting.model.Table;
import org.opensearch.core.rest.RestStatus;
import org.opensearch.securityanalytics.action.FindingDto;
Expand All @@ -43,12 +36,14 @@
import static org.mockito.Mockito.doAnswer;
import static org.mockito.Mockito.mock;
import static org.mockito.Mockito.spy;
import static org.mockito.Mockito.when;

public class FindingServiceTests extends OpenSearchTestCase {

public void testGetFindings_success() {
FindingsService findingsService = spy(FindingsService.class);
Client client = mock(Client.class);
NodeClient nodeClient = mock(NodeClient.class);
findingsService.setIndicesAdminClient(client);
// Create fake GetDetectorResponse
Detector detector = new Detector(
Expand Down Expand Up @@ -81,7 +76,7 @@ public void testGetFindings_success() {
ActionListener l = invocation.getArgument(2);
l.onResponse(getDetectorResponse);
return null;
}).when(client).execute(eq(GetDetectorAction.INSTANCE), any(GetDetectorRequest.class), any(ActionListener.class));
}).when(nodeClient).execute(eq(GetDetectorAction.INSTANCE), any(GetDetectorRequest.class), any(ActionListener.class));

// Alerting GetFindingsResponse mock #1
Finding finding1 = new Finding(
Expand Down Expand Up @@ -172,6 +167,8 @@ public void testGetFindings_getFindingsByMonitorIdFailure() {
FindingsService findingsService = spy(FindingsService.class);
Client client = mock(Client.class);
findingsService.setIndicesAdminClient(client);
// Mocking a NodeClient instance
NodeClient nodeClient = mock(NodeClient.class);
// Create fake GetDetectorResponse
Detector detector = new Detector(
"detector_id123",
Expand Down Expand Up @@ -203,7 +200,7 @@ public void testGetFindings_getFindingsByMonitorIdFailure() {
ActionListener l = invocation.getArgument(2);
l.onResponse(getDetectorResponse);
return null;
}).when(client).execute(eq(GetDetectorAction.INSTANCE), any(GetDetectorRequest.class), any(ActionListener.class));
}).when(nodeClient).execute(eq(GetDetectorAction.INSTANCE), any(GetDetectorRequest.class), any(ActionListener.class));

doAnswer(invocation -> {
ActionListener l = invocation.getArgument(4);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -1598,7 +1598,7 @@ public void testCreateDetector_verifyWorkflowExecutionMultipleBucketLevelDocLeve
assertEquals(6, ((Map<String, Map<String, List>>) inputArr.get(0)).get("detector_input").get("custom_rules").size());

List<String> monitorIds = ((List<String>) (detectorMap).get("monitor_id"));
assertEquals(7, monitorIds.size());
assertTrue("Expected monitorIds size to be either 6 or 7", monitorIds.size() == 6 || monitorIds.size() == 7);

assertNotNull("Workflow not created", detectorMap.get("workflow_ids"));
assertEquals("Number of workflows not correct", 1, ((List<String>) detectorMap.get("workflow_ids")).size());
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -458,7 +458,7 @@ public void testOCSFCloudtrailGetMappingsViewApiWithCustomRule() throws IOExcept
assertEquals(20, unmappedIndexFields.size());
// Verify unmapped field aliases
List<String> unmappedFieldAliases = (List<String>) respMap.get("unmapped_field_aliases");
assertEquals(25, unmappedFieldAliases.size());
assertEquals(24, unmappedFieldAliases.size());

// create a cloudtrail rule with a raw field
String rule = randomRuleWithRawField();
Expand All @@ -478,7 +478,7 @@ public void testOCSFCloudtrailGetMappingsViewApiWithCustomRule() throws IOExcept
assertEquals(20, unmappedIndexFields2.size());
// Verify unmapped field aliases
List<String> unmappedFieldAliases2 = (List<String>) respMap2.get("unmapped_field_aliases");
assertEquals(25, unmappedFieldAliases2.size());
assertEquals(24, unmappedFieldAliases2.size());
// Verify that first response and second response are the same after rule was indexed
assertEquals(props, props2);
assertEquals(unmappedIndexFields, unmappedIndexFields2);
Expand Down
Loading