You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@knox.apache.org by kr...@apache.org on 2020/01/27 18:28:49 UTC
[knox] branch master updated: KNOX-2200 - DefaultKeystoreService
can lose entries under concurrent access (#243)
This is an automated email from the ASF dual-hosted git repository.
krisden pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/knox.git
The following commit(s) were added to refs/heads/master by this push:
new 5e9b53a KNOX-2200 - DefaultKeystoreService can lose entries under concurrent access (#243)
5e9b53a is described below
commit 5e9b53ad8d5b8a6fe65b8995b17dae8d56d1aca4
Author: Kevin Risden <ri...@users.noreply.github.com>
AuthorDate: Mon Jan 27 13:27:19 2020 -0500
KNOX-2200 - DefaultKeystoreService can lose entries under concurrent access (#243)
Signed-off-by: Kevin Risden <kr...@apache.org>
---
.../security/impl/DefaultKeystoreService.java | 55 ++++++++------
.../security/impl/DefaultKeystoreServiceTest.java | 87 +++++++++++++++++++++-
2 files changed, 117 insertions(+), 25 deletions(-)
diff --git a/gateway-server/src/main/java/org/apache/knox/gateway/services/security/impl/DefaultKeystoreService.java b/gateway-server/src/main/java/org/apache/knox/gateway/services/security/impl/DefaultKeystoreService.java
index a195c1f..18acc74 100644
--- a/gateway-server/src/main/java/org/apache/knox/gateway/services/security/impl/DefaultKeystoreService.java
+++ b/gateway-server/src/main/java/org/apache/knox/gateway/services/security/impl/DefaultKeystoreService.java
@@ -76,7 +76,8 @@ public class DefaultKeystoreService implements KeystoreService, Service {
private static GatewayResources RES = ResourcesFactory.get(GatewayResources.class);
//let's configure the cache with hard-coded attributes now; we can introduce new gateway configuration later on if needed
- private final Cache<CacheKey, String> cache = Caffeine.newBuilder().expireAfterAccess(10, TimeUnit.MINUTES).maximumSize(1000).build();
+ // visible for testing
+ final Cache<CacheKey, String> cache = Caffeine.newBuilder().expireAfterAccess(10, TimeUnit.MINUTES).maximumSize(1000).build();
private GatewayConfig config;
private MasterService masterService;
@@ -286,18 +287,21 @@ public class DefaultKeystoreService implements KeystoreService, Service {
@Override
public void addCredentialForCluster(String clusterName, String alias, String value)
throws KeystoreServiceException {
- removeFromCache(clusterName, alias);
- KeyStore ks = getCredentialStoreForCluster(clusterName);
- if (ks != null) {
- try {
- final Key key = new SecretKeySpec(value.getBytes(StandardCharsets.UTF_8), "AES");
- ks.setKeyEntry(alias, key, masterService.getMasterSecret(), null);
-
- final Path keyStoreFilePath = keyStoreDirPath.resolve(clusterName + CREDENTIALS_SUFFIX);
- writeKeyStoreToFile(ks, keyStoreFilePath, masterService.getMasterSecret());
- addToCache(clusterName, alias, value);
- } catch (KeyStoreException | IOException | CertificateException | NoSuchAlgorithmException e) {
- LOG.failedToAddCredentialForCluster(clusterName, e);
+ // Needed to prevent read then write synchronization issue where alias is not added
+ synchronized (this) {
+ removeFromCache(clusterName, alias);
+ KeyStore ks = getCredentialStoreForCluster(clusterName);
+ if (ks != null) {
+ try {
+ final Key key = new SecretKeySpec(value.getBytes(StandardCharsets.UTF_8), "AES");
+ ks.setKeyEntry(alias, key, masterService.getMasterSecret(), null);
+
+ final Path keyStoreFilePath = keyStoreDirPath.resolve(clusterName + CREDENTIALS_SUFFIX);
+ writeKeyStoreToFile(ks, keyStoreFilePath, masterService.getMasterSecret());
+ addToCache(clusterName, alias, value);
+ } catch (KeyStoreException | IOException | CertificateException | NoSuchAlgorithmException e) {
+ LOG.failedToAddCredentialForCluster(clusterName, e);
+ }
}
}
}
@@ -329,18 +333,21 @@ public class DefaultKeystoreService implements KeystoreService, Service {
@Override
public void removeCredentialForCluster(String clusterName, String alias) throws KeystoreServiceException {
- KeyStore ks = getCredentialStoreForCluster(clusterName);
- if (ks != null) {
- try {
- if (ks.containsAlias(alias)) {
- ks.deleteEntry(alias);
- }
+ // Needed to prevent read then write synchronization issue where alias is not removed
+ synchronized (this) {
+ KeyStore ks = getCredentialStoreForCluster(clusterName);
+ if (ks != null) {
+ try {
+ if (ks.containsAlias(alias)) {
+ ks.deleteEntry(alias);
+ }
- final Path keyStoreFilePath = keyStoreDirPath.resolve(clusterName + CREDENTIALS_SUFFIX);
- writeKeyStoreToFile(ks, keyStoreFilePath, masterService.getMasterSecret());
- removeFromCache(clusterName, alias);
- } catch (KeyStoreException | IOException | CertificateException | NoSuchAlgorithmException e) {
- LOG.failedToRemoveCredentialForCluster(clusterName, e);
+ final Path keyStoreFilePath = keyStoreDirPath.resolve(clusterName + CREDENTIALS_SUFFIX);
+ writeKeyStoreToFile(ks, keyStoreFilePath, masterService.getMasterSecret());
+ removeFromCache(clusterName, alias);
+ } catch (KeyStoreException | IOException | CertificateException | NoSuchAlgorithmException e) {
+ LOG.failedToRemoveCredentialForCluster(clusterName, e);
+ }
}
}
}
diff --git a/gateway-server/src/test/java/org/apache/knox/gateway/services/security/impl/DefaultKeystoreServiceTest.java b/gateway-server/src/test/java/org/apache/knox/gateway/services/security/impl/DefaultKeystoreServiceTest.java
index b970934..8c2b1b5 100644
--- a/gateway-server/src/test/java/org/apache/knox/gateway/services/security/impl/DefaultKeystoreServiceTest.java
+++ b/gateway-server/src/test/java/org/apache/knox/gateway/services/security/impl/DefaultKeystoreServiceTest.java
@@ -32,6 +32,8 @@ import static org.easymock.EasyMock.eq;
import static org.easymock.EasyMock.expect;
import static org.easymock.EasyMock.replay;
import static org.easymock.EasyMock.verify;
+import static org.hamcrest.CoreMatchers.is;
+import static org.hamcrest.MatcherAssert.assertThat;
import static org.junit.Assert.assertEquals;
import static org.junit.Assert.assertFalse;
import static org.junit.Assert.assertNotNull;
@@ -47,6 +49,7 @@ import org.apache.knox.gateway.services.security.KeystoreService;
import org.apache.knox.gateway.services.security.KeystoreServiceException;
import org.apache.knox.gateway.services.security.MasterService;
import org.apache.knox.gateway.util.X509CertificateUtil;
+import org.easymock.IAnswer;
import org.junit.Rule;
import org.junit.Test;
import org.junit.rules.TemporaryFolder;
@@ -68,7 +71,13 @@ import java.security.cert.Certificate;
import java.security.cert.CertificateException;
import java.security.cert.X509Certificate;
import java.util.Collections;
+import java.util.HashSet;
import java.util.Locale;
+import java.util.Set;
+import java.util.concurrent.Callable;
+import java.util.concurrent.ExecutorService;
+import java.util.concurrent.Executors;
+import java.util.concurrent.TimeUnit;
public class DefaultKeystoreServiceTest {
@Rule
@@ -523,6 +532,82 @@ public class DefaultKeystoreServiceTest {
verify(masterService);
}
+ @Test
+ public void testAddRemoveCredentialForClusterSynchronization() throws Exception {
+ char[] masterPassword = "master_password".toCharArray();
+
+ MasterService masterService = createMock(MasterService.class);
+ IAnswer<? extends char[]> iAnswer = (IAnswer<char[]>) () -> {
+ // Wait .5-1 seconds to return
+// Thread.sleep(ThreadLocalRandom.current().nextInt(500, 1000));
+ return masterPassword;
+ };
+ expect(masterService.getMasterSecret()).andAnswer(iAnswer).anyTimes();
+ replay(masterService);
+
+ Path baseDir = testFolder.newFolder().toPath();
+ GatewayConfigImpl config = createGatewayConfig(baseDir);
+
+ DefaultKeystoreService keystoreService = new DefaultKeystoreService();
+ keystoreService.setMasterService(masterService);
+ keystoreService.init(config, Collections.emptyMap());
+
+ String clusterName = "cluster";
+ keystoreService.createCredentialStoreForCluster(clusterName);
+
+ int numberTotalRequests = 10;
+
+ Set<Callable<Void>> addCallables = new HashSet<>(numberTotalRequests);
+ for (int i = 0; i < numberTotalRequests; i++) {
+ String alias = "alias" + i;
+ String value = "value" + i;
+ addCallables.add(() -> {
+ keystoreService.addCredentialForCluster(clusterName, alias, value);
+ return null;
+ });
+ }
+
+ ExecutorService insertExecutor = Executors.newFixedThreadPool(numberTotalRequests);
+ insertExecutor.invokeAll(addCallables);
+ insertExecutor.shutdown();
+ insertExecutor.awaitTermination(5, TimeUnit.SECONDS);
+ assertThat(insertExecutor.isTerminated(), is(true));
+
+ // Ensure not to use cache
+ keystoreService.cache.invalidateAll();
+
+ for (int i = 0; i < numberTotalRequests; i++) {
+ String alias = "alias" + i;
+ String value = "value" + i;
+ assertEquals(value, String.valueOf(keystoreService.getCredentialForCluster(clusterName, alias)));
+ }
+
+ Set<Callable<Void>> removeCallables = new HashSet<>(numberTotalRequests);
+ for (int i = 0; i < numberTotalRequests; i++) {
+ String alias = "alias" + i;
+ removeCallables.add(() -> {
+ keystoreService.removeCredentialForCluster(clusterName, alias);
+ return null;
+ });
+ }
+
+ ExecutorService removeExecutor = Executors.newFixedThreadPool(numberTotalRequests);
+ removeExecutor.invokeAll(removeCallables);
+ removeExecutor.shutdown();
+ removeExecutor.awaitTermination(5, TimeUnit.SECONDS);
+ assertThat(removeExecutor.isTerminated(), is(true));
+
+ // Ensure not to use cache
+ keystoreService.cache.invalidateAll();
+
+ for (int i = 0; i < numberTotalRequests; i++) {
+ String alias = "alias" + i;
+ assertNull(keystoreService.getCredentialForCluster(clusterName, alias));
+ }
+
+ verify(masterService);
+ }
+
private void testAddSelfSignedCertForGateway(String hostname) throws Exception {
char[] masterPassword = "master_password".toCharArray();
@@ -647,4 +732,4 @@ public class DefaultKeystoreServiceTest {
keystoreService.writeKeyStoreToFile(keystore, keystoreFilePath, password);
}
-}
\ No newline at end of file
+}