-
Notifications
You must be signed in to change notification settings - Fork 594
HDDS-14574. Enforce 700 permissions on Ozone Metadata and Data(hdds) directories #9735
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Conversation
|
@ChenSammi , @sumitagrawl and @jojochuang Please review the patch. |
sreejasahithi
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @Gargi-jais11 for working on this, left few comments
| @@ -705,7 +713,7 @@ | |||
| </property> | |||
| <property> | |||
| <name>ozone.metadata.dirs.permissions</name> | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could we add some test coverage for the permission change of metadata dir as well as for the db.
| /** | ||
| * Sets permissions on the storage directory (e.g., hdds subdirectory). | ||
| */ | ||
| private void setStorageDirPermissions() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This method is called by initializeImpl() for DbVolume (and HddsVolume), but it always uses
HDDS_DATANODE_DATA_DIR_PERMISSIONS. This means DbVolume directories get data directory permissions. is this intentional?
| Path path = dir.toPath(); | ||
| Files.setPosixFilePermissions(path, | ||
| PosixFilePermissions.fromString(symbolicPermission)); | ||
| LOG.debug("Set permissions {} on directory {}", symbolicPermission, dir); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| LOG.debug("Set permissions {} on directory {}", symbolicPermission, dir); | |
| LOG.debug("Set permissions {} on directory {} using config key {}", | |
| symbolicPermission, dir, permissionConfigKey); |
| * @throws RuntimeException If setting permissions fails | ||
| */ | ||
| public static void setDataDirectoryPermissions(File dir, ConfigurationSource conf, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This method does not throw exception as we are logging the warning.
What changes were proposed in this pull request?
Current Behaviour:
For Ozone metadata of OM, SCM, DN and Recon and Datanode Directory(/data/hdds) have 750 and 755 permissions respectively.
Proposed Behaviour:
We should bring Ozone up to parity with HDFS, where we have both a parameter that controls the permission, as well as health alerts for lax permissions.
Incorrectly permissioned data directories can lead to a serious data breach as any user (e.g. any Spark application) is able to read the data files.
Make the default permissions for all ozone metadata and data directories as 700 similar to hdfs.
Added new config for data directory permission:
hdds.datanode.data.dir.permissionswith default value of 700 and changed ozone metadata directory permissions to 700 from 750.What is the link to the Apache JIRA
https://issues.apache.org/jira/browse/HDDS-14574
How was this patch tested?
Added unit tests. Also manually tested for permissions: