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

Feature findings enhancemnt #1427

Merged
merged 5 commits into from
Mar 9, 2024
Merged

Feature findings enhancemnt #1427

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
0d24cb4
added support for param in Finding API
riysaxen-amzn Jan 31, 2024
ee792d6
added detectionType as param for Findings API enhancements
riysaxen-amzn Jan 31, 2024
40be752
Merge branch 'opensearch-project:main' into feature-findingsEnhancemnt
riysaxen-amzn Feb 14, 2024
a360cc7
added searchString param in FIndingsAPI
riysaxen-amzn Feb 24, 2024
b9d9211
adding addiional params findingIds, startTime and endTime
riysaxen-amzn Feb 29, 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
6 changes: 5 additions & 1 deletion alerting/src/main/kotlin/org/opensearch/alerting/resthandler/RestGetFindingsAction.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -45,6 +45,8 @@ class RestGetFindingsAction : BaseRestHandler() {
val size = request.paramAsInt("size", 20)
val startIndex = request.paramAsInt("startIndex", 0)
val searchString = request.param("searchString", "")
val severity: String? = request.param("severity", "ALL")
val detectionType: String? = request.param("detectionType", "rules")

val table = Table(
sortOrder,
Expand All @@ -57,7 +59,9 @@ class RestGetFindingsAction : BaseRestHandler() {

val getFindingsSearchRequest = GetFindingsRequest(
findingID,
table
table,
severity,
detectionType
Copy link
Member

Choose a reason for hiding this comment

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

what is detectionType??

Copy link
Member

Choose a reason for hiding this comment

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

we can't use this variable .. this is not a keyword in alerting domain.. if it's for security analytics we need to translate it into a jargon or query searchString which is generalized to Alerting plugin?

)
return RestChannelConsumer {
channel ->
Expand Down
66 changes: 65 additions & 1 deletion alerting/src/main/kotlin/org/opensearch/alerting/transport/TransportGetFindingsAction.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -82,6 +82,9 @@ class TransportGetFindingsSearchAction @Inject constructor(
val getFindingsRequest = request as? GetFindingsRequest
?: recreateObject(request) { GetFindingsRequest(it) }
val tableProp = getFindingsRequest.table
val severity = getFindingsRequest.severity
val detectionType = getFindingsRequest.detectionType
val searchString = tableProp.searchString

val sortBuilder = SortBuilders
.fieldSort(tableProp.sortString)
Expand All @@ -103,12 +106,74 @@ class TransportGetFindingsSearchAction @Inject constructor(
if (!getFindingsRequest.findingId.isNullOrBlank())
queryBuilder.filter(QueryBuilders.termQuery("_id", getFindingsRequest.findingId))

if (!getFindingsRequest.findingIds.isNullOrEmpty()) {
queryBuilder.filter(QueryBuilders.termsQuery("id", getFindingsRequest.findingIds))
}

if (getFindingsRequest.monitorId != null) {
queryBuilder.filter(QueryBuilders.termQuery("monitor_id", getFindingsRequest.monitorId))
} else if (getFindingsRequest.monitorIds.isNullOrEmpty() == false) {
queryBuilder.filter(QueryBuilders.termsQuery("monitor_id", getFindingsRequest.monitorIds))
}

if (getFindingsRequest.startTime != null && getFindingsRequest.endTime != null) {
val startTime = getFindingsRequest.startTime!!.toEpochMilli()
val endTime = getFindingsRequest.endTime!!.toEpochMilli()
val timeRangeQuery = QueryBuilders.rangeQuery("timestamp")
.from(startTime) // Greater than or equal to start time
.to(endTime) // Less than or equal to end time
queryBuilder.filter(timeRangeQuery)
}

if (!detectionType.isNullOrBlank()) {
val nestedQueryBuilder = QueryBuilders.nestedQuery(
"queries",
when {
detectionType.equals("threat", ignoreCase = true) -> {
QueryBuilders.boolQuery().filter(
QueryBuilders.prefixQuery("queries.id", "threat_intel_")
)
}
else -> {
QueryBuilders.boolQuery().mustNot(
QueryBuilders.prefixQuery("queries.id", "threat_intel_")
Comment on lines +138 to +139
Copy link
Collaborator

Choose a reason for hiding this comment

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

What's the reason to filter out the threat intel queries here? As a user I would expect the filter to be based on the provided detectionType

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, I have categorized the detectionType parameter into two types: "threat" and normal rule based. If the frontend API calls the findings API with detectionType set to "threat," all threat intel-related findings will be generated with this query. I'm open to considering alternative solutions for this categorization.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I think the enum comment I had on the other CR makes this a bit more clear. Having the arbitrary text input + looking for exactly 2 cases seems like it should be enum-based.

If we don't expect the detectionType options to expand, then having a boolean field seems more user-friendly to me. Something like getThreatIntelFindingsOnly (name could use some work) that allows the user to pick which type of findings are returned.

Or maybe a new API, say GetThreatIntelFindings, would also make sense to me.

Another thought on this - this change is arguably a breaking one. We are changing the default behavior of an existing API to now filter out the threat intel findings. If a user has a system that relies on this API already and they are expecting threat intel findings, then it would break their use case.

Is there a way we can explore doing the filtering on the frontend, or at least leaving the default behavior as-is? I'm a bit concerned with merging this at the moment, but open to others' thoughts.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Missed line 128.. not a breaking change or change to default behavior since filtering is only applied if a detectionType is supplied

)
}
},
ScoreMode.None
)

// Add the nestedQueryBuilder to the main queryBuilder
queryBuilder.must(nestedQueryBuilder)
}

if (!searchString.isNullOrBlank()) {
queryBuilder
.should(QueryBuilders.matchQuery("index", searchString))
.should(
QueryBuilders.nestedQuery(
"queries",
QueryBuilders.matchQuery("queries.tags", searchString),
ScoreMode.None
)
)
.should(QueryBuilders.regexpQuery("monitor_name", searchString + ".*"))
.minimumShouldMatch(1)
}

if (!severity.isNullOrBlank()) {
queryBuilder
.must(
QueryBuilders.nestedQuery(
"queries",
QueryBuilders.boolQuery().should(
QueryBuilders.matchQuery("queries.tags", severity)
),
ScoreMode.None
)
)
}

if (!tableProp.searchString.isNullOrBlank()) {
queryBuilder
.should(
Expand All @@ -130,7 +195,6 @@ class TransportGetFindingsSearchAction @Inject constructor(
)
)
}

searchSourceBuilder.query(queryBuilder)

client.threadPool().threadContext.stashContext().use {
Expand Down
Loading