From fdfc66a8baa54c2bd8de20753831e9316e457164 Mon Sep 17 00:00:00 2001 From: javanna Date: Thu, 11 Aug 2016 19:28:36 +0200 Subject: [PATCH 1/7] Security: add tests for delete and update by query Original commit: elastic/x-pack-elasticsearch@e85877d03f4ed9148cd9931e942653138e31fe48 --- .../reindex-tests-with-security/build.gradle | 2 + .../xpack/security/ReindexWithSecurityIT.java | 128 ++++++++++++++++++ .../test/SecurityIntegTestCase.java | 2 +- ...cesAndAliasesResolverIntegrationTests.java | 63 +++------ 4 files changed, 148 insertions(+), 47 deletions(-) create mode 100644 elasticsearch/qa/reindex-tests-with-security/src/test/java/org/elasticsearch/xpack/security/ReindexWithSecurityIT.java diff --git a/elasticsearch/qa/reindex-tests-with-security/build.gradle b/elasticsearch/qa/reindex-tests-with-security/build.gradle index 4f7aba7b6db..d937efeae88 100644 --- a/elasticsearch/qa/reindex-tests-with-security/build.gradle +++ b/elasticsearch/qa/reindex-tests-with-security/build.gradle @@ -2,6 +2,8 @@ apply plugin: 'elasticsearch.rest-test' dependencies { testCompile project(path: ':x-plugins:elasticsearch:x-pack', configuration: 'runtime') + testCompile project(path: ':x-plugins:elasticsearch:x-pack', configuration: 'testArtifacts') + testCompile project(path: ':modules:reindex') } integTest { diff --git a/elasticsearch/qa/reindex-tests-with-security/src/test/java/org/elasticsearch/xpack/security/ReindexWithSecurityIT.java b/elasticsearch/qa/reindex-tests-with-security/src/test/java/org/elasticsearch/xpack/security/ReindexWithSecurityIT.java new file mode 100644 index 00000000000..933b170ecab --- /dev/null +++ b/elasticsearch/qa/reindex-tests-with-security/src/test/java/org/elasticsearch/xpack/security/ReindexWithSecurityIT.java @@ -0,0 +1,128 @@ +/* + * Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one + * or more contributor license agreements. Licensed under the Elastic License; + * you may not use this file except in compliance with the Elastic License. + */ +package org.elasticsearch.xpack.security; + +import org.elasticsearch.action.admin.indices.alias.Alias; +import org.elasticsearch.common.network.NetworkModule; +import org.elasticsearch.common.settings.Settings; +import org.elasticsearch.index.IndexNotFoundException; +import org.elasticsearch.index.reindex.BulkIndexByScrollResponse; +import org.elasticsearch.index.reindex.DeleteByQueryAction; +import org.elasticsearch.index.reindex.ReindexAction; +import org.elasticsearch.index.reindex.ReindexPlugin; +import org.elasticsearch.index.reindex.UpdateByQueryAction; +import org.elasticsearch.plugins.Plugin; +import org.elasticsearch.test.SecurityIntegTestCase; +import org.junit.Before; + +import java.util.ArrayList; +import java.util.Collection; +import java.util.Collections; + +public class ReindexWithSecurityIT extends SecurityIntegTestCase { + + private boolean useSecurity3; + + @Override + @Before + public void setUp() throws Exception { + super.setUp(); + useSecurity3 = randomBoolean(); + } + + @Override + protected Collection> nodePlugins() { + Collection> plugins = new ArrayList<>(super.nodePlugins()); + plugins.add(ReindexPlugin.class); + return Collections.unmodifiableCollection(plugins); + } + + @Override + protected Collection> transportClientPlugins() { + Collection> plugins = new ArrayList<>(super.nodePlugins()); + plugins.add(ReindexPlugin.class); + return Collections.unmodifiableCollection(plugins); + } + + @Override + protected Settings externalClusterClientSettings() { + Settings.Builder builder = Settings.builder().put(super.externalClusterClientSettings()); + if (useSecurity3) { + builder.put(NetworkModule.TRANSPORT_TYPE_KEY, Security.NAME3); + } else { + builder.put(NetworkModule.TRANSPORT_TYPE_KEY, Security.NAME4); + } + builder.put(Security.USER_SETTING.getKey(), "test_admin:changeme"); + return builder.build(); + } + + public void testDeleteByQuery() { + createIndices("test1", "test2", "test3"); + + BulkIndexByScrollResponse response = DeleteByQueryAction.INSTANCE.newRequestBuilder(client()).source("test1", "test2").get(); + assertNotNull(response); + + response = DeleteByQueryAction.INSTANCE.newRequestBuilder(client()).source("test*").get(); + assertNotNull(response); + + IndexNotFoundException e = expectThrows(IndexNotFoundException.class, + () -> DeleteByQueryAction.INSTANCE.newRequestBuilder(client()).source("test1", "index1").get()); + assertEquals("no such index", e.getMessage()); + } + + public void testUpdateByQuery() { + createIndices("test1", "test2", "test3"); + + BulkIndexByScrollResponse response = UpdateByQueryAction.INSTANCE.newRequestBuilder(client()).source("test1", "test2").get(); + assertNotNull(response); + + response = UpdateByQueryAction.INSTANCE.newRequestBuilder(client()).source("test*").get(); + assertNotNull(response); + + IndexNotFoundException e = expectThrows(IndexNotFoundException.class, + () -> UpdateByQueryAction.INSTANCE.newRequestBuilder(client()).source("test1", "index1").get()); + assertEquals("no such index", e.getMessage()); + } + + public void testReindex() { + createIndices("test1", "test2", "test3", "dest"); + + BulkIndexByScrollResponse response = ReindexAction.INSTANCE.newRequestBuilder(client()).source("test1", "test2") + .destination("dest").get(); + assertNotNull(response); + + response = ReindexAction.INSTANCE.newRequestBuilder(client()).source("test*").destination("dest").get(); + assertNotNull(response); + + IndexNotFoundException e = expectThrows(IndexNotFoundException.class, + () -> ReindexAction.INSTANCE.newRequestBuilder(client()).source("test1", "index1").destination("dest").get()); + assertEquals("no such index", e.getMessage()); + } + + private void createIndices(String... indices) { + if (randomBoolean()) { + //no aliases + createIndex(indices); + } else { + if (randomBoolean()) { + //one alias per index with suffix "-alias" + for (String index : indices) { + client().admin().indices().prepareCreate(index).setSettings(indexSettings()).addAlias(new Alias(index + "-alias")); + } + } else { + //same alias pointing to all indices + for (String index : indices) { + client().admin().indices().prepareCreate(index).setSettings(indexSettings()).addAlias(new Alias("alias")); + } + } + } + + for (String index : indices) { + client().prepareIndex(index, "type").setSource("field", "value").get(); + } + refresh(); + } +} diff --git a/elasticsearch/x-pack/security/src/test/java/org/elasticsearch/test/SecurityIntegTestCase.java b/elasticsearch/x-pack/security/src/test/java/org/elasticsearch/test/SecurityIntegTestCase.java index 912de25d471..48ddbf62112 100644 --- a/elasticsearch/x-pack/security/src/test/java/org/elasticsearch/test/SecurityIntegTestCase.java +++ b/elasticsearch/x-pack/security/src/test/java/org/elasticsearch/test/SecurityIntegTestCase.java @@ -153,7 +153,7 @@ public abstract class SecurityIntegTestCase extends ESIntegTestCase { // TODO: disable this assertion for now, due to random runs with mock plugins. perhaps run without mock plugins? // assertThat(nodeInfo.getPlugins().getInfos(), hasSize(2)); Collection pluginNames = - nodeInfo.getPlugins().getPluginInfos().stream().map(p -> p.getName()).collect(Collectors.toList()); + nodeInfo.getPlugins().getPluginInfos().stream().map(p -> p.getClassname()).collect(Collectors.toList()); assertThat("plugin [" + xpackPluginClass().getName() + "] not found in [" + pluginNames + "]", pluginNames, hasItem(xpackPluginClass().getName())); } diff --git a/elasticsearch/x-pack/security/src/test/java/org/elasticsearch/xpack/security/authz/indicesresolver/IndicesAndAliasesResolverIntegrationTests.java b/elasticsearch/x-pack/security/src/test/java/org/elasticsearch/xpack/security/authz/indicesresolver/IndicesAndAliasesResolverIntegrationTests.java index 43326bfc33a..132bec003a9 100644 --- a/elasticsearch/x-pack/security/src/test/java/org/elasticsearch/xpack/security/authz/indicesresolver/IndicesAndAliasesResolverIntegrationTests.java +++ b/elasticsearch/x-pack/security/src/test/java/org/elasticsearch/xpack/security/authz/indicesresolver/IndicesAndAliasesResolverIntegrationTests.java @@ -24,9 +24,9 @@ import static org.elasticsearch.test.SecurityTestsUtils.assertAuthorizationExcep import static org.hamcrest.Matchers.containsString; import static org.hamcrest.Matchers.equalTo; import static org.hamcrest.Matchers.hasItems; -import static org.hamcrest.Matchers.is; public class IndicesAndAliasesResolverIntegrationTests extends SecurityIntegTestCase { + @Override protected String configRoles() { return SecuritySettingsSource.DEFAULT_ROLE + ":\n" + @@ -57,50 +57,30 @@ public class IndicesAndAliasesResolverIntegrationTests extends SecurityIntegTest public void testSearchNonAuthorizedWildcard() { //wildcard doesn't match any authorized index createIndices("test1", "test2", "index1", "index2"); - try { - client().prepareSearch("index*").get(); - fail("Expected IndexNotFoundException"); - } catch (IndexNotFoundException e) { - assertThat(e.getMessage(), is("no such index")); - } + IndexNotFoundException e = expectThrows(IndexNotFoundException.class, () -> client().prepareSearch("index*").get()); + assertEquals("no such index", e.getMessage()); } public void testEmptyClusterSearchForAll() { - try { - client().prepareSearch().get(); - fail("Expected IndexNotFoundException"); - } catch (IndexNotFoundException e) { - assertThat(e.getMessage(), is("no such index")); - } + IndexNotFoundException e = expectThrows(IndexNotFoundException.class, () -> client().prepareSearch().get()); + assertEquals("no such index", e.getMessage()); } public void testEmptyClusterSearchForWildcard() { - try { - client().prepareSearch("*").get(); - fail("Expected IndexNotFoundException"); - } catch (IndexNotFoundException e) { - assertThat(e.getMessage(), is("no such index")); - } + IndexNotFoundException e = expectThrows(IndexNotFoundException.class, () -> client().prepareSearch("*").get()); + assertEquals("no such index", e.getMessage()); } public void testEmptyAuthorizedIndicesSearchForAll() { createIndices("index1", "index2"); - try { - client().prepareSearch().get(); - fail("Expected IndexNotFoundException"); - } catch (IndexNotFoundException e) { - assertThat(e.getMessage(), is("no such index")); - } + IndexNotFoundException e = expectThrows(IndexNotFoundException.class, () -> client().prepareSearch().get()); + assertEquals("no such index", e.getMessage()); } public void testEmptyAuthorizedIndicesSearchForWildcard() { createIndices("index1", "index2"); - try { - client().prepareSearch("*").get(); - fail("Expected IndexNotFoundException"); - } catch (IndexNotFoundException e) { - assertThat(e.getMessage(), is("no such index")); - } + IndexNotFoundException e = expectThrows(IndexNotFoundException.class, () -> client().prepareSearch("*").get()); + assertEquals("no such index", e.getMessage()); } public void testExplicitNonAuthorizedIndex() { @@ -187,14 +167,10 @@ public class IndicesAndAliasesResolverIntegrationTests extends SecurityIntegTest public void testMultiSearchWildcard() { //test4 is missing but authorized, only that specific item fails createIndices("test1", "test2", "test3", "index1"); - try { - client().prepareMultiSearch() - .add(Requests.searchRequest()) - .add(Requests.searchRequest("index*")).get(); - fail("Expected IndexNotFoundException"); - } catch (IndexNotFoundException e) { - assertThat(e.getMessage(), is("no such index")); - } + IndexNotFoundException e = expectThrows(IndexNotFoundException.class, + () -> client().prepareMultiSearch().add(Requests.searchRequest()) + .add(Requests.searchRequest("index*")).get()); + assertEquals("no such index", e.getMessage()); } private static void assertReturnedIndices(SearchResponse searchResponse, String... indices) { @@ -207,12 +183,8 @@ public class IndicesAndAliasesResolverIntegrationTests extends SecurityIntegTest } private static void assertThrowsAuthorizationException(ActionRequestBuilder actionRequestBuilder) { - try { - actionRequestBuilder.get(); - fail("search should fail due to attempt to access non authorized indices"); - } catch(ElasticsearchSecurityException e) { - assertAuthorizationException(e, containsString("is unauthorized for user [")); - } + ElasticsearchSecurityException e = expectThrows(ElasticsearchSecurityException.class, actionRequestBuilder::get); + assertAuthorizationException(e, containsString("is unauthorized for user [")); } private void createIndices(String... indices) { @@ -233,7 +205,6 @@ public class IndicesAndAliasesResolverIntegrationTests extends SecurityIntegTest } } - ensureGreen(); for (String index : indices) { client().prepareIndex(index, "type").setSource("field", "value").get(); } From 603db388d787422d764d759e67aba379081194bb Mon Sep 17 00:00:00 2001 From: javanna Date: Tue, 30 Aug 2016 15:02:13 +0200 Subject: [PATCH 2/7] Security: throw exception if we cannot extract indices from an indices request This used to be an assertion but we move it to an exception to be able to catch this at all times without requiring assertion enabled Original commit: elastic/x-pack-elasticsearch@fcb5fbe8524ba7c667a70a5e9422ba62fd9dc26a --- .../DefaultIndicesAndAliasesResolver.java | 9 +++------ 1 file changed, 3 insertions(+), 6 deletions(-) diff --git a/elasticsearch/x-pack/security/src/main/java/org/elasticsearch/xpack/security/authz/indicesresolver/DefaultIndicesAndAliasesResolver.java b/elasticsearch/x-pack/security/src/main/java/org/elasticsearch/xpack/security/authz/indicesresolver/DefaultIndicesAndAliasesResolver.java index 440ad73eab7..842f46c9e36 100644 --- a/elasticsearch/x-pack/security/src/main/java/org/elasticsearch/xpack/security/authz/indicesresolver/DefaultIndicesAndAliasesResolver.java +++ b/elasticsearch/x-pack/security/src/main/java/org/elasticsearch/xpack/security/authz/indicesresolver/DefaultIndicesAndAliasesResolver.java @@ -49,13 +49,10 @@ public class DefaultIndicesAndAliasesResolver implements IndicesAndAliasesResolv @Override public Set resolve(User user, String action, TransportRequest request, MetaData metaData) { - boolean isIndicesRequest = request instanceof CompositeIndicesRequest || request instanceof IndicesRequest; - assert isIndicesRequest : "Request [" + request + "] is not an Indices request, but should be."; - // if for some reason we are missing an action... just for safety we'll reject - if (!isIndicesRequest) { - return Collections.emptySet(); + if (isIndicesRequest == false) { + throw new IllegalStateException("Request [" + request + "] is not an Indices request, but should be."); } if (request instanceof CompositeIndicesRequest) { @@ -74,7 +71,7 @@ public class DefaultIndicesAndAliasesResolver implements IndicesAndAliasesResolv final Set indices; if (indicesRequest instanceof PutMappingRequest && ((PutMappingRequest) indicesRequest).getConcreteIndex() != null) { - /** + /* * This is a special case since PutMappingRequests from dynamic mapping updates have a concrete index * if this index is set and it's in the list of authorized indices we are good and don't need to put * the list of indices in there, if we do so it will result in an invalid request and the update will fail. From 942a70328c80a3b2e453200385fbb4f6524bb72b Mon Sep 17 00:00:00 2001 From: jaymode Date: Tue, 30 Aug 2016 12:57:42 -0400 Subject: [PATCH 3/7] test: smoke-test-plugins-ssl no longer relies on logging to start This change adds a HTTPS check for smoke-test-plugins-ssl so it no longer has to wait for a debug level log message. Closes elastic/elasticsearch#2303 Original commit: elastic/x-pack-elasticsearch@f3eaaad5d4af91da726c59b34fc7a8962d562260 --- .../qa/smoke-test-plugins-ssl/build.gradle | 63 ++++++++++++++----- elasticsearch/x-pack/build.gradle | 8 ++- .../xpack/monitoring/agent/AgentService.java | 1 - 3 files changed, 53 insertions(+), 19 deletions(-) diff --git a/elasticsearch/qa/smoke-test-plugins-ssl/build.gradle b/elasticsearch/qa/smoke-test-plugins-ssl/build.gradle index 049086e3811..533465d98ba 100644 --- a/elasticsearch/qa/smoke-test-plugins-ssl/build.gradle +++ b/elasticsearch/qa/smoke-test-plugins-ssl/build.gradle @@ -1,5 +1,14 @@ import org.elasticsearch.gradle.LoggedExec import org.elasticsearch.gradle.MavenFilteringHack +import org.elasticsearch.gradle.test.NodeInfo + +import javax.net.ssl.HttpsURLConnection +import javax.net.ssl.KeyManagerFactory +import javax.net.ssl.SSLContext +import javax.net.ssl.TrustManagerFactory +import java.nio.charset.StandardCharsets +import java.security.KeyStore +import java.security.SecureRandom apply plugin: 'elasticsearch.rest-test' @@ -177,25 +186,47 @@ integTest { setupCommand 'setupMonitoringUser', 'bin/x-pack/users', 'useradd', 'monitoring_agent', '-p', 'changeme', '-r', 'remote_monitoring_agent' - // Required to detect that the monitoring agent service has started - setting 'logger.level', 'DEBUG' - - waitCondition = { node, ant -> - // HTTPS check is tricky to do, so we wait for the log file to indicate that the node is started - String waitForNodeStartProp = "waitForNodeStart${name}" - ant.waitfor(maxwait: '30', maxwaitunit: 'second', checkevery: '100', checkeveryunit: 'millisecond', - timeoutproperty: waitForNodeStartProp) { - and { - resourcecontains(resource: "${node.startLog.toString()}", substring: 'started') - resourcecontains(resource: "${node.startLog.toString()}", substring: 'monitoring service started') + waitCondition = { NodeInfo node, AntBuilder ant -> + File tmpFile = new File(node.cwd, 'wait.success') + KeyStore keyStore = KeyStore.getInstance("JKS"); + keyStore.load(clientKeyStore.newInputStream(), 'keypass'.toCharArray()); + KeyManagerFactory kmf = KeyManagerFactory.getInstance(KeyManagerFactory.getDefaultAlgorithm()); + kmf.init(keyStore, 'keypass'.toCharArray()); + TrustManagerFactory tmf = TrustManagerFactory.getInstance(TrustManagerFactory.getDefaultAlgorithm()); + tmf.init(keyStore); + SSLContext sslContext = SSLContext.getInstance("TLSv1.2"); + sslContext.init(kmf.getKeyManagers(), tmf.getTrustManagers(), new SecureRandom()); + for (int i = 0; i < 10; i++) { + // we use custom wait logic here for HTTPS + HttpsURLConnection httpURLConnection = null; + try { + httpURLConnection = (HttpsURLConnection) new URL("https://${node.httpUri()}/_cluster/health?wait_for_nodes=${numNodes}").openConnection(); + httpURLConnection.setSSLSocketFactory(sslContext.getSocketFactory()); + httpURLConnection.setRequestProperty("Authorization", "Basic " + + Base64.getEncoder().encodeToString("test_user:changeme".getBytes(StandardCharsets.UTF_8))); + httpURLConnection.setRequestMethod("GET"); + httpURLConnection.connect(); + if (httpURLConnection.getResponseCode() == 200) { + tmpFile.withWriter StandardCharsets.UTF_8.name(), { + it.write(httpURLConnection.getInputStream().getText(StandardCharsets.UTF_8.name())) + } + } + } catch (IOException e) { + if (i == 9) { + logger.error("final attempt of calling cluster health failed", e) + } else { + logger.debug("failed to call cluster health", e) + } + } finally { + if (httpURLConnection != null) { + httpURLConnection.disconnect(); + } } - } - if (ant.project.getProperty(waitForNodeStartProp)) { - println "Timed out when looking for node startup in log file ${node.startLog.toString()}" - return false; + // did not start, so wait a bit before trying again + Thread.sleep(500L); } - return true; + return tmpFile.exists() } } } diff --git a/elasticsearch/x-pack/build.gradle b/elasticsearch/x-pack/build.gradle index 6f93876c826..e314451cdef 100644 --- a/elasticsearch/x-pack/build.gradle +++ b/elasticsearch/x-pack/build.gradle @@ -176,7 +176,7 @@ integTest { // we use custom wait logic here as the elastic user is not available immediately and ant.get will fail when a 401 is returned HttpURLConnection httpURLConnection = null; try { - httpURLConnection = (HttpURLConnection) new URL("http://${node.httpUri()}").openConnection(); + httpURLConnection = (HttpURLConnection) new URL("http://${node.httpUri()}/_cluster/health?wait_for_nodes=${numNodes}").openConnection(); httpURLConnection.setRequestProperty("Authorization", "Basic " + Base64.getEncoder().encodeToString("elastic:changeme".getBytes(StandardCharsets.UTF_8))); httpURLConnection.setRequestMethod("GET"); @@ -187,7 +187,11 @@ integTest { } } } catch (Exception e) { - e.printStackTrace() + if (i == 9) { + logger.error("final attempt of calling cluster health failed", e) + } else { + logger.debug("failed to call cluster health", e) + } } finally { if (httpURLConnection != null) { httpURLConnection.disconnect(); diff --git a/elasticsearch/x-pack/monitoring/src/main/java/org/elasticsearch/xpack/monitoring/agent/AgentService.java b/elasticsearch/x-pack/monitoring/src/main/java/org/elasticsearch/xpack/monitoring/agent/AgentService.java index bf10f959767..4ce85eeb09c 100644 --- a/elasticsearch/x-pack/monitoring/src/main/java/org/elasticsearch/xpack/monitoring/agent/AgentService.java +++ b/elasticsearch/x-pack/monitoring/src/main/java/org/elasticsearch/xpack/monitoring/agent/AgentService.java @@ -119,7 +119,6 @@ public class AgentService extends AbstractLifecycleComponent { @Override protected void doStart() { - // Please don't remove this log message since it can be used in integration tests logger.debug("monitoring service started"); for (Collector collector : collectors) { From 06ff97f63d1da2b28643d5e4990fc42a54f7585d Mon Sep 17 00:00:00 2001 From: jaymode Date: Wed, 31 Aug 2016 08:23:51 -0400 Subject: [PATCH 4/7] security: remove explicit handshake wait in netty4 transport Netty 4's SslHandler does not require the application to wait for the handshake to be completed before data is written. This change removes the explicit wait on each handshake future. Original commit: elastic/x-pack-elasticsearch@c19bcebb8330d6c2c19e55a5a4ced50b18b9bcbe --- .../netty4/SecurityNetty4Transport.java | 28 ------------------- 1 file changed, 28 deletions(-) diff --git a/elasticsearch/x-pack/security/src/main/java/org/elasticsearch/xpack/security/transport/netty4/SecurityNetty4Transport.java b/elasticsearch/x-pack/security/src/main/java/org/elasticsearch/xpack/security/transport/netty4/SecurityNetty4Transport.java index a58bed38d5a..4d3f40f7bd4 100644 --- a/elasticsearch/x-pack/security/src/main/java/org/elasticsearch/xpack/security/transport/netty4/SecurityNetty4Transport.java +++ b/elasticsearch/x-pack/security/src/main/java/org/elasticsearch/xpack/security/transport/netty4/SecurityNetty4Transport.java @@ -11,10 +11,7 @@ import io.netty.channel.ChannelHandlerContext; import io.netty.channel.ChannelOutboundHandlerAdapter; import io.netty.channel.ChannelPromise; import io.netty.handler.ssl.SslHandler; -import io.netty.util.concurrent.Future; -import org.elasticsearch.ElasticsearchException; import org.elasticsearch.common.SuppressForbidden; -import org.elasticsearch.common.collect.Tuple; import org.elasticsearch.common.inject.Inject; import org.elasticsearch.common.inject.internal.Nullable; import org.elasticsearch.common.io.stream.NamedWriteableRegistry; @@ -35,9 +32,6 @@ import javax.net.ssl.SSLParameters; import java.net.InetSocketAddress; import java.net.SocketAddress; -import java.util.ArrayList; -import java.util.List; -import java.util.concurrent.TimeUnit; import static org.elasticsearch.xpack.security.Security.setting; import static org.elasticsearch.xpack.security.Security.settingPrefix; @@ -131,28 +125,6 @@ public class SecurityNetty4Transport extends Netty4Transport { return new SecurityClientChannelInitializer(); } - /** - * This method ensures that all channels have their SSL handshakes completed. This is necessary to prevent the application from - * writing data while the handshake is in progress which could cause the handshake to fail. - */ - @Override - protected void onAfterChannelsConnected(NodeChannels nodeChannels) { - List, Channel>> handshakes = new ArrayList<>(); - for (Channel channel : nodeChannels.allChannels) { - SslHandler handler = channel.pipeline().get(SslHandler.class); - if (handler != null) { - handshakes.add(Tuple.tuple(handler.handshakeFuture(), channel)); - } - } - - for (Tuple, Channel> handshake : handshakes) { - handshake.v1().awaitUninterruptibly(30L, TimeUnit.SECONDS); - if (!handshake.v1().isSuccess()) { - throw new ElasticsearchException("handshake failed for channel [{}]", handshake.v2()); - } - } - } - class SecurityServerChannelInitializer extends ServerChannelInitializer { private final boolean sslEnabled; From 7d789110826a1b308426506af209ee1fac6214fb Mon Sep 17 00:00:00 2001 From: jaymode Date: Tue, 30 Aug 2016 09:01:26 -0400 Subject: [PATCH 5/7] security: limit the size of the role store cache Previously the roles store cache was unbounded as it was a just using a ConcurrentHashMap, which could lead to excessive memory usage in cases where there are a large number of roles as we tried to eagerly load the roles into the cache if they were not present. The roles store now loads roles on demand and caches them for a finite period of time. Additionally, the background polling of roles has been removed to reduce complexity. A best effort attempt is made to clear the roles cache upon modification and if necessary the cache can be cleared manually. See elastic/elasticsearch#1837 Original commit: elastic/x-pack-elasticsearch@450dd779c8f16822b11bf4e29631c4071bc295db --- .../xpack/security/Security.java | 2 +- .../authz/store/NativeRolesStore.java | 259 ++++++------------ .../integration/ClearRolesCacheTests.java | 88 +----- 3 files changed, 102 insertions(+), 247 deletions(-) diff --git a/elasticsearch/x-pack/security/src/main/java/org/elasticsearch/xpack/security/Security.java b/elasticsearch/x-pack/security/src/main/java/org/elasticsearch/xpack/security/Security.java index a13923c9590..fa4ab61f0ad 100644 --- a/elasticsearch/x-pack/security/src/main/java/org/elasticsearch/xpack/security/Security.java +++ b/elasticsearch/x-pack/security/src/main/java/org/elasticsearch/xpack/security/Security.java @@ -302,7 +302,7 @@ public class Security implements ActionPlugin, IngestPlugin { components.add(authcService); final FileRolesStore fileRolesStore = new FileRolesStore(settings, env, resourceWatcherService); - final NativeRolesStore nativeRolesStore = new NativeRolesStore(settings, client, threadPool); + final NativeRolesStore nativeRolesStore = new NativeRolesStore(settings, client); final ReservedRolesStore reservedRolesStore = new ReservedRolesStore(securityContext); final CompositeRolesStore allRolesStore = new CompositeRolesStore(settings, fileRolesStore, nativeRolesStore, reservedRolesStore); final AuthorizationService authzService = new AuthorizationService(settings, allRolesStore, clusterService, diff --git a/elasticsearch/x-pack/security/src/main/java/org/elasticsearch/xpack/security/authz/store/NativeRolesStore.java b/elasticsearch/x-pack/security/src/main/java/org/elasticsearch/xpack/security/authz/store/NativeRolesStore.java index df0a8d028ef..87e8abaf9eb 100644 --- a/elasticsearch/x-pack/security/src/main/java/org/elasticsearch/xpack/security/authz/store/NativeRolesStore.java +++ b/elasticsearch/x-pack/security/src/main/java/org/elasticsearch/xpack/security/authz/store/NativeRolesStore.java @@ -9,16 +9,14 @@ import java.util.ArrayList; import java.util.Arrays; import java.util.Collections; import java.util.HashMap; -import java.util.HashSet; import java.util.List; import java.util.Map; -import java.util.Set; -import java.util.concurrent.ConcurrentHashMap; import java.util.concurrent.CountDownLatch; +import java.util.concurrent.ExecutionException; import java.util.concurrent.TimeUnit; import java.util.concurrent.atomic.AtomicReference; -import java.util.function.BiFunction; -import java.util.function.Function; +import java.util.concurrent.locks.ReadWriteLock; +import java.util.concurrent.locks.ReentrantReadWriteLock; import org.elasticsearch.ElasticsearchException; import org.elasticsearch.action.ActionListener; @@ -38,18 +36,18 @@ import org.elasticsearch.action.search.SearchRequest; import org.elasticsearch.action.search.SearchResponse; import org.elasticsearch.action.search.SearchScrollRequest; import org.elasticsearch.action.support.WriteRequest.RefreshPolicy; -import org.elasticsearch.client.Client; import org.elasticsearch.cluster.ClusterChangedEvent; import org.elasticsearch.cluster.ClusterState; import org.elasticsearch.cluster.ClusterStateListener; import org.elasticsearch.common.Nullable; import org.elasticsearch.common.bytes.BytesReference; +import org.elasticsearch.common.cache.Cache; +import org.elasticsearch.common.cache.CacheBuilder; import org.elasticsearch.common.component.AbstractComponent; import org.elasticsearch.common.settings.Setting; import org.elasticsearch.common.settings.Setting.Property; import org.elasticsearch.common.settings.Settings; import org.elasticsearch.common.unit.TimeValue; -import org.elasticsearch.common.util.concurrent.AbstractRunnable; import org.elasticsearch.common.xcontent.ToXContent; import org.elasticsearch.gateway.GatewayService; import org.elasticsearch.index.IndexNotFoundException; @@ -57,8 +55,6 @@ import org.elasticsearch.index.get.GetResult; import org.elasticsearch.index.query.QueryBuilder; import org.elasticsearch.index.query.QueryBuilders; import org.elasticsearch.search.SearchHit; -import org.elasticsearch.threadpool.ThreadPool; -import org.elasticsearch.threadpool.ThreadPool.Names; import org.elasticsearch.xpack.security.InternalClient; import org.elasticsearch.xpack.security.SecurityTemplateService; import org.elasticsearch.xpack.security.action.role.ClearRolesCacheRequest; @@ -69,7 +65,6 @@ import org.elasticsearch.xpack.security.authz.RoleDescriptor; import org.elasticsearch.xpack.security.authz.permission.IndicesPermission.Group; import org.elasticsearch.xpack.security.authz.permission.Role; import org.elasticsearch.xpack.security.client.SecurityClient; -import org.elasticsearch.threadpool.ThreadPool.Cancellable; import static org.elasticsearch.common.xcontent.XContentFactory.jsonBuilder; import static org.elasticsearch.xpack.security.Security.setting; @@ -91,8 +86,10 @@ public class NativeRolesStore extends AbstractComponent implements RolesStore, C public static final Setting SCROLL_KEEP_ALIVE_SETTING = Setting.timeSetting(setting("authz.store.roles.index.scroll.keep_alive"), TimeValue.timeValueSeconds(10L), Property.NodeScope); - public static final Setting POLL_INTERVAL_SETTING = - Setting.timeSetting(setting("authz.store.roles.index.reload.interval"), TimeValue.timeValueSeconds(30L), Property.NodeScope); + private static final Setting CACHE_SIZE_SETTING = + Setting.intSetting(setting("authz.store.roles.index.cache.max_size"), 10000, Property.NodeScope); + private static final Setting CACHE_TTL_SETTING = + Setting.timeSetting(setting("authz.store.roles.index.cache.ttl"), TimeValue.timeValueMinutes(20), Property.NodeScope); public enum State { INITIALIZED, @@ -106,21 +103,27 @@ public class NativeRolesStore extends AbstractComponent implements RolesStore, C public static final String ROLE_DOC_TYPE = "role"; private final InternalClient client; - private final ThreadPool threadPool; private final AtomicReference state = new AtomicReference<>(State.INITIALIZED); - private final ConcurrentHashMap roleCache = new ConcurrentHashMap<>(); + private final Cache roleCache; + // the lock is used in an odd manner; when iterating over the cache we cannot have modifiers other than deletes using + // the iterator but when not iterating we can modify the cache without external locking. When making normal modifications to the cache + // the read lock is obtained so that we can allow concurrent modifications; however when we need to iterate over the keys or values of + // the cache the write lock must obtained to prevent any modifications + private final ReadWriteLock iterationLock = new ReentrantReadWriteLock(); private SecurityClient securityClient; private int scrollSize; private TimeValue scrollKeepAlive; - private Cancellable pollerCancellable; private volatile boolean securityIndexExists = false; - public NativeRolesStore(Settings settings, InternalClient client, ThreadPool threadPool) { + public NativeRolesStore(Settings settings, InternalClient client) { super(settings); this.client = client; - this.threadPool = threadPool; + this.roleCache = CacheBuilder.builder() + .setMaximumWeight(CACHE_SIZE_SETTING.get(settings)) + .setExpireAfterWrite(CACHE_TTL_SETTING.get(settings).getMillis()) + .build(); } public boolean canStart(ClusterState clusterState, boolean master) { @@ -144,15 +147,6 @@ public class NativeRolesStore extends AbstractComponent implements RolesStore, C this.securityClient = new SecurityClient(client); this.scrollSize = SCROLL_SIZE_SETTING.get(settings); this.scrollKeepAlive = SCROLL_KEEP_ALIVE_SETTING.get(settings); - TimeValue pollInterval = POLL_INTERVAL_SETTING.get(settings); - RolesStorePoller poller = new RolesStorePoller(); - try { - poller.doRun(); - } catch (Exception e) { - logger.warn("failed to perform initial poll of roles index [{}]. scheduling again in [{}]", e, - SecurityTemplateService.SECURITY_INDEX_NAME, pollInterval); - } - pollerCancellable = threadPool.scheduleWithFixedDelay(poller, pollInterval, Names.GENERIC); state.set(State.STARTED); } } catch (Exception e) { @@ -163,11 +157,7 @@ public class NativeRolesStore extends AbstractComponent implements RolesStore, C public void stop() { if (state.compareAndSet(State.STARTED, State.STOPPING)) { - try { - pollerCancellable.cancel(); - } finally { - state.set(State.STOPPED); - } + state.set(State.STOPPED); } } @@ -341,16 +331,21 @@ public class NativeRolesStore extends AbstractComponent implements RolesStore, C return usageStats; } - long count = (long) roleCache.size(); - for (RoleAndVersion rv : roleCache.values()) { - Role role = rv.getRole(); - for (Group group : role.indices()) { - fls = fls || group.hasFields(); - dls = dls || group.hasQuery(); - } - if (fls && dls) { - break; + long count = roleCache.count(); + iterationLock.writeLock().lock(); + try { + for (RoleAndVersion rv : roleCache.values()) { + Role role = rv.getRole(); + for (Group group : role.indices()) { + fls = fls || group.hasFields(); + dls = dls || group.hasQuery(); + } + if (fls && dls) { + break; + } } + } finally { + iterationLock.writeLock().unlock(); } // slow path - query for necessary information @@ -408,47 +403,53 @@ public class NativeRolesStore extends AbstractComponent implements RolesStore, C final AtomicReference getRef = new AtomicReference<>(null); final CountDownLatch latch = new CountDownLatch(1); try { - roleAndVersion = roleCache.computeIfAbsent(roleId, new Function() { - @Override - public RoleAndVersion apply(String key) { - logger.debug("attempting to load role [{}] from index", key); - executeGetRoleRequest(roleId, new LatchedActionListener<>(new ActionListener() { - @Override - public void onResponse(GetResponse role) { - getRef.set(role); + roleAndVersion = roleCache.computeIfAbsent(roleId, (key) -> { + logger.debug("attempting to load role [{}] from index", key); + executeGetRoleRequest(roleId, new LatchedActionListener<>(new ActionListener() { + @Override + public void onResponse(GetResponse role) { + getRef.set(role); + } + + @Override + public void onFailure(Exception t) { + if (t instanceof IndexNotFoundException) { + logger.trace("failed to retrieve role [{}] since security index does not exist", t, roleId); + } else { + logger.error("failed to retrieve role [{}]", t, roleId); } - - @Override - public void onFailure(Exception t) { - if (t instanceof IndexNotFoundException) { - logger.trace("failed to retrieve role [{}] since security index does not exist", t, roleId); - } else { - logger.error("failed to retrieve role [{}]", t, roleId); - } - } - }, latch)); - - try { - latch.await(30, TimeUnit.SECONDS); - } catch (InterruptedException e) { - logger.error("timed out retrieving role [{}]", roleId); } + }, latch)); - GetResponse response = getRef.get(); - if (response == null) { - return null; - } + try { + latch.await(30, TimeUnit.SECONDS); + } catch (InterruptedException e) { + logger.error("timed out retrieving role [{}]", roleId); + } - RoleDescriptor descriptor = transformRole(response); - if (descriptor == null) { - return null; - } - logger.debug("loaded role [{}] from index with version [{}]", key, response.getVersion()); + GetResponse response = getRef.get(); + if (response == null) { + return null; + } + + RoleDescriptor descriptor = transformRole(response); + if (descriptor == null) { + return null; + } + logger.debug("loaded role [{}] from index with version [{}]", key, response.getVersion()); + iterationLock.readLock().lock(); + try { return new RoleAndVersion(descriptor, response.getVersion()); + } finally { + iterationLock.readLock().unlock(); } }); - } catch (RuntimeException e) { - logger.error("could not get or load value from cache for role [{}]", e, roleId); + } catch (ExecutionException e) { + if (e.getCause() instanceof NullPointerException) { + logger.trace("role [{}] was not found", e, roleId); + } else { + logger.error("failed to load role [{}]", e, roleId); + } } return roleAndVersion; @@ -490,19 +491,29 @@ public class NativeRolesStore extends AbstractComponent implements RolesStore, C if (state != State.STOPPED && state != State.FAILED) { throw new IllegalStateException("can only reset if stopped!!!"); } - this.roleCache.clear(); + invalidateAll(); this.securityIndexExists = false; this.state.set(State.INITIALIZED); } public void invalidateAll() { logger.debug("invalidating all roles in cache"); - roleCache.clear(); + iterationLock.readLock().lock(); + try { + roleCache.invalidateAll(); + } finally { + iterationLock.readLock().unlock(); + } } public void invalidate(String role) { logger.debug("invalidating role [{}] in cache", role); - roleCache.remove(role); + iterationLock.readLock().lock(); + try { + roleCache.invalidate(role); + } finally { + iterationLock.readLock().unlock(); + } } private void clearRoleCache(final String role, ActionListener listener, Response response) { @@ -560,97 +571,6 @@ public class NativeRolesStore extends AbstractComponent implements RolesStore, C } } - private class RolesStorePoller extends AbstractRunnable { - - @Override - protected void doRun() throws Exception { - if (isStopped()) { - return; - } - if (securityIndexExists == false) { - logger.trace("cannot poll for role changes since security index [{}] does not exist", - SecurityTemplateService.SECURITY_INDEX_NAME); - return; - } - - // hold a reference to the client since the poller may run after the class is stopped (we don't interrupt it running) and - // we reset when we test which sets the client to null... - final Client client = NativeRolesStore.this.client; - - logger.trace("starting polling of roles index to check for changes"); - SearchResponse response = null; - // create a copy of the keys in the cache since we will be modifying this list - final Set existingRoles = new HashSet<>(roleCache.keySet()); - try { - client.admin().indices().prepareRefresh(SecurityTemplateService.SECURITY_INDEX_NAME); - SearchRequest request = client.prepareSearch(SecurityTemplateService.SECURITY_INDEX_NAME) - .setScroll(scrollKeepAlive) - .setQuery(QueryBuilders.typeQuery(ROLE_DOC_TYPE)) - .setSize(scrollSize) - .setFetchSource(true) - .setVersion(true) - .request(); - response = client.search(request).actionGet(); - - boolean keepScrolling = response.getHits().getHits().length > 0; - while (keepScrolling) { - if (isStopped()) { - return; - } - for (SearchHit hit : response.getHits().getHits()) { - final String roleName = hit.getId(); - final long version = hit.version(); - existingRoles.remove(roleName); - // we use the locking mechanisms provided by the map/cache to help protect against concurrent operations - // that will leave the cache in a bad state - roleCache.computeIfPresent(roleName, new BiFunction() { - @Override - public RoleAndVersion apply(String roleName, RoleAndVersion existing) { - if (version > existing.getVersion()) { - RoleDescriptor rd = transformRole(hit.getId(), hit.getSourceRef()); - if (rd != null) { - return new RoleAndVersion(rd, version); - } - } - return existing; - } - }); - } - SearchScrollRequest scrollRequest = client.prepareSearchScroll(response.getScrollId()) - .setScroll(scrollKeepAlive).request(); - response = client.searchScroll(scrollRequest).actionGet(); - keepScrolling = response.getHits().getHits().length > 0; - } - - // check to see if we had roles that do not exist in the index - if (existingRoles.isEmpty() == false) { - for (String roleName : existingRoles) { - logger.trace("role [{}] does not exist anymore, removing from cache", roleName); - roleCache.remove(roleName); - } - } - } catch (IndexNotFoundException e) { - logger.trace("security index does not exist", e); - } finally { - if (response != null) { - ClearScrollRequest clearScrollRequest = client.prepareClearScroll().addScrollId(response.getScrollId()).request(); - client.clearScroll(clearScrollRequest).actionGet(); - } - } - logger.trace("completed polling of roles index"); - } - - @Override - public void onFailure(Exception e) { - logger.error("error occurred while checking the native roles for changes", e); - } - - private boolean isStopped() { - State state = state(); - return state == State.STOPPED || state == State.STOPPING; - } - } - private static class RoleAndVersion { private final RoleDescriptor roleDescriptor; @@ -679,6 +599,7 @@ public class NativeRolesStore extends AbstractComponent implements RolesStore, C public static void addSettings(List> settings) { settings.add(SCROLL_SIZE_SETTING); settings.add(SCROLL_KEEP_ALIVE_SETTING); - settings.add(POLL_INTERVAL_SETTING); + settings.add(CACHE_SIZE_SETTING); + settings.add(CACHE_TTL_SETTING); } } diff --git a/elasticsearch/x-pack/security/src/test/java/org/elasticsearch/integration/ClearRolesCacheTests.java b/elasticsearch/x-pack/security/src/test/java/org/elasticsearch/integration/ClearRolesCacheTests.java index 87205804d63..f13fa46719e 100644 --- a/elasticsearch/x-pack/security/src/test/java/org/elasticsearch/integration/ClearRolesCacheTests.java +++ b/elasticsearch/x-pack/security/src/test/java/org/elasticsearch/integration/ClearRolesCacheTests.java @@ -5,25 +5,14 @@ */ package org.elasticsearch.integration; -import org.apache.http.message.BasicHeader; -import org.elasticsearch.action.DocWriteResponse; -import org.elasticsearch.action.delete.DeleteResponse; -import org.elasticsearch.action.update.UpdateResponse; import org.elasticsearch.client.Client; -import org.elasticsearch.client.Response; -import org.elasticsearch.common.Strings; import org.elasticsearch.common.network.NetworkModule; import org.elasticsearch.common.settings.Settings; -import org.elasticsearch.common.unit.TimeValue; -import org.elasticsearch.rest.RestStatus; import org.elasticsearch.test.NativeRealmIntegTestCase; -import org.elasticsearch.test.SecuritySettingsSource; import org.elasticsearch.xpack.security.SecurityTemplateService; +import org.elasticsearch.xpack.security.action.role.DeleteRoleResponse; import org.elasticsearch.xpack.security.action.role.GetRolesResponse; import org.elasticsearch.xpack.security.action.role.PutRoleResponse; -import org.elasticsearch.xpack.security.authc.support.SecuredString; -import org.elasticsearch.xpack.security.authc.support.UsernamePasswordToken; -import org.elasticsearch.xpack.security.authz.RoleDescriptor; import org.elasticsearch.xpack.security.authz.store.NativeRolesStore; import org.elasticsearch.xpack.security.client.SecurityClient; import org.junit.Before; @@ -34,14 +23,12 @@ import java.util.List; import static org.elasticsearch.action.support.WriteRequest.RefreshPolicy.IMMEDIATE; import static org.elasticsearch.action.support.WriteRequest.RefreshPolicy.NONE; -import static org.hamcrest.Matchers.arrayWithSize; import static org.hamcrest.Matchers.is; import static org.hamcrest.Matchers.notNullValue; /** - * Test for the clear roles API that changes the polling aspect of security to only run once an hour in order to - * test the cache clearing APIs. + * Test for the clear roles API */ public class ClearRolesCacheTests extends NativeRealmIntegTestCase { @@ -79,11 +66,8 @@ public class ClearRolesCacheTests extends NativeRealmIntegTestCase { @Override public Settings nodeSettings(int nodeOrdinal) { - TimeValue pollerInterval = TimeValue.timeValueMillis((long) randomIntBetween(2, 2000)); - logger.debug("using poller interval [{}]", pollerInterval); return Settings.builder() .put(super.nodeSettings(nodeOrdinal)) - .put(NativeRolesStore.POLL_INTERVAL_SETTING.getKey(), pollerInterval.getStringRep()) .put(NetworkModule.HTTP_ENABLED.getKey(), true) .build(); } @@ -109,67 +93,17 @@ public class ClearRolesCacheTests extends NativeRealmIntegTestCase { assertRolesAreCorrect(securityClient, toModify); } - public void testModifyingDocumentsDirectly() throws Exception { - int modifiedRolesCount = randomIntBetween(1, roles.length); - List toModify = randomSubsetOf(modifiedRolesCount, roles); - logger.debug("--> modifying roles {} to have run_as", toModify); - final boolean refresh = randomBoolean(); - for (String role : toModify) { - UpdateResponse response = internalClient().prepareUpdate().setId(role).setIndex(SecurityTemplateService.SECURITY_INDEX_NAME) - .setType(NativeRolesStore.ROLE_DOC_TYPE) - .setDoc("run_as", new String[] { role }) - .setRefreshPolicy(refresh ? IMMEDIATE : NONE) - .get(); - assertEquals(DocWriteResponse.Result.UPDATED, response.getResult()); - logger.debug("--> updated role [{}] with run_as", role); + public void testDeletingViaApiClearsCache() throws Exception { + final int rolesToDelete = randomIntBetween(1, roles.length - 1); + List toDelete = randomSubsetOf(rolesToDelete, roles); + for (String role : toDelete) { + DeleteRoleResponse response = securityClient().prepareDeleteRole(role).get(); + assertTrue(response.found()); } - // in this test, the poller runs too frequently to check the cache still has roles without run as - // clear the cache and we should definitely see the latest values! - SecurityClient securityClient = securityClient(internalCluster().transportClient()); - final boolean useHttp = randomBoolean(); - final boolean clearAll = randomBoolean(); - logger.debug("--> starting to clear roles. using http [{}] clearing all [{}]", useHttp, clearAll); - String[] rolesToClear = clearAll ? (randomBoolean() ? roles : null) : toModify.toArray(new String[toModify.size()]); - if (useHttp) { - String path; - if (rolesToClear == null) { - path = "/_xpack/security/role/" + (randomBoolean() ? "*" : "_all") + "/_clear_cache"; - } else { - path = "/_xpack/security/role/" + Strings.arrayToCommaDelimitedString(rolesToClear) + "/_clear_cache"; - } - Response response = getRestClient().performRequest("POST", path, - new BasicHeader(UsernamePasswordToken.BASIC_AUTH_HEADER, - UsernamePasswordToken.basicAuthHeaderValue(SecuritySettingsSource.DEFAULT_USER_NAME, - new SecuredString(SecuritySettingsSource.DEFAULT_PASSWORD.toCharArray())))); - assertThat(response.getStatusLine().getStatusCode(), is(RestStatus.OK.getStatus())); - } else { - securityClient.prepareClearRolesCache().names(rolesToClear).get(); - } - - assertRolesAreCorrect(securityClient, toModify); - } - - public void testDeletingRoleDocumentDirectly() throws Exception { - SecurityClient securityClient = securityClient(internalCluster().transportClient()); - - final String role = randomFrom(roles); - RoleDescriptor[] foundRoles = securityClient.prepareGetRoles().names(role).get().roles(); - assertThat(foundRoles.length, is(1)); - logger.debug("--> deleting role [{}]", role); - final boolean refresh = randomBoolean(); - DeleteResponse response = internalClient() - .prepareDelete(SecurityTemplateService.SECURITY_INDEX_NAME, NativeRolesStore.ROLE_DOC_TYPE, role) - .setRefreshPolicy(refresh ? IMMEDIATE : NONE) - .get(); - assertEquals(DocWriteResponse.Result.DELETED, response.getResult()); - - assertBusy(new Runnable() { - @Override - public void run() { - assertThat(securityClient.prepareGetRoles().names(role).get().roles(), arrayWithSize(0)); - } - }); + GetRolesResponse roleResponse = securityClient().prepareGetRoles().names(roles).get(); + assertTrue(roleResponse.hasRoles()); + assertThat(roleResponse.roles().length, is(roles.length - rolesToDelete)); } private void assertRolesAreCorrect(SecurityClient securityClient, List toModify) { From 82079185c2716690f7309c60e7e2c2a8a9faee13 Mon Sep 17 00:00:00 2001 From: Jason Tedor Date: Wed, 31 Aug 2016 10:18:09 -0400 Subject: [PATCH 6/7] Use releasable locks in NativeRolesStore This commit replaces the use of try/finally blocks to handle safe locking/unlocking for role cache read/write lock with releasable locks in try-with-resources blocks. Relates elastic/elasticsearch#3278 Original commit: elastic/x-pack-elasticsearch@fbd659cd85a0d0b9883687f62cce700a6c1ec77f --- .../authz/store/NativeRolesStore.java | 30 ++++++++----------- 1 file changed, 13 insertions(+), 17 deletions(-) diff --git a/elasticsearch/x-pack/security/src/main/java/org/elasticsearch/xpack/security/authz/store/NativeRolesStore.java b/elasticsearch/x-pack/security/src/main/java/org/elasticsearch/xpack/security/authz/store/NativeRolesStore.java index 87e8abaf9eb..b0f6e99aabf 100644 --- a/elasticsearch/x-pack/security/src/main/java/org/elasticsearch/xpack/security/authz/store/NativeRolesStore.java +++ b/elasticsearch/x-pack/security/src/main/java/org/elasticsearch/xpack/security/authz/store/NativeRolesStore.java @@ -48,6 +48,7 @@ import org.elasticsearch.common.settings.Setting; import org.elasticsearch.common.settings.Setting.Property; import org.elasticsearch.common.settings.Settings; import org.elasticsearch.common.unit.TimeValue; +import org.elasticsearch.common.util.concurrent.ReleasableLock; import org.elasticsearch.common.xcontent.ToXContent; import org.elasticsearch.gateway.GatewayService; import org.elasticsearch.index.IndexNotFoundException; @@ -109,7 +110,14 @@ public class NativeRolesStore extends AbstractComponent implements RolesStore, C // the iterator but when not iterating we can modify the cache without external locking. When making normal modifications to the cache // the read lock is obtained so that we can allow concurrent modifications; however when we need to iterate over the keys or values of // the cache the write lock must obtained to prevent any modifications - private final ReadWriteLock iterationLock = new ReentrantReadWriteLock(); + private final ReleasableLock readLock; + private final ReleasableLock writeLock; + + { + final ReadWriteLock iterationLock = new ReentrantReadWriteLock(); + readLock = new ReleasableLock(iterationLock.readLock()); + writeLock = new ReleasableLock(iterationLock.writeLock()); + } private SecurityClient securityClient; private int scrollSize; @@ -332,8 +340,7 @@ public class NativeRolesStore extends AbstractComponent implements RolesStore, C } long count = roleCache.count(); - iterationLock.writeLock().lock(); - try { + try (final ReleasableLock ignored = writeLock.acquire()) { for (RoleAndVersion rv : roleCache.values()) { Role role = rv.getRole(); for (Group group : role.indices()) { @@ -344,8 +351,6 @@ public class NativeRolesStore extends AbstractComponent implements RolesStore, C break; } } - } finally { - iterationLock.writeLock().unlock(); } // slow path - query for necessary information @@ -437,11 +442,8 @@ public class NativeRolesStore extends AbstractComponent implements RolesStore, C return null; } logger.debug("loaded role [{}] from index with version [{}]", key, response.getVersion()); - iterationLock.readLock().lock(); - try { + try (final ReleasableLock ignored = readLock.acquire()) { return new RoleAndVersion(descriptor, response.getVersion()); - } finally { - iterationLock.readLock().unlock(); } }); } catch (ExecutionException e) { @@ -498,21 +500,15 @@ public class NativeRolesStore extends AbstractComponent implements RolesStore, C public void invalidateAll() { logger.debug("invalidating all roles in cache"); - iterationLock.readLock().lock(); - try { + try (final ReleasableLock ignored = readLock.acquire()) { roleCache.invalidateAll(); - } finally { - iterationLock.readLock().unlock(); } } public void invalidate(String role) { logger.debug("invalidating role [{}] in cache", role); - iterationLock.readLock().lock(); - try { + try (final ReleasableLock ignored = readLock.acquire()) { roleCache.invalidate(role); - } finally { - iterationLock.readLock().unlock(); } } From 8757c2f6e1c321b743114a413873f82636304046 Mon Sep 17 00:00:00 2001 From: Ali Beyad Date: Mon, 29 Aug 2016 14:46:35 -0400 Subject: [PATCH 7/7] Changes tests to conform with new cluster health API, calling setWaitForNoRelocatingShards(true) instead of setWaitForRelocatingShards(0) Original commit: elastic/x-pack-elasticsearch@c7c12fe64cfa6caec5412b80d677b3723d764743 --- .../java/org/elasticsearch/xpack/security/MigrateToolIT.java | 5 +---- .../org/elasticsearch/license/TribeTransportTestCase.java | 2 +- .../xpack/security/audit/index/IndexAuditTrailTests.java | 2 +- 3 files changed, 3 insertions(+), 6 deletions(-) diff --git a/elasticsearch/qa/security-migrate-tests/src/test/java/org/elasticsearch/xpack/security/MigrateToolIT.java b/elasticsearch/qa/security-migrate-tests/src/test/java/org/elasticsearch/xpack/security/MigrateToolIT.java index 00361ffefe2..a1989873f09 100644 --- a/elasticsearch/qa/security-migrate-tests/src/test/java/org/elasticsearch/xpack/security/MigrateToolIT.java +++ b/elasticsearch/qa/security-migrate-tests/src/test/java/org/elasticsearch/xpack/security/MigrateToolIT.java @@ -13,11 +13,8 @@ import org.elasticsearch.client.Client; import org.elasticsearch.client.Requests; import org.elasticsearch.common.Priority; import org.elasticsearch.common.Strings; -import org.elasticsearch.common.bytes.BytesArray; import org.elasticsearch.common.settings.Settings; import org.elasticsearch.common.unit.TimeValue; -import org.elasticsearch.env.Environment; -import org.elasticsearch.xpack.security.SecurityTemplateService; import org.elasticsearch.xpack.security.action.role.GetRolesResponse; import org.elasticsearch.xpack.security.action.user.GetUsersResponse; import org.elasticsearch.xpack.security.action.user.PutUserResponse; @@ -123,7 +120,7 @@ public class MigrateToolIT extends MigrateToolTestCase { .timeout(TimeValue.timeValueSeconds(30)) .waitForYellowStatus() .waitForEvents(Priority.LANGUID) - .waitForRelocatingShards(0)) + .waitForNoRelocatingShards(true)) .actionGet(); SearchResponse searchResp = client.filterWithHeader(Collections.singletonMap("Authorization", token)).prepareSearch("index1").get(); } diff --git a/elasticsearch/x-pack/license-plugin/src/test/java/org/elasticsearch/license/TribeTransportTestCase.java b/elasticsearch/x-pack/license-plugin/src/test/java/org/elasticsearch/license/TribeTransportTestCase.java index 08c066e5f28..69154a2edb9 100644 --- a/elasticsearch/x-pack/license-plugin/src/test/java/org/elasticsearch/license/TribeTransportTestCase.java +++ b/elasticsearch/x-pack/license-plugin/src/test/java/org/elasticsearch/license/TribeTransportTestCase.java @@ -203,7 +203,7 @@ public abstract class TribeTransportTestCase extends ESIntegTestCase { private void ensureYellow(TestCluster testCluster) { ClusterHealthResponse actionGet = testCluster.client().admin().cluster() .health(Requests.clusterHealthRequest().waitForYellowStatus() - .waitForEvents(Priority.LANGUID).waitForRelocatingShards(0)).actionGet(); + .waitForEvents(Priority.LANGUID).waitForNoRelocatingShards(true)).actionGet(); if (actionGet.isTimedOut()) { logger.info("ensureGreen timed out, cluster state:\n{}\n{}", testCluster.client().admin().cluster() .prepareState().get().getState().prettyPrint(), diff --git a/elasticsearch/x-pack/security/src/test/java/org/elasticsearch/xpack/security/audit/index/IndexAuditTrailTests.java b/elasticsearch/x-pack/security/src/test/java/org/elasticsearch/xpack/security/audit/index/IndexAuditTrailTests.java index 7118f687fc5..6ed3520f046 100644 --- a/elasticsearch/x-pack/security/src/test/java/org/elasticsearch/xpack/security/audit/index/IndexAuditTrailTests.java +++ b/elasticsearch/x-pack/security/src/test/java/org/elasticsearch/xpack/security/audit/index/IndexAuditTrailTests.java @@ -736,7 +736,7 @@ public class IndexAuditTrailTests extends SecurityIntegTestCase { // pretty ugly but just a rip of ensureYellow that uses a different client ClusterHealthResponse actionGet = getClient().admin().cluster().health(Requests.clusterHealthRequest(indices) - .waitForRelocatingShards(0).waitForYellowStatus().waitForEvents(Priority.LANGUID)).actionGet(); + .waitForNoRelocatingShards(true).waitForYellowStatus().waitForEvents(Priority.LANGUID)).actionGet(); if (actionGet.isTimedOut()) { logger.info("ensureYellow timed out, cluster state:\n{}\n{}", getClient().admin().cluster().prepareState().get().getState().prettyPrint(),