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=", "")