feat: Closes #441, Added access control integration tests, fixed some bugs in bucket policy and acl access checking flow

auth/acl.go

@@ -206,15 +206,6 @@ func splitUnique(s, divider string) []string {
 }
 
 func verifyACL(acl ACL, access string, permission types.Permission) error {
-	// Default disabled ACL case
-	if acl.ACL == "" && len(acl.Grantees) == 0 {
-		if acl.Owner == access {
-			return nil
-		}
-
-		return s3err.GetAPIError(s3err.ErrAccessDenied)
-	}
-
 	if acl.ACL != "" {
 		if (permission == "READ" || permission == "READ_ACP") && (acl.ACL != "public-read" && acl.ACL != "public-read-write") {
 			return s3err.GetAPIError(s3err.ErrAccessDenied)
@@ -225,6 +216,9 @@ func verifyACL(acl ACL, access string, permission types.Permission) error {
 
 		return nil
 	} else {
+		if len(acl.Grantees) == 0 {
+			return nil
+		}
 		grantee := Grantee{Access: access, Permission: permission}
 		granteeFullCtrl := Grantee{Access: access, Permission: "FULL_CONTROL"}
 
@@ -298,10 +292,20 @@ func VerifyAccess(ctx context.Context, be backend.Backend, opts AccessOptions) e
 		return nil
 	}
 
-	if err := verifyACL(opts.Acl, opts.Acc.Access, opts.AclPermission); err != nil {
+	policy, err := be.GetBucketPolicy(ctx, opts.Bucket)
+	if err != nil {
 		return err
 	}
-	if err := verifyBucketPolicy(ctx, be, opts.Acc.Access, opts.Bucket, opts.Object, opts.Action); err != nil {
+
+	// If bucket policy is not set and the ACL is default, only the owner has access
+	if len(policy) == 0 && opts.Acl.ACL == "" && len(opts.Acl.Grantees) == 0 {
+		return s3err.GetAPIError(s3err.ErrAccessDenied)
+	}
+
+	if err := verifyBucketPolicy(policy, opts.Acc.Access, opts.Bucket, opts.Object, opts.Action); err != nil {
+		return err
+	}
+	if err := verifyACL(opts.Acl, opts.Acc.Access, opts.AclPermission); err != nil {
 		return err
 	}
 

auth/bucket_policy.go

@@ -15,12 +15,10 @@
 package auth
 
 import (
-	"context"
 	"encoding/json"
 	"fmt"
 	"net/http"
 
-	"github.com/versity/versitygw/backend"
 	"github.com/versity/versitygw/s3err"
 )
 
@@ -41,8 +39,13 @@ func (bp *BucketPolicy) Validate(bucket string, iam IAMService) error {
 
 func (bp *BucketPolicy) isAllowed(principal string, action Action, resource string) bool {
 	for _, statement := range bp.Statement {
-		if statement.isAllowed(principal, action, resource) {
-			return true
+		if statement.findMatch(principal, action, resource) {
+			switch statement.Effect {
+			case BucketPolicyAccessTypeAllow:
+				return true
+			case BucketPolicyAccessTypeDeny:
+				return false
+			}
 		}
 	}
 
@@ -83,14 +86,9 @@ func (bpi *BucketPolicyItem) Validate(bucket string, iam IAMService) error {
 	return nil
 }
 
-func (bpi *BucketPolicyItem) isAllowed(principal string, action Action, resource string) bool {
+func (bpi *BucketPolicyItem) findMatch(principal string, action Action, resource string) bool {
 	if bpi.Principals.Contains(principal) && bpi.Actions.FindMatch(action) && bpi.Resources.FindMatch(resource) {
-		switch bpi.Effect {
-		case BucketPolicyAccessTypeAllow:
-			return true
-		case BucketPolicyAccessTypeDeny:
-			return false
-		}
+		return true
 	}
 
 	return false
@@ -117,26 +115,23 @@ func ValidatePolicyDocument(policyBin []byte, bucket string, iam IAMService) err
 	return nil
 }
 
-func verifyBucketPolicy(ctx context.Context, be backend.Backend, access, bucket, object string, action Action) error {
-	policyDoc, err := be.GetBucketPolicy(ctx, bucket)
-	if err != nil {
-		return err
-	}
+func verifyBucketPolicy(policy []byte, access, bucket, object string, action Action) error {
 	// If bucket policy is not set
-	if len(policyDoc) == 0 {
+	if len(policy) == 0 {
 		return nil
 	}
 
 	var bucketPolicy BucketPolicy
-	if err := json.Unmarshal(policyDoc, &bucketPolicy); err != nil {
+	if err := json.Unmarshal(policy, &bucketPolicy); err != nil {
 		return err
 	}
 
 	resource := bucket
 	if object != "" {
-		resource += "" + object
+		resource += "/" + object
 	}
 
+	fmt.Println(access, action, resource)
 	if !bucketPolicy.isAllowed(access, action, resource) {
 		return s3err.GetAPIError(s3err.ErrAccessDenied)
 	}

backend/posix/posix.go

@@ -1728,9 +1728,10 @@ func (p *Posix) PutBucketTagging(_ context.Context, bucket string, tags map[stri
 
 	if tags == nil {
 		err = xattr.Remove(bucket, "user."+tagHdr)
-		if err != nil {
+		if err != nil && !isNoAttr(err) {
 			return fmt.Errorf("remove tags: %w", err)
 		}
+
 		return nil
 	}
 

cmd/versitygw/test.go

@@ -84,6 +84,11 @@ func initTestCommands() []*cli.Command {
 			Usage:  "Tests iam service",
 			Action: getAction(integration.TestIAM),
 		},
+		{
+			Name:   "access-control",
+			Usage:  "Tests gateway access control with bucket ACLs and Policies",
+			Action: getAction(integration.TestAccessControl),
+		},
 		{
 			Name:  "bench",
 			Usage: "Runs download/upload performance test on the gateway",

s3api/middlewares/acl-parser.go

@@ -54,7 +54,6 @@ func AclParser(be backend.Backend, logger s3log.AuditLogger) fiber.Handler {
 			}
 			return ctx.Next()
 		}
-		//TODO: provide correct action names for the logger, after implementing DetectAction middleware
 		data, err := be.GetBucketAcl(ctx.Context(), &s3.GetBucketAclInput{Bucket: &bucket})
 		if err != nil {
 			return controllers.SendResponse(ctx, err, &controllers.MetaOpts{Logger: logger})

tests/integration/group-tests.go

@@ -309,6 +309,7 @@ func TestFullFlow(s *S3Conf) {
 	TestPutBucketPolicy(s)
 	TestGetBucketPolicy(s)
 	TestDeleteBucketPolicy(s)
+	TestAccessControl(s)
 }
 
 func TestPosix(s *S3Conf) {
@@ -325,6 +326,16 @@ func TestIAM(s *S3Conf) {
 	IAM_admin_ChangeBucketOwner(s)
 }
 
+func TestAccessControl(s *S3Conf) {
+	AccessControl_default_ACL_user_access_denied(s)
+	AccessControl_default_ACL_userplus_access_denied(s)
+	AccessControl_default_ACL_admin_successful_access(s)
+	AccessControl_bucket_resource_single_action(s)
+	AccessControl_bucket_resource_all_action(s)
+	AccessControl_single_object_resource_actions(s)
+	AccessControl_multi_statement_policy(s)
+}
+
 type IntTests map[string]func(s *S3Conf) error
 
 func GetIntTests() IntTests {

tests/integration/tests.go

@@ -5797,6 +5797,320 @@ func DeleteBucketPolicy_success(s *S3Conf) error {
 	})
 }
 
+// Access control tests (with bucket ACLs and Policies)
+func AccessControl_default_ACL_user_access_denied(s *S3Conf) error {
+	testName := "AccessControl_default_ACL_user_access_denied"
+	return actionHandler(s, testName, func(s3client *s3.Client, bucket string) error {
+		usr := user{
+			access: "grt1",
+			secret: "grt1secret",
+			role:   "user",
+		}
+		err := createUsers(s, []user{usr})
+		if err != nil {
+			return err
+		}
+
+		cfg := *s
+		cfg.awsID = usr.access
+		cfg.awsSecret = usr.secret
+
+		err = putObjects(s3.NewFromConfig(cfg.Config()), []string{"my-obj"}, bucket)
+		if err := checkApiErr(err, s3err.GetAPIError(s3err.ErrAccessDenied)); err != nil {
+			return err
+		}
+
+		return nil
+	})
+}
+
+func AccessControl_default_ACL_userplus_access_denied(s *S3Conf) error {
+	testName := "AccessControl_default_ACL_userplus_access_denied"
+	return actionHandler(s, testName, func(s3client *s3.Client, bucket string) error {
+		usr := user{
+			access: "userplus1",
+			secret: "userplus1secret",
+			role:   "userplus",
+		}
+		err := createUsers(s, []user{usr})
+		if err != nil {
+			return err
+		}
+
+		cfg := *s
+		cfg.awsID = usr.access
+		cfg.awsSecret = usr.secret
+
+		err = putObjects(s3.NewFromConfig(cfg.Config()), []string{"my-obj"}, bucket)
+		if err := checkApiErr(err, s3err.GetAPIError(s3err.ErrAccessDenied)); err != nil {
+			return err
+		}
+
+		return nil
+	})
+}
+
+func AccessControl_default_ACL_admin_successful_access(s *S3Conf) error {
+	testName := "AccessControl_default_ACL_admin_successful_access"
+	return actionHandler(s, testName, func(s3client *s3.Client, bucket string) error {
+		admin := user{
+			access: "admin1",
+			secret: "admin1secret",
+			role:   "admin",
+		}
+		err := createUsers(s, []user{admin})
+		if err != nil {
+			return err
+		}
+
+		cfg := *s
+		cfg.awsID = admin.access
+		cfg.awsSecret = admin.secret
+
+		err = putObjects(s3.NewFromConfig(cfg.Config()), []string{"my-obj"}, bucket)
+		if err != nil {
+			return err
+		}
+
+		return nil
+	})
+}
+
+func AccessControl_bucket_resource_single_action(s *S3Conf) error {
+	testName := "AccessControl_bucket_resource_single_action"
+	return actionHandler(s, testName, func(s3client *s3.Client, bucket string) error {
+		usr1 := user{
+			access: "grt1",
+			secret: "grt1secret",
+			role:   "user",
+		}
+		usr2 := user{
+			access: "grt2",
+			secret: "grt2secret",
+			role:   "user",
+		}
+		err := createUsers(s, []user{usr1, usr2})
+		if err != nil {
+			return err
+		}
+
+		doc := genPolicyDoc("Allow", `["grt1"]`, `"s3:PutBucketTagging"`, fmt.Sprintf(`"arn:aws:s3:::%v"`, bucket))
+		ctx, cancel := context.WithTimeout(context.Background(), shortTimeout)
+		_, err = s3client.PutBucketPolicy(ctx, &s3.PutBucketPolicyInput{
+			Bucket: &bucket,
+			Policy: &doc,
+		})
+		cancel()
+		if err != nil {
+			return err
+		}
+
+		user1Client := getUserS3Client(usr1, s)
+
+		ctx, cancel = context.WithTimeout(context.Background(), shortTimeout)
+		_, err = user1Client.DeleteBucketTagging(ctx, &s3.DeleteBucketTaggingInput{
+			Bucket: &bucket,
+		})
+		cancel()
+		if err != nil {
+			return err
+		}
+
+		ctx, cancel = context.WithTimeout(context.Background(), shortTimeout)
+		_, err = user1Client.GetBucketTagging(ctx, &s3.GetBucketTaggingInput{
+			Bucket: &bucket,
+		})
+		cancel()
+		if err := checkApiErr(err, s3err.GetAPIError(s3err.ErrAccessDenied)); err != nil {
+			return err
+		}
+
+		user2Client := getUserS3Client(usr2, s)
+
+		ctx, cancel = context.WithTimeout(context.Background(), shortTimeout)
+		_, err = user2Client.DeleteBucketTagging(ctx, &s3.DeleteBucketTaggingInput{
+			Bucket: &bucket,
+		})
+		cancel()
+		if err := checkApiErr(err, s3err.GetAPIError(s3err.ErrAccessDenied)); err != nil {
+			return err
+		}
+
+		return nil
+	})
+}
+
+func AccessControl_bucket_resource_all_action(s *S3Conf) error {
+	testName := "AccessControl_bucket_resource_all_action"
+	return actionHandler(s, testName, func(s3client *s3.Client, bucket string) error {
+		usr1 := user{
+			access: "grt1",
+			secret: "grt1secret",
+			role:   "user",
+		}
+		usr2 := user{
+			access: "grt2",
+			secret: "grt2secret",
+			role:   "user",
+		}
+		err := createUsers(s, []user{usr1, usr2})
+		if err != nil {
+			return err
+		}
+
+		bucketResource := fmt.Sprintf(`"arn:aws:s3:::%v"`, bucket)
+		objectResource := fmt.Sprintf(`"arn:aws:s3:::%v/*"`, bucket)
+		doc := genPolicyDoc("Allow", `["grt1"]`, `"s3:*"`, fmt.Sprintf(`[%v, %v]`, bucketResource, objectResource))
+
+		ctx, cancel := context.WithTimeout(context.Background(), shortTimeout)
+		_, err = s3client.PutBucketPolicy(ctx, &s3.PutBucketPolicyInput{
+			Bucket: &bucket,
+			Policy: &doc,
+		})
+		cancel()
+		if err != nil {
+			return err
+		}
+
+		user1Client := getUserS3Client(usr1, s)
+		err = putObjects(user1Client, []string{"my-obj"}, bucket)
+		if err != nil {
+			return err
+		}
+
+		user2Client := getUserS3Client(usr2, s)
+
+		err = putObjects(user2Client, []string{"my-obj"}, bucket)
+		if err := checkApiErr(err, s3err.GetAPIError(s3err.ErrAccessDenied)); err != nil {
+			return err
+		}
+
+		return nil
+	})
+}
+
+func AccessControl_single_object_resource_actions(s *S3Conf) error {
+	testName := "AccessControl_single_object_resource_actions"
+	return actionHandler(s, testName, func(s3client *s3.Client, bucket string) error {
+		obj := "my-obj/nested-obj"
+		err := putObjects(s3client, []string{obj}, bucket)
+		if err != nil {
+			return err
+		}
+
+		usr1 := user{
+			access: "grt1",
+			secret: "grt1secret",
+			role:   "user",
+		}
+		err = createUsers(s, []user{usr1})
+		if err != nil {
+			return err
+		}
+
+		doc := genPolicyDoc("Allow", `["grt1"]`, `"s3:*"`, fmt.Sprintf(`"arn:aws:s3:::%v/%v"`, bucket, obj))
+
+		ctx, cancel := context.WithTimeout(context.Background(), shortTimeout)
+		_, err = s3client.PutBucketPolicy(ctx, &s3.PutBucketPolicyInput{
+			Bucket: &bucket,
+			Policy: &doc,
+		})
+		cancel()
+		if err != nil {
+			return err
+		}
+
+		user1Client := getUserS3Client(usr1, s)
+
+		ctx, cancel = context.WithTimeout(context.Background(), shortTimeout)
+		_, err = user1Client.GetObject(ctx, &s3.GetObjectInput{
+			Bucket: &bucket,
+			Key:    &obj,
+		})
+		cancel()
+		if err != nil {
+			return err
+		}
+
+		ctx, cancel = context.WithTimeout(context.Background(), shortTimeout)
+		_, err = user1Client.GetBucketTagging(ctx, &s3.GetBucketTaggingInput{
+			Bucket: &bucket,
+		})
+		cancel()
+		if err := checkApiErr(err, s3err.GetAPIError(s3err.ErrAccessDenied)); err != nil {
+			return err
+		}
+
+		return nil
+	})
+}
+
+func AccessControl_multi_statement_policy(s *S3Conf) error {
+	testName := "AccessControl_multi_statement_policy"
+	return actionHandler(s, testName, func(s3client *s3.Client, bucket string) error {
+		policy := fmt.Sprintf(`
+		{
+			"Statement": [
+				{
+					"Effect": "Deny",
+					"Principal": ["grt1"],
+					"Action":  "s3:DeleteBucket",
+					"Resource":  "arn:aws:s3:::%s"
+				},
+				{
+					"Effect": "Allow",
+					"Principal": "grt1",
+					"Action": "s3:*",
+					"Resource": ["arn:aws:s3:::%s", "arn:aws:s3:::%s/*"]
+				}
+			]
+		}	
+		`, bucket, bucket, bucket)
+
+		usr := user{
+			access: "grt1",
+			secret: "grt1secret",
+			role:   "user",
+		}
+		err := createUsers(s, []user{usr})
+		if err != nil {
+			return err
+		}
+
+		ctx, cancel := context.WithTimeout(context.Background(), shortTimeout)
+		_, err = s3client.PutBucketPolicy(ctx, &s3.PutBucketPolicyInput{
+			Bucket: &bucket,
+			Policy: &policy,
+		})
+		cancel()
+		if err != nil {
+			return err
+		}
+
+		userClient := getUserS3Client(usr, s)
+
+		ctx, cancel = context.WithTimeout(context.Background(), shortTimeout)
+		_, err = userClient.ListObjects(ctx, &s3.ListObjectsInput{
+			Bucket: &bucket,
+		})
+		cancel()
+		if err != nil {
+			return err
+		}
+
+		ctx, cancel = context.WithTimeout(context.Background(), shortTimeout)
+		_, err = userClient.DeleteBucket(ctx, &s3.DeleteBucketInput{
+			Bucket: &bucket,
+		})
+		cancel()
+		if err := checkApiErr(err, s3err.GetAPIError(s3err.ErrAccessDenied)); err != nil {
+			return err
+		}
+
+		return nil
+	})
+}
+
 // IAM related tests
 // multi-user iam tests
 func IAM_user_access_denied(s *S3Conf) error {
@@ -5808,13 +6122,8 @@ func IAM_user_access_denied(s *S3Conf) error {
 		secret: "grt1secret",
 		role:   "user",
 	}
-	err := deleteUser(s, usr.access)
-	if err != nil {
-		failF("%v: %v", testName, err)
-		return fmt.Errorf("%v: %w", testName, err)
-	}
 
-	err = createUsers(s, []user{usr})
+	err := createUsers(s, []user{usr})
 	if err != nil {
 		failF("%v: %v", testName, err)
 		return fmt.Errorf("%v: %w", testName, err)
@@ -5844,13 +6153,8 @@ func IAM_userplus_access_denied(s *S3Conf) error {
 		secret: "grt1secret",
 		role:   "userplus",
 	}
-	err := deleteUser(s, usr.access)
-	if err != nil {
-		failF("%v: %v", testName, err)
-		return fmt.Errorf("%v: %w", testName, err)
-	}
 
-	err = createUsers(s, []user{usr})
+	err := createUsers(s, []user{usr})
 	if err != nil {
 		failF("%v: %v", testName, err)
 		return fmt.Errorf("%v: %w", testName, err)
@@ -5879,12 +6183,8 @@ func IAM_userplus_CreateBucket(s *S3Conf) error {
 			secret: "grt1secret",
 			role:   "userplus",
 		}
-		err := deleteUser(s, usr.access)
-		if err != nil {
-			return err
-		}
 
-		err = createUsers(s, []user{usr})
+		err := createUsers(s, []user{usr})
 		if err != nil {
 			return err
 		}

tests/integration/utils.go

@@ -537,6 +537,10 @@ type user struct {
 
 func createUsers(s *S3Conf, users []user) error {
 	for _, usr := range users {
+		err := deleteUser(s, usr.access)
+		if err != nil {
+			return err
+		}
 		out, err := execCommand("admin", "-a", s.awsID, "-s", s.awsSecret, "-er", s.endpoint, "create-user", "-a", usr.access, "-s", usr.secret, "-r", usr.role)
 		if err != nil {
 			return err
@@ -633,3 +637,11 @@ func getMalformedPolicyError(msg string) s3err.APIError {
 		HTTPStatusCode: http.StatusBadRequest,
 	}
 }
+
+func getUserS3Client(usr user, cfg *S3Conf) *s3.Client {
+	config := *cfg
+	config.awsID = usr.access
+	config.awsSecret = usr.secret
+
+	return s3.NewFromConfig(config.Config())
+}