From 625b2f4fd1db8dc8a3cdbd600ec11f141e678700 Mon Sep 17 00:00:00 2001 From: Wei-Chiu Chuang Date: Thu, 8 May 2025 16:34:46 -0700 Subject: [PATCH 1/3] HDDS-12958. [Snapshot] Add ACL check regression tests for snapshot operations. --- .../snapshot/TestOzoneManagerSnapshotAcl.java | 126 +++++++++++++++++- .../apache/hadoop/ozone/om/OzoneManager.java | 12 ++ 2 files changed, 137 insertions(+), 1 deletion(-) diff --git a/hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/ozone/om/snapshot/TestOzoneManagerSnapshotAcl.java b/hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/ozone/om/snapshot/TestOzoneManagerSnapshotAcl.java index 30a9ed81b41c..99f85efff795 100644 --- a/hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/ozone/om/snapshot/TestOzoneManagerSnapshotAcl.java +++ b/hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/ozone/om/snapshot/TestOzoneManagerSnapshotAcl.java @@ -32,8 +32,12 @@ import java.io.File; import java.io.IOException; +import java.security.PrivilegedExceptionAction; import java.util.stream.Stream; import org.apache.commons.lang3.RandomStringUtils; +import org.apache.hadoop.fs.CommonConfigurationKeysPublic; +import org.apache.hadoop.fs.FileSystem; +import org.apache.hadoop.fs.Path; import org.apache.hadoop.hdds.conf.OzoneConfiguration; import org.apache.hadoop.hdds.scm.HddsWhiteboxTestUtils; import org.apache.hadoop.hdds.utils.IOUtils; @@ -65,6 +69,8 @@ import org.junit.jupiter.params.provider.Arguments; import org.junit.jupiter.params.provider.EnumSource; import org.junit.jupiter.params.provider.MethodSource; +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; /** * Test for Snapshot feature with ACL. @@ -84,8 +90,14 @@ public class TestOzoneManagerSnapshotAcl { private static final String GROUP2 = "group2"; private static final UserGroupInformation UGI2 = UserGroupInformation.createUserForTesting(USER2, new String[] {GROUP2}); + private static final String USER3 = "user3"; + private static final String GROUP3 = "group3"; + private static final UserGroupInformation UGI3 = + UserGroupInformation.createUserForTesting(USER3, new String[] {GROUP3}); private static final OzoneObj.ResourceType RESOURCE_TYPE_KEY = OzoneObj.ResourceType.KEY; + private static final Logger LOG = + LoggerFactory.getLogger(TestOzoneManagerSnapshotAcl.class); private static MiniOzoneCluster cluster; private static ObjectStore objectStore; private static OzoneManager ozoneManager; @@ -103,9 +115,14 @@ public static void init() throws Exception { final OzoneConfiguration conf = new OzoneConfiguration(); conf.setBoolean(OZONE_ACL_ENABLED, true); conf.set(OZONE_ACL_AUTHORIZER_CLASS, OZONE_ACL_AUTHORIZER_CLASS_NATIVE); + final String omServiceId = "om-service-test-1" + RandomStringUtils.secure().nextNumeric(32); + final String rootPath = String.format("%s://%s/", + OZONE_OFS_URI_SCHEME, omServiceId); + conf.set(CommonConfigurationKeysPublic.FS_DEFAULT_NAME_KEY, rootPath); + cluster = MiniOzoneCluster.newHABuilder(conf) .setOMServiceId(omServiceId) .setNumOfOzoneManagers(1) @@ -436,6 +453,108 @@ public void testLookupKeyWithNotAllowedUserForPrefixAcl(BucketLayout bucketLayou assertDoesNotThrow(() -> ozoneManager.lookupKey(keyArgs)); } + @ParameterizedTest + @EnumSource(BucketLayout.class) + public void testSnapshotPermissions(BucketLayout bucketLayout) throws Exception { + createVolume(); + + // create a bucket whose owner is user1 + final OzoneVolume volume = objectStore.getVolume(volumeName); + createBucket(bucketLayout, volume, USER1); + // allow user2 volume read and bucket read and list permissions. + setDefaultVolumeAcls(); + setBucketAcl(); + String bucket1Name = bucketName; + + String snapshot1 = + "snapshot-" + RandomStringUtils.secure().nextNumeric(32); + objectStore.createSnapshot(volumeName, bucketName, snapshot1); + String snapshot2 = + "snapshot-" + RandomStringUtils.secure().nextNumeric(32); + objectStore.createSnapshot(volumeName, bucketName, snapshot2); + + // user2 should be able to list the snapshots, get snapshot info, + // snapshot diff, list snapshot diff jobs and cancel snapshot diff jobs + UGI2.doAs( + (PrivilegedExceptionAction)() + -> { + OzoneClient client2 = cluster.newClient(); + ObjectStore objectStore2 = client2.getObjectStore(); + objectStore2.listSnapshot(volumeName, bucketName, null, null); + objectStore2.getSnapshotInfo(volumeName, bucketName, snapshot1); + objectStore2.snapshotDiff(volumeName, bucketName, + snapshot1, snapshot2, null, 0, false, false); + objectStore2.listSnapshotDiffJobs(volumeName, bucketName, "", true, null); + objectStore2.cancelSnapshotDiff(volumeName, bucketName, snapshot1, snapshot2); + return null; + }); + + // user3 has no permission, should not be able to list the snapshots, get snapshot info, + // snapshot diff, list snapshot diff jobs and cancel snapshot diff jobs. + ObjectStore objectStore3 = UGI3.doAs( + (PrivilegedExceptionAction)() + -> { + OzoneClient client2 = cluster.newClient(); + return client2.getObjectStore(); + }); + OMException ex = assertThrows(OMException.class, + () -> objectStore3.listSnapshot(volumeName, bucketName, null, null)); + assertEquals(OMException.ResultCodes.PERMISSION_DENIED, ex.getResult()); + + ex = assertThrows(OMException.class, + () -> objectStore3.getSnapshotInfo(volumeName, bucketName, snapshot1)); + assertEquals(OMException.ResultCodes.PERMISSION_DENIED, ex.getResult()); + + ex = assertThrows(OMException.class, + () -> objectStore3.snapshotDiff(volumeName, bucketName, snapshot1, snapshot2, + null, 0, false, false)); + assertEquals(OMException.ResultCodes.PERMISSION_DENIED, ex.getResult()); + + ex = assertThrows(OMException.class, + () -> objectStore3.listSnapshotDiffJobs(volumeName, bucketName, "", true, null)); + assertEquals(OMException.ResultCodes.PERMISSION_DENIED, ex.getResult()); + + ex = assertThrows(OMException.class, + () -> objectStore3.cancelSnapshotDiff(volumeName, bucketName, snapshot1, snapshot2)); + assertEquals(OMException.ResultCodes.PERMISSION_DENIED, ex.getResult()); + + // create a bucket whose owner is user2 + createBucket(bucketLayout, volume, USER2); + String bucket2Name = bucketName; + + // user2 should not be able to create a snapshot on bucket1 but should + // be able to create a snapshot on bucket2, rename it and delete it + final FileSystem fs2 = UGI2.doAs( + (PrivilegedExceptionAction)() + -> FileSystem.get(cluster.getConf())); + final String snapshotPrefix = "snapshot-"; + final String snapshotName = + snapshotPrefix + RandomStringUtils.secure().nextNumeric(32); + ex = assertThrows(OMException.class, () -> { + fs2.createSnapshot(new Path("/" + volumeName + "/" + bucket1Name), snapshotName); + }); + assertEquals(OMException.ResultCodes.PERMISSION_DENIED, ex.getResult()); + Path bucket2Path = new Path("/" + volumeName + "/" + bucket2Name); + fs2.createSnapshot(bucket2Path, snapshotName); + final String snapshotNameNew = snapshotName + ".new"; + fs2.renameSnapshot(bucket2Path, snapshotName, snapshotNameNew); + fs2.deleteSnapshot(bucket2Path, snapshotNameNew); + + // user3 should not be able to create a snapshot on bucket2 + // and should not be able to rename it, delete it + ex = assertThrows(OMException.class, + () -> objectStore3.createSnapshot(volumeName, bucket2Name, snapshotName)); + assertEquals(OMException.ResultCodes.PERMISSION_DENIED, ex.getResult()); + + ex = assertThrows(OMException.class, + () -> objectStore3.renameSnapshot(volumeName, bucket2Name, snapshotName, snapshotNameNew)); + assertEquals(OMException.ResultCodes.PERMISSION_DENIED, ex.getResult()); + + ex = assertThrows(OMException.class, + () -> objectStore3.deleteSnapshot(volumeName, bucket2Name, snapshotName)); + assertEquals(OMException.ResultCodes.PERMISSION_DENIED, ex.getResult()); + } + private void setup(BucketLayout bucketLayout) throws IOException { UserGroupInformation.setLoginUser(UGI1); @@ -612,10 +731,15 @@ private OmKeyArgs getOmKeyArgs(boolean isSnapshot) { private void createBucket(BucketLayout bucketLayout, OzoneVolume volume) throws IOException { + createBucket(bucketLayout, volume, ADMIN); + } + + private void createBucket(BucketLayout bucketLayout, + OzoneVolume volume, String owner) throws IOException { final String bucketPrefix = "bucket-"; bucketName = bucketPrefix + RandomStringUtils.secure().nextNumeric(32); final BucketArgs bucketArgs = BucketArgs.newBuilder() - .setOwner(ADMIN) + .setOwner(owner) .setBucketLayout(bucketLayout).build(); volume.createBucket(bucketName, bucketArgs); } diff --git a/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/OzoneManager.java b/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/OzoneManager.java index 23a7e7021cd2..d8653fa4c523 100644 --- a/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/OzoneManager.java +++ b/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/OzoneManager.java @@ -3042,6 +3042,10 @@ public SnapshotInfo getSnapshotInfo(String volumeName, String bucketName, ResolvedBucket resolvedBucket = resolveBucketLink(Pair.of(volumeName, bucketName)); auditMap = buildAuditMap(resolvedBucket.realVolume()); auditMap.put(OzoneConsts.BUCKET, resolvedBucket.realBucket()); + if (isAclEnabled) { + omMetadataReader.checkAcls(ResourceType.BUCKET, StoreType.OZONE, + ACLType.READ, resolvedBucket.realVolume(), resolvedBucket.realBucket(), null); + } SnapshotInfo snapshotInfo = metadataManager.getSnapshotInfo(resolvedBucket.realVolume(), resolvedBucket.realBucket(), snapshotName); @@ -4995,6 +4999,10 @@ public SnapshotDiffResponse snapshotDiff(String volume, // Updating the volumeName & bucketName in case the bucket is a linked bucket. We need to do this before a // permission check, since linked bucket permissions and source bucket permissions could be different. ResolvedBucket resolvedBucket = resolveBucketLink(Pair.of(volume, bucket), false); + if (isAclEnabled) { + omMetadataReader.checkAcls(ResourceType.BUCKET, StoreType.OZONE, + ACLType.READ, resolvedBucket.realVolume(), resolvedBucket.realBucket(), null); + } return omSnapshotManager.getSnapshotDiffReport(resolvedBucket.realVolume(), resolvedBucket.realBucket(), fromSnapshot, toSnapshot, token, pageSize, forceFullDiff, disableNativeDiff); } @@ -5006,6 +5014,10 @@ public CancelSnapshotDiffResponse cancelSnapshotDiff(String volume, String toSnapshot) throws IOException { ResolvedBucket resolvedBucket = this.resolveBucketLink(Pair.of(volume, bucket), false); + if (isAclEnabled) { + omMetadataReader.checkAcls(ResourceType.BUCKET, StoreType.OZONE, + ACLType.READ, resolvedBucket.realVolume(), resolvedBucket.realBucket(), null); + } return omSnapshotManager.cancelSnapshotDiff(resolvedBucket.realVolume(), resolvedBucket.realBucket(), fromSnapshot, toSnapshot); } From be5505b5274734e6ba2f6b4346a56566ad609333 Mon Sep 17 00:00:00 2001 From: Wei-Chiu Chuang Date: Thu, 8 May 2025 17:56:40 -0700 Subject: [PATCH 2/3] Remove unused code. Change-Id: I368860c6795cc0ceb7dfd84bf18b85be916645c4 --- .../hadoop/ozone/om/snapshot/TestOzoneManagerSnapshotAcl.java | 4 ---- 1 file changed, 4 deletions(-) diff --git a/hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/ozone/om/snapshot/TestOzoneManagerSnapshotAcl.java b/hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/ozone/om/snapshot/TestOzoneManagerSnapshotAcl.java index 99f85efff795..261004423b4f 100644 --- a/hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/ozone/om/snapshot/TestOzoneManagerSnapshotAcl.java +++ b/hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/ozone/om/snapshot/TestOzoneManagerSnapshotAcl.java @@ -69,8 +69,6 @@ import org.junit.jupiter.params.provider.Arguments; import org.junit.jupiter.params.provider.EnumSource; import org.junit.jupiter.params.provider.MethodSource; -import org.slf4j.Logger; -import org.slf4j.LoggerFactory; /** * Test for Snapshot feature with ACL. @@ -96,8 +94,6 @@ public class TestOzoneManagerSnapshotAcl { UserGroupInformation.createUserForTesting(USER3, new String[] {GROUP3}); private static final OzoneObj.ResourceType RESOURCE_TYPE_KEY = OzoneObj.ResourceType.KEY; - private static final Logger LOG = - LoggerFactory.getLogger(TestOzoneManagerSnapshotAcl.class); private static MiniOzoneCluster cluster; private static ObjectStore objectStore; private static OzoneManager ozoneManager; From 53617a1ef5e9fd443f56e671b583e0341ec151db Mon Sep 17 00:00:00 2001 From: Wei-Chiu Chuang Date: Thu, 8 May 2025 18:42:49 -0700 Subject: [PATCH 3/3] Add javadocs. Change-Id: I438ac4e2531030f4474515d5e147d877bfea162c --- .../snapshot/TestOzoneManagerSnapshotAcl.java | 118 ++++++++---------- 1 file changed, 54 insertions(+), 64 deletions(-) diff --git a/hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/ozone/om/snapshot/TestOzoneManagerSnapshotAcl.java b/hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/ozone/om/snapshot/TestOzoneManagerSnapshotAcl.java index 261004423b4f..f735ad15d295 100644 --- a/hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/ozone/om/snapshot/TestOzoneManagerSnapshotAcl.java +++ b/hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/ozone/om/snapshot/TestOzoneManagerSnapshotAcl.java @@ -36,8 +36,6 @@ import java.util.stream.Stream; import org.apache.commons.lang3.RandomStringUtils; import org.apache.hadoop.fs.CommonConfigurationKeysPublic; -import org.apache.hadoop.fs.FileSystem; -import org.apache.hadoop.fs.Path; import org.apache.hadoop.hdds.conf.OzoneConfiguration; import org.apache.hadoop.hdds.scm.HddsWhiteboxTestUtils; import org.apache.hadoop.hdds.utils.IOUtils; @@ -449,6 +447,16 @@ public void testLookupKeyWithNotAllowedUserForPrefixAcl(BucketLayout bucketLayou assertDoesNotThrow(() -> ozoneManager.lookupKey(keyArgs)); } + /** + * Verifies that bucket owner can: create, rename and delete snapshots. + * Verifies that non bucket owner can not create, rename and delete. + * Verifies that user with bucket read permissions can: list snapshots, get snapshot info, + * snapshot diff, list snapshot diff jobs and cancel snapshot diff jobs. + * Verifies that user with no permissions cannot do any of the above. + * + * @param bucketLayout + * @throws Exception + */ @ParameterizedTest @EnumSource(BucketLayout.class) public void testSnapshotPermissions(BucketLayout bucketLayout) throws Exception { @@ -457,43 +465,60 @@ public void testSnapshotPermissions(BucketLayout bucketLayout) throws Exception // create a bucket whose owner is user1 final OzoneVolume volume = objectStore.getVolume(volumeName); createBucket(bucketLayout, volume, USER1); - // allow user2 volume read and bucket read and list permissions. + // allow user2 volume read, and bucket read and list permissions. setDefaultVolumeAcls(); setBucketAcl(); - String bucket1Name = bucketName; - String snapshot1 = - "snapshot-" + RandomStringUtils.secure().nextNumeric(32); - objectStore.createSnapshot(volumeName, bucketName, snapshot1); - String snapshot2 = - "snapshot-" + RandomStringUtils.secure().nextNumeric(32); - objectStore.createSnapshot(volumeName, bucketName, snapshot2); + ObjectStore objectStore1 = UGI1.doAs( + (PrivilegedExceptionAction)() -> cluster.newClient().getObjectStore()); + String snapshot1 = "snapshot-" + RandomStringUtils.secure().nextNumeric(32); + objectStore1.createSnapshot(volumeName, bucketName, snapshot1); + String snapshot2 = "snapshot-" + RandomStringUtils.secure().nextNumeric(32); + objectStore1.createSnapshot(volumeName, bucketName, snapshot2); + String snapshot3 = "snapshot-" + RandomStringUtils.secure().nextNumeric(32); + objectStore1.createSnapshot(volumeName, bucketName, snapshot3); + String snapshot4 = "snapshot-" + RandomStringUtils.secure().nextNumeric(32); + + objectStore1.renameSnapshot(volumeName, bucketName, snapshot3, snapshot4); + objectStore1.deleteSnapshot(volumeName, bucketName, snapshot4); + + objectStore1.listSnapshot(volumeName, bucketName, null, null); + objectStore1.getSnapshotInfo(volumeName, bucketName, snapshot1); + objectStore1.snapshotDiff(volumeName, bucketName, + snapshot1, snapshot2, null, 0, false, false); + objectStore1.listSnapshotDiffJobs(volumeName, bucketName, "", true, null); + objectStore1.cancelSnapshotDiff(volumeName, bucketName, snapshot1, snapshot2); + + ObjectStore objectStore2 = UGI2.doAs( + (PrivilegedExceptionAction)() -> cluster.newClient().getObjectStore()); + // user2 should not be able to create a snapshot in bucket1, + // should not be able to rename in it, delete in it. + OMException ex = assertThrows(OMException.class, + () -> objectStore2.createSnapshot(volumeName, bucketName, snapshot1)); + assertEquals(OMException.ResultCodes.PERMISSION_DENIED, ex.getResult()); + + ex = assertThrows(OMException.class, + () -> objectStore2.renameSnapshot(volumeName, bucketName, snapshot1, snapshot2)); + assertEquals(OMException.ResultCodes.PERMISSION_DENIED, ex.getResult()); + + ex = assertThrows(OMException.class, + () -> objectStore2.deleteSnapshot(volumeName, bucketName, snapshot1)); + assertEquals(OMException.ResultCodes.PERMISSION_DENIED, ex.getResult()); // user2 should be able to list the snapshots, get snapshot info, // snapshot diff, list snapshot diff jobs and cancel snapshot diff jobs - UGI2.doAs( - (PrivilegedExceptionAction)() - -> { - OzoneClient client2 = cluster.newClient(); - ObjectStore objectStore2 = client2.getObjectStore(); - objectStore2.listSnapshot(volumeName, bucketName, null, null); - objectStore2.getSnapshotInfo(volumeName, bucketName, snapshot1); - objectStore2.snapshotDiff(volumeName, bucketName, + objectStore2.listSnapshot(volumeName, bucketName, null, null); + objectStore2.getSnapshotInfo(volumeName, bucketName, snapshot1); + objectStore2.snapshotDiff(volumeName, bucketName, snapshot1, snapshot2, null, 0, false, false); - objectStore2.listSnapshotDiffJobs(volumeName, bucketName, "", true, null); - objectStore2.cancelSnapshotDiff(volumeName, bucketName, snapshot1, snapshot2); - return null; - }); + objectStore2.listSnapshotDiffJobs(volumeName, bucketName, "", true, null); + objectStore2.cancelSnapshotDiff(volumeName, bucketName, snapshot1, snapshot2); - // user3 has no permission, should not be able to list the snapshots, get snapshot info, + // user3 has no ACL permissions, should not be able to list the snapshots, get snapshot info, // snapshot diff, list snapshot diff jobs and cancel snapshot diff jobs. ObjectStore objectStore3 = UGI3.doAs( - (PrivilegedExceptionAction)() - -> { - OzoneClient client2 = cluster.newClient(); - return client2.getObjectStore(); - }); - OMException ex = assertThrows(OMException.class, + (PrivilegedExceptionAction)() -> cluster.newClient().getObjectStore()); + ex = assertThrows(OMException.class, () -> objectStore3.listSnapshot(volumeName, bucketName, null, null)); assertEquals(OMException.ResultCodes.PERMISSION_DENIED, ex.getResult()); @@ -514,41 +539,6 @@ public void testSnapshotPermissions(BucketLayout bucketLayout) throws Exception () -> objectStore3.cancelSnapshotDiff(volumeName, bucketName, snapshot1, snapshot2)); assertEquals(OMException.ResultCodes.PERMISSION_DENIED, ex.getResult()); - // create a bucket whose owner is user2 - createBucket(bucketLayout, volume, USER2); - String bucket2Name = bucketName; - - // user2 should not be able to create a snapshot on bucket1 but should - // be able to create a snapshot on bucket2, rename it and delete it - final FileSystem fs2 = UGI2.doAs( - (PrivilegedExceptionAction)() - -> FileSystem.get(cluster.getConf())); - final String snapshotPrefix = "snapshot-"; - final String snapshotName = - snapshotPrefix + RandomStringUtils.secure().nextNumeric(32); - ex = assertThrows(OMException.class, () -> { - fs2.createSnapshot(new Path("/" + volumeName + "/" + bucket1Name), snapshotName); - }); - assertEquals(OMException.ResultCodes.PERMISSION_DENIED, ex.getResult()); - Path bucket2Path = new Path("/" + volumeName + "/" + bucket2Name); - fs2.createSnapshot(bucket2Path, snapshotName); - final String snapshotNameNew = snapshotName + ".new"; - fs2.renameSnapshot(bucket2Path, snapshotName, snapshotNameNew); - fs2.deleteSnapshot(bucket2Path, snapshotNameNew); - - // user3 should not be able to create a snapshot on bucket2 - // and should not be able to rename it, delete it - ex = assertThrows(OMException.class, - () -> objectStore3.createSnapshot(volumeName, bucket2Name, snapshotName)); - assertEquals(OMException.ResultCodes.PERMISSION_DENIED, ex.getResult()); - - ex = assertThrows(OMException.class, - () -> objectStore3.renameSnapshot(volumeName, bucket2Name, snapshotName, snapshotNameNew)); - assertEquals(OMException.ResultCodes.PERMISSION_DENIED, ex.getResult()); - - ex = assertThrows(OMException.class, - () -> objectStore3.deleteSnapshot(volumeName, bucket2Name, snapshotName)); - assertEquals(OMException.ResultCodes.PERMISSION_DENIED, ex.getResult()); } private void setup(BucketLayout bucketLayout)