GetBucketLocation requires path-style S3 lookups

[jared-gs]

What issue did you see?

The updateEndpointForS3Config function parses S3 bucket endpoints with virtual hosted-style addressing, except when the operation is s3/GetBucketLocation, which attempts to use path-style regardless of preference. Environments forbidding S3 path-style (i.e. virtual hosted-style addressing only) are therefore unable to utilize this s3/GetBucketLocation function.

https://github.com/aws/aws-sdk-go/blob/master/service/s3/host_style_bucket.go#L47-L49

Can we please evaluate if this conditional is still required? Last update to this behavior was in 4 years ago: 27361e6. The GetBucketLocation syntax accepts virtual hosted-style addressing, as far as I can determine from AWS API documentation:

• https://docs.aws.amazon.com/AmazonS3/latest/API/API_GetBucketLocation.html

[diehlaws]

Hi @jared-gs, thanks for reaching out to us. What you're suggesting seems reasonable, as you mentioned the API call looks to support virtual-hosted style endpoints and at the surface level this does not look like it would be a breaking change. I'll bring this up with the rest of our team during our next sprint and will update the issue once I have more information.

[nyetsche]

@diehlaws any update?

[jasdel]

@jared-gs and @nyetsche have you tried to use the SDK's custom s3manager.GetBucketRegion utility. This utility does not require authentication, and is able to make the API call using with virtual host style addressing, using the HeadBucket API operation. This utility is also more consistent in its behavior cross account, and unauthenticated when trying to identifying the region a bucket exists in.

The SDK explicitly is forcing the GetBucketLocation method to be path style. It looks like this was done because the GetBucketLocation could return an unexpected an error message the SDK does not handle when the request is made using virtual host styling. When GetBucketLocation is used as path style the API operation response is as documented in the API reference guide. Amazon S3 may of since corrected this behavior since this change was made about 5 years ago.

• ffc9803
• S3:GetBucketLocation throwing error #380

To support virtual host style the GetBucketLocation we should check and verify S3's current behavior with regard to the error message reported in #380, and GetBucketLocation would need to be customization to handle the error format returned if that is still the behavior.

[jared-gs]

Hey there @jasdel,

Thanks, I haven't looked into using s3manager.GetBucketRegion before. It might not applicable in my use-case, since I'm relying on 3rd-party software that leverages aws-sdk-go, but I can see if can be integrated in their upstream.

To recap. the SDK is forcing the path-style on certain logic here. The API reference docs linked above indicate this should work with virtual hosted-style calls.

The US commercial S3 FIPS endpoints are real-world examples of path-style breaking the SDK workflows: https://aws.amazon.com/compliance/fips/

Note: These Endpoints can only be used with Virtual Hosted-Style addressing. For example: https://bucket.s3-fips.us-east-2.amazonaws.com. Visit the Amazon S3 Documentation page for more information.

Here is a quick test to illustrate this issue here: https://gist.github.com/jared-gs/76fdaca185c4b3bc149c5538b5c78f98

$ ./fips-test
RequestError: send request failed
caused by: Get https://s3-fips.us-east-1.amazonaws.com/testbucket?location=: dial tcp: lookup s3-fips.us-east-1.amazonaws.com: no such host

The URI https://s3-fips.us-east-1.amazonaws.com/testbucket is not valid, but https://testbucket.s3-fips.us-east-1.amazonaws.com is. This is just how the FIPS endpoints seem to work: no path-style. In essence, functions forcing path-style in this SDK cannot be used with AWS' own FIPS endpoints in us-east-1, us-east-2, us-west-1 & us-west-2. It would be good to verify if this behavior has been corrected and rectify this in the SDK if possible.

[jasdel]

Thanks for the clarification and background @jared-gs this is very helpful. The documented behavior of GetBucketLocation isn't documented well, and doesn't document the cross-region behavior of the operation. I think the API reference documenting that virtual path style is supported, while technically true, isn't really a complete pictured. Using GetBucketLocation with virtual style is has strange usecase/behavior, because if the request is made, and signed with a region the bucket doesn't exist in an response error is returned. While an error returned does include the bucket's actual region, that doesn't match the operation's "ConstraintLocation" format.

Because of this I think it would be best for us to reach out to Terraform and help the Terraform provider switch to s3manager.GetBucketRegion instead of GetBucketLocation.

Looks like Terraform uses GetBucketLocation in 3 locations:

• resource_aws_s3_bucket.go
• resource_aws_s3_bucket_test.go
• data_source_aws_s3_bucket.go

[jasdel]

On more investigation it looks like the SDK's s3manager.GetBucketRegion utility also uses path style for its requests as well. Though in this case this behavior can be changed by the caller without additional configuration. I created #3428 clarifying this documentation with examples.

func main() {
	sess, err := session.NewSession()
	if err != nil {
		log.Fatalf("failed to load session, %v", err)
	}

	region, err := s3manager.GetBucketRegionWithClient(context.Background(),
		s3.New(sess, &aws.Config{
			Endpoint: aws.String("https://s3-fips.us-west-2.amazonaws.com"),
		}),
		"testbucket", func(r *request.Request) {
			r.Config.S3ForcePathStyle = aws.Bool(false)
		})

	fmt.Println(region, err)
}

[jared-gs]

@jasdel,

Apologizes for the late response. Thank you so much for getting this change pushed through! This would appear to solve our FIPs endpoint issue.

I'll follow up if you run into any further issues.

[jasdel]

Thanks for the update. This change is also now available in Terraform's 3.0 product release, https://github.com/terraform-providers/terraform-provider-aws/blob/v3.0.0/CHANGELOG.md Let us know if you have any additional issues, feedback or questions!