[GitHub] [ozone] jojochuang commented on a diff in pull request #4682: HDDS-8573. Verify default setting for DN root dir to restrict non-admin access

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

jojochuang commented on code in PR #4682:
URL: https://github.com/apache/ozone/pull/4682#discussion_r1195628347


##########
hadoop-hdds/common/src/main/java/org/apache/hadoop/hdds/server/ServerUtils.java:
##########
@@ -178,12 +184,90 @@ public static File getDirectoryFromConfig(ConfigurationSource conf,
             dbDirPath + " specified in configuration setting " +
             key);
       }
+      try {
+        Path path = dbDirPath.toPath();
+        // Fetch the permissions for the respective component from the config
+        String permissionValue = getPermissions(key, conf);
+        String symbolicPermission = getSymbolicPermission(permissionValue);
+
+        // Set the permissions for the directory
+        Files.setPosixFilePermissions(path,
+            PosixFilePermissions.fromString(symbolicPermission));
+      } catch (Exception e) {
+        throw new RuntimeException("Failed to set directory permissions for " +
+            dbDirPath + ": " + e.getMessage(), e);
+      }
       return dbDirPath;
     }
 
     return null;
   }
 
+  /**
+   * Fetches the symbolic representation of the permission value.
+   *
+   * @param permissionValue the permission value (octal or symbolic)
+   * @return the symbolic representation of the permission value
+   */
+  private static String getSymbolicPermission(String permissionValue) {
+    if (isSymbolic(permissionValue)) {
+      // For symbolic representation, use it directly
+      return permissionValue;
+    } else {
+      // For octal representation, convert it to FsPermission object and then
+      // to symbolic representation
+      short octalPermission = Short.parseShort(permissionValue, 8);
+      FsPermission fsPermission = new FsPermission(octalPermission);
+      return fsPermission.toString();
+    }
+  }
+
+  /**
+   * Checks if the permission value is in symbolic representation.
+   *
+   * @param permissionValue the permission value to check
+   * @return true if the permission value is in symbolic representation,
+   * false otherwise
+   */
+  private static boolean isSymbolic(String permissionValue) {
+    return permissionValue.matches(".*[rwx].*");
+  }
+
+
+  /**
+   * Retrieves the permissions' configuration value for a given config key.
+   *
+   * @param key  The configuration key.
+   * @param conf The ConfigurationSource object containing the config
+   * @return The permissions' configuration value for the specified key.
+   * @throws IllegalArgumentException If the configuration value is not defined
+   */
+  public static String getPermissions(String key, ConfigurationSource conf) {
+    String configName = "";
+
+    // Assign the appropriate config name based on the KEY
+    if (key.equals(ReconConfigKeys.OZONE_RECON_DB_DIR)) {
+      configName = ReconConfigKeys.OZONE_RECON_DB_DIRS_PERMISSIONS;
+    } else if (key.equals(ScmConfigKeys.OZONE_SCM_DB_DIRS)) {
+      configName = ScmConfigKeys.OZONE_SCM_DB_DIRS_PERMISSIONS;
+    } else if (key.equals(OzoneConfigKeys.OZONE_OM_DB_DIRS)) {
+      configName = OzoneConfigKeys.OZONE_OM_DB_DIRS_PERMISSIONS;
+    } else {
+      // If the permissions are not defined for the config, we make it fall
+      // back to the default permissions for metadata files and directories
+      configName = OzoneConfigKeys.OZONE_METADATA_DIRS_PERMISSIONS;
+    }
+
+    String configValue = conf.get(configName);
+    if (configValue != null) {
+      return configValue;
+    }
+
+    throw new IllegalArgumentException(
+        "Invalid configuration value for key: " + key);

Review Comment:
   If configValue is null it means the value for the corresponding configuration key is not defined.



##########
hadoop-hdds/framework/src/test/java/org/apache/hadoop/hdds/server/TestServerUtils.java:
##########
@@ -18,25 +18,145 @@
 package org.apache.hadoop.hdds.server;
 
 import java.io.File;
+import java.io.IOException;
+import java.nio.file.Files;
+import java.nio.file.Path;
+import java.nio.file.attribute.PosixFilePermission;
+import java.nio.file.attribute.PosixFilePermissions;
+import java.util.Set;
 
 import org.apache.hadoop.hdds.HddsConfigKeys;
 import org.apache.hadoop.hdds.conf.OzoneConfiguration;
 import org.apache.hadoop.hdds.scm.ScmConfigKeys;
 import org.apache.hadoop.test.PathUtils;
 
 import org.apache.commons.io.FileUtils;
-import org.junit.jupiter.api.Test;
+import org.junit.Rule;
+import org.junit.Test;
+import org.junit.rules.TemporaryFolder;
 
-import static org.junit.jupiter.api.Assertions.assertEquals;
+import static org.junit.Assert.assertEquals;
+import static org.junit.Assert.assertTrue;
+import static org.junit.jupiter.api.Assertions.assertNotNull;
+import static org.junit.jupiter.api.Assertions.assertNull;
 import static org.junit.jupiter.api.Assertions.assertFalse;
 import static org.junit.jupiter.api.Assertions.assertThrows;
-import static org.junit.jupiter.api.Assertions.assertTrue;
+
 
 /**
  * Unit tests for {@link ServerUtils}.
  */
 public class TestServerUtils {
 
+  @Rule
+  public TemporaryFolder folder = new TemporaryFolder();
+
+  /**
+   * Test case for {@link ServerUtils#getPermissions}.
+   * Verifies the retrieval of permissions for different configs.
+   */
+  @Test
+  public void testGetPermissions() {
+    // Create an OzoneConfiguration object and set the permissions
+    // for different keys
+    OzoneConfiguration conf = new OzoneConfiguration();
+    conf.set("ozone.recon.db.dir.perm", "750");

Review Comment:
   Can you use the static variable OZONE_RECON_DB_DIRS_PERMISSIONS instead?
   Replace the string literals below with static variables in the blow too.



##########
hadoop-hdds/common/src/main/java/org/apache/hadoop/hdds/server/ServerUtils.java:
##########
@@ -178,12 +184,90 @@ public static File getDirectoryFromConfig(ConfigurationSource conf,
             dbDirPath + " specified in configuration setting " +
             key);
       }
+      try {

Review Comment:
   Can you update the javadoc of this method so that users of the method is aware it has side effects?



##########
hadoop-hdds/common/src/main/resources/ozone-default.xml:
##########
@@ -669,6 +669,15 @@
       production environments.
     </description>
   </property>
+  <property>
+    <name>ozone.om.db.dirs.permissions</name>
+    <value>750</value>
+    <description>
+      Permissions for the metadata directories for Ozone Manager The

Review Comment:
   ```suggestion
         Permissions for the metadata directories for Ozone Manager. The
   ```



-- 
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: issues-unsubscribe@ozone.apache.org

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


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