From 31ddff58cfbdd77293f130f8c3ab47b1f03996f1 Mon Sep 17 00:00:00 2001 From: haydenmarchant Date: Wed, 9 Jul 2014 12:10:49 -0700 Subject: [PATCH] ACCUMULO-2943 Fixing failures where no RNG "SUN" provider Both org.apache.accumulo.core.security.crypto.CrypoTest & org.apache.accumulo.core.file.rfile.RFileTest have lots of failures due to calls to SecureRandom with Random Number Generator Provider hard-coded as Sun. The IBM JVM has it's own built in RNG Provider called IBMJCE. 2 issues - hard-coded calls to SecureRandom.getInstance(,"SUN") and also default value in Property class is "SUN". Most failures are due to the CryptoModuleParameters instance being populated with default value of Crypto Secure RNG Provider, in particular, the following line from CryptoModelFactory.fillParamsObjectFromStringMap(): params.setRandomNumberGeneratorProvider(cryptoOpts.get(Property.CRYPTO_SECURE_RNG_PROVIDER.getKey())); Since the default as described in Property class for RNG provider is "SUN", I have made an override mechanism in which a default property can be overidden by passing System property of same name. Any property with annotation @SystemOverride has this functionality enabled. So, when using a JVM which does not have the "SUN" RNG Provider, a system property (-Dcrypto.secure.rng.provider={provname}) can be added to the parent pom.xml in the surefire plugin definition (same location as the max memory for tests). In addition, CryptoTest.testCryptoModuleParamsParsing() has been changed to read from a separate config file so the on/off variants, since it just focuses on parsing of params. --- .../apache/accumulo/core/conf/Property.java | 8 + .../accumulo/core/conf/SystemOverride.java | 33 ++++ .../accumulo/core/conf/PropertyTest.java | 46 +++++ .../core/security/crypto/CryptoTest.java | 8 +- .../resources/crypto-on-accumulo-site.xml | 4 - ...pto-on-no-key-encryption-accumulo-site.xml | 4 - .../crypto-on-parse-test-accumulo-site.xml | 164 ++++++++++++++++++ 7 files changed, 257 insertions(+), 10 deletions(-) create mode 100644 core/src/main/java/org/apache/accumulo/core/conf/SystemOverride.java create mode 100644 core/src/test/resources/crypto-on-parse-test-accumulo-site.xml diff --git a/core/src/main/java/org/apache/accumulo/core/conf/Property.java b/core/src/main/java/org/apache/accumulo/core/conf/Property.java index 87a29fb15b9..3060fa39cc1 100644 --- a/core/src/main/java/org/apache/accumulo/core/conf/Property.java +++ b/core/src/main/java/org/apache/accumulo/core/conf/Property.java @@ -60,6 +60,7 @@ public enum Property { CRYPTO_SECURE_RNG("crypto.secure.rng", "SHA1PRNG", PropertyType.STRING, "States the secure random number generator to use, and defaults to the built-in Sun SHA1PRNG"), @Experimental + @SystemOverride CRYPTO_SECURE_RNG_PROVIDER("crypto.secure.rng.provider", "SUN", PropertyType.STRING, "States the secure random number generator provider to use, and defaults to the built-in SUN provider"), @Experimental @@ -475,6 +476,9 @@ public String getDefaultValue() { } pconf.addProperty("hack_default_value", this.defaultValue); v = pconf.getString("hack_default_value"); + } else if (isSystemOverride()) { + String systemOverrideVal = System.getProperty(getKey()); + v = systemOverrideVal != null ? systemOverrideVal : getRawDefaultValue(); } else { v = getRawDefaultValue(); } @@ -507,6 +511,10 @@ public boolean isSensitive() { return hasAnnotation(Sensitive.class) || hasPrefixWithAnnotation(getKey(), Sensitive.class); } + public boolean isSystemOverride() { + return hasAnnotation(SystemOverride.class) || hasPrefixWithAnnotation(getKey(), SystemOverride.class); + } + public static boolean isSensitive(String key) { return hasPrefixWithAnnotation(key, Sensitive.class); } diff --git a/core/src/main/java/org/apache/accumulo/core/conf/SystemOverride.java b/core/src/main/java/org/apache/accumulo/core/conf/SystemOverride.java new file mode 100644 index 00000000000..ffb0b2013d2 --- /dev/null +++ b/core/src/main/java/org/apache/accumulo/core/conf/SystemOverride.java @@ -0,0 +1,33 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to You under the Apache License, Version 2.0 + * (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package org.apache.accumulo.core.conf; + +import java.lang.annotation.Inherited; +import java.lang.annotation.Retention; +import java.lang.annotation.RetentionPolicy; + +/** + * An annotation to denote {@link AccumuloConfiguration} {@link Property} keys, whose values should be interpolated with system properties. + * + * Interpolated items need to be careful, as JVM properties could be updates and we may want that propogated when those changes occur. Currently only + * VFS_CLASSLOADER_CACHE_DIR, which isn't ZK mutable, is interpolated, so this shouldn't be an issue as java.io.tmpdir also shouldn't be changing. + */ +@Inherited +@Retention(RetentionPolicy.RUNTIME) +@interface SystemOverride { + +} diff --git a/core/src/test/java/org/apache/accumulo/core/conf/PropertyTest.java b/core/src/test/java/org/apache/accumulo/core/conf/PropertyTest.java index bca2e2211db..64d65f28687 100644 --- a/core/src/test/java/org/apache/accumulo/core/conf/PropertyTest.java +++ b/core/src/test/java/org/apache/accumulo/core/conf/PropertyTest.java @@ -147,4 +147,50 @@ public void validatePropertyKeys() { } } } + + @Test + public void validateDefaultValWithSytemOverride() { + String key = Property.CRYPTO_SECURE_RNG_PROVIDER.getKey(); + String prevVal = System.getProperty(key); + System.clearProperty(key); + try { + assertEquals("SUN",Property.CRYPTO_SECURE_RNG_PROVIDER.getDefaultValue()); + + System.setProperty(key, "OTHER_PROV"); + assertEquals("OTHER_PROV",Property.CRYPTO_SECURE_RNG_PROVIDER.getDefaultValue()); + } finally { + if (prevVal != null) { + System.setProperty(key, prevVal); + } else { + System.clearProperty(key); + } + } + } + + @Test + public void validateDefaultValWithNonSytemOverride() { + String key = Property.MASTER_CLIENTPORT.getKey(); + String prevVal = System.getProperty(key); + System.clearProperty(key); + try { + assertEquals("9999",Property.MASTER_CLIENTPORT.getDefaultValue()); + + System.setProperty(key, "8888"); + //ensure that System property is ignored in this case + assertEquals("9999",Property.MASTER_CLIENTPORT.getDefaultValue()); + } finally { + if (prevVal != null) { + System.setProperty(key, prevVal); + } else { + System.clearProperty(key); + } + } + } + + @Test + public void validateSystemOverrideAnnotations() { + assertTrue(Property.CRYPTO_SECURE_RNG_PROVIDER.isSystemOverride()); + assertFalse(Property.MASTER_CLIENTPORT.isSystemOverride()); + } + } diff --git a/core/src/test/java/org/apache/accumulo/core/security/crypto/CryptoTest.java b/core/src/test/java/org/apache/accumulo/core/security/crypto/CryptoTest.java index fe16c0e068b..07d4b05e554 100644 --- a/core/src/test/java/org/apache/accumulo/core/security/crypto/CryptoTest.java +++ b/core/src/test/java/org/apache/accumulo/core/security/crypto/CryptoTest.java @@ -57,6 +57,7 @@ public class CryptoTest { private static final int MARKER_INT = 0xCADEFEDD; private static final String MARKER_STRING = "1 2 3 a b c"; public static final String CONFIG_FILE_SYSTEM_PROP = "org.apache.accumulo.config.file"; + public static final String CRYPTO_ON_PARSE_TEST_CONF = "crypto-on-parse-test-accumulo-site.xml"; public static final String CRYPTO_ON_CONF = "crypto-on-accumulo-site.xml"; public static final String CRYPTO_OFF_CONF = "crypto-off-accumulo-site.xml"; public static final String CRYPTO_ON_KEK_OFF_CONF = "crypto-on-no-key-encryption-accumulo-site.xml"; @@ -90,7 +91,7 @@ public void testNoCryptoStream() throws IOException { @Test public void testCryptoModuleParamsParsing() { - AccumuloConfiguration conf = setAndGetAccumuloConfig(CRYPTO_ON_CONF); + AccumuloConfiguration conf = setAndGetAccumuloConfig(CRYPTO_ON_PARSE_TEST_CONF); CryptoModuleParameters params = CryptoModuleFactory.createParamsObjectFromAccumuloConfiguration(conf); @@ -335,6 +336,9 @@ private AccumuloConfiguration setAndGetAccumuloConfig(String cryptoConfSetting) ConfigurationCopy result = new ConfigurationCopy(AccumuloConfiguration.getDefaultConfiguration()); Configuration conf = new Configuration(false); conf.addResource(cryptoConfSetting); + String oveerride = System.getProperty("crypto.secure.rng.provider.override"); + String prov = conf.get("crypto.secure.rng.provider"); + for (Entry e : conf) { result.set(e.getKey(), e.getValue()); } @@ -344,7 +348,7 @@ private AccumuloConfiguration setAndGetAccumuloConfig(String cryptoConfSetting) @Test public void testKeyWrapAndUnwrap() throws NoSuchAlgorithmException, NoSuchPaddingException, NoSuchProviderException, InvalidKeyException, IllegalBlockSizeException, BadPaddingException { Cipher keyWrapCipher = Cipher.getInstance("AES/ECB/NoPadding"); - SecureRandom random = SecureRandom.getInstance("SHA1PRNG", "SUN"); + SecureRandom random = SecureRandom.getInstance("SHA1PRNG"); byte[] kek = new byte[16]; random.nextBytes(kek); diff --git a/core/src/test/resources/crypto-on-accumulo-site.xml b/core/src/test/resources/crypto-on-accumulo-site.xml index 3da743790b7..29c6a2ff848 100644 --- a/core/src/test/resources/crypto-on-accumulo-site.xml +++ b/core/src/test/resources/crypto-on-accumulo-site.xml @@ -128,10 +128,6 @@ crypto.secure.rng SHA1PRNG - - crypto.secure.rng.provider - SUN - crypto.secret.key.encryption.strategy.class org.apache.accumulo.core.security.crypto.CachingHDFSSecretKeyEncryptionStrategy diff --git a/core/src/test/resources/crypto-on-no-key-encryption-accumulo-site.xml b/core/src/test/resources/crypto-on-no-key-encryption-accumulo-site.xml index edbcfebe495..cba896ee9b5 100644 --- a/core/src/test/resources/crypto-on-no-key-encryption-accumulo-site.xml +++ b/core/src/test/resources/crypto-on-no-key-encryption-accumulo-site.xml @@ -128,10 +128,6 @@ crypto.secure.rng SHA1PRNG - - crypto.secure.rng.provider - SUN - instance.dfs.dir /tmp diff --git a/core/src/test/resources/crypto-on-parse-test-accumulo-site.xml b/core/src/test/resources/crypto-on-parse-test-accumulo-site.xml new file mode 100644 index 00000000000..3da743790b7 --- /dev/null +++ b/core/src/test/resources/crypto-on-parse-test-accumulo-site.xml @@ -0,0 +1,164 @@ + + + + + + + + + instance.zookeeper.host + localhost:2181 + comma separated list of zookeeper servers + + + + logger.dir.walog + walogs + The directory used to store write-ahead logs on the local filesystem. It is possible to specify a comma-separated list of directories. + + + + instance.secret + DEFAULT + A secret unique to a given instance that all servers must know in order to communicate with one another. + Change it before initialization. To change it later use ./bin/accumulo org.apache.accumulo.server.util.ChangeSecret [oldpasswd] [newpasswd], + and then update this file. + + + + + tserver.memory.maps.max + 80M + + + + tserver.cache.data.size + 7M + + + + tserver.cache.index.size + 20M + + + + trace.password + + password + + + + trace.user + root + + + + tserver.sort.buffer.size + 50M + + + + tserver.walog.max.size + 100M + + + + general.classpaths + + $ACCUMULO_HOME/server/target/classes/, + $ACCUMULO_HOME/core/target/classes/, + $ACCUMULO_HOME/start/target/classes/, + $ACCUMULO_HOME/fate/target/classes/, + $ACCUMULO_HOME/proxy/target/classes/, + $ACCUMULO_HOME/examples/target/classes/, + $ACCUMULO_HOME/lib/[^.].$ACCUMULO_VERSION.jar, + $ACCUMULO_HOME/lib/[^.].*.jar, + $ZOOKEEPER_HOME/zookeeper[^.].*.jar, + $HADOOP_CONF_DIR, + $HADOOP_PREFIX/[^.].*.jar, + $HADOOP_PREFIX/lib/[^.].*.jar, + + Classpaths that accumulo checks for updates and class files. + When using the Security Manager, please remove the ".../target/classes/" values. + + + + + crypto.module.class + org.apache.accumulo.core.security.crypto.DefaultCryptoModule + + + crypto.cipher.suite + AES/CFB/NoPadding + + + crypto.cipher.algorithm.name + AES + + + crypto.cipher.key.length + 128 + + + crypto.secure.rng + SHA1PRNG + + + crypto.secure.rng.provider + SUN + + + crypto.secret.key.encryption.strategy.class + org.apache.accumulo.core.security.crypto.CachingHDFSSecretKeyEncryptionStrategy + + + instance.dfs.dir + /tmp + + + instance.dfs.uri + file:/// + + + + crypto.default.key.strategy.hdfs.uri + file:/// + + + crypto.default.key.strategy.key.location + /tmp/test.secret.key + + + + crypto.default.key.strategy.cipher.suite + AES/ECB/NoPadding + + + + +