Merge pull request #2474 from dualfroz/fix/1923-1930-list-buckets-bucket-region

fix: validate and apply the ListBuckets bucket-region query

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{

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{

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"

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

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,
+	}
+}

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: &region,
+			})
+			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 {

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,

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
 }