From c196b5f999e6b771f3ef682de285a5b4582e5969 Mon Sep 17 00:00:00 2001 From: Ben McClelland Date: Wed, 9 Jul 2025 09:13:14 -0700 Subject: [PATCH 1/2] fix: admin bucket actions for s3proxy We were incorrctly trying to pass through the admin request actions through to the backend s3 service in s3proxy. This was resulting in internal server errors since not all s3 backends would understand these requests. Instead the gateway needs to handle these requests directly. Fixes #1381 --- backend/s3proxy/s3.go | 104 ++++++++++++------------------------------ 1 file changed, 28 insertions(+), 76 deletions(-) diff --git a/backend/s3proxy/s3.go b/backend/s3proxy/s3.go index 8fc953e0..2620ddf0 100644 --- a/backend/s3proxy/s3.go +++ b/backend/s3proxy/s3.go @@ -17,17 +17,12 @@ package s3proxy import ( "bytes" "context" - "crypto/sha256" - "encoding/hex" - "encoding/json" "errors" "fmt" "io" - "net/http" "strconv" "time" - "github.com/aws/aws-sdk-go-v2/aws" v4 "github.com/aws/aws-sdk-go-v2/aws/signer/v4" awshttp "github.com/aws/aws-sdk-go-v2/aws/transport/http" "github.com/aws/aws-sdk-go-v2/service/s3" @@ -1553,81 +1548,38 @@ func (s *S3Proxy) GetObjectLegalHold(ctx context.Context, bucket, object, versio } func (s *S3Proxy) ChangeBucketOwner(ctx context.Context, bucket string, acl []byte) error { - acll, err := auth.ParseACL(acl) - if err != nil { - return err - } - - req, err := http.NewRequest(http.MethodPatch, fmt.Sprintf("%v/change-bucket-owner/?bucket=%v&owner=%v", s.endpoint, bucket, acll.Owner), nil) - if err != nil { - return fmt.Errorf("failed to send the request: %w", err) - } - - signer := v4.NewSigner() - - hashedPayload := sha256.Sum256([]byte{}) - hexPayload := hex.EncodeToString(hashedPayload[:]) - - req.Header.Set("X-Amz-Content-Sha256", hexPayload) - - signErr := signer.SignHTTP(req.Context(), aws.Credentials{AccessKeyID: s.access, SecretAccessKey: s.secret}, req, hexPayload, "s3", s.awsRegion, time.Now()) - if signErr != nil { - return fmt.Errorf("failed to sign the request: %w", err) - } - - client := http.Client{} - - resp, err := client.Do(req) - if err != nil { - return fmt.Errorf("failed to send the request: %w", err) - } - - if resp.StatusCode > 300 { - body, err := io.ReadAll(resp.Body) - if err != nil { - return err - } - defer resp.Body.Close() - return fmt.Errorf("%v", string(body)) - } - - return nil + return s.PutBucketAcl(ctx, bucket, acl) } func (s *S3Proxy) ListBucketsAndOwners(ctx context.Context) ([]s3response.Bucket, error) { - req, err := http.NewRequest(http.MethodPatch, fmt.Sprintf("%v/list-buckets", s.endpoint), nil) - if err != nil { - return []s3response.Bucket{}, fmt.Errorf("failed to send the request: %w", err) - } - - signer := v4.NewSigner() - - hashedPayload := sha256.Sum256([]byte{}) - hexPayload := hex.EncodeToString(hashedPayload[:]) - - req.Header.Set("X-Amz-Content-Sha256", hexPayload) - - signErr := signer.SignHTTP(req.Context(), aws.Credentials{AccessKeyID: s.access, SecretAccessKey: s.secret}, req, hexPayload, "s3", s.awsRegion, time.Now()) - if signErr != nil { - return []s3response.Bucket{}, fmt.Errorf("failed to sign the request: %w", err) - } - - client := http.Client{} - - resp, err := client.Do(req) - if err != nil { - return []s3response.Bucket{}, fmt.Errorf("failed to send the request: %w", err) - } - - body, err := io.ReadAll(resp.Body) - if err != nil { - return []s3response.Bucket{}, err - } - defer resp.Body.Close() - var buckets []s3response.Bucket - if err := json.Unmarshal(body, &buckets); err != nil { - return []s3response.Bucket{}, err + + paginator := s3.NewListBucketsPaginator(s.client, &s3.ListBucketsInput{}) + + for paginator.HasMorePages() { + page, err := paginator.NextPage(ctx) + if err != nil { + return nil, handleError(err) + } + for _, bucket := range page.Buckets { + if *bucket.Name == s.metaBucket { + continue + } + aclJSON, err := s.getMetaBucketObjData(ctx, *bucket.Name, metaPrefixAcl, false) + if err != nil { + return nil, handleError(err) + } + + acl, err := auth.ParseACL(aclJSON) + if err != nil { + return buckets, fmt.Errorf("parse acl tag: %w", err) + } + + buckets = append(buckets, s3response.Bucket{ + Name: *bucket.Name, + Owner: acl.Owner, + }) + } } return buckets, nil From f295df2217f0bc4b099c764933274832b27455ae Mon Sep 17 00:00:00 2001 From: Ben McClelland Date: Wed, 9 Jul 2025 12:19:09 -0700 Subject: [PATCH 2/2] fix: add new auth method to update ownership within acl Add helper util auth.UpdateBucketACLOwner() that sets new default ACL based on new owner and removes old bucket policy. The ChangeBucketOwner() remains in the backend.Backend interface in case there is ever a backend that needs to manage ownership in some other way than with bucket ACLs. The arguments are changing to clarify the updated owner. This will break any plugins implementing the old interface. They should use the new auth.UpdateBucketACLOwner() or implement the corresponding change specific for the backend. --- auth/acl.go | 27 +++++++++++++++++++++++++++ backend/azure/azure.go | 4 ++-- backend/backend.go | 4 ++-- backend/posix/posix.go | 4 ++-- backend/s3proxy/s3.go | 4 ++-- s3api/controllers/admin.go | 25 +------------------------ s3api/controllers/admin_test.go | 2 +- s3api/controllers/backend_moq_test.go | 20 ++++++++++---------- 8 files changed, 47 insertions(+), 43 deletions(-) diff --git a/auth/acl.go b/auth/acl.go index 3b32cdab..e9003243 100644 --- a/auth/acl.go +++ b/auth/acl.go @@ -466,3 +466,30 @@ func VerifyPublicBucketACL(ctx context.Context, be backend.Backend, bucket strin return nil } + +// UpdateBucketACLOwner sets default ACL with new owner and removes +// any previous bucket policy that was in place +func UpdateBucketACLOwner(ctx context.Context, be backend.Backend, bucket, newOwner string) error { + acl := ACL{ + Owner: newOwner, + Grantees: []Grantee{ + { + Permission: PermissionFullControl, + Access: newOwner, + Type: types.TypeCanonicalUser, + }, + }, + } + + result, err := json.Marshal(acl) + if err != nil { + return fmt.Errorf("marshal ACL: %w", err) + } + + err = be.PutBucketAcl(ctx, bucket, result) + if err != nil { + return err + } + + return be.DeleteBucketPolicy(ctx, bucket) +} diff --git a/backend/azure/azure.go b/backend/azure/azure.go index 5cfa6388..9934a5b6 100644 --- a/backend/azure/azure.go +++ b/backend/azure/azure.go @@ -1683,8 +1683,8 @@ func (az *Azure) GetObjectLegalHold(ctx context.Context, bucket, object, version return &status, nil } -func (az *Azure) ChangeBucketOwner(ctx context.Context, bucket string, acl []byte) error { - return az.PutBucketAcl(ctx, bucket, acl) +func (az *Azure) ChangeBucketOwner(ctx context.Context, bucket, owner string) error { + return auth.UpdateBucketACLOwner(ctx, az, bucket, owner) } // The action actually returns the containers owned by the user, who initialized the gateway diff --git a/backend/backend.go b/backend/backend.go index de9ea2b7..946ee735 100644 --- a/backend/backend.go +++ b/backend/backend.go @@ -96,7 +96,7 @@ type Backend interface { GetObjectLegalHold(_ context.Context, bucket, object, versionId string) (*bool, error) // non AWS actions - ChangeBucketOwner(_ context.Context, bucket string, acl []byte) error + ChangeBucketOwner(_ context.Context, bucket, owner string) error ListBucketsAndOwners(context.Context) ([]s3response.Bucket, error) } @@ -280,7 +280,7 @@ func (BackendUnsupported) GetObjectLegalHold(_ context.Context, bucket, object, return nil, s3err.GetAPIError(s3err.ErrNotImplemented) } -func (BackendUnsupported) ChangeBucketOwner(_ context.Context, bucket string, acl []byte) error { +func (BackendUnsupported) ChangeBucketOwner(_ context.Context, bucket, owner string) error { return s3err.GetAPIError(s3err.ErrNotImplemented) } func (BackendUnsupported) ListBucketsAndOwners(context.Context) ([]s3response.Bucket, error) { diff --git a/backend/posix/posix.go b/backend/posix/posix.go index bb99ebea..3c969899 100644 --- a/backend/posix/posix.go +++ b/backend/posix/posix.go @@ -4888,8 +4888,8 @@ func (p *Posix) GetObjectRetention(_ context.Context, bucket, object, versionId return data, nil } -func (p *Posix) ChangeBucketOwner(ctx context.Context, bucket string, acl []byte) error { - return p.PutBucketAcl(ctx, bucket, acl) +func (p *Posix) ChangeBucketOwner(ctx context.Context, bucket, owner string) error { + return auth.UpdateBucketACLOwner(ctx, p, bucket, owner) } func listBucketFileInfos(bucketlinks bool) ([]fs.FileInfo, error) { diff --git a/backend/s3proxy/s3.go b/backend/s3proxy/s3.go index 2620ddf0..d29db3df 100644 --- a/backend/s3proxy/s3.go +++ b/backend/s3proxy/s3.go @@ -1547,8 +1547,8 @@ func (s *S3Proxy) GetObjectLegalHold(ctx context.Context, bucket, object, versio return nil, s3err.GetAPIError(s3err.ErrNotImplemented) } -func (s *S3Proxy) ChangeBucketOwner(ctx context.Context, bucket string, acl []byte) error { - return s.PutBucketAcl(ctx, bucket, acl) +func (s *S3Proxy) ChangeBucketOwner(ctx context.Context, bucket, owner string) error { + return auth.UpdateBucketACLOwner(ctx, s, bucket, owner) } func (s *S3Proxy) ListBucketsAndOwners(ctx context.Context) ([]s3response.Bucket, error) { diff --git a/s3api/controllers/admin.go b/s3api/controllers/admin.go index 9aada79f..d10dae18 100644 --- a/s3api/controllers/admin.go +++ b/s3api/controllers/admin.go @@ -15,13 +15,10 @@ package controllers import ( - "encoding/json" "encoding/xml" - "fmt" "net/http" "strings" - "github.com/aws/aws-sdk-go-v2/service/s3/types" "github.com/gofiber/fiber/v2" "github.com/versity/versitygw/auth" "github.com/versity/versitygw/backend" @@ -172,27 +169,7 @@ func (c AdminController) ChangeBucketOwner(ctx *fiber.Ctx) error { }) } - acl := auth.ACL{ - Owner: owner, - Grantees: []auth.Grantee{ - { - Permission: auth.PermissionFullControl, - Access: owner, - Type: types.TypeCanonicalUser, - }, - }, - } - - aclParsed, err := json.Marshal(acl) - if err != nil { - return SendResponse(ctx, fmt.Errorf("failed to marshal the bucket acl: %w", err), - &MetaOpts{ - Logger: c.l, - Action: metrics.ActionAdminChangeBucketOwner, - }) - } - - err = c.be.ChangeBucketOwner(ctx.Context(), bucket, aclParsed) + err = c.be.ChangeBucketOwner(ctx.Context(), bucket, owner) return SendResponse(ctx, err, &MetaOpts{ Logger: c.l, diff --git a/s3api/controllers/admin_test.go b/s3api/controllers/admin_test.go index 3382461a..aaa82a04 100644 --- a/s3api/controllers/admin_test.go +++ b/s3api/controllers/admin_test.go @@ -324,7 +324,7 @@ func TestAdminController_ChangeBucketOwner(t *testing.T) { } adminController := AdminController{ be: &BackendMock{ - ChangeBucketOwnerFunc: func(contextMoqParam context.Context, bucket string, acl []byte) error { + ChangeBucketOwnerFunc: func(contextMoqParam context.Context, bucket, owner string) error { return nil }, }, diff --git a/s3api/controllers/backend_moq_test.go b/s3api/controllers/backend_moq_test.go index 5d2e9a28..d0ef8d70 100644 --- a/s3api/controllers/backend_moq_test.go +++ b/s3api/controllers/backend_moq_test.go @@ -26,7 +26,7 @@ var _ backend.Backend = &BackendMock{} // AbortMultipartUploadFunc: func(contextMoqParam context.Context, abortMultipartUploadInput *s3.AbortMultipartUploadInput) error { // panic("mock out the AbortMultipartUpload method") // }, -// ChangeBucketOwnerFunc: func(contextMoqParam context.Context, bucket string, acl []byte) error { +// ChangeBucketOwnerFunc: func(contextMoqParam context.Context, bucket string, owner string) error { // panic("mock out the ChangeBucketOwner method") // }, // CompleteMultipartUploadFunc: func(contextMoqParam context.Context, completeMultipartUploadInput *s3.CompleteMultipartUploadInput) (s3response.CompleteMultipartUploadResult, string, error) { @@ -196,7 +196,7 @@ type BackendMock struct { AbortMultipartUploadFunc func(contextMoqParam context.Context, abortMultipartUploadInput *s3.AbortMultipartUploadInput) error // ChangeBucketOwnerFunc mocks the ChangeBucketOwner method. - ChangeBucketOwnerFunc func(contextMoqParam context.Context, bucket string, acl []byte) error + ChangeBucketOwnerFunc func(contextMoqParam context.Context, bucket string, owner string) error // CompleteMultipartUploadFunc mocks the CompleteMultipartUpload method. CompleteMultipartUploadFunc func(contextMoqParam context.Context, completeMultipartUploadInput *s3.CompleteMultipartUploadInput) (s3response.CompleteMultipartUploadResult, string, error) @@ -369,8 +369,8 @@ type BackendMock struct { ContextMoqParam context.Context // Bucket is the bucket argument value. Bucket string - // ACL is the acl argument value. - ACL []byte + // Owner is the owner argument value. + Owner string } // CompleteMultipartUpload holds details about calls to the CompleteMultipartUpload method. CompleteMultipartUpload []struct { @@ -864,23 +864,23 @@ func (mock *BackendMock) AbortMultipartUploadCalls() []struct { } // ChangeBucketOwner calls ChangeBucketOwnerFunc. -func (mock *BackendMock) ChangeBucketOwner(contextMoqParam context.Context, bucket string, acl []byte) error { +func (mock *BackendMock) ChangeBucketOwner(contextMoqParam context.Context, bucket string, owner string) error { if mock.ChangeBucketOwnerFunc == nil { panic("BackendMock.ChangeBucketOwnerFunc: method is nil but Backend.ChangeBucketOwner was just called") } callInfo := struct { ContextMoqParam context.Context Bucket string - ACL []byte + Owner string }{ ContextMoqParam: contextMoqParam, Bucket: bucket, - ACL: acl, + Owner: owner, } mock.lockChangeBucketOwner.Lock() mock.calls.ChangeBucketOwner = append(mock.calls.ChangeBucketOwner, callInfo) mock.lockChangeBucketOwner.Unlock() - return mock.ChangeBucketOwnerFunc(contextMoqParam, bucket, acl) + return mock.ChangeBucketOwnerFunc(contextMoqParam, bucket, owner) } // ChangeBucketOwnerCalls gets all the calls that were made to ChangeBucketOwner. @@ -890,12 +890,12 @@ func (mock *BackendMock) ChangeBucketOwner(contextMoqParam context.Context, buck func (mock *BackendMock) ChangeBucketOwnerCalls() []struct { ContextMoqParam context.Context Bucket string - ACL []byte + Owner string } { var calls []struct { ContextMoqParam context.Context Bucket string - ACL []byte + Owner string } mock.lockChangeBucketOwner.RLock() calls = mock.calls.ChangeBucketOwner