You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@accumulo.apache.org by kt...@apache.org on 2018/03/08 19:22:49 UTC
[accumulo] 01/02: ACCUMULO-4769 Removed silent failure in the
CryptoModuleFactory (#345)
This is an automated email from the ASF dual-hosted git repository.
kturner pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/accumulo.git
commit 416b743a808a10b4eccd107d8919b3da85f556da
Author: Nick Felts <31...@users.noreply.github.com>
AuthorDate: Thu Mar 8 14:16:58 2018 -0500
ACCUMULO-4769 Removed silent failure in the CryptoModuleFactory (#345)
Added a check to the ConfigSanityCheck to validate the cryptomodule and the keyencryptionstrategy
Added unit tests to make sure the checks work
Removed silent failure in the CryptoModuleFactory since we have already checked for these failures when these values were set.
---
.../accumulo/core/conf/ConfigSanityCheck.java | 30 +++++++++++
.../core/security/crypto/CryptoModuleFactory.java | 49 ++++++-----------
.../accumulo/core/conf/ConfigSanityCheckTest.java | 61 ++++++++++++++++++++--
3 files changed, 102 insertions(+), 38 deletions(-)
diff --git a/core/src/main/java/org/apache/accumulo/core/conf/ConfigSanityCheck.java b/core/src/main/java/org/apache/accumulo/core/conf/ConfigSanityCheck.java
index 33453be..e6c1894 100644
--- a/core/src/main/java/org/apache/accumulo/core/conf/ConfigSanityCheck.java
+++ b/core/src/main/java/org/apache/accumulo/core/conf/ConfigSanityCheck.java
@@ -16,9 +16,12 @@
*/
package org.apache.accumulo.core.conf;
+import java.io.IOException;
import java.util.Map.Entry;
import java.util.Objects;
+import org.apache.accumulo.core.security.crypto.CryptoModule;
+import org.apache.accumulo.core.security.crypto.SecretKeyEncryptionStrategy;
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;
@@ -96,9 +99,16 @@ public class ConfigSanityCheck {
if (key.equals(Property.CRYPTO_MODULE_CLASS.getKey())) {
cryptoModule = Objects.requireNonNull(value);
+ if (!cryptoModule.equals(NULL_CRYPTO_MODULE)) {
+ verifyValidClassName(key, cryptoModule, CryptoModule.class);
+ }
+
}
if (key.equals(Property.CRYPTO_SECRET_KEY_ENCRYPTION_STRATEGY_CLASS.getKey())) {
secretKeyEncryptionStrategy = Objects.requireNonNull(value);
+ if (!secretKeyEncryptionStrategy.equals(NULL_SECRET_KEY_ENCRYPTION_STRATEGY)) {
+ verifyValidClassName(key, secretKeyEncryptionStrategy, SecretKeyEncryptionStrategy.class);
+ }
}
}
@@ -175,4 +185,24 @@ public class ConfigSanityCheck {
log.error("FATAL: {}", msg);
throw new SanityCheckException(msg);
}
+
+ /**
+ * Verifies a configured option is a legal class and has a required base class.
+ *
+ * @param confOption
+ * The Property key name
+ * @param className
+ * The Property value, the string representation of a class to be loaded
+ * @param requiredBaseClass
+ * The base class required for the className
+ */
+ private static void verifyValidClassName(String confOption, String className, Class<?> requiredBaseClass) {
+ try {
+ ConfigurationTypeHelper.getClassInstance(null, className, requiredBaseClass);
+ } catch (ClassNotFoundException | InstantiationException | IllegalAccessException | IOException e) {
+ fatal(confOption + " has an invalid class name: " + className);
+ } catch (ClassCastException e) {
+ fatal(confOption + " must implement " + requiredBaseClass + ", but the configured class does not: " + className);
+ }
+ }
}
diff --git a/core/src/main/java/org/apache/accumulo/core/security/crypto/CryptoModuleFactory.java b/core/src/main/java/org/apache/accumulo/core/security/crypto/CryptoModuleFactory.java
index dd9cc4c..f69e5d1 100644
--- a/core/src/main/java/org/apache/accumulo/core/security/crypto/CryptoModuleFactory.java
+++ b/core/src/main/java/org/apache/accumulo/core/security/crypto/CryptoModuleFactory.java
@@ -71,24 +71,22 @@ public class CryptoModuleFactory {
return cryptoModule;
}
- @SuppressWarnings({"rawtypes"})
private static CryptoModule instantiateCryptoModule(String cryptoModuleClassname) {
log.debug("About to instantiate crypto module {}", cryptoModuleClassname);
CryptoModule cryptoModule = null;
- Class cryptoModuleClazz = null;
+ Class<?> cryptoModuleClazz = null;
try {
cryptoModuleClazz = AccumuloVFSClassLoader.loadClass(cryptoModuleClassname);
} catch (ClassNotFoundException e1) {
- log.warn("Could not find configured crypto module \"{}\". No encryption will be used.", cryptoModuleClassname);
- return new NullCryptoModule();
+ throw new IllegalArgumentException("Could not find configured crypto module " + cryptoModuleClassname);
}
// Check if the given class implements the CryptoModule interface
- Class[] interfaces = cryptoModuleClazz.getInterfaces();
+ Class<?>[] interfaces = cryptoModuleClazz.getInterfaces();
boolean implementsCryptoModule = false;
- for (Class clazz : interfaces) {
+ for (Class<?> clazz : interfaces) {
if (clazz.equals(CryptoModule.class)) {
implementsCryptoModule = true;
break;
@@ -96,23 +94,15 @@ public class CryptoModuleFactory {
}
if (!implementsCryptoModule) {
- log.warn("Configured Accumulo crypto module \"{}\" does not implement the CryptoModule interface. No encryption will be used.", cryptoModuleClassname);
- return new NullCryptoModule();
+ throw new IllegalArgumentException("Configured Accumulo crypto module " + cryptoModuleClassname + " does not implement the CryptoModule interface.");
} else {
try {
cryptoModule = (CryptoModule) cryptoModuleClazz.newInstance();
log.debug("Successfully instantiated crypto module {}", cryptoModuleClassname);
- } catch (InstantiationException e) {
- log.warn("Got instantiation exception {} when instantiating crypto module \"{}\". No encryption will be used.", e.getCause().getClass().getName(),
- cryptoModuleClassname);
- log.warn("InstantiationException {}", e.getCause());
- return new NullCryptoModule();
- } catch (IllegalAccessException e) {
- log.warn("Got illegal access exception when trying to instantiate crypto module \"{}\". No encryption will be used.", cryptoModuleClassname);
- log.warn("IllegalAccessException", e);
- return new NullCryptoModule();
+ } catch (InstantiationException | IllegalAccessException e) {
+ throw new IllegalArgumentException("Unable to instantiate the crypto module: " + cryptoModuleClassname, e);
}
}
return cryptoModule;
@@ -146,25 +136,23 @@ public class CryptoModuleFactory {
return strategy;
}
- @SuppressWarnings("rawtypes")
private static SecretKeyEncryptionStrategy instantiateSecreteKeyEncryptionStrategy(String className) {
log.debug("About to instantiate secret key encryption strategy {}", className);
SecretKeyEncryptionStrategy strategy = null;
- Class keyEncryptionStrategyClazz = null;
+ Class<?> keyEncryptionStrategyClazz = null;
try {
keyEncryptionStrategyClazz = AccumuloVFSClassLoader.loadClass(className);
} catch (ClassNotFoundException e1) {
- log.warn("Could not find configured secret key encryption strategy \"{}\". No encryption will be used.", className);
- return new NullSecretKeyEncryptionStrategy();
+ throw new IllegalArgumentException("Could not find configured secret key encryption strategy: " + className);
}
// Check if the given class implements the CryptoModule interface
- Class[] interfaces = keyEncryptionStrategyClazz.getInterfaces();
+ Class<?>[] interfaces = keyEncryptionStrategyClazz.getInterfaces();
boolean implementsSecretKeyStrategy = false;
- for (Class clazz : interfaces) {
+ for (Class<?> clazz : interfaces) {
if (clazz.equals(SecretKeyEncryptionStrategy.class)) {
implementsSecretKeyStrategy = true;
break;
@@ -172,23 +160,16 @@ public class CryptoModuleFactory {
}
if (!implementsSecretKeyStrategy) {
- log.warn("Configured Accumulo secret key encryption strategy \"%s\" does not implement the SecretKeyEncryptionStrategy interface. No encryption will be used.");
- return new NullSecretKeyEncryptionStrategy();
+ throw new IllegalArgumentException(
+ "Configured Accumulo secret key encryption strategy \"%s\" does not implement the SecretKeyEncryptionStrategy interface.");
} else {
try {
strategy = (SecretKeyEncryptionStrategy) keyEncryptionStrategyClazz.newInstance();
log.debug("Successfully instantiated secret key encryption strategy {}", className);
- } catch (InstantiationException e) {
- log.warn("Got instantiation exception {} when instantiating secret key encryption strategy \"{}\". No encryption will be used.", e.getCause()
- .getClass().getName(), className);
- log.warn("InstantiationException {}", e.getCause());
- return new NullSecretKeyEncryptionStrategy();
- } catch (IllegalAccessException e) {
- log.warn("Got illegal access exception when trying to instantiate secret key encryption strategy \"{}\". No encryption will be used.", className);
- log.warn("IllegalAccessException", e);
- return new NullSecretKeyEncryptionStrategy();
+ } catch (InstantiationException | IllegalAccessException e) {
+ throw new IllegalArgumentException("Unable to instantiate the secret key encryption strategy: " + className, e);
}
}
return strategy;
diff --git a/core/src/test/java/org/apache/accumulo/core/conf/ConfigSanityCheckTest.java b/core/src/test/java/org/apache/accumulo/core/conf/ConfigSanityCheckTest.java
index f359b4e..df1e62a 100644
--- a/core/src/test/java/org/apache/accumulo/core/conf/ConfigSanityCheckTest.java
+++ b/core/src/test/java/org/apache/accumulo/core/conf/ConfigSanityCheckTest.java
@@ -25,6 +25,10 @@ import org.junit.Test;
public class ConfigSanityCheckTest {
private Map<String,String> m;
+ // These are used when a valid class is needed for testing
+ private static final String DEFAULT_CRYPTO_MODULE = "org.apache.accumulo.core.security.crypto.DefaultCryptoModule";
+ private static final String DEFAULT_SECRET_KEY_ENCRYPTION_STRATEGY = "org.apache.accumulo.core.security.crypto.NonCachingSecretKeyEncryptionStrategy";
+
@Before
public void setUp() {
m = new java.util.HashMap<>();
@@ -93,8 +97,57 @@ public class ConfigSanityCheckTest {
}
@Test(expected = SanityCheckException.class)
+ public void testFail_cryptoModuleInvalidClass() {
+ // a random hex dump is unlikely to be a real class name
+ m.put(Property.CRYPTO_MODULE_CLASS.getKey(), "e0218734bcd1e4d239203f970806786b");
+ m.put(Property.CRYPTO_SECRET_KEY_ENCRYPTION_STRATEGY_CLASS.getKey(), DEFAULT_SECRET_KEY_ENCRYPTION_STRATEGY);
+ ConfigSanityCheck.validate(m.entrySet());
+ }
+
+ @Test(expected = SanityCheckException.class)
+ public void testFail_cryptoModuleValidClassNotValidInterface() {
+ m.put(Property.CRYPTO_MODULE_CLASS.getKey(), "java.lang.String");
+ m.put(Property.CRYPTO_SECRET_KEY_ENCRYPTION_STRATEGY_CLASS.getKey(), DEFAULT_SECRET_KEY_ENCRYPTION_STRATEGY);
+ ConfigSanityCheck.validate(m.entrySet());
+ }
+
+ @Test
+ public void testPass_cryptoModuleAndSecretKeyEncryptionStrategyValidClasses() {
+ m.put(Property.CRYPTO_MODULE_CLASS.getKey(), DEFAULT_CRYPTO_MODULE);
+ m.put(Property.CRYPTO_SECRET_KEY_ENCRYPTION_STRATEGY_CLASS.getKey(), DEFAULT_SECRET_KEY_ENCRYPTION_STRATEGY);
+ ConfigSanityCheck.validate(m.entrySet());
+ }
+
+ @Test
+ public void testPass_cryptoModuleValidNullModule() {
+ m.put(Property.CRYPTO_MODULE_CLASS.getKey(), "NullCryptoModule");
+ ConfigSanityCheck.validate(m.entrySet());
+ }
+
+ @Test(expected = SanityCheckException.class)
+ public void testFail_secretKeyEncryptionStrategyInvalidClass() {
+ // a random hex dump is unlikely to be a real class name
+ m.put(Property.CRYPTO_SECRET_KEY_ENCRYPTION_STRATEGY_CLASS.getKey(), "e0218734bcd1e4d239203f970806786b");
+ m.put(Property.CRYPTO_MODULE_CLASS.getKey(), DEFAULT_CRYPTO_MODULE);
+ ConfigSanityCheck.validate(m.entrySet());
+ }
+
+ @Test(expected = SanityCheckException.class)
+ public void testFail_secretKeyEncryptionStrategyValidClassNotValidInterface() {
+ m.put(Property.CRYPTO_SECRET_KEY_ENCRYPTION_STRATEGY_CLASS.getKey(), "java.lang.String");
+ m.put(Property.CRYPTO_MODULE_CLASS.getKey(), DEFAULT_CRYPTO_MODULE);
+ ConfigSanityCheck.validate(m.entrySet());
+ }
+
+ @Test
+ public void testPass_secretKeyEncryptionStrategyValidNullStrategy() {
+ m.put(Property.CRYPTO_SECRET_KEY_ENCRYPTION_STRATEGY_CLASS.getKey(), "NullSecretKeyEncryptionStrategy");
+ ConfigSanityCheck.validate(m.entrySet());
+ }
+
+ @Test(expected = SanityCheckException.class)
public void testFail_cryptoModuleSetSecretKeyEncryptionStrategyNotSet() {
- m.put(Property.CRYPTO_MODULE_CLASS.getKey(), "DefaultCryptoModule");
+ m.put(Property.CRYPTO_MODULE_CLASS.getKey(), DEFAULT_CRYPTO_MODULE);
m.put(Property.CRYPTO_SECRET_KEY_ENCRYPTION_STRATEGY_CLASS.getKey(), "NullSecretKeyEncryptionStrategy");
ConfigSanityCheck.validate(m.entrySet());
}
@@ -102,7 +155,7 @@ public class ConfigSanityCheckTest {
@Test(expected = SanityCheckException.class)
public void testFail_cryptoModuleNotSetSecretKeyEncryptionStrategySet() {
m.put(Property.CRYPTO_MODULE_CLASS.getKey(), "NullCryptoModule");
- m.put(Property.CRYPTO_SECRET_KEY_ENCRYPTION_STRATEGY_CLASS.getKey(), "SecretKeyEncryptionStrategy");
+ m.put(Property.CRYPTO_SECRET_KEY_ENCRYPTION_STRATEGY_CLASS.getKey(), DEFAULT_SECRET_KEY_ENCRYPTION_STRATEGY);
ConfigSanityCheck.validate(m.entrySet());
}
@@ -115,8 +168,8 @@ public class ConfigSanityCheckTest {
@Test
public void testPass_cryptoModuleAndSecretKeyEncryptionStrategyBothSet() {
- m.put(Property.CRYPTO_MODULE_CLASS.getKey(), "DefaultCryptoModule");
- m.put(Property.CRYPTO_SECRET_KEY_ENCRYPTION_STRATEGY_CLASS.getKey(), "SecretKeyEncryptionStrategy");
+ m.put(Property.CRYPTO_MODULE_CLASS.getKey(), DEFAULT_CRYPTO_MODULE);
+ m.put(Property.CRYPTO_SECRET_KEY_ENCRYPTION_STRATEGY_CLASS.getKey(), DEFAULT_SECRET_KEY_ENCRYPTION_STRATEGY);
ConfigSanityCheck.validate(m.entrySet());
}
}
--
To stop receiving notification emails like this one, please contact
kturner@apache.org.