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:48 UTC
[accumulo] branch master updated (cdd8c9e -> dce8c35)
This is an automated email from the ASF dual-hosted git repository.
kturner pushed a change to branch master
in repository https://gitbox.apache.org/repos/asf/accumulo.git.
from cdd8c9e Merge branch '1.8'
new 416b743 ACCUMULO-4769 Removed silent failure in the CryptoModuleFactory (#345)
new dce8c35 ACCUMULO-4769 simplify crypto module class instantiation
The 2 revisions listed above as "new" are entirely new to this
repository and will be described in separate emails. The revisions
listed as "add" were already present in the repository and have only
been added to this reference.
Summary of changes:
.../accumulo/core/conf/ConfigSanityCheck.java | 30 +++++++
.../core/security/crypto/CryptoModuleFactory.java | 95 +++++-----------------
.../accumulo/core/conf/ConfigSanityCheckTest.java | 61 +++++++++++++-
3 files changed, 106 insertions(+), 80 deletions(-)
--
To stop receiving notification emails like this one, please contact
kturner@apache.org.
[accumulo] 02/02: ACCUMULO-4769 simplify crypto module class
instantiation
Posted by kt...@apache.org.
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 dce8c359056905405abaea9b99efc21dadbc5920
Author: Keith Turner <kt...@apache.org>
AuthorDate: Thu Mar 8 13:47:08 2018 -0500
ACCUMULO-4769 simplify crypto module class instantiation
---
.../core/security/crypto/CryptoModuleFactory.java | 74 ++++++----------------
1 file changed, 18 insertions(+), 56 deletions(-)
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 f69e5d1..556c621 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
@@ -74,38 +74,19 @@ public class CryptoModuleFactory {
private static CryptoModule instantiateCryptoModule(String cryptoModuleClassname) {
log.debug("About to instantiate crypto module {}", cryptoModuleClassname);
- CryptoModule cryptoModule = null;
- Class<?> cryptoModuleClazz = null;
try {
- cryptoModuleClazz = AccumuloVFSClassLoader.loadClass(cryptoModuleClassname);
- } catch (ClassNotFoundException e1) {
- throw new IllegalArgumentException("Could not find configured crypto module " + cryptoModuleClassname);
- }
+ CryptoModule cryptoModule = AccumuloVFSClassLoader.loadClass(cryptoModuleClassname).asSubclass(CryptoModule.class).newInstance();
- // Check if the given class implements the CryptoModule interface
- Class<?>[] interfaces = cryptoModuleClazz.getInterfaces();
- boolean implementsCryptoModule = false;
-
- for (Class<?> clazz : interfaces) {
- if (clazz.equals(CryptoModule.class)) {
- implementsCryptoModule = true;
- break;
- }
- }
+ log.debug("Successfully instantiated crypto module {}", cryptoModuleClassname);
- if (!implementsCryptoModule) {
+ return cryptoModule;
+ } catch (ClassNotFoundException e1) {
+ throw new IllegalArgumentException("Could not find configured crypto module " + cryptoModuleClassname);
+ } catch (ClassCastException cce) {
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 | IllegalAccessException e) {
- throw new IllegalArgumentException("Unable to instantiate the crypto module: " + cryptoModuleClassname, e);
- }
+ } catch (InstantiationException | IllegalAccessException e) {
+ throw new IllegalArgumentException("Unable to instantiate the crypto module: " + cryptoModuleClassname, e);
}
- return cryptoModule;
}
public static SecretKeyEncryptionStrategy getSecretKeyEncryptionStrategy(AccumuloConfiguration conf) {
@@ -140,39 +121,20 @@ public class CryptoModuleFactory {
log.debug("About to instantiate secret key encryption strategy {}", className);
- SecretKeyEncryptionStrategy strategy = null;
- Class<?> keyEncryptionStrategyClazz = null;
try {
- keyEncryptionStrategyClazz = AccumuloVFSClassLoader.loadClass(className);
- } catch (ClassNotFoundException e1) {
- throw new IllegalArgumentException("Could not find configured secret key encryption strategy: " + className);
- }
+ SecretKeyEncryptionStrategy strategy = AccumuloVFSClassLoader.loadClass(className).asSubclass(SecretKeyEncryptionStrategy.class).newInstance();
- // Check if the given class implements the CryptoModule interface
- Class<?>[] interfaces = keyEncryptionStrategyClazz.getInterfaces();
- boolean implementsSecretKeyStrategy = false;
-
- for (Class<?> clazz : interfaces) {
- if (clazz.equals(SecretKeyEncryptionStrategy.class)) {
- implementsSecretKeyStrategy = true;
- break;
- }
- }
+ log.debug("Successfully instantiated secret key encryption strategy {}", className);
- if (!implementsSecretKeyStrategy) {
- 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 | IllegalAccessException e) {
- throw new IllegalArgumentException("Unable to instantiate the secret key encryption strategy: " + className, e);
- }
+ return strategy;
+ } catch (ClassNotFoundException e1) {
+ throw new IllegalArgumentException("Could not find configured secret key encryption strategy: " + className);
+ } catch (ClassCastException e) {
+ throw new IllegalArgumentException("Configured Accumulo secret key encryption strategy \"" + className
+ + "\" does not implement the SecretKeyEncryptionStrategy interface.");
+ } catch (InstantiationException | IllegalAccessException e) {
+ throw new IllegalArgumentException("Unable to instantiate the secret key encryption strategy: " + className, e);
}
- return strategy;
}
static class NullSecretKeyEncryptionStrategy implements SecretKeyEncryptionStrategy {
--
To stop receiving notification emails like this one, please contact
kturner@apache.org.
[accumulo] 01/02: ACCUMULO-4769 Removed silent failure in the
CryptoModuleFactory (#345)
Posted by kt...@apache.org.
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.