From 7fb2a7f9bad11d267afe01a97c951446aacf0540 Mon Sep 17 00:00:00 2001 From: jonaustin09 Date: Wed, 26 Jul 2023 20:54:50 +0400 Subject: [PATCH] feat: ACL refactoring, moved ace parsing from controllers to middleware --- auth/acl.go | 2 +- s3api/controllers/base.go | 137 +++++--------------------------- s3api/controllers/base_test.go | 33 ++++---- s3api/middlewares/acl-parser.go | 61 ++++++++++++++ s3api/server.go | 1 + 5 files changed, 99 insertions(+), 135 deletions(-) create mode 100644 s3api/middlewares/acl-parser.go diff --git a/auth/acl.go b/auth/acl.go index 305248e..b2e2802 100644 --- a/auth/acl.go +++ b/auth/acl.go @@ -261,5 +261,5 @@ func IsAdmin(access string, isRoot bool) error { if acc.Role == "admin" { return nil } - return fmt.Errorf("only admin users have access to this resource") + return s3err.GetAPIError(s3err.ErrAccessDenied) } diff --git a/s3api/controllers/base.go b/s3api/controllers/base.go index f9e6120..566dd4c 100644 --- a/s3api/controllers/base.go +++ b/s3api/controllers/base.go @@ -55,9 +55,6 @@ func New(be backend.Backend, iam auth.IAMService, logger s3log.AuditLogger, evs func (c S3ApiController) ListBuckets(ctx *fiber.Ctx) error { access, isRoot := ctx.Locals("access").(string), ctx.Locals("isRoot").(bool) - if err := auth.IsAdmin(access, isRoot); err != nil { - return SendXMLResponse(ctx, nil, err, &MetaOpts{Logger: c.logger, Action: "ListBucket"}) - } res, err := c.be.ListBuckets(access, isRoot) return SendXMLResponse(ctx, res, err, &MetaOpts{Logger: c.logger, Action: "ListBucket"}) } @@ -72,20 +69,11 @@ func (c S3ApiController) GetActions(ctx *fiber.Ctx) error { acceptRange := ctx.Get("Range") access := ctx.Locals("access").(string) isRoot := ctx.Locals("isRoot").(bool) + parsedAcl := ctx.Locals("parsedAcl").(auth.ACL) if keyEnd != "" { key = strings.Join([]string{key, keyEnd}, "/") } - data, err := c.be.GetBucketAcl(&s3.GetBucketAclInput{Bucket: &bucket}) - if err != nil { - return SendResponse(ctx, err, &MetaOpts{Logger: c.logger}) - } - - parsedAcl, err := auth.ParseACL(data) - if err != nil { - return SendResponse(ctx, err, &MetaOpts{Logger: c.logger}) - } - if ctx.Request().URI().QueryArgs().Has("tagging") { if err := auth.VerifyACL(parsedAcl, bucket, access, "READ", isRoot); err != nil { return SendXMLResponse(ctx, nil, err, &MetaOpts{Logger: c.logger, Action: "GetObjectTagging", BucketOwner: parsedAcl.Owner}) @@ -236,22 +224,18 @@ func (c S3ApiController) ListActions(ctx *fiber.Ctx) error { maxkeys := ctx.QueryInt("max-keys") access := ctx.Locals("access").(string) isRoot := ctx.Locals("isRoot").(bool) - - data, err := c.be.GetBucketAcl(&s3.GetBucketAclInput{Bucket: &bucket}) - if err != nil { - return SendResponse(ctx, err, &MetaOpts{Logger: c.logger}) - } - - parsedAcl, err := auth.ParseACL(data) - if err != nil { - return SendResponse(ctx, err, &MetaOpts{Logger: c.logger}) - } + parsedAcl := ctx.Locals("parsedAcl").(auth.ACL) if ctx.Request().URI().QueryArgs().Has("acl") { if err := auth.VerifyACL(parsedAcl, bucket, access, "READ_ACP", isRoot); err != nil { return SendXMLResponse(ctx, nil, err, &MetaOpts{Logger: c.logger, Action: "GetBucketAcl", BucketOwner: parsedAcl.Owner}) } + data, err := c.be.GetBucketAcl(&s3.GetBucketAclInput{Bucket: &bucket}) + if err != nil { + return SendResponse(ctx, err, &MetaOpts{Logger: c.logger}) + } + res, err := auth.ParseACLOutput(data) return SendXMLResponse(ctx, res, err, &MetaOpts{Logger: c.logger, Action: "GetBucketAcl", BucketOwner: parsedAcl.Owner}) } @@ -308,18 +292,9 @@ func (c S3ApiController) PutBucketActions(ctx *fiber.Ctx) error { grants := grantFullControl + grantRead + grantReadACP + granWrite + grantWriteACP if ctx.Request().URI().QueryArgs().Has("acl") { + parsedAcl := ctx.Locals("parsedAcl").(auth.ACL) var input *s3.PutBucketAclInput - data, err := c.be.GetBucketAcl(&s3.GetBucketAclInput{Bucket: &bucket}) - if err != nil { - return SendResponse(ctx, err, &MetaOpts{Logger: c.logger, Action: "PutBucketAcl"}) - } - - parsedAcl, err := auth.ParseACL(data) - if err != nil { - return SendResponse(ctx, err, &MetaOpts{Logger: c.logger, Action: "PutBucketAcl"}) - } - if err := auth.VerifyACL(parsedAcl, bucket, access, "WRITE_ACP", isRoot); err != nil { return SendResponse(ctx, err, &MetaOpts{Logger: c.logger, Action: "PutBucketAcl", BucketOwner: parsedAcl.Owner}) } @@ -391,6 +366,7 @@ func (c S3ApiController) PutActions(ctx *fiber.Ctx) error { uploadId := ctx.Query("uploadId") access := ctx.Locals("access").(string) isRoot := ctx.Locals("isRoot").(bool) + parsedAcl := ctx.Locals("parsedAcl").(auth.ACL) tagging := ctx.Get("x-amz-tagging") // Copy source headers @@ -423,16 +399,6 @@ func (c S3ApiController) PutActions(ctx *fiber.Ctx) error { keyStart = keyStart + "/" } - data, err := c.be.GetBucketAcl(&s3.GetBucketAclInput{Bucket: &bucket}) - if err != nil { - return SendResponse(ctx, err, &MetaOpts{Logger: c.logger}) - } - - parsedAcl, err := auth.ParseACL(data) - if err != nil { - return SendResponse(ctx, err, &MetaOpts{Logger: c.logger}) - } - if ctx.Request().URI().QueryArgs().Has("tagging") { var objTagging s3response.Tagging err := xml.Unmarshal(ctx.Body(), &objTagging) @@ -557,7 +523,7 @@ func (c S3ApiController) PutActions(ctx *fiber.Ctx) error { } } - err = c.be.PutObjectAcl(input) + err := c.be.PutObjectAcl(input) return SendResponse(ctx, err, &MetaOpts{ Logger: c.logger, EvSender: c.evSender, @@ -573,6 +539,7 @@ func (c S3ApiController) PutActions(ctx *fiber.Ctx) error { } var mtime time.Time + var err error if copySrcModifSince != "" { mtime, err = time.Parse(iso8601Format, copySrcModifSince) if err != nil { @@ -647,42 +614,22 @@ func (c S3ApiController) PutActions(ctx *fiber.Ctx) error { } func (c S3ApiController) DeleteBucket(ctx *fiber.Ctx) error { - bucket, access, isRoot := ctx.Params("bucket"), ctx.Locals("access").(string), ctx.Locals("isRoot").(bool) - - data, err := c.be.GetBucketAcl(&s3.GetBucketAclInput{Bucket: &bucket}) - if err != nil { - return SendResponse(ctx, err, &MetaOpts{Logger: c.logger, Action: "DeleteBuckets"}) - } - - parsedAcl, err := auth.ParseACL(data) - if err != nil { - return SendResponse(ctx, err, &MetaOpts{Logger: c.logger, Action: "DeleteBuckets"}) - } + bucket, access, isRoot, parsedAcl := ctx.Params("bucket"), ctx.Locals("access").(string), ctx.Locals("isRoot").(bool), ctx.Locals("parsedAcl").(auth.ACL) if err := auth.VerifyACL(parsedAcl, bucket, access, "WRITE", isRoot); err != nil { return SendResponse(ctx, err, &MetaOpts{Logger: c.logger, Action: "DeleteBucket", BucketOwner: parsedAcl.Owner}) } - err = c.be.DeleteBucket(&s3.DeleteBucketInput{ + err := c.be.DeleteBucket(&s3.DeleteBucketInput{ Bucket: &bucket, }) return SendResponse(ctx, err, &MetaOpts{Logger: c.logger, Action: "DeleteBucket", BucketOwner: parsedAcl.Owner}) } func (c S3ApiController) DeleteObjects(ctx *fiber.Ctx) error { - bucket, access, isRoot := ctx.Params("bucket"), ctx.Locals("access").(string), ctx.Locals("isRoot").(bool) + bucket, access, isRoot, parsedAcl := ctx.Params("bucket"), ctx.Locals("access").(string), ctx.Locals("isRoot").(bool), ctx.Locals("parsedAcl").(auth.ACL) var dObj types.Delete - data, err := c.be.GetBucketAcl(&s3.GetBucketAclInput{Bucket: &bucket}) - if err != nil { - return SendResponse(ctx, err, &MetaOpts{Logger: c.logger, Action: "DeleteObjects"}) - } - - parsedAcl, err := auth.ParseACL(data) - if err != nil { - return SendResponse(ctx, err, &MetaOpts{Logger: c.logger, Action: "DeleteObjects"}) - } - if err := xml.Unmarshal(ctx.Body(), &dObj); err != nil { return SendResponse(ctx, s3err.GetAPIError(s3err.ErrInvalidRequest), &MetaOpts{Logger: c.logger, Action: "DeleteObjects", BucketOwner: parsedAcl.Owner}) } @@ -691,7 +638,7 @@ func (c S3ApiController) DeleteObjects(ctx *fiber.Ctx) error { return SendResponse(ctx, err, &MetaOpts{Logger: c.logger, Action: "DeleteObjects", BucketOwner: parsedAcl.Owner}) } - err = c.be.DeleteObjects(&s3.DeleteObjectsInput{ + err := c.be.DeleteObjects(&s3.DeleteObjectsInput{ Bucket: &bucket, Delete: &dObj, }) @@ -705,27 +652,18 @@ func (c S3ApiController) DeleteActions(ctx *fiber.Ctx) error { uploadId := ctx.Query("uploadId") access := ctx.Locals("access").(string) isRoot := ctx.Locals("isRoot").(bool) + parsedAcl := ctx.Locals("parsedAcl").(auth.ACL) if keyEnd != "" { key = strings.Join([]string{key, keyEnd}, "/") } - data, err := c.be.GetBucketAcl(&s3.GetBucketAclInput{Bucket: &bucket}) - if err != nil { - return SendResponse(ctx, err, &MetaOpts{Logger: c.logger}) - } - - parsedAcl, err := auth.ParseACL(data) - if err != nil { - return SendResponse(ctx, err, &MetaOpts{Logger: c.logger}) - } - if ctx.Request().URI().QueryArgs().Has("tagging") { if err := auth.VerifyACL(parsedAcl, bucket, access, "WRITE", isRoot); err != nil { return SendResponse(ctx, err, &MetaOpts{Logger: c.logger, Action: "RemoveObjectTagging", BucketOwner: parsedAcl.Owner}) } - err = c.be.RemoveTags(bucket, key) + err := c.be.RemoveTags(bucket, key) return SendResponse(ctx, err, &MetaOpts{ Logger: c.logger, EvSender: c.evSender, @@ -756,7 +694,7 @@ func (c S3ApiController) DeleteActions(ctx *fiber.Ctx) error { return SendResponse(ctx, err, &MetaOpts{Logger: c.logger, Action: "DeleteObject", BucketOwner: parsedAcl.Owner}) } - err = c.be.DeleteObject(&s3.DeleteObjectInput{ + err := c.be.DeleteObject(&s3.DeleteObjectInput{ Bucket: &bucket, Key: &key, }) @@ -770,23 +708,13 @@ func (c S3ApiController) DeleteActions(ctx *fiber.Ctx) error { } func (c S3ApiController) HeadBucket(ctx *fiber.Ctx) error { - bucket, access, isRoot := ctx.Params("bucket"), ctx.Locals("access").(string), ctx.Locals("isRoot").(bool) - - data, err := c.be.GetBucketAcl(&s3.GetBucketAclInput{Bucket: &bucket}) - if err != nil { - return SendResponse(ctx, err, &MetaOpts{Logger: c.logger, Action: "HeadBucket"}) - } - - parsedAcl, err := auth.ParseACL(data) - if err != nil { - return SendResponse(ctx, err, &MetaOpts{Logger: c.logger, Action: "HeadBucket"}) - } + bucket, access, isRoot, parsedAcl := ctx.Params("bucket"), ctx.Locals("access").(string), ctx.Locals("isRoot").(bool), ctx.Locals("parsedAcl").(auth.ACL) if err := auth.VerifyACL(parsedAcl, bucket, access, "READ", isRoot); err != nil { return SendResponse(ctx, err, &MetaOpts{Logger: c.logger, Action: "HeadBucket", BucketOwner: parsedAcl.Owner}) } - _, err = c.be.HeadBucket(&s3.HeadBucketInput{ + _, err := c.be.HeadBucket(&s3.HeadBucketInput{ Bucket: &bucket, }) // TODO: set bucket response headers @@ -798,25 +726,13 @@ const ( ) func (c S3ApiController) HeadObject(ctx *fiber.Ctx) error { - bucket, access, isRoot := ctx.Params("bucket"), ctx.Locals("access").(string), ctx.Locals("isRoot").(bool) + bucket, access, isRoot, parsedAcl := ctx.Params("bucket"), ctx.Locals("access").(string), ctx.Locals("isRoot").(bool), ctx.Locals("parsedAcl").(auth.ACL) key := ctx.Params("key") keyEnd := ctx.Params("*1") if keyEnd != "" { key = strings.Join([]string{key, keyEnd}, "/") } - data, err := c.be.GetBucketAcl(&s3.GetBucketAclInput{ - Bucket: &bucket, - }) - if err != nil { - return SendResponse(ctx, err, &MetaOpts{Logger: c.logger, Action: "HeadObject"}) - } - - parsedAcl, err := auth.ParseACL(data) - if err != nil { - return SendResponse(ctx, err, &MetaOpts{Logger: c.logger, Action: "HeadObject"}) - } - if err := auth.VerifyACL(parsedAcl, bucket, access, "READ", isRoot); err != nil { return SendResponse(ctx, err, &MetaOpts{Logger: c.logger, Action: "HeadObject", BucketOwner: parsedAcl.Owner}) } @@ -878,21 +794,12 @@ func (c S3ApiController) CreateActions(ctx *fiber.Ctx) error { uploadId := ctx.Query("uploadId") access := ctx.Locals("access").(string) isRoot := ctx.Locals("isRoot").(bool) + parsedAcl := ctx.Locals("parsedAcl").(auth.ACL) if keyEnd != "" { key = strings.Join([]string{key, keyEnd}, "/") } - data, err := c.be.GetBucketAcl(&s3.GetBucketAclInput{Bucket: &bucket}) - if err != nil { - return SendResponse(ctx, err, &MetaOpts{Logger: c.logger}) - } - - parsedAcl, err := auth.ParseACL(data) - if err != nil { - return SendResponse(ctx, err, &MetaOpts{Logger: c.logger}) - } - var restoreRequest s3.RestoreObjectInput if ctx.Request().URI().QueryArgs().Has("restore") { err := xml.Unmarshal(ctx.Body(), &restoreRequest) diff --git a/s3api/controllers/base_test.go b/s3api/controllers/base_test.go index af331ce..d65333e 100644 --- a/s3api/controllers/base_test.go +++ b/s3api/controllers/base_test.go @@ -127,16 +127,6 @@ func TestS3ApiController_ListBuckets(t *testing.T) { }) appErr.Get("/", s3ApiControllerErr.ListBuckets) - //Admin error case - admErr := fiber.New() - admErr.Use(func(ctx *fiber.Ctx) error { - ctx.Locals("access", "valid access") - ctx.Locals("isRoot", false) - ctx.Locals("isDebug", false) - return ctx.Next() - }) - admErr.Get("/", s3ApiController.ListBuckets) - tests := []struct { name string args args @@ -162,15 +152,6 @@ func TestS3ApiController_ListBuckets(t *testing.T) { wantErr: false, statusCode: 200, }, - { - name: "admin-error-case", - args: args{ - req: httptest.NewRequest(http.MethodGet, "/", nil), - }, - app: admErr, - wantErr: false, - statusCode: 500, - }, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { @@ -233,6 +214,7 @@ func TestS3ApiController_GetActions(t *testing.T) { ctx.Locals("access", "valid access") ctx.Locals("isRoot", true) ctx.Locals("isDebug", false) + ctx.Locals("parsedAcl", auth.ACL{}) return ctx.Next() }) app.Get("/:bucket/:key/*", s3ApiController.GetActions) @@ -372,6 +354,7 @@ func TestS3ApiController_ListActions(t *testing.T) { ctx.Locals("access", "valid access") ctx.Locals("isRoot", true) ctx.Locals("isDebug", false) + ctx.Locals("parsedAcl", auth.ACL{}) return ctx.Next() }) @@ -393,6 +376,7 @@ func TestS3ApiController_ListActions(t *testing.T) { ctx.Locals("access", "valid access") ctx.Locals("isRoot", true) ctx.Locals("isDebug", false) + ctx.Locals("parsedAcl", auth.ACL{}) return ctx.Next() }) appError.Get("/:bucket", s3ApiControllerError.ListActions) @@ -514,6 +498,7 @@ func TestS3ApiController_PutBucketActions(t *testing.T) { ctx.Locals("access", "valid access") ctx.Locals("isRoot", true) ctx.Locals("isDebug", false) + ctx.Locals("parsedAcl", auth.ACL{Owner: "valid access"}) return ctx.Next() }) app.Put("/:bucket", s3ApiController.PutBucketActions) @@ -679,6 +664,7 @@ func TestS3ApiController_PutActions(t *testing.T) { ctx.Locals("access", "valid access") ctx.Locals("isRoot", true) ctx.Locals("isDebug", false) + ctx.Locals("parsedAcl", auth.ACL{}) return ctx.Next() }) app.Put("/:bucket/:key/*", s3ApiController.PutActions) @@ -877,6 +863,7 @@ func TestS3ApiController_DeleteBucket(t *testing.T) { ctx.Locals("access", "valid access") ctx.Locals("isRoot", true) ctx.Locals("isDebug", false) + ctx.Locals("parsedAcl", auth.ACL{}) return ctx.Next() }) @@ -933,6 +920,7 @@ func TestS3ApiController_DeleteObjects(t *testing.T) { ctx.Locals("access", "valid access") ctx.Locals("isRoot", true) ctx.Locals("isDebug", false) + ctx.Locals("parsedAcl", auth.ACL{}) return ctx.Next() }) app.Post("/:bucket", s3ApiController.DeleteObjects) @@ -1009,6 +997,7 @@ func TestS3ApiController_DeleteActions(t *testing.T) { ctx.Locals("access", "valid access") ctx.Locals("isRoot", true) ctx.Locals("isDebug", false) + ctx.Locals("parsedAcl", auth.ACL{}) return ctx.Next() }) app.Delete("/:bucket/:key/*", s3ApiController.DeleteActions) @@ -1029,6 +1018,7 @@ func TestS3ApiController_DeleteActions(t *testing.T) { ctx.Locals("access", "valid access") ctx.Locals("isRoot", true) ctx.Locals("isDebug", false) + ctx.Locals("parsedAcl", auth.ACL{}) return ctx.Next() }) appErr.Delete("/:bucket/:key/*", s3ApiControllerErr.DeleteActions) @@ -1111,6 +1101,7 @@ func TestS3ApiController_HeadBucket(t *testing.T) { ctx.Locals("access", "valid access") ctx.Locals("isRoot", true) ctx.Locals("isDebug", false) + ctx.Locals("parsedAcl", auth.ACL{}) return ctx.Next() }) @@ -1133,6 +1124,7 @@ func TestS3ApiController_HeadBucket(t *testing.T) { ctx.Locals("access", "valid access") ctx.Locals("isRoot", true) ctx.Locals("isDebug", false) + ctx.Locals("parsedAcl", auth.ACL{}) return ctx.Next() }) @@ -1211,6 +1203,7 @@ func TestS3ApiController_HeadObject(t *testing.T) { ctx.Locals("access", "valid access") ctx.Locals("isRoot", true) ctx.Locals("isDebug", false) + ctx.Locals("parsedAcl", auth.ACL{}) return ctx.Next() }) app.Head("/:bucket/:key/*", s3ApiController.HeadObject) @@ -1233,6 +1226,7 @@ func TestS3ApiController_HeadObject(t *testing.T) { ctx.Locals("access", "valid access") ctx.Locals("isRoot", true) ctx.Locals("isDebug", false) + ctx.Locals("parsedAcl", auth.ACL{}) return ctx.Next() }) appErr.Head("/:bucket/:key/*", s3ApiControllerErr.HeadObject) @@ -1302,6 +1296,7 @@ func TestS3ApiController_CreateActions(t *testing.T) { ctx.Locals("access", "valid access") ctx.Locals("isRoot", true) ctx.Locals("isDebug", false) + ctx.Locals("parsedAcl", auth.ACL{}) return ctx.Next() }) app.Post("/:bucket/:key/*", s3ApiController.CreateActions) diff --git a/s3api/middlewares/acl-parser.go b/s3api/middlewares/acl-parser.go new file mode 100644 index 0000000..6b7db31 --- /dev/null +++ b/s3api/middlewares/acl-parser.go @@ -0,0 +1,61 @@ +// Copyright 2023 Versity Software +// This file is licensed under the Apache License, Version 2.0 +// (the "License"); you may not use this file except in compliance +// with the License. You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, +// software distributed under the License is distributed on an +// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +// KIND, either express or implied. See the License for the +// specific language governing permissions and limitations +// under the License. + +package middlewares + +import ( + "net/http" + "strings" + + "github.com/aws/aws-sdk-go-v2/service/s3" + "github.com/gofiber/fiber/v2" + "github.com/versity/versitygw/auth" + "github.com/versity/versitygw/backend" + "github.com/versity/versitygw/s3api/controllers" + "github.com/versity/versitygw/s3log" +) + +func AclParser(be backend.Backend, logger s3log.AuditLogger) fiber.Handler { + return func(ctx *fiber.Ctx) error { + isRoot, access := ctx.Locals("isRoot").(bool), ctx.Locals("access").(string) + path := ctx.Path() + pathParts := strings.Split(path, "/") + bucket := pathParts[1] + if path == "/" && ctx.Method() == http.MethodGet { + return ctx.Next() + } + if ctx.Method() == http.MethodPatch { + return ctx.Next() + } + if len(pathParts) == 2 && pathParts[1] != "" && ctx.Method() == http.MethodPut && !ctx.Request().URI().QueryArgs().Has("acl") { + if err := auth.IsAdmin(access, isRoot); err != nil { + return controllers.SendXMLResponse(ctx, nil, err, &controllers.MetaOpts{Logger: logger, Action: "ListBuckets"}) + } + return ctx.Next() + } + //TODO: provide correct action names for the logger, after implementing DetectAction middleware + data, err := be.GetBucketAcl(&s3.GetBucketAclInput{Bucket: &bucket}) + if err != nil { + return controllers.SendResponse(ctx, err, &controllers.MetaOpts{Logger: logger}) + } + + parsedAcl, err := auth.ParseACL(data) + if err != nil { + return controllers.SendResponse(ctx, err, &controllers.MetaOpts{Logger: logger}) + } + + ctx.Locals("parsedAcl", parsedAcl) + return ctx.Next() + } +} diff --git a/s3api/server.go b/s3api/server.go index e92e7cf..a5d803e 100644 --- a/s3api/server.go +++ b/s3api/server.go @@ -54,6 +54,7 @@ func New(app *fiber.App, be backend.Backend, root middlewares.RootUserConfig, po // Authentication middlewares app.Use(middlewares.VerifyV4Signature(root, iam, l, region, server.debug)) app.Use(middlewares.VerifyMD5Body(l)) + app.Use(middlewares.AclParser(be, l)) server.router.Init(app, be, iam, l, evs)