diff --git a/s3api/controllers/bucket-list.go b/s3api/controllers/bucket-list.go index c85f77c4..5611dcde 100644 --- a/s3api/controllers/bucket-list.go +++ b/s3api/controllers/bucket-list.go @@ -17,6 +17,7 @@ package controllers import ( "github.com/gofiber/fiber/v3" "github.com/versity/versitygw/auth" + "github.com/versity/versitygw/debuglogger" "github.com/versity/versitygw/s3api/utils" "github.com/versity/versitygw/s3response" ) @@ -25,6 +26,7 @@ func (c S3ApiController) ListBuckets(ctx fiber.Ctx) (*Response, error) { cToken := ctx.Query("continuation-token") prefix := ctx.Query("prefix") maxBucketsStr := ctx.Query("max-buckets") + bucketRegion := ctx.Query("bucket-region") acct := utils.ContextKeyAccount.Get(ctx).(auth.Account) isRoot, _ := utils.ContextKeyIsRoot.Get(ctx).(bool) @@ -55,6 +57,16 @@ func (c S3ApiController) ListBuckets(ctx fiber.Ctx) (*Response, error) { }, err } + // 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. res, err := c.be.ListBuckets(ctx.RequestCtx(), s3response.ListBucketsInput{ diff --git a/s3api/controllers/bucket-list_test.go b/s3api/controllers/bucket-list_test.go index bfa16f6a..8909b58c 100644 --- a/s3api/controllers/bucket-list_test.go +++ b/s3api/controllers/bucket-list_test.go @@ -99,6 +99,84 @@ func TestS3ApiController_ListBuckets(t *testing.T) { err: s3err.GetInvalidArgMaxLimiter("max-buckets", "bla"), }, }, + { + name: "invalid bucket region", + input: testInput{ + locals: defaultLocals, + beRes: validRes, + queries: map[string]string{ + "bucket-region": "abc", + }, + }, + output: testOutput{ + response: &Response{ + MetaOpts: &MetaOptions{}, + }, + err: s3err.GetInvalidArgBucketRegion("abc"), + }, + }, + { + name: "bucket region of another region", + input: testInput{ + locals: defaultLocals, + beRes: validRes, + queries: map[string]string{ + "bucket-region": "us-west-2", + }, + }, + output: testOutput{ + response: &Response{ + 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, + }, + }, + }, + { + name: "bucket region of the gateway region", + input: testInput{ + locals: defaultLocals, + beRes: validRes, + queries: map[string]string{ + "bucket-region": "us-east-1", + }, + }, + output: testOutput{ + response: &Response{ + Data: validRes, + }, + }, + }, { name: "backend returns error", input: testInput{ diff --git a/s3api/utils/utils.go b/s3api/utils/utils.go index 16e7bfa8..a3faa6cd 100644 --- a/s3api/utils/utils.go +++ b/s3api/utils/utils.go @@ -40,6 +40,8 @@ import ( var ( bucketNameRegexp = regexp.MustCompile(`^[a-z0-9][a-z0-9.-]+[a-z0-9]$`) bucketNameIpRegexp = regexp.MustCompile(`^(?:[0-9]{1,3}\.){3}[0-9]{1,3}$`) + // AWS region codes, such as us-east-1, us-gov-west-1 or eusc-de-east-1 + awsRegionRegexp = regexp.MustCompile(`^[a-z]{2,4}(-[a-z]+)+-[0-9]+$`) ) var strictBucketNameValidation atomic.Bool @@ -1153,6 +1155,19 @@ func ValidateLocationConstraint(constraint *string, region string) error { return nil } +// 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 // aws-chunked, as the SDKs do to carry a trailing checksum. const AwsChunkedEncoding = "aws-chunked" diff --git a/s3api/utils/utils_test.go b/s3api/utils/utils_test.go index 217d4bc6..ea5f9c4e 100644 --- a/s3api/utils/utils_test.go +++ b/s3api/utils/utils_test.go @@ -1539,6 +1539,34 @@ func TestHasAwsChunkedEncoding(t *testing.T) { } } +func TestValidateBucketRegion(t *testing.T) { + endpointErr := s3err.GetInvalidArgumentErr(s3err.InvalidArgBucketRegionEndpoint, "") + tests := []struct { + name string + bucketRegion string + region string + want error + }{ + {"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, ValidateBucketRegion(tt.bucketRegion, tt.region)) + }) + } +} + func TestParseContentEncoding(t *testing.T) { tests := []struct { name string diff --git a/s3err/invalid-argument.go b/s3err/invalid-argument.go index 7bf61cdf..27f992f8 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", @@ -384,3 +389,15 @@ func GetInvalidArgObjectOwnership(value string) InvalidArgumentError { Description: fmt.Sprintf("Invalid x-amz-object-ownership header: %s", value), } } + +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: description, + } +} diff --git a/tests/integration/ListBuckets.go b/tests/integration/ListBuckets.go index 178fa618..e8b64283 100644 --- a/tests/integration/ListBuckets.go +++ b/tests/integration/ListBuckets.go @@ -17,7 +17,9 @@ package integration import ( "context" "fmt" + "slices" "sort" + "strings" "github.com/aws/aws-sdk-go-v2/service/s3" "github.com/aws/aws-sdk-go-v2/service/s3/types" @@ -271,6 +273,69 @@ func ListBuckets_invalid_max_buckets(s *S3Conf) error { }) } +func ListBuckets_invalid_bucket_region(s *S3Conf) error { + testName := "ListBuckets_invalid_bucket_region" + return actionHandler(s, testName, func(s3client *s3.Client, bucket string) error { + ctx, cancel := context.WithTimeout(context.Background(), shortTimeout) + _, err := s3client.ListBuckets(ctx, &s3.ListBucketsInput{ + BucketRegion: getPtr("abc"), + }) + cancel() + return checkApiErr(err, s3err.GetInvalidArgBucketRegion("abc")) + }) +} + +func ListBuckets_bucket_region(s *S3Conf) error { + testName := "ListBuckets_bucket_region" + return actionHandler(s, testName, func(s3client *s3.Client, bucket string) error { + listBuckets := func(region string) ([]types.Bucket, error) { + ctx, cancel := context.WithTimeout(context.Background(), shortTimeout) + out, err := s3client.ListBuckets(ctx, &s3.ListBucketsInput{ + BucketRegion: ®ion, + }) + cancel() + if err != nil { + return nil, err + } + return out.Buckets, nil + } + + buckets, err := listBuckets(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, s.awsRegion, buckets) + } + + // 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" + } + _, err = listBuckets(otherRegion) + if err := checkApiErr(err, s3err.GetInvalidArgumentErr(s3err.InvalidArgBucketRegionEndpoint, "")); err != nil { + return err + } + + // an empty bucket-region is not a valid region either + _, err = listBuckets("") + return checkApiErr(err, s3err.GetInvalidArgBucketRegion("")) + }) +} + func ListBuckets_truncated(s *S3Conf) error { testName := "ListBuckets_truncated" return actionHandler(s, testName, func(s3client *s3.Client, bucket string) error { diff --git a/tests/integration/group-tests.go b/tests/integration/group-tests.go index 0da061bd..328683b1 100644 --- a/tests/integration/group-tests.go +++ b/tests/integration/group-tests.go @@ -110,6 +110,8 @@ func TestListBuckets(ts *TestState) { ts.Sync(ListBuckets_as_admin) ts.Sync(ListBuckets_with_prefix) ts.Sync(ListBuckets_invalid_max_buckets) + ts.Sync(ListBuckets_invalid_bucket_region) + ts.Sync(ListBuckets_bucket_region) ts.Sync(ListBuckets_truncated) ts.Sync(ListBuckets_success) ts.Sync(ListBuckets_empty_success) @@ -2991,6 +2993,8 @@ func GetIntTests() IntTests { "ListBuckets_as_admin": ListBuckets_as_admin, "ListBuckets_with_prefix": ListBuckets_with_prefix, "ListBuckets_invalid_max_buckets": ListBuckets_invalid_max_buckets, + "ListBuckets_invalid_bucket_region": ListBuckets_invalid_bucket_region, + "ListBuckets_bucket_region": ListBuckets_bucket_region, "ListBuckets_truncated": ListBuckets_truncated, "ListBuckets_success": ListBuckets_success, "ListBuckets_empty_success": ListBuckets_empty_success, diff --git a/tests/test_rest_list_buckets.sh b/tests/test_rest_list_buckets.sh index 7ca63ad7..128cf1c5 100755 --- a/tests/test_rest_list_buckets.sh +++ b/tests/test_rest_list_buckets.sh @@ -258,9 +258,6 @@ export RUN_USERS=true # tags: curl,ListBuckets,bucket-region,invalid-query @test "REST - ListBuckets - invalid bucket-region query" { - if [ "$DIRECT" != "true" ]; then - skip "https://github.com/versity/versitygw/issues/1923" - fi local invalid_region="abc" run get_bucket_name "$BUCKET_ONE_NAME" @@ -277,9 +274,6 @@ export RUN_USERS=true # tags: curl,ListBuckets,bucket-region @test "REST - ListBuckets - incorrect bucket region" { - if [ "$DIRECT" != "true" ]; then - skip "https://github.com/versity/versitygw/issues/1930" - fi local test_region="us-east-1" if [ "$AWS_REGION" == "us-east-1" ]; then test_region="us-west-1" @@ -291,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 }