Skip to content

S3 GetObjectAsync broken in version AWSSDK.S3 3.7.104.28 #2622

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

Closed
renemadsen opened this issue Jun 7, 2023 · 9 comments
Closed

S3 GetObjectAsync broken in version AWSSDK.S3 3.7.104.28 #2622

renemadsen opened this issue Jun 7, 2023 · 9 comments
Labels
bug This issue is a bug. needs-review s3

Comments

@renemadsen
Copy link

renemadsen commented Jun 7, 2023

Describe the bug

Using the S3 part and consider a structure, where there are a bucket with "folders" in it, so we have this structure bucketname/foldername/filename.png

The location of foldername in parameters for GetObjectRequest has changed.

Expected Behavior

Return the object

Current Behavior

The request signature we calculated does not match the signature you provided. Check your key and signing method.

Reproduction Steps

In version 3.7.104.20 and older, you could do the following to retrieve an object:

                GetObjectRequest request = new GetObjectRequest
                {
                    BucketName =
                        $"{bucketName}/{folderName}",
                    Key = fileName
                };
                return await _s3Client.GetObjectAsync(request);

In version 3.7.104.28 it's changed and now you have to do:

                    GetObjectRequest request = new GetObjectRequest
                    {
                        BucketName =
                            bucketName,
                        Key = $"{folderName}/{fileName}"
                    };

                    return await _s3Client.GetObjectAsync(request);

Possible Solution

No response

Additional Information/Context

No response

AWS .NET SDK and/or Package version used

AWSSDK.S3 3.7.104.28

Targeted .NET Platform

Net 7

Operating System and version

Ubuntu 22.04, 22.10, 23.04

@renemadsen renemadsen added bug This issue is a bug. needs-triage This issue or PR still needs to be triaged. labels Jun 7, 2023
@renemadsen renemadsen changed the title (short issue description) S3 GetObjectAsync broken in version AWSSDK.S3 3.7.104.28 Jun 7, 2023
@jeferssonlemes
Copy link

same here

@renemadsen
Copy link
Author

PutObjectRequest is also broken after testing

@renemadsen
Copy link
Author

33bfebe this change is breaking the old behaviour.

renemadsen referenced this issue Jun 7, 2023
@ashishdhingra
Copy link
Contributor

ashishdhingra commented Jun 7, 2023

Appears to be reproducible. Needs review with the team. I'm not sure if the syntax {bucketName}/{folderName} in earlier versions was somehow supported as a side effect of the issue that was fixed in 33bfebe. Bucket name per https://docs.aws.amazon.com/AmazonS3/latest/API/API_GetObject.html should only contain name of bucket whereas Key could contain path elements per example at the above documentation.

In AWSSDK.S3 version 3.7.104.20 (assuming us-east-2 as region),

  • Using the code:
using (AmazonS3Client client = new AmazonS3Client(new AmazonS3Config()))
{
    AWSConfigs.LoggingConfig.LogTo = LoggingOptions.Console;
    AWSConfigs.LoggingConfig.LogResponses = ResponseLoggingOption.Always;
    AWSConfigs.LoggingConfig.LogMetrics = true;
    var getObjectResponse = await client.GetObjectAsync(new GetObjectRequest() {
        BucketName = "testbucket",
        Key = "foldername/test.txt"
    });

    Console.WriteLine($"Object Size: {getObjectResponse.ContentLength}"); 
}

used https://testbucket.s3.us-east-2.amazonaws.com as endpoint.

  • Using the code
using (AmazonS3Client client = new AmazonS3Client(new AmazonS3Config()))
{
    AWSConfigs.LoggingConfig.LogTo = LoggingOptions.Console;
    AWSConfigs.LoggingConfig.LogResponses = ResponseLoggingOption.Always;
    AWSConfigs.LoggingConfig.LogMetrics = true;
    var getObjectResponse = await client.GetObjectAsync(new GetObjectRequest() {
        BucketName = "testbucket/foldername",
        Key = "test.txt"
    });

    Console.WriteLine($"Object Size: {getObjectResponse.ContentLength}"); 
}

used https://s3.us-east-2.amazonaws.com/testbucket/foldername as endpoint.

@ashishdhingra ashishdhingra added needs-review and removed needs-triage This issue or PR still needs to be triaged. labels Jun 7, 2023
@normj
Copy link
Member

normj commented Jun 7, 2023

Hi @renemadsen @jeferssonlemes

Sorry the 33bfebe change affected you but it was never intended to pass in a key prefix as part of the BucketName property and that working in the past was an accidental side affect.

There is a lot of logic in the SDK inspecting the bucket name for figuring out DNS routing and signing. In many of those scenarios if you would have added the key prefix that would have broken the scenario. For example if you were using a multi region access point as a the bucket name and added a key prefix the SDK would have failed to recognize the access point and would have signed it incorrectly.

I'm sure S3 will want us to add even more logic like this to the BucketName property in the future. It is not practical for the SDK to maintain this accidental side affect behavior of using the BucketName property for anything but the bucket name as S3 defines it.

To give you an appreciation of the complexity of the BucketName property we have to send the bucket name through this huge code generated condition block to figure out where to send the request to. So adding an unexpected key prefix into that will result in undefined behavior that in the past you got lucky but many of the conditions blocks in there would not have resolved correctly. https://github.com/aws/aws-sdk-net/blob/master/sdk/src/Services/S3/Generated/Internal/AmazonS3EndpointProvider.cs

@ashishdhingra ashishdhingra added response-requested Waiting on additional info and feedback. Will move to "closing-soon" in 7 days. and removed needs-review labels Jun 7, 2023
@renemadsen
Copy link
Author

It's not a problem that we have this kind of change, it should just have been marked as a breaking change.
But I also see that it can be hard to discover that it would affect a side behavior.
It makes a lot of sense not to keep the accidental behavior.

@normj
Copy link
Member

normj commented Jun 7, 2023

@renemadsen fair callout that this was a breaking change. This just wasn't a scenario we thought of to have a test for to catch the breaking change.

@renemadsen
Copy link
Author

@renemadsen fair callout that this was a breaking change. This just wasn't a scenario we thought of to have a test for to catch the breaking change.

Maybe there should be some test in the code to ensure one does not try to do what we apparently was able to do for several years. Throw an exception if the bucketname includes "/" ?

@github-actions github-actions bot removed the response-requested Waiting on additional info and feedback. Will move to "closing-soon" in 7 days. label Jun 8, 2023
@normj normj closed this as completed Jun 13, 2023
@github-actions
Copy link

⚠️COMMENT VISIBILITY WARNING⚠️

Comments on closed issues are hard for our team to see.
If you need more assistance, please either tag a team member or open a new issue that references this one.
If you wish to keep having a conversation with other community members under this issue feel free to do so.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug This issue is a bug. needs-review s3
Projects
None yet
Development

No branches or pull requests

4 participants