HADOOP-10427. KeyProvider implementations should be thread safe. (tucu)

git-svn-id: https://svn.apache.org/repos/asf/hadoop/common/trunk@1586103 13f79535-47bb-0310-9956-ffa450edef68

hadoop-common-project/hadoop-common/CHANGES.txt

@@ -130,6 +130,8 @@ Trunk (Unreleased)
     HADOOP-10432. Refactor SSLFactory to expose static method to determine
     HostnameVerifier. (tucu)
 
+    HADOOP-10427. KeyProvider implementations should be thread safe. (tucu)
+
   BUG FIXES
 
     HADOOP-9451. Fault single-layer config if node group topology is enabled.

hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/crypto/key/JavaKeyStoreProvider.java

@@ -43,6 +43,9 @@
 import java.util.HashMap;
 import java.util.List;
 import java.util.Map;
+import java.util.concurrent.locks.Lock;
+import java.util.concurrent.locks.ReadWriteLock;
+import java.util.concurrent.locks.ReentrantReadWriteLock;
 
 /**
  * KeyProvider based on Java's KeyStore file format. The file may be stored in
@@ -73,6 +76,8 @@ public class JavaKeyStoreProvider extends KeyProvider {
   private final KeyStore keyStore;
   private final char[] password;
   private boolean changed = false;
+  private Lock readLock;
+  private Lock writeLock;
 
   private final Map<String, Metadata> cache = new HashMap<String, Metadata>();
 
@@ -107,138 +112,171 @@ private JavaKeyStoreProvider(URI uri, Configuration conf) throws IOException {
     } catch (CertificateException e) {
       throw new IOException("Can't load keystore " + path, e);
     }
+    ReadWriteLock lock = new ReentrantReadWriteLock(true);
+    readLock = lock.readLock();
+    writeLock = lock.writeLock();
   }
 
   @Override
   public KeyVersion getKeyVersion(String versionName) throws IOException {
-    SecretKeySpec key = null;
+    readLock.lock();
     try {
-      if (!keyStore.containsAlias(versionName)) {
+      SecretKeySpec key = null;
-        return null;
+      try {
+        if (!keyStore.containsAlias(versionName)) {
+          return null;
+        }
+        key = (SecretKeySpec) keyStore.getKey(versionName, password);
+      } catch (KeyStoreException e) {
+        throw new IOException("Can't get key " + versionName + " from " +
+                              path, e);
+      } catch (NoSuchAlgorithmException e) {
+        throw new IOException("Can't get algorithm for key " + key + " from " +
+                              path, e);
+      } catch (UnrecoverableKeyException e) {
+        throw new IOException("Can't recover key " + key + " from " + path, e);
       }
-      key = (SecretKeySpec) keyStore.getKey(versionName, password);
+      return new KeyVersion(versionName, key.getEncoded());
-    } catch (KeyStoreException e) {
+    } finally {
-      throw new IOException("Can't get key " + versionName + " from " +
+      readLock.unlock();
-                            path, e);
-    } catch (NoSuchAlgorithmException e) {
-      throw new IOException("Can't get algorithm for key " + key + " from " +
-                            path, e);
-    } catch (UnrecoverableKeyException e) {
-      throw new IOException("Can't recover key " + key + " from " + path, e);
     }
-    return new KeyVersion(versionName, key.getEncoded());
   }
 
   @Override
   public List<String> getKeys() throws IOException {
-    ArrayList<String> list = new ArrayList<String>();
+    readLock.lock();
-    String alias = null;
     try {
-      Enumeration<String> e = keyStore.aliases();
+      ArrayList<String> list = new ArrayList<String>();
-      while (e.hasMoreElements()) {
+      String alias = null;
-         alias = e.nextElement();
+      try {
-         // only include the metadata key names in the list of names
+        Enumeration<String> e = keyStore.aliases();
-         if (!alias.contains("@")) {
+        while (e.hasMoreElements()) {
-             list.add(alias);
+           alias = e.nextElement();
-         }
+           // only include the metadata key names in the list of names
+           if (!alias.contains("@")) {
+               list.add(alias);
+           }
+        }
+      } catch (KeyStoreException e) {
+        throw new IOException("Can't get key " + alias + " from " + path, e);
       }
-    } catch (KeyStoreException e) {
+      return list;
-      throw new IOException("Can't get key " + alias + " from " + path, e);
+    } finally {
+      readLock.unlock();
     }
-    return list;
   }
 
   @Override
   public List<KeyVersion> getKeyVersions(String name) throws IOException {
-    List<KeyVersion> list = new ArrayList<KeyVersion>();
+    readLock.lock();
-    Metadata km = getMetadata(name);
+    try {
-    if (km != null) {
+      List<KeyVersion> list = new ArrayList<KeyVersion>();
-      int latestVersion = km.getVersions();
+      Metadata km = getMetadata(name);
-      KeyVersion v = null;
+      if (km != null) {
-      String versionName = null;
+        int latestVersion = km.getVersions();
-      for (int i = 0; i < latestVersion; i++) {
+        KeyVersion v = null;
-        versionName = buildVersionName(name, i);
+        String versionName = null;
-        v = getKeyVersion(versionName);
+        for (int i = 0; i < latestVersion; i++) {
-        if (v != null) {
+          versionName = buildVersionName(name, i);
-          list.add(v);
+          v = getKeyVersion(versionName);
+          if (v != null) {
+            list.add(v);
+          }
         }
       }
+      return list;
+    } finally {
+      readLock.unlock();
     }
-    return list;
   }
 
   @Override
   public Metadata getMetadata(String name) throws IOException {
-    if (cache.containsKey(name)) {
+    readLock.lock();
-      return cache.get(name);
-    }
     try {
-      if (!keyStore.containsAlias(name)) {
+      if (cache.containsKey(name)) {
-        return null;
+        return cache.get(name);
       }
-      Metadata meta = ((KeyMetadata) keyStore.getKey(name, password)).metadata;
+      try {
-      cache.put(name, meta);
+        if (!keyStore.containsAlias(name)) {
-      return meta;
+          return null;
-    } catch (KeyStoreException e) {
+        }
-      throw new IOException("Can't get metadata for " + name +
+        Metadata meta = ((KeyMetadata) keyStore.getKey(name, password)).metadata;
-          " from keystore " + path, e);
+        cache.put(name, meta);
-    } catch (NoSuchAlgorithmException e) {
+        return meta;
-      throw new IOException("Can't get algorithm for " + name +
+      } catch (KeyStoreException e) {
-          " from keystore " + path, e);
+        throw new IOException("Can't get metadata for " + name +
-    } catch (UnrecoverableKeyException e) {
+            " from keystore " + path, e);
-      throw new IOException("Can't recover key for " + name +
+      } catch (NoSuchAlgorithmException e) {
-          " from keystore " + path, e);
+        throw new IOException("Can't get algorithm for " + name +
+            " from keystore " + path, e);
+      } catch (UnrecoverableKeyException e) {
+        throw new IOException("Can't recover key for " + name +
+            " from keystore " + path, e);
+      }
+    } finally {
+      readLock.unlock();
     }
   }
 
   @Override
   public KeyVersion createKey(String name, byte[] material,
                                Options options) throws IOException {
+    writeLock.lock();
     try {
-      if (keyStore.containsAlias(name) || cache.containsKey(name)) {
+      try {
-        throw new IOException("Key " + name + " already exists in " + this);
+        if (keyStore.containsAlias(name) || cache.containsKey(name)) {
+          throw new IOException("Key " + name + " already exists in " + this);
+        }
+      } catch (KeyStoreException e) {
+        throw new IOException("Problem looking up key " + name + " in " + this,
+            e);
       }
-    } catch (KeyStoreException e) {
+      Metadata meta = new Metadata(options.getCipher(), options.getBitLength(),
-      throw new IOException("Problem looking up key " + name + " in " + this,
+          new Date(), 1);
-          e);
+      if (options.getBitLength() != 8 * material.length) {
+        throw new IOException("Wrong key length. Required " +
+            options.getBitLength() + ", but got " + (8 * material.length));
+      }
+      cache.put(name, meta);
+      String versionName = buildVersionName(name, 0);
+      return innerSetKeyVersion(versionName, material, meta.getCipher());
+    } finally {
+      writeLock.unlock();
     }
-    Metadata meta = new Metadata(options.getCipher(), options.getBitLength(),
-        new Date(), 1);
-    if (options.getBitLength() != 8 * material.length) {
-      throw new IOException("Wrong key length. Required " +
-          options.getBitLength() + ", but got " + (8 * material.length));
-    }
-    cache.put(name, meta);
-    String versionName = buildVersionName(name, 0);
-    return innerSetKeyVersion(versionName, material, meta.getCipher());
   }
 
   @Override
   public void deleteKey(String name) throws IOException {
-    Metadata meta = getMetadata(name);
+    writeLock.lock();
-    if (meta == null) {
+    try {
-      throw new IOException("Key " + name + " does not exist in " + this);
+      Metadata meta = getMetadata(name);
-    }
+      if (meta == null) {
-    for(int v=0; v < meta.getVersions(); ++v) {
+        throw new IOException("Key " + name + " does not exist in " + this);
-      String versionName = buildVersionName(name, v);
+      }
+      for(int v=0; v < meta.getVersions(); ++v) {
+        String versionName = buildVersionName(name, v);
+        try {
+          if (keyStore.containsAlias(versionName)) {
+            keyStore.deleteEntry(versionName);
+          }
+        } catch (KeyStoreException e) {
+          throw new IOException("Problem removing " + versionName + " from " +
+              this, e);
+        }
+      }
       try {
-        if (keyStore.containsAlias(versionName)) {
+        if (keyStore.containsAlias(name)) {
-          keyStore.deleteEntry(versionName);
+          keyStore.deleteEntry(name);
         }
       } catch (KeyStoreException e) {
-        throw new IOException("Problem removing " + versionName + " from " +
+        throw new IOException("Problem removing " + name + " from " + this, e);
-            this, e);
       }
+      cache.remove(name);
+      changed = true;
+    } finally {
+      writeLock.unlock();
     }
-    try {
-      if (keyStore.containsAlias(name)) {
-        keyStore.deleteEntry(name);
-      }
-    } catch (KeyStoreException e) {
-      throw new IOException("Problem removing " + name + " from " + this, e);
-    }
-    cache.remove(name);
-    changed = true;
   }
 
   KeyVersion innerSetKeyVersion(String versionName, byte[] material,
@@ -257,47 +295,57 @@ KeyVersion innerSetKeyVersion(String versionName, byte[] material,
   @Override
   public KeyVersion rollNewVersion(String name,
                                     byte[] material) throws IOException {
-    Metadata meta = getMetadata(name);
+    writeLock.lock();
-    if (meta == null) {
+    try {
-      throw new IOException("Key " + name + " not found");
+      Metadata meta = getMetadata(name);
+      if (meta == null) {
+        throw new IOException("Key " + name + " not found");
+      }
+      if (meta.getBitLength() != 8 * material.length) {
+        throw new IOException("Wrong key length. Required " +
+            meta.getBitLength() + ", but got " + (8 * material.length));
+      }
+      int nextVersion = meta.addVersion();
+      String versionName = buildVersionName(name, nextVersion);
+      return innerSetKeyVersion(versionName, material, meta.getCipher());
+    } finally {
+      writeLock.unlock();
     }
-    if (meta.getBitLength() != 8 * material.length) {
-      throw new IOException("Wrong key length. Required " +
-          meta.getBitLength() + ", but got " + (8 * material.length));
-    }
-    int nextVersion = meta.addVersion();
-    String versionName = buildVersionName(name, nextVersion);
-    return innerSetKeyVersion(versionName, material, meta.getCipher());
   }
 
   @Override
   public void flush() throws IOException {
-    if (!changed) {
+    writeLock.lock();
-      return;
-    }
-    // put all of the updates into the keystore
-    for(Map.Entry<String, Metadata> entry: cache.entrySet()) {
-      try {
-        keyStore.setKeyEntry(entry.getKey(), new KeyMetadata(entry.getValue()),
-            password, null);
-      } catch (KeyStoreException e) {
-        throw new IOException("Can't set metadata key " + entry.getKey(),e );
-      }
-    }
-    // write out the keystore
-    FSDataOutputStream out = FileSystem.create(fs, path, permissions);
     try {
-      keyStore.store(out, password);
+      if (!changed) {
-    } catch (KeyStoreException e) {
+        return;
-      throw new IOException("Can't store keystore " + this, e);
+      }
-    } catch (NoSuchAlgorithmException e) {
+      // put all of the updates into the keystore
-      throw new IOException("No such algorithm storing keystore " + this, e);
+      for(Map.Entry<String, Metadata> entry: cache.entrySet()) {
-    } catch (CertificateException e) {
+        try {
-      throw new IOException("Certificate exception storing keystore " + this,
+          keyStore.setKeyEntry(entry.getKey(), new KeyMetadata(entry.getValue()),
-          e);
+              password, null);
+        } catch (KeyStoreException e) {
+          throw new IOException("Can't set metadata key " + entry.getKey(),e );
+        }
+      }
+      // write out the keystore
+      FSDataOutputStream out = FileSystem.create(fs, path, permissions);
+      try {
+        keyStore.store(out, password);
+      } catch (KeyStoreException e) {
+        throw new IOException("Can't store keystore " + this, e);
+      } catch (NoSuchAlgorithmException e) {
+        throw new IOException("No such algorithm storing keystore " + this, e);
+      } catch (CertificateException e) {
+        throw new IOException("Certificate exception storing keystore " + this,
+            e);
+      }
+      out.close();
+      changed = false;
+    } finally {
+      writeLock.unlock();
     }
-    out.close();
-    changed = false;
   }
 
   @Override

hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/crypto/key/KeyProvider.java

@@ -39,6 +39,8 @@
  * abstraction to separate key storage from users of encryption. It
  * is intended to support getting or storing keys in a variety of ways,
  * including third party bindings.
+ * <P/>
+ * <code>KeyProvider</code> implementations must be thread safe.
  */
 @InterfaceAudience.Public
 @InterfaceStability.Unstable

hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/crypto/key/UserProvider.java

@@ -55,7 +55,7 @@ public boolean isTransient() {
   }
 
   @Override
-  public KeyVersion getKeyVersion(String versionName) {
+  public synchronized KeyVersion getKeyVersion(String versionName) {
     byte[] bytes = credentials.getSecretKey(new Text(versionName));
     if (bytes == null) {
       return null;
@@ -64,7 +64,7 @@ public KeyVersion getKeyVersion(String versionName) {
   }
 
   @Override
-  public Metadata getMetadata(String name) throws IOException {
+  public synchronized Metadata getMetadata(String name) throws IOException {
     if (cache.containsKey(name)) {
       return cache.get(name);
     }
@@ -78,7 +78,7 @@ public Metadata getMetadata(String name) throws IOException {
   }
 
   @Override
-  public KeyVersion createKey(String name, byte[] material,
+  public synchronized KeyVersion createKey(String name, byte[] material,
                                Options options) throws IOException {
     Text nameT = new Text(name);
     if (credentials.getSecretKey(nameT) != null) {
@@ -98,7 +98,7 @@ public KeyVersion createKey(String name, byte[] material,
   }
 
   @Override
-  public void deleteKey(String name) throws IOException {
+  public synchronized void deleteKey(String name) throws IOException {
     Metadata meta = getMetadata(name);
     if (meta == null) {
       throw new IOException("Key " + name + " does not exist in " + this);
@@ -111,7 +111,7 @@ public void deleteKey(String name) throws IOException {
   }
 
   @Override
-  public KeyVersion rollNewVersion(String name,
+  public synchronized KeyVersion rollNewVersion(String name,
                                     byte[] material) throws IOException {
     Metadata meta = getMetadata(name);
     if (meta == null) {
@@ -134,7 +134,7 @@ public String toString() {
   }
 
   @Override
-  public void flush() {
+  public synchronized void flush() {
     user.addCredentials(credentials);
   }
 
@@ -151,7 +151,7 @@ public KeyProvider createProvider(URI providerName,
   }
 
   @Override
-  public List<String> getKeys() throws IOException {
+  public synchronized List<String> getKeys() throws IOException {
     List<String> list = new ArrayList<String>();
     List<Text> keys = credentials.getAllSecretKeys();
     for (Text key : keys) {
@@ -163,7 +163,7 @@ public List<String> getKeys() throws IOException {
   }
 
   @Override
-  public List<KeyVersion> getKeyVersions(String name) throws IOException {
+  public synchronized List<KeyVersion> getKeyVersions(String name) throws IOException {
       List<KeyVersion> list = new ArrayList<KeyVersion>();
       Metadata km = getMetadata(name);
       if (km != null) {