From f09bc1416550267aab1de5cf611451bb1d14afa1 Mon Sep 17 00:00:00 2001 From: Chris Lu Date: Wed, 5 Aug 2026 13:15:28 -0700 Subject: [PATCH] s3: report the effective ownership when a bucket has none stored (#10591) * s3: report the effective ownership when a bucket has none stored GetBucketOwnershipControls read Seaweed-X-Amz-Ownership straight out of the bucket entry, so a bucket that never had one written reported an empty ObjectOwnership. The object write path defaults the same missing attribute to BucketOwnerEnforced, so the API contradicted the behavior it describes. Resolve the stored value through one helper both readers share, and let PutBucketOwnershipControls persist unconditionally so setting the default value still gives DeleteBucketOwnershipControls something to remove. * test: cover the bucket ownership controls round trip Pins the behaviors the ownership default fix depends on: a bucket that never had ownership controls written reports BucketOwnerEnforced, and putting that same value on such a bucket still persists it, so the delete that follows has something to remove. The put-then-delete case gets its own bucket -- run after an ObjectWriter put, it would pass against an implementation that skips only the initial write. The acl workflow already runs this package against a live weed mini, so it needs no wiring. --- test/s3/acl/s3_ownership_controls_test.go | 93 +++++++++++++++++++ weed/s3api/bucket_metadata.go | 14 +-- weed/s3api/s3_constants/acp_ownership.go | 9 ++ weed/s3api/s3_constants/acp_ownership_test.go | 24 +++++ weed/s3api/s3api_bucket_config.go | 2 +- weed/s3api/s3api_bucket_handlers.go | 13 +-- weed/s3api/s3api_bucket_handlers_misc_test.go | 22 +++++ 7 files changed, 156 insertions(+), 21 deletions(-) create mode 100644 test/s3/acl/s3_ownership_controls_test.go create mode 100644 weed/s3api/s3_constants/acp_ownership_test.go diff --git a/test/s3/acl/s3_ownership_controls_test.go b/test/s3/acl/s3_ownership_controls_test.go new file mode 100644 index 000000000..257908b88 --- /dev/null +++ b/test/s3/acl/s3_ownership_controls_test.go @@ -0,0 +1,93 @@ +package acl + +import ( + "context" + "strings" + "testing" + "time" + + "github.com/aws/aws-sdk-go-v2/aws" + "github.com/aws/aws-sdk-go-v2/service/s3" + "github.com/aws/aws-sdk-go-v2/service/s3/types" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +func createOwnershipTestBucket(t *testing.T, client *s3.Client) string { + bucketName := "test-ownership-" + strings.ToLower(strings.ReplaceAll(time.Now().Format("2006-01-02-15-04-05.000"), ":", "-")) + _, err := client.CreateBucket(context.TODO(), &s3.CreateBucketInput{ + Bucket: aws.String(bucketName), + }) + require.NoError(t, err) + return bucketName +} + +func getObjectOwnership(t *testing.T, client *s3.Client, bucketName string) types.ObjectOwnership { + t.Helper() + resp, err := client.GetBucketOwnershipControls(context.TODO(), &s3.GetBucketOwnershipControlsInput{ + Bucket: aws.String(bucketName), + }) + require.NoError(t, err) + require.NotNil(t, resp.OwnershipControls) + require.Len(t, resp.OwnershipControls.Rules, 1) + return resp.OwnershipControls.Rules[0].ObjectOwnership +} + +func putObjectOwnership(t *testing.T, client *s3.Client, bucketName string, ownership types.ObjectOwnership) { + t.Helper() + _, err := client.PutBucketOwnershipControls(context.TODO(), &s3.PutBucketOwnershipControlsInput{ + Bucket: aws.String(bucketName), + OwnershipControls: &types.OwnershipControls{ + Rules: []types.OwnershipControlsRule{{ObjectOwnership: ownership}}, + }, + }) + require.NoError(t, err) +} + +func TestGetBucketOwnershipControlsDefault(t *testing.T) { + client := getS3Client(t) + bucketName := createOwnershipTestBucket(t, client) + defer cleanupTestBucket(t, client, bucketName) + + assert.Equal(t, types.ObjectOwnershipBucketOwnerEnforced, getObjectOwnership(t, client, bucketName)) +} + +func TestBucketOwnershipControlsLifecycle(t *testing.T) { + client := getS3Client(t) + bucketName := createOwnershipTestBucket(t, client) + defer cleanupTestBucket(t, client, bucketName) + + putObjectOwnership(t, client, bucketName, types.ObjectOwnershipObjectWriter) + assert.Equal(t, types.ObjectOwnershipObjectWriter, getObjectOwnership(t, client, bucketName)) + + putObjectOwnership(t, client, bucketName, types.ObjectOwnershipBucketOwnerEnforced) + assert.Equal(t, types.ObjectOwnershipBucketOwnerEnforced, getObjectOwnership(t, client, bucketName)) + + _, err := client.DeleteBucketOwnershipControls(context.TODO(), &s3.DeleteBucketOwnershipControlsInput{ + Bucket: aws.String(bucketName), + }) + require.NoError(t, err) + + assert.Equal(t, types.ObjectOwnershipBucketOwnerEnforced, getObjectOwnership(t, client, bucketName)) +} + +// A bucket with nothing stored already reports BucketOwnerEnforced, so this put has +// to persist anyway or the delete below finds nothing to remove. +func TestPutBucketOwnershipControlsDefaultOnNewBucket(t *testing.T) { + client := getS3Client(t) + bucketName := createOwnershipTestBucket(t, client) + defer cleanupTestBucket(t, client, bucketName) + + putObjectOwnership(t, client, bucketName, types.ObjectOwnershipBucketOwnerEnforced) + + _, err := client.DeleteBucketOwnershipControls(context.TODO(), &s3.DeleteBucketOwnershipControlsInput{ + Bucket: aws.String(bucketName), + }) + require.NoError(t, err) + + _, err = client.DeleteBucketOwnershipControls(context.TODO(), &s3.DeleteBucketOwnershipControlsInput{ + Bucket: aws.String(bucketName), + }) + require.Error(t, err) + assert.Contains(t, err.Error(), "OwnershipControlsNotFound") +} diff --git a/weed/s3api/bucket_metadata.go b/weed/s3api/bucket_metadata.go index b86d7756b..eb04cd012 100644 --- a/weed/s3api/bucket_metadata.go +++ b/weed/s3api/bucket_metadata.go @@ -101,7 +101,7 @@ func buildBucketMetadata(accountManager AccountManager, entry *filer_pb.Entry) * IsTableBucket: s3tables.IsTableBucketEntry(entry), //Default ownership: OwnershipBucketOwnerEnforced, which means Acl is disabled - ObjectOwnership: s3_constants.OwnershipBucketOwnerEnforced, + ObjectOwnership: s3_constants.DefaultOwnershipForExists, // Default owner: `AccountAdmin` Owner: &s3.Owner{ @@ -111,15 +111,11 @@ func buildBucketMetadata(accountManager AccountManager, entry *filer_pb.Entry) * } if entry.Extended != nil { //ownership control - ownership, ok := entry.Extended[s3_constants.ExtOwnershipKey] - if ok { - ownership := string(ownership) - valid := s3_constants.ValidateOwnership(ownership) - if valid { - bucketMetadata.ObjectOwnership = ownership - } else { - glog.Warningf("Invalid ownership: %s, bucket: %s", ownership, bucketMetadata.Name) + if ownership, ok := entry.Extended[s3_constants.ExtOwnershipKey]; ok { + if !s3_constants.ValidateOwnership(string(ownership)) { + glog.Warningf("Invalid ownership: %s, bucket: %s", string(ownership), bucketMetadata.Name) } + bucketMetadata.ObjectOwnership = s3_constants.EffectiveOwnership(string(ownership)) } //access control policy diff --git a/weed/s3api/s3_constants/acp_ownership.go b/weed/s3api/s3_constants/acp_ownership.go index e11e95935..18663c35c 100644 --- a/weed/s3api/s3_constants/acp_ownership.go +++ b/weed/s3api/s3_constants/acp_ownership.go @@ -16,3 +16,12 @@ func ValidateOwnership(ownership string) bool { return true } } + +// EffectiveOwnership resolves a stored ownership setting to the one that governs +// the bucket: absent or invalid behaves as BucketOwnerEnforced. +func EffectiveOwnership(ownership string) string { + if !ValidateOwnership(ownership) { + return DefaultOwnershipForExists + } + return ownership +} diff --git a/weed/s3api/s3_constants/acp_ownership_test.go b/weed/s3api/s3_constants/acp_ownership_test.go new file mode 100644 index 000000000..dd91c950a --- /dev/null +++ b/weed/s3api/s3_constants/acp_ownership_test.go @@ -0,0 +1,24 @@ +package s3_constants + +import "testing" + +func TestEffectiveOwnership(t *testing.T) { + cases := []struct { + name string + input string + want string + }{ + {name: "unset", input: "", want: OwnershipBucketOwnerEnforced}, + {name: "invalid", input: "Bogus", want: OwnershipBucketOwnerEnforced}, + {name: "object writer", input: OwnershipObjectWriter, want: OwnershipObjectWriter}, + {name: "bucket owner preferred", input: OwnershipBucketOwnerPreferred, want: OwnershipBucketOwnerPreferred}, + {name: "bucket owner enforced", input: OwnershipBucketOwnerEnforced, want: OwnershipBucketOwnerEnforced}, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + if got := EffectiveOwnership(tc.input); got != tc.want { + t.Fatalf("EffectiveOwnership(%q) = %q, want %q", tc.input, got, tc.want) + } + }) + } +} diff --git a/weed/s3api/s3api_bucket_config.go b/weed/s3api/s3api_bucket_config.go index 20367f982..fdecb24db 100644 --- a/weed/s3api/s3api_bucket_config.go +++ b/weed/s3api/s3api_bucket_config.go @@ -696,7 +696,7 @@ func (s3a *S3ApiServer) getBucketOwnership(bucket string) (string, s3err.ErrorCo return "", errCode } - return config.Ownership, s3err.ErrNone + return s3_constants.EffectiveOwnership(config.Ownership), s3err.ErrNone } // setBucketOwnership sets the ownership setting for a bucket diff --git a/weed/s3api/s3api_bucket_handlers.go b/weed/s3api/s3api_bucket_handlers.go index 1181ff727..2f4cae39d 100644 --- a/weed/s3api/s3api_bucket_handlers.go +++ b/weed/s3api/s3api_bucket_handlers.go @@ -1276,21 +1276,12 @@ func (s3a *S3ApiServer) PutBucketOwnershipControls(w http.ResponseWriter, r *htt return } - // Check if ownership needs to be updated - currentOwnership, errCode := s3a.getBucketOwnership(bucket) - if errCode != s3err.ErrNone { + // Persist even when it matches the implicit default, so a later delete has something to remove. + if errCode := s3a.setBucketOwnership(bucket, ownership); errCode != s3err.ErrNone { s3err.WriteErrorResponse(w, r, errCode) return } - if currentOwnership != ownership { - errCode = s3a.setBucketOwnership(bucket, ownership) - if errCode != s3err.ErrNone { - s3err.WriteErrorResponse(w, r, errCode) - return - } - } - if printOwnership { result := &s3.PutBucketOwnershipControlsInput{ OwnershipControls: &v, diff --git a/weed/s3api/s3api_bucket_handlers_misc_test.go b/weed/s3api/s3api_bucket_handlers_misc_test.go index 2c1019fa8..b25d8720e 100644 --- a/weed/s3api/s3api_bucket_handlers_misc_test.go +++ b/weed/s3api/s3api_bucket_handlers_misc_test.go @@ -152,6 +152,28 @@ func TestPutBucketOwnershipControlsRejectsRuleWithoutObjectOwnership(t *testing. } } +func TestGetBucketOwnershipControlsDefaultsToBucketOwnerEnforced(t *testing.T) { + ownerID := AccountAdmin.Id + s3a := newMiscTestServer(t, "b") + s3a.bucketRegistry = NewBucketRegistry(nil) + s3a.bucketRegistry.setMetadataCache(&BucketMetaData{ + Name: "b", + Owner: &s3.Owner{ID: &ownerID}, + }) + req := newBucketRequest(http.MethodGet, "b", "ownershipControls=", "") + req.Header.Set(s3_constants.AmzAccountId, AccountAdmin.Id) + rec := httptest.NewRecorder() + + s3a.GetBucketOwnershipControls(rec, req) + + if rec.Code != http.StatusOK { + t.Fatalf("status = %d, want %d, body=%s", rec.Code, http.StatusOK, rec.Body.String()) + } + if !strings.Contains(rec.Body.String(), ""+s3_constants.OwnershipBucketOwnerEnforced+"") { + t.Fatalf("body missing default ownership: %s", rec.Body.String()) + } +} + func TestGetBucketAccelerateConfiguration(t *testing.T) { s3a := newMiscTestServer(t, "b") req := newBucketRequest(http.MethodGet, "b", "accelerate=", "")