You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@ozone.apache.org by "ss77892 (via GitHub)" <gi...@apache.org> on 2026/05/21 19:57:54 UTC

[PR] HDDS-13855. Move ACL check in Volume requests to preExecute [ozone]

ss77892 opened a new pull request, #10328:
URL: https://github.com/apache/ozone/pull/10328

   ## Summary
   
   - Move ACL authorization checks for **volume** operations (`DeleteVolume`, `SetVolumeOwner`, `SetVolumeQuota`) from `validateAndUpdateCache` to `preExecute`
   - Move ACL checks for volume ACL operations (`AddAcl`, `RemoveAcl`, `SetAcl`) from `validateAndUpdateCache` to `preExecute`
   - Add audit logging for preExecute ACL failures so rejections are recorded even before the Ratis log entry is written
   
   ## Motivation
   
   When ACL enforcement happens inside `validateAndUpdateCache`, the request has already been written to the Ratis log on all OM peers. Moving the check to `preExecute` (which runs only on the leader, before log submission) prevents unauthorized requests from polluting the log and ensures consistent ACL rejection across HA leader changes.
   
   ## Test plan
   
   - [ ] Unit tests pass for volume request handlers
   - [ ] Integration test `TestOMHALeaderSpecificACLEnforcement` covers volume operations (in a follow-up PR)
   
   ## Related
   
   Part of HDDS-13855. See also:
   - Bucket requests (separate PR)
   - Key + Prefix requests (separate PR)
   
   Made with [Cursor](https://cursor.com)


-- 
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

Re: [PR] HDDS-13855. Move ACL check in Volume requests to preExecute [ozone]

Posted by "ivandika3 (via GitHub)" <gi...@apache.org>.
ivandika3 commented on code in PR #10328:
URL: https://github.com/apache/ozone/pull/10328#discussion_r3323588695


##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/volume/OMVolumeDeleteRequest.java:
##########
@@ -57,6 +60,36 @@ public OMVolumeDeleteRequest(OMRequest omRequest) {
     super(omRequest);
   }
 
+  @Override
+  public OMRequest preExecute(OzoneManager ozoneManager) throws IOException {
+    super.preExecute(ozoneManager);
+    DeleteVolumeRequest deleteVolumeRequest =
+        getOmRequest().getDeleteVolumeRequest();
+    Objects.requireNonNull(deleteVolumeRequest);
+    String volume = deleteVolumeRequest.getVolumeName();
+
+    // ACL check during preExecute
+    if (ozoneManager.getAclsEnabled()) {
+      try {
+        checkAcls(ozoneManager, OzoneObj.ResourceType.VOLUME,
+            OzoneObj.StoreType.OZONE, IAccessAuthorizer.ACLType.DELETE, volume,
+            null, null);
+      } catch (IOException ex) {
+        // Ensure audit log captures preExecute failures
+        Map<String, String> auditMap = new LinkedHashMap<>();
+        auditMap.put(OzoneConsts.VOLUME, volume);
+        markForAudit(ozoneManager.getAuditLogger(),
+            buildAuditMessage(OMAction.DELETE_VOLUME, auditMap, ex,
+                getOmRequest().getUserInfo()));
+        throw ex;
+      }
+    }
+
+    return getOmRequest().toBuilder()
+        .setUserInfo(getUserIfNotExists(ozoneManager))
+        .build();

Review Comment:
   If this is valid, we should also fix other OM requests that unnecessarily create OMRequest objects.



-- 
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

Re: [PR] HDDS-15462. Move ACL check in Volume requests to preExecute [ozone]

Posted by "ivandika3 (via GitHub)" <gi...@apache.org>.
ivandika3 commented on code in PR #10328:
URL: https://github.com/apache/ozone/pull/10328#discussion_r3345395025


##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/volume/acl/OMVolumeAclRequest.java:
##########
@@ -53,6 +55,43 @@ public abstract class OMVolumeAclRequest extends OMVolumeRequest {
     omVolumeAclOp = aclOp;
   }
 
+  @Override
+  public OzoneManagerProtocolProtos.OMRequest preExecute(OzoneManager ozoneManager)
+      throws IOException {
+    OzoneManagerProtocolProtos.OMRequest omRequest = super.preExecute(ozoneManager);

Review Comment:
   Raised https://issues.apache.org/jira/browse/HDDS-15467



-- 
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

Re: [PR] HDDS-15462. Move ACL check in Volume requests to preExecute [ozone]

Posted by "ivandika3 (via GitHub)" <gi...@apache.org>.
ivandika3 commented on code in PR #10328:
URL: https://github.com/apache/ozone/pull/10328#discussion_r3345464811


##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/volume/acl/OMVolumeAddAclRequest.java:
##########
@@ -58,14 +58,16 @@ public class OMVolumeAddAclRequest extends OMVolumeAclRequest {
 
   @Override
   public OMRequest preExecute(OzoneManager ozoneManager) throws IOException {
+    // Call parent preExecute to perform ACL check
+    OMRequest omRequest = super.preExecute(ozoneManager);
+
     long modificationTime = Time.now();
     OzoneManagerProtocolProtos.AddAclRequest.Builder addAclRequestBuilder =
-        getOmRequest().getAddAclRequest().toBuilder()
+        omRequest.getAddAclRequest().toBuilder()
             .setModificationTime(modificationTime);

Review Comment:
   In the future, we also might want to have `OMRequest` carries the modificationTime so that we are not rebuilding the OMRequest just to add a modificationTime.



-- 
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

Re: [PR] HDDS-13855. Move ACL check in Volume requests to preExecute [ozone]

Posted by "ivandika3 (via GitHub)" <gi...@apache.org>.
ivandika3 commented on code in PR #10328:
URL: https://github.com/apache/ozone/pull/10328#discussion_r3323523933


##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/volume/acl/OMVolumeAclRequest.java:
##########
@@ -53,6 +55,43 @@ public abstract class OMVolumeAclRequest extends OMVolumeRequest {
     omVolumeAclOp = aclOp;
   }
 
+  @Override
+  public OzoneManagerProtocolProtos.OMRequest preExecute(OzoneManager ozoneManager)
+      throws IOException {
+    OzoneManagerProtocolProtos.OMRequest omRequest = super.preExecute(ozoneManager);

Review Comment:
   Not directly related to this patch, but seems there are cases where `super.preExecute` is called and there are cases where `super.preExecute` is not called (e.g. `OMKeyDeleteRequest#preExecute`) which seems to be a code smell. We can check whether we need to call `super.preExecute` every time.
   
   Also found a possible security issue where `OmClientRequest#getUserInfoNotExists` might user an admin user (OM starter user) privilege if the client does not specify any user info. I don't think normal clients will gain admin user currently since both Hadoop RPC and gRPC clients should already have the user info. However, I think it's best to for `getUserInfoNotExists` to not fallback to the admin user since if we make any changes in `getUserInfo` that causes userInfo's remoteAddress and userInfo's username to not be set, it might cause cause privilege escalations.



-- 
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

Re: [PR] HDDS-13855. Move ACL check in Volume requests to preExecute [ozone]

Posted by "ivandika3 (via GitHub)" <gi...@apache.org>.
ivandika3 commented on code in PR #10328:
URL: https://github.com/apache/ozone/pull/10328#discussion_r3323523933


##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/volume/acl/OMVolumeAclRequest.java:
##########
@@ -53,6 +55,43 @@ public abstract class OMVolumeAclRequest extends OMVolumeRequest {
     omVolumeAclOp = aclOp;
   }
 
+  @Override
+  public OzoneManagerProtocolProtos.OMRequest preExecute(OzoneManager ozoneManager)
+      throws IOException {
+    OzoneManagerProtocolProtos.OMRequest omRequest = super.preExecute(ozoneManager);

Review Comment:
   Not directly related to this patch, but seems there are cases where `super.preExecute` is called and there are cases where `super.preExecute` is not called (e.g. `OMKeyDeleteRequest#preExecute`) which seems to be a code smell. We can check whether we need to call `super.preExecute` every time.
   
   Also found a possible security issue where `OmClientRequest#getUserInfoNotExists` might user an admin user (OM starter user) privilege if the client does not specify any user info. I don't think normal clients will gain admin user currently since both Hadoop RPC and gRPC clients should already have the user info. However, I think it's best to for `getUserInfoNotExists` to not fallback to the admin user since if we make any changes in `getUserInfo` that causes userInfo's remoteAddress and userInfo's username to not be used, it might cause cause security issues.



##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/volume/OMVolumeDeleteRequest.java:
##########
@@ -57,6 +60,36 @@ public OMVolumeDeleteRequest(OMRequest omRequest) {
     super(omRequest);
   }
 
+  @Override
+  public OMRequest preExecute(OzoneManager ozoneManager) throws IOException {
+    super.preExecute(ozoneManager);
+    DeleteVolumeRequest deleteVolumeRequest =
+        getOmRequest().getDeleteVolumeRequest();
+    Objects.requireNonNull(deleteVolumeRequest);
+    String volume = deleteVolumeRequest.getVolumeName();
+
+    // ACL check during preExecute
+    if (ozoneManager.getAclsEnabled()) {
+      try {
+        checkAcls(ozoneManager, OzoneObj.ResourceType.VOLUME,
+            OzoneObj.StoreType.OZONE, IAccessAuthorizer.ACLType.DELETE, volume,
+            null, null);
+      } catch (IOException ex) {
+        // Ensure audit log captures preExecute failures
+        Map<String, String> auditMap = new LinkedHashMap<>();
+        auditMap.put(OzoneConsts.VOLUME, volume);
+        markForAudit(ozoneManager.getAuditLogger(),
+            buildAuditMessage(OMAction.DELETE_VOLUME, auditMap, ex,
+                getOmRequest().getUserInfo()));
+        throw ex;
+      }
+    }
+
+    return getOmRequest().toBuilder()
+        .setUserInfo(getUserIfNotExists(ozoneManager))
+        .build();

Review Comment:
   Can we use the OMRequest returned in `super.preExecute` to prevent rebuilding another `OMRequest`? Similar to other places like `OmVolumeSetOwnerRequest` and `OMVolumeSetQuotaRequest`.



-- 
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

Re: [PR] HDDS-15462. Move ACL check in Volume requests to preExecute [ozone]

Posted by "ivandika3 (via GitHub)" <gi...@apache.org>.
ivandika3 commented on code in PR #10328:
URL: https://github.com/apache/ozone/pull/10328#discussion_r3357123279


##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/volume/acl/OMVolumeAclRequest.java:
##########
@@ -53,6 +55,43 @@ public abstract class OMVolumeAclRequest extends OMVolumeRequest {
     omVolumeAclOp = aclOp;
   }
 
+  @Override
+  public OzoneManagerProtocolProtos.OMRequest preExecute(OzoneManager ozoneManager)
+      throws IOException {
+    OzoneManagerProtocolProtos.OMRequest omRequest = super.preExecute(ozoneManager);
+
+    // ACL check during preExecute
+    if (ozoneManager.getAclsEnabled()) {
+      String volume = getVolumeName();
+      try {
+        checkAcls(ozoneManager, OzoneObj.ResourceType.VOLUME,
+            OzoneObj.StoreType.OZONE, IAccessAuthorizer.ACLType.WRITE_ACL,
+            volume, null, null);

Review Comment:
   Seems this is a regression, previously we don't always require WRITE_ACL, but not super.preExecute requires WRITE_ACL. Please check this and write a test for this in another ticket.



-- 
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

Re: [PR] HDDS-13855. Move ACL check in Volume requests to preExecute [ozone]

Posted by "ChenSammi (via GitHub)" <gi...@apache.org>.
ChenSammi commented on PR #10328:
URL: https://github.com/apache/ozone/pull/10328#issuecomment-4589450689

   @ss77892 , there are three different patches for the same "HDDS-13855. Move ACL check in Volume requests to preExecute" JIRA.  We should create individual JIRA for each of them.   


-- 
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

Re: [PR] HDDS-15462. Move ACL check in Volume requests to preExecute [ozone]

Posted by "ivandika3 (via GitHub)" <gi...@apache.org>.
ivandika3 commented on PR #10328:
URL: https://github.com/apache/ozone/pull/10328#issuecomment-4612700227

   Thanks @ss77892 for the patch and @ChenSammi for noticing the ticket issue.


-- 
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

Re: [PR] HDDS-15462. Move ACL check in Volume requests to preExecute [ozone]

Posted by "ivandika3 (via GitHub)" <gi...@apache.org>.
ivandika3 merged PR #10328:
URL: https://github.com/apache/ozone/pull/10328


-- 
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

Re: [PR] HDDS-15462. Move ACL check in Volume requests to preExecute [ozone]

Posted by "ivandika3 (via GitHub)" <gi...@apache.org>.
ivandika3 commented on code in PR #10328:
URL: https://github.com/apache/ozone/pull/10328#discussion_r3345464811


##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/volume/acl/OMVolumeAddAclRequest.java:
##########
@@ -58,14 +58,16 @@ public class OMVolumeAddAclRequest extends OMVolumeAclRequest {
 
   @Override
   public OMRequest preExecute(OzoneManager ozoneManager) throws IOException {
+    // Call parent preExecute to perform ACL check
+    OMRequest omRequest = super.preExecute(ozoneManager);
+
     long modificationTime = Time.now();
     OzoneManagerProtocolProtos.AddAclRequest.Builder addAclRequestBuilder =
-        getOmRequest().getAddAclRequest().toBuilder()
+        omRequest.getAddAclRequest().toBuilder()
             .setModificationTime(modificationTime);

Review Comment:
   In the future, we also might want to have `OMRequest` carries the modificationTime so that we are not rebuilding the OMRequest all the time.



-- 
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

Re: [PR] HDDS-15462. Move ACL check in Volume requests to preExecute [ozone]

Posted by "ss77892 (via GitHub)" <gi...@apache.org>.
ss77892 commented on code in PR #10328:
URL: https://github.com/apache/ozone/pull/10328#discussion_r3343182318


##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/volume/acl/OMVolumeAclRequest.java:
##########
@@ -53,6 +55,43 @@ public abstract class OMVolumeAclRequest extends OMVolumeRequest {
     omVolumeAclOp = aclOp;
   }
 
+  @Override
+  public OzoneManagerProtocolProtos.OMRequest preExecute(OzoneManager ozoneManager)
+      throws IOException {
+    OzoneManagerProtocolProtos.OMRequest omRequest = super.preExecute(ozoneManager);

Review Comment:
   That sounds reasonable. I will address it in a separate jira.
   



-- 
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