From 58117c011aa6ad54fdf00cef12c4433be8402bc6 Mon Sep 17 00:00:00 2001 From: Ben McClelland Date: Fri, 29 Aug 2025 15:40:30 -0700 Subject: [PATCH] feat: add get bucket location frontend handlers GetBucketLocation is being deprecated by AWS, but is still used by some clients. We don't need any backend handlers for this since the region is managed by the frontend. All we need is to test for bucket existence, so we can use HeadBucket for this. Fixes #1499 --- auth/bucket_policy_actions.go | 2 + metrics/actions.go | 5 ++ s3api/controllers/base_test.go | 1 + s3api/controllers/bucket-get.go | 49 +++++++++++++++++ s3api/controllers/bucket-get_test.go | 80 ++++++++++++++++++++++++++++ s3api/router.go | 15 ++++++ s3response/s3response.go | 6 +++ tests/integration/group-tests.go | 11 ++++ tests/integration/tests.go | 71 ++++++++++++++++++++++++ 9 files changed, 240 insertions(+) diff --git a/auth/bucket_policy_actions.go b/auth/bucket_policy_actions.go index 716fc40..f1a2c9f 100644 --- a/auth/bucket_policy_actions.go +++ b/auth/bucket_policy_actions.go @@ -87,6 +87,7 @@ const ( PutBucketWebsiteAction Action = "s3:PutBucketWebsite" GetBucketWebsiteAction Action = "s3:GetBucketWebsite" GetBucketPolicyStatusAction Action = "s3:GetBucketPolicyStatus" + GetBucketLocationAction Action = "s3:GetBucketLocation" AllActions Action = "s3:*" ) @@ -157,6 +158,7 @@ var supportedActionList = map[Action]struct{}{ PutBucketWebsiteAction: {}, GetBucketWebsiteAction: {}, GetBucketPolicyStatusAction: {}, + GetBucketLocationAction: {}, AllActions: {}, } diff --git a/metrics/actions.go b/metrics/actions.go index ecc3806..07559f9 100644 --- a/metrics/actions.go +++ b/metrics/actions.go @@ -116,6 +116,7 @@ var ( ActionGetBucketWebsite = "s3_GetBucketWebsite" ActionDeleteBucketWebsite = "s3_DeleteBucketWebsite" ActionGetBucketPolicyStatus = "s3_GetBucketPolicyStatus" + ActionGetBucketLocation = "s3_GetBucketLocation" // Admin actions ActionAdminCreateUser = "admin_CreateUser" @@ -498,4 +499,8 @@ func init() { Name: "GetBucketPolicyStatus", Service: "s3", } + ActionMap[ActionGetBucketLocation] = Action{ + Name: "GetBucketLocation", + Service: "s3", + } } diff --git a/s3api/controllers/base_test.go b/s3api/controllers/base_test.go index 186726a..4cdebd0 100644 --- a/s3api/controllers/base_test.go +++ b/s3api/controllers/base_test.go @@ -49,6 +49,7 @@ var ( Access: "root", Role: auth.RoleAdmin, }, + utils.ContextKeyRegion: "us-east-1", } accessDeniedLocals map[utils.ContextKey]any = map[utils.ContextKey]any{ diff --git a/s3api/controllers/bucket-get.go b/s3api/controllers/bucket-get.go index c3116c5..904e5c1 100644 --- a/s3api/controllers/bucket-get.go +++ b/s3api/controllers/bucket-get.go @@ -619,3 +619,52 @@ func (c S3ApiController) ListObjects(ctx *fiber.Ctx) (*Response, error) { }, }, err } + +// GetBucketLocation handles GET /:bucket?location +func (c S3ApiController) GetBucketLocation(ctx *fiber.Ctx) (*Response, error) { + bucket := ctx.Params("bucket") + acct := utils.ContextKeyAccount.Get(ctx).(auth.Account) + isRoot := utils.ContextKeyIsRoot.Get(ctx).(bool) + isPublicBucket := utils.ContextKeyPublicBucket.IsSet(ctx) + parsedAcl := utils.ContextKeyParsedAcl.Get(ctx).(auth.ACL) + + err := auth.VerifyAccess(ctx.Context(), c.be, auth.AccessOptions{ + Readonly: c.readonly, + Acl: parsedAcl, + AclPermission: auth.PermissionRead, + IsRoot: isRoot, + Acc: acct, + Bucket: bucket, + Action: auth.GetBucketLocationAction, + IsBucketPublic: isPublicBucket, + }) + if err != nil { + return &Response{ + MetaOpts: &MetaOptions{ + BucketOwner: parsedAcl.Owner, + }, + }, err + } + + // verify bucket existence/access via backend HeadBucket + _, err = c.be.HeadBucket(ctx.Context(), &s3.HeadBucketInput{Bucket: &bucket}) + if err != nil { + return &Response{ + MetaOpts: &MetaOptions{ + BucketOwner: parsedAcl.Owner, + }, + }, err + } + + // pick up configured region from locals (set by router middleware) + region, _ := ctx.Locals("region").(string) + + return &Response{ + Data: s3response.LocationConstraint{ + Value: region, + }, + MetaOpts: &MetaOptions{ + BucketOwner: parsedAcl.Owner, + }, + }, nil +} diff --git a/s3api/controllers/bucket-get_test.go b/s3api/controllers/bucket-get_test.go index f45a45b..459df37 100644 --- a/s3api/controllers/bucket-get_test.go +++ b/s3api/controllers/bucket-get_test.go @@ -1266,3 +1266,83 @@ func TestS3ApiController_ListObjects(t *testing.T) { }) } } + +func TestS3ApiController_GetBucketLocation(t *testing.T) { + tests := []struct { + name string + input testInput + output testOutput + }{ + { + name: "verify access fails", + input: testInput{ + locals: accessDeniedLocals, + }, + output: testOutput{ + response: &Response{ + MetaOpts: &MetaOptions{ + BucketOwner: "root", + }, + }, + err: s3err.GetAPIError(s3err.ErrAccessDenied), + }, + }, + { + name: "backend returns error", + input: testInput{ + locals: defaultLocals, + beErr: s3err.GetAPIError(s3err.ErrNoSuchBucket), + }, + output: testOutput{ + response: &Response{ + MetaOpts: &MetaOptions{ + BucketOwner: "root", + }, + }, + err: s3err.GetAPIError(s3err.ErrNoSuchBucket), + }, + }, + { + name: "successful response", + input: testInput{ + locals: defaultLocals, + }, + output: testOutput{ + response: &Response{ + Data: s3response.LocationConstraint{ + Value: "us-east-1", + }, + MetaOpts: &MetaOptions{ + BucketOwner: "root", + }, + }, + }, + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + be := &BackendMock{ + HeadBucketFunc: func(contextMoqParam context.Context, headBucketInput *s3.HeadBucketInput) (*s3.HeadBucketOutput, error) { + return &s3.HeadBucketOutput{}, tt.input.beErr + }, + GetBucketPolicyFunc: func(contextMoqParam context.Context, bucket string) ([]byte, error) { + return nil, s3err.GetAPIError(s3err.ErrAccessDenied) + }, + } + + ctrl := S3ApiController{ + be: be, + } + + testController( + t, + ctrl.GetBucketLocation, + tt.output.response, + tt.output.err, + ctxInputs{ + locals: tt.input.locals, + body: tt.input.body, + }) + }) + } +} diff --git a/s3api/router.go b/s3api/router.go index 51ba5d6..043c5f4 100644 --- a/s3api/router.go +++ b/s3api/router.go @@ -611,6 +611,21 @@ func (sa *S3ApiRouter) Init(app *fiber.App, be backend.Backend, iam auth.IAMServ )) // GET bucket operations + bucketRouter.Get("", + middlewares.MatchQueryArgs("location"), + controllers.ProcessHandlers( + ctrl.GetBucketLocation, + metrics.ActionGetBucketLocation, + services, + middlewares.BucketObjectNameValidator(), + middlewares.AuthorizePublicBucketAccess(be, metrics.ActionGetBucketLocation, auth.GetBucketLocationAction, auth.PermissionRead), + middlewares.VerifyPresignedV4Signature(root, iam, region, debug), + middlewares.VerifyV4Signature(root, iam, region, debug), + middlewares.VerifyMD5Body(), + middlewares.ApplyBucketCORS(be), + middlewares.ParseAcl(be), + ), + ) bucketRouter.Get("", middlewares.MatchQueryArgs("tagging"), controllers.ProcessHandlers( diff --git a/s3response/s3response.go b/s3response/s3response.go index 67ca3e3..28c076b 100644 --- a/s3response/s3response.go +++ b/s3response/s3response.go @@ -721,3 +721,9 @@ type Checksum struct { SHA256 *string CRC64NVME *string } + +// LocationConstraint represents the GetBucketLocation response +type LocationConstraint struct { + XMLName xml.Name `xml:"http://s3.amazonaws.com/doc/2006-03-01/ LocationConstraint"` + Value string `xml:",chardata"` +} diff --git a/tests/integration/group-tests.go b/tests/integration/group-tests.go index 764aa21..16a493b 100644 --- a/tests/integration/group-tests.go +++ b/tests/integration/group-tests.go @@ -135,6 +135,12 @@ func TestDeleteBucketTagging(s *S3Conf) { DeleteBucketTagging_success(s) } +func TestGetBucketLocation(s *S3Conf) { + GetBucketLocation_success(s) + GetBucketLocation_non_exist(s) + GetBucketLocation_no_access(s) +} + func TestPutObject(s *S3Conf) { PutObject_non_existing_bucket(s) PutObject_special_chars(s) @@ -708,6 +714,7 @@ func TestFullFlow(s *S3Conf) { TestPutBucketTagging(s) TestGetBucketTagging(s) TestDeleteBucketTagging(s) + TestGetBucketLocation(s) TestPutObject(s) TestHeadObject(s) TestGetObjectAttributes(s) @@ -791,6 +798,7 @@ func TestScoutfs(s *S3Conf) { TestPutBucketTagging(s) TestGetBucketTagging(s) TestDeleteBucketTagging(s) + TestGetBucketLocation(s) TestPutObject(s) TestHeadObject(s) TestGetObjectAttributes(s) @@ -1092,6 +1100,9 @@ func GetIntTests() IntTests { "DeleteBucketTagging_non_existing_object": DeleteBucketTagging_non_existing_object, "DeleteBucketTagging_success_status": DeleteBucketTagging_success_status, "DeleteBucketTagging_success": DeleteBucketTagging_success, + "GetBucketLocation_success": GetBucketLocation_success, + "GetBucketLocation_non_exist": GetBucketLocation_non_exist, + "GetBucketLocation_no_access": GetBucketLocation_no_access, "PutObject_non_existing_bucket": PutObject_non_existing_bucket, "PutObject_special_chars": PutObject_special_chars, "PutObject_tagging": PutObject_tagging, diff --git a/tests/integration/tests.go b/tests/integration/tests.go index 07fd0b9..6dae124 100644 --- a/tests/integration/tests.go +++ b/tests/integration/tests.go @@ -1728,6 +1728,77 @@ func HeadBucket_success(s *S3Conf) error { }) } +func GetBucketLocation_success(s *S3Conf) error { + testName := "GetBucketLocation_success" + return actionHandler(s, testName, func(s3client *s3.Client, bucket string) error { + ctx, cancel := context.WithTimeout(context.Background(), shortTimeout) + resp, err := s3client.GetBucketLocation(ctx, &s3.GetBucketLocationInput{ + Bucket: &bucket, + }) + cancel() + if err != nil { + return err + } + + if string(resp.LocationConstraint) != s.awsRegion { + return fmt.Errorf("expected bucket region to be %v, instead got %v", + s.awsRegion, resp.LocationConstraint) + } + + return nil + }) +} + +func GetBucketLocation_non_exist(s *S3Conf) error { + testName := "GetBucketLocation_non_exist" + return actionHandler(s, testName, func(s3client *s3.Client, bucket string) error { + ctx, cancel := context.WithTimeout(context.Background(), shortTimeout) + invalidBucket := "bucket-no-exist" + resp, err := s3client.GetBucketLocation(ctx, &s3.GetBucketLocationInput{ + Bucket: &invalidBucket, + }) + cancel() + if err := checkApiErr(err, s3err.GetAPIError(s3err.ErrNoSuchBucket)); err != nil { + return err + } + + if resp != nil && resp.LocationConstraint != "" { + return fmt.Errorf("expected empty location constraint, instead got %v", + resp.LocationConstraint) + } + + return nil + }) +} + +func GetBucketLocation_no_access(s *S3Conf) error { + testName := "GetBucketLocation_no_access" + return actionHandler(s, testName, func(s3client *s3.Client, bucket string) error { + err := createUsers(s, []user{testuser1}) + if err != nil { + return err + } + + userClient := s.getUserClient(testuser1) + + ctx, cancel := context.WithTimeout(context.Background(), shortTimeout) + resp, err := userClient.GetBucketLocation(ctx, &s3.GetBucketLocationInput{ + Bucket: &bucket, + }) + cancel() + if err := checkApiErr(err, s3err.GetAPIError(s3err.ErrAccessDenied)); err != nil { + return err + } + + if resp != nil && resp.LocationConstraint != "" { + return fmt.Errorf("expected empty location constraint, instead got %v", + resp.LocationConstraint) + } + + return nil + }) +} + func ListBuckets_as_user(s *S3Conf) error { testName := "ListBuckets_as_user" return actionHandler(s, testName, func(s3client *s3.Client, bucket string) error {