diff --git a/s3api/controllers/bucket-list.go b/s3api/controllers/bucket-list.go index c85f77c..93885e0 100644 --- a/s3api/controllers/bucket-list.go +++ b/s3api/controllers/bucket-list.go @@ -17,7 +17,9 @@ 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/s3err" "github.com/versity/versitygw/s3response" ) @@ -25,6 +27,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 +58,13 @@ 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) + } + // IsAdmin is the backends' "return every bucket, unfiltered" flag. res, err := c.be.ListBuckets(ctx.RequestCtx(), s3response.ListBucketsInput{ @@ -68,6 +78,13 @@ 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 bfa16f6..bdf9b5b 100644 --- a/s3api/controllers/bucket-list_test.go +++ b/s3api/controllers/bucket-list_test.go @@ -99,6 +99,63 @@ 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: 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", + }, + queries: map[string]string{ + "bucket-region": "us-west-2", + "prefix": "te", + }, + }, + output: testOutput{ + response: &Response{ + Data: s3response.ListAllMyBucketsResult{ + Owner: validRes.Owner, + Prefix: "te", + }, + }, + }, + }, + { + 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 16e7bfa..27912bf 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,13 @@ 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) +} + // 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 217d4bc..9d51f44 100644 --- a/s3api/utils/utils_test.go +++ b/s3api/utils/utils_test.go @@ -1539,6 +1539,31 @@ func TestHasAwsChunkedEncoding(t *testing.T) { } } +func TestIsValidBucketRegion(t *testing.T) { + tests := []struct { + name string + bucketRegion string + region string + want bool + }{ + {"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}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + assert.Equal(t, tt.want, IsValidBucketRegion(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 7bf61cd..e410597 100644 --- a/s3err/invalid-argument.go +++ b/s3err/invalid-argument.go @@ -384,3 +384,11 @@ func GetInvalidArgObjectOwnership(value string) InvalidArgumentError { Description: fmt.Sprintf("Invalid x-amz-object-ownership header: %s", value), } } + +func GetInvalidArgBucketRegion(value string) InvalidArgumentError { + 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), + } +} diff --git a/tests/integration/ListBuckets.go b/tests/integration/ListBuckets.go index 178fa61..a9a6967 100644 --- a/tests/integration/ListBuckets.go +++ b/tests/integration/ListBuckets.go @@ -17,6 +17,7 @@ package integration import ( "context" "fmt" + "slices" "sort" "github.com/aws/aws-sdk-go-v2/service/s3" @@ -271,6 +272,60 @@ 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) + } + + // every bucket of the gateway lives in its own region + otherRegion := "us-west-2" + if s.awsRegion == otherRegion { + otherRegion = "us-east-1" + } + buckets, err = listBuckets(otherRegion) + if err != nil { + return err + } + if len(buckets) != 0 { + return fmt.Errorf("expected an empty %v listing, instead got %v", + otherRegion, buckets) + } + + return nil + }) +} + 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 fb99680..47ac55f 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) @@ -2985,6 +2987,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 7ca63ad..f925d3d 100644 --- 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"