Skip to content
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

S3Utilities.parseUri() doesn't work with localhost-based URIs (specifically, LocalStack) #4996

Closed
michaeldavis-toast opened this issue Mar 6, 2024 · 3 comments
Labels
closing-soon This issue will close in 4 days unless further comments are made. feature-request A feature should be added or improved.

Comments

@michaeldavis-toast
Copy link

Describe the bug

The parseUri() helper method in S3Utilities throws an Exception when the given URI does not contain the string "s3", which means it can not be used when testing with LocalStack.

Expected Behavior

I would expect parseUri to return a correctly parsed S3Uri with the provided information.

Current Behavior

parseURI throws an exception.

java.lang.IllegalArgumentException: Invalid S3 URI: hostname does not appear to be a valid S3 endpoint: http://127.0.0.1:49221/test-bucket/0c05ee16-ea61-4e96-810e-5900b2de9457
	at software.amazon.awssdk.services.s3.S3Utilities.parseStandardUri(S3Utilities.java:310)
	at software.amazon.awssdk.services.s3.S3Utilities.parseUri(S3Utilities.java:299)

Reproduction Steps

S3Client client = S3Client.builder()
    .endpointOverride(URI.create("http://localhost:4575"))
    .forcePathStyle(true)
    .build();

S3Utilities utils = client.utilities();

PutObjectResult result = client.putObject("test-bucket", "test.txt", "hello, world");

URL url = utils.getUrl(
    GetUrlRequest.builder()
        .bucket("test-bucket")
        .key("test.txt")
        .build());

// this is where we store the Url for fetching later.
// when testing against LocalStack, we receive a URL string that looks like "http://127.0.0.1:49221/test-bucket/test.txt"

S3Uri s3Uri = utils.parseUri(url);

Possible Solution

Alter the regex to no longer require the string "s3" to be part of the URI being parsed. This would take a little bit of rework because the location of that substring in the hostname is used to determine if the URI is using path-based vs virtualhost-based routing.

Additional Information/Context

The original discussion around including the parseUri method in the first place offers some insight into why this might have been intended behavior, or at the very least why this issue would have been ignored.

#272 (comment)

Some customers expect us to validate that the URLs are actually AWS S3-owned URLs, and unintentionally introduce security issues into their service based on that assumption.

However, simply including the string "s3" in the hostname doesn't actually enforce that the URL is owned by Amazon's S3 service.

AWS Java SDK version used

2.20.112

JDK version used

openjdk 11.0.15 2022-04-19 LTS

Operating System and version

OSX 14.3.1

@michaeldavis-toast michaeldavis-toast added bug This issue is a bug. needs-triage This issue or PR still needs to be triaged. labels Mar 6, 2024
@debora-ito debora-ito added needs-review This issue or PR needs review from the team. and removed needs-triage This issue or PR still needs to be triaged. labels Mar 7, 2024
@debora-ito
Copy link
Member

Hi @michaeldavis-toast, this is not a bug, the S3Utilities is intended to be used with s3 endpoints, we don't guarantee that the SDK will work with third-party apps.

We don't have plans to relax the hostname validation in the parseUri method currently.

@debora-ito debora-ito added closing-soon This issue will close in 4 days unless further comments are made. feature-request A feature should be added or improved. and removed bug This issue is a bug. needs-review This issue or PR needs review from the team. labels Mar 8, 2024
@michaeldavis-toast
Copy link
Author

👍 I had thought that might be the case. Thanks for the response.

Copy link

This issue is now closed. Comments on closed issues are hard for our team to see.
If you need more assistance, please open a new issue that references this one.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
closing-soon This issue will close in 4 days unless further comments are made. feature-request A feature should be added or improved.
Projects
None yet
Development

No branches or pull requests

2 participants