ivandika3 commented on code in PR #8558:
URL: https://github.com/apache/ozone/pull/8558#discussion_r2145242659
##########
hadoop-ozone/integration-test-s3/src/test/java/org/apache/hadoop/ozone/s3/awssdk/v2/AbstractS3SDKV2Tests.java:
##########
@@ -621,4 +650,591 @@ private void completeMultipartUpload(String bucketName,
String key, String uploa
assertEquals(bucketName, compResponse.bucket());
assertEquals(key, compResponse.key());
}
+
+ @Nested
+ @TestInstance(TestInstance.Lifecycle.PER_CLASS)
+ class S3BucketVerificationConditionTests {
Review Comment:
Nit: Change to `S3BucketOwnershipVerificationConditionsTests`
##########
hadoop-ozone/s3gateway/src/main/java/org/apache/hadoop/ozone/s3/exception/S3ErrorTable.java:
##########
@@ -156,6 +156,10 @@ public final class S3ErrorTable {
"a valid custom EC storage config for if using STANDARD_IA.",
HTTP_BAD_REQUEST);
+ public static final OS3Exception BUCKET_OWNER_MISSMATCH = new OS3Exception(
Review Comment:
Nit: typo `BUCKET_OWNER_MISSMATCH` to `BUCKET_OWNER_MISMATCH`
##########
hadoop-ozone/integration-test-s3/src/test/java/org/apache/hadoop/ozone/s3/awssdk/v2/AbstractS3SDKV2Tests.java:
##########
@@ -621,4 +650,591 @@ private void completeMultipartUpload(String bucketName,
String key, String uploa
assertEquals(bucketName, compResponse.bucket());
assertEquals(key, compResponse.key());
}
+
+ @Nested
+ @TestInstance(TestInstance.Lifecycle.PER_CLASS)
+ class S3BucketVerificationConditionTests {
+
+ private static final String DEFAULT_BUCKET_NAME = "default-bucket";
+ private String correctOwner;
+ private static final String WRONG_OWNER = "wrong-owner";
+ private static final String BUCKET_VERIFICATION_TEST_KEY = "test-key";
+
+ @BeforeAll
+ public void setup() {
+ // create bucket to verify bucket ownership
+ s3Client.createBucket(b -> b.bucket(DEFAULT_BUCKET_NAME));
+ GetBucketAclRequest normalRequest =
GetBucketAclRequest.builder().bucket(DEFAULT_BUCKET_NAME).build();
+ correctOwner =
s3Client.getBucketAcl(normalRequest).owner().displayName();
+
+ // create objects to verify bucket ownership
+ s3Client.putObject(b ->
b.bucket(DEFAULT_BUCKET_NAME).key(BUCKET_VERIFICATION_TEST_KEY),
RequestBody.empty());
+ }
+
+ @Nested
+ class BucketEndpointTests {
+
+ @Test
+ public void testGetBucketAcl() {
+ GetBucketAclRequest correctRequest = GetBucketAclRequest.builder()
+ .bucket(DEFAULT_BUCKET_NAME)
+ .expectedBucketOwner(correctOwner)
+ .build();
+ verifyPassBucketOwnershipVerification(() ->
s3Client.getBucketAcl(correctRequest));
+
+ GetBucketAclRequest wrongRequest = GetBucketAclRequest.builder()
+ .bucket(DEFAULT_BUCKET_NAME)
+ .expectedBucketOwner(WRONG_OWNER)
+ .build();
+ verifyBucketOwnershipVerificationAccessDenied(() ->
s3Client.getBucketAcl(wrongRequest));
+ }
+
+ @Test
+ public void testListObject() {
+ ListObjectsRequest correctRequest = ListObjectsRequest.builder()
+ .bucket(DEFAULT_BUCKET_NAME)
+ .expectedBucketOwner(correctOwner)
+ .build();
+ verifyPassBucketOwnershipVerification(() ->
s3Client.listObjects(correctRequest));
+
+ ListObjectsRequest wrongRequest = ListObjectsRequest.builder()
+ .bucket(DEFAULT_BUCKET_NAME)
+ .expectedBucketOwner(WRONG_OWNER)
+ .build();
+ verifyBucketOwnershipVerificationAccessDenied(() ->
s3Client.listObjects(wrongRequest));
+ }
+
+ @Test
+ public void testListMultipartUploads() {
+ ListMultipartUploadsRequest correctRequest =
ListMultipartUploadsRequest.builder()
+ .bucket(DEFAULT_BUCKET_NAME)
+ .expectedBucketOwner(correctOwner)
+ .build();
+ verifyPassBucketOwnershipVerification(() ->
s3Client.listMultipartUploads(correctRequest));
+
+ ListMultipartUploadsRequest wrongRequest =
ListMultipartUploadsRequest.builder()
+ .bucket(DEFAULT_BUCKET_NAME)
+ .expectedBucketOwner(WRONG_OWNER)
+ .build();
+ verifyBucketOwnershipVerificationAccessDenied(
+ () -> s3Client.listMultipartUploads(wrongRequest));
+ }
+
+ @Test
+ public void testPutAcl() {
Review Comment:
Nit: Rename to `testPutBucketAcl` (standardize with `testGetBucketAcl`)
##########
hadoop-ozone/s3gateway/src/main/java/org/apache/hadoop/ozone/s3/endpoint/ObjectEndpoint.java:
##########
@@ -889,6 +898,9 @@ public Response
completeMultipartUpload(@PathParam("bucket") String bucket,
OmMultipartUploadCompleteInfo omMultipartUploadCompleteInfo;
try {
+ OzoneBucket ozoneBucket = volume.getBucket(bucket);
+ S3Owner.verifyBucketOwnerCondition(headers, bucket,
ozoneBucket.getOwner());
+
Review Comment:
Nit: The following `getClientProtocol().completeMultipartUpload` can be
changed to use `ozoneBucket#completeMultipartUpload` instead.
This is just a preference and can be ignored. Using `getClientProtocol()`
avoids the OM getBucket call, but since it's called anyway, we can use
`OzoneBucket`.
##########
hadoop-ozone/integration-test-s3/src/test/java/org/apache/hadoop/ozone/s3/awssdk/v2/AbstractS3SDKV2Tests.java:
##########
@@ -621,4 +650,591 @@ private void completeMultipartUpload(String bucketName,
String key, String uploa
assertEquals(bucketName, compResponse.bucket());
assertEquals(key, compResponse.key());
}
+
+ @Nested
+ @TestInstance(TestInstance.Lifecycle.PER_CLASS)
+ class S3BucketVerificationConditionTests {
+
+ private static final String DEFAULT_BUCKET_NAME = "default-bucket";
+ private String correctOwner;
+ private static final String WRONG_OWNER = "wrong-owner";
+ private static final String BUCKET_VERIFICATION_TEST_KEY = "test-key";
+
+ @BeforeAll
+ public void setup() {
+ // create bucket to verify bucket ownership
+ s3Client.createBucket(b -> b.bucket(DEFAULT_BUCKET_NAME));
+ GetBucketAclRequest normalRequest =
GetBucketAclRequest.builder().bucket(DEFAULT_BUCKET_NAME).build();
+ correctOwner =
s3Client.getBucketAcl(normalRequest).owner().displayName();
+
+ // create objects to verify bucket ownership
+ s3Client.putObject(b ->
b.bucket(DEFAULT_BUCKET_NAME).key(BUCKET_VERIFICATION_TEST_KEY),
RequestBody.empty());
+ }
+
+ @Nested
+ class BucketEndpointTests {
+
+ @Test
+ public void testGetBucketAcl() {
+ GetBucketAclRequest correctRequest = GetBucketAclRequest.builder()
+ .bucket(DEFAULT_BUCKET_NAME)
+ .expectedBucketOwner(correctOwner)
+ .build();
+ verifyPassBucketOwnershipVerification(() ->
s3Client.getBucketAcl(correctRequest));
+
+ GetBucketAclRequest wrongRequest = GetBucketAclRequest.builder()
+ .bucket(DEFAULT_BUCKET_NAME)
+ .expectedBucketOwner(WRONG_OWNER)
+ .build();
+ verifyBucketOwnershipVerificationAccessDenied(() ->
s3Client.getBucketAcl(wrongRequest));
+ }
+
+ @Test
+ public void testListObject() {
Review Comment:
Nit: Rename to `testListObjects`
##########
hadoop-ozone/s3gateway/src/main/java/org/apache/hadoop/ozone/s3/endpoint/BucketEndpoint.java:
##########
@@ -408,12 +409,13 @@ public Response listMultipartUploads(
* for more details.
*/
@HEAD
- public Response head(@PathParam("bucket") String bucketName)
+ public Response head(@PathParam("bucket") String bucketName, @Context
HttpHeaders httpHeaders)
Review Comment:
Instead of adding a `@Context HttpHeaders` on each of the function, we can
instead introduce the `HttpHeaders` as a `BucketEndpoint` attribute, similar to
`ObjectEndpoint`.
##########
hadoop-ozone/s3gateway/src/main/java/org/apache/hadoop/ozone/s3/endpoint/ObjectEndpoint.java:
##########
@@ -243,6 +243,9 @@ public Response put(
throw newError(NOT_IMPLEMENTED, keyPath);
}
OzoneVolume volume = getVolume();
+ OzoneBucket bucket = volume.getBucket(bucketName);
Review Comment:
Similar comment to remove `getBucket` duplication.
##########
hadoop-ozone/s3gateway/src/main/java/org/apache/hadoop/ozone/s3/endpoint/ObjectEndpoint.java:
##########
@@ -1236,6 +1250,10 @@ private CopyObjectResponse copyObject(OzoneVolume volume,
String sourceBucket = result.getLeft();
String sourceKey = result.getRight();
DigestInputStream sourceDigestInputStream = null;
+
+ String sourceBucketOwner = volume.getBucket(sourceBucket).getOwner();
+ String destBucketOwner = volume.getBucket(destBucket).getOwner();
+ S3Owner.verifyBucketOwnerConditionOnCopyOperation(headers, destBucket,
sourceBucketOwner, destBucketOwner);
Review Comment:
Since the `destBucket` owner is already checked in the outer scope
(`ObjectEndpoint#put`), there is no need to call `getBucket` again. We can
simply check the source bucket.
##########
hadoop-ozone/s3gateway/src/main/java/org/apache/hadoop/ozone/s3/endpoint/BucketEndpoint.java:
##########
@@ -132,6 +132,8 @@ public Response get(
OzoneBucket bucket = null;
try {
+ bucket = getBucket(bucketName);
+ S3Owner.verifyBucketOwnerCondition(hh, bucketName, bucket.getOwner());
Review Comment:
Since `getBucket` is pushed up, we can pass the `OzoneBucket` into the
`getAcl` and `listMultipartUploads` instead to prevent multiple duplicate
`getBucket` calls.
We can address this in a follow up ticket.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
