From 808205e38f6eb578e36a85d16682370760d26d09 Mon Sep 17 00:00:00 2001 From: Chris Lu Date: Mon, 29 Dec 2025 23:54:00 -0800 Subject: [PATCH] s3: implement Bucket Owner Enforced for object ownership (#7913) * s3: implement Bucket Owner Enforced for object ownership Objects uploaded by service accounts (or any user) are now owned by the bucket owner when the bucket has BucketOwnerEnforced ownership policy (the modern AWS default since April 2023). This provides a more intuitive ownership model where users expect objects created by their service accounts to be owned by themselves. - Modified setObjectOwnerFromRequest to check bucket ObjectOwnership - When BucketOwnerEnforced: use bucket owner's account ID - When ObjectWriter: use uploader's account ID (backward compatible) * s3: add nil check and fix ownership logic hole - Add nil check for bucketRegistry before calling GetBucketMetadata - Fix logic hole where objects could be created without owner when BucketOwnerEnforced is set but bucket owner is nil - Refactor to ensure objects always have an owner by falling back to uploader when bucket owner is unavailable - Improve logging to distinguish between different fallback scenarios Addresses code review feedback from Gemini on PR #7913 * s3: add comprehensive tests for object ownership logic Add unit tests for setObjectOwnerFromRequest covering: - BucketOwnerEnforced: uses bucket owner - ObjectWriter: uses uploader - BucketOwnerPreferred: uses uploader - Nil owner fallback scenarios - Bucket metadata errors - Nil bucketRegistry - Empty account ID handling All 8 test cases pass, verifying correct ownership assignment in all scenarios including edge cases. --- weed/s3api/s3api_object_handlers_put.go | 45 ++++- weed/s3api/s3api_object_ownership_test.go | 196 ++++++++++++++++++++++ 2 files changed, 233 insertions(+), 8 deletions(-) create mode 100644 weed/s3api/s3api_object_ownership_test.go diff --git a/weed/s3api/s3api_object_handlers_put.go b/weed/s3api/s3api_object_handlers_put.go index 9e325801d..8c0561a89 100644 --- a/weed/s3api/s3api_object_handlers_put.go +++ b/weed/s3api/s3api_object_handlers_put.go @@ -141,7 +141,7 @@ func (s3a *S3ApiServer) PutObjectHandler(w http.ResponseWriter, r *http.Request) entry.Attributes.Mime = objectContentType // Set object owner for directory objects (same as regular objects) - s3a.setObjectOwnerFromRequest(r, entry) + s3a.setObjectOwnerFromRequest(r, bucket, entry) }); err != nil { s3err.WriteErrorResponse(w, r, s3err.ErrInternalError) return @@ -748,15 +748,44 @@ func filerErrorToS3Error(err error) s3err.ErrorCode { } } -// setObjectOwnerFromRequest sets the object owner metadata based on the authenticated user -func (s3a *S3ApiServer) setObjectOwnerFromRequest(r *http.Request, entry *filer_pb.Entry) { - amzAccountId := r.Header.Get(s3_constants.AmzAccountId) - if amzAccountId != "" { +// setObjectOwnerFromRequest sets the object owner metadata based on the bucket ownership policy. +// When BucketOwnerEnforced (the modern AWS default), the bucket owner owns all objects. +// Otherwise, the uploader's account ID is used (ObjectWriter mode). +func (s3a *S3ApiServer) setObjectOwnerFromRequest(r *http.Request, bucket string, entry *filer_pb.Entry) { + var ownerId string + + // Check if bucketRegistry is available + if s3a.bucketRegistry == nil { + // Fallback to uploader if registry unavailable + ownerId = r.Header.Get(s3_constants.AmzAccountId) + glog.V(2).Infof("setObjectOwnerFromRequest: bucketRegistry unavailable, fallback to uploader %s", ownerId) + } else { + // Check bucket ownership policy + bucketMetadata, errCode := s3a.bucketRegistry.GetBucketMetadata(bucket) + useBucketOwner := errCode == s3err.ErrNone && bucketMetadata != nil && + bucketMetadata.ObjectOwnership == s3_constants.OwnershipBucketOwnerEnforced && + bucketMetadata.Owner != nil && bucketMetadata.Owner.ID != nil + + if useBucketOwner { + ownerId = *bucketMetadata.Owner.ID + glog.V(2).Infof("setObjectOwnerFromRequest: using bucket owner %s (BucketOwnerEnforced)", ownerId) + } else { + ownerId = r.Header.Get(s3_constants.AmzAccountId) + if errCode != s3err.ErrNone || bucketMetadata == nil { + glog.V(2).Infof("setObjectOwnerFromRequest: fallback to uploader %s", ownerId) + } else if bucketMetadata.ObjectOwnership == s3_constants.OwnershipBucketOwnerEnforced { + glog.V(2).Infof("setObjectOwnerFromRequest: BucketOwnerEnforced but no owner found, fallback to uploader %s", ownerId) + } else { + glog.V(2).Infof("setObjectOwnerFromRequest: using uploader %s (ObjectWriter mode)", ownerId) + } + } + } + + if ownerId != "" { if entry.Extended == nil { entry.Extended = make(map[string][]byte) } - entry.Extended[s3_constants.ExtAmzOwnerKey] = []byte(amzAccountId) - glog.V(2).Infof("setObjectOwnerFromRequest: set object owner to %s", amzAccountId) + entry.Extended[s3_constants.ExtAmzOwnerKey] = []byte(ownerId) } } @@ -1035,7 +1064,7 @@ func (s3a *S3ApiServer) putVersionedObject(r *http.Request, bucket, object strin versionEntry.Extended[s3_constants.ExtETagKey] = []byte(etag) // Set object owner for versioned objects - s3a.setObjectOwnerFromRequest(r, versionEntry) + s3a.setObjectOwnerFromRequest(r, bucket, versionEntry) // Extract and store object lock metadata from request headers if err := s3a.extractObjectLockMetadataFromRequest(r, versionEntry); err != nil { diff --git a/weed/s3api/s3api_object_ownership_test.go b/weed/s3api/s3api_object_ownership_test.go new file mode 100644 index 000000000..849f442c7 --- /dev/null +++ b/weed/s3api/s3api_object_ownership_test.go @@ -0,0 +1,196 @@ +package s3api + +import ( + "net/http" + "testing" + + "github.com/aws/aws-sdk-go/service/s3" + "github.com/seaweedfs/seaweedfs/weed/pb/filer_pb" + "github.com/seaweedfs/seaweedfs/weed/s3api/s3_constants" + "github.com/seaweedfs/seaweedfs/weed/s3api/s3err" +) + +func TestSetObjectOwnerFromRequest(t *testing.T) { + tests := []struct { + name string + bucketRegistryNil bool + bucketMetadata *BucketMetaData + bucketMetadataError s3err.ErrorCode + uploaderAccountId string + expectedOwnerId string + description string + }{ + { + name: "BucketOwnerEnforced - use bucket owner", + bucketRegistryNil: false, + bucketMetadata: &BucketMetaData{ + Name: "test-bucket", + ObjectOwnership: s3_constants.OwnershipBucketOwnerEnforced, + Owner: &s3.Owner{ + ID: stringPtr("bucket-owner-123"), + DisplayName: stringPtr("Bucket Owner"), + }, + }, + bucketMetadataError: s3err.ErrNone, + uploaderAccountId: "uploader-456", + expectedOwnerId: "bucket-owner-123", + description: "Should use bucket owner when BucketOwnerEnforced", + }, + { + name: "ObjectWriter - use uploader", + bucketRegistryNil: false, + bucketMetadata: &BucketMetaData{ + Name: "test-bucket", + ObjectOwnership: s3_constants.OwnershipObjectWriter, + Owner: &s3.Owner{ + ID: stringPtr("bucket-owner-123"), + DisplayName: stringPtr("Bucket Owner"), + }, + }, + bucketMetadataError: s3err.ErrNone, + uploaderAccountId: "uploader-456", + expectedOwnerId: "uploader-456", + description: "Should use uploader when ObjectWriter mode", + }, + { + name: "BucketOwnerPreferred - use uploader", + bucketRegistryNil: false, + bucketMetadata: &BucketMetaData{ + Name: "test-bucket", + ObjectOwnership: s3_constants.OwnershipBucketOwnerPreferred, + Owner: &s3.Owner{ + ID: stringPtr("bucket-owner-123"), + DisplayName: stringPtr("Bucket Owner"), + }, + }, + bucketMetadataError: s3err.ErrNone, + uploaderAccountId: "uploader-456", + expectedOwnerId: "uploader-456", + description: "Should use uploader when BucketOwnerPreferred mode", + }, + { + name: "BucketOwnerEnforced but owner is nil - fallback to uploader", + bucketRegistryNil: false, + bucketMetadata: &BucketMetaData{ + Name: "test-bucket", + ObjectOwnership: s3_constants.OwnershipBucketOwnerEnforced, + Owner: nil, + }, + bucketMetadataError: s3err.ErrNone, + uploaderAccountId: "uploader-456", + expectedOwnerId: "uploader-456", + description: "Should fallback to uploader when bucket owner is nil", + }, + { + name: "BucketOwnerEnforced but owner ID is nil - fallback to uploader", + bucketRegistryNil: false, + bucketMetadata: &BucketMetaData{ + Name: "test-bucket", + ObjectOwnership: s3_constants.OwnershipBucketOwnerEnforced, + Owner: &s3.Owner{ + ID: nil, + DisplayName: stringPtr("Bucket Owner"), + }, + }, + bucketMetadataError: s3err.ErrNone, + uploaderAccountId: "uploader-456", + expectedOwnerId: "uploader-456", + description: "Should fallback to uploader when bucket owner ID is nil", + }, + { + name: "Bucket metadata error - fallback to uploader", + bucketRegistryNil: false, + bucketMetadata: nil, + bucketMetadataError: s3err.ErrNoSuchBucket, + uploaderAccountId: "uploader-456", + expectedOwnerId: "uploader-456", + description: "Should fallback to uploader when bucket metadata unavailable", + }, + { + name: "Bucket registry is nil - fallback to uploader", + bucketRegistryNil: true, + uploaderAccountId: "uploader-456", + expectedOwnerId: "uploader-456", + description: "Should fallback to uploader when bucketRegistry is nil", + }, + { + name: "Empty uploader account ID - no owner set", + bucketRegistryNil: false, + bucketMetadata: &BucketMetaData{ + Name: "test-bucket", + ObjectOwnership: s3_constants.OwnershipObjectWriter, + Owner: &s3.Owner{ + ID: stringPtr("bucket-owner-123"), + DisplayName: stringPtr("Bucket Owner"), + }, + }, + bucketMetadataError: s3err.ErrNone, + uploaderAccountId: "", + expectedOwnerId: "", + description: "Should not set owner when uploader account ID is empty", + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + // Create mock S3ApiServer + s3a := &S3ApiServer{} + + // Setup bucket registry with mock behavior + if !tt.bucketRegistryNil { + // Create a minimal BucketRegistry with overridden GetBucketMetadata + s3a.bucketRegistry = &BucketRegistry{ + metadataCache: map[string]*BucketMetaData{}, + notFound: map[string]struct{}{}, + } + + // Pre-populate the cache with test metadata + if tt.bucketMetadata != nil && tt.bucketMetadataError == s3err.ErrNone { + s3a.bucketRegistry.metadataCache["test-bucket"] = tt.bucketMetadata + } else if tt.bucketMetadataError == s3err.ErrNoSuchBucket { + s3a.bucketRegistry.notFound["test-bucket"] = struct{}{} + } + } + + // Create mock request with uploader account ID + req, _ := http.NewRequest("PUT", "/test-bucket/test-object", nil) + req.Header.Set(s3_constants.AmzAccountId, tt.uploaderAccountId) + + // Create entry + entry := &filer_pb.Entry{ + Name: "test-object", + } + + // Call the function + s3a.setObjectOwnerFromRequest(req, "test-bucket", entry) + + // Verify the owner ID + if tt.expectedOwnerId == "" { + if entry.Extended != nil { + if _, exists := entry.Extended[s3_constants.ExtAmzOwnerKey]; exists { + t.Errorf("%s: Expected no owner to be set, but owner was set", tt.description) + } + } + } else { + if entry.Extended == nil { + t.Errorf("%s: Expected owner to be set, but Extended is nil", tt.description) + return + } + ownerBytes, exists := entry.Extended[s3_constants.ExtAmzOwnerKey] + if !exists { + t.Errorf("%s: Expected owner to be set, but ExtAmzOwnerKey not found", tt.description) + return + } + actualOwnerId := string(ownerBytes) + if actualOwnerId != tt.expectedOwnerId { + t.Errorf("%s: Expected owner ID %s, got %s", tt.description, tt.expectedOwnerId, actualOwnerId) + } + } + }) + } +} + +// Helper function to create string pointers +func stringPtr(s string) *string { + return &s +}