[GitHub] [hadoop] steveloughran commented on a diff in pull request #5352: HADOOP-18618 : Support custom property for credential provider.

["steveloughran (via GitHub)" <gi...@apache.org>]

steveloughran commented on code in PR #5352:
URL: https://github.com/apache/hadoop/pull/5352#discussion_r1097271398


##########
hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/conf/Configuration.java:
##########
@@ -2405,6 +2405,30 @@ public char[] getPassword(String name) throws IOException {
     return pass;
   }
 
+  /**
+   * Get the value for a known password configuration element.
+   * In order to enable the elimination of clear text passwords in config,
+   * this method attempts to resolve the property name as an alias through
+   * the CredentialProvider API and conditionally fallsback to config. This
+   * method accept external provider property name.
+   * @param name property name
+   * @param providerKey provider property name
+   * @return password
+   * @throws IOException when error in fetching password
+   */
+  public char[] getPassword(String name, String providerKey)

Review Comment:
   this needs a better name to make clear that arg2 is a key, such as `getPasswordFromCredentialsOrConfig()`



##########
hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/security/alias/TestCredentialProviderFactory.java:
##########
@@ -258,6 +259,51 @@ public void testLocalBCFKSProvider() {
     assertEquals("Can't create keystore", exception.getMessage());
   }
 
+  @Test
+  public void testCustomKeyProviderProperty() throws Exception {
+    Configuration conf = new Configuration();
+    final String DEFAULT_CREDENTIAL_KEY = "default.credential.key";
+    final char[] DEFAULT_CREDENTIAL_PASSWORD = { 'p', 'a', 's', 's', 'w', 'o',
+        'r', 'd', '1', '2', '3' };
+
+    final String CUSTOM_CREDENTIAL_PROVIDER_KEY =
+        "fs.cloud.storage.account.key.provider.path";
+    final String CUSTOM_CREDENTIAL_KEY = "custom.credential.key";
+    final char[] CUSTOM_CREDENTIAL_PASSWORD = { 'c', 'u', 's', 't', 'o', 'm', '.',
+        'p', 'a', 's', 's', 'w', 'o', 'r', 'd' };
+
+    // Set provider in default credential path property
+    createCredentialProviderPath(conf, "default.jks",
+        CredentialProviderFactory.CREDENTIAL_PROVIDER_PATH,
+        DEFAULT_CREDENTIAL_KEY, DEFAULT_CREDENTIAL_PASSWORD);
+    // Set provider in custom credential path property
+    createCredentialProviderPath(conf, "custom.jks",
+        CUSTOM_CREDENTIAL_PROVIDER_KEY, CUSTOM_CREDENTIAL_KEY,
+        CUSTOM_CREDENTIAL_PASSWORD);
+
+    assertTrue("Password should match for default provider path", Arrays.equals(
+        conf.getPassword(DEFAULT_CREDENTIAL_KEY), DEFAULT_CREDENTIAL_PASSWORD));
+
+    assertTrue("Password should match for custom provider path", Arrays.equals(

Review Comment:
   *never* use assertTrue for asserting on equality as the error messages are inadequate.
   
   Use assertJ for new code, assertEquals(expected, actual) for old stuff



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: common-issues-unsubscribe@hadoop.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: common-issues-unsubscribe@hadoop.apache.org
For additional commands, e-mail: common-issues-help@hadoop.apache.org