From d39685947d3e494564e0916e40b0eeb72136b3d2 Mon Sep 17 00:00:00 2001 From: niksis02 Date: Wed, 15 Oct 2025 19:20:04 +0400 Subject: [PATCH] feat: adds the x-amz-expected-bucket-owner check in the gateway MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Fixes #1428 The `x-amz-expected-bucket-owner` header in S3 specifies the account ID of the expected bucket owner. If the account ID provided does not match the actual owner of the bucket, the request fails with an HTTP 403 Forbidden (AccessDenied) error. If the provided account ID is not 12 characters long, S3 returns a 400 Bad Request error. In our case, we expect the header to contain the bucket owner’s access key ID, and we skip validation errors related to the access key ID, since there is no validation mechanism for user access key IDs. If the provided value does not match the bucket owner’s access key ID, the gateway returns an AccessDenied error. A few integration tests are added for random actions, as this feature applies to all actions, but it is unnecessary to add test cases for every single one. --- s3api/middlewares/acl-parser.go | 11 ++++ tests/integration/group-tests.go | 8 +++ tests/integration/tests.go | 100 +++++++++++++++++++++++++++++++ 3 files changed, 119 insertions(+) diff --git a/s3api/middlewares/acl-parser.go b/s3api/middlewares/acl-parser.go index 53f3ec8a..e138a11f 100644 --- a/s3api/middlewares/acl-parser.go +++ b/s3api/middlewares/acl-parser.go @@ -20,6 +20,7 @@ import ( "github.com/versity/versitygw/auth" "github.com/versity/versitygw/backend" "github.com/versity/versitygw/s3api/utils" + "github.com/versity/versitygw/s3err" ) // ParseAcl retreives the bucket acl and stores in the context locals @@ -42,6 +43,16 @@ func ParseAcl(be backend.Backend) fiber.Handler { parsedAcl.Owner = utils.ContextKeyRootAccessKey.Get(ctx).(string) } + // if expected bucket owner doesn't match the bucket owner + // the gateway should return AccessDenied. + // This header appears in all actions except 'CreateBucket' and 'ListBuckets'. + // 'ParseACL' is also applied to all actions except for 'CreateBucket' and 'ListBuckets', + // so it's a perfect place to check the expected bucket owner + bucketOwner := ctx.Get("X-Amz-Expected-Bucket-Owner") + if bucketOwner != "" && bucketOwner != parsedAcl.Owner { + return s3err.GetAPIError(s3err.ErrAccessDenied) + } + utils.ContextKeyParsedAcl.Set(ctx, parsedAcl) return nil } diff --git a/tests/integration/group-tests.go b/tests/integration/group-tests.go index 2e81bc49..dbb06980 100644 --- a/tests/integration/group-tests.go +++ b/tests/integration/group-tests.go @@ -101,6 +101,7 @@ func TestDeleteBucket(ts *TestState) { ts.Run(DeleteBucket_non_existing_bucket) ts.Run(DeleteBucket_non_empty_bucket) ts.Run(DeleteBucket_success_status_code) + ts.Run(DeleteBucket_incorrect_expected_bucket_owner) } func TestPutBucketOwnershipControls(ts *TestState) { @@ -291,6 +292,8 @@ func TestDeleteObject(ts *TestState) { ts.Run(DeleteObject_conditional_writes) ts.Run(DeleteObject_success) ts.Run(DeleteObject_success_status_code) + ts.Run(DeleteObject_incorrect_expected_bucket_owner) + ts.Run(DeleteObject_expected_bucket_owner) } func TestDeleteObjects(ts *TestState) { @@ -348,6 +351,7 @@ func TestDeleteObjectTagging(ts *TestState) { ts.Run(DeleteObjectTagging_non_existing_object) ts.Run(DeleteObjectTagging_success_status) ts.Run(DeleteObjectTagging_success) + ts.Run(DeleteObjectTagging_expected_bucket_owner) } func TestCreateMultipartUpload(ts *TestState) { @@ -1128,6 +1132,7 @@ func GetIntTests() IntTests { "ListBuckets_success": ListBuckets_success, "DeleteBucket_non_existing_bucket": DeleteBucket_non_existing_bucket, "DeleteBucket_non_empty_bucket": DeleteBucket_non_empty_bucket, + "DeleteBucket_incorrect_expected_bucket_owner": DeleteBucket_incorrect_expected_bucket_owner, "DeleteBucket_success_status_code": DeleteBucket_success_status_code, "PutBucketOwnershipControls_non_existing_bucket": PutBucketOwnershipControls_non_existing_bucket, "PutBucketOwnershipControls_multiple_rules": PutBucketOwnershipControls_multiple_rules, @@ -1238,6 +1243,8 @@ func GetIntTests() IntTests { "DeleteObject_directory_object": DeleteObject_directory_object, "DeleteObject_success": DeleteObject_success, "DeleteObject_success_status_code": DeleteObject_success_status_code, + "DeleteObject_incorrect_expected_bucket_owner": DeleteObject_incorrect_expected_bucket_owner, + "DeleteObject_expected_bucket_owner": DeleteObject_expected_bucket_owner, "DeleteObjects_empty_input": DeleteObjects_empty_input, "DeleteObjects_non_existing_objects": DeleteObjects_non_existing_objects, "DeleteObjects_success": DeleteObjects_success, @@ -1277,6 +1284,7 @@ func GetIntTests() IntTests { "DeleteObjectTagging_non_existing_object": DeleteObjectTagging_non_existing_object, "DeleteObjectTagging_success_status": DeleteObjectTagging_success_status, "DeleteObjectTagging_success": DeleteObjectTagging_success, + "DeleteObjectTagging_expected_bucket_owner": DeleteObjectTagging_expected_bucket_owner, "CreateMultipartUpload_non_existing_bucket": CreateMultipartUpload_non_existing_bucket, "CreateMultipartUpload_with_metadata": CreateMultipartUpload_with_metadata, "CreateMultipartUpload_with_tagging": CreateMultipartUpload_with_tagging, diff --git a/tests/integration/tests.go b/tests/integration/tests.go index a7c442ab..6416e6ed 100644 --- a/tests/integration/tests.go +++ b/tests/integration/tests.go @@ -2300,6 +2300,19 @@ func DeleteBucket_success_status_code(s *S3Conf) error { return nil } +func DeleteBucket_incorrect_expected_bucket_owner(s *S3Conf) error { + testName := "DeleteBucket_incorrect_expected_bucket_owner" + return actionHandler(s, testName, func(s3client *s3.Client, bucket string) error { + ctx, cancel := context.WithTimeout(context.Background(), shortTimeout) + _, err := s3client.DeleteBucket(ctx, &s3.DeleteBucketInput{ + Bucket: &bucket, + ExpectedBucketOwner: getPtr(s.awsID + "something"), + }) + cancel() + return checkApiErr(err, s3err.GetAPIError(s3err.ErrAccessDenied)) + }) +} + func PutBucketOwnershipControls_non_existing_bucket(s *S3Conf) error { testName := "PutBucketOwnershipControls_non_existing_bucket" return actionHandler(s, testName, func(s3client *s3.Client, bucket string) error { @@ -7191,6 +7204,38 @@ func DeleteObject_success_status_code(s *S3Conf) error { }) } +func DeleteObject_incorrect_expected_bucket_owner(s *S3Conf) error { + testName := "DeleteObject_incorrect_expected_bucket_owner" + return actionHandler(s, testName, func(s3client *s3.Client, bucket string) error { + ctx, cancel := context.WithTimeout(context.Background(), shortTimeout) + _, err := s3client.DeleteObject(ctx, &s3.DeleteObjectInput{ + Bucket: &bucket, + // anyways if object doesn't exist, a 200 response should be received + Key: getPtr("my-obj"), + ExpectedBucketOwner: getPtr(s.awsID + "something"), + }) + cancel() + + return checkApiErr(err, s3err.GetAPIError(s3err.ErrAccessDenied)) + }) +} + +func DeleteObject_expected_bucket_owner(s *S3Conf) error { + testName := "DeleteObject_expected_bucket_owner" + return actionHandler(s, testName, func(s3client *s3.Client, bucket string) error { + ctx, cancel := context.WithTimeout(context.Background(), shortTimeout) + _, err := s3client.DeleteObject(ctx, &s3.DeleteObjectInput{ + Bucket: &bucket, + // anyways if object doesn't exist, a 200 response should be received + Key: getPtr("my-obj"), + ExpectedBucketOwner: &s.awsID, + }) + cancel() + + return err + }) +} + func DeleteObjects_empty_input(s *S3Conf) error { testName := "DeleteObjects_empty_input" return actionHandler(s, testName, func(s3client *s3.Client, bucket string) error { @@ -8994,6 +9039,61 @@ func DeleteObjectTagging_success(s *S3Conf) error { }) } +func DeleteObjectTagging_expected_bucket_owner(s *S3Conf) error { + testName := "DeleteObjectTagging_expected_bucket_owner" + return actionHandler(s, testName, func(s3client *s3.Client, bucket string) error { + obj := "my-obj" + tagging := types.Tagging{TagSet: []types.Tag{ + {Key: getPtr("key1"), Value: getPtr("val2")}, + {Key: getPtr("key2"), Value: getPtr("val2")}, + }} + _, err := putObjects(s3client, []string{obj}, bucket) + if err != nil { + return err + } + + ctx, cancel := context.WithTimeout(context.Background(), shortTimeout) + _, err = s3client.PutObjectTagging(ctx, &s3.PutObjectTaggingInput{ + Bucket: &bucket, + Key: &obj, + Tagging: &tagging, + ExpectedBucketOwner: &s.awsID, + }) + cancel() + if err != nil { + return err + } + + ctx, cancel = context.WithTimeout(context.Background(), shortTimeout) + _, err = s3client.DeleteObjectTagging(ctx, &s3.DeleteObjectTaggingInput{ + Bucket: &bucket, + Key: &obj, + ExpectedBucketOwner: &s.awsID, + }) + cancel() + if err != nil { + return nil + } + + ctx, cancel = context.WithTimeout(context.Background(), shortTimeout) + out, err := s3client.GetObjectTagging(ctx, &s3.GetObjectTaggingInput{ + Bucket: &bucket, + Key: &obj, + ExpectedBucketOwner: &s.awsID, + }) + cancel() + if err != nil { + return nil + } + + if len(out.TagSet) > 0 { + return fmt.Errorf("expected empty tag set, instead got %v", out.TagSet) + } + + return nil + }) +} + func CreateMultipartUpload_non_existing_bucket(s *S3Conf) error { testName := "CreateMultipartUpload_non_existing_bucket" return actionHandler(s, testName, func(s3client *s3.Client, bucket string) error {