diff --git a/s3api/controllers/bucket-list.go b/s3api/controllers/bucket-list.go index 93885e0..5611dcd 100644 --- a/s3api/controllers/bucket-list.go +++ b/s3api/controllers/bucket-list.go @@ -19,7 +19,6 @@ import ( "github.com/versity/versitygw/auth" "github.com/versity/versitygw/debuglogger" "github.com/versity/versitygw/s3api/utils" - "github.com/versity/versitygw/s3err" "github.com/versity/versitygw/s3response" ) @@ -58,11 +57,14 @@ func (c S3ApiController) ListBuckets(ctx fiber.Ctx) (*Response, error) { }, err } - if bucketRegion != "" && !utils.IsValidBucketRegion(bucketRegion, region) { - debuglogger.Logf("invalid bucket region: %s", bucketRegion) - return &Response{ - MetaOpts: &MetaOptions{}, - }, s3err.GetInvalidArgBucketRegion(bucketRegion) + // an empty bucket-region= is checked too, so look for the argument itself + if ctx.Request().URI().QueryArgs().Has("bucket-region") { + if err := utils.ValidateBucketRegion(bucketRegion, region); err != nil { + debuglogger.Logf("invalid bucket region: %q", bucketRegion) + return &Response{ + MetaOpts: &MetaOptions{}, + }, err + } } // IsAdmin is the backends' "return every bucket, unfiltered" flag. @@ -78,13 +80,6 @@ func (c S3ApiController) ListBuckets(ctx fiber.Ctx) (*Response, error) { return &Response{}, err } - // every bucket lives in the gateway region, so there is nothing - // to list for any other region - if bucketRegion != "" && bucketRegion != region { - res.Buckets.Bucket = nil - res.ContinuationToken = "" - } - for i := range res.Buckets.Bucket { res.Buckets.Bucket[i].BucketRegion = region } diff --git a/s3api/controllers/bucket-list_test.go b/s3api/controllers/bucket-list_test.go index bdf9b5b..8909b58 100644 --- a/s3api/controllers/bucket-list_test.go +++ b/s3api/controllers/bucket-list_test.go @@ -119,25 +119,46 @@ func TestS3ApiController_ListBuckets(t *testing.T) { name: "bucket region of another region", input: testInput{ locals: defaultLocals, - beRes: s3response.ListAllMyBucketsResult{ - Owner: validRes.Owner, - Buckets: validRes.Buckets, - Prefix: "te", - // a continuation token from the backend must not - // leak into the empty listing - ContinuationToken: "test", - }, + beRes: validRes, queries: map[string]string{ "bucket-region": "us-west-2", - "prefix": "te", }, }, output: testOutput{ response: &Response{ - Data: s3response.ListAllMyBucketsResult{ - Owner: validRes.Owner, - Prefix: "te", - }, + MetaOpts: &MetaOptions{}, + }, + err: s3err.GetInvalidArgumentErr(s3err.InvalidArgBucketRegionEndpoint, ""), + }, + }, + { + name: "empty bucket region", + input: testInput{ + locals: defaultLocals, + beRes: validRes, + queries: map[string]string{ + "bucket-region": "", + }, + }, + output: testOutput{ + response: &Response{ + MetaOpts: &MetaOptions{}, + }, + err: s3err.GetInvalidArgBucketRegion(""), + }, + }, + { + name: "bucket region of the gateway region in uppercase", + input: testInput{ + locals: defaultLocals, + beRes: validRes, + queries: map[string]string{ + "bucket-region": "US-EAST-1", + }, + }, + output: testOutput{ + response: &Response{ + Data: validRes, }, }, }, diff --git a/s3api/utils/utils.go b/s3api/utils/utils.go index 27912bf..a3faa6c 100644 --- a/s3api/utils/utils.go +++ b/s3api/utils/utils.go @@ -1155,11 +1155,17 @@ func ValidateLocationConstraint(constraint *string, region string) error { return nil } -// IsValidBucketRegion checks a ListBuckets bucket-region value. The gateway -// region is valid whatever its name, any other value has to be an AWS region -// code. -func IsValidBucketRegion(bucketRegion, region string) bool { - return bucketRegion == region || awsRegionRegexp.MatchString(bucketRegion) +// ValidateBucketRegion checks a ListBuckets bucket-region value. Every bucket +// lives in the gateway region, so like an S3 regional endpoint the gateway +// rejects any other region, matching it case-insensitively. +func ValidateBucketRegion(bucketRegion, region string) error { + if strings.EqualFold(bucketRegion, region) { + return nil + } + if !awsRegionRegexp.MatchString(strings.ToLower(bucketRegion)) { + return s3err.GetInvalidArgBucketRegion(bucketRegion) + } + return s3err.GetInvalidArgumentErr(s3err.InvalidArgBucketRegionEndpoint, "") } // AwsChunkedEncoding is the coding a client announces when it frames a body in diff --git a/s3api/utils/utils_test.go b/s3api/utils/utils_test.go index 9d51f44..ea5f9c4 100644 --- a/s3api/utils/utils_test.go +++ b/s3api/utils/utils_test.go @@ -1539,27 +1539,30 @@ func TestHasAwsChunkedEncoding(t *testing.T) { } } -func TestIsValidBucketRegion(t *testing.T) { +func TestValidateBucketRegion(t *testing.T) { + endpointErr := s3err.GetInvalidArgumentErr(s3err.InvalidArgBucketRegionEndpoint, "") tests := []struct { name string bucketRegion string region string - want bool + want error }{ - {"gateway region", "us-east-1", "us-east-1", true}, - {"custom gateway region", "my-region", "my-region", true}, - {"another aws region", "eu-central-1", "us-east-1", true}, - {"gov cloud region", "us-gov-west-1", "us-east-1", true}, - {"iso partition region", "us-isob-east-1", "us-east-1", true}, - {"sovereign cloud region", "eusc-de-east-1", "us-east-1", true}, - {"gibberish", "abc", "us-east-1", false}, - {"missing number", "us-east", "us-east-1", false}, - {"uppercase", "US-EAST-1", "us-east-1", false}, - {"custom region of another gateway", "my-region", "us-east-1", false}, + {"gateway region", "us-east-1", "us-east-1", nil}, + {"gateway region in uppercase", "EU-CENTRAL-1", "eu-central-1", nil}, + {"custom gateway region", "my-region", "my-region", nil}, + {"another aws region", "eu-central-1", "us-east-1", endpointErr}, + {"another aws region in uppercase", "US-EAST-1", "eu-central-1", endpointErr}, + {"gov cloud region", "us-gov-west-1", "us-east-1", endpointErr}, + {"iso partition region", "us-isob-east-1", "us-east-1", endpointErr}, + {"sovereign cloud region", "eusc-de-east-1", "us-east-1", endpointErr}, + {"gibberish", "abc", "us-east-1", s3err.GetInvalidArgBucketRegion("abc")}, + {"empty", "", "us-east-1", s3err.GetInvalidArgBucketRegion("")}, + {"missing number", "us-east", "us-east-1", s3err.GetInvalidArgBucketRegion("us-east")}, + {"custom region of another gateway", "my-region", "us-east-1", s3err.GetInvalidArgBucketRegion("my-region")}, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - assert.Equal(t, tt.want, IsValidBucketRegion(tt.bucketRegion, tt.region)) + assert.Equal(t, tt.want, ValidateBucketRegion(tt.bucketRegion, tt.region)) }) } } diff --git a/s3err/invalid-argument.go b/s3err/invalid-argument.go index e410597..27f992f 100644 --- a/s3err/invalid-argument.go +++ b/s3err/invalid-argument.go @@ -74,9 +74,14 @@ const ( InvalidArgSSEInvalidEncryptionMethod InvalidArgSSECRequiresTLS InvalidArgUploadIdWithUploads + InvalidArgBucketRegionEndpoint ) var invalidArgErrResponses = map[InvalidArgErrorCode]InvalidArgumentError{ + InvalidArgBucketRegionEndpoint: { + Description: "Requests with bucket-region specified must be made to the corresponding regional endpoint", + ArgumentName: "bucket-region", + }, InvalidArgMaxBuckets: { Description: "Argument max-buckets must be an integer between 1 and 10000.", ArgumentName: "max-buckets", @@ -386,9 +391,13 @@ func GetInvalidArgObjectOwnership(value string) InvalidArgumentError { } func GetInvalidArgBucketRegion(value string) InvalidArgumentError { + description := fmt.Sprintf("Argument value %s is not a valid AWS Region", value) + if value == "" { + description = "Argument value is not a valid AWS Region" + } return InvalidArgumentError{ ArgumentName: "bucket-region", // no ArgumentValue is returned for this error - Description: fmt.Sprintf("Argument value %s is not a valid AWS Region", value), + Description: description, } } diff --git a/tests/integration/ListBuckets.go b/tests/integration/ListBuckets.go index a9a6967..e8b6428 100644 --- a/tests/integration/ListBuckets.go +++ b/tests/integration/ListBuckets.go @@ -19,6 +19,7 @@ import ( "fmt" "slices" "sort" + "strings" "github.com/aws/aws-sdk-go-v2/service/s3" "github.com/aws/aws-sdk-go-v2/service/s3/types" @@ -308,21 +309,30 @@ func ListBuckets_bucket_region(s *S3Conf) error { bucket, s.awsRegion, buckets) } - // every bucket of the gateway lives in its own region + // the region is matched case-insensitively + buckets, err = listBuckets(strings.ToUpper(s.awsRegion)) + if err != nil { + return err + } + if !slices.ContainsFunc(buckets, func(b types.Bucket) bool { return getString(b.Name) == bucket }) { + return fmt.Errorf("expected bucket %v in the %v listing, instead got %v", + bucket, strings.ToUpper(s.awsRegion), buckets) + } + + // every bucket of the gateway lives in its own region, so like an S3 + // regional endpoint it rejects any other region otherRegion := "us-west-2" if s.awsRegion == otherRegion { otherRegion = "us-east-1" } - buckets, err = listBuckets(otherRegion) - if err != nil { + _, err = listBuckets(otherRegion) + if err := checkApiErr(err, s3err.GetInvalidArgumentErr(s3err.InvalidArgBucketRegionEndpoint, "")); err != nil { return err } - if len(buckets) != 0 { - return fmt.Errorf("expected an empty %v listing, instead got %v", - otherRegion, buckets) - } - return nil + // an empty bucket-region is not a valid region either + _, err = listBuckets("") + return checkApiErr(err, s3err.GetInvalidArgBucketRegion("")) }) } diff --git a/tests/test_rest_list_buckets.sh b/tests/test_rest_list_buckets.sh index f925d3d..128cf1c 100644 --- a/tests/test_rest_list_buckets.sh +++ b/tests/test_rest_list_buckets.sh @@ -285,12 +285,16 @@ export RUN_USERS=true run setup_bucket "$bucket_name" assert_success - local params=() if [ "$DIRECT" == "true" ]; then - params+=("-url" "https://s3.$test_region.amazonaws.com" "-awsRegion" "$test_region") + # the regional endpoint of that region lists only its own buckets + run list_buckets_bucket_not_in_list "$bucket_name" "$test_region" "-url" "https://s3.$test_region.amazonaws.com" "-awsRegion" "$test_region" + assert_success + return fi - run list_buckets_bucket_not_in_list "$bucket_name" "$test_region" "${params[@]}" + # the gateway is a single regional endpoint, so any other region is rejected + run send_rest_go_command_expect_error_with_specific_arg_name_value "400" "InvalidArgument" "Requests with bucket-region specified must be made to the corresponding regional endpoint" \ + "ArgumentName" "bucket-region" "-query" "bucket-region=$test_region" assert_success }