From 7dfea7a38be2f2db68f21c6af17783337f9643ba Mon Sep 17 00:00:00 2001 From: niksis02 Date: Mon, 31 Aug 2026 16:18:48 +0400 Subject: [PATCH] feat: govern GetBucketVersioning with the regular access checks MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit S3 explicitly documents this action as owner-only ("To retrieve the versioning state of a bucket, you must be the bucket owner." — https://docs.aws.amazon.com/AmazonS3/latest/API/API_GetBucketVersioning.html), and the handler enforced that with an extra auth.IsAdminOrOwner check on top of VerifyAccess. Real S3 behaves differently: verified against AWS that a bucket policy explicitly denying s3:GetBucketVersioning denies the bucket owner itself, and that an Allow grants the action to a principal that doesn't own the bucket. It goes through ordinary bucket policy/ACL evaluation like any other bucket subresource read, which is what the write side, PutBucketVersioning, already did here. Removes the extra check along with auth.IsAdminOrOwner, which had no other call site. --- auth/access-control.go | 20 ---- s3api/controllers/bucket-get.go | 8 -- s3api/controllers/bucket-get_test.go | 5 +- tests/integration/GetBucketVersioning.go | 113 +++++++++++++++++++++++ tests/integration/group-tests.go | 6 ++ 5 files changed, 122 insertions(+), 30 deletions(-) diff --git a/auth/access-control.go b/auth/access-control.go index 90f60eb0..8fa0dab9 100644 --- a/auth/access-control.go +++ b/auth/access-control.go @@ -618,26 +618,6 @@ func verifyIdentityOnlyAccess(ctx fiber.Ctx, pe PolicyEvaluator, acc Account, ac return s3err.GetImplicitDenyAccessErr(principal, string(action), resourceArn) } -func IsAdminOrOwner(acct Account, isRoot bool, acl ACL) error { - // Owner check - if acct.Access == acl.Owner { - return nil - } - - // Root user has access over almost everything - if isRoot { - return nil - } - - // Admin user case - if acct.Role == RoleAdmin { - return nil - } - - // Return access denied in all other cases - return s3err.GetAPIError(s3err.ErrAccessDenied) -} - type PublicACLAllowedActions map[Action]struct{} var publicACLAllowedActions PublicACLAllowedActions = PublicACLAllowedActions{ diff --git a/s3api/controllers/bucket-get.go b/s3api/controllers/bucket-get.go index 9e287fd7..96423fa0 100644 --- a/s3api/controllers/bucket-get.go +++ b/s3api/controllers/bucket-get.go @@ -138,14 +138,6 @@ func (c S3ApiController) GetBucketVersioning(ctx fiber.Ctx) (*Response, error) { }, }, err } - // Only admin users and the bucket owner are allowed to get the versioning state of a bucket. - if err := auth.IsAdminOrOwner(acct, isRoot, parsedAcl); err != nil { - return &Response{ - MetaOpts: &MetaOptions{ - BucketOwner: parsedAcl.Owner, - }, - }, err - } data, err := c.be.GetBucketVersioning(ctx.RequestCtx(), bucket) return &Response{ diff --git a/s3api/controllers/bucket-get_test.go b/s3api/controllers/bucket-get_test.go index 5736adcd..92ded301 100644 --- a/s3api/controllers/bucket-get_test.go +++ b/s3api/controllers/bucket-get_test.go @@ -231,7 +231,7 @@ func TestS3ApiController_GetBucketVersioning(t *testing.T) { }, }, { - name: "not admin or root", + name: "non owner user with verified access", input: testInput{ locals: map[utils.ContextKey]any{ utils.ContextKeyIsRoot: false, @@ -244,14 +244,15 @@ func TestS3ApiController_GetBucketVersioning(t *testing.T) { }, utils.ContextKeyPublicBucket: true, }, + beRes: validRes, }, output: testOutput{ response: &Response{ + Data: validRes, MetaOpts: &MetaOptions{ BucketOwner: "root", }, }, - err: s3err.GetAPIError(s3err.ErrAccessDenied), }, }, { diff --git a/tests/integration/GetBucketVersioning.go b/tests/integration/GetBucketVersioning.go index 517e01c3..128577f8 100644 --- a/tests/integration/GetBucketVersioning.go +++ b/tests/integration/GetBucketVersioning.go @@ -83,3 +83,116 @@ func GetBucketVersioning_success(s *S3Conf) error { return nil }, withVersioning(types.BucketVersioningStatusEnabled)) } + +func GetBucketVersioning_non_owner_access_denied(s *S3Conf) error { + testName := "GetBucketVersioning_non_owner_access_denied" + return actionHandler(s, testName, func(s3client *s3.Client, bucket string) error { + testuser := getUser("user") + if err := createUsers(s, []user{testuser}); err != nil { + return err + } + + userClient := s.getUserClient(testuser) + ctx, cancel := context.WithTimeout(context.Background(), shortTimeout) + _, err := userClient.GetBucketVersioning(ctx, &s3.GetBucketVersioningInput{ + Bucket: &bucket, + }) + cancel() + if err := checkApiErr(err, s3err.GetAPIError(s3err.ErrAccessDenied)); err != nil { + return err + } + + return nil + }, withVersioning(types.BucketVersioningStatusEnabled)) +} + +func GetBucketVersioning_with_policy_access(s *S3Conf) error { + testName := "GetBucketVersioning_with_policy_access" + return actionHandler(s, testName, func(s3client *s3.Client, bucket string) error { + testuser := getUser("user") + if err := createUsers(s, []user{testuser}); err != nil { + return err + } + + bucketResource := fmt.Sprintf(`"arn:aws:s3:::%v"`, bucket) + principal := fmt.Sprintf(`"%v"`, testuser.access) + userClient := s.getUserClient(testuser) + + // Error path: the policy grants the user another bucket action, + // but not s3:GetBucketVersioning. + policy := genPolicyDoc("Allow", principal, `"s3:GetBucketTagging"`, bucketResource) + if err := putBucketPolicy(s3client, bucket, policy); err != nil { + return err + } + + ctx, cancel := context.WithTimeout(context.Background(), shortTimeout) + _, err := userClient.GetBucketVersioning(ctx, &s3.GetBucketVersioningInput{ + Bucket: &bucket, + }) + cancel() + if err := checkApiErr(err, s3err.GetAPIError(s3err.ErrAccessDenied)); err != nil { + return err + } + + // Happy path: the policy grants s3:GetBucketVersioning to a user + // who neither owns the bucket nor is an admin. + policy = genPolicyDoc("Allow", principal, `"s3:GetBucketVersioning"`, bucketResource) + if err := putBucketPolicy(s3client, bucket, policy); err != nil { + return err + } + + ctx, cancel = context.WithTimeout(context.Background(), shortTimeout) + res, err := userClient.GetBucketVersioning(ctx, &s3.GetBucketVersioningInput{ + Bucket: &bucket, + }) + cancel() + if err != nil { + return err + } + + if res.Status != types.BucketVersioningStatusEnabled { + return fmt.Errorf("expected bucket versioning status to be %v, instead got %v", + types.BucketVersioningStatusEnabled, res.Status) + } + + return nil + }, withVersioning(types.BucketVersioningStatusEnabled)) +} + +func GetBucketVersioning_with_acl_access(s *S3Conf) error { + testName := "GetBucketVersioning_with_acl_access" + return actionHandler(s, testName, func(s3client *s3.Client, bucket string) error { + testuser := getUser("user") + if err := createUsers(s, []user{testuser}); err != nil { + return err + } + + ctx, cancel := context.WithTimeout(context.Background(), shortTimeout) + _, err := s3client.PutBucketAcl(ctx, &s3.PutBucketAclInput{ + Bucket: &bucket, + GrantRead: &testuser.access, + }) + cancel() + if err != nil { + return err + } + + userClient := s.getUserClient(testuser) + ctx, cancel = context.WithTimeout(context.Background(), shortTimeout) + res, err := userClient.GetBucketVersioning(ctx, &s3.GetBucketVersioningInput{ + Bucket: &bucket, + }) + cancel() + if err != nil { + return err + } + + if res.Status != types.BucketVersioningStatusEnabled { + return fmt.Errorf("expected bucket versioning status to be %v, instead got %v", + types.BucketVersioningStatusEnabled, res.Status) + } + + return nil + }, withVersioning(types.BucketVersioningStatusEnabled), + withOwnership(types.ObjectOwnershipBucketOwnerPreferred)) +} diff --git a/tests/integration/group-tests.go b/tests/integration/group-tests.go index 66b88ba9..bd296085 100644 --- a/tests/integration/group-tests.go +++ b/tests/integration/group-tests.go @@ -1874,6 +1874,9 @@ func TestVersioning(ts *TestState) { ts.Run(GetBucketVersioning_non_existing_bucket) ts.Run(GetBucketVersioning_empty_response) ts.Run(GetBucketVersioning_success) + ts.Run(GetBucketVersioning_non_owner_access_denied) + ts.Run(GetBucketVersioning_with_policy_access) + ts.Run(GetBucketVersioning_with_acl_access) // DeleteBucket action ts.Run(Versioning_DeleteBucket_not_empty) // PutObject action @@ -3343,6 +3346,9 @@ func GetIntTests() IntTests { "GetBucketVersioning_non_existing_bucket": GetBucketVersioning_non_existing_bucket, "GetBucketVersioning_empty_response": GetBucketVersioning_empty_response, "GetBucketVersioning_success": GetBucketVersioning_success, + "GetBucketVersioning_non_owner_access_denied": GetBucketVersioning_non_owner_access_denied, + "GetBucketVersioning_with_policy_access": GetBucketVersioning_with_policy_access, + "GetBucketVersioning_with_acl_access": GetBucketVersioning_with_acl_access, "Versioning_DeleteBucket_not_empty": Versioning_DeleteBucket_not_empty, "Versioning_PutObject_suspended_null_versionId_obj": Versioning_PutObject_suspended_null_versionId_obj, "Versioning_PutObject_null_versionId_obj": Versioning_PutObject_null_versionId_obj,