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 daeffd8d9cf980814fb7d131a0d89cbdfd856298 Author: Nick <31989480+pirc...@users.noreply.github.com> AuthorDate: Mon Nov 6 11:56:59 2017 -0500 ACCUMULO-4737 Clean up cipher algorithm configuration Renamed crypto.cipher.algorithm.name to crypto.cipher.key.algorithm.name Removed the unused mode/padding code in favor of passing around the crypto suite (instead of splitting it up and rebuilding it) Accumulo will now use crypto.cipher.key.algorithm.name to build the encryption key Accumulo will now use crypto.cipher.suite to build the Java Cipher object Unit test was updated to reflect the change Additionally, this was stumbled upon when implementing a separate cipher algorithm option for WAL files, so that change has been included. Added sanity check for the config file along with tests --- .../accumulo/core/conf/ConfigSanityCheck.java | 19 ++++++ .../org/apache/accumulo/core/conf/Property.java | 14 ++++- .../accumulo/core/file/rfile/bcfile/BCFile.java | 8 +-- .../CachingHDFSSecretKeyEncryptionStrategy.java | 6 +- .../core/security/crypto/CryptoModuleFactory.java | 21 ++----- .../security/crypto/CryptoModuleParameters.java | 73 +++++++--------------- .../core/security/crypto/DefaultCryptoModule.java | 46 +++++--------- .../NonCachingSecretKeyEncryptionStrategy.java | 6 +- .../security/crypto/RFileCipherOutputStream.java | 15 +++-- .../accumulo/core/conf/ConfigSanityCheckTest.java | 16 +++++ .../accumulo/core/security/crypto/CryptoTest.java | 13 ++-- .../src/test/resources/crypto-on-accumulo-site.xml | 6 +- .../crypto-on-no-key-encryption-accumulo-site.xml | 2 +- .../org/apache/accumulo/tserver/log/DfsLogger.java | 4 ++ .../org/apache/accumulo/test/ShellConfigIT.java | 4 +- 15 files changed, 121 insertions(+), 132 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 9c2ed6c..baf1818 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 @@ -17,6 +17,7 @@ package org.apache.accumulo.core.conf; import java.util.Map.Entry; +import java.util.Objects; import org.slf4j.Logger; import org.slf4j.LoggerFactory; @@ -48,6 +49,8 @@ public class ConfigSanityCheck { public static void validate(Iterable<Entry<String,String>> entries) { String instanceZkTimeoutValue = null; boolean usingVolumes = false; + String cipherSuite = null; + String keyAlgorithm = null; for (Entry<String,String> entry : entries) { String key = entry.getKey(); String value = entry.getValue(); @@ -75,6 +78,14 @@ public class ConfigSanityCheck { Preconditions.checkArgument(bsize > 0 && bsize < Integer.MAX_VALUE, key + " must be greater than 0 and less than " + Integer.MAX_VALUE + " but was: " + bsize); } + + if (key.equals(Property.CRYPTO_CIPHER_SUITE.getKey())) { + cipherSuite = Objects.requireNonNull(value); + } + + if (key.equals(Property.CRYPTO_CIPHER_KEY_ALGORITHM_NAME.getKey())) { + keyAlgorithm = Objects.requireNonNull(value); + } } if (instanceZkTimeoutValue != null) { @@ -84,6 +95,14 @@ public class ConfigSanityCheck { if (!usingVolumes) { log.warn("Use of {} and {} are deprecated. Consider using {} instead.", INSTANCE_DFS_URI, INSTANCE_DFS_DIR, Property.INSTANCE_VOLUMES); } + + if (cipherSuite.equals("NullCipher") && !keyAlgorithm.equals("NullCipher")) { + fatal(Property.CRYPTO_CIPHER_SUITE.getKey() + " should be configured when " + Property.CRYPTO_CIPHER_KEY_ALGORITHM_NAME.getKey() + " is set."); + } + + if (!cipherSuite.equals("NullCipher") && keyAlgorithm.equals("NullCipher")) { + fatal(Property.CRYPTO_CIPHER_KEY_ALGORITHM_NAME.getKey() + " should be configured when " + Property.CRYPTO_CIPHER_SUITE.getKey() + " is set."); + } } private interface CheckTimeDuration { 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 e08df9e..f0e2338 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 @@ -48,10 +48,18 @@ public enum Property { "Fully qualified class name of the class that implements the CryptoModule interface, to be used in setting up encryption at rest for the WAL and " + "(future) other parts of the code."), @Experimental - CRYPTO_CIPHER_SUITE("crypto.cipher.suite", "NullCipher", PropertyType.STRING, "Describes the cipher suite to use for the write-ahead log"), + CRYPTO_CIPHER_SUITE("crypto.cipher.suite", "NullCipher", PropertyType.STRING, + "Describes the cipher suite to use for rfile encryption. If a WAL cipher suite is not set, it will default to this value. The suite should be in the " + + "form of algorithm/mode/padding, e.g. AES/CBC/NoPadding"), @Experimental - CRYPTO_CIPHER_ALGORITHM_NAME("crypto.cipher.algorithm.name", "NullCipher", PropertyType.STRING, - "States the name of the algorithm used in the corresponding cipher suite. Do not make these different, unless you enjoy mysterious exceptions and bugs."), + CRYPTO_WAL_CIPHER_SUITE( + "crypto.wal.cipher.suite", + "NullCipher", + PropertyType.STRING, + "Describes the cipher suite to use for the write-ahead log. Defaults to 'cyrpto.cipher.suite' and will use that value for WAL encryption unless otherwise specified."), + @Experimental + CRYPTO_CIPHER_KEY_ALGORITHM_NAME("crypto.cipher.key.algorithm.name", "NullCipher", PropertyType.STRING, + "States the name of the algorithm used for the key for the corresponding cipher suite. The key type must be compatible with the cipher suite."), @Experimental CRYPTO_BLOCK_STREAM_SIZE("crypto.block.stream.size", "1K", PropertyType.BYTES, "The size of the buffer above the cipher stream. Used for reading files and padding walog entries."), diff --git a/core/src/main/java/org/apache/accumulo/core/file/rfile/bcfile/BCFile.java b/core/src/main/java/org/apache/accumulo/core/file/rfile/bcfile/BCFile.java index a169619..f9a61a7 100644 --- a/core/src/main/java/org/apache/accumulo/core/file/rfile/bcfile/BCFile.java +++ b/core/src/main/java/org/apache/accumulo/core/file/rfile/bcfile/BCFile.java @@ -283,9 +283,9 @@ public final class BCFile { /** * Get the raw size of the block. * - * Caution: size() comes from DataOutputStream which returns Integer.MAX_VALUE on an overflow. This results in a value of 2GiB meaning that - * an unknown amount of data, at least 2GiB large, has been written. RFiles handle this issue by keeping track of the position of blocks - * instead of relying on blocks to provide this information. + * Caution: size() comes from DataOutputStream which returns Integer.MAX_VALUE on an overflow. This results in a value of 2GiB meaning that an unknown + * amount of data, at least 2GiB large, has been written. RFiles handle this issue by keeping track of the position of blocks instead of relying on blocks + * to provide this information. * * @return the number of uncompressed bytes written through the BlockAppender so far. */ @@ -395,7 +395,7 @@ public final class BCFile { long offsetIndexMeta = out.position(); metaIndex.write(out); - if (cryptoParams.getAlgorithmName() == null || cryptoParams.getAlgorithmName().equals(Property.CRYPTO_CIPHER_SUITE.getDefaultValue())) { + if (cryptoParams.getCipherSuite() == null || cryptoParams.getCipherSuite().equals(Property.CRYPTO_CIPHER_SUITE.getDefaultValue())) { out.writeLong(offsetIndexMeta); API_VERSION_1.write(out); } else { diff --git a/core/src/main/java/org/apache/accumulo/core/security/crypto/CachingHDFSSecretKeyEncryptionStrategy.java b/core/src/main/java/org/apache/accumulo/core/security/crypto/CachingHDFSSecretKeyEncryptionStrategy.java index 7b79d99..ed6d6a5 100644 --- a/core/src/main/java/org/apache/accumulo/core/security/crypto/CachingHDFSSecretKeyEncryptionStrategy.java +++ b/core/src/main/java/org/apache/accumulo/core/security/crypto/CachingHDFSSecretKeyEncryptionStrategy.java @@ -72,7 +72,7 @@ public class CachingHDFSSecretKeyEncryptionStrategy implements SecretKeyEncrypti Cipher cipher = DefaultCryptoModuleUtils.getCipher(params.getAllOptions().get(Property.CRYPTO_DEFAULT_KEY_STRATEGY_CIPHER_SUITE.getKey())); try { - cipher.init(encryptionMode, new SecretKeySpec(secretKeyCache.getKeyEncryptionKey(), params.getAlgorithmName())); + cipher.init(encryptionMode, new SecretKeySpec(secretKeyCache.getKeyEncryptionKey(), params.getKeyAlgorithmName())); } catch (InvalidKeyException e) { log.error("{}", e.getMessage(), e); throw new RuntimeException(e); @@ -80,7 +80,7 @@ public class CachingHDFSSecretKeyEncryptionStrategy implements SecretKeyEncrypti if (Cipher.UNWRAP_MODE == encryptionMode) { try { - Key plaintextKey = cipher.unwrap(params.getEncryptedKey(), params.getAlgorithmName(), Cipher.SECRET_KEY); + Key plaintextKey = cipher.unwrap(params.getEncryptedKey(), params.getKeyAlgorithmName(), Cipher.SECRET_KEY); params.setPlaintextKey(plaintextKey.getEncoded()); } catch (InvalidKeyException e) { log.error("{}", e.getMessage(), e); @@ -90,7 +90,7 @@ public class CachingHDFSSecretKeyEncryptionStrategy implements SecretKeyEncrypti throw new RuntimeException(e); } } else { - Key plaintextKey = new SecretKeySpec(params.getPlaintextKey(), params.getAlgorithmName()); + Key plaintextKey = new SecretKeySpec(params.getPlaintextKey(), params.getKeyAlgorithmName()); try { byte[] encryptedSecretKey = cipher.wrap(plaintextKey); params.setEncryptedKey(encryptedSecretKey); 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 6a295ad..e56ee92 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 @@ -239,14 +239,6 @@ public class CryptoModuleFactory { } - public static String[] parseCipherTransform(String cipherTransform) { - if (cipherTransform == null) { - return new String[3]; - } - - return cipherTransform.split("/"); - } - public static CryptoModuleParameters createParamsObjectFromAccumuloConfiguration(AccumuloConfiguration conf) { CryptoModuleParameters params = new CryptoModuleParameters(); @@ -264,26 +256,21 @@ public class CryptoModuleFactory { } public static CryptoModuleParameters fillParamsObjectFromStringMap(CryptoModuleParameters params, Map<String,String> cryptoOpts) { - - // Parse the cipher suite for the mode and padding options - String[] cipherTransformParts = parseCipherTransform(cryptoOpts.get(Property.CRYPTO_CIPHER_SUITE.getKey())); - + params.setCipherSuite(cryptoOpts.get(Property.CRYPTO_CIPHER_SUITE.getKey())); // If no encryption has been specified, then we abort here. - if (cipherTransformParts[0] == null || cipherTransformParts[0].equals("NullCipher")) { + if (params.getCipherSuite() == null || params.getCipherSuite().equals("NullCipher")) { params.setAllOptions(cryptoOpts); - params.setAlgorithmName("NullCipher"); + return params; } params.setAllOptions(cryptoOpts); - params.setAlgorithmName(cryptoOpts.get(Property.CRYPTO_CIPHER_ALGORITHM_NAME.getKey())); - params.setEncryptionMode(cipherTransformParts[1]); + params.setKeyAlgorithmName(cryptoOpts.get(Property.CRYPTO_CIPHER_KEY_ALGORITHM_NAME.getKey())); params.setKeyEncryptionStrategyClass(cryptoOpts.get(Property.CRYPTO_SECRET_KEY_ENCRYPTION_STRATEGY_CLASS.getKey())); params.setKeyLength(Integer.parseInt(cryptoOpts.get(Property.CRYPTO_CIPHER_KEY_LENGTH.getKey()))); params.setOverrideStreamsSecretKeyEncryptionStrategy(Boolean.parseBoolean(cryptoOpts.get(Property.CRYPTO_OVERRIDE_KEY_STRATEGY_WITH_CONFIGURED_STRATEGY .getKey()))); - params.setPadding(cipherTransformParts[2]); params.setRandomNumberGenerator(cryptoOpts.get(Property.CRYPTO_SECURE_RNG.getKey())); params.setRandomNumberGeneratorProvider(cryptoOpts.get(Property.CRYPTO_SECURE_RNG_PROVIDER.getKey())); String blockStreamSize = cryptoOpts.get(Property.CRYPTO_BLOCK_STREAM_SIZE.getKey()); diff --git a/core/src/main/java/org/apache/accumulo/core/security/crypto/CryptoModuleParameters.java b/core/src/main/java/org/apache/accumulo/core/security/crypto/CryptoModuleParameters.java index a7bb93d..b5a1ae4 100644 --- a/core/src/main/java/org/apache/accumulo/core/security/crypto/CryptoModuleParameters.java +++ b/core/src/main/java/org/apache/accumulo/core/security/crypto/CryptoModuleParameters.java @@ -37,17 +37,17 @@ import javax.crypto.CipherOutputStream; public class CryptoModuleParameters { /** - * Gets the name of the symmetric algorithm to use for encryption. + * Gets the name of the symmetric algorithm to use for the creation of encryption keys. * - * @see CryptoModuleParameters#setAlgorithmName(String) + * @see CryptoModuleParameters#setKeyAlgorithmName(String) */ - public String getAlgorithmName() { - return algorithmName; + public String getKeyAlgorithmName() { + return keyAlgorithmName; } /** - * Sets the name of the symmetric algorithm to use for an encryption stream. + * Sets the name of the symmetric algorithm to use for the creation of encryption keys. * <p> * Valid names are names recognized by your cryptographic engine provider. For the default Java provider, valid names would include things like "AES", "RC4", * "DESede", etc. @@ -56,73 +56,45 @@ public class CryptoModuleParameters { * decryption. <br> * For <b>decryption</b>, this value is often disregarded in favor of the value encoded with the ciphertext. * - * @param algorithmName + * @param keyAlgorithmName * the name of the cryptographic algorithm to use. * @see <a href="http://docs.oracle.com/javase/1.5.0/docs/guide/security/jce/JCERefGuide.html#AppA">Standard Algorithm Names in JCE</a> * */ - public void setAlgorithmName(String algorithmName) { - this.algorithmName = algorithmName; + public void setKeyAlgorithmName(String keyAlgorithmName) { + this.keyAlgorithmName = keyAlgorithmName; } /** - * Gets the name of the encryption mode to use for encryption. + * Gets the name of the cipher suite used for encryption * - * @see CryptoModuleParameters#setEncryptionMode(String) + * @see CryptoModuleParameters#setCipherSuite(String) */ - public String getEncryptionMode() { - return encryptionMode; + public String getCipherSuite() { + return cipherSuite; } /** - * Sets the name of the encryption mode to use for an encryption stream. + * Sets the name of the crypto suite to use for an encryption stream. * <p> - * Valid names are names recognized by your cryptographic engine provider. For the default Java provider, valid names would include things like "EBC", "CBC", - * "CFB", etc. - * <p> - * For <b>encryption</b>, this value is <b>required</b> and is always used. Its value should be prepended or otherwise included with the ciphertext for future - * decryption. <br> - * For <b>decryption</b>, this value is often disregarded in favor of the value encoded with the ciphertext. + * Valid names are names recognized by your cryptographic engine provider. * - * @param encryptionMode - * the name of the encryption mode to use. - * @see <a href="http://docs.oracle.com/javase/1.5.0/docs/guide/security/jce/JCERefGuide.html#AppA">Standard Mode Names in JCE</a> - * - */ - - public void setEncryptionMode(String encryptionMode) { - this.encryptionMode = encryptionMode; - } - - /** - * Gets the name of the padding type to use for encryption. + * The format for input should be: algorithm/mode/padding * - * @see CryptoModuleParameters#setPadding(String) - */ - - public String getPadding() { - return padding; - } - - /** - * Sets the name of the padding type to use for an encryption stream. - * <p> - * Valid names are names recognized by your cryptographic engine provider. For the default Java provider, valid names would include things like "NoPadding", - * "None", etc. + * For the default Java provider, valid names would include things like "AES/CBC/NoPadding". * <p> * For <b>encryption</b>, this value is <b>required</b> and is always used. Its value should be prepended or otherwise included with the ciphertext for future * decryption. <br> * For <b>decryption</b>, this value is often disregarded in favor of the value encoded with the ciphertext. * - * @param padding - * the name of the padding type to use. - * @see <a href="http://docs.oracle.com/javase/1.5.0/docs/guide/security/jce/JCERefGuide.html#AppA">Standard Padding Names in JCE</a> + * @param cipherSuite + * the cipher suite to use. * */ - public void setPadding(String padding) { - this.padding = padding; + public void setCipherSuite(String cipherSuite) { + this.cipherSuite = cipherSuite; } /** @@ -602,9 +574,8 @@ public class CryptoModuleParameters { this.allOptions = allOptions; } - private String algorithmName = null; - private String encryptionMode = null; - private String padding = null; + private String cipherSuite = null; + private String keyAlgorithmName = null; private byte[] plaintextKey; private int keyLength = 0; private String randomNumberGenerator = null; diff --git a/core/src/main/java/org/apache/accumulo/core/security/crypto/DefaultCryptoModule.java b/core/src/main/java/org/apache/accumulo/core/security/crypto/DefaultCryptoModule.java index c5c41cd..1798e89 100644 --- a/core/src/main/java/org/apache/accumulo/core/security/crypto/DefaultCryptoModule.java +++ b/core/src/main/java/org/apache/accumulo/core/security/crypto/DefaultCryptoModule.java @@ -56,10 +56,9 @@ public class DefaultCryptoModule implements CryptoModule { @Override public CryptoModuleParameters initializeCipher(CryptoModuleParameters params) { - String cipherTransformation = getCipherTransformation(params); log.trace(String.format("Using cipher suite \"%s\" with key length %d with RNG \"%s\" and RNG provider \"%s\" and key encryption strategy \"%s\"", - cipherTransformation, params.getKeyLength(), params.getRandomNumberGenerator(), params.getRandomNumberGeneratorProvider(), + params.getCipherSuite(), params.getKeyLength(), params.getRandomNumberGenerator(), params.getRandomNumberGeneratorProvider(), params.getKeyEncryptionStrategyClass())); if (params.getSecureRandom() == null) { @@ -67,11 +66,11 @@ public class DefaultCryptoModule implements CryptoModule { params.setSecureRandom(secureRandom); } - Cipher cipher = DefaultCryptoModuleUtils.getCipher(cipherTransformation); + Cipher cipher = DefaultCryptoModuleUtils.getCipher(params.getCipherSuite()); if (params.getInitializationVector() == null) { try { - cipher.init(Cipher.ENCRYPT_MODE, new SecretKeySpec(params.getPlaintextKey(), params.getAlgorithmName()), params.getSecureRandom()); + cipher.init(Cipher.ENCRYPT_MODE, new SecretKeySpec(params.getPlaintextKey(), params.getKeyAlgorithmName()), params.getSecureRandom()); } catch (InvalidKeyException e) { log.error("Accumulo encountered an unknown error in generating the secret key object (SecretKeySpec) for an encrypted stream"); throw new RuntimeException(e); @@ -81,7 +80,7 @@ public class DefaultCryptoModule implements CryptoModule { } else { try { - cipher.init(Cipher.ENCRYPT_MODE, new SecretKeySpec(params.getPlaintextKey(), params.getAlgorithmName()), + cipher.init(Cipher.ENCRYPT_MODE, new SecretKeySpec(params.getPlaintextKey(), params.getKeyAlgorithmName()), new IvParameterSpec(params.getInitializationVector())); } catch (InvalidKeyException e) { log.error("Accumulo encountered an unknown error in generating the secret key object (SecretKeySpec) for an encrypted stream"); @@ -98,15 +97,6 @@ public class DefaultCryptoModule implements CryptoModule { } - private String getCipherTransformation(CryptoModuleParameters params) { - String cipherSuite = params.getAlgorithmName() + "/" + params.getEncryptionMode() + "/" + params.getPadding(); - return cipherSuite; - } - - private String[] parseCipherSuite(String cipherSuite) { - return cipherSuite.split("/"); - } - private boolean validateNotEmpty(String givenValue, boolean allIsWell, StringBuilder buf, String errorMessage) { if (givenValue == null || givenValue.equals("")) { buf.append(errorMessage); @@ -145,15 +135,14 @@ public class DefaultCryptoModule implements CryptoModule { "The following problems were found with the CryptoModuleParameters object you provided for an encrypt operation:\n"); boolean allIsWell = true; - allIsWell = validateNotEmpty(params.getAlgorithmName(), allIsWell, errorBuf, "No algorithm name was specified."); + allIsWell = validateNotEmpty(params.getCipherSuite(), allIsWell, errorBuf, "No cipher suite was specified."); - if (allIsWell && params.getAlgorithmName().equals("NullCipher")) { + if (allIsWell && params.getCipherSuite().equals("NullCipher")) { return true; } - allIsWell = validateNotEmpty(params.getPadding(), allIsWell, errorBuf, "No padding was specified."); allIsWell = validateNotZero(params.getKeyLength(), allIsWell, errorBuf, "No key length was specified."); - allIsWell = validateNotEmpty(params.getEncryptionMode(), allIsWell, errorBuf, "No encryption mode was specified."); + allIsWell = validateNotEmpty(params.getKeyAlgorithmName(), allIsWell, errorBuf, "No key algorithm name was specified."); allIsWell = validateNotEmpty(params.getRandomNumberGenerator(), allIsWell, errorBuf, "No random number generator was specified."); allIsWell = validateNotEmpty(params.getRandomNumberGeneratorProvider(), allIsWell, errorBuf, "No random number generate provider was specified."); allIsWell = validateNotNull(params.getPlaintextOutputStream(), allIsWell, errorBuf, "No plaintext output stream was specified."); @@ -171,9 +160,7 @@ public class DefaultCryptoModule implements CryptoModule { "The following problems were found with the CryptoModuleParameters object you provided for a decrypt operation:\n"); boolean allIsWell = true; - allIsWell = validateNotEmpty(params.getPadding(), allIsWell, errorBuf, "No padding was specified."); allIsWell = validateNotZero(params.getKeyLength(), allIsWell, errorBuf, "No key length was specified."); - allIsWell = validateNotEmpty(params.getEncryptionMode(), allIsWell, errorBuf, "No encryption mode was specified."); allIsWell = validateNotEmpty(params.getRandomNumberGenerator(), allIsWell, errorBuf, "No random number generator was specified."); allIsWell = validateNotEmpty(params.getRandomNumberGeneratorProvider(), allIsWell, errorBuf, "No random number generate provider was specified."); allIsWell = validateNotNull(params.getEncryptedInputStream(), allIsWell, errorBuf, "No encrypted input stream was specified."); @@ -211,7 +198,7 @@ public class DefaultCryptoModule implements CryptoModule { } // If they want a null output stream, just return their plaintext stream as the encrypted stream - if (params.getAlgorithmName().equals("NullCipher")) { + if (params.getCipherSuite().equals("NullCipher")) { params.setEncryptedOutputStream(params.getPlaintextOutputStream()); return params; } @@ -271,8 +258,8 @@ public class DefaultCryptoModule implements CryptoModule { // Write out the cipher suite and algorithm used to encrypt this file. In case the admin changes, we want to still // decode the old format. - dataOut.writeUTF(getCipherTransformation(params)); - dataOut.writeUTF(params.getAlgorithmName()); + dataOut.writeUTF(params.getCipherSuite()); + dataOut.writeUTF(params.getKeyAlgorithmName()); // Write the init vector to the log file dataOut.writeInt(params.getInitializationVector().length); @@ -313,10 +300,8 @@ public class DefaultCryptoModule implements CryptoModule { // Set the cipher parameters String cipherSuiteFromFile = dataIn.readUTF(); String algorithmNameFromFile = dataIn.readUTF(); - String[] cipherSuiteParts = parseCipherSuite(cipherSuiteFromFile); - params.setAlgorithmName(algorithmNameFromFile); - params.setEncryptionMode(cipherSuiteParts[1]); - params.setPadding(cipherSuiteParts[2]); + params.setCipherSuite(cipherSuiteFromFile); + params.setKeyAlgorithmName(algorithmNameFromFile); // Read the secret key and initialization vector from the file int initVectorLength = dataIn.readInt(); @@ -382,10 +367,10 @@ public class DefaultCryptoModule implements CryptoModule { throw new RuntimeException("CryptoModuleParameters object failed validation for decrypt"); } - Cipher cipher = DefaultCryptoModuleUtils.getCipher(getCipherTransformation(params)); + Cipher cipher = DefaultCryptoModuleUtils.getCipher(params.getCipherSuite()); try { - cipher.init(Cipher.DECRYPT_MODE, new SecretKeySpec(params.getPlaintextKey(), params.getAlgorithmName()), + cipher.init(Cipher.DECRYPT_MODE, new SecretKeySpec(params.getPlaintextKey(), params.getKeyAlgorithmName()), new IvParameterSpec(params.getInitializationVector())); } catch (InvalidKeyException e) { log.error("Error when trying to initialize cipher with secret key"); @@ -400,7 +385,7 @@ public class DefaultCryptoModule implements CryptoModule { if (params.getBlockStreamSize() > 0) blockedDecryptingInputStream = new BlockedInputStream(blockedDecryptingInputStream, cipher.getBlockSize(), params.getBlockStreamSize()); - log.trace("Initialized cipher input stream with transformation [{}]", getCipherTransformation(params)); + log.trace("Initialized cipher input stream with transformation [{}]", params.getCipherSuite()); params.setPlaintextInputStream(blockedDecryptingInputStream); @@ -420,5 +405,4 @@ public class DefaultCryptoModule implements CryptoModule { return params; } - } diff --git a/core/src/main/java/org/apache/accumulo/core/security/crypto/NonCachingSecretKeyEncryptionStrategy.java b/core/src/main/java/org/apache/accumulo/core/security/crypto/NonCachingSecretKeyEncryptionStrategy.java index 5c9ca8c..2d2d2f7 100644 --- a/core/src/main/java/org/apache/accumulo/core/security/crypto/NonCachingSecretKeyEncryptionStrategy.java +++ b/core/src/main/java/org/apache/accumulo/core/security/crypto/NonCachingSecretKeyEncryptionStrategy.java @@ -83,7 +83,7 @@ public class NonCachingSecretKeyEncryptionStrategy implements SecretKeyEncryptio // check if the number of bytes read into the array is the same as the value of the length field, if (bytesRead == keyEncryptionKeyLength) { try { - cipher.init(encryptionMode, new SecretKeySpec(keyEncryptionKey, params.getAlgorithmName())); + cipher.init(encryptionMode, new SecretKeySpec(keyEncryptionKey, params.getKeyAlgorithmName())); } catch (InvalidKeyException e) { log.error("{}", e.getMessage(), e); throw new RuntimeException(e); @@ -91,7 +91,7 @@ public class NonCachingSecretKeyEncryptionStrategy implements SecretKeyEncryptio if (Cipher.UNWRAP_MODE == encryptionMode) { try { - Key plaintextKey = cipher.unwrap(params.getEncryptedKey(), params.getAlgorithmName(), Cipher.SECRET_KEY); + Key plaintextKey = cipher.unwrap(params.getEncryptedKey(), params.getKeyAlgorithmName(), Cipher.SECRET_KEY); params.setPlaintextKey(plaintextKey.getEncoded()); } catch (InvalidKeyException e) { log.error("{}", e.getMessage(), e); @@ -101,7 +101,7 @@ public class NonCachingSecretKeyEncryptionStrategy implements SecretKeyEncryptio throw new RuntimeException(e); } } else { - Key plaintextKey = new SecretKeySpec(params.getPlaintextKey(), params.getAlgorithmName()); + Key plaintextKey = new SecretKeySpec(params.getPlaintextKey(), params.getKeyAlgorithmName()); try { byte[] encryptedSecretKey = cipher.wrap(plaintextKey); params.setEncryptedKey(encryptedSecretKey); diff --git a/core/src/main/java/org/apache/accumulo/core/security/crypto/RFileCipherOutputStream.java b/core/src/main/java/org/apache/accumulo/core/security/crypto/RFileCipherOutputStream.java index 7dad802..cb85d45 100644 --- a/core/src/main/java/org/apache/accumulo/core/security/crypto/RFileCipherOutputStream.java +++ b/core/src/main/java/org/apache/accumulo/core/security/crypto/RFileCipherOutputStream.java @@ -24,9 +24,8 @@ import javax.crypto.CipherOutputStream; /** * - * This class extends {@link CipherOutputStream} to include a way to track the number of bytes that have - * been encrypted by the stream. The write method also includes a mechanism to stop writing and - * throw an exception if exceeding a maximum number of bytes is attempted. + * This class extends {@link CipherOutputStream} to include a way to track the number of bytes that have been encrypted by the stream. The write method also + * includes a mechanism to stop writing and throw an exception if exceeding a maximum number of bytes is attempted. * */ public class RFileCipherOutputStream extends CipherOutputStream { @@ -35,7 +34,7 @@ public class RFileCipherOutputStream extends CipherOutputStream { // will cause an exception. Given that each block in an rfile is encrypted separately, and blocks // should be written such that a block cannot ever reach 16GiB, this is believed to be a safe number. // If this does cause an exception, it is an issue best addressed elsewhere. - private final long maxOutputSize = 1L << 34; //16GiB + private final long maxOutputSize = 1L << 34; // 16GiB // The total number of bytes that have been written out private long count = 0; @@ -54,8 +53,8 @@ public class RFileCipherOutputStream extends CipherOutputStream { } /** - * Override of CipherOutputStream's write to count the number of bytes that have been encrypted. - * This method now throws an exception if an attempt to write bytes beyond a maximum is made. + * Override of CipherOutputStream's write to count the number of bytes that have been encrypted. This method now throws an exception if an attempt to write + * bytes beyond a maximum is made. */ @Override public void write(byte b[], int off, int len) throws IOException { @@ -72,8 +71,8 @@ public class RFileCipherOutputStream extends CipherOutputStream { } /** - * Override of CipherOutputStream's write for a single byte to count it. This method now throws - * an exception if an attempt to write bytes beyond a maximum is made. + * Override of CipherOutputStream's write for a single byte to count it. This method now throws an exception if an attempt to write bytes beyond a maximum is + * made. */ @Override public void write(int b) throws IOException { 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 7ac2113..9f2ff8e 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 @@ -28,6 +28,8 @@ public class ConfigSanityCheckTest { @Before public void setUp() { m = new java.util.HashMap<>(); + m.put(Property.CRYPTO_CIPHER_SUITE.getKey(), "NullCipher"); + m.put(Property.CRYPTO_CIPHER_KEY_ALGORITHM_NAME.getKey(), "NullCipher"); } @Test @@ -77,4 +79,18 @@ public class ConfigSanityCheckTest { m.put(Property.INSTANCE_ZK_TIMEOUT.getKey(), "10ms"); ConfigSanityCheck.validate(m.entrySet()); } + + @Test(expected = SanityCheckException.class) + public void testFail_cipherSuiteSetKeyAlgorithmNotSet() { + m.put(Property.CRYPTO_CIPHER_SUITE.getKey(), "AES/CBC/NoPadding"); + m.put(Property.CRYPTO_CIPHER_KEY_ALGORITHM_NAME.getKey(), "NullCipher"); + ConfigSanityCheck.validate(m.entrySet()); + } + + @Test(expected = SanityCheckException.class) + public void testFail_cipherSuiteNotSetKeyAlgorithmSet() { + m.put(Property.CRYPTO_CIPHER_SUITE.getKey(), "NullCipher"); + m.put(Property.CRYPTO_CIPHER_KEY_ALGORITHM_NAME.getKey(), "AES"); + ConfigSanityCheck.validate(m.entrySet()); + } } 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 a32a465..4467828 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 @@ -19,7 +19,6 @@ package org.apache.accumulo.core.security.crypto; import static org.junit.Assert.assertEquals; import static org.junit.Assert.assertNotNull; -import static org.junit.Assert.assertNull; import static org.junit.Assert.assertTrue; import java.io.ByteArrayInputStream; @@ -71,9 +70,7 @@ public class CryptoTest { CryptoModuleParameters params = CryptoModuleFactory.createParamsObjectFromAccumuloConfiguration(conf); assertNotNull(params); - assertEquals("NullCipher", params.getAlgorithmName()); - assertNull(params.getEncryptionMode()); - assertNull(params.getPadding()); + assertEquals("NullCipher", params.getCipherSuite()); CryptoModule cryptoModule = CryptoModuleFactory.getCryptoModule(conf); assertNotNull(cryptoModule); @@ -95,9 +92,9 @@ public class CryptoTest { CryptoModuleParameters params = CryptoModuleFactory.createParamsObjectFromAccumuloConfiguration(conf); assertNotNull(params); - assertEquals("AES", params.getAlgorithmName()); - assertEquals("CFB", params.getEncryptionMode()); - assertEquals("NoPadding", params.getPadding()); + assertEquals("AES/CFB/NoPadding", params.getCipherSuite()); + assertEquals("AES/CBC/NoPadding", params.getAllOptions().get(Property.CRYPTO_WAL_CIPHER_SUITE.getKey())); + assertEquals("AES", params.getKeyAlgorithmName()); assertEquals(128, params.getKeyLength()); assertEquals("SHA1PRNG", params.getRandomNumberGenerator()); assertEquals("SUN", params.getRandomNumberGeneratorProvider()); @@ -314,7 +311,7 @@ public class CryptoTest { // from those configured within the site configuration. After doing this, we should // still be able to read the file that was created with a different set of parameters. params = CryptoModuleFactory.createParamsObjectFromAccumuloConfiguration(conf); - params.setAlgorithmName("DESede"); + params.setKeyAlgorithmName("DESede"); params.setKeyLength(24 * 8); ByteArrayInputStream in = new ByteArrayInputStream(resultingBytes); diff --git a/core/src/test/resources/crypto-on-accumulo-site.xml b/core/src/test/resources/crypto-on-accumulo-site.xml index ebfc9ae..f5824c4 100644 --- a/core/src/test/resources/crypto-on-accumulo-site.xml +++ b/core/src/test/resources/crypto-on-accumulo-site.xml @@ -80,7 +80,11 @@ <value>AES/CFB/NoPadding</value> </property> <property> - <name>crypto.cipher.algorithm.name</name> + <name>crypto.wal.cipher.suite</name> + <value>AES/CBC/NoPadding</value> + </property> + <property> + <name>crypto.cipher.key.algorithm.name</name> <value>AES</value> </property> <property> 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 d980783..a82f9f9 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 @@ -80,7 +80,7 @@ <value>AES/CFB/NoPadding</value> </property> <property> - <name>crypto.cipher.algorithm.name</name> + <name>crypto.cipher.key.algorithm.name</name> <value>AES</value> </property> <property> diff --git a/server/tserver/src/main/java/org/apache/accumulo/tserver/log/DfsLogger.java b/server/tserver/src/main/java/org/apache/accumulo/tserver/log/DfsLogger.java index d336c3c..d48907f 100644 --- a/server/tserver/src/main/java/org/apache/accumulo/tserver/log/DfsLogger.java +++ b/server/tserver/src/main/java/org/apache/accumulo/tserver/log/DfsLogger.java @@ -467,6 +467,10 @@ public class DfsLogger implements Comparable<DfsLogger> { logFile.write(LOG_FILE_HEADER_V3.getBytes(UTF_8)); CryptoModuleParameters params = CryptoModuleFactory.createParamsObjectFromAccumuloConfiguration(conf.getConfiguration()); + // Immediately update to the correct cipher. Doing this here keeps the CryptoModule independent of the writers using it + if (params.getAllOptions().get(Property.CRYPTO_WAL_CIPHER_SUITE.getKey()) != null) { + params.setCipherSuite(params.getAllOptions().get(Property.CRYPTO_WAL_CIPHER_SUITE.getKey())); + } NoFlushOutputStream nfos = new NoFlushOutputStream(logFile); params.setPlaintextOutputStream(nfos); diff --git a/test/src/main/java/org/apache/accumulo/test/ShellConfigIT.java b/test/src/main/java/org/apache/accumulo/test/ShellConfigIT.java index 977cc36..f90435d 100644 --- a/test/src/main/java/org/apache/accumulo/test/ShellConfigIT.java +++ b/test/src/main/java/org/apache/accumulo/test/ShellConfigIT.java @@ -93,11 +93,11 @@ public class ShellConfigIT extends AccumuloClusterHarness { Assert.fail("Unknown token type"); } - assertTrue(Property.CRYPTO_CIPHER_ALGORITHM_NAME.isExperimental()); + assertTrue(Property.CRYPTO_CIPHER_KEY_ALGORITHM_NAME.isExperimental()); String configOutput = ts.exec("config"); assertTrue(configOutput.contains(PerTableVolumeChooser.TABLE_VOLUME_CHOOSER)); - assertFalse(configOutput.contains(Property.CRYPTO_CIPHER_ALGORITHM_NAME.getKey())); + assertFalse(configOutput.contains(Property.CRYPTO_CIPHER_KEY_ALGORITHM_NAME.getKey())); } } -- To stop receiving notification emails like this one, please contact "commits@accumulo.apache.org" <commits@accumulo.apache.org>.