Merge pull request #1871 from versity/sis/create-bucket-private-canned-acl

fix: correct private canned ACL behavior on bucket creation

auth/acl.go

@@ -496,7 +496,7 @@ func UpdateBucketACLOwner(ctx context.Context, be backend.Backend, bucket, newOw
 }
 
 // ValidateCannedACL validates bucket canned acl value
-func ValidateCannedACL(acl string) error {
+func ValidateCannedACL(acl types.BucketCannedACL) error {
 	switch types.BucketCannedACL(acl) {
 	case types.BucketCannedACLPrivate, types.BucketCannedACLPublicRead, types.BucketCannedACLPublicReadWrite, "":
 		return nil

s3api/controllers/bucket-put.go

@@ -321,7 +321,7 @@ func (c S3ApiController) PutBucketPolicy(ctx *fiber.Ctx) (*Response, error) {
 
 func (c S3ApiController) PutBucketAcl(ctx *fiber.Ctx) (*Response, error) {
 	bucket := ctx.Params("bucket")
-	acl := ctx.Get("X-Amz-Acl")
+	acl := types.BucketCannedACL(ctx.Get("X-Amz-Acl"))
 	grantFullControl := ctx.Get("X-Amz-Grant-Full-Control")
 	grantRead := ctx.Get("X-Amz-Grant-Read")
 	grantReadACP := ctx.Get("X-Amz-Grant-Read-Acp")
@@ -414,7 +414,7 @@ func (c S3ApiController) PutBucketAcl(ctx *fiber.Ctx) (*Response, error) {
 				}
 		}
 
-		if grants+acl != "" {
+		if grants+string(acl) != "" {
 			debuglogger.Logf("invalid request: %q (grants) %q (acl)",
 				grants, acl)
 			return &Response{
@@ -480,7 +480,7 @@ func (c S3ApiController) PutBucketAcl(ctx *fiber.Ctx) (*Response, error) {
 
 func (c S3ApiController) CreateBucket(ctx *fiber.Ctx) (*Response, error) {
 	bucket := ctx.Params("bucket")
-	acl := ctx.Get("X-Amz-Acl")
+	acl := types.BucketCannedACL(ctx.Get("X-Amz-Acl"))
 	grantFullControl := ctx.Get("X-Amz-Grant-Full-Control")
 	grantRead := ctx.Get("X-Amz-Grant-Read")
 	grantReadACP := ctx.Get("X-Amz-Grant-Read-Acp")
@@ -488,9 +488,7 @@ func (c S3ApiController) CreateBucket(ctx *fiber.Ctx) (*Response, error) {
 	grantWriteACP := ctx.Get("X-Amz-Grant-Write-Acp")
 	lockEnabled := strings.EqualFold(ctx.Get("X-Amz-Bucket-Object-Lock-Enabled"), "true")
 	grants := grantFullControl + grantRead + grantReadACP + grantWrite + grantWriteACP
-	objectOwnership := types.ObjectOwnership(
-		ctx.Get("X-Amz-Object-Ownership", string(types.ObjectOwnershipBucketOwnerEnforced)),
-	)
+	objectOwnership := types.ObjectOwnership(ctx.Get("X-Amz-Object-Ownership"))
 
 	if c.readonly {
 		return &Response{
@@ -519,6 +517,16 @@ func (c S3ApiController) CreateBucket(ctx *fiber.Ctx) (*Response, error) {
 		}, s3err.GetAPIError(s3err.ErrInvalidBucketName)
 	}
 
+	// both bucket canned ACL and acl grants is not allowed
+	if acl != "" && grants != "" {
+		debuglogger.Logf("invalid request: %q (grants) %q (acl)", grants, acl)
+		return &Response{
+			MetaOpts: &MetaOptions{
+				BucketOwner: bucketOwner.Access,
+			},
+		}, s3err.GetAPIError(s3err.ErrBothCannedAndHeaderGrants)
+	}
+
 	// validate bucket canned acl
 	err := auth.ValidateCannedACL(acl)
 	if err != nil {
@@ -529,6 +537,16 @@ func (c S3ApiController) CreateBucket(ctx *fiber.Ctx) (*Response, error) {
 		}, err
 	}
 
+	// if bucket acl is 'private', object ownership should default to 'BucketOwnerPreferred'
+	if acl == types.BucketCannedACLPrivate && objectOwnership == "" {
+		objectOwnership = types.ObjectOwnershipBucketOwnerPreferred
+	}
+
+	// if object ownership is so far empty, it should default to BucketOwnerEnforced
+	if objectOwnership == "" {
+		objectOwnership = types.ObjectOwnershipBucketOwnerEnforced
+	}
+
 	// validate the object ownership value
 	if ok := utils.IsValidOwnership(objectOwnership); !ok {
 		return &Response{
@@ -542,7 +560,11 @@ func (c S3ApiController) CreateBucket(ctx *fiber.Ctx) (*Response, error) {
 			}
 	}
 
-	if acl+grants != "" && objectOwnership == types.ObjectOwnershipBucketOwnerEnforced {
+	// any bucket ACL(canned, grants) is not allowed with object ownership 'BucketOwnerEnforced'
+	// but there's an exception for 'private' bucket canned ACL,
+	// which is the effective default for all buckets. In this case
+	// the ACL(private canned) is allowed with 'BucketOwnerEnforced'
+	if acl != types.BucketCannedACLPrivate && string(acl)+grants != "" && objectOwnership == types.ObjectOwnershipBucketOwnerEnforced {
 		debuglogger.Logf("bucket acls are disabled for %v object ownership", objectOwnership)
 		return &Response{
 			MetaOpts: &MetaOptions{
@@ -551,15 +573,6 @@ func (c S3ApiController) CreateBucket(ctx *fiber.Ctx) (*Response, error) {
 		}, s3err.GetAPIError(s3err.ErrInvalidBucketAclWithObjectOwnership)
 	}
 
-	if acl != "" && grants != "" {
-		debuglogger.Logf("invalid request: %q (grants) %q (acl)", grants, acl)
-		return &Response{
-			MetaOpts: &MetaOptions{
-				BucketOwner: bucketOwner.Access,
-			},
-		}, s3err.GetAPIError(s3err.ErrBothCannedAndHeaderGrants)
-	}
-
 	var body s3response.CreateBucketConfiguration
 	if len(ctx.Body()) != 0 {
 		// request body is optional for CreateBucket

tests/integration/CreateBucket.go

@@ -287,6 +287,133 @@ func CreateBucket_non_default_acl(s *S3Conf) error {
 	return nil
 }
 
+func CreateBucket_private_canned_acl(s *S3Conf) error {
+	testName := "CreateBucket_private_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.BucketCannedACLPrivate,
+		})
+		cancel()
+		if err != nil {
+			return err
+		}
+
+		ctx, cancel = context.WithTimeout(context.Background(), shortTimeout)
+		res, err := s3client.GetBucketAcl(ctx, &s3.GetBucketAclInput{
+			Bucket: &bucket,
+		})
+		cancel()
+		if err != nil {
+			return err
+		}
+
+		if getString(res.Owner.ID) != s.awsID {
+			return fmt.Errorf("expected bucket owner to be %v, instead got %v",
+				s.awsID, getString(res.Owner.ID))
+		}
+		if len(res.Grants) != 1 {
+			return fmt.Errorf("expected grants length to be 1, instead got %v",
+				len(res.Grants))
+		}
+		grt := res.Grants[0]
+		if grt.Permission != types.PermissionFullControl {
+			return fmt.Errorf("expected the grantee to have full-control permission, instead got %v",
+				grt.Permission)
+		}
+		if getString(grt.Grantee.ID) != s.awsID {
+			return fmt.Errorf("expected the grantee id to be %v, instead got %v",
+				s.awsID, getString(grt.Grantee.ID))
+		}
+
+		ctx, cancel = context.WithTimeout(context.Background(), shortTimeout)
+		out, err := s3client.GetBucketOwnershipControls(ctx, &s3.GetBucketOwnershipControlsInput{
+			Bucket: &bucket,
+		})
+		cancel()
+		if err != nil {
+			return err
+		}
+
+		if out.OwnershipControls == nil {
+			return fmt.Errorf("unexpected nil OwnershipControls")
+		}
+		if len(out.OwnershipControls.Rules) != 1 {
+			return fmt.Errorf("expected the OwnershipControls rules length to be 1, instead got %v", len(out.OwnershipControls.Rules))
+		}
+		if out.OwnershipControls.Rules[0].ObjectOwnership != types.ObjectOwnershipBucketOwnerPreferred {
+			return fmt.Errorf("expected the ObjectOwnership to be %s, instead got %s", types.ObjectOwnershipBucketOwnerPreferred, out.OwnershipControls.Rules[0].ObjectOwnership)
+		}
+
+		return teardown(s, bucket)
+	})
+}
+
+func CreateBucket_private_canned_acl_bucket_owner_enforced_ownership(s *S3Conf) error {
+	testName := "CreateBucket_private_canned_acl_bucket_owner_enforced_ownership"
+	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.BucketCannedACLPrivate,
+			ObjectOwnership: types.ObjectOwnershipBucketOwnerEnforced,
+		})
+		cancel()
+		if err != nil {
+			return err
+		}
+
+		ctx, cancel = context.WithTimeout(context.Background(), shortTimeout)
+		res, err := s3client.GetBucketAcl(ctx, &s3.GetBucketAclInput{
+			Bucket: &bucket,
+		})
+		cancel()
+		if err != nil {
+			return err
+		}
+
+		if getString(res.Owner.ID) != s.awsID {
+			return fmt.Errorf("expected bucket owner to be %v, instead got %v",
+				s.awsID, getString(res.Owner.ID))
+		}
+		if len(res.Grants) != 1 {
+			return fmt.Errorf("expected grants length to be 1, instead got %v",
+				len(res.Grants))
+		}
+		grt := res.Grants[0]
+		if grt.Permission != types.PermissionFullControl {
+			return fmt.Errorf("expected the grantee to have full-control permission, instead got %v",
+				grt.Permission)
+		}
+		if getString(grt.Grantee.ID) != s.awsID {
+			return fmt.Errorf("expected the grantee id to be %v, instead got %v",
+				s.awsID, getString(grt.Grantee.ID))
+		}
+
+		ctx, cancel = context.WithTimeout(context.Background(), shortTimeout)
+		out, err := s3client.GetBucketOwnershipControls(ctx, &s3.GetBucketOwnershipControlsInput{
+			Bucket: &bucket,
+		})
+		cancel()
+		if err != nil {
+			return err
+		}
+
+		if out.OwnershipControls == nil {
+			return fmt.Errorf("unexpected nil OwnershipControls")
+		}
+		if len(out.OwnershipControls.Rules) != 1 {
+			return fmt.Errorf("expected the OwnershipControls rules length to be 1, instead got %v", len(out.OwnershipControls.Rules))
+		}
+		if out.OwnershipControls.Rules[0].ObjectOwnership != types.ObjectOwnershipBucketOwnerEnforced {
+			return fmt.Errorf("expected the ObjectOwnership to be %s, instead got %s", types.ObjectOwnershipBucketOwnerEnforced, out.OwnershipControls.Rules[0].ObjectOwnership)
+		}
+
+		return teardown(s, bucket)
+	})
+}
+
 func CreateBucket_default_object_lock(s *S3Conf) error {
 	testName := "CreateBucket_default_object_lock"
 	runF(testName)

tests/integration/group-tests.go

@@ -79,6 +79,8 @@ func TestCreateBucket(ts *TestState) {
 	ts.Run(CreateBucket_as_user)
 	ts.Run(CreateBucket_default_acl)
 	ts.Run(CreateBucket_non_default_acl)
+	ts.Run(CreateBucket_private_canned_acl)
+	ts.Run(CreateBucket_private_canned_acl_bucket_owner_enforced_ownership)
 	ts.Run(CreateDeleteBucket_success)
 	ts.Run(CreateBucket_default_object_lock)
 	ts.Run(CreateBucket_invalid_location_constraint)
@@ -1247,6 +1249,8 @@ func GetIntTests() IntTests {
 		"CreateDeleteBucket_success":                                               CreateDeleteBucket_success,
 		"CreateBucket_default_acl":                                                 CreateBucket_default_acl,
 		"CreateBucket_non_default_acl":                                             CreateBucket_non_default_acl,
+		"CreateBucket_private_canned_acl":                                          CreateBucket_private_canned_acl,
+		"CreateBucket_private_canned_acl_bucket_owner_enforced_ownership":          CreateBucket_private_canned_acl_bucket_owner_enforced_ownership,
 		"CreateBucket_default_object_lock":                                         CreateBucket_default_object_lock,
 		"CreateBucket_invalid_location_constraint":                                 CreateBucket_invalid_location_constraint,
 		"CreateBucket_long_tags":                                                   CreateBucket_long_tags,