Mirror
/
seaweedfs
mirror of https://github.com/seaweedfs/seaweedfs.git
1
0
Fork 0
Code Issues Projects Releases Wiki Activity
Browse Source

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.
master
Chris Lu 12 hours ago
committed by GitHub
parent
b6d99f1c9e
commit
808205e38f
No known key found for this signature in database GPG Key ID: B5690EEEBB952194
2 changed files with 233 additions and 8 deletions
Whitespace
Show all changes Ignore whitespace when comparing lines Ignore changes in amount of whitespace Ignore changes in whitespace at EOL
Split View
Diff Options
Show Stats Download Patch File Download Diff File
  1. 45
      weed/s3api/s3api_object_handlers_put.go
  2. 196
      weed/s3api/s3api_object_ownership_test.go

45
weed/s3api/s3api_object_handlers_put.go
View File

@ -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 {

196
weed/s3api/s3api_object_ownership_test.go
View File

@ -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
}
Write Preview
Loading…
Cancel
Save