adoroszlai commented on code in PR #9859:
URL: https://github.com/apache/ozone/pull/9859#discussion_r2876980846
##########
hadoop-hdds/common/src/main/java/org/apache/hadoop/ozone/OzoneConfigKeys.java:
##########
@@ -469,6 +469,10 @@ public final class OzoneConfigKeys {
"ozone.acl.enabled";
public static final boolean OZONE_ACL_ENABLED_DEFAULT =
false;
+ public static final String OZONE_AUTHORIZATION_ENABLED =
+ "ozone.authorization.enabled";
+ public static final boolean OZONE_AUTHORIZATION_ENABLED_DEFAULT =
+ true;
Review Comment:
nit: no need to wrap lines
##########
hadoop-hdds/common/src/main/resources/ozone-default.xml:
##########
@@ -2376,6 +2376,20 @@
<tag>OZONE, SECURITY, ACL</tag>
<description>Key to enable/disable ozone acls.</description>
</property>
+ <property>
+ <name>ozone.authorization.enabled</name>
+ <value>true</value>
+ <tag>OZONE, SECURITY, AUTHORIZATION</tag>
+ <description>
+ Master switch to enable/disable authorization checks in Ozone
+ (admin privilege checks and ACL checks).
+ This property only takes effect when ozone.security.enabled is true.
+ When true: admin privilege checks are always performed, and object
+ ACL checks are controlled by ozone.acl.enabled.
+ When false: no authorization checks are performed.
+ Default is true to align with HDFS's dfs.permissions.enabled behavior.
Review Comment:
nit: it defaults to `true` for backward compatibility, not as much due to
Hadoop's behavior
##########
hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/server/StorageContainerManager.java:
##########
@@ -1952,8 +1969,32 @@ private void checkAdminAccess(String op) throws
IOException {
checkAdminAccess(getRemoteUser(), false);
}
+ /**
+ * Check if admin privilege authorization should be enforced.
+ * This controls system-level admin operations (upgrades, decommission, etc.)
+ *
+ * @return true if admin authorization checks should be performed
+ */
+ public boolean isAdminAuthorizationEnabled() {
+ // ONLY IN TESTS: Allow authorization testing without Kerberos
+ if (testSecureScmFlag) {
+ return isAuthorizationEnabled;
+ }
+ return OzoneSecurityUtil.isAuthorizationEnabled(configuration);
+ }
+
+ @VisibleForTesting
+ public static void setTestSecureScmFlag(boolean flag) {
+ testSecureScmFlag = flag;
+ }
+
Review Comment:
I'd like to avoid such flags in SCM/OM. We can define a private config
property (not listed in `ozone-default.xml`, etc.) to use in tests, and let
`SecurityConfig`/`OzoneSecurityUtil` check for that.
##########
hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/server/StorageContainerManager.java:
##########
@@ -357,6 +368,12 @@ private StorageContainerManager(OzoneConfiguration conf,
scmHANodeDetails = SCMHANodeDetails.loadSCMHAConfig(conf,
scmStorageConfig);
configuration = conf;
+ this.isSecurityEnabled = OzoneSecurityUtil.isSecurityEnabled(conf);
+ this.isAuthorizationEnabled = conf.getBoolean(
+ OZONE_AUTHORIZATION_ENABLED,
+ OZONE_AUTHORIZATION_ENABLED_DEFAULT);
Review Comment:
No need for new member variables, use via `SecurityConfig` (set in
`initializeCertificateClient()`).
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/OzoneManager.java:
##########
@@ -426,6 +428,7 @@ public final class OzoneManager extends
ServiceRuntimeInfoImpl
new ObjectMapper().readerFor(OmMetricsInfo.class);
private static final int SHUTDOWN_HOOK_PRIORITY = 30;
private final File omMetaDir;
+ private boolean isAuthorizationEnabled;
Review Comment:
No need for new member, please use via `SecurityConfig`.
##########
hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/HddsDatanodeService.java:
##########
@@ -669,6 +669,11 @@ public boolean isStopped() {
*/
private void checkAdminPrivilege(String operation)
throws IOException {
+ // Skip check if authorization is disabled
+ if (!OzoneSecurityUtil.isAuthorizationEnabled(conf)) {
+ return;
+ }
+
Review Comment:
Please:
- update `SecurityConfig` to store `authorizationEnabled` and expose it via
`isAuthorizationEnabled()`
- use `SecurityConfig.isAuthorizationEnabled()` whereever `SecurityConfig`
is already set, e.g. in `HddsDatanodeService`
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/OzoneManager.java:
##########
@@ -2748,6 +2756,32 @@ public boolean checkAcls(ResourceType resType, StoreType
storeType,
return omMetadataReader.checkAcls(obj, context, throwIfPermissionDenied);
}
+ /**
+ * Check if admin privilege authorization should be enforced.
+ * This controls system-level admin operations (upgrades, decommission, etc.)
+ *
+ * @return true if admin authorization checks should be performed
+ */
+ public boolean isAdminAuthorizationEnabled() {
+ // ONLY IN TESTS: Allow authorization testing without Kerberos
+ if (testSecureOmFlag) {
+ return isAuthorizationEnabled; // Skip security check
+ }
+
+ // require full security + authorization
+ return OzoneSecurityUtil.isAuthorizationEnabled(configuration);
+ }
+
+ /**
+ * Check if object ACL checks should be enforced.
+ * This controls volume/bucket/key level permissions.
+ *
+ * @return true if object ACL checks should be performed
+ */
+ public boolean isObjectAclEnabled() {
+ return isAdminAuthorizationEnabled() && getAclsEnabled();
+ }
Review Comment:
Do we need to keep the current implementation of `getAclsEnabled()` (simply
returns `isAclEnabled`)? If not, can we change that to this implementation
keeping the original name, to reduce change? (In other words, let
`getAclsEnabled()` return `isAclEnabled && isAdminAuthorizationEnabled`.)
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
