diff --git a/auth/acl.go b/auth/acl.go index 6e6e501..1053fca 100644 --- a/auth/acl.go +++ b/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 } diff --git a/auth/bucket_policy.go b/auth/bucket_policy.go index ed9545a..116f62a 100644 --- a/auth/bucket_policy.go +++ b/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) } diff --git a/backend/posix/posix.go b/backend/posix/posix.go index 4156eca..5e3c97e 100644 --- a/backend/posix/posix.go +++ b/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 } diff --git a/cmd/versitygw/test.go b/cmd/versitygw/test.go index 1ac2c37..021ffcd 100644 --- a/cmd/versitygw/test.go +++ b/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", diff --git a/s3api/middlewares/acl-parser.go b/s3api/middlewares/acl-parser.go index ccd5466..5444655 100644 --- a/s3api/middlewares/acl-parser.go +++ b/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}) diff --git a/tests/integration/group-tests.go b/tests/integration/group-tests.go index c5f58d5..a5cb6dd 100644 --- a/tests/integration/group-tests.go +++ b/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 { diff --git a/tests/integration/tests.go b/tests/integration/tests.go index 04b6c65..cec1a5c 100644 --- a/tests/integration/tests.go +++ b/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 } diff --git a/tests/integration/utils.go b/tests/integration/utils.go index 626907c..8d1c780 100644 --- a/tests/integration/utils.go +++ b/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()) +}