From 17b24e4e8c4c2d3fc9af2c53a1da32650a67311f Mon Sep 17 00:00:00 2001 From: Fabian Morgan Date: Fri, 6 Mar 2026 14:06:56 -0800 Subject: [PATCH 1/4] authorize against LIST instead of READ for STS requests Conflicts: hadoop-ozone/ozone-manager/src/test/java/org/apache/hadoop/ozone/om/TestOMMetadataReader.java --- .../hadoop/ozone/om/OmMetadataReader.java | 40 +- .../hadoop/ozone/om/TestOMMetadataReader.java | 408 ++++++++++++++++-- 2 files changed, 409 insertions(+), 39 deletions(-) diff --git a/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/OmMetadataReader.java b/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/OmMetadataReader.java index 8ce694993780..be28e3312cc2 100644 --- a/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/OmMetadataReader.java +++ b/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/OmMetadataReader.java @@ -234,8 +234,24 @@ public List listStatus(OmKeyArgs args, boolean recursive, try { if (isAclEnabled) { - checkAcls(getResourceType(args), StoreType.OZONE, ACLType.READ, - bucket, args.getKeyName()); + if (isStsS3Request()) { + // We need to be able to tell the difference between being able to download a file and merely seeing the file + // name in a list. Use READ for download ability and LIST (here) for listing. + // When keyName is empty (root listing), use listPrefix for auth if set (e.g. from S3 shallow list with + // prefix). Otherwise fall back to "*" which requires full bucket LIST permission. + final String aclKey; + if (args.getKeyName() != null && !args.getKeyName().isEmpty()) { + aclKey = args.getKeyName(); + } else if (args.getListPrefix() != null && !args.getListPrefix().isEmpty()) { + aclKey = args.getListPrefix(); + } else { + aclKey = "*"; + } + checkAcls(ResourceType.KEY, StoreType.OZONE, ACLType.LIST, bucket.realVolume(), bucket.realBucket(), aclKey); + } else { + checkAcls(getResourceType(args), StoreType.OZONE, ACLType.READ, + bucket, args.getKeyName()); + } } metrics.incNumListStatus(); return keyManager.listStatus(args, recursive, startKey, @@ -277,8 +293,12 @@ public OzoneFileStatus getFileStatus(OmKeyArgs args) throws IOException { try { if (isAclEnabled) { - checkAcls(getResourceType(args), StoreType.OZONE, ACLType.READ, - bucket, args.getKeyName()); + if (isStsS3Request()) { + checkAcls(getResourceType(args), StoreType.OZONE, ACLType.LIST, bucket, args.getKeyName()); + } else { + checkAcls(getResourceType(args), StoreType.OZONE, ACLType.READ, + bucket, args.getKeyName()); + } } metrics.incNumGetFileStatus(); return keyManager.getFileStatus(args, getClientAddress()); @@ -347,6 +367,14 @@ public ListKeysResult listKeys(String volumeName, String bucketName, checkAcls(ResourceType.BUCKET, StoreType.OZONE, ACLType.LIST, bucket.realVolume(), bucket.realBucket(), keyPrefix) ); + + if (isStsS3Request()) { + // With STS we must check acl on the prefix to be compliant with AWS + final String aclKey = (keyPrefix == null || keyPrefix.isEmpty()) ? "*" : keyPrefix; + captureLatencyNs( + perfMetrics.getListKeysAclCheckLatencyNs(), () -> checkAcls( + ResourceType.KEY, StoreType.OZONE, ACLType.LIST, bucket.realVolume(), bucket.realBucket(), aclKey)); + } } metrics.incNumKeyLists(); return keyManager.listKeys(bucket.realVolume(), bucket.realBucket(), @@ -698,6 +726,10 @@ public boolean isNativeAuthorizerEnabled() { return accessAuthorizer.isNative(); } + private boolean isStsS3Request() { + return getS3Auth() != null && OzoneManager.getStsTokenIdentifier() != null; + } + private ResourceType getResourceType(OmKeyArgs args) { if (args.getKeyName() == null || args.getKeyName().isEmpty()) { return ResourceType.BUCKET; diff --git a/hadoop-ozone/ozone-manager/src/test/java/org/apache/hadoop/ozone/om/TestOMMetadataReader.java b/hadoop-ozone/ozone-manager/src/test/java/org/apache/hadoop/ozone/om/TestOMMetadataReader.java index adfaa5d6cf03..f45277a06a7b 100644 --- a/hadoop-ozone/ozone-manager/src/test/java/org/apache/hadoop/ozone/om/TestOMMetadataReader.java +++ b/hadoop-ozone/ozone-manager/src/test/java/org/apache/hadoop/ozone/om/TestOMMetadataReader.java @@ -17,20 +17,41 @@ package org.apache.hadoop.ozone.om; +import static org.apache.hadoop.ozone.om.helpers.BucketLayout.FILE_SYSTEM_OPTIMIZED; +import static org.apache.hadoop.ozone.security.acl.IAccessAuthorizer.ACLType.LIST; +import static org.apache.hadoop.ozone.security.acl.IAccessAuthorizer.ACLType.READ; +import static org.apache.hadoop.ozone.security.acl.OzoneObj.ResourceType.KEY; +import static org.apache.hadoop.ozone.security.acl.OzoneObj.ResourceType.VOLUME; import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertFalse; import static org.junit.jupiter.api.Assertions.assertTrue; import static org.mockito.ArgumentMatchers.any; import static org.mockito.ArgumentMatchers.eq; import static org.mockito.Mockito.mock; import static org.mockito.Mockito.mockStatic; +import static org.mockito.Mockito.reset; +import static org.mockito.Mockito.times; import static org.mockito.Mockito.verify; import static org.mockito.Mockito.when; import io.grpc.Context; +import java.io.IOException; import java.net.InetAddress; +import java.net.InetSocketAddress; +import java.util.ArrayList; +import java.util.Collections; +import java.util.List; +import org.apache.commons.lang3.tuple.Pair; +import org.apache.hadoop.hdds.conf.OzoneConfiguration; import org.apache.hadoop.ipc_.Server; +import org.apache.hadoop.metrics2.lib.MutableRate; import org.apache.hadoop.ozone.audit.AuditLogger; import org.apache.hadoop.ozone.om.exceptions.OMException; +import org.apache.hadoop.ozone.om.helpers.ListKeysResult; +import org.apache.hadoop.ozone.om.helpers.OmKeyArgs; +import org.apache.hadoop.ozone.om.helpers.OzoneFileStatus; +import org.apache.hadoop.ozone.om.protocolPB.grpc.GrpcClientConstants; +import org.apache.hadoop.ozone.protocol.proto.OzoneManagerProtocolProtos.S3Authentication; import org.apache.hadoop.ozone.security.STSTokenIdentifier; import org.apache.hadoop.ozone.security.acl.IAccessAuthorizer; import org.apache.hadoop.ozone.security.acl.OzoneObj; @@ -48,17 +69,20 @@ */ public class TestOMMetadataReader { + private static final String ACCESS_KEY_ID = "ASIA7O1AJD8VV4KCEAX5"; + private static final String VOLUME_NAME = "s3v"; + private static final String BUCKET_NAME = "bucket123"; + private static final String KEY_PREFIX = "prefix"; + private static final long MAX_KEYS = 100L; + @AfterEach public void clearStsThreadLocal() { OzoneManager.setStsTokenIdentifier(null); } @Test - public void testGetClientAddress() { - try ( - MockedStatic ipcServerStaticMock = mockStatic(Server.class); - MockedStatic grpcRequestContextStaticMock = mockStatic(Context.class); - ) { + public void testGetClientAddress() throws Exception { + try (MockedStatic ipcServerStaticMock = mockStatic(Server.class)) { // given String expectedClientAddressInCaseOfHadoopRpcCall = "hadoop.ipc.client.com"; @@ -66,15 +90,11 @@ public void testGetClientAddress() { .thenReturn(null, null, expectedClientAddressInCaseOfHadoopRpcCall); String expectedClientAddressInCaseOfGrpcCall = "172.45.23.4"; - Context.Key clientIpAddressKey = mock(Context.Key.class); - when(clientIpAddressKey.get()) - .thenReturn(expectedClientAddressInCaseOfGrpcCall, null); - - grpcRequestContextStaticMock.when(() -> Context.key("CLIENT_IP_ADDRESS")) - .thenReturn(clientIpAddressKey); - // when (GRPC call with defined client address) - String clientAddress = OmMetadataReader.getClientAddress(); + String clientAddress = Context.current() + .withValue(GrpcClientConstants.CLIENT_IP_ADDRESS_CTX_KEY, + expectedClientAddressInCaseOfGrpcCall) + .call(OmMetadataReader::getClientAddress); // then assertEquals(expectedClientAddressInCaseOfGrpcCall, clientAddress); @@ -93,12 +113,12 @@ public void testGetClientAddress() { @Test public void testCheckAclsAttachesSessionPolicyFromThreadLocal() throws Exception { final String sessionPolicy = "session-policy-from-thread-local"; - setupStsTokenIdentifier(sessionPolicy); + setupStsTokenIdentifier(); final IAccessAuthorizer accessAuthorizer = createMockIAccessAuthorizerReturningTrue(); final OmMetadataReader omMetadataReader = createMetadataReader(accessAuthorizer); - final RequestContext contextWithoutSessionPolicy = createTestRequestContext(null); + final RequestContext contextWithoutSessionPolicy = createTestRequestContext(); final OzoneObj obj = createTestOzoneObj(); assertTrue(omMetadataReader.checkAcls(obj, contextWithoutSessionPolicy, true)); @@ -114,7 +134,7 @@ public void testNoSessionPolicyWhenThreadLocalIsNull() throws Exception { final IAccessAuthorizer accessAuthorizer = createMockIAccessAuthorizerReturningTrue(); final OmMetadataReader omMetadataReader = createMetadataReader(accessAuthorizer); - final RequestContext contextWithoutSessionPolicy = createTestRequestContext(null); + final RequestContext contextWithoutSessionPolicy = createTestRequestContext(); final OzoneObj obj = createTestOzoneObj(); assertTrue(omMetadataReader.checkAcls(obj, contextWithoutSessionPolicy, true)); @@ -122,25 +142,222 @@ public void testNoSessionPolicyWhenThreadLocalIsNull() throws Exception { verifySessionPolicyPassedToAuthorizer(accessAuthorizer, obj, null); } - private OmMetadataReader createMetadataReader(IAccessAuthorizer accessAuthorizer) { + @Test + public void testListStatusUsesListAclForStsS3Request() throws Exception { + setupStsS3Request(); + + final IAccessAuthorizer accessAuthorizer = createMockIAccessAuthorizerReturningTrue(); + final KeyManager keyManager = createListStatusKeyManagerReturningEmpty(); + + final OmMetadataReader omMetadataReader = createMetadataReader(accessAuthorizer, keyManager); + final OmKeyArgs args = createOmKeyArgs(); + + final List statuses = omMetadataReader.listStatus(args, false, "", MAX_KEYS, false); + assertTrue(statuses.isEmpty()); + + final List checks = captureAclChecks(accessAuthorizer, 2); + + // For STS S3 requests, listStatus() performs these checks: + // 1. Volume READ (for volume access) + // 2) Key LIST (for the specific prefix being listed) - we need LIST permission for STS in order to tell whether the + // file should be listed only or downloadable (downloadable would be READ) + assertContainsVolumeReadCheck(checks); + assertContainsKeyListCheckWithName(checks, KEY_PREFIX); + } + + @Test + public void testListStatusUsesReadAclForNonStsRequest() throws Exception { + setupNonStsS3Request(); + + final IAccessAuthorizer accessAuthorizer = createMockIAccessAuthorizerReturningTrue(); + final KeyManager keyManager = createListStatusKeyManagerReturningEmpty(); + + final OmMetadataReader omMetadataReader = createMetadataReader(accessAuthorizer, keyManager); + final OmKeyArgs args = createOmKeyArgs(); + + final List statuses = omMetadataReader.listStatus(args, false, "", MAX_KEYS, false); + assertTrue(statuses.isEmpty()); + + final List checks = captureAclChecks(accessAuthorizer, 2); + assertTrue(checks.stream().allMatch(check -> check.getContext().getAclRights() == READ)); + + assertContainsVolumeReadCheck(checks); + // We want to ensure the current behavior for non-STS requests remains the same + assertContainsKeyReadCheckWithName(checks); + assertDoesNotContainKeyListCheck(checks); + } + + @Test + public void testListStatusUsesListPrefixForAclWhenKeyNameEmptyAndListPrefixSet() throws Exception { + setupStsS3Request(); + + final IAccessAuthorizer accessAuthorizer = createMockIAccessAuthorizerReturningTrue(); + final KeyManager keyManager = createListStatusKeyManagerReturningEmpty(); + + final OmMetadataReader omMetadataReader = createMetadataReader(accessAuthorizer, keyManager); + final OmKeyArgs args = new OmKeyArgs.Builder() + .setVolumeName(VOLUME_NAME) + .setBucketName(BUCKET_NAME) + .setKeyName("") + .setListPrefix("userA/") + .build(); + + final List statuses = omMetadataReader.listStatus(args, false, "", MAX_KEYS, false); + assertTrue(statuses.isEmpty()); + + final List checks = captureAclChecks(accessAuthorizer, 2); + assertContainsVolumeReadCheck(checks); + assertContainsKeyListCheckWithName(checks, "userA/"); + } + + @Test + public void testListStatusUsesWildcardForAclWhenKeyNameAndListPrefixEmpty() throws Exception { + setupStsS3Request(); + + final IAccessAuthorizer accessAuthorizer = createMockIAccessAuthorizerReturningTrue(); + final KeyManager keyManager = createListStatusKeyManagerReturningEmpty(); + + final OmMetadataReader omMetadataReader = createMetadataReader(accessAuthorizer, keyManager); + final OmKeyArgs args = new OmKeyArgs.Builder() + .setVolumeName(VOLUME_NAME) + .setBucketName(BUCKET_NAME) + .setKeyName("") + .build(); + + final List statuses = omMetadataReader.listStatus(args, false, "", MAX_KEYS, false); + assertTrue(statuses.isEmpty()); + + final List checks = captureAclChecks(accessAuthorizer, 2); + assertContainsVolumeReadCheck(checks); + assertContainsKeyListCheckWithName(checks, "*"); + } + + @Test + public void testListStatusKeyNameTakesPrecedenceOverListPrefix() throws Exception { + setupStsS3Request(); + + final IAccessAuthorizer accessAuthorizer = createMockIAccessAuthorizerReturningTrue(); + final KeyManager keyManager = createListStatusKeyManagerReturningEmpty(); + + final OmMetadataReader omMetadataReader = createMetadataReader(accessAuthorizer, keyManager); + final OmKeyArgs args = new OmKeyArgs.Builder() + .setVolumeName(VOLUME_NAME) + .setBucketName(BUCKET_NAME) + .setKeyName(KEY_PREFIX) + .setListPrefix("other/") + .build(); + + final List statuses = omMetadataReader.listStatus(args, false, "", MAX_KEYS, false); + assertTrue(statuses.isEmpty()); + + final List checks = captureAclChecks(accessAuthorizer, 2); + assertContainsVolumeReadCheck(checks); + assertContainsKeyListCheckWithName(checks, KEY_PREFIX); + } + + @Test + public void testGetFileStatusUsesListAclForStsS3Request() throws Exception { + setupStsS3Request(); + + final IAccessAuthorizer accessAuthorizer = createMockIAccessAuthorizerReturningTrue(); + final KeyManager keyManager = createGetFileStatusKeyManagerReturningStatus(); + + final OmMetadataReader omMetadataReader = createMetadataReader(accessAuthorizer, keyManager); + final OmKeyArgs args = createOmKeyArgs(); + + omMetadataReader.getFileStatus(args); + + final List checks = captureAclChecks(accessAuthorizer, 2); + assertContainsVolumeReadCheck(checks); + assertContainsKeyListCheckWithName(checks, KEY_PREFIX); + assertDoesNotContainKeyReadCheck(checks); + } + + @Test + public void testGetFileStatusUsesReadAclForNonStsS3Request() throws Exception { + setupNonStsS3Request(); + + final IAccessAuthorizer accessAuthorizer = createMockIAccessAuthorizerReturningTrue(); + final KeyManager keyManager = createGetFileStatusKeyManagerReturningStatus(); + + final OmMetadataReader omMetadataReader = createMetadataReader(accessAuthorizer, keyManager); + final OmKeyArgs args = createOmKeyArgs(); + + omMetadataReader.getFileStatus(args); + + final List checks = captureAclChecks(accessAuthorizer, 2); + assertContainsVolumeReadCheck(checks); + assertContainsKeyReadCheckWithName(checks); + assertDoesNotContainKeyListCheck(checks); + } + + @Test + public void testListKeysUsesPrefixCheckForStsS3Request() throws Exception { + setupStsS3Request(); + + final IAccessAuthorizer accessAuthorizer = createMockIAccessAuthorizerReturningTrue(); + final KeyManager keyManager = createListKeysKeyManagerReturningEmpty(); + + final OmMetadataReader omMetadataReader = createMetadataReader(accessAuthorizer, keyManager); + + // Case 1: List with prefix "userA/" + omMetadataReader.listKeys(VOLUME_NAME, BUCKET_NAME, "", "userA/", (int) MAX_KEYS); + + List checks = captureAclChecks(accessAuthorizer, 4); + assertContainsBucketListCheck(checks); + assertContainsKeyListCheckWithName(checks, "userA/"); + + // Reset to make case 2 assertions independent of case 1 captures. + reset(accessAuthorizer); + reenableAllowAllAccessChecks(accessAuthorizer); + + // Case 2: List with empty prefix (should check "*") + omMetadataReader.listKeys(VOLUME_NAME, BUCKET_NAME, "", "", (int) MAX_KEYS); + + checks = captureAclChecks(accessAuthorizer, 4); + assertContainsBucketListCheck(checks); + assertContainsKeyListCheckWithName(checks, "*"); + } + + private OmMetadataReader createMetadataReader(IAccessAuthorizer accessAuthorizer) throws IOException { + return createMetadataReader(accessAuthorizer, mock(KeyManager.class)); + } + + private OmMetadataReader createMetadataReader(IAccessAuthorizer accessAuthorizer, KeyManager keyManager) + throws IOException { final OzoneManager ozoneManager = mock(OzoneManager.class); when(ozoneManager.getBucketManager()).thenReturn(mock(BucketManager.class)); when(ozoneManager.getVolumeManager()).thenReturn(mock(VolumeManager.class)); + when(ozoneManager.getConfiguration()).thenReturn(new OzoneConfiguration()); when(ozoneManager.getAclsEnabled()).thenReturn(true); - when(ozoneManager.getPerfMetrics()).thenReturn(mock(OMPerformanceMetrics.class)); + final OMPerformanceMetrics perfMetrics = mock(OMPerformanceMetrics.class); + // OmMetadataReader uses these MutableRate metrics via MetricUtil.captureLatencyNs(...). + when(perfMetrics.getListKeysResolveBucketLatencyNs()).thenReturn(mock(MutableRate.class)); + when(perfMetrics.getListKeysAclCheckLatencyNs()).thenReturn(mock(MutableRate.class)); + when(ozoneManager.getPerfMetrics()).thenReturn(perfMetrics); + when(ozoneManager.getVolumeOwner(any(), any(), any())).thenReturn("volume-owner"); + when(ozoneManager.getBucketOwner(any(), any(), any(), any())).thenReturn("bucket-owner"); + when(ozoneManager.getOmRpcServerAddr()).thenReturn(new InetSocketAddress("127.0.0.1", 9874)); + when(ozoneManager.resolveBucketLink(any(Pair.class))) + .thenReturn( + new ResolvedBucket( + VOLUME_NAME, BUCKET_NAME, VOLUME_NAME, BUCKET_NAME, "bucket-owner", FILE_SYSTEM_OPTIMIZED)); + when(ozoneManager.resolveBucketLink(any(OmKeyArgs.class))) + .thenReturn( + new ResolvedBucket( + VOLUME_NAME, BUCKET_NAME, VOLUME_NAME, BUCKET_NAME, "bucket-owner", FILE_SYSTEM_OPTIMIZED)); return new OmMetadataReader( - mock(KeyManager.class), mock(PrefixManager.class), ozoneManager, mock(Logger.class), mock(AuditLogger.class), + keyManager, mock(PrefixManager.class), ozoneManager, mock(Logger.class), mock(AuditLogger.class), mock(OmMetadataReaderMetrics.class), accessAuthorizer); } /** - * Creates and sets a mock STSTokenIdentifier with the given session policy in the thread-local. - * @param sessionPolicy the session policy to return, or null + * Creates and sets a mock STSTokenIdentifier with a session policy in the thread-local. */ - private void setupStsTokenIdentifier(String sessionPolicy) { + private void setupStsTokenIdentifier() { final STSTokenIdentifier stsTokenIdentifier = mock(STSTokenIdentifier.class); - when(stsTokenIdentifier.getSessionPolicy()).thenReturn(sessionPolicy); + when(stsTokenIdentifier.getSessionPolicy()).thenReturn("session-policy-from-thread-local"); OzoneManager.setStsTokenIdentifier(stsTokenIdentifier); } @@ -156,23 +373,19 @@ private IAccessAuthorizer createMockIAccessAuthorizerReturningTrue() throws OMEx } /** - * Creates a test RequestContext with the given session policy. - * @param sessionPolicy the session policy to set, or null + * Creates a test RequestContext. + * * @return the constructed RequestContext */ - private RequestContext createTestRequestContext(String sessionPolicy) { + private RequestContext createTestRequestContext() { RequestContext.Builder builder = RequestContext.newBuilder() .setClientUgi(UserGroupInformation.createRemoteUser("testUser")) .setIp(InetAddress.getLoopbackAddress()) .setHost("localhost") .setAclType(IAccessAuthorizer.ACLIdentityType.USER) - .setAclRights(IAccessAuthorizer.ACLType.READ) + .setAclRights(READ) .setOwnerName("owner"); - if (sessionPolicy != null) { - builder.setSessionPolicy(sessionPolicy); - } - return builder.build(); } @@ -182,19 +395,63 @@ private RequestContext createTestRequestContext(String sessionPolicy) { */ private OzoneObj createTestOzoneObj() { return OzoneObjInfo.Builder.newBuilder() - .setResType(OzoneObj.ResourceType.KEY) + .setResType(KEY) .setStoreType(OzoneObj.StoreType.OZONE) - .setVolumeName("vol") - .setBucketName("bucket") + .setVolumeName(VOLUME_NAME) + .setBucketName(BUCKET_NAME) .setKeyName("key") .build(); } + private void setupStsS3Request() { + OzoneManager.setStsTokenIdentifier(mock(STSTokenIdentifier.class)); + OzoneManager.setS3Auth(S3Authentication.newBuilder().setAccessId(TestOMMetadataReader.ACCESS_KEY_ID).build()); + } + + private void setupNonStsS3Request() { + OzoneManager.setStsTokenIdentifier(null); + OzoneManager.setS3Auth(null); + } + + private OmKeyArgs createOmKeyArgs() { + return new OmKeyArgs.Builder() + .setVolumeName(VOLUME_NAME) + .setBucketName(BUCKET_NAME) + .setKeyName(TestOMMetadataReader.KEY_PREFIX) + .build(); + } + + private KeyManager createListStatusKeyManagerReturningEmpty() throws IOException { + final KeyManager keyManager = mock(KeyManager.class); + when(keyManager.listStatus(any(OmKeyArgs.class), eq(false), eq(""), eq(MAX_KEYS), any(), eq(false))) + .thenReturn(Collections.emptyList()); + return keyManager; + } + + private KeyManager createGetFileStatusKeyManagerReturningStatus() throws IOException { + final KeyManager keyManager = mock(KeyManager.class); + when(keyManager.getFileStatus(any(OmKeyArgs.class), any())) + .thenReturn(mock(OzoneFileStatus.class)); + return keyManager; + } + + private KeyManager createListKeysKeyManagerReturningEmpty() throws IOException { + final KeyManager keyManager = mock(KeyManager.class); + when(keyManager.listKeys(any(), any(), any(), any(), eq(100))) + .thenReturn(new ListKeysResult(Collections.emptyList(), false)); + return keyManager; + } + + private void reenableAllowAllAccessChecks(IAccessAuthorizer accessAuthorizer) throws OMException { + when(accessAuthorizer.checkAccess(any(OzoneObj.class), any(RequestContext.class))) + .thenReturn(true); + } + /** * Verifies that the accessAuthorizer received a call to checkAccess with the expected session policy. * @param accessAuthorizer the mock authorizer to verify * @param expectedObj the expected OzoneObj - * @param expectedSessionPolicy the expected session policy (may be null) + * @param expectedSessionPolicy the expected session policy (could be null) */ private void verifySessionPolicyPassedToAuthorizer(IAccessAuthorizer accessAuthorizer, OzoneObj expectedObj, String expectedSessionPolicy) throws OMException { @@ -202,4 +459,85 @@ private void verifySessionPolicyPassedToAuthorizer(IAccessAuthorizer accessAutho verify(accessAuthorizer).checkAccess(eq(expectedObj), captor.capture()); assertEquals(expectedSessionPolicy, captor.getValue().getSessionPolicy()); } + + private List captureAclChecks(IAccessAuthorizer accessAuthorizer, int expectedCheckCount) + throws OMException { + final ArgumentCaptor objCaptor = ArgumentCaptor.forClass(OzoneObj.class); + final ArgumentCaptor ctxCaptor = ArgumentCaptor.forClass(RequestContext.class); + verify(accessAuthorizer, times(expectedCheckCount)).checkAccess(objCaptor.capture(), ctxCaptor.capture()); + return toAclChecks(objCaptor.getAllValues(), ctxCaptor.getAllValues()); + } + + private List toAclChecks(List objs, List contexts) { + assertEquals(objs.size(), contexts.size(), "Captured ACL objects and contexts should align"); + final List checks = new ArrayList<>(); + for (int i = 0; i < objs.size(); i++) { + checks.add(new AclCheck(objs.get(i), contexts.get(i))); + } + return checks; + } + + private void assertContainsVolumeReadCheck(List checks) { + assertTrue(checks.stream().anyMatch(this::isVolumeReadCheck), "Expected a VOLUME READ ACL check"); + } + + private boolean isVolumeReadCheck(AclCheck check) { + return check.getObj().getResourceType() == VOLUME && check.getContext().getAclRights() == READ; + } + + private void assertContainsBucketListCheck(List checks) { + assertTrue( + checks.stream().anyMatch( + check -> check.getObj().getResourceType() == OzoneObj.ResourceType.BUCKET && + check.getContext().getAclRights() == LIST), + "Expected a BUCKET LIST ACL check"); + } + + private void assertContainsKeyListCheckWithName(List checks, String keyName) { + assertTrue( + checks.stream().anyMatch( + check -> check.getObj().getResourceType() == KEY && check.getContext().getAclRights() == LIST && + keyName.equals(check.getObj().getKeyName())), + "Expected a KEY LIST ACL check for key '" + keyName + "'"); + } + + private void assertContainsKeyReadCheckWithName(List checks) { + assertTrue( + checks.stream().anyMatch( + check -> check.getObj().getResourceType() == KEY && check.getContext().getAclRights() == READ && + TestOMMetadataReader.KEY_PREFIX.equals(check.getObj().getKeyName())), + "Expected a KEY READ ACL check for key '" + TestOMMetadataReader.KEY_PREFIX + "'"); + } + + private void assertDoesNotContainKeyReadCheck(List checks) { + assertFalse( + checks.stream().anyMatch( + check -> check.getObj().getResourceType() == KEY && check.getContext().getAclRights() == READ), + "Did not expect a KEY READ ACL check"); + } + + private void assertDoesNotContainKeyListCheck(List checks) { + assertFalse( + checks.stream().anyMatch( + check -> check.getObj().getResourceType() == KEY && check.getContext().getAclRights() == LIST), + "Did not expect a KEY LIST ACL check"); + } + + private static final class AclCheck { + private final OzoneObj obj; + private final RequestContext context; + + private AclCheck(OzoneObj obj, RequestContext context) { + this.obj = obj; + this.context = context; + } + + private OzoneObj getObj() { + return obj; + } + + private RequestContext getContext() { + return context; + } + } } From 8278a67c68e6545d50681507e2110fb99f8ca98d Mon Sep 17 00:00:00 2001 From: Fabian Morgan Date: Mon, 30 Mar 2026 15:43:44 -0700 Subject: [PATCH 2/4] fix bug identified by ChenSammi --- .../hadoop/ozone/om/OmMetadataReader.java | 56 +++++++++++++++---- .../hadoop/ozone/om/TestOMMetadataReader.java | 53 ++++++++++++++++-- 2 files changed, 94 insertions(+), 15 deletions(-) diff --git a/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/OmMetadataReader.java b/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/OmMetadataReader.java index be28e3312cc2..99a65d52b061 100644 --- a/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/OmMetadataReader.java +++ b/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/OmMetadataReader.java @@ -32,6 +32,7 @@ import java.util.List; import java.util.Map; import java.util.stream.Collectors; +import org.apache.commons.lang3.StringUtils; import org.apache.commons.lang3.tuple.Pair; import org.apache.hadoop.ipc_.ProtobufRpcEngine; import org.apache.hadoop.ipc_.Server; @@ -237,13 +238,22 @@ public List listStatus(OmKeyArgs args, boolean recursive, if (isStsS3Request()) { // We need to be able to tell the difference between being able to download a file and merely seeing the file // name in a list. Use READ for download ability and LIST (here) for listing. - // When keyName is empty (root listing), use listPrefix for auth if set (e.g. from S3 shallow list with - // prefix). Otherwise fall back to "*" which requires full bucket LIST permission. + // When listPrefix is set (original S3 ListObjects prefix), authorize LIST on that prefix for the whole + // listing, including FSO traversal where keyName is an internal directory (e.g. userA) under prefix user. + final String listPrefix = args.getListPrefix(); + final String keyName = args.getKeyName(); final String aclKey; - if (args.getKeyName() != null && !args.getKeyName().isEmpty()) { - aclKey = args.getKeyName(); - } else if (args.getListPrefix() != null && !args.getListPrefix().isEmpty()) { - aclKey = args.getListPrefix(); + if (StringUtils.isNotBlank(listPrefix)) { + if (StringUtils.isBlank(keyName)) { + aclKey = listPrefix; + } else if (isStsListPathUnderRequestPrefix(keyName, listPrefix)) { + aclKey = listPrefix; + } else { + throw new OMException( + "STS listStatus: key path does not match authorized list prefix", ResultCodes.PERMISSION_DENIED); + } + } else if (keyName != null && !keyName.isEmpty()) { + aclKey = keyName; } else { aclKey = "*"; } @@ -363,17 +373,22 @@ public ListKeysResult listKeys(String volumeName, String bucketName, try { if (isAclEnabled) { - captureLatencyNs(perfMetrics.getListKeysAclCheckLatencyNs(), () -> - checkAcls(ResourceType.BUCKET, StoreType.OZONE, ACLType.LIST, - bucket.realVolume(), bucket.realBucket(), keyPrefix) - ); - if (isStsS3Request()) { + // Check with key null to ensure there is LIST permission on the bucket + captureLatencyNs( + perfMetrics.getListKeysAclCheckLatencyNs(), () -> checkAcls( + ResourceType.BUCKET, StoreType.OZONE, ACLType.LIST, bucket.realVolume(), bucket.realBucket(), + null)); // With STS we must check acl on the prefix to be compliant with AWS final String aclKey = (keyPrefix == null || keyPrefix.isEmpty()) ? "*" : keyPrefix; captureLatencyNs( perfMetrics.getListKeysAclCheckLatencyNs(), () -> checkAcls( ResourceType.KEY, StoreType.OZONE, ACLType.LIST, bucket.realVolume(), bucket.realBucket(), aclKey)); + } else { + captureLatencyNs(perfMetrics.getListKeysAclCheckLatencyNs(), () -> + checkAcls(ResourceType.BUCKET, StoreType.OZONE, ACLType.LIST, + bucket.realVolume(), bucket.realBucket(), keyPrefix) + ); } } metrics.incNumKeyLists(); @@ -730,6 +745,25 @@ private boolean isStsS3Request() { return getS3Auth() != null && OzoneManager.getStsTokenIdentifier() != null; } + /** + * For STS, {@code listPrefix} is the original S3 ListObjects prefix. Internal FSO listing may call + * {@code listStatus} with {@code keyName} set to a subdirectory (e.g. userA) while the session policy + * still authorizes only the request prefix (e.g. user). This returns true when {@code keyName} is the + * prefix itself, a key path under that prefix, or an ancestor directory on the way to a deeper prefix. + */ + private static boolean isStsListPathUnderRequestPrefix(String keyName, String listPrefix) { + if (StringUtils.isBlank(listPrefix) || StringUtils.isBlank(keyName)) { + return false; + } + if (keyName.equals(listPrefix)) { + return true; + } + if (keyName.startsWith(listPrefix)) { + return true; + } + return listPrefix.startsWith(keyName + "/"); + } + private ResourceType getResourceType(OmKeyArgs args) { if (args.getKeyName() == null || args.getKeyName().isEmpty()) { return ResourceType.BUCKET; diff --git a/hadoop-ozone/ozone-manager/src/test/java/org/apache/hadoop/ozone/om/TestOMMetadataReader.java b/hadoop-ozone/ozone-manager/src/test/java/org/apache/hadoop/ozone/om/TestOMMetadataReader.java index f45277a06a7b..8403d2203e01 100644 --- a/hadoop-ozone/ozone-manager/src/test/java/org/apache/hadoop/ozone/om/TestOMMetadataReader.java +++ b/hadoop-ozone/ozone-manager/src/test/java/org/apache/hadoop/ozone/om/TestOMMetadataReader.java @@ -24,6 +24,7 @@ import static org.apache.hadoop.ozone.security.acl.OzoneObj.ResourceType.VOLUME; import static org.junit.jupiter.api.Assertions.assertEquals; import static org.junit.jupiter.api.Assertions.assertFalse; +import static org.junit.jupiter.api.Assertions.assertThrows; import static org.junit.jupiter.api.Assertions.assertTrue; import static org.mockito.ArgumentMatchers.any; import static org.mockito.ArgumentMatchers.eq; @@ -47,6 +48,7 @@ import org.apache.hadoop.metrics2.lib.MutableRate; import org.apache.hadoop.ozone.audit.AuditLogger; import org.apache.hadoop.ozone.om.exceptions.OMException; +import org.apache.hadoop.ozone.om.exceptions.OMException.ResultCodes; import org.apache.hadoop.ozone.om.helpers.ListKeysResult; import org.apache.hadoop.ozone.om.helpers.OmKeyArgs; import org.apache.hadoop.ozone.om.helpers.OzoneFileStatus; @@ -233,7 +235,7 @@ public void testListStatusUsesWildcardForAclWhenKeyNameAndListPrefixEmpty() thro } @Test - public void testListStatusKeyNameTakesPrecedenceOverListPrefix() throws Exception { + public void testListStatusUsesListPrefixForAclWhenKeyNameIsDescendantOfListPrefix() throws Exception { setupStsS3Request(); final IAccessAuthorizer accessAuthorizer = createMockIAccessAuthorizerReturningTrue(); @@ -243,8 +245,8 @@ public void testListStatusKeyNameTakesPrecedenceOverListPrefix() throws Exceptio final OmKeyArgs args = new OmKeyArgs.Builder() .setVolumeName(VOLUME_NAME) .setBucketName(BUCKET_NAME) - .setKeyName(KEY_PREFIX) - .setListPrefix("other/") + .setKeyName("userA") + .setListPrefix("user") .build(); final List statuses = omMetadataReader.listStatus(args, false, "", MAX_KEYS, false); @@ -252,7 +254,50 @@ public void testListStatusKeyNameTakesPrecedenceOverListPrefix() throws Exceptio final List checks = captureAclChecks(accessAuthorizer, 2); assertContainsVolumeReadCheck(checks); - assertContainsKeyListCheckWithName(checks, KEY_PREFIX); + assertContainsKeyListCheckWithName(checks, "user"); + } + + @Test + public void testListStatusUsesListPrefixForAclWhenKeyNameIsAncestorOfListPrefix() throws Exception { + setupStsS3Request(); + + final IAccessAuthorizer accessAuthorizer = createMockIAccessAuthorizerReturningTrue(); + final KeyManager keyManager = createListStatusKeyManagerReturningEmpty(); + + final OmMetadataReader omMetadataReader = createMetadataReader(accessAuthorizer, keyManager); + final OmKeyArgs args = new OmKeyArgs.Builder() + .setVolumeName(VOLUME_NAME) + .setBucketName(BUCKET_NAME) + .setKeyName("user") + .setListPrefix("user/foo") + .build(); + + final List statuses = omMetadataReader.listStatus(args, false, "", MAX_KEYS, false); + assertTrue(statuses.isEmpty()); + + final List checks = captureAclChecks(accessAuthorizer, 2); + assertContainsVolumeReadCheck(checks); + assertContainsKeyListCheckWithName(checks, "user/foo"); + } + + @Test + public void testListStatusThrowsWhenStsKeyNameNotUnderListPrefix() throws Exception { + setupStsS3Request(); + + final IAccessAuthorizer accessAuthorizer = createMockIAccessAuthorizerReturningTrue(); + final KeyManager keyManager = createListStatusKeyManagerReturningEmpty(); + + final OmMetadataReader omMetadataReader = createMetadataReader(accessAuthorizer, keyManager); + final OmKeyArgs args = new OmKeyArgs.Builder() + .setVolumeName(VOLUME_NAME) + .setBucketName(BUCKET_NAME) + .setKeyName(KEY_PREFIX) + .setListPrefix("other/") + .build(); + + final OMException ex = assertThrows( + OMException.class, () -> omMetadataReader.listStatus(args, false, "", MAX_KEYS, false)); + assertEquals(ResultCodes.PERMISSION_DENIED, ex.getResult()); } @Test From 0b70e05a1ac149dbe35db865338630981935c291 Mon Sep 17 00:00:00 2001 From: Fabian Morgan Date: Wed, 1 Apr 2026 10:07:02 -0700 Subject: [PATCH 3/4] missed commit from previous pr --- .../hadoop/ozone/client/rpc/RpcClient.java | 23 ++++++++++--------- ...ManagerProtocolClientSideTranslatorPB.java | 17 +++++++------- 2 files changed, 21 insertions(+), 19 deletions(-) diff --git a/hadoop-ozone/client/src/main/java/org/apache/hadoop/ozone/client/rpc/RpcClient.java b/hadoop-ozone/client/src/main/java/org/apache/hadoop/ozone/client/rpc/RpcClient.java index 482bb9fd9234..a9075320dbee 100644 --- a/hadoop-ozone/client/src/main/java/org/apache/hadoop/ozone/client/rpc/RpcClient.java +++ b/hadoop-ozone/client/src/main/java/org/apache/hadoop/ozone/client/rpc/RpcClient.java @@ -2253,14 +2253,17 @@ public OzoneOutputStream createFile(String volumeName, String bucketName, } private OmKeyArgs prepareOmKeyArgs(String volumeName, String bucketName, - String keyName) { - return new OmKeyArgs.Builder() + String keyName, String listPrefix) { + final OmKeyArgs.Builder builder = new OmKeyArgs.Builder() .setVolumeName(volumeName) .setBucketName(bucketName) .setKeyName(keyName) .setSortDatanodesInPipeline(topologyAwareReadEnabled) - .setLatestVersionLocation(getLatestVersionLocation) - .build(); + .setLatestVersionLocation(getLatestVersionLocation); + if (listPrefix != null && !listPrefix.isEmpty()) { + builder.setListPrefix(listPrefix); + } + return builder.build(); } @Override @@ -2288,7 +2291,8 @@ public OzoneDataStreamOutput createStreamFile(String volumeName, public List listStatus(String volumeName, String bucketName, String keyName, boolean recursive, String startKey, long numEntries) throws IOException { - OmKeyArgs keyArgs = prepareOmKeyArgs(volumeName, bucketName, keyName); + final OmKeyArgs keyArgs = prepareOmKeyArgs(volumeName, bucketName, keyName, + null); return ozoneManagerClient .listStatus(keyArgs, recursive, startKey, numEntries); } @@ -2297,7 +2301,7 @@ public List listStatus(String volumeName, String bucketName, public List listStatus(String volumeName, String bucketName, String keyName, boolean recursive, String startKey, long numEntries, boolean allowPartialPrefixes) throws IOException { - OmKeyArgs keyArgs = prepareOmKeyArgs(volumeName, bucketName, keyName); + final OmKeyArgs keyArgs = prepareOmKeyArgs(volumeName, bucketName, keyName, null); return ozoneManagerClient .listStatus(keyArgs, recursive, startKey, numEntries, allowPartialPrefixes); @@ -2306,11 +2310,8 @@ public List listStatus(String volumeName, String bucketName, @Override public List listStatusLight(ListStatusLightOptions options) throws IOException { - OmKeyArgs keyArgs = prepareOmKeyArgs(options.getVolumeName(), - options.getBucketName(), options.getKeyName()); - if (options.getListPrefix() != null && !options.getListPrefix().isEmpty()) { - keyArgs = keyArgs.toBuilder().setListPrefix(options.getListPrefix()).build(); - } + final OmKeyArgs keyArgs = prepareOmKeyArgs( + options.getVolumeName(), options.getBucketName(), options.getKeyName(), options.getListPrefix()); if (omVersion.compareTo(OzoneManagerVersion.LIGHTWEIGHT_LIST_STATUS) >= 0) { return ozoneManagerClient.listStatusLight( keyArgs, options.isRecursive(), options.getStartKey(), options.getNumEntries(), diff --git a/hadoop-ozone/common/src/main/java/org/apache/hadoop/ozone/om/protocolPB/OzoneManagerProtocolClientSideTranslatorPB.java b/hadoop-ozone/common/src/main/java/org/apache/hadoop/ozone/om/protocolPB/OzoneManagerProtocolClientSideTranslatorPB.java index 9ca351bbb5a7..b42b0e6ddf87 100644 --- a/hadoop-ozone/common/src/main/java/org/apache/hadoop/ozone/om/protocolPB/OzoneManagerProtocolClientSideTranslatorPB.java +++ b/hadoop-ozone/common/src/main/java/org/apache/hadoop/ozone/om/protocolPB/OzoneManagerProtocolClientSideTranslatorPB.java @@ -2369,8 +2369,8 @@ public List listStatus(OmKeyArgs args, boolean recursive, .setLatestVersionLocation(args.getLatestVersionLocation()) .build(); - ListStatusRequest.Builder listStatusRequestBuilder = createListStatusRequestBuilder(keyArgs, recursive, startKey, - numEntries, allowPartialPrefixes, null); + final ListStatusRequest.Builder listStatusRequestBuilder = createListStatusRequestBuilder( + keyArgs, recursive, startKey, numEntries, allowPartialPrefixes); OMRequest omRequest = createOMRequest(Type.ListStatus) .setListStatusRequest(listStatusRequestBuilder.build()) @@ -2398,8 +2398,12 @@ public List listStatusLight(OmKeyArgs args, .setLatestVersionLocation(true) .build(); - ListStatusRequest.Builder listStatusRequestBuilder = createListStatusRequestBuilder(keyArgs, recursive, startKey, - numEntries, allowPartialPrefixes, args.getListPrefix()); + final ListStatusRequest.Builder listStatusRequestBuilder = createListStatusRequestBuilder( + keyArgs, recursive, startKey, numEntries, allowPartialPrefixes); + final String listPrefix = args.getListPrefix(); + if (listPrefix != null && !listPrefix.isEmpty()) { + listStatusRequestBuilder.setListPrefix(listPrefix); + } OMRequest omRequest = createOMRequest(Type.ListStatusLight) .setListStatusRequest(listStatusRequestBuilder.build()) @@ -2417,7 +2421,7 @@ public List listStatusLight(OmKeyArgs args, } private ListStatusRequest.Builder createListStatusRequestBuilder(KeyArgs keyArgs, boolean recursive, String startKey, - long numEntries, boolean allowPartialPrefixes, String listPrefix) { + long numEntries, boolean allowPartialPrefixes) { ListStatusRequest.Builder listStatusRequestBuilder = ListStatusRequest.newBuilder() .setKeyArgs(keyArgs) @@ -2433,9 +2437,6 @@ private ListStatusRequest.Builder createListStatusRequestBuilder(KeyArgs keyArgs if (allowPartialPrefixes) { listStatusRequestBuilder.setAllowPartialPrefix(allowPartialPrefixes); } - if (listPrefix != null && !listPrefix.isEmpty()) { - listStatusRequestBuilder.setListPrefix(listPrefix); - } return listStatusRequestBuilder; } From 7f2ef273b3fd65c0813170f6d5cb9733105a6bd9 Mon Sep 17 00:00:00 2001 From: Fabian Morgan Date: Wed, 1 Apr 2026 22:35:58 -0700 Subject: [PATCH 4/4] pr review update for ChenSammi: better exception message --- .../main/java/org/apache/hadoop/ozone/om/OmMetadataReader.java | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/OmMetadataReader.java b/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/OmMetadataReader.java index 99a65d52b061..b14f01cf6ba0 100644 --- a/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/OmMetadataReader.java +++ b/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/OmMetadataReader.java @@ -250,7 +250,8 @@ public List listStatus(OmKeyArgs args, boolean recursive, aclKey = listPrefix; } else { throw new OMException( - "STS listStatus: key path does not match authorized list prefix", ResultCodes.PERMISSION_DENIED); + "STS listStatus: key path: " + keyName + " does not match authorized list prefix: " + listPrefix, + ResultCodes.PERMISSION_DENIED); } } else if (keyName != null && !keyName.isEmpty()) { aclKey = keyName;