diff --git a/auth/acl.go b/auth/acl.go index 79ac6e1..0b0bba3 100644 --- a/auth/acl.go +++ b/auth/acl.go @@ -25,6 +25,7 @@ import ( "github.com/aws/aws-sdk-go-v2/service/s3" "github.com/aws/aws-sdk-go-v2/service/s3/types" "github.com/versity/versitygw/backend" + "github.com/versity/versitygw/debuglogger" "github.com/versity/versitygw/s3err" ) @@ -493,3 +494,14 @@ func UpdateBucketACLOwner(ctx context.Context, be backend.Backend, bucket, newOw return be.DeleteBucketPolicy(ctx, bucket) } + +// ValidateCannedACL validates bucket canned acl value +func ValidateCannedACL(acl string) error { + switch types.BucketCannedACL(acl) { + case types.BucketCannedACLPrivate, types.BucketCannedACLPublicRead, types.BucketCannedACLPublicReadWrite, "": + return nil + default: + debuglogger.Logf("invalid bucket canned acl: %v", acl) + return s3err.GetAPIError(s3err.ErrInvalidArgument) + } +} diff --git a/s3api/controllers/bucket-put.go b/s3api/controllers/bucket-put.go index af21dc4..143f9a1 100644 --- a/s3api/controllers/bucket-put.go +++ b/s3api/controllers/bucket-put.go @@ -352,6 +352,15 @@ func (c S3ApiController) PutBucketAcl(ctx *fiber.Ctx) (*Response, error) { }, err } + err = auth.ValidateCannedACL(acl) + if err != nil { + return &Response{ + MetaOpts: &MetaOptions{ + BucketOwner: parsedAcl.Owner, + }, + }, err + } + ownership, err := c.be.GetBucketOwnershipControls(ctx.Context(), bucket) if err != nil && !errors.Is(err, s3err.GetAPIError(s3err.ErrOwnershipControlsNotFound)) { return &Response{ @@ -419,14 +428,6 @@ func (c S3ApiController) PutBucketAcl(ctx *fiber.Ctx) (*Response, error) { AccessControlPolicy: &accessControlPolicy, } } else if acl != "" { - if acl != "private" && acl != "public-read" && acl != "public-read-write" { - debuglogger.Logf("invalid acl: %q", acl) - return &Response{ - MetaOpts: &MetaOptions{ - BucketOwner: parsedAcl.Owner, - }, - }, s3err.GetAPIError(s3err.ErrInvalidRequest) - } if grants != "" { debuglogger.Logf("invalid request: %q (grants) %q (acl)", grants, acl) @@ -500,14 +501,28 @@ func (c S3ApiController) CreateBucket(ctx *fiber.Ctx) (*Response, error) { // validate the bucket name if ok := utils.IsValidBucketName(bucket); !ok { return &Response{ - MetaOpts: &MetaOptions{}, + MetaOpts: &MetaOptions{ + BucketOwner: acct.Access, + }, }, s3err.GetAPIError(s3err.ErrInvalidBucketName) } + // validate bucket canned acl + err := auth.ValidateCannedACL(acl) + if err != nil { + return &Response{ + MetaOpts: &MetaOptions{ + BucketOwner: acct.Access, + }, + }, err + } + // validate the object ownership value if ok := utils.IsValidOwnership(objectOwnership); !ok { return &Response{ - MetaOpts: &MetaOptions{}, + MetaOpts: &MetaOptions{ + BucketOwner: acct.Access, + }, }, s3err.APIError{ Code: "InvalidArgument", Description: fmt.Sprintf("Invalid x-amz-object-ownership header: %v", objectOwnership), diff --git a/s3api/controllers/bucket-put_test.go b/s3api/controllers/bucket-put_test.go index 3dadc3f..689fef9 100644 --- a/s3api/controllers/bucket-put_test.go +++ b/s3api/controllers/bucket-put_test.go @@ -729,7 +729,9 @@ func TestS3ApiController_CreateBucket(t *testing.T) { }, output: testOutput{ response: &Response{ - MetaOpts: &MetaOptions{}, + MetaOpts: &MetaOptions{ + BucketOwner: adminAcc.Access, + }, }, err: s3err.GetAPIError(s3err.ErrInvalidBucketName), }, @@ -749,6 +751,24 @@ func TestS3ApiController_CreateBucket(t *testing.T) { err: s3err.GetAPIError(s3err.ErrMalformedXML), }, }, + + { + name: "invalid canned acl", + input: testInput{ + locals: map[utils.ContextKey]any{ + utils.ContextKeyAccount: adminAcc, + }, + headers: map[string]string{ + "x-amz-acl": "invalid_acl", + }, + }, + output: testOutput{ + response: &Response{ + MetaOpts: &MetaOptions{BucketOwner: adminAcc.Access}, + }, + err: s3err.GetAPIError(s3err.ErrInvalidArgument), + }, + }, { name: "invalid location constraint", input: testInput{ @@ -777,7 +797,9 @@ func TestS3ApiController_CreateBucket(t *testing.T) { }, output: testOutput{ response: &Response{ - MetaOpts: &MetaOptions{}, + MetaOpts: &MetaOptions{ + BucketOwner: adminAcc.Access, + }, }, err: s3err.APIError{ Code: "InvalidArgument", @@ -1079,7 +1101,7 @@ func TestS3ApiController_PutBucketAcl(t *testing.T) { BucketOwner: "root", }, }, - err: s3err.GetAPIError(s3err.ErrInvalidRequest), + err: s3err.GetAPIError(s3err.ErrInvalidArgument), }, }, { diff --git a/s3err/s3err.go b/s3err/s3err.go index 44a54dd..a2b1674 100644 --- a/s3err/s3err.go +++ b/s3err/s3err.go @@ -174,6 +174,7 @@ const ( ErrCORSIsNotEnabled ErrNotModified ErrInvalidLocationConstraint + ErrInvalidArgument // Non-AWS errors ErrExistingObjectIsDirectory @@ -768,6 +769,11 @@ var errorCodeResponse = map[ErrorCode]APIError{ Description: "The specified location-constraint is not valid", HTTPStatusCode: http.StatusBadRequest, }, + ErrInvalidArgument: { + Code: "InvalidArgument", + Description: "", + HTTPStatusCode: http.StatusBadRequest, + }, // non aws errors ErrExistingObjectIsDirectory: { diff --git a/tests/integration/CreateBucket.go b/tests/integration/CreateBucket.go index b1756ec..af95dd7 100644 --- a/tests/integration/CreateBucket.go +++ b/tests/integration/CreateBucket.go @@ -490,3 +490,16 @@ func CreateBucket_tag_count_limit(s *S3Conf) error { return checkApiErr(err, s3err.GetAPIError(s3err.ErrBucketTaggingLimited)) }) } + +func CreateBucket_invalid_canned_acl(s *S3Conf) error { + testName := "CreateBucket_invalid_canned_acl" + return actionHandlerNoSetup(s, testName, func(s3client *s3.Client, bucket string) error { + ctx, cancel := context.WithTimeout(context.Background(), shortTimeout) + _, err := s3client.CreateBucket(ctx, &s3.CreateBucketInput{ + Bucket: &bucket, + ACL: types.BucketCannedACL("invalid_acl"), + }) + cancel() + return checkSdkApiErr(err, "InvalidArgument") + }) +} diff --git a/tests/integration/PutBucketAcl.go b/tests/integration/PutBucketAcl.go index f655b26..d5689a0 100644 --- a/tests/integration/PutBucketAcl.go +++ b/tests/integration/PutBucketAcl.go @@ -71,6 +71,21 @@ func PutBucketAcl_none_of_the_options_specified(s *S3Conf) error { }, withOwnership(types.ObjectOwnershipBucketOwnerPreferred)) } +func PutBucketAcl_invalid_canned_acl(s *S3Conf) error { + testName := "PutBucketAcl_invalid_canned_acl" + return actionHandler(s, testName, func(s3client *s3.Client, bucket string) error { + ctx, cancel := context.WithTimeout(context.Background(), shortTimeout) + _, err := s3client.PutBucketAcl(ctx, &s3.PutBucketAclInput{ + Bucket: &bucket, + ACL: types.BucketCannedACL("invalid_acl"), + }) + cancel() + // the expected error message is empty for this case + // only check the error Code + return checkSdkApiErr(err, "InvalidArgument") + }, withOwnership(types.ObjectOwnershipBucketOwnerPreferred)) +} + func PutBucketAcl_invalid_acl_canned_and_acp(s *S3Conf) error { testName := "PutBucketAcl_invalid_acl_canned_and_acp" 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 dfc888c..caef6f1 100644 --- a/tests/integration/group-tests.go +++ b/tests/integration/group-tests.go @@ -85,6 +85,7 @@ func TestCreateBucket(ts *TestState) { ts.Run(CreateBucket_invalid_tags) ts.Run(CreateBucket_duplicate_keys) ts.Run(CreateBucket_tag_count_limit) + ts.Run(CreateBucket_invalid_canned_acl) } func TestHeadBucket(ts *TestState) { @@ -509,6 +510,7 @@ func TestPutBucketAcl(ts *TestState) { ts.Run(PutBucketAcl_non_existing_bucket) ts.Run(PutBucketAcl_disabled) ts.Run(PutBucketAcl_none_of_the_options_specified) + ts.Run(PutBucketAcl_invalid_canned_acl) ts.Run(PutBucketAcl_invalid_acl_canned_and_acp) ts.Run(PutBucketAcl_invalid_acl_canned_and_grants) ts.Run(PutBucketAcl_invalid_acl_acp_and_grants) @@ -1145,6 +1147,7 @@ func GetIntTests() IntTests { "CreateBucket_invalid_tags": CreateBucket_invalid_tags, "CreateBucket_duplicate_keys": CreateBucket_duplicate_keys, "CreateBucket_tag_count_limit": CreateBucket_tag_count_limit, + "CreateBucket_invalid_canned_acl": CreateBucket_invalid_canned_acl, "HeadBucket_non_existing_bucket": HeadBucket_non_existing_bucket, "HeadBucket_success": HeadBucket_success, "ListBuckets_as_user": ListBuckets_as_user, @@ -1410,6 +1413,7 @@ func GetIntTests() IntTests { "PutBucketAcl_non_existing_bucket": PutBucketAcl_non_existing_bucket, "PutBucketAcl_disabled": PutBucketAcl_disabled, "PutBucketAcl_none_of_the_options_specified": PutBucketAcl_none_of_the_options_specified, + "PutBucketAcl_invalid_canned_acl": PutBucketAcl_invalid_canned_acl, "PutBucketAcl_invalid_acl_canned_and_acp": PutBucketAcl_invalid_acl_canned_and_acp, "PutBucketAcl_invalid_acl_canned_and_grants": PutBucketAcl_invalid_acl_canned_and_grants, "PutBucketAcl_invalid_acl_acp_and_grants": PutBucketAcl_invalid_acl_acp_and_grants,