From 63cac49e470775b1b62e7a578c8148add813a18c Mon Sep 17 00:00:00 2001 From: Aryan Gupta Date: Tue, 27 Jun 2023 22:44:44 +0530 Subject: [PATCH 1/3] HDDS-7750. Incorrect WRITE ACL check --- .../hadoop/ozone/om/KeyManagerImpl.java | 45 ++++++++----------- .../S3MultipartUploadCommitPartRequest.java | 8 ++-- .../s3/multipart/TestS3MultipartRequest.java | 3 ++ 3 files changed, 26 insertions(+), 30 deletions(-) diff --git a/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/KeyManagerImpl.java b/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/KeyManagerImpl.java index 46eda684dff1..6b289e8a765e 100644 --- a/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/KeyManagerImpl.java +++ b/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/KeyManagerImpl.java @@ -934,32 +934,25 @@ public boolean checkAccess(OzoneObj ozObject, RequestContext context) OMFileRequest.validateBucket(metadataManager, volume, bucket); OmKeyInfo keyInfo; - // For Acl Type "WRITE", the key can only be found in - // OpenKeyTable since appends to existing keys are not supported. - if (context.getAclRights() == IAccessAuthorizer.ACLType.WRITE) { - keyInfo = - metadataManager.getOpenKeyTable(bucketLayout).get(objectKey); - } else { - // Recursive check is done only for ACL_TYPE DELETE - // Rename and delete operations will send ACL_TYPE DELETE - if (context.isRecursiveAccessCheck() - && context.getAclRights() == IAccessAuthorizer.ACLType.DELETE) { - return checkChildrenAcls(ozObject, context); - } - try { - OzoneFileStatus fileStatus = getFileStatus(args); - keyInfo = fileStatus.getKeyInfo(); - } catch (IOException e) { - // OzoneFS will check whether the key exists when write a new key. - // For Acl Type "READ", when the key is not exist return true. - // To Avoid KEY_NOT_FOUND Exception. - if (context.getAclRights() == IAccessAuthorizer.ACLType.READ) { - return true; - } else { - throw new OMException( - "Key not found, checkAccess failed. Key:" + objectKey, - KEY_NOT_FOUND); - } + // Recursive check is done only for ACL_TYPE DELETE + // Rename and delete operations will send ACL_TYPE DELETE + if (context.isRecursiveAccessCheck() + && context.getAclRights() == IAccessAuthorizer.ACLType.DELETE) { + return checkChildrenAcls(ozObject, context); + } + try { + OzoneFileStatus fileStatus = getFileStatus(args); + keyInfo = fileStatus.getKeyInfo(); + } catch (IOException e) { + // OzoneFS will check whether the key exists when write a new key. + // For Acl Type "READ", when the key is not exist return true. + // To Avoid KEY_NOT_FOUND Exception. + if (context.getAclRights() == IAccessAuthorizer.ACLType.READ) { + return true; + } else { + throw new OMException( + "Key not found, checkAccess failed. Key:" + objectKey, + KEY_NOT_FOUND); } } diff --git a/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/s3/multipart/S3MultipartUploadCommitPartRequest.java b/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/s3/multipart/S3MultipartUploadCommitPartRequest.java index 1769fdbb2014..b6ffe01ac791 100644 --- a/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/s3/multipart/S3MultipartUploadCommitPartRequest.java +++ b/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/s3/multipart/S3MultipartUploadCommitPartRequest.java @@ -133,9 +133,11 @@ public OMClientResponse validateAndUpdateCache(OzoneManager ozoneManager, volumeName = keyArgs.getVolumeName(); bucketName = keyArgs.getBucketName(); + long clientID = multipartCommitUploadPartRequest.getClientID(); + // check acl - checkKeyAcls(ozoneManager, volumeName, bucketName, keyName, - IAccessAuthorizer.ACLType.WRITE, OzoneObj.ResourceType.KEY); + checkKeyAclsInOpenKeyTable(ozoneManager, volumeName, bucketName, keyName, + IAccessAuthorizer.ACLType.WRITE, clientID); acquiredLock = omMetadataManager.getLock().acquireWriteLock(BUCKET_LOCK, volumeName, bucketName); @@ -149,8 +151,6 @@ public OMClientResponse validateAndUpdateCache(OzoneManager ozoneManager, multipartKeyInfo = omMetadataManager.getMultipartInfoTable() .get(multipartKey); - long clientID = multipartCommitUploadPartRequest.getClientID(); - openKey = getOpenKey(volumeName, bucketName, keyName, omMetadataManager, clientID); diff --git a/hadoop-ozone/ozone-manager/src/test/java/org/apache/hadoop/ozone/om/request/s3/multipart/TestS3MultipartRequest.java b/hadoop-ozone/ozone-manager/src/test/java/org/apache/hadoop/ozone/om/request/s3/multipart/TestS3MultipartRequest.java index 591c3bfa76af..0c71b503a4cb 100644 --- a/hadoop-ozone/ozone-manager/src/test/java/org/apache/hadoop/ozone/om/request/s3/multipart/TestS3MultipartRequest.java +++ b/hadoop-ozone/ozone-manager/src/test/java/org/apache/hadoop/ozone/om/request/s3/multipart/TestS3MultipartRequest.java @@ -38,6 +38,7 @@ import org.apache.hadoop.ozone.audit.AuditMessage; import org.apache.hadoop.ozone.om.OMConfigKeys; import org.apache.hadoop.ozone.om.OMMetadataManager; +import org.apache.hadoop.ozone.om.OmMetadataReader; import org.apache.hadoop.ozone.om.OMMetrics; import org.apache.hadoop.ozone.om.OmMetadataManagerImpl; import org.apache.hadoop.ozone.om.OzoneManager; @@ -83,6 +84,8 @@ public void setup() throws Exception { when(ozoneManager.getMetrics()).thenReturn(omMetrics); when(ozoneManager.getMetadataManager()).thenReturn(omMetadataManager); auditLogger = Mockito.mock(AuditLogger.class); + OmMetadataReader omMetadataReader = Mockito.mock(OmMetadataReader.class); + when(ozoneManager.getOmMetadataReader()).thenReturn(omMetadataReader); when(ozoneManager.getAuditLogger()).thenReturn(auditLogger); when(ozoneManager.getDefaultReplicationConfig()).thenReturn( ReplicationConfig.getDefault(ozoneConfiguration)); From fcbe3eef88ec04bd4b3bce62c527478ab66dbd8e Mon Sep 17 00:00:00 2001 From: Aryan Gupta Date: Wed, 28 Jun 2023 18:39:18 +0530 Subject: [PATCH 2/3] Revert change in KeyManagerImpl.java --- .../hadoop/ozone/om/KeyManagerImpl.java | 45 +++++++++++-------- 1 file changed, 26 insertions(+), 19 deletions(-) diff --git a/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/KeyManagerImpl.java b/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/KeyManagerImpl.java index 6b289e8a765e..46eda684dff1 100644 --- a/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/KeyManagerImpl.java +++ b/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/KeyManagerImpl.java @@ -934,25 +934,32 @@ public boolean checkAccess(OzoneObj ozObject, RequestContext context) OMFileRequest.validateBucket(metadataManager, volume, bucket); OmKeyInfo keyInfo; - // Recursive check is done only for ACL_TYPE DELETE - // Rename and delete operations will send ACL_TYPE DELETE - if (context.isRecursiveAccessCheck() - && context.getAclRights() == IAccessAuthorizer.ACLType.DELETE) { - return checkChildrenAcls(ozObject, context); - } - try { - OzoneFileStatus fileStatus = getFileStatus(args); - keyInfo = fileStatus.getKeyInfo(); - } catch (IOException e) { - // OzoneFS will check whether the key exists when write a new key. - // For Acl Type "READ", when the key is not exist return true. - // To Avoid KEY_NOT_FOUND Exception. - if (context.getAclRights() == IAccessAuthorizer.ACLType.READ) { - return true; - } else { - throw new OMException( - "Key not found, checkAccess failed. Key:" + objectKey, - KEY_NOT_FOUND); + // For Acl Type "WRITE", the key can only be found in + // OpenKeyTable since appends to existing keys are not supported. + if (context.getAclRights() == IAccessAuthorizer.ACLType.WRITE) { + keyInfo = + metadataManager.getOpenKeyTable(bucketLayout).get(objectKey); + } else { + // Recursive check is done only for ACL_TYPE DELETE + // Rename and delete operations will send ACL_TYPE DELETE + if (context.isRecursiveAccessCheck() + && context.getAclRights() == IAccessAuthorizer.ACLType.DELETE) { + return checkChildrenAcls(ozObject, context); + } + try { + OzoneFileStatus fileStatus = getFileStatus(args); + keyInfo = fileStatus.getKeyInfo(); + } catch (IOException e) { + // OzoneFS will check whether the key exists when write a new key. + // For Acl Type "READ", when the key is not exist return true. + // To Avoid KEY_NOT_FOUND Exception. + if (context.getAclRights() == IAccessAuthorizer.ACLType.READ) { + return true; + } else { + throw new OMException( + "Key not found, checkAccess failed. Key:" + objectKey, + KEY_NOT_FOUND); + } } } From 5df39e1a84b2549e08de5710a96583e89d98fda6 Mon Sep 17 00:00:00 2001 From: Aryan Gupta Date: Thu, 29 Jun 2023 00:16:57 +0530 Subject: [PATCH 3/3] Fixed failing CI check. --- .../request/s3/multipart/S3MultipartUploadCommitPartRequest.java | 1 - 1 file changed, 1 deletion(-) diff --git a/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/s3/multipart/S3MultipartUploadCommitPartRequest.java b/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/s3/multipart/S3MultipartUploadCommitPartRequest.java index b6ffe01ac791..fbd2f4e20e3e 100644 --- a/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/s3/multipart/S3MultipartUploadCommitPartRequest.java +++ b/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/s3/multipart/S3MultipartUploadCommitPartRequest.java @@ -53,7 +53,6 @@ .OMResponse; import org.apache.hadoop.ozone.protocol.proto.OzoneManagerProtocolProtos.Type; import org.apache.hadoop.ozone.security.acl.IAccessAuthorizer; -import org.apache.hadoop.ozone.security.acl.OzoneObj; import org.apache.hadoop.util.Time; import org.apache.hadoop.hdds.utils.db.cache.CacheKey; import org.apache.hadoop.hdds.utils.db.cache.CacheValue;