Skip to content

[Hackathon] Add extension method: verifyBucketOwnership #3076

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -16,13 +16,16 @@
package software.amazon.awssdk.services.s3.extensions;

import static org.assertj.core.api.Assertions.assertThat;
import static org.assertj.core.api.Assertions.assertThatCode;
import static org.assertj.core.api.Assertions.assertThatThrownBy;
import static software.amazon.awssdk.testutils.service.S3BucketUtils.temporaryBucketName;

import java.io.File;
import java.io.IOException;
import org.junit.AfterClass;
import org.junit.BeforeClass;
import org.junit.Test;
import software.amazon.awssdk.core.exception.SdkClientException;
import software.amazon.awssdk.services.s3.S3IntegrationTestBase;
import software.amazon.awssdk.services.s3.model.PutObjectRequest;
import software.amazon.awssdk.testutils.RandomTempFile;
Expand Down Expand Up @@ -86,4 +89,17 @@ public void objectDoesNotExistSync() {
public void objectDoesNotExistAsync() {
assertThat(s3Async.doesObjectExist(BUCKET, "noexist").join()).isFalse();
}

@Test
public void verifyBucketOwnership_userOwnsBucket() {
assertThatCode(() -> s3.verifyBucketOwnership(BUCKET)).doesNotThrowAnyException();
}

@Test
public void verifyBucketOwnership_userDoesNotOwnBucket() {
assertThatThrownBy(() -> s3.verifyBucketOwnership(temporaryBucketName("noexist")))
.isInstanceOf(SdkClientException.class)
.hasMessage("Bucket ownership verification failed.");
}

}
Original file line number Diff line number Diff line change
Expand Up @@ -124,4 +124,22 @@ default URL getUrl(Consumer<GetUrlRequest.Builder> getUrlRequest) {
default S3Presigner presigner() {
return S3Presigner.create();
}

/**
* Validate that the currently configured AWS Account is the owner of a given S3 bucket. Because Amazon S3 identifies buckets
* based on their names, an application that uses an incorrect bucket name in a request could inadvertently perform operations
* against a different bucket than expected.
* <p>
* Because buckets can be deleted and re-created at any time, this method should only be used when you know that the bucket in
* question will not be deleted after its ownership is verified. For more robust protection, you should include the {@code
* expectedBucketOwner} parameter with all eligible requests. For more information, see:
* <p>
* https://docs.aws.amazon.com/AmazonS3/latest/userguide/bucket-owner-condition.html
*
* @param bucket the bucket to verify ownership
*/
@SdkExtensionMethod
default void verifyBucketOwnership(String bucket) {
new DefaultS3ClientSdkExtension((S3Client) this).verifyBucketOwnership(bucket);
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -16,8 +16,10 @@
package software.amazon.awssdk.services.s3.internal.extensions;

import software.amazon.awssdk.annotations.SdkInternalApi;
import software.amazon.awssdk.core.exception.SdkClientException;
import software.amazon.awssdk.services.s3.S3Client;
import software.amazon.awssdk.services.s3.extensions.S3ClientSdkExtension;
import software.amazon.awssdk.services.s3.model.Bucket;
import software.amazon.awssdk.services.s3.model.NoSuchBucketException;
import software.amazon.awssdk.services.s3.model.NoSuchKeyException;
import software.amazon.awssdk.utils.Validate;
Expand Down Expand Up @@ -53,4 +55,17 @@ public boolean doesObjectExist(String bucket, String key) {
return false;
}
}

@Override
public void verifyBucketOwnership(String bucket) {
Validate.notEmpty(bucket, "bucket");
boolean isBucketOwner = s3.listBuckets()
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What if there are many many buckets?
Is it possible to use a headBucket request, or is that too inconclusive?

Copy link
Contributor Author

@Bennett-Lynch Bennett-Lynch Mar 3, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  1. The maximum number of buckets is 1,000. listBuckets isn't a paginated operation.
  2. Simply verifying access to a bucket via headBucket doesn't verify that we own it. The intent here is to verify that we own a bucket that we may be potentially writing to.
  3. To use headBucket we would need to use the expectedBucketOwner parameter, but the difficulty here is that we don't necessarily know the caller's AWS Account ID. However, listBuckets works as a clever workaround here because it "returns a list of all buckets owned by the authenticated sender of the request".

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sounds good! Thanks for clarifying.

.buckets()
.stream()
.map(Bucket::name)
.anyMatch(b -> b.equals(bucket));
if (!isBucketOwner) {
throw SdkClientException.create("Bucket ownership verification failed.");
}
}
}