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

Check if AD backend role is enabled #968

Merged
merged 2 commits into from
Jul 10, 2023
Merged

Check if AD backend role is enabled #968

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
25df4be
check if ad filter is enabled
amitgalitz Jun 22, 2023
bf2c90f
updated integ tests
amitgalitz Jul 10, 2023
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
4 changes: 2 additions & 2 deletions alerting/src/main/kotlin/org/opensearch/alerting/AlertingPlugin.kt
View file
Open in desktop
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
18 changes: 9 additions & 9 deletions alerting/src/main/kotlin/org/opensearch/alerting/InputService.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,13 +15,16 @@ 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.getADBackendRoleFilterEnabled
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 @@ 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)
Expand Down Expand Up @@ -194,13 +199,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
Expand All @@ -209,7 +207,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 (getADBackendRoleFilterEnabled(clusterService, settings)) {
Copy link
Member

@getsaurabh02 getsaurabh02 Jul 7, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Can getADBackendRoleFilterEnabled should be made more generic like getClientPluginRoleFilter so that we can supply the AD filter path just for that one use but the method itself can be used for other setting check?

Copy link
Member Author

Choose a reason for hiding this comment

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

made this change

addUserBackendRolesFilter(monitor.user, searchRequest.source())
}
val searchResponse: SearchResponse = client.suspendUntil { client.search(searchRequest, it) }
results += searchResponse.convertToMap()
}
Expand Down
5 changes: 4 additions & 1 deletion alerting/src/main/kotlin/org/opensearch/alerting/transport/TransportIndexMonitorAction.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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.getADBackendRoleFilterEnabled
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 @@ 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 (getADBackendRoleFilterEnabled(clusterService, settings)) {
addUserBackendRolesFilter(user, searchSourceBuilder)
}
val searchRequest = SearchRequest().indices(".opendistro-anomaly-detectors").source(searchSourceBuilder)
client.search(
searchRequest,
Expand Down
25 changes: 25 additions & 0 deletions alerting/src/main/kotlin/org/opensearch/alerting/util/AnomalyDetectionUtils.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,8 @@
package org.opensearch.alerting.util

import org.apache.lucene.search.join.ScoreMode
import org.opensearch.cluster.service.ClusterService
import org.opensearch.common.settings.Settings
import org.opensearch.commons.alerting.model.Monitor
import org.opensearch.commons.alerting.model.SearchInput
import org.opensearch.commons.authuser.User
Expand All @@ -32,6 +34,29 @@ fun isADMonitor(monitor: Monitor): Boolean {
return false
}

fun getADBackendRoleFilterEnabled(clusterService: ClusterService, settings: Settings): Boolean {
var adBackendRoleFilterEnabled: Boolean
val metaData = clusterService.state().metadata()
val adFilterString = "plugins.anomaly_detection.filter_by_backend_roles"

// get default value for setting
if (clusterService.clusterSettings.get(adFilterString) != null) {
adBackendRoleFilterEnabled = clusterService.clusterSettings.get(adFilterString).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(adFilterString) != null) {
metaData.transientSettings().getAsBoolean(adFilterString, adBackendRoleFilterEnabled)
} else if (metaData.persistentSettings().get(adFilterString) != null) {
metaData.persistentSettings().getAsBoolean(adFilterString, adBackendRoleFilterEnabled)
} else {
adBackendRoleFilterEnabled
}
}

fun addUserBackendRolesFilter(user: User?, searchSourceBuilder: SearchSourceBuilder): SearchSourceBuilder {
var boolQueryBuilder = BoolQueryBuilder()
val userFieldName = "user"
Expand Down
Loading