keith-turner closed pull request #345: ACCUMULO-4769 Sanity check for valid
CryptoModule and KeyEncryptionStrategy in config
URL: https://github.com/apache/accumulo/pull/345
This is a PR merged from a forked repository.
As GitHub hides the original diff on merge, it is displayed below for
the sake of provenance:
As this is a foreign pull request (from a fork), the diff is supplied
below (as it won't show otherwise due to GitHub magic):
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 33453be436..e6c1894939 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 @@ else if (!prop.getType().isValidFormat(value))
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 @@ private static void fatal(String msg) {
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 dd9cc4c7c4..f69e5d1588 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 static CryptoModule getCryptoModule(String
cryptoModuleClassname) {
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 @@ private static CryptoModule instantiateCryptoModule(String
cryptoModuleClassname
}
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 static SecretKeyEncryptionStrategy
getSecretKeyEncryptionStrategy(String
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 @@ private static SecretKeyEncryptionStrategy
instantiateSecreteKeyEncryptionStrate
}
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 f359b4ef74..df1e62a5a3 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 @@
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<>();
@@ -92,9 +96,58 @@ public void testFail_cipherSuiteNotSetKeyAlgorithmSet() {
ConfigSanityCheck.validate(m.entrySet());
}
+ @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 void
testFail_cryptoModuleSetSecretKeyEncryptionStrategyNotSet() {
@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 void
testPass_cryptoModuleAndSecretKeyEncryptionStrategyBothNull() {
@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());
}
}
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
[email protected]
With regards,
Apache Git Services
