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

Check if AD backend role is enabled #968

Merged
merged 2 commits into from
Jul 10, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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 @@ -242,7 +242,7 @@ internal class AlertingPlugin : PainlessExtension, ActionPlugin, ScriptPlugin, R
.registerSettings(settings)
.registerThreadPool(threadPool)
.registerAlertIndices(alertIndices)
.registerInputService(InputService(client, scriptService, namedWriteableRegistry, xContentRegistry))
.registerInputService(InputService(client, scriptService, namedWriteableRegistry, xContentRegistry, clusterService, settings))
.registerTriggerService(TriggerService(scriptService))
.registerAlertService(AlertService(client, xContentRegistry, alertIndices))
.registerDocLevelMonitorQueries(DocLevelMonitorQueries(client, clusterService))
Expand All @@ -257,7 +257,7 @@ internal class AlertingPlugin : PainlessExtension, ActionPlugin, ScriptPlugin, R
.registerSettings(settings)
.registerThreadPool(threadPool)
.registerAlertIndices(alertIndices)
.registerInputService(InputService(client, scriptService, namedWriteableRegistry, xContentRegistry))
.registerInputService(InputService(client, scriptService, namedWriteableRegistry, xContentRegistry, clusterService, settings))
.registerTriggerService(TriggerService(scriptService))
.registerAlertService(AlertService(client, xContentRegistry, alertIndices))
.registerDocLevelMonitorQueries(DocLevelMonitorQueries(client, clusterService))
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -15,13 +15,16 @@
import org.opensearch.alerting.util.AggregationQueryRewriter
import org.opensearch.alerting.util.addUserBackendRolesFilter
import org.opensearch.alerting.util.executeTransportAction
import org.opensearch.alerting.util.getRoleFilterEnabled
import org.opensearch.alerting.util.toMap
import org.opensearch.alerting.util.use
import org.opensearch.alerting.workflow.WorkflowRunContext
import org.opensearch.client.Client
import org.opensearch.cluster.service.ClusterService
import org.opensearch.common.io.stream.BytesStreamOutput
import org.opensearch.common.io.stream.NamedWriteableAwareStreamInput
import org.opensearch.common.io.stream.NamedWriteableRegistry
import org.opensearch.common.settings.Settings
import org.opensearch.common.xcontent.LoggingDeprecationHandler
import org.opensearch.common.xcontent.XContentType
import org.opensearch.commons.alerting.model.ClusterMetricsInput
Expand All @@ -45,7 +48,9 @@
val client: Client,
val scriptService: ScriptService,
val namedWriteableRegistry: NamedWriteableRegistry,
val xContentRegistry: NamedXContentRegistry
val xContentRegistry: NamedXContentRegistry,
val clusterService: ClusterService,
val settings: Settings
) {

private val logger = LogManager.getLogger(InputService::class.java)
Expand Down Expand Up @@ -194,13 +199,6 @@

// Add user role filter for AD result
client.threadPool().threadContext.stashContext().use {
// Currently we have no way to verify if user has AD read permission or not. So we always add user
// role filter here no matter AD backend role filter enabled or not. If we don't add user role filter
// when AD backend filter disabled, user can run monitor on any detector and get anomaly data even
// they have no AD read permission. So if domain disabled AD backend role filter, monitor runner
// still can't get AD result with different user backend role, even the monitor user has permission
// to read AD result. This is a short term solution to trade off between user experience and security.
//
// Possible long term solution:
// 1.Use secure rest client to send request to AD search result API. If no permission exception,
// that mean user has read access on AD result. Then don't need to add user role filter when query
Expand All @@ -209,7 +207,9 @@
// Monitor runner will send transport request to check permission first. If security plugin response
// is yes, user has permission to query AD result. If AD role filter enabled, we will add user role
// filter to protect data at user role level; otherwise, user can query any AD result.
addUserBackendRolesFilter(monitor.user, searchRequest.source())
if (getRoleFilterEnabled(clusterService, settings, "plugins.anomaly_detection.filter_by_backend_roles")) {
addUserBackendRolesFilter(monitor.user, searchRequest.source())

Check warning on line 211 in alerting/src/main/kotlin/org/opensearch/alerting/InputService.kt

View check run for this annotation

Codecov / codecov/patch

alerting/src/main/kotlin/org/opensearch/alerting/InputService.kt#L211

Added line #L211 was not covered by tests
}
val searchResponse: SearchResponse = client.suspendUntil { client.search(searchRequest, it) }
results += searchResponse.convertToMap()
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -45,6 +45,7 @@
import org.opensearch.alerting.util.DocLevelMonitorQueries
import org.opensearch.alerting.util.IndexUtils
import org.opensearch.alerting.util.addUserBackendRolesFilter
import org.opensearch.alerting.util.getRoleFilterEnabled
import org.opensearch.alerting.util.isADMonitor
import org.opensearch.alerting.util.use
import org.opensearch.client.Client
Expand Down Expand Up @@ -269,7 +270,9 @@
request.monitor = request.monitor
.copy(user = User(user.name, user.backendRoles, user.roles, user.customAttNames))
val searchSourceBuilder = SearchSourceBuilder().size(0)
addUserBackendRolesFilter(user, searchSourceBuilder)
if (getRoleFilterEnabled(clusterService, settings, "plugins.anomaly_detection.filter_by_backend_roles")) {
addUserBackendRolesFilter(user, searchSourceBuilder)

Check warning on line 274 in alerting/src/main/kotlin/org/opensearch/alerting/transport/TransportIndexMonitorAction.kt

View check run for this annotation

Codecov / codecov/patch

alerting/src/main/kotlin/org/opensearch/alerting/transport/TransportIndexMonitorAction.kt#L274

Added line #L274 was not covered by tests
}
val searchRequest = SearchRequest().indices(".opendistro-anomaly-detectors").source(searchSourceBuilder)
client.search(
searchRequest,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,8 @@
import org.opensearch.alerting.model.BucketLevelTriggerRunResult
import org.opensearch.alerting.model.destination.Destination
import org.opensearch.alerting.settings.DestinationSettings
import org.opensearch.cluster.service.ClusterService
import org.opensearch.common.settings.Settings
import org.opensearch.common.util.concurrent.ThreadContext
import org.opensearch.commons.alerting.model.AggregationResultBucket
import org.opensearch.commons.alerting.model.Monitor
Expand Down Expand Up @@ -39,6 +41,28 @@
return validEmailPattern.matches(email)
}

fun getRoleFilterEnabled(clusterService: ClusterService, settings: Settings, settingPath: String): Boolean {
var adBackendRoleFilterEnabled: Boolean
val metaData = clusterService.state().metadata()

// get default value for setting
if (clusterService.clusterSettings.get(settingPath) != null) {
adBackendRoleFilterEnabled = clusterService.clusterSettings.get(settingPath).getDefault(settings) as Boolean
} else {
// default setting doesn't exist, so returning false as it means AD plugins isn't in cluster anyway
return false
}

// Transient settings are prioritized so those are checked first.
return if (metaData.transientSettings().get(settingPath) != null) {
metaData.transientSettings().getAsBoolean(settingPath, adBackendRoleFilterEnabled)

Check warning on line 58 in alerting/src/main/kotlin/org/opensearch/alerting/util/AlertingUtils.kt

View check run for this annotation

Codecov / codecov/patch

alerting/src/main/kotlin/org/opensearch/alerting/util/AlertingUtils.kt#L58

Added line #L58 was not covered by tests
} else if (metaData.persistentSettings().get(settingPath) != null) {
metaData.persistentSettings().getAsBoolean(settingPath, adBackendRoleFilterEnabled)

Check warning on line 60 in alerting/src/main/kotlin/org/opensearch/alerting/util/AlertingUtils.kt

View check run for this annotation

Codecov / codecov/patch

alerting/src/main/kotlin/org/opensearch/alerting/util/AlertingUtils.kt#L60

Added line #L60 was not covered by tests
} else {
adBackendRoleFilterEnabled

Check warning on line 62 in alerting/src/main/kotlin/org/opensearch/alerting/util/AlertingUtils.kt

View check run for this annotation

Codecov / codecov/patch

alerting/src/main/kotlin/org/opensearch/alerting/util/AlertingUtils.kt#L62

Added line #L62 was not covered by tests
}
}

/** Allowed Destinations are ones that are specified in the [DestinationSettings.ALLOW_LIST] setting. */
fun Destination.isAllowed(allowList: List<String>): Boolean = allowList.contains(this.type.value)

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -1024,7 +1024,7 @@ class MonitorRunnerServiceIT : AlertingRestTestCase() {
}
}

fun `test execute AD monitor doesn't return search result without user`() {
fun `test execute AD monitor returns search result without user`() {
// TODO: change to REST API call to test security enabled case
if (!securityEnabled()) {
val user = randomUser()
Expand All @@ -1044,14 +1044,14 @@ class MonitorRunnerServiceIT : AlertingRestTestCase() {
val searchResult = (output.objectMap("input_results")["results"] as List<Map<String, Any>>).first()
@Suppress("UNCHECKED_CAST")
val total = searchResult.stringMap("hits")?.get("total") as Map<String, String>
assertEquals("Incorrect search result", 1, total["value"])
assertEquals("Incorrect search result", 5, total["value"])
@Suppress("UNCHECKED_CAST")
val maxAnomalyGrade = searchResult.stringMap("aggregations")?.get("max_anomaly_grade") as Map<String, String>
assertEquals("Incorrect search result", 0.75, maxAnomalyGrade["value"])
assertEquals("Incorrect search result", 0.9, maxAnomalyGrade["value"])
}
}

fun `test execute AD monitor doesn't return search result with empty backend role`() {
fun `test execute AD monitor returns search result with empty backend role`() {
// TODO: change to REST API call to test security enabled case
if (!securityEnabled()) {
val user = randomUser()
Expand All @@ -1074,7 +1074,7 @@ class MonitorRunnerServiceIT : AlertingRestTestCase() {
val searchResult = (output.objectMap("input_results")["results"] as List<Map<String, Any>>).first()
@Suppress("UNCHECKED_CAST")
val total = searchResult.stringMap("hits")?.get("total") as Map<String, String>
assertEquals("Incorrect search result", 1, total["value"])
assertEquals("Incorrect search result", 5, total["value"])
@Suppress("UNCHECKED_CAST")
val maxAnomalyGrade = searchResult.stringMap("aggregations")?.get("max_anomaly_grade") as Map<String, String>
assertEquals("Incorrect search result", 0.9, maxAnomalyGrade["value"])
Expand All @@ -1100,10 +1100,10 @@ class MonitorRunnerServiceIT : AlertingRestTestCase() {
val searchResult = (output.objectMap("input_results")["results"] as List<Map<String, Any>>).first()
@Suppress("UNCHECKED_CAST")
val total = searchResult.stringMap("hits")?.get("total") as Map<String, String>
assertEquals("Incorrect search result", 3, total["value"])
assertEquals("Incorrect search result", 5, total["value"])
@Suppress("UNCHECKED_CAST")
val maxAnomalyGrade = searchResult.stringMap("aggregations")?.get("max_anomaly_grade") as Map<String, String>
assertEquals("Incorrect search result", 0.8, maxAnomalyGrade["value"])
assertEquals("Incorrect search result", 0.9, maxAnomalyGrade["value"])
}
}

Expand All @@ -1123,13 +1123,13 @@ class MonitorRunnerServiceIT : AlertingRestTestCase() {
@Suppress("UNCHECKED_CAST")
(output["trigger_results"] as HashMap<String, Any>).forEach {
_, v ->
assertFalse((v as HashMap<String, Boolean>)["triggered"] as Boolean)
assertTrue((v as HashMap<String, Boolean>)["triggered"] as Boolean)
}
@Suppress("UNCHECKED_CAST")
val searchResult = (output.objectMap("input_results")["results"] as List<Map<String, Any>>).first()
@Suppress("UNCHECKED_CAST")
val total = searchResult.stringMap("hits")?.get("total") as Map<String, String>
assertEquals("Incorrect search result", 0, total["value"])
assertEquals("Incorrect search result", 5, total["value"])
}
}

Expand Down
Loading