HADOOP-13157. Follow-on improvements to hadoop credential commands. Contributed by Mike Yoder.

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

@@ -19,7 +19,6 @@
 package org.apache.hadoop.crypto.key;
 
 import com.google.common.base.Preconditions;
-import org.apache.commons.io.IOUtils;
 import org.apache.hadoop.classification.InterfaceAudience;
 import org.apache.hadoop.conf.Configuration;
 import org.apache.hadoop.fs.FSDataInputStream;
@@ -38,12 +37,10 @@ import com.google.common.annotations.VisibleForTesting;
 import javax.crypto.spec.SecretKeySpec;
 
 import java.io.IOException;
-import java.io.InputStream;
 import java.io.ObjectInputStream;
 import java.io.ObjectOutputStream;
 import java.io.Serializable;
 import java.net.URI;
-import java.net.URL;
 import java.security.GeneralSecurityException;
 import java.security.Key;
 import java.security.KeyStore;
@@ -138,44 +135,15 @@ public class JavaKeyStoreProvider extends KeyProvider {
     writeLock = lock.writeLock();
   }
 
-  /**
-   * The password is either found in the environment or in a file. This
-   * routine implements the logic for locating the password in these
-   * locations.
-   * @return The password as a char []; null if not found.
-   * @throws IOException
-   */
-  private char[] locatePassword() throws IOException {
-    char[] pass = null;
-    // Get the password file from the conf, if not present from the user's
-    // environment var
-    if (System.getenv().containsKey(KEYSTORE_PASSWORD_ENV_VAR)) {
-      pass = System.getenv(KEYSTORE_PASSWORD_ENV_VAR).toCharArray();
-    }
-    if (pass == null) {
-      String pwFile = getConf().get(KEYSTORE_PASSWORD_FILE_KEY);
-      if (pwFile != null) {
-        ClassLoader cl = Thread.currentThread().getContextClassLoader();
-        URL pwdFile = cl.getResource(pwFile);
-        if (pwdFile == null) {
-          // Provided Password file does not exist
-          throw new IOException("Password file does not exists");
-        }
-        try (InputStream is = pwdFile.openStream()) {
-          pass = IOUtils.toString(is).trim().toCharArray();
-        }
-      }
-    }
-    return pass;
-  }
-
   /**
    * Open up and initialize the keyStore.
-   * @throws IOException
+   * @throws IOException If there is a problem reading the password file
+   * or a problem reading the keystore.
    */
   private void locateKeystore() throws IOException {
     try {
-      password = locatePassword();
+      password = ProviderUtils.locatePassword(KEYSTORE_PASSWORD_ENV_VAR,
+          getConf().get(KEYSTORE_PASSWORD_FILE_KEY));
       if (password == null) {
         password = KEYSTORE_PASSWORD_DEFAULT;
       }
@@ -331,48 +299,31 @@ public class JavaKeyStoreProvider extends KeyProvider {
     }
   }
 
-  private Path constructNewPath(Path path) {
+  private static Path constructNewPath(Path path) {
-    Path newPath = new Path(path.toString() + "_NEW");
+    return new Path(path.toString() + "_NEW");
-    return newPath;
   }
 
-  private Path constructOldPath(Path path) {
+  private static Path constructOldPath(Path path) {
-    Path oldPath = new Path(path.toString() + "_OLD");
+    return new Path(path.toString() + "_OLD");
-    return oldPath;
   }
 
   @Override
   public boolean needsPassword() throws IOException {
-    return (null == locatePassword());
+    return (null == ProviderUtils.locatePassword(KEYSTORE_PASSWORD_ENV_VAR,
-  }
+        getConf().get(KEYSTORE_PASSWORD_FILE_KEY)));
 
-  @VisibleForTesting
+  }
-  public static final String NO_PASSWORD_WARN =
-      "WARNING: You have accepted the use of the default provider password\n" +
-      "by not configuring a password in one of the two following locations:\n";
-  public static final String NO_PASSWORD_ERROR =
-      "ERROR: The provider cannot find a password in the expected " +
-          "locations.\nPlease supply a password using one of the " +
-          "following two mechanisms:\n";
-  @VisibleForTesting public static final String NO_PASSWORD_INSTRUCTIONS =
-          "    o In the environment variable " +
-          KEYSTORE_PASSWORD_ENV_VAR + "\n" +
-          "    o In a file referred to by the configuration entry\n" +
-          "      " + KEYSTORE_PASSWORD_FILE_KEY + ".\n" +
-          "Please review the documentation regarding provider passwords at\n" +
-          "http://hadoop.apache.org/docs/current/hadoop-project-dist/" +
-              "hadoop-common/CredentialProviderAPI.html#Keystore_Passwords\n";
-  @VisibleForTesting public static final String NO_PASSWORD_CONT =
-      "Continuing with the default provider password.\n";
 
   @Override
   public String noPasswordWarning() {
-    return NO_PASSWORD_WARN + NO_PASSWORD_INSTRUCTIONS + NO_PASSWORD_CONT;
+    return ProviderUtils.noPasswordWarning(KEYSTORE_PASSWORD_ENV_VAR,
+        KEYSTORE_PASSWORD_FILE_KEY);
   }
 
   @Override
   public String noPasswordError() {
-    return NO_PASSWORD_ERROR + NO_PASSWORD_INSTRUCTIONS;
+    return ProviderUtils.noPasswordError(KEYSTORE_PASSWORD_ENV_VAR,
+        KEYSTORE_PASSWORD_FILE_KEY);
   }
 
   @Override

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

@@ -47,7 +47,8 @@ public class KeyShell extends Configured implements Tool {
       "   [" + DeleteCommand.USAGE + "]\n" +
       "   [" + ListCommand.USAGE + "]\n";
   private static final String LIST_METADATA = "keyShell.list.metadata";
-  @VisibleForTesting public static final String NO_VALID_PROVIDERS =
+  @VisibleForTesting
+  public static final String NO_VALID_PROVIDERS =
       "There are no valid (non-transient) providers configured.\n" +
       "No action has been taken. Use the -provider option to specify\n" +
       "a provider. If you want to use a transient provider then you\n" +
@@ -60,9 +61,11 @@ public class KeyShell extends Configured implements Tool {
   private boolean strict = false;
 
   /** allows stdout to be captured if necessary. */
-  @VisibleForTesting public PrintStream out = System.out;
+  @VisibleForTesting
+  public PrintStream out = System.out;
   /** allows stderr to be captured if necessary. */
-  @VisibleForTesting public PrintStream err = System.err;
+  @VisibleForTesting
+  public PrintStream err = System.err;
 
   private boolean userSuppliedProvider = false;
 

hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/security/ProviderUtils.java

@@ -19,9 +19,13 @@
 package org.apache.hadoop.security;
 
 import java.io.IOException;
+import java.io.InputStream;
 import java.net.URI;
 import java.net.URISyntaxException;
+import java.net.URL;
 
+import com.google.common.annotations.VisibleForTesting;
+import org.apache.commons.io.IOUtils;
 import org.apache.commons.logging.Log;
 import org.apache.commons.logging.LogFactory;
 import org.apache.hadoop.conf.Configuration;
@@ -36,6 +40,23 @@ import org.apache.hadoop.security.alias.LocalJavaKeyStoreProvider;
  *
  */
 public final class ProviderUtils {
+  @VisibleForTesting
+  public static final String NO_PASSWORD_WARN =
+      "WARNING: You have accepted the use of the default provider password\n" +
+      "by not configuring a password in one of the two following locations:\n";
+  @VisibleForTesting
+  public static final String NO_PASSWORD_ERROR =
+      "ERROR: The provider cannot find a password in the expected " +
+      "locations.\nPlease supply a password using one of the " +
+      "following two mechanisms:\n";
+  @VisibleForTesting
+  public static final String NO_PASSWORD_CONT =
+      "Continuing with the default provider password.\n";
+  @VisibleForTesting
+  public static final String NO_PASSWORD_INSTRUCTIONS_DOC =
+      "Please review the documentation regarding provider passwords in\n" +
+      "the keystore passwords section of the Credential Provider API\n";
+
   private static final Log LOG = LogFactory.getLog(ProviderUtils.class);
 
   /**
@@ -174,4 +195,58 @@ public final class ProviderUtils {
     }
     return conf;
   }
+
+  /**
+   * The password is either found in the environment or in a file. This
+   * routine implements the logic for locating the password in these
+   * locations.
+   *
+   * @param envWithPass  The name of the environment variable that might
+   *                     contain the password. Must not be null.
+   * @param fileWithPass The name of a file that could contain the password.
+   *                     Can be null.
+   * @return The password as a char []; null if not found.
+   * @throws IOException If fileWithPass is non-null and points to a
+   * nonexistent file or a file that fails to open and be read properly.
+   */
+  public static char[] locatePassword(String envWithPass, String fileWithPass)
+      throws IOException {
+    char[] pass = null;
+    // Get the password file from the conf, if not present from the user's
+    // environment var
+    if (System.getenv().containsKey(envWithPass)) {
+      pass = System.getenv(envWithPass).toCharArray();
+    }
+    if (pass == null) {
+      if (fileWithPass != null) {
+        ClassLoader cl = Thread.currentThread().getContextClassLoader();
+        URL pwdFile = cl.getResource(fileWithPass);
+        if (pwdFile == null) {
+          // Provided Password file does not exist
+          throw new IOException("Password file does not exist");
+        }
+        try (InputStream is = pwdFile.openStream()) {
+          pass = IOUtils.toString(is).trim().toCharArray();
+        }
+      }
+    }
+    return pass;
+  }
+
+  private static String noPasswordInstruction(String envKey, String fileKey) {
+    return
+        "    * In the environment variable " + envKey + "\n" +
+        "    * In a file referred to by the configuration entry\n" +
+        "      " + fileKey + ".\n" +
+        NO_PASSWORD_INSTRUCTIONS_DOC;
+  }
+
+  public static String noPasswordWarning(String envKey, String fileKey) {
+    return NO_PASSWORD_WARN + noPasswordInstruction(envKey, fileKey) +
+        NO_PASSWORD_CONT;
+  }
+
+  public static String noPasswordError(String envKey, String fileKey) {
+    return NO_PASSWORD_ERROR + noPasswordInstruction(envKey, fileKey);
+  }
 }

hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/security/alias/AbstractJavaKeyStoreProvider.java

@@ -18,10 +18,8 @@
 
 package org.apache.hadoop.security.alias;
 
-import com.google.common.annotations.VisibleForTesting;
 import org.apache.commons.logging.Log;
 import org.apache.commons.logging.LogFactory;
-import org.apache.commons.io.IOUtils;
 import org.apache.hadoop.classification.InterfaceAudience;
 import org.apache.hadoop.conf.Configuration;
 import org.apache.hadoop.fs.Path;
@@ -34,7 +32,6 @@ import java.io.IOException;
 import java.io.InputStream;
 import java.io.OutputStream;
 import java.net.URI;
-import java.net.URL;
 import java.security.GeneralSecurityException;
 import java.security.KeyStore;
 import java.security.KeyStoreException;
@@ -64,11 +61,11 @@ import java.util.concurrent.locks.ReentrantReadWriteLock;
 public abstract class AbstractJavaKeyStoreProvider extends CredentialProvider {
   public static final Log LOG = LogFactory.getLog(
       AbstractJavaKeyStoreProvider.class);
-  public static final String CREDENTIAL_PASSWORD_NAME =
+  public static final String CREDENTIAL_PASSWORD_ENV_VAR =
       "HADOOP_CREDSTORE_PASSWORD";
-  public static final String KEYSTORE_PASSWORD_FILE_KEY =
+  public static final String CREDENTIAL_PASSWORD_FILE_KEY =
       "hadoop.security.credstore.java-keystore-provider.password-file";
-  public static final String KEYSTORE_PASSWORD_DEFAULT = "none";
+  public static final String CREDENTIAL_PASSWORD_DEFAULT = "none";
 
   private Path path;
   private final URI uri;
@@ -302,45 +299,18 @@ public abstract class AbstractJavaKeyStoreProvider extends CredentialProvider {
     }
   }
 
-  /**
-   * The password is either found in the environment or in a file. This
-   * routine implements the logic for locating the password in these
-   * locations.
-   *
-   * @return The password as a char []; null if not found.
-   * @throws IOException
-   */
-  private char[] locatePassword() throws IOException {
-    char[] pass = null;
-    if (System.getenv().containsKey(CREDENTIAL_PASSWORD_NAME)) {
-      pass = System.getenv(CREDENTIAL_PASSWORD_NAME).toCharArray();
-    }
-    // if not in ENV get check for file
-    if (pass == null) {
-      String pwFile = conf.get(KEYSTORE_PASSWORD_FILE_KEY);
-      if (pwFile != null) {
-        ClassLoader cl = Thread.currentThread().getContextClassLoader();
-        URL pwdFile = cl.getResource(pwFile);
-        if (pwdFile != null) {
-          try (InputStream is = pwdFile.openStream()) {
-            pass = IOUtils.toString(is).trim().toCharArray();
-          }
-        }
-      }
-    }
-    return pass;
-  }
-
   /**
    * Open up and initialize the keyStore.
    *
-   * @throws IOException
+   * @throws IOException If there is a problem reading the password file
+   * or a problem reading the keystore.
    */
   private void locateKeystore() throws IOException {
     try {
-      password = locatePassword();
+      password = ProviderUtils.locatePassword(CREDENTIAL_PASSWORD_ENV_VAR,
+          conf.get(CREDENTIAL_PASSWORD_FILE_KEY));
       if (password == null) {
-        password = KEYSTORE_PASSWORD_DEFAULT.toCharArray();
+        password = CREDENTIAL_PASSWORD_DEFAULT.toCharArray();
       }
       KeyStore ks;
       ks = KeyStore.getInstance("jceks");
@@ -364,38 +334,21 @@ public abstract class AbstractJavaKeyStoreProvider extends CredentialProvider {
 
   @Override
   public boolean needsPassword() throws IOException {
-    return (null == locatePassword());
+    return (null == ProviderUtils.locatePassword(CREDENTIAL_PASSWORD_ENV_VAR,
-  }
+        conf.get(CREDENTIAL_PASSWORD_FILE_KEY)));
 
-  @VisibleForTesting
+  }
-  public static final String NO_PASSWORD_WARN =
-      "WARNING: You have accepted the use of the default provider password\n" +
-      "by not configuring a password in one of the two following locations:\n";
-  @VisibleForTesting
-  public static final String NO_PASSWORD_ERROR =
-      "ERROR: The provider cannot find a password in the expected " +
-          "locations.\nPlease supply a password using one of the " +
-          "following two mechanisms:\n";
-  @VisibleForTesting
-  public static final String NO_PASSWORD_INSTRUCTIONS =
-          "    o In the environment variable " +
-          CREDENTIAL_PASSWORD_NAME + "\n" +
-          "    o In a file referred to by the configuration entry\n" +
-          "      " + KEYSTORE_PASSWORD_FILE_KEY + ".\n" +
-          "Please review the documentation regarding provider passwords at\n" +
-          "http://hadoop.apache.org/docs/current/hadoop-project-dist/" +
-              "hadoop-common/CredentialProviderAPI.html#Keystore_Passwords\n";
-  @VisibleForTesting public static final String NO_PASSWORD_CONT =
-      "Continuing with the default provider password.\n";
 
   @Override
   public String noPasswordWarning() {
-    return NO_PASSWORD_WARN + NO_PASSWORD_INSTRUCTIONS + NO_PASSWORD_CONT;
+    return ProviderUtils.noPasswordWarning(CREDENTIAL_PASSWORD_ENV_VAR,
+            CREDENTIAL_PASSWORD_FILE_KEY);
   }
 
   @Override
   public String noPasswordError() {
-    return NO_PASSWORD_ERROR + NO_PASSWORD_INSTRUCTIONS;
+    return ProviderUtils.noPasswordError(CREDENTIAL_PASSWORD_ENV_VAR,
+        CREDENTIAL_PASSWORD_FILE_KEY);
   }
 
   @Override

hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/security/alias/CredentialShell.java

@@ -58,9 +58,11 @@ public class CredentialShell extends Configured implements Tool {
   private boolean strict = false;
 
   /** Allows stdout to be captured if necessary. */
-  @VisibleForTesting public PrintStream out = System.out;
+  @VisibleForTesting
+  public PrintStream out = System.out;
   /** Allows stderr to be captured if necessary. */
-  @VisibleForTesting public PrintStream err = System.err;
+  @VisibleForTesting
+  public PrintStream err = System.err;
 
   private boolean userSuppliedProvider = false;
   private String value = null;

hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/util/Shell.java

@@ -869,7 +869,7 @@ public abstract class Shell {
       // might be owned by a different user.  For example, the NodeManager
       // running a User's container.
       builder.environment().remove(
-          AbstractJavaKeyStoreProvider.CREDENTIAL_PASSWORD_NAME);
+          AbstractJavaKeyStoreProvider.CREDENTIAL_PASSWORD_ENV_VAR);
     }
 
     if (dir != null) {

hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/crypto/key/TestKeyShell.java

@@ -26,6 +26,7 @@ import java.util.UUID;
 import org.apache.hadoop.conf.Configuration;
 import org.apache.hadoop.fs.Path;
 import org.apache.hadoop.test.GenericTestUtils;
+import org.apache.hadoop.security.ProviderUtils;
 import org.junit.After;
 import org.junit.Before;
 import org.junit.Test;
@@ -116,11 +117,11 @@ public class TestKeyShell {
     assertTrue(outContent.toString().contains(keyName + " has been " +
             "successfully created"));
     assertTrue(outContent.toString()
-        .contains(JavaKeyStoreProvider.NO_PASSWORD_WARN));
+        .contains(ProviderUtils.NO_PASSWORD_WARN));
     assertTrue(outContent.toString()
-        .contains(JavaKeyStoreProvider.NO_PASSWORD_INSTRUCTIONS));
+        .contains(ProviderUtils.NO_PASSWORD_INSTRUCTIONS_DOC));
     assertTrue(outContent.toString()
-        .contains(JavaKeyStoreProvider.NO_PASSWORD_CONT));
+        .contains(ProviderUtils.NO_PASSWORD_CONT));
 
     String listOut = listKeys(ks, false);
     assertTrue(listOut.contains(keyName));
@@ -240,9 +241,9 @@ public class TestKeyShell {
     rc = ks.run(args1);
     assertEquals(1, rc);
     assertTrue(outContent.toString()
-        .contains(JavaKeyStoreProvider.NO_PASSWORD_ERROR));
+        .contains(ProviderUtils.NO_PASSWORD_ERROR));
     assertTrue(outContent.toString()
-        .contains(JavaKeyStoreProvider.NO_PASSWORD_INSTRUCTIONS));
+        .contains(ProviderUtils.NO_PASSWORD_INSTRUCTIONS_DOC));
   }
 
   @Test

hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/security/alias/TestCredShell.java

@@ -31,6 +31,7 @@ import java.util.List;
 import org.apache.hadoop.conf.Configuration;
 import org.apache.hadoop.fs.Path;
 import org.apache.hadoop.test.GenericTestUtils;
+import org.apache.hadoop.security.ProviderUtils;
 import org.junit.Before;
 import org.junit.Test;
 
@@ -64,11 +65,11 @@ public class TestCredShell {
     assertTrue(outContent.toString().contains("credential1 has been successfully " +
     		"created."));
     assertTrue(outContent.toString()
-        .contains(AbstractJavaKeyStoreProvider.NO_PASSWORD_WARN));
+        .contains(ProviderUtils.NO_PASSWORD_WARN));
     assertTrue(outContent.toString()
-        .contains(AbstractJavaKeyStoreProvider.NO_PASSWORD_INSTRUCTIONS));
+        .contains(ProviderUtils.NO_PASSWORD_INSTRUCTIONS_DOC));
     assertTrue(outContent.toString()
-        .contains(AbstractJavaKeyStoreProvider.NO_PASSWORD_CONT));
+        .contains(ProviderUtils.NO_PASSWORD_CONT));
 
     outContent.reset();
     String[] args2 = {"list", "-provider",
@@ -246,9 +247,9 @@ public class TestCredShell {
     assertFalse(outContent.toString().contains("credential1 has been " +
         "successfully created."));
     assertTrue(outContent.toString()
-        .contains(AbstractJavaKeyStoreProvider.NO_PASSWORD_ERROR));
+        .contains(ProviderUtils.NO_PASSWORD_ERROR));
     assertTrue(outContent.toString()
-        .contains(AbstractJavaKeyStoreProvider.NO_PASSWORD_INSTRUCTIONS));
+        .contains(ProviderUtils.NO_PASSWORD_INSTRUCTIONS_DOC));
   }
 
   @Test

hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/util/TestShell.java

@@ -164,7 +164,7 @@ public class TestShell extends Assert {
 
   private void testEnvHelper(boolean inheritParentEnv) throws Exception {
     Map<String, String> customEnv = Collections.singletonMap(
-        AbstractJavaKeyStoreProvider.CREDENTIAL_PASSWORD_NAME, "foo");
+        AbstractJavaKeyStoreProvider.CREDENTIAL_PASSWORD_ENV_VAR, "foo");
     Shell.ShellCommandExecutor command = new ShellCommandExecutor(
         new String[]{"env"}, null, customEnv, 0L,
         inheritParentEnv);
@@ -181,9 +181,9 @@ public class TestShell extends Assert {
       expectedEnv.putAll(customEnv);
     } else {
       assertFalse("child process environment should not have contained "
-              + AbstractJavaKeyStoreProvider.CREDENTIAL_PASSWORD_NAME,
+              + AbstractJavaKeyStoreProvider.CREDENTIAL_PASSWORD_ENV_VAR,
           vars.containsKey(
-              AbstractJavaKeyStoreProvider.CREDENTIAL_PASSWORD_NAME));
+              AbstractJavaKeyStoreProvider.CREDENTIAL_PASSWORD_ENV_VAR));
     }
     assertEquals(expectedEnv, vars);
   }