From 5af47615de1f0f1df3c418cb49dab22a2a288ae9 Mon Sep 17 00:00:00 2001 From: Sergey Soldatov Date: Thu, 21 May 2026 12:57:32 -0700 Subject: [PATCH] HDDS-13855. Move ACL check in Volume requests to preExecute Move ACL authorization checks for volume operations (delete, set-owner, set-quota) and volume ACL operations (add, remove, set ACL) from validateAndUpdateCache to preExecute. This ensures ACL enforcement happens before the Ratis log entry is written, so unauthorized requests are rejected early on the OM leader without producing log entries. Co-authored-by: Cursor --- .../request/volume/OMVolumeDeleteRequest.java | 42 +++++++++++++---- .../volume/OMVolumeSetOwnerRequest.java | 34 ++++++++++---- .../volume/OMVolumeSetQuotaRequest.java | 38 +++++++++++----- .../volume/acl/OMVolumeAclRequest.java | 45 ++++++++++++++++--- .../volume/acl/OMVolumeAddAclRequest.java | 8 ++-- .../volume/acl/OMVolumeRemoveAclRequest.java | 8 ++-- .../volume/acl/OMVolumeSetAclRequest.java | 8 ++-- 7 files changed, 141 insertions(+), 42 deletions(-) diff --git a/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/volume/OMVolumeDeleteRequest.java b/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/volume/OMVolumeDeleteRequest.java index ba32fe542c98..d90f67cac814 100644 --- a/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/volume/OMVolumeDeleteRequest.java +++ b/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/volume/OMVolumeDeleteRequest.java @@ -22,9 +22,12 @@ import java.io.IOException; import java.nio.file.InvalidPathException; +import java.util.LinkedHashMap; +import java.util.Map; import java.util.Objects; import org.apache.hadoop.hdds.utils.db.cache.CacheKey; import org.apache.hadoop.hdds.utils.db.cache.CacheValue; +import org.apache.hadoop.ozone.OzoneConsts; import org.apache.hadoop.ozone.audit.OMAction; import org.apache.hadoop.ozone.om.OMMetadataManager; import org.apache.hadoop.ozone.om.OMMetrics; @@ -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 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(); + } + @Override public OMClientResponse validateAndUpdateCache(OzoneManager ozoneManager, ExecutionContext context) { final long transactionLogIndex = context.getIndex(); @@ -80,13 +113,6 @@ public OMClientResponse validateAndUpdateCache(OzoneManager ozoneManager, Execut String owner = null; OMClientResponse omClientResponse = null; try { - // check Acl - if (ozoneManager.getAclsEnabled()) { - checkAcls(ozoneManager, OzoneObj.ResourceType.VOLUME, - OzoneObj.StoreType.OZONE, IAccessAuthorizer.ACLType.DELETE, volume, - null, null); - } - mergeOmLockDetails(omMetadataManager.getLock().acquireWriteLock( VOLUME_LOCK, volume)); acquiredVolumeLock = getOmLockDetails().isLockAcquired(); @@ -169,6 +195,4 @@ public OMClientResponse validateAndUpdateCache(OzoneManager ozoneManager, Execut } return omClientResponse; } - } - diff --git a/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/volume/OMVolumeSetOwnerRequest.java b/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/volume/OMVolumeSetOwnerRequest.java index 02c3b7874e99..d4c5d155fe84 100644 --- a/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/volume/OMVolumeSetOwnerRequest.java +++ b/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/volume/OMVolumeSetOwnerRequest.java @@ -21,6 +21,7 @@ import java.io.IOException; import java.nio.file.InvalidPathException; +import java.util.LinkedHashMap; import java.util.Map; import java.util.Objects; import org.apache.hadoop.hdds.utils.db.cache.CacheKey; @@ -61,15 +62,39 @@ public OMVolumeSetOwnerRequest(OMRequest omRequest) { @Override public OMRequest preExecute(OzoneManager ozoneManager) throws IOException { + super.preExecute(ozoneManager); long modificationTime = Time.now(); SetVolumePropertyRequest.Builder setPropertyRequestBuilder = getOmRequest() .getSetVolumePropertyRequest().toBuilder() .setModificationTime(modificationTime); + SetVolumePropertyRequest setVolumePropertyRequest = + getOmRequest().getSetVolumePropertyRequest(); + String volume = setVolumePropertyRequest.getVolumeName(); + + // ACL check during preExecute + if (ozoneManager.getAclsEnabled()) { + try { + checkAcls(ozoneManager, OzoneObj.ResourceType.VOLUME, + OzoneObj.StoreType.OZONE, IAccessAuthorizer.ACLType.WRITE_ACL, + volume, null, null); + } catch (IOException ex) { + // Ensure audit log captures preExecute failures + Map auditMap = new LinkedHashMap<>(); + auditMap.put(OzoneConsts.VOLUME, volume); + auditMap.put(OzoneConsts.OWNER, + setVolumePropertyRequest.getOwnerName()); + markForAudit(ozoneManager.getAuditLogger(), + buildAuditMessage(OMAction.SET_OWNER, auditMap, ex, + getOmRequest().getUserInfo())); + throw ex; + } + } + return getOmRequest().toBuilder() .setSetVolumePropertyRequest(setPropertyRequestBuilder) - .setUserInfo(getUserInfo()) + .setUserInfo(getUserIfNotExists(ozoneManager)) .build(); } @@ -108,13 +133,6 @@ public OMClientResponse validateAndUpdateCache(OzoneManager ozoneManager, Execut String oldOwner = null; OMClientResponse omClientResponse = null; try { - // check Acl - if (ozoneManager.getAclsEnabled()) { - checkAcls(ozoneManager, OzoneObj.ResourceType.VOLUME, - OzoneObj.StoreType.OZONE, IAccessAuthorizer.ACLType.WRITE_ACL, - volume, null, null); - } - long maxUserVolumeCount = ozoneManager.getMaxUserVolumeCount(); OzoneManagerStorageProtos.PersistedUserVolumeInfo oldOwnerVolumeList; OzoneManagerStorageProtos.PersistedUserVolumeInfo newOwnerVolumeList; diff --git a/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/volume/OMVolumeSetQuotaRequest.java b/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/volume/OMVolumeSetQuotaRequest.java index c93e8cbeb6c3..5269cf8a6ed3 100644 --- a/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/volume/OMVolumeSetQuotaRequest.java +++ b/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/volume/OMVolumeSetQuotaRequest.java @@ -21,6 +21,7 @@ import java.io.IOException; import java.nio.file.InvalidPathException; +import java.util.LinkedHashMap; import java.util.List; import java.util.Map; import java.util.Objects; @@ -63,15 +64,39 @@ public OMVolumeSetQuotaRequest(OMRequest omRequest) { @Override public OMRequest preExecute(OzoneManager ozoneManager) throws IOException { + super.preExecute(ozoneManager); long modificationTime = Time.now(); - SetVolumePropertyRequest.Builder setPropertyRequestBuilde = getOmRequest() + SetVolumePropertyRequest.Builder setPropertyRequestBuilder = getOmRequest() .getSetVolumePropertyRequest().toBuilder() .setModificationTime(modificationTime); + SetVolumePropertyRequest setVolumePropertyRequest = + getOmRequest().getSetVolumePropertyRequest(); + String volume = setVolumePropertyRequest.getVolumeName(); + + // ACL check during preExecute + if (ozoneManager.getAclsEnabled()) { + try { + checkAcls(ozoneManager, OzoneObj.ResourceType.VOLUME, + OzoneObj.StoreType.OZONE, IAccessAuthorizer.ACLType.WRITE, volume, + null, null); + } catch (IOException ex) { + // Ensure audit log captures preExecute failures + Map auditMap = new LinkedHashMap<>(); + auditMap.put(OzoneConsts.VOLUME, volume); + auditMap.put(OzoneConsts.QUOTA_IN_BYTES, + String.valueOf(setVolumePropertyRequest.getQuotaInBytes())); + markForAudit(ozoneManager.getAuditLogger(), + buildAuditMessage(OMAction.SET_QUOTA, auditMap, ex, + getOmRequest().getUserInfo())); + throw ex; + } + } + return getOmRequest().toBuilder() - .setSetVolumePropertyRequest(setPropertyRequestBuilde) - .setUserInfo(getUserInfo()) + .setSetVolumePropertyRequest(setPropertyRequestBuilder) + .setUserInfo(getUserIfNotExists(ozoneManager)) .build(); } @@ -109,13 +134,6 @@ public OMClientResponse validateAndUpdateCache(OzoneManager ozoneManager, Execut boolean acquireVolumeLock = false; OMClientResponse omClientResponse = null; try { - // check Acl - if (ozoneManager.getAclsEnabled()) { - checkAcls(ozoneManager, OzoneObj.ResourceType.VOLUME, - OzoneObj.StoreType.OZONE, IAccessAuthorizer.ACLType.WRITE, volume, - null, null); - } - mergeOmLockDetails(omMetadataManager.getLock().acquireWriteLock( VOLUME_LOCK, volume)); acquireVolumeLock = getOmLockDetails().isLockAcquired(); diff --git a/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/volume/acl/OMVolumeAclRequest.java b/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/volume/acl/OMVolumeAclRequest.java index 23b522ad77a6..0af78aa9d73a 100644 --- a/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/volume/acl/OMVolumeAclRequest.java +++ b/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/volume/acl/OMVolumeAclRequest.java @@ -21,6 +21,7 @@ import java.io.IOException; import java.nio.file.InvalidPathException; +import java.util.LinkedHashMap; import java.util.List; import java.util.Map; import org.apache.hadoop.hdds.utils.db.cache.CacheKey; @@ -28,6 +29,7 @@ import org.apache.hadoop.ozone.OzoneAcl; import org.apache.hadoop.ozone.OzoneConsts; import org.apache.hadoop.ozone.audit.AuditLogger; +import org.apache.hadoop.ozone.audit.OMAction; import org.apache.hadoop.ozone.om.OMMetadataManager; import org.apache.hadoop.ozone.om.OMMetrics; import org.apache.hadoop.ozone.om.OzoneManager; @@ -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); + } catch (IOException ex) { + // Ensure audit log captures preExecute failures + Map auditMap = new LinkedHashMap<>(); + auditMap.put(OzoneConsts.VOLUME, volume); + List acls = getAcls(); + if (acls != null) { + auditMap.put(OzoneConsts.ACL, acls.toString()); + } + // Determine which action based on request type + OMAction action = OMAction.SET_ACL; + if (omRequest.hasAddAclRequest()) { + action = OMAction.ADD_ACL; + } else if (omRequest.hasRemoveAclRequest()) { + action = OMAction.REMOVE_ACL; + } + markForAudit(ozoneManager.getAuditLogger(), + buildAuditMessage(action, auditMap, ex, + omRequest.getUserInfo())); + throw ex; + } + } + + return omRequest; + } + @Override public OMClientResponse validateAndUpdateCache(OzoneManager ozoneManager, ExecutionContext context) { final long trxnLogIndex = context.getIndex(); @@ -71,12 +110,6 @@ public OMClientResponse validateAndUpdateCache(OzoneManager ozoneManager, Execut boolean lockAcquired = false; Result result; try { - // check Acl - if (ozoneManager.getAclsEnabled()) { - checkAcls(ozoneManager, OzoneObj.ResourceType.VOLUME, - OzoneObj.StoreType.OZONE, IAccessAuthorizer.ACLType.WRITE_ACL, - volume, null, null); - } mergeOmLockDetails(omMetadataManager.getLock().acquireWriteLock( VOLUME_LOCK, volume)); lockAcquired = getOmLockDetails().isLockAcquired(); diff --git a/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/volume/acl/OMVolumeAddAclRequest.java b/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/volume/acl/OMVolumeAddAclRequest.java index c0e87043ea34..6dfc64547189 100644 --- a/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/volume/acl/OMVolumeAddAclRequest.java +++ b/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); - return getOmRequest().toBuilder() + return omRequest.toBuilder() .setAddAclRequest(addAclRequestBuilder) - .setUserInfo(getUserInfo()) .build(); } diff --git a/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/volume/acl/OMVolumeRemoveAclRequest.java b/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/volume/acl/OMVolumeRemoveAclRequest.java index 05f338957ee6..ceabf00b5663 100644 --- a/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/volume/acl/OMVolumeRemoveAclRequest.java +++ b/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/volume/acl/OMVolumeRemoveAclRequest.java @@ -58,14 +58,16 @@ public class OMVolumeRemoveAclRequest 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.RemoveAclRequest.Builder removeAclRequestBuilder - = getOmRequest().getRemoveAclRequest().toBuilder() + = omRequest.getRemoveAclRequest().toBuilder() .setModificationTime(modificationTime); - return getOmRequest().toBuilder() + return omRequest.toBuilder() .setRemoveAclRequest(removeAclRequestBuilder) - .setUserInfo(getUserInfo()) .build(); } diff --git a/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/volume/acl/OMVolumeSetAclRequest.java b/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/volume/acl/OMVolumeSetAclRequest.java index 6abffc2197fa..c68f24906d71 100644 --- a/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/volume/acl/OMVolumeSetAclRequest.java +++ b/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/volume/acl/OMVolumeSetAclRequest.java @@ -57,14 +57,16 @@ public class OMVolumeSetAclRequest 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.SetAclRequest.Builder setAclRequestBuilder = - getOmRequest().getSetAclRequest().toBuilder() + omRequest.getSetAclRequest().toBuilder() .setModificationTime(modificationTime); - return getOmRequest().toBuilder() + return omRequest.toBuilder() .setSetAclRequest(setAclRequestBuilder) - .setUserInfo(getUserInfo()) .build(); }