knox-commits mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From kris...@apache.org
Subject [knox] branch master updated: KNOX-2200 - DefaultKeystoreService can lose entries under concurrent access (#243)
Date Mon, 27 Jan 2020 18:28:49 GMT
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 <risdenk@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 <krisden@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
+}


Mime
View raw message