[GitHub] keith-turner closed pull request #315: ACCUMULO-4731 Improve exception handling if a key encryption key cannot be loaded

[GitBox <gi...@apache.org>]

keith-turner closed pull request #315: ACCUMULO-4731 Improve exception handling if a key encryption key cannot be loaded
URL: https://github.com/apache/accumulo/pull/315
 
 
   

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/security/crypto/CachingHDFSSecretKeyEncryptionStrategy.java b/core/src/main/java/org/apache/accumulo/core/security/crypto/CachingHDFSSecretKeyEncryptionStrategy.java
index 1fa659a3fd..58f10101f7 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
@@ -18,6 +18,7 @@
 
 import java.io.DataInputStream;
 import java.io.DataOutputStream;
+import java.io.EOFException;
 import java.io.IOException;
 import java.security.InvalidKeyException;
 import java.security.Key;
@@ -45,13 +46,13 @@
   private SecretKeyCache secretKeyCache = new SecretKeyCache();
 
   @Override
-  public CryptoModuleParameters encryptSecretKey(CryptoModuleParameters context) {
+  public CryptoModuleParameters encryptSecretKey(CryptoModuleParameters context) throws IOException {
     try {
       secretKeyCache.ensureSecretKeyCacheInitialized(context);
       doKeyEncryptionOperation(Cipher.WRAP_MODE, context);
     } catch (IOException e) {
       log.error("{}", e.getMessage(), e);
-      throw new RuntimeException(e);
+      throw new IOException(e);
     }
     return context;
   }
@@ -128,11 +129,14 @@ public synchronized void ensureSecretKeyCacheInitialized(CryptoModuleParameters
         pathToKeyName = Property.CRYPTO_DEFAULT_KEY_STRATEGY_KEY_LOCATION.getDefaultValue();
       }
 
-      // TODO ACCUMULO-2530 Ensure volumes a properly supported
+      // TODO ACCUMULO-2530 Ensure volumes are properly supported
       Path pathToKey = new Path(pathToKeyName);
       FileSystem fs = FileSystem.get(CachedConfiguration.getInstance());
 
       DataInputStream in = null;
+      boolean invalidFile = false;
+      int keyEncryptionKeyLength = 0;
+
       try {
         if (!fs.exists(pathToKey)) {
           initializeKeyEncryptionKey(fs, pathToKey, context);
@@ -140,14 +144,29 @@ public synchronized void ensureSecretKeyCacheInitialized(CryptoModuleParameters
 
         in = fs.open(pathToKey);
 
-        int keyEncryptionKeyLength = in.readInt();
+        keyEncryptionKeyLength = in.readInt();
+        // If the file length does not correctly relate to the expected key size, there is an inconsistency and
+        // we have no way of knowing the correct key length.
+        // The keyEncryptionKeyLength+4 accounts for the integer read from the file.
+        if (fs.getFileStatus(pathToKey).getLen() != keyEncryptionKeyLength + 4) {
+          invalidFile = true;
+	// Passing this exception forward so we can provide the more useful error message
+          throw new IOException();
+        }
         keyEncryptionKey = new byte[keyEncryptionKeyLength];
         in.readFully(keyEncryptionKey);
 
         initialized = true;
 
+      } catch (EOFException e) {
+        throw new IOException("Could not initialize key encryption cache, malformed key encryption key file", e);
       } catch (IOException e) {
-        log.error("Could not initialize key encryption cache", e);
+        if (invalidFile) {
+          throw new IOException("Could not initialize key encryption cache, malformed key encryption key file. Expected key of lengh " + keyEncryptionKeyLength
+              + " but file contained " + (fs.getFileStatus(pathToKey).getLen() - 4) + "bytes for key encryption key.");
+        } else {
+          throw new IOException("Could not initialize key encryption cache, unable to access or find key encryption key file", e);
+        }
       } finally {
         IOUtils.closeQuietly(in);
       }
diff --git a/core/src/main/java/org/apache/accumulo/core/security/crypto/SecretKeyEncryptionStrategy.java b/core/src/main/java/org/apache/accumulo/core/security/crypto/SecretKeyEncryptionStrategy.java
index 7d3c33302b..8dfdee16aa 100644
--- a/core/src/main/java/org/apache/accumulo/core/security/crypto/SecretKeyEncryptionStrategy.java
+++ b/core/src/main/java/org/apache/accumulo/core/security/crypto/SecretKeyEncryptionStrategy.java
@@ -16,12 +16,14 @@
  */
 package org.apache.accumulo.core.security.crypto;
 
+import java.io.IOException;
+
 /**
  *
  */
 public interface SecretKeyEncryptionStrategy {
 
-  CryptoModuleParameters encryptSecretKey(CryptoModuleParameters params);
+  CryptoModuleParameters encryptSecretKey(CryptoModuleParameters params) throws IOException;
 
   CryptoModuleParameters decryptSecretKey(CryptoModuleParameters params);
 
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 a403f69583..d4bf4aead3 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
@@ -28,8 +28,11 @@
 import java.io.ByteArrayOutputStream;
 import java.io.DataInputStream;
 import java.io.DataOutputStream;
+import java.io.File;
+import java.io.FileOutputStream;
 import java.io.IOException;
 import java.io.InputStream;
+import java.nio.file.Files;
 import java.security.InvalidKeyException;
 import java.security.Key;
 import java.security.NoSuchAlgorithmException;
@@ -37,6 +40,7 @@
 import java.security.SecureRandom;
 import java.util.Arrays;
 import java.util.Map.Entry;
+import java.util.Random;
 
 import javax.crypto.AEADBadTagException;
 import javax.crypto.BadPaddingException;
@@ -50,6 +54,7 @@
 import org.apache.accumulo.core.conf.DefaultConfiguration;
 import org.apache.accumulo.core.conf.Property;
 import org.apache.hadoop.conf.Configuration;
+import org.junit.AfterClass;
 import org.junit.Rule;
 import org.junit.Test;
 import org.junit.rules.ExpectedException;
@@ -63,6 +68,11 @@
   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";
+  
+  //Used for kek file testing
+  private static File kekWorks;
+  private static File kekTooLong;
+  private static File kekTooShort;
 
   @Rule
   public ExpectedException exception = ExpectedException.none();
@@ -378,6 +388,68 @@ public void testKeyWrapAndUnwrap() throws NoSuchAlgorithmException, NoSuchPaddin
   }
 
   @Test
+  public void testKeyEncryptionKeyCatchCorrectlyUsesValidKEKFile() throws IOException {
+    kekWorks = createKekFile("kekWorks.kek", 16);
+    testKekFile(kekWorks);
+  }
+
+  @Test
+  public void testKeyEncryptionKeyCacheCorrectlyFailsWithInvalidLongKEKFile() throws IOException {
+    kekTooLong = createKekFile("kekTooLong.kek", 8);
+    exception.expect(IOException.class);
+    testKekFile(kekTooLong);
+  }
+
+  @Test
+  public void testKeyEncryptionKeyCacheCorrectlyFailsWithInvalidShortKEKFile() throws IOException {
+    kekTooShort = createKekFile("kekTooShort.kek", 32);
+    exception.expect(IOException.class);
+    testKekFile(kekTooShort);
+  }
+
+  @AfterClass
+  public static void removeAllTestKekFiles() throws IOException {
+	Files.deleteIfExists(kekWorks.toPath());
+    Files.deleteIfExists(kekTooShort.toPath());
+    Files.deleteIfExists(kekTooLong.toPath());
+  }
+
+  // Used to check reading of KEK files
+  @SuppressWarnings("deprecation")
+  private void testKekFile(File testFile) throws IOException {
+	assertTrue(testFile.exists());
+	assertFalse(testFile.isDirectory());
+		
+    AccumuloConfiguration conf = setAndGetAccumuloConfig(CRYPTO_ON_CONF);
+    CryptoModuleParameters params = CryptoModuleFactory.createParamsObjectFromAccumuloConfiguration(conf);
+    // TODO ACCUMULO-2530 this will need to be fixed when CachingHDFSSecretKeyEncryptionStrategy is fixed
+    params.getAllOptions().put(Property.INSTANCE_DFS_DIR.getKey(), testFile.getParent());
+    byte[] ptk = new byte[16];
+    params.setPlaintextKey(ptk);
+    CachingHDFSSecretKeyEncryptionStrategy skc = new CachingHDFSSecretKeyEncryptionStrategy();
+    params.getAllOptions().put(Property.CRYPTO_DEFAULT_KEY_STRATEGY_KEY_LOCATION.getKey(), testFile.getName());
+    skc.encryptSecretKey(params);
+  }
+
+  private File createKekFile(String filename, Integer size) throws IOException {
+    File dir = new File(System.getProperty("user.dir") + "/target/cryptoTest");
+    boolean unused = dir.mkdirs(); // if the directories don't already exist, it'll return 1. If they do, 0. Both cases can be fine.
+
+    File testFile = File.createTempFile(filename, ".kek", dir);
+    DataOutputStream os = new DataOutputStream(new FileOutputStream(testFile));
+    Integer kl = 16;
+    byte[] key = new byte[kl];
+    Random rand = new Random();
+    rand.nextBytes(key);
+    os.writeInt(size);
+    os.write(key);
+    os.flush();
+    os.close();
+    
+    return testFile;
+    
+  }
+
   public void AESGCM_Encryption_Test_Correct_Encryption_And_Decryption() throws IOException, AEADBadTagException {
     AccumuloConfiguration conf = setAndGetAccumuloConfig(CRYPTO_ON_CONF);
     byte[] encryptedBytes = testEncryption(conf, new byte[] {0, 1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 20});
@@ -567,5 +639,4 @@ private static Integer testDecryption(AccumuloConfiguration conf, byte[] encrypt
       return 0;
     }
   }
-
 }


 

----------------------------------------------------------------
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:
users@infra.apache.org


With regards,
Apache Git Services