Skip to content

Commit ee8e38d

Browse files
DarshitChanpuraDarshitChanpura
authored
Introduces resource permissions for detectors (#1400)
* Introduces resource sharing model as a feature flag Signed-off-by: Darshit Chanpura <dchanp@amazon.com> * Introduces resource sharing model as a feature flag Signed-off-by: Darshit Chanpura <dchanp@amazon.com> * Updates build.gradle Signed-off-by: Darshit Chanpura <dchanp@amazon.com> * Updates method signature to conform to client Signed-off-by: Darshit Chanpura <dchanp@amazon.com> * Resolves jar hell Signed-off-by: Darshit Chanpura <dchanp@amazon.com> * Updates gradle dependency and conforms to changes in client Signed-off-by: Darshit Chanpura <dchanp@amazon.com> * Upgrades qualifier to beta1 Signed-off-by: Darshit Chanpura <dchanp@amazon.com> * Updates to security SPI changes Signed-off-by: Darshit Chanpura <dchanp@amazon.com> * Registers forecaster as resource Signed-off-by: Darshit Chanpura <dchanp@amazon.com> * Consumes feature flag exposed by SPI Signed-off-by: Darshit Chanpura <dchanp@amazon.com> * Aligns with main Signed-off-by: Darshit Chanpura <dchanp@amazon.com> * Adds changelog entry Signed-off-by: Darshit Chanpura <dchanp@amazon.com> * Adds todo to remove feature flag once feature is GA Signed-off-by: Darshit Chanpura <dchanp@amazon.com> * Addresses PR comments Signed-off-by: Darshit Chanpura <dchanp@amazon.com> * Adds filterByBackendRole control to resource sharing feature Signed-off-by: Darshit Chanpura <dchanp@amazon.com> * Fixes a compilation and a runtime bug Signed-off-by: Darshit Chanpura <dchanp@amazon.com> * Fix getName Signed-off-by: Darshit Chanpura <dchanp@amazon.com> * Skips verification of permission on creation Signed-off-by: Darshit Chanpura <dchanp@amazon.com> * Conforms to SPI changes Signed-off-by: Darshit Chanpura <dchanp@amazon.com> * Conforms to SPI changes in security plugin Signed-off-by: Darshit Chanpura <dchanp@amazon.com> * Address PR comments Signed-off-by: Darshit Chanpura <dchanp@amazon.com> * Reflects changes in SPI Signed-off-by: Darshit Chanpura <dchanp@amazon.com> * Fix index name Signed-off-by: Darshit Chanpura <dchanp@amazon.com> * Adds integ tests when resource-sharing feature-flag is enabled Signed-off-by: Darshit Chanpura <dchanp@amazon.com> * Refactored unwanted changes Signed-off-by: Darshit Chanpura <dchanp@amazon.com> * Fixes onResponse in actionHandler Signed-off-by: Darshit Chanpura <dchanp@amazon.com> * Adds unit tests for resource-sharing related files and updates secure-rest-test-case Signed-off-by: Darshit Chanpura <dchanp@amazon.com> * Adds a new test flow that runs with resource sharing feature enabled Signed-off-by: Darshit Chanpura <dchanp@amazon.com> * Uses correct method name Signed-off-by: Darshit Chanpura <dchanp@amazon.com> * Reverts to implementation Signed-off-by: Darshit Chanpura <dchanp@amazon.com> * Fix 409 when running feature enabled tests Signed-off-by: Darshit Chanpura <dchanp@amazon.com> * Fix coverage for AbstractTimeSeriesActionHandler Signed-off-by: Darshit Chanpura <dchanp@amazon.com> * Fix coverage for BaseDeleteConfigTransportAction Signed-off-by: Darshit Chanpura <dchanp@amazon.com> * Cleans SecureRestADIT test class Signed-off-by: Darshit Chanpura <dchanp@amazon.com> * Parallelize security tests workflow Signed-off-by: Darshit Chanpura <dchanp@amazon.com> * Boost BaseDeleteConfigTransportAction coverage Signed-off-by: Darshit Chanpura <dchanp@amazon.com> * Rename method Signed-off-by: Darshit Chanpura <dchanp@amazon.com> * Clarifies enable and disable filter by scenarios for create detector test with security enabled Signed-off-by: Darshit Chanpura <dchanp@amazon.com> * Pass concrete configIndex name via constructor Signed-off-by: Darshit Chanpura <dchanp@amazon.com> --------- Signed-off-by: Darshit Chanpura <dchanp@amazon.com>
1 parent 9953e5c commit ee8e38d

29 files changed

+1182
-340
lines changed

‎.github/workflows/test_security.yml‎

Lines changed: 19 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,5 @@
11
name: Security test workflow
2-
# This workflow is triggered on pull requests to main
2+
# This workflow is triggered on pull requests or pushes to any branch
33
on:
44
pull_request:
55
branches:
@@ -16,34 +16,41 @@ jobs:
1616

1717
security-test:
1818
needs: Get-CI-Image-Tag
19-
# This job runs on Linux
2019
runs-on: ubuntu-latest
2120
container:
22-
# using the same image which is used by opensearch-build team to build the OpenSearch Distribution
23-
# this image tag is subject to change as more dependencies and updates will arrive over time
2421
image: ${{ needs.Get-CI-Image-Tag.outputs.ci-image-version-linux }}
2522
options: ${{ needs.Get-CI-Image-Tag.outputs.ci-image-start-options }}
2623

24+
strategy:
25+
matrix:
26+
# empty string = disabled, flag = enabled
27+
resource_sharing_flag: ["", "-Dresource_sharing.enabled=true"]
28+
2729
steps:
2830
- name: Run start commands
2931
run: ${{ needs.Get-CI-Image-Tag.outputs.ci-image-start-command }}
30-
# This step uses the setup-java Github action: https://github.com/actions/setup-java
32+
3133
- name: Set Up JDK
3234
uses: actions/setup-java@v4
3335
with:
34-
distribution: temurin # Temurin is a distribution of adoptium
36+
distribution: temurin
3537
java-version: 21
36-
# index-management
38+
3739
- name: Checkout Branch
3840
uses: actions/checkout@v4
41+
3942
- name: Run integration tests
4043
run: |
4144
chown -R 1000:1000 `pwd`
42-
su `id -un 1000` -c "./gradlew integTest -Dsecurity=true -Dhttps=true --tests '*IT'"
45+
su `id -un 1000` -c "./gradlew integTest \
46+
-Dsecurity=true \
47+
-Dhttps=true \
48+
${{ matrix.resource_sharing_flag }} \
49+
--tests '*IT'"
50+
4351
- name: Upload failed logs
44-
uses: actions/upload-artifact@v4
4552
if: failure()
53+
uses: actions/upload-artifact@v4
4654
with:
47-
name: logs
48-
overwrite: 'true'
49-
path: build/testclusters/integTest-*/logs/*
55+
name: logs-${{ matrix.resource_sharing_flag != '' && 'resource-sharing' || 'no-resource-sharing' }}
56+
path: build/testclusters/integTest-*/logs/*

‎CHANGELOG.md‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -6,6 +6,7 @@ Inspired from [Keep a Changelog](https://keepachangelog.com/en/1.1.0/)
66
## [Unreleased 3.x](https://github.com/opensearch-project/anomaly-detection/compare/3.0...HEAD)
77
### Features
88
### Enhancements
9+
- Use Centralized Resource Access Control framework provided by security plugin ([#1400](https://github.com/opensearch-project/anomaly-detection/pull/1400))
910
### Bug Fixes
1011
### Infrastructure
1112
### Documentation

‎build.gradle‎

Lines changed: 9 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -132,13 +132,14 @@ configurations {
132132
}
133133

134134
dependencies {
135+
compileOnly group: 'org.opensearch', name:'opensearch-security-spi', version:"${opensearch_build}"
135136
implementation "org.opensearch:opensearch:${opensearch_version}"
136137
compileOnly "org.opensearch.plugin:opensearch-scripting-painless-spi:${opensearch_version}"
137138
compileOnly "org.opensearch:opensearch-job-scheduler-spi:${job_scheduler_version}"
138139
implementation "org.opensearch:common-utils:${common_utils_version}"
139140
implementation "org.opensearch.client:opensearch-rest-client:${opensearch_version}"
140-
implementation group: 'com.google.guava', name: 'guava', version:'32.1.3-jre'
141-
implementation group: 'com.google.guava', name: 'failureaccess', version:'1.0.2'
141+
implementation group: 'com.google.guava', name: 'guava', version:'33.4.5-jre'
142+
implementation group: 'com.google.guava', name: 'failureaccess', version:'1.0.3'
142143
implementation group: 'org.apache.commons', name: 'commons-math3', version: '3.6.1'
143144
implementation group: 'com.google.code.gson', name: 'gson', version: '2.11.0'
144145
implementation group: 'com.yahoo.datasketches', name: 'sketches-core', version: '0.13.4'
@@ -241,7 +242,7 @@ opensearchplugin {
241242
name 'opensearch-anomaly-detection'
242243
description 'OpenSearch anomaly detector plugin'
243244
classname 'org.opensearch.timeseries.TimeSeriesAnalyticsPlugin'
244-
extendedPlugins = ['lang-painless', 'opensearch-job-scheduler']
245+
extendedPlugins = ['lang-painless', 'opensearch-job-scheduler', 'opensearch-security;optional=true']
245246
}
246247

247248
// Handle case where older versions of esplugin doesn't expose the joda time version it uses
@@ -261,9 +262,10 @@ configurations.all {
261262
force "com.google.code.gson:gson:2.11.0"
262263
force "junit:junit:4.13.2"
263264

264-
force "com.google.guava:guava:32.1.3-jre" // CVE for 31.1
265+
force "com.google.guava:guava:33.4.5-jre" // CVE for 31.1
265266
force("com.fasterxml.jackson.core:jackson-core:${jacksonVersion}")
266267
force "org.eclipse.platform:org.eclipse.core.runtime:3.29.0" // CVE for < 3.29.0
268+
force "org.ow2.asm:asm:9.7.1"
267269
}
268270
}
269271

@@ -547,7 +549,9 @@ def configureClusterPlugins(cluster, jobSchedulerFile, securityPluginFile, secur
547549
node.setting("plugins.security.check_snapshot_restore_write_privileges", "true")
548550
node.setting("plugins.security.restapi.roles_enabled", "[\"all_access\", \"security_rest_api_access\"]")
549551
node.setting("plugins.security.system_indices.enabled", "true")
550-
// node.setting("plugins.security.system_indices.indices", "[\".opendistro-ism-config\"]")
552+
if (System.getProperty("resource_sharing.enabled") == "true") {
553+
node.setting("plugins.security.experimental.resource_sharing.enabled", "true")
554+
}
551555
}
552556
}
553557
}

‎src/main/java/org/opensearch/ad/transport/AnomalyDetectorJobTransportAction.java‎

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -60,7 +60,8 @@ public AnomalyDetectorJobTransportAction(
6060
FAIL_TO_START_DETECTOR,
6161
FAIL_TO_STOP_DETECTOR,
6262
AnomalyDetector.class,
63-
adIndexJobActionHandler
63+
adIndexJobActionHandler,
64+
ADIndex.CONFIG.getIndexName()
6465
);
6566
}
6667
}

‎src/main/java/org/opensearch/ad/transport/DeleteAnomalyDetectorTransportAction.java‎

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -59,7 +59,8 @@ public DeleteAnomalyDetectorTransportAction(
5959
AnalysisType.AD,
6060
ADCommonName.DETECTION_STATE_INDEX,
6161
AnomalyDetector.class,
62-
ADTaskType.HISTORICAL_DETECTOR_TASK_TYPES
62+
ADTaskType.HISTORICAL_DETECTOR_TASK_TYPES,
63+
ADIndex.CONFIG.getIndexName()
6364
);
6465
}
6566
}

‎src/main/java/org/opensearch/ad/transport/GetAnomalyDetectorTransportAction.java‎

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -81,7 +81,8 @@ public GetAnomalyDetectorTransportAction(
8181
ADTaskType.HISTORICAL_HC_DETECTOR.name(),
8282
ADTaskType.HISTORICAL_SINGLE_ENTITY.name(),
8383
AnomalyDetectorSettings.AD_FILTER_BY_BACKEND_ROLES,
84-
adTaskProfileRunner
84+
adTaskProfileRunner,
85+
ADIndex.CONFIG.getIndexName()
8586
);
8687
}
8788

‎src/main/java/org/opensearch/ad/transport/IndexAnomalyDetectorTransportAction.java‎

Lines changed: 66 additions & 23 deletions
Original file line numberDiff line numberDiff line change
@@ -16,6 +16,8 @@
1616
import static org.opensearch.ad.settings.AnomalyDetectorSettings.AD_FILTER_BY_BACKEND_ROLES;
1717
import static org.opensearch.timeseries.util.ParseUtils.checkFilterByBackendRoles;
1818
import static org.opensearch.timeseries.util.ParseUtils.getConfig;
19+
import static org.opensearch.timeseries.util.ParseUtils.shouldUseResourceAuthz;
20+
import static org.opensearch.timeseries.util.ParseUtils.verifyResourceAccessAndProcessRequest;
1921
import static org.opensearch.timeseries.util.RestHandlerUtils.wrapRestActionListener;
2022

2123
import java.util.List;
@@ -27,6 +29,7 @@
2729
import org.opensearch.action.support.ActionFilters;
2830
import org.opensearch.action.support.HandledTransportAction;
2931
import org.opensearch.action.support.WriteRequest;
32+
import org.opensearch.ad.indices.ADIndex;
3033
import org.opensearch.ad.indices.ADIndexManagement;
3134
import org.opensearch.ad.model.AnomalyDetector;
3235
import org.opensearch.ad.rest.handler.IndexAnomalyDetectorActionHandler;
@@ -99,8 +102,35 @@ protected void doExecute(Task task, IndexAnomalyDetectorRequest request, ActionL
99102
RestRequest.Method method = request.getMethod();
100103
String errorMessage = method == RestRequest.Method.PUT ? FAIL_TO_UPDATE_DETECTOR : FAIL_TO_CREATE_DETECTOR;
101104
ActionListener<IndexAnomalyDetectorResponse> listener = wrapRestActionListener(actionListener, errorMessage);
105+
106+
// TODO: Remove following and any other conditional check, post GA for Resource Authz.
107+
boolean shouldEvaluateWithNewAuthz = shouldUseResourceAuthz(settings);
108+
102109
try (ThreadContext.StoredContext context = client.threadPool().getThreadContext().stashContext()) {
103-
resolveUserAndExecute(user, detectorId, method, listener, (detector) -> adExecute(request, user, detector, context, listener));
110+
verifyResourceAccessAndProcessRequest(
111+
user,
112+
ADIndex.CONFIG.getIndexName(),
113+
detectorId,
114+
shouldEvaluateWithNewAuthz,
115+
listener,
116+
args -> indexDetector(
117+
user,
118+
detectorId,
119+
method,
120+
listener,
121+
detector -> adExecute(request, user, detector, context, listener)
122+
),
123+
new Object[] {},
124+
(fallbackArgs) -> resolveUserAndExecute(
125+
user,
126+
detectorId,
127+
method,
128+
listener,
129+
(detector) -> adExecute(request, user, detector, context, listener)
130+
),
131+
new Object[] {}
132+
);
133+
104134
} catch (Exception e) {
105135
LOG.error(e);
106136
listener.onFailure(e);
@@ -124,33 +154,44 @@ private void resolveUserAndExecute(
124154
return;
125155
}
126156
}
127-
if (method == RestRequest.Method.PUT) {
128-
// requestedUser == null means security is disabled or user is superadmin. In this case we don't need to
129-
// check if request user have access to the detector or not. But we still need to get current detector for
130-
// this case, so we can keep current detector's user data.
131-
boolean filterByBackendRole = requestedUser == null ? false : filterByEnabled;
132-
// Update detector request, check if user has permissions to update the detector
133-
// Get detector and verify backend roles
134-
getConfig(
135-
requestedUser,
136-
detectorId,
137-
listener,
138-
function,
139-
client,
140-
clusterService,
141-
xContentRegistry,
142-
filterByBackendRole,
143-
AnomalyDetector.class
144-
);
145-
} else {
146-
// Create Detector. No need to get current detector.
147-
function.accept(null);
148-
}
157+
158+
indexDetector(requestedUser, detectorId, method, listener, function);
149159
} catch (Exception e) {
150160
listener.onFailure(e);
151161
}
152162
}
153163

164+
private void indexDetector(
165+
User requestedUser,
166+
String detectorId,
167+
RestRequest.Method method,
168+
ActionListener<IndexAnomalyDetectorResponse> listener,
169+
Consumer<AnomalyDetector> function
170+
) {
171+
if (method == RestRequest.Method.PUT) {
172+
// requestedUser == null means security is disabled or user is superadmin. In this case we don't need to
173+
// check if request user have access to the detector or not. But we still need to get current detector for
174+
// this case, so we can keep current detector's user data.
175+
boolean filterByBackendRole = requestedUser == null ? false : filterByEnabled;
176+
// Update detector request, check if user has permissions to update the detector
177+
// Get detector and verify backend roles
178+
getConfig(
179+
requestedUser,
180+
detectorId,
181+
listener,
182+
function,
183+
client,
184+
clusterService,
185+
xContentRegistry,
186+
filterByBackendRole,
187+
AnomalyDetector.class
188+
);
189+
} else {
190+
// Create Detector. No need to get current detector.
191+
function.accept(null);
192+
}
193+
}
194+
154195
protected void adExecute(
155196
IndexAnomalyDetectorRequest request,
156197
User user,
@@ -175,6 +216,8 @@ protected void adExecute(
175216
checkIndicesAndExecute(detector.getIndices(), () -> {
176217
// Don't replace detector's user when update detector
177218
// Github issue: https://github.com/opensearch-project/anomaly-detection/issues/124
219+
// TODO this and similar code should be updated to remove reference to a user
220+
178221
User detectorUser = currentDetector == null ? user : currentDetector.getUser();
179222
IndexAnomalyDetectorActionHandler indexAnomalyDetectorActionHandler = new IndexAnomalyDetectorActionHandler(
180223
clusterService,

‎src/main/java/org/opensearch/ad/transport/PreviewAnomalyDetectorTransportAction.java‎

Lines changed: 28 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -17,6 +17,8 @@
1717
import static org.opensearch.ad.settings.AnomalyDetectorSettings.MAX_CONCURRENT_PREVIEW;
1818
import static org.opensearch.core.xcontent.XContentParserUtils.ensureExpectedToken;
1919
import static org.opensearch.timeseries.util.ParseUtils.resolveUserAndExecute;
20+
import static org.opensearch.timeseries.util.ParseUtils.shouldUseResourceAuthz;
21+
import static org.opensearch.timeseries.util.ParseUtils.verifyResourceAccessAndProcessRequest;
2022
import static org.opensearch.timeseries.util.RestHandlerUtils.wrapRestActionListener;
2123

2224
import java.io.IOException;
@@ -34,6 +36,7 @@
3436
import org.opensearch.action.support.HandledTransportAction;
3537
import org.opensearch.ad.AnomalyDetectorRunner;
3638
import org.opensearch.ad.constant.ADCommonMessages;
39+
import org.opensearch.ad.indices.ADIndex;
3740
import org.opensearch.ad.model.AnomalyDetector;
3841
import org.opensearch.ad.model.AnomalyResult;
3942
import org.opensearch.ad.settings.AnomalyDetectorSettings;
@@ -65,6 +68,7 @@ public class PreviewAnomalyDetectorTransportAction extends
6568
private final AnomalyDetectorRunner anomalyDetectorRunner;
6669
private final ClusterService clusterService;
6770
private final Client client;
71+
private final Settings settings;
6872
private final NamedXContentRegistry xContentRegistry;
6973
private volatile Integer maxAnomalyFeatures;
7074
private volatile Boolean filterByEnabled;
@@ -85,6 +89,7 @@ public PreviewAnomalyDetectorTransportAction(
8589
super(PreviewAnomalyDetectorAction.NAME, transportService, actionFilters, PreviewAnomalyDetectorRequest::new);
8690
this.clusterService = clusterService;
8791
this.client = client;
92+
this.settings = settings;
8893
this.anomalyDetectorRunner = anomalyDetectorRunner;
8994
this.xContentRegistry = xContentRegistry;
9095
maxAnomalyFeatures = MAX_ANOMALY_FEATURES.get(settings);
@@ -105,18 +110,34 @@ protected void doExecute(
105110
String detectorId = request.getId();
106111
User user = ParseUtils.getUserContext(client);
107112
ActionListener<PreviewAnomalyDetectorResponse> listener = wrapRestActionListener(actionListener, FAIL_TO_PREVIEW_DETECTOR);
113+
114+
// TODO: Remove following and any other conditional check, post GA for Resource Authz.
115+
boolean shouldEvaluateWithNewAuthz = shouldUseResourceAuthz(settings);
116+
108117
try (ThreadContext.StoredContext context = client.threadPool().getThreadContext().stashContext()) {
109-
resolveUserAndExecute(
118+
// Call the verifyResourceAccessAndProcessRequest method
119+
verifyResourceAccessAndProcessRequest(
110120
user,
121+
ADIndex.CONFIG.getIndexName(),
111122
detectorId,
112-
filterByEnabled,
123+
shouldEvaluateWithNewAuthz,
113124
listener,
114-
(anomalyDetector) -> previewExecute(request, context, listener),
115-
client,
116-
clusterService,
117-
xContentRegistry,
118-
AnomalyDetector.class
125+
args -> previewExecute(request, context, listener),
126+
new Object[] {},
127+
(fallbackArgs) -> resolveUserAndExecute(
128+
user,
129+
detectorId,
130+
filterByEnabled,
131+
listener,
132+
ad -> previewExecute(request, context, listener),
133+
client,
134+
clusterService,
135+
xContentRegistry,
136+
AnomalyDetector.class
137+
),
138+
new Object[] {}
119139
);
140+
120141
} catch (Exception e) {
121142
logger.error(e);
122143
listener.onFailure(e);

‎src/main/java/org/opensearch/forecast/transport/DeleteForecasterTransportAction.java‎

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -52,7 +52,8 @@ public DeleteForecasterTransportAction(
5252
AnalysisType.FORECAST,
5353
ForecastIndex.STATE.getIndexName(),
5454
Forecaster.class,
55-
ForecastTaskType.RUN_ONCE_TASK_TYPES
55+
ForecastTaskType.RUN_ONCE_TASK_TYPES,
56+
ForecastIndex.CONFIG.getIndexName()
5657
);
5758
}
5859
}

0 commit comments

Comments
 (0)