From 23eafaa111632cdab3b1c831a6260931dbbb47df Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Przemys=C5=82aw=20Witek?= Date: Thu, 24 Nov 2022 17:02:52 +0100 Subject: [PATCH] [ML] Skip remote clusters when performing up front privileges validation (#91895) --- docs/changelog/91895.yaml | 6 + .../build.gradle | 63 ++++++++++ .../MultiClusterYamlTestSuiteIT.java | 46 +++++++ .../test/multi_cluster/30_jobs.yml | 118 ++++++++++++++++++ .../test/remote_cluster/30_jobs.yml | 97 ++++++++++++++ .../TransportPutDataFrameAnalyticsAction.java | 25 +++- .../xpack/ml/datafeed/DatafeedManager.java | 16 ++- 7 files changed, 363 insertions(+), 8 deletions(-) create mode 100644 docs/changelog/91895.yaml create mode 100644 x-pack/plugin/ml/qa/multi-cluster-tests-with-security/build.gradle create mode 100644 x-pack/plugin/ml/qa/multi-cluster-tests-with-security/src/test/java/org/elasticsearch/multi_cluster/MultiClusterYamlTestSuiteIT.java create mode 100644 x-pack/plugin/ml/qa/multi-cluster-tests-with-security/src/test/resources/rest-api-spec/test/multi_cluster/30_jobs.yml create mode 100644 x-pack/plugin/ml/qa/multi-cluster-tests-with-security/src/test/resources/rest-api-spec/test/remote_cluster/30_jobs.yml diff --git a/docs/changelog/91895.yaml b/docs/changelog/91895.yaml new file mode 100644 index 000000000000..66f876ee52f7 --- /dev/null +++ b/docs/changelog/91895.yaml @@ -0,0 +1,6 @@ +pr: 91895 +summary: Skip remote clusters when performing up front privileges validation for datafeeds +area: Machine Learning +type: bug +issues: + - 87832 diff --git a/x-pack/plugin/ml/qa/multi-cluster-tests-with-security/build.gradle b/x-pack/plugin/ml/qa/multi-cluster-tests-with-security/build.gradle new file mode 100644 index 000000000000..368aab4615fc --- /dev/null +++ b/x-pack/plugin/ml/qa/multi-cluster-tests-with-security/build.gradle @@ -0,0 +1,63 @@ +import org.elasticsearch.gradle.internal.test.RestIntegTestTask +import org.elasticsearch.gradle.Version +import org.elasticsearch.gradle.VersionProperties + +apply plugin: 'elasticsearch.internal-testclusters' +apply plugin: 'elasticsearch.standalone-rest-test' +apply plugin: 'elasticsearch.rest-resources' + +dependencies { + testImplementation project(':x-pack:qa') +} + +Version ccsCompatVersion = new Version(VersionProperties.getElasticsearchVersion().getMajor(), VersionProperties.getElasticsearchVersion().getMinor() - 1, 0) + +restResources { + restApi { + include '_common', 'bulk', 'indices', 'cluster', 'search', 'security', 'ml' + } +} + +def remoteCluster = testClusters.register('remote-cluster') { + testDistribution = 'DEFAULT' + versions = [ccsCompatVersion.toString(), project.version] + numberOfNodes = 2 + setting 'node.roles', '[data,ingest,master]' + setting 'xpack.security.enabled', 'true' + setting 'xpack.watcher.enabled', 'false' + setting 'xpack.license.self_generated.type', 'trial' + + user username: "test_user", password: "x-pack-test-password" +} + +testClusters.register('mixed-cluster') { + testDistribution = 'DEFAULT' + numberOfNodes = 2 + setting 'node.roles', '[data,ingest,master]' + setting 'xpack.security.enabled', 'true' + setting 'xpack.watcher.enabled', 'false' + setting 'xpack.license.self_generated.type', 'trial' + setting 'cluster.remote.my_remote_cluster.seeds', { + remoteCluster.get().getAllTransportPortURI().collect { "\"$it\"" }.toString() + } + setting 'cluster.remote.connections_per_cluster', "1" + + user username: "test_user", password: "x-pack-test-password" +} + +tasks.register('remote-cluster', RestIntegTestTask) { + mustRunAfter("precommit") + systemProperty 'tests.rest.suite', 'remote_cluster' +} + +tasks.register('mixed-cluster', RestIntegTestTask) { + dependsOn 'remote-cluster' + useCluster remoteCluster + systemProperty 'tests.rest.suite', 'multi_cluster' +} + +tasks.register("integTest") { + dependsOn 'mixed-cluster' +} + +tasks.named("check").configure { dependsOn("integTest") } diff --git a/x-pack/plugin/ml/qa/multi-cluster-tests-with-security/src/test/java/org/elasticsearch/multi_cluster/MultiClusterYamlTestSuiteIT.java b/x-pack/plugin/ml/qa/multi-cluster-tests-with-security/src/test/java/org/elasticsearch/multi_cluster/MultiClusterYamlTestSuiteIT.java new file mode 100644 index 000000000000..0fbd3d463f7f --- /dev/null +++ b/x-pack/plugin/ml/qa/multi-cluster-tests-with-security/src/test/java/org/elasticsearch/multi_cluster/MultiClusterYamlTestSuiteIT.java @@ -0,0 +1,46 @@ +/* + * Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one + * or more contributor license agreements. Licensed under the Elastic License + * 2.0; you may not use this file except in compliance with the Elastic License + * 2.0. + */ + +package org.elasticsearch.multi_cluster; + +import com.carrotsearch.randomizedtesting.annotations.Name; +import com.carrotsearch.randomizedtesting.annotations.ParametersFactory; +import com.carrotsearch.randomizedtesting.annotations.TimeoutSuite; + +import org.apache.lucene.tests.util.TimeUnits; +import org.elasticsearch.common.settings.SecureString; +import org.elasticsearch.common.settings.Settings; +import org.elasticsearch.common.util.concurrent.ThreadContext; +import org.elasticsearch.test.rest.yaml.ClientYamlTestCandidate; +import org.elasticsearch.test.rest.yaml.ESClientYamlSuiteTestCase; + +@TimeoutSuite(millis = 5 * TimeUnits.MINUTE) // to account for slow as hell VMs +public class MultiClusterYamlTestSuiteIT extends ESClientYamlSuiteTestCase { + + private static final String USER = "test_user"; + private static final String PASS = "x-pack-test-password"; + + @Override + protected boolean preserveIndicesUponCompletion() { + return true; + } + + public MultiClusterYamlTestSuiteIT(@Name("yaml") ClientYamlTestCandidate testCandidate) { + super(testCandidate); + } + + @ParametersFactory + public static Iterable parameters() throws Exception { + return createParameters(); + } + + @Override + protected Settings restClientSettings() { + String token = basicAuthHeaderValue(USER, new SecureString(PASS.toCharArray())); + return Settings.builder().put(super.restClientSettings()).put(ThreadContext.PREFIX + ".Authorization", token).build(); + } +} diff --git a/x-pack/plugin/ml/qa/multi-cluster-tests-with-security/src/test/resources/rest-api-spec/test/multi_cluster/30_jobs.yml b/x-pack/plugin/ml/qa/multi-cluster-tests-with-security/src/test/resources/rest-api-spec/test/multi_cluster/30_jobs.yml new file mode 100644 index 000000000000..540fed64ec53 --- /dev/null +++ b/x-pack/plugin/ml/qa/multi-cluster-tests-with-security/src/test/resources/rest-api-spec/test/multi_cluster/30_jobs.yml @@ -0,0 +1,118 @@ +--- +setup: + - skip: + features: headers + + - do: + cluster.health: + wait_for_status: yellow + + - do: + security.put_user: + username: "joe" + body: > + { + "password": "joe-password", + "roles" : [ "x_cluster_role" ] + } + - do: + security.put_role: + name: "x_cluster_role" + body: > + { + "cluster": ["manage_ml"], + "indices": [ + { + "names": ["test_index"], + "privileges": ["read", "view_index_metadata"] + } + ] + } +--- +teardown: + - do: + security.delete_user: + username: "joe" + ignore: 404 + +--- +"Test ML job with local indices only": + - do: + catch: /Cannot create datafeed \[job-with-local-indices\] because user joe lacks permissions on the indices/ + headers: { Authorization: "Basic am9lOmpvZS1wYXNzd29yZA==" } # This is joe + ml.put_job: + job_id: job-with-local-indices + body: > + { + "description":"Analysis of response time by airline", + "analysis_config" : { + "detectors" :[{"function":"count"}] + }, + "analysis_limits" : { + "model_memory_limit": "20mb" + }, + "data_description" : { + "format":"xcontent" + }, + "datafeed_config": { + "indexes":["test_index", "test_index-2"] + } + } + +--- +"Test ML job with local and remote index": + - do: + headers: { Authorization: "Basic am9lOmpvZS1wYXNzd29yZA==" } # This is joe + ml.put_job: + job_id: job-with-local-and-remote-index + body: > + { + "description":"Analysis of response time by airline", + "analysis_config" : { + "detectors" :[{"function":"count"}] + }, + "analysis_limits" : { + "model_memory_limit": "20mb" + }, + "data_description" : { + "format":"xcontent" + }, + "datafeed_config": { + "indexes":["test_index", "my_remote_cluster:remote_test_index"] + } + } + - match: { job_id: "job-with-local-and-remote-index" } + - is_true: datafeed_config + - match: { datafeed_config.job_id: "job-with-local-and-remote-index" } + - match: { datafeed_config.datafeed_id: "job-with-local-and-remote-index" } + - is_true: datafeed_config.authorization.roles + - is_true: create_time + +--- +"Test ML job with remote index only": + - do: + headers: { Authorization: "Basic am9lOmpvZS1wYXNzd29yZA==" } # This is joe + ml.put_job: + job_id: job-with-remote-index-only + body: > + { + "description":"Analysis of response time by airline", + "analysis_config" : { + "detectors" :[{"function":"count"}] + }, + "analysis_limits" : { + "model_memory_limit": "20mb" + }, + "data_description" : { + "format":"xcontent" + }, + "datafeed_config": { + "indexes":["my_remote_cluster:remote_test_index"] + } + } + - match: { job_id: "job-with-remote-index-only" } + - is_true: datafeed_config + - match: { datafeed_config.job_id: "job-with-remote-index-only" } + - match: { datafeed_config.datafeed_id: "job-with-remote-index-only" } + - is_true: datafeed_config.authorization.roles + - is_true: create_time diff --git a/x-pack/plugin/ml/qa/multi-cluster-tests-with-security/src/test/resources/rest-api-spec/test/remote_cluster/30_jobs.yml b/x-pack/plugin/ml/qa/multi-cluster-tests-with-security/src/test/resources/rest-api-spec/test/remote_cluster/30_jobs.yml new file mode 100644 index 000000000000..2e4e7866fb07 --- /dev/null +++ b/x-pack/plugin/ml/qa/multi-cluster-tests-with-security/src/test/resources/rest-api-spec/test/remote_cluster/30_jobs.yml @@ -0,0 +1,97 @@ +--- +setup: + - skip: + features: headers + + - do: + cluster.health: + wait_for_status: yellow + - do: + security.put_user: + username: "joe" + body: > + { + "password": "joe-password", + "roles" : [ "x_cluster_role" ] + } + - do: + security.put_role: + name: "x_cluster_role" + body: > + { + "cluster": [], + "indices": [ + { + "names": ["remote_test_index"], + "privileges": ["read", "view_index_metadata"] + } + ] + } +--- +teardown: + - do: + security.delete_user: + username: "joe" + ignore: 404 + +--- +"Index data on the remote cluster": + - do: + indices.create: + index: remote_test_index + body: + settings: + index: + number_of_shards: 3 + number_of_replicas: 0 + aliases: + test_alias: {} + mappings: + properties: + time: + type: date + user: + type: keyword + stars: + type: integer + coolness: + type: integer + + - do: + bulk: + refresh: true + body: + - '{"index": {"_index": "remote_test_index"}}' + - '{"user": "a", "stars": 1, "date" : "2018-10-29T12:12:12.123456789Z"}' + - '{"index": {"_index": "remote_test_index"}}' + - '{"user": "a", "stars": 4, "date" : "2018-10-29T12:14:12.123456789Z"}' + - '{"index": {"_index": "remote_test_index"}}' + - '{"user": "a", "stars": 5, "date" : "2018-10-29T12:16:12.123456789Z"}' + - '{"index": {"_index": "remote_test_index"}}' + - '{"user": "b", "stars": 2, "date" : "2018-10-29T12:17:12.123456789Z"}' + - '{"index": {"_index": "remote_test_index"}}' + - '{"user": "b", "stars": 3, "date" : "2018-10-29T12:22:12.123456789Z"}' + - '{"index": {"_index": "remote_test_index"}}' + - '{"user": "a", "stars": 5, "date" : "2018-10-29T12:23:12.123456789Z"}' + - '{"index": {"_index": "remote_test_index"}}' + - '{"user": "b", "stars": 1, "date" : "2018-10-29T12:32:12.123456789Z"}' + - '{"index": {"_index": "remote_test_index"}}' + - '{"user": "a", "stars": 3, "date" : "2018-10-29T12:34:12.123456789Z"}' + - '{"index": {"_index": "remote_test_index"}}' + - '{"user": "c", "stars": 4, "date" : "2018-10-29T12:35:12.123456789Z"}' + - do: + headers: { Authorization: "Basic am9lOmpvZS1wYXNzd29yZA==" } # This is joe + search: + rest_total_hits_as_int: true + index: remote_test_index + body: + aggs: + user: + terms: + field: user + + - match: { _shards.total: 3 } + - match: { hits.total: 9 } + - length: { aggregations.user.buckets: 3 } + - match: { aggregations.user.buckets.0.key: "a" } + - match: { aggregations.user.buckets.0.doc_count: 5 } diff --git a/x-pack/plugin/ml/src/main/java/org/elasticsearch/xpack/ml/action/TransportPutDataFrameAnalyticsAction.java b/x-pack/plugin/ml/src/main/java/org/elasticsearch/xpack/ml/action/TransportPutDataFrameAnalyticsAction.java index 802be705e7ea..a4982addd1d0 100644 --- a/x-pack/plugin/ml/src/main/java/org/elasticsearch/xpack/ml/action/TransportPutDataFrameAnalyticsAction.java +++ b/x-pack/plugin/ml/src/main/java/org/elasticsearch/xpack/ml/action/TransportPutDataFrameAnalyticsAction.java @@ -25,6 +25,7 @@ import org.elasticsearch.core.TimeValue; import org.elasticsearch.license.License; import org.elasticsearch.license.LicenseUtils; +import org.elasticsearch.license.RemoteClusterLicenseChecker; import org.elasticsearch.license.XPackLicenseState; import org.elasticsearch.tasks.Task; import org.elasticsearch.threadpool.ThreadPool; @@ -54,10 +55,12 @@ import java.io.IOException; import java.time.Instant; +import java.util.Arrays; import java.util.Map; import java.util.Objects; import java.util.function.Supplier; +import static java.util.function.Predicate.not; import static org.elasticsearch.xpack.ml.utils.SecondaryAuthorizationUtils.useSecondaryAuthIfAvailable; public class TransportPutDataFrameAnalyticsAction extends TransportMasterNodeAction< @@ -160,10 +163,12 @@ private void putValidatedConfig( if (securityContext != null) { useSecondaryAuthIfAvailable(securityContext, () -> { final String username = securityContext.getUser().principal(); - RoleDescriptor.IndicesPrivileges sourceIndexPrivileges = RoleDescriptor.IndicesPrivileges.builder() - .indices(preparedForPutConfig.getSource().getIndex()) - .privileges("read") - .build(); + // DFA doesn't support CCS, but if it did it would need this filter, so it's safest to have the filter + // in place even though it's a no-op. + // TODO: Remove this filter once https://github.com/elastic/elasticsearch/issues/67798 is fixed. + final String[] sourceIndices = Arrays.stream(preparedForPutConfig.getSource().getIndex()) + .filter(not(RemoteClusterLicenseChecker::isRemoteIndex)) + .toArray(String[]::new); RoleDescriptor.IndicesPrivileges destIndexPrivileges = RoleDescriptor.IndicesPrivileges.builder() .indices(preparedForPutConfig.getDest().getIndex()) .privileges("read", "index", "create_index") @@ -173,7 +178,17 @@ private void putValidatedConfig( privRequest.applicationPrivileges(new RoleDescriptor.ApplicationResourcePrivileges[0]); privRequest.username(username); privRequest.clusterPrivileges(Strings.EMPTY_ARRAY); - privRequest.indexPrivileges(sourceIndexPrivileges, destIndexPrivileges); + RoleDescriptor.IndicesPrivileges[] indicesPrivileges; + if (sourceIndices.length > 0) { + RoleDescriptor.IndicesPrivileges sourceIndexPrivileges = RoleDescriptor.IndicesPrivileges.builder() + .indices(sourceIndices) + .privileges("read") + .build(); + indicesPrivileges = new RoleDescriptor.IndicesPrivileges[] { sourceIndexPrivileges, destIndexPrivileges }; + } else { + indicesPrivileges = new RoleDescriptor.IndicesPrivileges[] { destIndexPrivileges }; + } + privRequest.indexPrivileges(indicesPrivileges); ActionListener privResponseListener = ActionListener.wrap( r -> handlePrivsResponse(username, preparedForPutConfig, r, masterNodeTimeout, listener), diff --git a/x-pack/plugin/ml/src/main/java/org/elasticsearch/xpack/ml/datafeed/DatafeedManager.java b/x-pack/plugin/ml/src/main/java/org/elasticsearch/xpack/ml/datafeed/DatafeedManager.java index 4c6b9a78e306..7fcf7cf98024 100644 --- a/x-pack/plugin/ml/src/main/java/org/elasticsearch/xpack/ml/datafeed/DatafeedManager.java +++ b/x-pack/plugin/ml/src/main/java/org/elasticsearch/xpack/ml/datafeed/DatafeedManager.java @@ -59,6 +59,7 @@ import java.util.Set; import java.util.stream.Collectors; +import static java.util.function.Predicate.not; import static org.elasticsearch.xpack.core.ClientHelper.ML_ORIGIN; import static org.elasticsearch.xpack.core.ClientHelper.executeAsyncWithOrigin; import static org.elasticsearch.xpack.ml.utils.SecondaryAuthorizationUtils.useSecondaryAuthIfAvailable; @@ -106,7 +107,12 @@ public void putDatafeed( ) { if (XPackSettings.SECURITY_ENABLED.get(settings)) { useSecondaryAuthIfAvailable(securityContext, () -> { - final String[] indices = request.getDatafeed().getIndices().toArray(new String[0]); + // TODO: Remove this filter once https://github.com/elastic/elasticsearch/issues/67798 is fixed. + final String[] indices = request.getDatafeed() + .getIndices() + .stream() + .filter(not(RemoteClusterLicenseChecker::isRemoteIndex)) + .toArray(String[]::new); final String username = securityContext.getUser().principal(); final HasPrivilegesRequest privRequest = new HasPrivilegesRequest(); @@ -128,8 +134,12 @@ public void putDatafeed( } else { indicesPrivilegesBuilder.privileges(SearchAction.NAME, RollupSearchAction.NAME); } - privRequest.indexPrivileges(indicesPrivilegesBuilder.build()); - client.execute(HasPrivilegesAction.INSTANCE, privRequest, privResponseListener); + if (indices.length == 0) { + privResponseListener.onResponse(new HasPrivilegesResponse()); + } else { + privRequest.indexPrivileges(indicesPrivilegesBuilder.build()); + client.execute(HasPrivilegesAction.INSTANCE, privRequest, privResponseListener); + } }, e -> { if (ExceptionsHelper.unwrapCause(e) instanceof IndexNotFoundException) { indicesPrivilegesBuilder.privileges(SearchAction.NAME);