diff --git a/alerting/src/main/kotlin/org/opensearch/alerting/AlertingPlugin.kt b/alerting/src/main/kotlin/org/opensearch/alerting/AlertingPlugin.kt index f63406d77..03dbfbac3 100644 --- a/alerting/src/main/kotlin/org/opensearch/alerting/AlertingPlugin.kt +++ b/alerting/src/main/kotlin/org/opensearch/alerting/AlertingPlugin.kt @@ -252,7 +252,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)) @@ -267,7 +267,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)) diff --git a/alerting/src/main/kotlin/org/opensearch/alerting/InputService.kt b/alerting/src/main/kotlin/org/opensearch/alerting/InputService.kt index 5013b1eff..d027600a7 100644 --- a/alerting/src/main/kotlin/org/opensearch/alerting/InputService.kt +++ b/alerting/src/main/kotlin/org/opensearch/alerting/InputService.kt @@ -15,12 +15,15 @@ import org.opensearch.alerting.opensearchapi.suspendUntil 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.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 @@ -44,7 +47,9 @@ class InputService( 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) @@ -197,13 +202,6 @@ class InputService( // 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 @@ -212,7 +210,9 @@ class InputService( // 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()) + } val searchResponse: SearchResponse = client.suspendUntil { client.search(searchRequest, it) } results += searchResponse.convertToMap() } diff --git a/alerting/src/main/kotlin/org/opensearch/alerting/transport/TransportIndexMonitorAction.kt b/alerting/src/main/kotlin/org/opensearch/alerting/transport/TransportIndexMonitorAction.kt index 38d1aed6b..2f3b41b02 100644 --- a/alerting/src/main/kotlin/org/opensearch/alerting/transport/TransportIndexMonitorAction.kt +++ b/alerting/src/main/kotlin/org/opensearch/alerting/transport/TransportIndexMonitorAction.kt @@ -45,6 +45,7 @@ import org.opensearch.alerting.util.AlertingException 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.client.Client import org.opensearch.cluster.service.ClusterService @@ -278,7 +279,9 @@ class TransportIndexMonitorAction @Inject constructor( 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) + } val searchRequest = SearchRequest().indices(".opendistro-anomaly-detectors").source(searchSourceBuilder) client.search( searchRequest, diff --git a/alerting/src/main/kotlin/org/opensearch/alerting/util/AlertingUtils.kt b/alerting/src/main/kotlin/org/opensearch/alerting/util/AlertingUtils.kt index 5255de8c9..33911b216 100644 --- a/alerting/src/main/kotlin/org/opensearch/alerting/util/AlertingUtils.kt +++ b/alerting/src/main/kotlin/org/opensearch/alerting/util/AlertingUtils.kt @@ -9,6 +9,8 @@ import org.apache.logging.log4j.LogManager 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.commons.alerting.model.AggregationResultBucket import org.opensearch.commons.alerting.model.Monitor import org.opensearch.commons.alerting.model.action.Action @@ -38,6 +40,28 @@ fun isValidEmail(email: String): Boolean { 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) + } else if (metaData.persistentSettings().get(settingPath) != null) { + metaData.persistentSettings().getAsBoolean(settingPath, adBackendRoleFilterEnabled) + } else { + adBackendRoleFilterEnabled + } +} + /** Allowed Destinations are ones that are specified in the [DestinationSettings.ALLOW_LIST] setting. */ fun Destination.isAllowed(allowList: List): Boolean = allowList.contains(this.type.value) diff --git a/alerting/src/test/kotlin/org/opensearch/alerting/MonitorRunnerServiceIT.kt b/alerting/src/test/kotlin/org/opensearch/alerting/MonitorRunnerServiceIT.kt index 4ce7dcd23..54842f60c 100644 --- a/alerting/src/test/kotlin/org/opensearch/alerting/MonitorRunnerServiceIT.kt +++ b/alerting/src/test/kotlin/org/opensearch/alerting/MonitorRunnerServiceIT.kt @@ -998,7 +998,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() @@ -1018,14 +1018,14 @@ class MonitorRunnerServiceIT : AlertingRestTestCase() { val searchResult = (output.objectMap("input_results")["results"] as List>).first() @Suppress("UNCHECKED_CAST") val total = searchResult.stringMap("hits")?.get("total") as Map - 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 - 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() @@ -1048,7 +1048,7 @@ class MonitorRunnerServiceIT : AlertingRestTestCase() { val searchResult = (output.objectMap("input_results")["results"] as List>).first() @Suppress("UNCHECKED_CAST") val total = searchResult.stringMap("hits")?.get("total") as Map - 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 assertEquals("Incorrect search result", 0.9, maxAnomalyGrade["value"]) @@ -1074,10 +1074,10 @@ class MonitorRunnerServiceIT : AlertingRestTestCase() { val searchResult = (output.objectMap("input_results")["results"] as List>).first() @Suppress("UNCHECKED_CAST") val total = searchResult.stringMap("hits")?.get("total") as Map - 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 - assertEquals("Incorrect search result", 0.8, maxAnomalyGrade["value"]) + assertEquals("Incorrect search result", 0.9, maxAnomalyGrade["value"]) } } @@ -1097,13 +1097,13 @@ class MonitorRunnerServiceIT : AlertingRestTestCase() { @Suppress("UNCHECKED_CAST") (output["trigger_results"] as HashMap).forEach { _, v -> - assertFalse((v as HashMap)["triggered"] as Boolean) + assertTrue((v as HashMap)["triggered"] as Boolean) } @Suppress("UNCHECKED_CAST") val searchResult = (output.objectMap("input_results")["results"] as List>).first() @Suppress("UNCHECKED_CAST") val total = searchResult.stringMap("hits")?.get("total") as Map - assertEquals("Incorrect search result", 0, total["value"]) + assertEquals("Incorrect search result", 5, total["value"]) } }