diff --git a/x-pack/plugin/security/qa/consistency-checks/build.gradle b/x-pack/plugin/security/qa/consistency-checks/build.gradle index 06dca2fe67569..dc400291b714c 100644 --- a/x-pack/plugin/security/qa/consistency-checks/build.gradle +++ b/x-pack/plugin/security/qa/consistency-checks/build.gradle @@ -6,7 +6,7 @@ dependencies { 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 issues as well as jar hell since the test uses a single classpath + //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') @@ -35,3 +35,7 @@ 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 f849ff49c4d25..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 @@ -58,15 +58,15 @@ import java.util.stream.Collectors; /** - * This test helps to ensure that RCS 2.0 transport actions and transport request handlers 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 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 { - @Override protected Collection> getPlugins() { final ArrayList> plugins = new ArrayList<>(super.getPlugins()); @@ -99,7 +99,7 @@ protected Collection> getPlugins() { return plugins; } - @SuppressWarnings({ "unchecked", "rawtypes" }) + @SuppressWarnings("rawtypes") public void testCheckForNewShardLevelTransportActions() throws Exception { Node node = node(); @@ -111,7 +111,10 @@ public void testCheckForNewShardLevelTransportActions() throws Exception { Set> indicesRequest = reflections.getSubTypesOf(IndicesRequest.class); // Ignore any indices requests that are already marked with the RemoteClusterShardRequest interface - indicesRequest.removeAll(reflections.getSubTypesOf(IndicesRequest.RemoteClusterShardRequest.class)); + 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<>(); @@ -120,7 +123,7 @@ public void testCheckForNewShardLevelTransportActions() throws Exception { // 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()))) { + if (ShardId.class.getCanonicalName().equals(getTypeFromMaybeGeneric(method.getGenericReturnType()))) { candidateRequests.add(method.getDeclaringClass().getCanonicalName()); } } @@ -154,7 +157,7 @@ public void testCheckForNewShardLevelTransportActions() throws Exception { for (Class transportRequestHandler : transportRequestHandlers) { for (Method method : transportRequestHandler.getDeclaredMethods()) { if (method.getName().equals("messageReceived")) { - //first parameter is the resolved generic type of the TransportRequestHandler + // first parameter is the resolved generic type of the TransportRequestHandler Parameter firstParameter = method.getParameters()[0]; String actionRequestType = firstParameter.getType().getCanonicalName(); if (candidateRequests.contains(actionRequestType)) { @@ -164,18 +167,66 @@ public void testCheckForNewShardLevelTransportActions() throws Exception { } } + // Fail if we find any requests that should have the interface if (actionsWithShardRequests.isEmpty() == false) { + fail( + 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 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) + 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] - """, actionsWithShardRequests.stream().map(FinalCandidate::actionClassName).collect(Collectors.joining(", ")), - actionsWithShardRequests.stream().map(FinalCandidate::requestClassName).collect(Collectors.joining(", ")))); + """, 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; } /** @@ -187,7 +238,7 @@ private static String getTypeFromMaybeGeneric(Type type) { 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 + // 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();