[GitHub] ctubbsii commented on a change in pull request #345: ACCUMULO-4769 Sanity check for valid CryptoModule and KeyEncryptionStrategy in config
ctubbsii commented on a change in pull request #345: ACCUMULO-4769 Sanity check
for valid CryptoModule and KeyEncryptionStrategy in config
URL: https://github.com/apache/accumulo/pull/345#discussion_r162407837
##
File path:
core/src/main/java/org/apache/accumulo/core/security/crypto/CryptoModuleFactory.java
##
@@ -96,23 +95,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.");
Review comment:
Agreed. This can be refactored/deferred to later. It's safer to leave it as
is for now.
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:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] ctubbsii commented on a change in pull request #345: ACCUMULO-4769 Sanity check for valid CryptoModule and KeyEncryptionStrategy in config
ctubbsii commented on a change in pull request #345: ACCUMULO-4769 Sanity check
for valid CryptoModule and KeyEncryptionStrategy in config
URL: https://github.com/apache/accumulo/pull/345#discussion_r162407579
##
File path:
core/src/main/java/org/apache/accumulo/core/conf/ConfigSanityCheck.java
##
@@ -88,6 +91,12 @@ else if (!prop.getType().isValidFormat(value))
if (key.equals(Property.CRYPTO_CIPHER_KEY_ALGORITHM_NAME.getKey())) {
keyAlgorithm = Objects.requireNonNull(value);
}
+ if
(key.equals(Property.CRYPTO_SECRET_KEY_ENCRYPTION_STRATEGY_CLASS.getKey()) &&
!(value == null || value.equals("NullSecretKeyEncryptionStrategy"))) {
Review comment:
Ah, I see. It probably doesn't matter much anyway.
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:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] ctubbsii commented on a change in pull request #345: ACCUMULO-4769 Sanity check for valid CryptoModule and KeyEncryptionStrategy in config
ctubbsii commented on a change in pull request #345: ACCUMULO-4769 Sanity check
for valid CryptoModule and KeyEncryptionStrategy in config
URL: https://github.com/apache/accumulo/pull/345#discussion_r162384070
##
File path:
core/src/main/java/org/apache/accumulo/core/security/crypto/CryptoModuleFactory.java
##
@@ -172,23 +162,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(
Review comment:
Again, I'm not sure any of this is necessary if the class is loaded with the
ConfigurationTypeHelper from the config. It may be redundant. I'm not 100% sure
on this, though.
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:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] ctubbsii commented on a change in pull request #345: ACCUMULO-4769 Sanity check for valid CryptoModule and KeyEncryptionStrategy in config
ctubbsii commented on a change in pull request #345: ACCUMULO-4769 Sanity check
for valid CryptoModule and KeyEncryptionStrategy in config
URL: https://github.com/apache/accumulo/pull/345#discussion_r162380001
##
File path:
core/src/main/java/org/apache/accumulo/core/conf/ConfigSanityCheck.java
##
@@ -88,6 +91,12 @@ else if (!prop.getType().isValidFormat(value))
if (key.equals(Property.CRYPTO_CIPHER_KEY_ALGORITHM_NAME.getKey())) {
keyAlgorithm = Objects.requireNonNull(value);
}
+ if
(key.equals(Property.CRYPTO_SECRET_KEY_ENCRYPTION_STRATEGY_CLASS.getKey()) &&
!(value == null || value.equals("NullSecretKeyEncryptionStrategy"))) {
Review comment:
Should this be
`value.equals(NullSecretKeyEncryptionStrategy.class.getName()) ||
value.equals(NullSecretKeyEncryptionStrategy.class.getSimpleName())` instead of
checking value by String literal?
(Similar comment below on String literal comparison for "NullCryptoModule")
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:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] ctubbsii commented on a change in pull request #345: ACCUMULO-4769 Sanity check for valid CryptoModule and KeyEncryptionStrategy in config
ctubbsii commented on a change in pull request #345: ACCUMULO-4769 Sanity check
for valid CryptoModule and KeyEncryptionStrategy in config
URL: https://github.com/apache/accumulo/pull/345#discussion_r162383253
##
File path:
core/src/main/java/org/apache/accumulo/core/security/crypto/CryptoModuleFactory.java
##
@@ -156,8 +147,7 @@ private static SecretKeyEncryptionStrategy
instantiateSecreteKeyEncryptionStrate
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 RuntimeException("Could not find configured secret key
encryption strategy: " + className);
Review comment:
I think this could be a more specific RuntimeException also.
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:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] ctubbsii commented on a change in pull request #345: ACCUMULO-4769 Sanity check for valid CryptoModule and KeyEncryptionStrategy in config
ctubbsii commented on a change in pull request #345: ACCUMULO-4769 Sanity check
for valid CryptoModule and KeyEncryptionStrategy in config
URL: https://github.com/apache/accumulo/pull/345#discussion_r162380160
##
File path:
core/src/main/java/org/apache/accumulo/core/security/crypto/CryptoModuleFactory.java
##
@@ -80,8 +80,7 @@ private static CryptoModule instantiateCryptoModule(String
cryptoModuleClassname
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 RuntimeException("Could not find configured crypto module " +
cryptoModuleClassname);
Review comment:
I still think this one could be a more specific RuntimeException
(IllegalArgumentException?)
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:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] ctubbsii commented on a change in pull request #345: ACCUMULO-4769 Sanity check for valid CryptoModule and KeyEncryptionStrategy in config
ctubbsii commented on a change in pull request #345: ACCUMULO-4769 Sanity check
for valid CryptoModule and KeyEncryptionStrategy in config
URL: https://github.com/apache/accumulo/pull/345#discussion_r162380728
##
File path:
core/src/main/java/org/apache/accumulo/core/security/crypto/CryptoModuleFactory.java
##
@@ -80,8 +80,7 @@ private static CryptoModule instantiateCryptoModule(String
cryptoModuleClassname
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 RuntimeException("Could not find configured crypto module " +
cryptoModuleClassname);
Review comment:
This was preexisting before this change, but I also noticed that the
rawtypes should not be suppressed. Generic `` wildcard params should be used
on `Class` instead.
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:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] ctubbsii commented on a change in pull request #345: ACCUMULO-4769 Sanity check for valid CryptoModule and KeyEncryptionStrategy in config
ctubbsii commented on a change in pull request #345: ACCUMULO-4769 Sanity check
for valid CryptoModule and KeyEncryptionStrategy in config
URL: https://github.com/apache/accumulo/pull/345#discussion_r162120342
##
File path:
core/src/main/java/org/apache/accumulo/core/conf/ConfigSanityCheck.java
##
@@ -88,6 +90,31 @@ else if (!prop.getType().isValidFormat(value))
if (key.equals(Property.CRYPTO_CIPHER_KEY_ALGORITHM_NAME.getKey())) {
keyAlgorithm = Objects.requireNonNull(value);
}
+ if
(key.equals(Property.CRYPTO_SECRET_KEY_ENCRYPTION_STRATEGY_CLASS.getKey()) &&
!(value == null || value.equals("NullSecretKeyEncryptionStrategy"))) {
+Class strat = null;
+try {
+ strat = Class.forName(value);
Review comment:
We have a "ConfigurationTypeHelper" class with utility methods for loading
classes from config properties. Should use that, if possible.
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:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] ctubbsii commented on a change in pull request #345: ACCUMULO-4769 Sanity check for valid CryptoModule and KeyEncryptionStrategy in config
ctubbsii commented on a change in pull request #345: ACCUMULO-4769 Sanity check
for valid CryptoModule and KeyEncryptionStrategy in config
URL: https://github.com/apache/accumulo/pull/345#discussion_r158139376
##
File path:
core/src/main/java/org/apache/accumulo/core/conf/ConfigSanityCheck.java
##
@@ -88,6 +90,33 @@ else if (!prop.getType().isValidFormat(value))
if (key.equals(Property.CRYPTO_CIPHER_KEY_ALGORITHM_NAME.getKey())) {
keyAlgorithm = Objects.requireNonNull(value);
}
+ if
(key.equals(Property.CRYPTO_SECRET_KEY_ENCRYPTION_STRATEGY_CLASS.getKey()) &&
!(value == null || value.equals("NullSecretKeyEncryptionStrategy"))) {
+@SuppressWarnings("rawtypes")
Review comment:
Should not suppress warnings which can be addressed with proper generics
use. Can use `` here.
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:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] ctubbsii commented on a change in pull request #345: ACCUMULO-4769 Sanity check for valid CryptoModule and KeyEncryptionStrategy in config
ctubbsii commented on a change in pull request #345: ACCUMULO-4769 Sanity check
for valid CryptoModule and KeyEncryptionStrategy in config
URL: https://github.com/apache/accumulo/pull/345#discussion_r158141223
##
File path:
core/src/main/java/org/apache/accumulo/core/security/crypto/CryptoModuleFactory.java
##
@@ -172,23 +164,17 @@ 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 RuntimeException("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();
+throw new RuntimeException("Got instantiation exception {} when
instantiating secret key encryption strategy " + className);
Review comment:
Since a lot of these catch blocks have essentially the same error, it'd
probably be better to use the multi-catch syntax, just to clean things up a
bit. The original exception should be passed as a parameter, so we don't lose
portions of the stack trace.
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:
us...@infra.apache.org
With regards,
Apache Git Services
