From 49389c46c8299ef32998774a2c78859251a9293f Mon Sep 17 00:00:00 2001 From: Sergey Soldatov Date: Thu, 21 May 2026 12:58:20 -0700 Subject: [PATCH] HDDS-13855. Move ACL check in Bucket requests to preExecute Move ACL authorization checks for bucket operations (delete, set-owner, set-property) and bucket 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/bucket/OMBucketDeleteRequest.java | 41 ++++++++++++--- .../bucket/OMBucketSetOwnerRequest.java | 35 ++++++++++--- .../bucket/OMBucketSetPropertyRequest.java | 27 +++++++--- .../bucket/acl/OMBucketAclRequest.java | 51 ++++++++++++++++--- .../bucket/acl/OMBucketSetAclRequest.java | 1 + 5 files changed, 127 insertions(+), 28 deletions(-) diff --git a/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/bucket/OMBucketDeleteRequest.java b/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/bucket/OMBucketDeleteRequest.java index deb2c8a05b32..8ff4495736d3 100644 --- a/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/bucket/OMBucketDeleteRequest.java +++ b/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/bucket/OMBucketDeleteRequest.java @@ -25,6 +25,7 @@ import java.io.IOException; import java.nio.file.InvalidPathException; import java.util.Iterator; +import java.util.LinkedHashMap; import java.util.Map; import org.apache.hadoop.hdds.utils.db.Table; import org.apache.hadoop.hdds.utils.db.TableIterator; @@ -73,6 +74,37 @@ public OMBucketDeleteRequest(OMRequest omRequest) { super(omRequest); } + @Override + public OMRequest preExecute(OzoneManager ozoneManager) throws IOException { + super.preExecute(ozoneManager); + DeleteBucketRequest deleteBucketRequest = + getOmRequest().getDeleteBucketRequest(); + String volumeName = deleteBucketRequest.getVolumeName(); + String bucketName = deleteBucketRequest.getBucketName(); + + // ACL check during preExecute + if (ozoneManager.getAclsEnabled()) { + try { + checkAcls(ozoneManager, OzoneObj.ResourceType.BUCKET, + OzoneObj.StoreType.OZONE, IAccessAuthorizer.ACLType.DELETE, + volumeName, bucketName, null); + } catch (IOException ex) { + // Ensure audit log captures preExecute failures + Map auditMap = new LinkedHashMap<>(); + auditMap.put(OzoneConsts.VOLUME, volumeName); + auditMap.put(OzoneConsts.BUCKET, bucketName); + markForAudit(ozoneManager.getAuditLogger(), + buildAuditMessage(OMAction.DELETE_BUCKET, 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(); @@ -101,13 +133,6 @@ public OMClientResponse validateAndUpdateCache(OzoneManager ozoneManager, Execut boolean success = true; OMClientResponse omClientResponse = null; try { - // check Acl - if (ozoneManager.getAclsEnabled()) { - checkAcls(ozoneManager, OzoneObj.ResourceType.BUCKET, - OzoneObj.StoreType.OZONE, IAccessAuthorizer.ACLType.DELETE, - volumeName, bucketName, null); - } - // acquire lock mergeOmLockDetails( omMetadataManager.getLock().acquireReadLock(VOLUME_LOCK, volumeName)); @@ -265,7 +290,7 @@ private boolean bucketContainsSnapshotInCache( /** * Validates bucket delete requests. - * Handles the cases where an older client attempts to delete a bucket + * Handles the cases where an older client attempts to delete a bucket with * a new bucket layout. * We do not want to allow this to happen, since this would cause the client * to be able to delete buckets it cannot understand. diff --git a/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/bucket/OMBucketSetOwnerRequest.java b/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/bucket/OMBucketSetOwnerRequest.java index 6d0c90cdca29..619a1b3fc3d8 100644 --- a/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/bucket/OMBucketSetOwnerRequest.java +++ b/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/bucket/OMBucketSetOwnerRequest.java @@ -21,6 +21,7 @@ import java.io.IOException; import java.nio.file.InvalidPathException; +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; @@ -63,15 +64,40 @@ public OMBucketSetOwnerRequest(OMRequest omRequest) { @Override public OMRequest preExecute(OzoneManager ozoneManager) throws IOException { + super.preExecute(ozoneManager); + long modificationTime = Time.now(); OzoneManagerProtocolProtos.SetBucketPropertyRequest.Builder setBucketPropertyRequestBuilder = getOmRequest() .getSetBucketPropertyRequest().toBuilder() .setModificationTime(modificationTime); + SetBucketPropertyRequest setBucketPropertyRequest = + getOmRequest().getSetBucketPropertyRequest(); + BucketArgs bucketArgs = setBucketPropertyRequest.getBucketArgs(); + String volumeName = bucketArgs.getVolumeName(); + String bucketName = bucketArgs.getBucketName(); + + // ACL check during preExecute + if (ozoneManager.getAclsEnabled()) { + try { + checkAcls(ozoneManager, OzoneObj.ResourceType.BUCKET, + OzoneObj.StoreType.OZONE, IAccessAuthorizer.ACLType.WRITE_ACL, + volumeName, bucketName, null); + } catch (IOException ex) { + // Ensure audit log captures preExecute failures + OmBucketArgs omBucketArgs = OmBucketArgs.getFromProtobuf(bucketArgs); + Map auditMap = omBucketArgs.toAuditMap(); + markForAudit(ozoneManager.getAuditLogger(), + buildAuditMessage(OMAction.SET_OWNER, auditMap, ex, + getOmRequest().getUserInfo())); + throw ex; + } + } + return getOmRequest().toBuilder() .setSetBucketPropertyRequest(setBucketPropertyRequestBuilder) - .setUserInfo(getUserInfo()) + .setUserInfo(getUserIfNotExists(ozoneManager)) .build(); } @@ -109,13 +135,6 @@ public OMClientResponse validateAndUpdateCache(OzoneManager ozoneManager, Execut boolean acquiredBucketLock = false, success = true; OMClientResponse omClientResponse = null; try { - // check Acl - if (ozoneManager.getAclsEnabled()) { - checkAcls(ozoneManager, OzoneObj.ResourceType.BUCKET, - OzoneObj.StoreType.OZONE, IAccessAuthorizer.ACLType.WRITE_ACL, - volumeName, bucketName, null); - } - // acquire lock. mergeOmLockDetails(omMetadataManager.getLock().acquireWriteLock( BUCKET_LOCK, volumeName, bucketName)); diff --git a/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/bucket/OMBucketSetPropertyRequest.java b/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/bucket/OMBucketSetPropertyRequest.java index a88e5fb73334..6f91bcff9317 100644 --- a/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/bucket/OMBucketSetPropertyRequest.java +++ b/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/bucket/OMBucketSetPropertyRequest.java @@ -22,6 +22,7 @@ import java.io.IOException; import java.nio.file.InvalidPathException; import java.util.List; +import java.util.Map; import java.util.Objects; import org.apache.hadoop.crypto.key.KeyProviderCryptoExtension; import org.apache.hadoop.hdds.client.DefaultReplicationConfig; @@ -79,6 +80,8 @@ public OMBucketSetPropertyRequest(OMRequest omRequest) { @Override public OMRequest preExecute(OzoneManager ozoneManager) throws IOException { + super.preExecute(ozoneManager); + long modificationTime = Time.now(); OzoneManagerProtocolProtos.SetBucketPropertyRequest.Builder setBucketPropertyRequestBuilder = getOmRequest() @@ -97,9 +100,26 @@ public OMRequest preExecute(OzoneManager ozoneManager) setBucketPropertyRequestBuilder.setBucketArgs(bucketArgsBuilder.build()); } + // ACL check during preExecute + if (ozoneManager.getAclsEnabled()) { + String volumeName = bucketArgs.getVolumeName(); + String bucketName = bucketArgs.getBucketName(); + try { + checkAclPermission(ozoneManager, volumeName, bucketName); + } catch (IOException ex) { + // Ensure audit log captures preExecute failures + OmBucketArgs omBucketArgs = OmBucketArgs.getFromProtobuf(bucketArgs); + Map auditMap = omBucketArgs.toAuditMap(); + markForAudit(ozoneManager.getAuditLogger(), + buildAuditMessage(OMAction.UPDATE_BUCKET, auditMap, ex, + getOmRequest().getUserInfo())); + throw ex; + } + } + return getOmRequest().toBuilder() .setSetBucketPropertyRequest(setBucketPropertyRequestBuilder) - .setUserInfo(getUserInfo()) + .setUserInfo(getUserIfNotExists(ozoneManager)) .build(); } @@ -132,11 +152,6 @@ public OMClientResponse validateAndUpdateCache(OzoneManager ozoneManager, Execut boolean acquiredBucketLock = false, success = true; OMClientResponse omClientResponse = null; try { - // check Acl - if (ozoneManager.getAclsEnabled()) { - checkAclPermission(ozoneManager, volumeName, bucketName); - } - // acquire lock. mergeOmLockDetails(omMetadataManager.getLock().acquireWriteLock( BUCKET_LOCK, volumeName, bucketName)); diff --git a/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/bucket/acl/OMBucketAclRequest.java b/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/bucket/acl/OMBucketAclRequest.java index 6faac1a24dc3..783d42315df8 100644 --- a/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/bucket/acl/OMBucketAclRequest.java +++ b/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/bucket/acl/OMBucketAclRequest.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.commons.lang3.tuple.Pair; @@ -29,6 +30,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; @@ -59,6 +61,49 @@ public OMBucketAclRequest(OMRequest omRequest, AclOp aclOp) { omBucketAclOp = aclOp; } + @Override + public OMRequest preExecute(OzoneManager ozoneManager) throws IOException { + OMRequest omRequest = super.preExecute(ozoneManager); + + // ACL check during preExecute + if (ozoneManager.getAclsEnabled()) { + try { + ObjectParser objectParser = new ObjectParser(getPath(), + ObjectType.BUCKET); + ResolvedBucket resolvedBucket = ozoneManager.resolveBucketLink( + Pair.of(objectParser.getVolume(), objectParser.getBucket())); + String volume = resolvedBucket.realVolume(); + String bucket = resolvedBucket.realBucket(); + + checkAcls(ozoneManager, OzoneObj.ResourceType.BUCKET, + OzoneObj.StoreType.OZONE, IAccessAuthorizer.ACLType.WRITE_ACL, + volume, bucket, null); + } catch (IOException ex) { + // Ensure audit log captures preExecute failures + Map auditMap = new LinkedHashMap<>(); + OzoneObj obj = getObject(); + auditMap.putAll(obj.toAuditMap()); + 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 transactionLogIndex = context.getIndex(); @@ -87,12 +132,6 @@ public OMClientResponse validateAndUpdateCache(OzoneManager ozoneManager, Execut volume = resolvedBucket.realVolume(); bucket = resolvedBucket.realBucket(); - // check Acl - if (ozoneManager.getAclsEnabled()) { - checkAcls(ozoneManager, OzoneObj.ResourceType.BUCKET, - OzoneObj.StoreType.OZONE, IAccessAuthorizer.ACLType.WRITE_ACL, - volume, bucket, null); - } mergeOmLockDetails( omMetadataManager.getLock().acquireWriteLock(BUCKET_LOCK, volume, bucket)); diff --git a/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/bucket/acl/OMBucketSetAclRequest.java b/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/bucket/acl/OMBucketSetAclRequest.java index 97dca83c1978..d7a02e2a54fe 100644 --- a/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/bucket/acl/OMBucketSetAclRequest.java +++ b/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/bucket/acl/OMBucketSetAclRequest.java @@ -54,6 +54,7 @@ public class OMBucketSetAclRequest extends OMBucketAclRequest { @Override public OMRequest preExecute(OzoneManager ozoneManager) throws IOException { + super.preExecute(ozoneManager); long modificationTime = Time.now(); OzoneManagerProtocolProtos.SetAclRequest.Builder setAclRequestBuilder = getOmRequest().getSetAclRequest().toBuilder()