This is an automated email from the ASF dual-hosted git repository.
smolnar 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 4be5227 KNOX-2136 - Caching credentials in DefaultKeystoreService
when an alias is being added or loaded from keystore and using a different
cache implementation (#213)
4be5227 is described below
commit 4be5227269695efc862d2876e65f10dc18c40c33
Author: Sandor Molnar <[email protected]>
AuthorDate: Fri Dec 13 07:50:16 2019 +0100
KNOX-2136 - Caching credentials in DefaultKeystoreService when an alias is
being added or loaded from keystore and using a different cache implementation
(#213)
---
gateway-server/pom.xml | 4 ++
.../security/impl/DefaultKeystoreService.java | 60 ++++++++++++++--------
.../security/impl/DefaultKeystoreServiceTest.java | 33 ++++++++++++
pom.xml | 16 ++++++
4 files changed, 91 insertions(+), 22 deletions(-)
diff --git a/gateway-server/pom.xml b/gateway-server/pom.xml
index dd7eb26..2924c22 100644
--- a/gateway-server/pom.xml
+++ b/gateway-server/pom.xml
@@ -371,6 +371,10 @@
<groupId>org.apache.knox</groupId>
<artifactId>gateway-util-configinjector</artifactId>
</dependency>
+ <dependency>
+ <groupId>com.github.ben-manes.caffeine</groupId>
+ <artifactId>caffeine</artifactId>
+ </dependency>
<!-- ********** ********** ********** ********** ********** **********
-->
<!-- ********** Test Dependencies **********
-->
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 6d6445a..a195c1f 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
@@ -19,6 +19,8 @@ package org.apache.knox.gateway.services.security.impl;
import static
org.apache.knox.gateway.services.security.AliasService.NO_CLUSTER_NAME;
+import org.apache.commons.lang3.builder.EqualsBuilder;
+import org.apache.commons.lang3.builder.HashCodeBuilder;
import org.apache.knox.gateway.GatewayMessages;
import org.apache.knox.gateway.GatewayResources;
import org.apache.knox.gateway.config.GatewayConfig;
@@ -31,6 +33,9 @@ import
org.apache.knox.gateway.services.security.KeystoreServiceException;
import org.apache.knox.gateway.services.security.MasterService;
import org.apache.knox.gateway.util.X509CertificateUtil;
+import com.github.benmanes.caffeine.cache.Cache;
+import com.github.benmanes.caffeine.cache.Caffeine;
+
import java.io.IOException;
import java.io.InputStream;
import java.io.OutputStream;
@@ -54,10 +59,9 @@ import java.security.cert.Certificate;
import java.security.cert.CertificateException;
import java.security.cert.X509Certificate;
import java.text.MessageFormat;
-import java.util.HashMap;
import java.util.Locale;
import java.util.Map;
-import java.util.concurrent.ConcurrentHashMap;
+import java.util.concurrent.TimeUnit;
import javax.crypto.spec.SecretKeySpec;
@@ -71,8 +75,9 @@ public class DefaultKeystoreService implements
KeystoreService, Service {
private static GatewayMessages LOG =
MessagesFactory.get(GatewayMessages.class);
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();
private GatewayConfig config;
- private Map<String, Map<String, String>> cache = new ConcurrentHashMap<>();
private MasterService masterService;
private Path keyStoreDirPath;
@@ -290,6 +295,7 @@ public class DefaultKeystoreService implements
KeystoreService, Service {
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);
}
@@ -323,7 +329,6 @@ public class DefaultKeystoreService implements
KeystoreService, Service {
@Override
public void removeCredentialForCluster(String clusterName, String alias)
throws KeystoreServiceException {
- removeFromCache(clusterName, alias);
KeyStore ks = getCredentialStoreForCluster(clusterName);
if (ks != null) {
try {
@@ -333,6 +338,7 @@ public class DefaultKeystoreService implements
KeystoreService, Service {
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);
}
@@ -343,36 +349,22 @@ public class DefaultKeystoreService implements
KeystoreService, Service {
* Called only from within critical sections of other methods above.
*/
private char[] checkCache(String clusterName, String alias) {
- char[] c = null;
- String cred;
- Map<String, String> clusterCache = cache.get(clusterName);
- if (clusterCache == null) {
- return null;
- }
- cred = clusterCache.get(alias);
- if (cred != null) {
- c = cred.toCharArray();
- }
- return c;
+ final String cachedCredential =
cache.getIfPresent(CacheKey.of(clusterName, alias));
+ return cachedCredential == null ? null : cachedCredential.toCharArray();
}
/**
* Called only from within critical sections of other methods above.
*/
private void addToCache(String clusterName, String alias, String
credentialString) {
- Map<String, String> clusterCache = cache.computeIfAbsent(clusterName, k ->
new HashMap<>());
- clusterCache.put(alias, credentialString);
+ cache.put(CacheKey.of(clusterName, alias), credentialString);
}
/**
* Called only from within critical sections of other methods above.
*/
private void removeFromCache(String clusterName, String alias) {
- Map<String, String> clusterCache = cache.get(clusterName);
- if (clusterCache == null) {
- return;
- }
- clusterCache.remove(alias);
+ cache.invalidate(CacheKey.of(clusterName, alias));
}
@Override
@@ -508,4 +500,28 @@ public class DefaultKeystoreService implements
KeystoreService, Service {
}
return (password == null) ? masterService.getMasterSecret() : password;
}
+
+ private static class CacheKey {
+ private final String clusterName;
+ private final String alias;
+
+ private CacheKey(String clusterName, String alias) {
+ this.clusterName = clusterName;
+ this.alias = alias;
+ }
+
+ private static CacheKey of(String clusterName, String alias) {
+ return new CacheKey(clusterName, alias);
+ }
+
+ @Override
+ public int hashCode() {
+ return HashCodeBuilder.reflectionHashCode(this);
+ }
+
+ @Override
+ public boolean equals(Object obj) {
+ return EqualsBuilder.reflectionEquals(this, obj);
+ }
+ }
}
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 f2971f9..b970934 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
@@ -409,6 +409,39 @@ public class DefaultKeystoreServiceTest {
}
@Test
+ public void testCredentialCache() throws Exception {
+ final String clusterName = "my_cluster";
+ final String credentialAlias = "my_alias";
+ final String credentialValue = "my_value";
+
+ final MasterService masterService = createMock(MasterService.class);
+
expect(masterService.getMasterSecret()).andReturn("master_password".toCharArray()).anyTimes();
+ replay(masterService);
+
+ final GatewayConfigImpl gatewayConfig =
createGatewayConfig(testFolder.newFolder().toPath());
+
+ final DefaultKeystoreService keystoreService = new
DefaultKeystoreService();
+ keystoreService.setMasterService(masterService);
+ keystoreService.init(gatewayConfig, null);
+
+ keystoreService.addCredentialForCluster(clusterName, credentialAlias,
credentialValue);
+
+ // changing the security folder so that we "invalidate" the previously
created
+ // keystore and rely on the cache when fetching a credential for an alias
+ gatewayConfig.set(GatewayConfigImpl.SECURITY_DIR,
gatewayConfig.getGatewaySecurityDir() + "_other");
+ keystoreService.init(gatewayConfig, null);
+
+ // A request for a existing alias should return the expected value from
cache.
+ // Since the service now has a new security folder set it should return
'null'
+ // if the cache did not contain the previously added value
+ final char[] keyValue =
keystoreService.getCredentialForCluster(clusterName, credentialAlias);
+ assertNotNull(keyValue);
+ assertEquals(credentialValue, new String(keyValue));
+
+ verify(masterService);
+ }
+
+ @Test
public void testAddSelfSignedCertForGatewayLocalhost() throws Exception {
testAddSelfSignedCertForGateway(null);
}
diff --git a/pom.xml b/pom.xml
index 2187e7d..2c789c8 100644
--- a/pom.xml
+++ b/pom.xml
@@ -154,6 +154,7 @@
<asm.version>7.2</asm.version>
<aspectj.version>1.9.4</aspectj.version>
<bcprov-jdk15on.version>1.64</bcprov-jdk15on.version>
+ <ben-manes.caffeine.version>2.8.0</ben-manes.caffeine.version>
<buildnumber-maven-plugin.version>1.4</buildnumber-maven-plugin.version>
<cglib.version>3.3.0</cglib.version>
<checkstyle.version>8.26</checkstyle.version>
@@ -2203,6 +2204,21 @@
<artifactId>cloudera-manager-api-swagger</artifactId>
<version>${cloudera-manager-api-swagger.version}</version>
<!-- or CM version 6.0 and above -->
</dependency>
+ <dependency>
+ <groupId>com.github.ben-manes.caffeine</groupId>
+ <artifactId>caffeine</artifactId>
+ <version>${ben-manes.caffeine.version}</version>
+ <exclusions>
+ <exclusion>
+ <groupId>org.checkerframework</groupId>
+ <artifactId>checker-qual</artifactId>
+ </exclusion>
+ <exclusion>
+ <groupId>com.google.errorprone</groupId>
+ <artifactId>error_prone_annotations</artifactId>
+ </exclusion>
+ </exclusions>
+ </dependency>
<!-- ********** ********** ********** ********** **********
********** -->
<!-- ********** Test Dependencies
********** -->
