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

[DRAFT] Introduces resource permissions for detectors #1400

Draft
wants to merge 7 commits into
base: main
Choose a base branch
from
Draft
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
5 changes: 3 additions & 2 deletions build.gradle
Original file line number Diff line number Diff line change
Expand Up @@ -112,13 +112,14 @@ configurations {

dependencies {
zipArchive group: 'org.opensearch.plugin', name:'opensearch-job-scheduler', version: "${opensearch_build}"
implementation group: 'org.opensearch', name:'opensearch-security-client', version: "${opensearch_build}"
implementation "org.opensearch:opensearch:${opensearch_version}"
compileOnly "org.opensearch.plugin:opensearch-scripting-painless-spi:${opensearch_version}"
compileOnly "org.opensearch:opensearch-job-scheduler-spi:${job_scheduler_version}"
implementation "org.opensearch:common-utils:${common_utils_version}"
implementation "org.opensearch.client:opensearch-rest-client:${opensearch_version}"
compileOnly group: 'com.google.guava', name: 'guava', version:'32.1.3-jre'
compileOnly group: 'com.google.guava', name: 'failureaccess', version:'1.0.2'
// compileOnly group: 'com.google.guava', name: 'failureaccess', version:'1.0.2'
implementation group: 'org.apache.commons', name: 'commons-math3', version: '3.6.1'
implementation group: 'com.google.code.gson', name: 'gson', version: '2.11.0'
implementation group: 'com.yahoo.datasketches', name: 'sketches-core', version: '0.13.4'
Expand Down Expand Up @@ -205,7 +206,7 @@ opensearchplugin {
name 'opensearch-anomaly-detection'
description 'OpenSearch anomaly detector plugin'
classname 'org.opensearch.timeseries.TimeSeriesAnalyticsPlugin'
extendedPlugins = ['lang-painless', 'opensearch-job-scheduler']
extendedPlugins = ['lang-painless', 'opensearch-job-scheduler', 'opensearch-security;optional=true']
}

// Handle case where older versions of esplugin doesn't expose the joda time version it uses
Expand Down
30 changes: 30 additions & 0 deletions src/main/java/org/opensearch/ad/constant/ADResourceScope.java
Original file line number Diff line number Diff line change
@@ -0,0 +1,30 @@
/*
* SPDX-License-Identifier: Apache-2.0
*
* The OpenSearch Contributors require contributions made to
* this file be licensed under the Apache-2.0 license or a
* compatible open source license.
*
* Modifications Copyright OpenSearch Contributors. See
* GitHub history for details.
*/

package org.opensearch.ad.constant;

import org.opensearch.security.spi.resources.ResourceAccessScope;

public enum ADResourceScope implements ResourceAccessScope<ADResourceScope> {
AD_READ_ACCESS("ad_read_access"),
AD_FULL_ACCESS("ad_full_access");

private final String scopeName;

ADResourceScope(String scopeName) {
this.scopeName = scopeName;
}

@Override
public String value() {
return scopeName;
}
}
17 changes: 17 additions & 0 deletions src/main/java/org/opensearch/ad/constant/ConfigConstants.java
Original file line number Diff line number Diff line change
@@ -0,0 +1,17 @@
/*
* SPDX-License-Identifier: Apache-2.0
*
* The OpenSearch Contributors require contributions made to
* this file be licensed under the Apache-2.0 license or a
* compatible open source license.
*
* Modifications Copyright OpenSearch Contributors. See
* GitHub history for details.
*/

package org.opensearch.ad.constant;

public class ConfigConstants {
public static final String OPENSEARCH_RESOURCE_SHARING_ENABLED = "plugins.security.resource_sharing.enabled";
public static final Boolean OPENSEARCH_RESOURCE_SHARING_ENABLED_DEFAULT = true;
}
19 changes: 18 additions & 1 deletion src/main/java/org/opensearch/ad/model/AnomalyDetector.java
Original file line number Diff line number Diff line change
Expand Up @@ -42,6 +42,7 @@
import org.opensearch.core.xcontent.XContentParser;
import org.opensearch.index.query.QueryBuilder;
import org.opensearch.index.query.QueryBuilders;
import org.opensearch.security.spi.resources.Resource;
import org.opensearch.timeseries.annotation.Generated;
import org.opensearch.timeseries.common.exception.ValidationException;
import org.opensearch.timeseries.constant.CommonMessages;
Expand All @@ -66,7 +67,8 @@
* TODO: Will replace detector config mapping in AD task with detector config setting directly \
* in code rather than config it in anomaly-detection-state.json file.
*/
public class AnomalyDetector extends Config {
public class AnomalyDetector extends Config implements Resource {

static class ADShingleGetter implements ShingleGetter {
private Integer seasonIntervals;

Expand Down Expand Up @@ -240,6 +242,21 @@ public AnomalyDetector(
this.rules = rules == null || rules.isEmpty() ? getDefaultRule() : rules;
}

@Override
public String getResourceName() {
return "detector";
}

@Override
public String getWriteableName() {
return "anomaly_detector";
}

@Override
public boolean isFragment() {
return super.isFragment();
}

/*
* For backward compatiblity reason, we cannot use super class
* Config's constructor as we have detectionDateRange and
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,7 @@
import org.opensearch.timeseries.transport.BaseJobTransportAction;
import org.opensearch.transport.TransportService;
import org.opensearch.transport.client.Client;
import org.opensearch.transport.client.node.NodeClient;

public class AnomalyDetectorJobTransportAction extends
BaseJobTransportAction<ADIndex, ADIndexManagement, ADTaskCacheManager, ADTaskType, ADTask, ADTaskManager, AnomalyResult, ADProfileAction, ExecuteADResultResponseRecorder, ADIndexJobActionHandler> {
Expand All @@ -45,7 +46,8 @@ public AnomalyDetectorJobTransportAction(
ClusterService clusterService,
Settings settings,
NamedXContentRegistry xContentRegistry,
ADIndexJobActionHandler adIndexJobActionHandler
ADIndexJobActionHandler adIndexJobActionHandler,
NodeClient nodeClient
) {
super(
transportService,
Expand All @@ -60,7 +62,8 @@ public AnomalyDetectorJobTransportAction(
FAIL_TO_START_DETECTOR,
FAIL_TO_STOP_DETECTOR,
AnomalyDetector.class,
adIndexJobActionHandler
adIndexJobActionHandler,
nodeClient
);
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,7 @@
import org.opensearch.timeseries.transport.BaseDeleteConfigTransportAction;
import org.opensearch.transport.TransportService;
import org.opensearch.transport.client.Client;
import org.opensearch.transport.client.node.NodeClient;

public class DeleteAnomalyDetectorTransportAction extends
BaseDeleteConfigTransportAction<ADTaskCacheManager, ADTaskType, ADTask, ADIndex, ADIndexManagement, ADTaskManager, AnomalyDetector> {
Expand All @@ -43,7 +44,8 @@ public DeleteAnomalyDetectorTransportAction(
Settings settings,
NamedXContentRegistry xContentRegistry,
NodeStateManager nodeStateManager,
ADTaskManager adTaskManager
ADTaskManager adTaskManager,
NodeClient nodeClient
) {
super(
transportService,
Expand All @@ -59,7 +61,8 @@ public DeleteAnomalyDetectorTransportAction(
AnalysisType.AD,
ADCommonName.DETECTION_STATE_INDEX,
AnomalyDetector.class,
ADTaskType.HISTORICAL_DETECTOR_TASK_TYPES
ADTaskType.HISTORICAL_DETECTOR_TASK_TYPES,
nodeClient
);
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -43,6 +43,7 @@
import org.opensearch.timeseries.util.SecurityClientUtil;
import org.opensearch.transport.TransportService;
import org.opensearch.transport.client.Client;
import org.opensearch.transport.client.node.NodeClient;

public class GetAnomalyDetectorTransportAction extends
BaseGetConfigTransportAction<GetAnomalyDetectorResponse, ADTaskCacheManager, ADTaskType, ADTask, ADIndex, ADIndexManagement, ADTaskManager, AnomalyDetector, ADEntityProfileAction, ADEntityProfileRunner, ADTaskProfile, DetectorProfile, ADProfileAction, ADTaskProfileRunner, AnomalyDetectorProfileRunner> {
Expand All @@ -60,7 +61,8 @@ public GetAnomalyDetectorTransportAction(
Settings settings,
NamedXContentRegistry xContentRegistry,
ADTaskManager adTaskManager,
ADTaskProfileRunner adTaskProfileRunner
ADTaskProfileRunner adTaskProfileRunner,
NodeClient nodeClient
) {
super(
transportService,
Expand All @@ -81,7 +83,8 @@ public GetAnomalyDetectorTransportAction(
ADTaskType.HISTORICAL_HC_DETECTOR.name(),
ADTaskType.HISTORICAL_SINGLE_ENTITY.name(),
AnomalyDetectorSettings.AD_FILTER_BY_BACKEND_ROLES,
adTaskProfileRunner
adTaskProfileRunner,
nodeClient
);
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -16,9 +16,11 @@
import static org.opensearch.ad.settings.AnomalyDetectorSettings.AD_FILTER_BY_BACKEND_ROLES;
import static org.opensearch.timeseries.util.ParseUtils.checkFilterByBackendRoles;
import static org.opensearch.timeseries.util.ParseUtils.getConfig;
import static org.opensearch.timeseries.util.ParseUtils.verifyResourceAccessAndProcessRequest;
import static org.opensearch.timeseries.util.RestHandlerUtils.wrapRestActionListener;

import java.util.List;
import java.util.Set;
import java.util.function.Consumer;

import org.apache.logging.log4j.LogManager;
Expand All @@ -27,6 +29,8 @@
import org.opensearch.action.support.ActionFilters;
import org.opensearch.action.support.HandledTransportAction;
import org.opensearch.action.support.WriteRequest;
import org.opensearch.ad.constant.ADResourceScope;
import org.opensearch.ad.constant.ConfigConstants;
import org.opensearch.ad.indices.ADIndexManagement;
import org.opensearch.ad.model.AnomalyDetector;
import org.opensearch.ad.rest.handler.IndexAnomalyDetectorActionHandler;
Expand All @@ -51,6 +55,7 @@
import org.opensearch.timeseries.util.SecurityClientUtil;
import org.opensearch.transport.TransportService;
import org.opensearch.transport.client.Client;
import org.opensearch.transport.client.node.NodeClient;

public class IndexAnomalyDetectorTransportAction extends HandledTransportAction<IndexAnomalyDetectorRequest, IndexAnomalyDetectorResponse> {
private static final Logger LOG = LogManager.getLogger(IndexAnomalyDetectorTransportAction.class);
Expand All @@ -64,6 +69,8 @@ public class IndexAnomalyDetectorTransportAction extends HandledTransportAction<
private volatile Boolean filterByEnabled;
private final SearchFeatureDao searchFeatureDao;
private final Settings settings;
private final boolean resourceSharingEnabled;
private final NodeClient nodeClient;

@Inject
public IndexAnomalyDetectorTransportAction(
Expand All @@ -76,7 +83,8 @@ public IndexAnomalyDetectorTransportAction(
ADIndexManagement anomalyDetectionIndices,
NamedXContentRegistry xContentRegistry,
ADTaskManager adTaskManager,
SearchFeatureDao searchFeatureDao
SearchFeatureDao searchFeatureDao,
NodeClient nodeClient
) {
super(IndexAnomalyDetectorAction.NAME, transportService, actionFilters, IndexAnomalyDetectorRequest::new);
this.client = client;
Expand All @@ -90,6 +98,9 @@ public IndexAnomalyDetectorTransportAction(
filterByEnabled = AnomalyDetectorSettings.AD_FILTER_BY_BACKEND_ROLES.get(settings);
clusterService.getClusterSettings().addSettingsUpdateConsumer(AD_FILTER_BY_BACKEND_ROLES, it -> filterByEnabled = it);
this.settings = settings;
this.resourceSharingEnabled = settings
.getAsBoolean(ConfigConstants.OPENSEARCH_RESOURCE_SHARING_ENABLED, ConfigConstants.OPENSEARCH_RESOURCE_SHARING_ENABLED_DEFAULT);
this.nodeClient = nodeClient;
}

@Override
Expand All @@ -99,7 +110,29 @@ protected void doExecute(Task task, IndexAnomalyDetectorRequest request, ActionL
RestRequest.Method method = request.getMethod();
String errorMessage = method == RestRequest.Method.PUT ? FAIL_TO_UPDATE_DETECTOR : FAIL_TO_CREATE_DETECTOR;
ActionListener<IndexAnomalyDetectorResponse> listener = wrapRestActionListener(actionListener, errorMessage);

try (ThreadContext.StoredContext context = client.threadPool().getThreadContext().stashContext()) {
if (resourceSharingEnabled) {
// Call verifyResourceAccessAndProcessRequest before proceeding
verifyResourceAccessAndProcessRequest(
user,
detectorId,
Set.of(ADResourceScope.AD_FULL_ACCESS.value()),
nodeClient,
settings,
listener,
args -> indexDetector(
user,
detectorId,
method,
listener,
detector -> adExecute(request, user, detector, context, listener)
) // Execute only if access is granted
);
return;
}

// Proceed with normal execution if resource sharing is not enabled
resolveUserAndExecute(user, detectorId, method, listener, (detector) -> adExecute(request, user, detector, context, listener));
} catch (Exception e) {
LOG.error(e);
Expand All @@ -115,42 +148,51 @@ private void resolveUserAndExecute(
Consumer<AnomalyDetector> function
) {
try {
// Check if user has backend roles
// When filter by is enabled, block users creating/updating detectors who do not have backend roles.
if (filterByEnabled) {
String error = checkFilterByBackendRoles(requestedUser);
if (error != null) {
listener.onFailure(new TimeSeriesException(error));
return;
}
}
if (method == RestRequest.Method.PUT) {
// requestedUser == null means security is disabled or user is superadmin. In this case we don't need to
// check if request user have access to the detector or not. But we still need to get current detector for
// this case, so we can keep current detector's user data.
boolean filterByBackendRole = requestedUser == null ? false : filterByEnabled;
// Update detector request, check if user has permissions to update the detector
// Get detector and verify backend roles
getConfig(
requestedUser,
detectorId,
listener,
function,
client,
clusterService,
xContentRegistry,
filterByBackendRole,
AnomalyDetector.class
);
} else {
// Create Detector. No need to get current detector.
function.accept(null);
}

indexDetector(requestedUser, detectorId, method, listener, function);
} catch (Exception e) {
listener.onFailure(e);
}
}

private void indexDetector(
User requestedUser,
String detectorId,
RestRequest.Method method,
ActionListener<IndexAnomalyDetectorResponse> listener,
Consumer<AnomalyDetector> function
) {
if (method == RestRequest.Method.PUT) {
// requestedUser == null means security is disabled or user is superadmin. In this case we don't need to
// check if request user have access to the detector or not. But we still need to get current detector for
// this case, so we can keep current detector's user data.
boolean filterByBackendRole = requestedUser == null ? false : filterByEnabled;
// Update detector request, check if user has permissions to update the detector
// Get detector and verify backend roles
getConfig(
requestedUser,
detectorId,
listener,
function,
client,
clusterService,
xContentRegistry,
filterByBackendRole,
AnomalyDetector.class
);
} else {
// Create Detector. No need to get current detector.
function.accept(null);
}
}

protected void adExecute(
IndexAnomalyDetectorRequest request,
User user,
Expand All @@ -175,6 +217,8 @@ protected void adExecute(
checkIndicesAndExecute(detector.getIndices(), () -> {
// Don't replace detector's user when update detector
// Github issue: https://github.com/opensearch-project/anomaly-detection/issues/124
// TODO this and similar code should be updated to remove reference to a user

User detectorUser = currentDetector == null ? user : currentDetector.getUser();
IndexAnomalyDetectorActionHandler indexAnomalyDetectorActionHandler = new IndexAnomalyDetectorActionHandler(
clusterService,
Expand Down
Loading
Loading