From e0fcd74e5248de2c624250c57d15f666a88d46c7 Mon Sep 17 00:00:00 2001 From: chrislu Date: Sat, 12 Jul 2025 09:32:20 -0700 Subject: [PATCH] address comments --- weed/s3api/s3api_object_handlers_put.go | 8 + weed/s3api/s3api_object_retention.go | 21 +-- weed/s3api/s3api_object_retention_test.go | 169 ++++++++++++++++++++++ weed/s3api/s3api_server.go | 8 +- weed/s3api/s3err/s3api_errors.go | 2 +- 5 files changed, 193 insertions(+), 15 deletions(-) create mode 100644 weed/s3api/s3api_object_retention_test.go diff --git a/weed/s3api/s3api_object_handlers_put.go b/weed/s3api/s3api_object_handlers_put.go index 21bb60eff..80f7979af 100644 --- a/weed/s3api/s3api_object_handlers_put.go +++ b/weed/s3api/s3api_object_handlers_put.go @@ -87,6 +87,14 @@ func (s3a *S3ApiServer) PutObjectHandler(w http.ResponseWriter, r *http.Request) if versioningEnabled { // Handle versioned PUT + // Check object lock permissions before creating new version (objects under retention should not be overwritten even in versioned buckets) + bypassGovernance := r.Header.Get("x-amz-bypass-governance-retention") == "true" + if err := s3a.checkObjectLockPermissions(bucket, object, "", bypassGovernance); err != nil { + glog.V(2).Infof("PutObjectHandler: object lock check failed for versioned PUT %s/%s: %v", bucket, object, err) + s3err.WriteErrorResponse(w, r, s3err.ErrAccessDenied) + return + } + glog.V(1).Infof("PutObjectHandler: using versioned PUT for %s/%s", bucket, object) versionId, etag, errCode := s3a.putVersionedObject(r, bucket, object, dataReader, objectContentType) if errCode != s3err.ErrNone { diff --git a/weed/s3api/s3api_object_retention.go b/weed/s3api/s3api_object_retention.go index a17915422..fa7eb6856 100644 --- a/weed/s3api/s3api_object_retention.go +++ b/weed/s3api/s3api_object_retention.go @@ -150,20 +150,21 @@ func parseObjectLockConfiguration(r *http.Request) (*ObjectLockConfiguration, er // validateRetention validates retention configuration func validateRetention(retention *ObjectRetention) error { - if retention.Mode == "" && retention.RetainUntilDate == nil { - return fmt.Errorf("retention configuration must specify either Mode or RetainUntilDate") + // AWS requires both Mode and RetainUntilDate for PutObjectRetention + if retention.Mode == "" { + return fmt.Errorf("retention configuration must specify Mode") } - if retention.Mode != "" { - if retention.Mode != s3_constants.RetentionModeGovernance && retention.Mode != s3_constants.RetentionModeCompliance { - return fmt.Errorf("invalid retention mode: %s", retention.Mode) - } + if retention.RetainUntilDate == nil { + return fmt.Errorf("retention configuration must specify RetainUntilDate") } - if retention.RetainUntilDate != nil { - if retention.RetainUntilDate.Before(time.Now()) { - return fmt.Errorf("retain until date must be in the future") - } + if retention.Mode != s3_constants.RetentionModeGovernance && retention.Mode != s3_constants.RetentionModeCompliance { + return fmt.Errorf("invalid retention mode: %s", retention.Mode) + } + + if retention.RetainUntilDate.Before(time.Now()) { + return fmt.Errorf("retain until date must be in the future") } return nil diff --git a/weed/s3api/s3api_object_retention_test.go b/weed/s3api/s3api_object_retention_test.go new file mode 100644 index 000000000..ef194317f --- /dev/null +++ b/weed/s3api/s3api_object_retention_test.go @@ -0,0 +1,169 @@ +package s3api + +import ( + "testing" + "time" + + "github.com/seaweedfs/seaweedfs/weed/s3api/s3_constants" +) + +func TestValidateRetention(t *testing.T) { + futureTime := time.Now().Add(24 * time.Hour) + pastTime := time.Now().Add(-24 * time.Hour) + + tests := []struct { + name string + retention *ObjectRetention + expectError bool + errorMsg string + }{ + { + name: "Valid GOVERNANCE retention", + retention: &ObjectRetention{ + Mode: s3_constants.RetentionModeGovernance, + RetainUntilDate: &futureTime, + }, + expectError: false, + }, + { + name: "Valid COMPLIANCE retention", + retention: &ObjectRetention{ + Mode: s3_constants.RetentionModeCompliance, + RetainUntilDate: &futureTime, + }, + expectError: false, + }, + { + name: "Missing Mode", + retention: &ObjectRetention{ + RetainUntilDate: &futureTime, + }, + expectError: true, + errorMsg: "retention configuration must specify Mode", + }, + { + name: "Missing RetainUntilDate", + retention: &ObjectRetention{ + Mode: s3_constants.RetentionModeGovernance, + }, + expectError: true, + errorMsg: "retention configuration must specify RetainUntilDate", + }, + { + name: "Invalid Mode", + retention: &ObjectRetention{ + Mode: "INVALID_MODE", + RetainUntilDate: &futureTime, + }, + expectError: true, + errorMsg: "invalid retention mode: INVALID_MODE", + }, + { + name: "Past RetainUntilDate", + retention: &ObjectRetention{ + Mode: s3_constants.RetentionModeGovernance, + RetainUntilDate: &pastTime, + }, + expectError: true, + errorMsg: "retain until date must be in the future", + }, + { + name: "Empty retention", + retention: &ObjectRetention{}, + expectError: true, + errorMsg: "retention configuration must specify Mode", + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + err := validateRetention(tt.retention) + + if tt.expectError { + if err == nil { + t.Errorf("Expected error but got none") + } else if err.Error() != tt.errorMsg { + t.Errorf("Expected error message '%s', got '%s'", tt.errorMsg, err.Error()) + } + } else { + if err != nil { + t.Errorf("Expected no error but got: %v", err) + } + } + }) + } +} + +func TestValidateLegalHold(t *testing.T) { + tests := []struct { + name string + legalHold *ObjectLegalHold + expectError bool + errorMsg string + }{ + { + name: "Valid ON status", + legalHold: &ObjectLegalHold{ + Status: s3_constants.LegalHoldOn, + }, + expectError: false, + }, + { + name: "Valid OFF status", + legalHold: &ObjectLegalHold{ + Status: s3_constants.LegalHoldOff, + }, + expectError: false, + }, + { + name: "Invalid status", + legalHold: &ObjectLegalHold{ + Status: "INVALID_STATUS", + }, + expectError: true, + errorMsg: "invalid legal hold status: INVALID_STATUS", + }, + { + name: "Empty status", + legalHold: &ObjectLegalHold{ + Status: "", + }, + expectError: true, + errorMsg: "invalid legal hold status: ", + }, + { + name: "Lowercase on", + legalHold: &ObjectLegalHold{ + Status: "on", + }, + expectError: true, + errorMsg: "invalid legal hold status: on", + }, + { + name: "Lowercase off", + legalHold: &ObjectLegalHold{ + Status: "off", + }, + expectError: true, + errorMsg: "invalid legal hold status: off", + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + err := validateLegalHold(tt.legalHold) + + if tt.expectError { + if err == nil { + t.Errorf("Expected error but got none") + } else if err.Error() != tt.errorMsg { + t.Errorf("Expected error message '%s', got '%s'", tt.errorMsg, err.Error()) + } + } else { + if err != nil { + t.Errorf("Expected no error but got: %v", err) + } + } + }) + } +} diff --git a/weed/s3api/s3api_server.go b/weed/s3api/s3api_server.go index 32dd29f4d..7f1876f24 100644 --- a/weed/s3api/s3api_server.go +++ b/weed/s3api/s3api_server.go @@ -206,8 +206,6 @@ func (s3a *S3ApiServer) registerRouter(router *mux.Router) { bucket.Methods(http.MethodPut).Path("/{object:.+}").HandlerFunc(track(s3a.iam.Auth(s3a.cb.Limit(s3a.PutObjectRetentionHandler, ACTION_WRITE)), "PUT")).Queries("retention", "") // PutObjectLegalHold bucket.Methods(http.MethodPut).Path("/{object:.+}").HandlerFunc(track(s3a.iam.Auth(s3a.cb.Limit(s3a.PutObjectLegalHoldHandler, ACTION_WRITE)), "PUT")).Queries("legal-hold", "") - // PutObjectLockConfiguration - bucket.Methods(http.MethodPut).Path("/{object:.+}").HandlerFunc(track(s3a.iam.Auth(s3a.cb.Limit(s3a.PutObjectLockConfigurationHandler, ACTION_WRITE)), "PUT")).Queries("object-lock", "") // GetObjectACL bucket.Methods(http.MethodGet).Path("/{object:.+}").HandlerFunc(track(s3a.iam.Auth(s3a.cb.Limit(s3a.GetObjectAclHandler, ACTION_READ_ACP)), "GET")).Queries("acl", "") @@ -215,8 +213,6 @@ func (s3a *S3ApiServer) registerRouter(router *mux.Router) { bucket.Methods(http.MethodGet).Path("/{object:.+}").HandlerFunc(track(s3a.iam.Auth(s3a.cb.Limit(s3a.GetObjectRetentionHandler, ACTION_READ)), "GET")).Queries("retention", "") // GetObjectLegalHold bucket.Methods(http.MethodGet).Path("/{object:.+}").HandlerFunc(track(s3a.iam.Auth(s3a.cb.Limit(s3a.GetObjectLegalHoldHandler, ACTION_READ)), "GET")).Queries("legal-hold", "") - // GetObjectLockConfiguration - bucket.Methods(http.MethodGet).Path("/{object:.+}").HandlerFunc(track(s3a.iam.Auth(s3a.cb.Limit(s3a.GetObjectLockConfigurationHandler, ACTION_READ)), "GET")).Queries("object-lock", "") // objects with query @@ -278,6 +274,10 @@ func (s3a *S3ApiServer) registerRouter(router *mux.Router) { bucket.Methods(http.MethodGet).HandlerFunc(track(s3a.iam.Auth(s3a.cb.Limit(s3a.GetBucketVersioningHandler, ACTION_READ)), "GET")).Queries("versioning", "") bucket.Methods(http.MethodPut).HandlerFunc(track(s3a.iam.Auth(s3a.cb.Limit(s3a.PutBucketVersioningHandler, ACTION_WRITE)), "PUT")).Queries("versioning", "") + // GetObjectLockConfiguration / PutObjectLockConfiguration (bucket-level operations) + bucket.Methods(http.MethodGet).HandlerFunc(track(s3a.iam.Auth(s3a.cb.Limit(s3a.GetObjectLockConfigurationHandler, ACTION_READ)), "GET")).Queries("object-lock", "") + bucket.Methods(http.MethodPut).HandlerFunc(track(s3a.iam.Auth(s3a.cb.Limit(s3a.PutObjectLockConfigurationHandler, ACTION_WRITE)), "PUT")).Queries("object-lock", "") + // GetBucketTagging bucket.Methods(http.MethodGet).HandlerFunc(track(s3a.iam.Auth(s3a.cb.Limit(s3a.GetBucketTaggingHandler, ACTION_TAGGING)), "GET")).Queries("tagging", "") bucket.Methods(http.MethodPut).HandlerFunc(track(s3a.iam.Auth(s3a.cb.Limit(s3a.PutBucketTaggingHandler, ACTION_TAGGING)), "PUT")).Queries("tagging", "") diff --git a/weed/s3api/s3err/s3api_errors.go b/weed/s3api/s3err/s3api_errors.go index c5f7dbf33..a73a9631c 100644 --- a/weed/s3api/s3err/s3api_errors.go +++ b/weed/s3api/s3err/s3api_errors.go @@ -199,7 +199,7 @@ var errorCodeResponse = map[ErrorCode]APIError{ HTTPStatusCode: http.StatusNotFound, }, ErrNoSuchObjectLockConfiguration: { - Code: "ObjectLockConfigurationNotFoundError", + Code: "NoSuchObjectLockConfiguration", Description: "The specified object does not have an ObjectLock configuration", HTTPStatusCode: http.StatusNotFound, },