[ 
https://issues.apache.org/jira/browse/YARN-11738?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=17900221#comment-17900221
 ] 

ASF GitHub Bot commented on YARN-11738:
---------------------------------------

szetszwo commented on code in PR #7144:
URL: https://github.com/apache/hadoop/pull/7144#discussion_r1853159801


##########
hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/fs/CommonConfigurationKeysPublic.java:
##########
@@ -1005,6 +1005,22 @@ public class CommonConfigurationKeysPublic {
   public static final String  HADOOP_SECURITY_CREDENTIAL_PASSWORD_FILE_KEY =
       "hadoop.security.credstore.java-keystore-provider.password-file";
 
+  /**
+   * @see
+   * <a 
href="{@docRoot}/../hadoop-project-dist/hadoop-common/core-default.xml">
+   * core-default.xml</a>
+   */
+  public static final String HMAC_ALGORITHM = "hadoop.security.hmac-algorithm";
+  public static final String DEFAULT_HMAC_ALGORITHM = "HmacSHA1";

Review Comment:
   Let's add "secret-manager.key-generator.algorithm" (we may use a non-hmac 
algorithm).
   - Also, we should use the existing naming convention.
   - The javadoc is not useful.  Let's skip it.
   ```java
     public static final String 
HADOOP_SECURITY_SECRET_MANAGER_KEY_GENERATOR_ALGORITHM_KEY
         = "hadoop.security.secret-manager.key-generator.algorithm";
     public static final String 
HADOOP_SECURITY_SECRET_MANAGER_KEY_GENERATOR_ALGORITHM_DEFAULT
         = "HmacSHA1";
   ```



##########
hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/security/token/SecretManager.java:
##########
@@ -140,11 +153,10 @@ protected Mac initialValue() {
   private final KeyGenerator keyGen;
   {
     try {
-      keyGen = KeyGenerator.getInstance(DEFAULT_HMAC_ALGORITHM);
-      keyGen.init(KEY_LENGTH);
+      keyGen = KeyGenerator.getInstance(SELECTED_ALGORITHM);
+      keyGen.init(SELECTED_LENGTH);
     } catch (NoSuchAlgorithmException nsa) {
-      throw new IllegalArgumentException("Can't find " + 
DEFAULT_HMAC_ALGORITHM +
-      " algorithm.");
+      throw new IllegalArgumentException("Can't find " + SELECTED_ALGORITHM + 
" algorithm.");

Review Comment:
   Let's include the cause:
   ```java
         throw new IllegalArgumentException("Failed to get " + HMAC_ALGORITHM, 
nsa);
   ```



##########
hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/security/token/SecretManager.java:
##########
@@ -107,16 +114,23 @@ public byte[] retriableRetrievePassword(T identifier)
   public void checkAvailableForRead() throws StandbyException {
     // Default to being available for read.
   }
-  
-  /**
-   * The name of the hashing algorithm.
-   */
-  private static final String DEFAULT_HMAC_ALGORITHM = "HmacSHA1";
 
-  /**
-   * The length of the random keys to use.
-   */
-  private static final int KEY_LENGTH = 64;
+  private static final String SELECTED_ALGORITHM;
+  private static final int SELECTED_LENGTH;

Review Comment:
   Let use `HMAC_ALGORITHM` and `KEY_LENGTH`.



##########
hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/fs/CommonConfigurationKeysPublic.java:
##########
@@ -1005,6 +1005,22 @@ public class CommonConfigurationKeysPublic {
   public static final String  HADOOP_SECURITY_CREDENTIAL_PASSWORD_FILE_KEY =
       "hadoop.security.credstore.java-keystore-provider.password-file";
 
+  /**
+   * @see
+   * <a 
href="{@docRoot}/../hadoop-project-dist/hadoop-common/core-default.xml">
+   * core-default.xml</a>
+   */
+  public static final String HMAC_ALGORITHM = "hadoop.security.hmac-algorithm";
+  public static final String DEFAULT_HMAC_ALGORITHM = "HmacSHA1";
+
+  /**
+   * @see
+   * <a 
href="{@docRoot}/../hadoop-project-dist/hadoop-common/core-default.xml">
+   * core-default.xml</a>
+   */
+  public static final String HMAC_LENGTH = "hadoop.security.hmac-length";
+  public static final int DEFAULT_HMAC_LENGTH = 64;

Review Comment:
   This should be "key-length".  It has nothing to do with hmac.
   ```java
     public static final String HADOOP_SECURITY_SECRET_MANAGER_KEY_LENGTH_KEY
         = "hadoop.security.secret-manager.key-length";
     public static final int HADOOP_SECURITY_SECRET_MANAGER_KEY_LENGTH_DEFAULT
         = 64;
   ```



##########
hadoop-common-project/hadoop-common/src/main/resources/core-default.xml:
##########
@@ -1046,6 +1046,32 @@
   </description>
 </property>
 
+<property>
+  <name>hadoop.security.hmac-algorithm</name>
+  <value>HmacSHA1</value>
+  <description>The configuration key specifying the hashing algorithm used for
+    HMAC (Hash-based Message Authentication Code) operations.
+
+    The HMAC algorithm is used in token management to compute secure
+    message digests. This configuration allows users to specify the
+    algorithm to be used for HMAC operations. The algorithm must be a
+    valid cryptographic hash algorithm supported by the Java Cryptography
+    Architecture (JCA). Common examples include "HmacSHA1", "HmacSHA256",
+    and "HmacSHA512".</description>
+</property>
+
+<property>
+  <name>hadoop.security.hmac-length</name>
+  <value>64</value>
+  <description>The configuration key specifying the key length for HMAC 
(Hash-based
+    Message Authentication Code) operations.
+
+    This property determines the size of the secret keys generated
+    for HMAC computations. The key length must be appropriate for the
+    selected HMAC algorithm. For example, longer keys are generally
+    more secure but may not be supported by all algorithms.</description>

Review Comment:
   Let's change this to: 
   
   > The configuration key specifying the key length of the generated secret 
keys 
   > in SecretManager. The key length must be appropriate for the algorithm.
   > For example, longer keys are generally more secure but may not be supported
   > by all algorithms.</description>
   



##########
hadoop-common-project/hadoop-common/src/main/resources/core-default.xml:
##########
@@ -1046,6 +1046,32 @@
   </description>
 </property>
 
+<property>
+  <name>hadoop.security.hmac-algorithm</name>
+  <value>HmacSHA1</value>
+  <description>The configuration key specifying the hashing algorithm used for
+    HMAC (Hash-based Message Authentication Code) operations.
+
+    The HMAC algorithm is used in token management to compute secure
+    message digests. This configuration allows users to specify the
+    algorithm to be used for HMAC operations. The algorithm must be a
+    valid cryptographic hash algorithm supported by the Java Cryptography
+    Architecture (JCA). Common examples include "HmacSHA1", "HmacSHA256",
+    and "HmacSHA512".</description>

Review Comment:
   Let's update the description as below:
   > The configuration key specifying the KeyGenerator algorithm used in 
SecretManager
   > for generating secret keys.  The algorithm must be a KeyGenerator 
algorithm supported by 
   > the Java Cryptography Architecture (JCA). Common examples include 
"HmacSHA1", 
   > "HmacSHA256",  and "HmacSHA512".
    



##########
hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/security/token/SecretManager.java:
##########
@@ -126,10 +140,9 @@ public void checkAvailableForRead() throws 
StandbyException {
     @Override
     protected Mac initialValue() {
       try {
-        return Mac.getInstance(DEFAULT_HMAC_ALGORITHM);
+        return Mac.getInstance(SELECTED_ALGORITHM);
       } catch (NoSuchAlgorithmException nsa) {
-        throw new IllegalArgumentException("Can't find " + 
DEFAULT_HMAC_ALGORITHM +
-                                           " algorithm.");
+        throw new IllegalArgumentException("Can't find " + SELECTED_ALGORITHM 
+ " algorithm.");

Review Comment:
   Let's include the cause:
   ```java
         throw new IllegalArgumentException("Failed to get " + HMAC_ALGORITHM, 
nsa);
   ```





> Modernize SecretManager config
> ------------------------------
>
>                 Key: YARN-11738
>                 URL: https://issues.apache.org/jira/browse/YARN-11738
>             Project: Hadoop YARN
>          Issue Type: Improvement
>          Components: yarn
>    Affects Versions: 3.4.1
>            Reporter: Bence Kosztolnik
>            Assignee: Bence Kosztolnik
>            Priority: Major
>              Labels: pull-request-available
>
> FIPS-compliant HMAC-SHA1 algorithms require secret keys to be at least 112 
> bits long. 
> https://github.com/apache/hadoop/blob/98c2bc87b1445c533268c58d382ea4e4297303fd/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/security/token/SecretManager.java#L144
> Should be set to 128 to be FIPS compatible.



--
This message was sent by Atlassian Jira
(v8.20.10#820010)

---------------------------------------------------------------------
To unsubscribe, e-mail: yarn-issues-unsubscr...@hadoop.apache.org
For additional commands, e-mail: yarn-issues-h...@hadoop.apache.org

Reply via email to