diff --git a/gradle/verification-metadata.xml b/gradle/verification-metadata.xml index 5e26d96c4ca17..3e45f715395cd 100644 --- a/gradle/verification-metadata.xml +++ b/gradle/verification-metadata.xml @@ -3607,6 +3607,11 @@ + + + + + diff --git a/server/src/main/java/org/elasticsearch/action/fieldcaps/FieldCapabilitiesNodeRequest.java b/server/src/main/java/org/elasticsearch/action/fieldcaps/FieldCapabilitiesNodeRequest.java index 6c1734bde401f..514d1caa17fc9 100644 --- a/server/src/main/java/org/elasticsearch/action/fieldcaps/FieldCapabilitiesNodeRequest.java +++ b/server/src/main/java/org/elasticsearch/action/fieldcaps/FieldCapabilitiesNodeRequest.java @@ -25,11 +25,12 @@ import java.io.IOException; import java.util.Arrays; +import java.util.Collection; import java.util.List; import java.util.Map; import java.util.Objects; -class FieldCapabilitiesNodeRequest extends ActionRequest implements IndicesRequest { +class FieldCapabilitiesNodeRequest extends ActionRequest implements IndicesRequest.RemoteClusterShardRequest { private final List shardIds; private final String[] fields; @@ -215,4 +216,9 @@ public int hashCode() { result = 31 * result + Arrays.hashCode(allowedTypes); return result; } + + @Override + public Collection shards() { + return shardIds(); + } } diff --git a/server/src/main/java/org/elasticsearch/action/search/TransportOpenPointInTimeAction.java b/server/src/main/java/org/elasticsearch/action/search/TransportOpenPointInTimeAction.java index a31593d06a521..be9516c5d733e 100644 --- a/server/src/main/java/org/elasticsearch/action/search/TransportOpenPointInTimeAction.java +++ b/server/src/main/java/org/elasticsearch/action/search/TransportOpenPointInTimeAction.java @@ -47,6 +47,8 @@ import org.elasticsearch.transport.TransportService; import java.io.IOException; +import java.util.Collection; +import java.util.List; import java.util.Map; import java.util.concurrent.Executor; import java.util.function.BiFunction; @@ -280,7 +282,7 @@ boolean buildPointInTimeFromSearchResults() { } } - private static final class ShardOpenReaderRequest extends TransportRequest implements IndicesRequest { + private static final class ShardOpenReaderRequest extends TransportRequest implements IndicesRequest.RemoteClusterShardRequest { final ShardId shardId; final OriginalIndices originalIndices; final TimeValue keepAlive; @@ -319,6 +321,11 @@ public String[] indices() { public IndicesOptions indicesOptions() { return originalIndices.indicesOptions(); } + + @Override + public Collection shards() { + return List.of(getShardId()); + } } private static final class ShardOpenReaderResponse extends SearchPhaseResult { diff --git a/server/src/main/java/org/elasticsearch/action/support/broadcast/unpromotable/BroadcastUnpromotableRequest.java b/server/src/main/java/org/elasticsearch/action/support/broadcast/unpromotable/BroadcastUnpromotableRequest.java index 312a9843c9e2b..28703486d322f 100644 --- a/server/src/main/java/org/elasticsearch/action/support/broadcast/unpromotable/BroadcastUnpromotableRequest.java +++ b/server/src/main/java/org/elasticsearch/action/support/broadcast/unpromotable/BroadcastUnpromotableRequest.java @@ -20,6 +20,8 @@ import org.elasticsearch.index.shard.ShardId; import java.io.IOException; +import java.util.Collection; +import java.util.List; import java.util.Objects; import static org.elasticsearch.action.ValidateActions.addValidationError; @@ -28,7 +30,7 @@ /** * A request that is broadcast to the unpromotable assigned replicas of a primary. */ -public class BroadcastUnpromotableRequest extends ActionRequest implements IndicesRequest { +public class BroadcastUnpromotableRequest extends ActionRequest implements IndicesRequest.RemoteClusterShardRequest { /** * Holds the index shard routing table that will be used by {@link TransportBroadcastUnpromotableAction} to broadcast the requests to @@ -105,4 +107,9 @@ public boolean failShardOnError() { public IndicesOptions indicesOptions() { return strictSingleIndexNoExpandForbidClosed(); } + + @Override + public Collection shards() { + return List.of(shardId); + } } diff --git a/server/src/main/java/org/elasticsearch/search/internal/ShardSearchRequest.java b/server/src/main/java/org/elasticsearch/search/internal/ShardSearchRequest.java index 488c956c187d5..45d2408fa4ea0 100644 --- a/server/src/main/java/org/elasticsearch/search/internal/ShardSearchRequest.java +++ b/server/src/main/java/org/elasticsearch/search/internal/ShardSearchRequest.java @@ -56,6 +56,7 @@ import java.io.IOException; import java.util.ArrayList; import java.util.Arrays; +import java.util.Collection; import java.util.List; import java.util.Map; import java.util.function.Function; @@ -68,7 +69,7 @@ * It provides all the methods that the {@link SearchContext} needs. * Provides a cache key based on its content that can be used to cache shard level response. */ -public class ShardSearchRequest extends TransportRequest implements IndicesRequest { +public class ShardSearchRequest extends TransportRequest implements IndicesRequest.RemoteClusterShardRequest { private final String clusterAlias; private final ShardId shardId; private final int shardRequestIndex; @@ -573,6 +574,11 @@ public Rewriteable getRewriteable() { return new RequestRewritable(this); } + @Override + public Collection shards() { + return List.of(shardId); + } + @SuppressWarnings("rawtypes") static class RequestRewritable implements Rewriteable { diff --git a/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/termsenum/action/NodeTermsEnumRequest.java b/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/termsenum/action/NodeTermsEnumRequest.java index 84a28d9c8d1ef..42eba2e838a35 100644 --- a/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/termsenum/action/NodeTermsEnumRequest.java +++ b/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/termsenum/action/NodeTermsEnumRequest.java @@ -19,6 +19,7 @@ import org.elasticsearch.transport.TransportRequest; import java.io.IOException; +import java.util.Collection; import java.util.Collections; import java.util.Set; @@ -26,7 +27,7 @@ * Internal terms enum request executed directly against a specific node, querying potentially many * shards in one request */ -public class NodeTermsEnumRequest extends TransportRequest implements IndicesRequest { +public class NodeTermsEnumRequest extends TransportRequest implements IndicesRequest.RemoteClusterShardRequest { private final String field; private final String string; @@ -181,4 +182,9 @@ public IndicesOptions indicesOptions() { public boolean remove(ShardId shardId) { return shardIds.remove(shardId); } + + @Override + public Collection shards() { + return shardIds(); + } } diff --git a/x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/plugin/DataNodeRequest.java b/x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/plugin/DataNodeRequest.java index ab2df4a2ba6a9..e467157373a76 100644 --- a/x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/plugin/DataNodeRequest.java +++ b/x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/plugin/DataNodeRequest.java @@ -32,11 +32,12 @@ import org.elasticsearch.xpack.esql.session.EsqlConfiguration; import java.io.IOException; +import java.util.Collection; import java.util.List; import java.util.Map; import java.util.Objects; -final class DataNodeRequest extends TransportRequest implements IndicesRequest { +final class DataNodeRequest extends TransportRequest implements IndicesRequest.RemoteClusterShardRequest { private static final PlanNameRegistry planNameRegistry = new PlanNameRegistry(); private final String sessionId; private final EsqlConfiguration configuration; @@ -140,6 +141,11 @@ List shardIds() { return shardIds; } + @Override + public Collection shards() { + return shardIds(); + } + /** * Returns a map from index UUID to alias filters */ @@ -179,4 +185,5 @@ public boolean equals(Object o) { public int hashCode() { return Objects.hash(sessionId, configuration, clusterAlias, shardIds, aliasFilters, plan); } + } diff --git a/x-pack/plugin/security/qa/consistency-checks/build.gradle b/x-pack/plugin/security/qa/consistency-checks/build.gradle index 6fa3deb773e4c..dc400291b714c 100644 --- a/x-pack/plugin/security/qa/consistency-checks/build.gradle +++ b/x-pack/plugin/security/qa/consistency-checks/build.gradle @@ -2,6 +2,12 @@ apply plugin: 'elasticsearch.standalone-test' dependencies { + testImplementation "org.reflections:reflections:0.10.2" + testImplementation "org.javassist:javassist:3.30.2-GA" + + //while it is possible to place a dependency on all x-pack plugins and modules, + //we will end up with dependency convergence / jar hell issues + //the set here is a best attempt to include all the relevant modules and x-pack plugins testImplementation(testArtifact(project(xpackModule('core')))) testImplementation project(path: ':modules:ingest-common') testImplementation project(path: ':modules:data-streams') @@ -25,3 +31,11 @@ dependencies { testImplementation project(path: xpackModule('slm')) testImplementation project(path: xpackModule('sql')) } +tasks.named("test").configure { + //test uses reflections to find classes, so we need to disable the security manager + systemProperty 'tests.security.manager', 'false' +} +tasks.named("forbiddenApisTest") { + //this test intentionally uses reflection to violate java access control + it.enabled = false +} diff --git a/x-pack/plugin/security/qa/consistency-checks/src/test/java/org/elasticsearch/xpack/security/CrossClusterShardTests.java b/x-pack/plugin/security/qa/consistency-checks/src/test/java/org/elasticsearch/xpack/security/CrossClusterShardTests.java index d9e870b031877..b991f62293198 100644 --- a/x-pack/plugin/security/qa/consistency-checks/src/test/java/org/elasticsearch/xpack/security/CrossClusterShardTests.java +++ b/x-pack/plugin/security/qa/consistency-checks/src/test/java/org/elasticsearch/xpack/security/CrossClusterShardTests.java @@ -7,14 +7,13 @@ package org.elasticsearch.xpack.security; -import org.elasticsearch.action.admin.cluster.shards.TransportClusterSearchShardsAction; -import org.elasticsearch.action.search.TransportSearchShardsAction; +import org.elasticsearch.action.IndicesRequest; import org.elasticsearch.action.support.TransportAction; -import org.elasticsearch.action.support.single.shard.TransportSingleShardAction; import org.elasticsearch.common.inject.Binding; import org.elasticsearch.common.inject.TypeLiteral; import org.elasticsearch.datastreams.DataStreamsPlugin; import org.elasticsearch.index.rankeval.RankEvalPlugin; +import org.elasticsearch.index.shard.ShardId; import org.elasticsearch.ingest.IngestTestPlugin; import org.elasticsearch.ingest.common.IngestCommonPlugin; import org.elasticsearch.node.Node; @@ -22,6 +21,7 @@ import org.elasticsearch.reindex.ReindexPlugin; import org.elasticsearch.script.mustache.MustachePlugin; import org.elasticsearch.test.ESSingleNodeTestCase; +import org.elasticsearch.transport.TransportRequestHandler; import org.elasticsearch.xpack.analytics.AnalyticsPlugin; import org.elasticsearch.xpack.autoscaling.Autoscaling; import org.elasticsearch.xpack.ccr.Ccr; @@ -29,7 +29,6 @@ import org.elasticsearch.xpack.core.security.action.apikey.CrossClusterApiKeyRoleDescriptorBuilder; import org.elasticsearch.xpack.core.security.authz.privilege.IndexPrivilege; import org.elasticsearch.xpack.downsample.Downsample; -import org.elasticsearch.xpack.downsample.DownsampleShardPersistentTaskExecutor; import org.elasticsearch.xpack.eql.plugin.EqlPlugin; import org.elasticsearch.xpack.esql.plugin.EsqlPlugin; import org.elasticsearch.xpack.frozen.FrozenIndices; @@ -41,34 +40,33 @@ import org.elasticsearch.xpack.search.AsyncSearch; import org.elasticsearch.xpack.slm.SnapshotLifecycle; import org.elasticsearch.xpack.sql.plugin.SqlPlugin; +import org.reflections.Reflections; +import org.reflections.scanners.Scanners; +import org.reflections.util.ConfigurationBuilder; +import java.lang.reflect.Method; +import java.lang.reflect.Parameter; +import java.lang.reflect.ParameterizedType; +import java.lang.reflect.Type; +import java.lang.reflect.TypeVariable; import java.util.ArrayList; import java.util.Collection; import java.util.HashSet; import java.util.List; import java.util.Locale; import java.util.Set; -import java.util.function.Predicate; +import java.util.stream.Collectors; +/** + * This test helps to ensure actions available via RCS 2.0 (api key based cross cluster security) are correctly marked with the + * IndicesRequest.RemoteClusterShardRequest interface. + * This interface is used to identify transport actions and request handlers that operate on shards directly and can be used across + * clusters. This test will fail if a new transport action or request handler is added that operates on shards directly and is not + * marked with the IndicesRequest.RemoteClusterShardRequest interface. + * This is a best effort and not a guarantee that all transport actions and request handlers are correctly marked. + */ public class CrossClusterShardTests extends ESSingleNodeTestCase { - Set MANUALLY_CHECKED_SHARD_ACTIONS = Set.of( - // The request types for these actions are all subtypes of SingleShardRequest, and have been evaluated to make sure their - // `shards()` methods return the correct thing. - TransportSearchShardsAction.NAME, - - // These types have had the interface implemented manually. - DownsampleShardPersistentTaskExecutor.DelegatingAction.NAME, - - // These actions do not have any references to shard IDs in their requests. - TransportClusterSearchShardsAction.TYPE.name() - ); - - Set> CHECKED_ABSTRACT_CLASSES = Set.of( - // This abstract class implements the interface so we can assume all of its subtypes do so properly as well. - TransportSingleShardAction.class - ); - @Override protected Collection> getPlugins() { final ArrayList> plugins = new ArrayList<>(super.getPlugins()); @@ -104,62 +102,149 @@ protected Collection> getPlugins() { @SuppressWarnings("rawtypes") public void testCheckForNewShardLevelTransportActions() throws Exception { Node node = node(); + + Reflections reflections = new Reflections( + new ConfigurationBuilder().forPackages("org.elasticsearch").addScanners(Scanners.SubTypes).setParallel(false) + ); + + // Find all subclasses of IndicesRequest + Set> indicesRequest = reflections.getSubTypesOf(IndicesRequest.class); + + // Ignore any indices requests that are already marked with the RemoteClusterShardRequest interface + Set> remoteClusterShardRequest = reflections.getSubTypesOf( + IndicesRequest.RemoteClusterShardRequest.class + ); + indicesRequest.removeAll(remoteClusterShardRequest); + + // Find any IndicesRequest that have methods related to shards, these are the candidate requests for the marker interface + Set candidateRequests = new HashSet<>(); + for (Class clazz : indicesRequest) { + for (Method method : clazz.getDeclaredMethods()) { + // not the most efficient way to check for shard related methods, but it's good enough for this test + if (method.getName().toLowerCase(Locale.ROOT).contains("shard")) { + // only care if the return type is a ShardId or a collection of ShardIds + if (ShardId.class.getCanonicalName().equals(getTypeFromMaybeGeneric(method.getGenericReturnType()))) { + candidateRequests.add(method.getDeclaringClass().getCanonicalName()); + } + } + } + } + + // Find all transport actions List> transportActionBindings = node.injector().findBindingsByType(TypeLiteral.get(TransportAction.class)); + + // Find all transport actions that can execute over RCS 2.0 Set crossClusterPrivilegeNames = new HashSet<>(); crossClusterPrivilegeNames.addAll(List.of(CrossClusterApiKeyRoleDescriptorBuilder.CCS_INDICES_PRIVILEGE_NAMES)); crossClusterPrivilegeNames.addAll(List.of(CrossClusterApiKeyRoleDescriptorBuilder.CCR_INDICES_PRIVILEGE_NAMES)); - - List shardActions = transportActionBindings.stream() + List candidateActions = transportActionBindings.stream() .map(binding -> binding.getProvider().get()) .filter(action -> IndexPrivilege.get(crossClusterPrivilegeNames).predicate().test(action.actionName)) - .filter(this::actionIsLikelyShardAction) - .map(action -> action.actionName) .toList(); - List actionsNotOnAllowlist = shardActions.stream().filter(Predicate.not(MANUALLY_CHECKED_SHARD_ACTIONS::contains)).toList(); - if (actionsNotOnAllowlist.isEmpty() == false) { - fail(""" - If this test fails, you likely just added a transport action, probably with `shard` in the name. Transport actions which - operate on shards directly and can be used across clusters must meet some additional requirements in order to be - handled correctly by all Elasticsearch infrastructure, so please make sure you have read the javadoc on the - IndicesRequest.RemoteClusterShardRequest interface and implemented it if appropriate and not already appropriately - implemented by a supertype, then add the name (as in "indices:data/read/get") of your new transport action to - MANUALLY_CHECKED_SHARD_ACTIONS above. Found actions not in allowlist: - """ + actionsNotOnAllowlist); + Set actionsWithShardRequests = new HashSet<>(); + + // Find any transport actions that have methods related to shards, these are the candidate actions for the marker interface + for (TransportAction transportAction : candidateActions) { + String actionRequestType = getTypeFromMaybeGeneric(transportAction.getClass().getGenericSuperclass()); + if (candidateRequests.contains(actionRequestType)) { + actionsWithShardRequests.add(new FinalCandidate(transportAction.getClass().getCanonicalName(), actionRequestType)); + } } - // Also make sure the allowlist stays up to date and doesn't have any unnecessary entries. - List actionsOnAllowlistNotFound = MANUALLY_CHECKED_SHARD_ACTIONS.stream() - .filter(Predicate.not(shardActions::contains)) - .toList(); - if (actionsOnAllowlistNotFound.isEmpty() == false) { + // Find any TransportRequestHandler by looking at the request type of the messageReceived method + Set> transportRequestHandlers = reflections.getSubTypesOf(TransportRequestHandler.class); + for (Class transportRequestHandler : transportRequestHandlers) { + for (Method method : transportRequestHandler.getDeclaredMethods()) { + if (method.getName().equals("messageReceived")) { + // first parameter is the resolved generic type of the TransportRequestHandler + Parameter firstParameter = method.getParameters()[0]; + String actionRequestType = firstParameter.getType().getCanonicalName(); + if (candidateRequests.contains(actionRequestType)) { + actionsWithShardRequests.add(new FinalCandidate(transportRequestHandler.getCanonicalName(), actionRequestType)); + } + } + } + } + + // Fail if we find any requests that should have the interface + if (actionsWithShardRequests.isEmpty() == false) { fail( - "Some actions were on the allowlist but not found in the list of cross-cluster capable transport actions, please remove " - + "these from MANUALLY_CHECKED_SHARD_ACTIONS if they have been removed from Elasticsearch: " - + actionsOnAllowlistNotFound + String.format( + """ + This test failed. You likely just added an index level transport action(s) or transport request handler(s) + [%s] + with an associated TransportRequest with `shard` in a method name. Transport actions or transport request handlers + which operate directly on shards and can be used across clusters must meet some additional requirements in order to + be handled correctly by the Elasticsearch security infrastructure. Please review the javadoc for + IndicesRequest.RemoteClusterShardRequest and implement the interface on the transport request(s) + [%s] + """, + actionsWithShardRequests.stream().map(FinalCandidate::actionClassName).collect(Collectors.joining(", ")), + actionsWithShardRequests.stream().map(FinalCandidate::requestClassName).collect(Collectors.joining(", ")) + ) ); } + + // Look for any requests that have the interface but should not + Set existingRequests = remoteClusterShardRequest.stream().map(Class::getCanonicalName).collect(Collectors.toSet()); + for (Class clazz : remoteClusterShardRequest) { + removeExistingRequests(clazz, existingRequests, null); + } + + // Fail if we find any requests that should not have the interface + if (existingRequests.isEmpty() == false) { + fail(String.format(""" + This test failed. You likely just implemented IndicesRequest.RemoteClusterShardRequest where it is not needed. + This interface is only needed for requests associated with actions that are allowed to go across clusters via the + API key based (RCS 2.0) cross cluster implementation. You should remove the interface from the following requests: + [%s] + """, String.join(", ", existingRequests))); + } + } + + /** + * @return the set of classes that have the interface, but should not. + */ + private static Set removeExistingRequests(Class clazzToCheck, Set existingRequests, Class originalClazz) { + if (originalClazz == null) { + originalClazz = clazzToCheck; + } + for (Method method : clazzToCheck.getDeclaredMethods()) { + // not the most efficient way to check for shard related methods, but it's good enough for this test + if (method.getName().toLowerCase(Locale.ROOT).contains("shard")) { + // only care if the return type is a ShardId or a collection of ShardIds + if (ShardId.class.getCanonicalName().equals(getTypeFromMaybeGeneric(method.getGenericReturnType()))) { + existingRequests.remove(originalClazz.getCanonicalName()); + } + } + } + + if (clazzToCheck.getSuperclass() == null) { + return existingRequests; + } else { + // check parents too + removeExistingRequests(clazzToCheck.getSuperclass(), existingRequests, originalClazz); + } + return existingRequests; } /** - * Getting to the actual request classes themselves is made difficult by the design of Elasticsearch's transport - * protocol infrastructure combined with JVM type erasure. Therefore, we resort to a crude heuristic here. - * @param transportAction The transportport action to be checked. - * @return True if the action is suspected of being an action which may operate on shards directly. + * @return The canonical class name of the first parameter type of a generic type, + * or the canonical class name of the class if it's not a generic type */ - private boolean actionIsLikelyShardAction(TransportAction transportAction) { - Class clazz = transportAction.getClass(); - Set> classHeirarchy = new HashSet<>(); - while (clazz != TransportAction.class) { - classHeirarchy.add(clazz); - clazz = clazz.getSuperclass(); + private static String getTypeFromMaybeGeneric(Type type) { + if (type instanceof ParameterizedType parameterizedType) { + Type[] typeArguments = parameterizedType.getActualTypeArguments(); + return getTypeFromMaybeGeneric(typeArguments[0]); + } else if (type instanceof TypeVariable) { + // too complex to handle this case, and is likely a CRTP pattern which we will catch the children of this class + return ""; + } else if (type instanceof Class) { + return ((Class) type).getCanonicalName(); } - boolean hasCheckedSuperclass = classHeirarchy.stream().anyMatch(clz -> CHECKED_ABSTRACT_CLASSES.contains(clz)); - boolean shardInClassName = classHeirarchy.stream().anyMatch(clz -> clz.getName().toLowerCase(Locale.ROOT).contains("shard")); - return hasCheckedSuperclass == false - && (shardInClassName - || transportAction.actionName.toLowerCase(Locale.ROOT).contains("shard") - || transportAction.actionName.toLowerCase(Locale.ROOT).contains("[s]")); + throw new RuntimeException("Unknown type: " + type.getClass()); } + private record FinalCandidate(String actionClassName, String requestClassName) {} }