From b9b480e6054a0498367f8089a5fcafbdc1e80a67 Mon Sep 17 00:00:00 2001 From: chrislu Date: Sat, 12 Jul 2025 11:56:58 -0700 Subject: [PATCH] only checkObjectLockPermissions if versioningEnabled --- weed/s3api/s3api_object_handlers_delete.go | 46 ++-- weed/s3api/s3api_object_handlers_put.go | 23 +- weed/s3api/s3api_object_handlers_retention.go | 48 +++- weed/s3api/s3api_object_retention.go | 84 ++++++ weed/s3api/s3api_object_retention_test.go | 250 +++++++++++++++++- 5 files changed, 406 insertions(+), 45 deletions(-) diff --git a/weed/s3api/s3api_object_handlers_delete.go b/weed/s3api/s3api_object_handlers_delete.go index 901aeb912..3e7ccefc2 100644 --- a/weed/s3api/s3api_object_handlers_delete.go +++ b/weed/s3api/s3api_object_handlers_delete.go @@ -49,12 +49,14 @@ func (s3a *S3ApiServer) DeleteObjectHandler(w http.ResponseWriter, r *http.Reque auditLog = s3err.GetAccessLog(r, http.StatusNoContent, s3err.ErrNone) } - // Check object lock permissions before deletion - bypassGovernance := r.Header.Get("x-amz-bypass-governance-retention") == "true" - if err := s3a.checkObjectLockPermissions(bucket, object, versionId, bypassGovernance); err != nil { - glog.V(2).Infof("DeleteObjectHandler: object lock check failed for %s/%s: %v", bucket, object, err) - s3err.WriteErrorResponse(w, r, s3err.ErrAccessDenied) - return + // Check object lock permissions before deletion (only for versioned buckets) + if versioningEnabled { + bypassGovernance := r.Header.Get("x-amz-bypass-governance-retention") == "true" + if err := s3a.checkObjectLockPermissions(bucket, object, versionId, bypassGovernance); err != nil { + glog.V(2).Infof("DeleteObjectHandler: object lock check failed for %s/%s: %v", bucket, object, err) + s3err.WriteErrorResponse(w, r, s3err.ErrAccessDenied) + return + } } if versioningEnabled { @@ -192,6 +194,18 @@ func (s3a *S3ApiServer) DeleteMultipleObjectsHandler(w http.ResponseWriter, r *h // Check for bypass governance retention header bypassGovernance := r.Header.Get("x-amz-bypass-governance-retention") == "true" + // Check if versioning is enabled for the bucket (needed for object lock checks) + versioningEnabled, err := s3a.isVersioningEnabled(bucket) + if err != nil { + if err == filer_pb.ErrNotFound { + s3err.WriteErrorResponse(w, r, s3err.ErrNoSuchBucket) + return + } + glog.Errorf("Error checking versioning status for bucket %s: %v", bucket, err) + s3err.WriteErrorResponse(w, r, s3err.ErrInternalError) + return + } + s3a.WithFilerClient(false, func(client filer_pb.SeaweedFilerClient) error { // delete file entries @@ -200,15 +214,17 @@ func (s3a *S3ApiServer) DeleteMultipleObjectsHandler(w http.ResponseWriter, r *h continue } - // Check object lock permissions before deletion - if err := s3a.checkObjectLockPermissions(bucket, object.ObjectName, "", bypassGovernance); err != nil { - glog.V(2).Infof("DeleteMultipleObjectsHandler: object lock check failed for %s/%s: %v", bucket, object.ObjectName, err) - deleteErrors = append(deleteErrors, DeleteError{ - Code: "AccessDenied", - Message: err.Error(), - Key: object.ObjectName, - }) - continue + // Check object lock permissions before deletion (only for versioned buckets) + if versioningEnabled { + if err := s3a.checkObjectLockPermissions(bucket, object.ObjectName, "", bypassGovernance); err != nil { + glog.V(2).Infof("DeleteMultipleObjectsHandler: object lock check failed for %s/%s: %v", bucket, object.ObjectName, err) + deleteErrors = append(deleteErrors, DeleteError{ + Code: "AccessDenied", + Message: err.Error(), + Key: object.ObjectName, + }) + continue + } } lastSeparator := strings.LastIndex(object.ObjectName, "/") parentDirectoryPath, entryName, isDeleteData, isRecursive := "", object.ObjectName, true, false diff --git a/weed/s3api/s3api_object_handlers_put.go b/weed/s3api/s3api_object_handlers_put.go index 80f7979af..371ab870f 100644 --- a/weed/s3api/s3api_object_handlers_put.go +++ b/weed/s3api/s3api_object_handlers_put.go @@ -85,16 +85,15 @@ func (s3a *S3ApiServer) PutObjectHandler(w http.ResponseWriter, r *http.Request) glog.V(1).Infof("PutObjectHandler: bucket %s, object %s, versioningEnabled=%v", bucket, object, versioningEnabled) + // Check object lock permissions before PUT operation (only for versioned buckets) + bypassGovernance := r.Header.Get("x-amz-bypass-governance-retention") == "true" + if err := s3a.checkObjectLockPermissionsForPut(bucket, object, bypassGovernance, versioningEnabled); err != nil { + s3err.WriteErrorResponse(w, r, s3err.ErrAccessDenied) + return + } + 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 { @@ -111,14 +110,6 @@ func (s3a *S3ApiServer) PutObjectHandler(w http.ResponseWriter, r *http.Request) setEtag(w, etag) } else { // Handle regular PUT (non-versioned) - // Check object lock permissions before overwriting (only for non-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 %s/%s: %v", bucket, object, err) - s3err.WriteErrorResponse(w, r, s3err.ErrAccessDenied) - return - } - glog.V(1).Infof("PutObjectHandler: using regular PUT for %s/%s", bucket, object) uploadUrl := s3a.toFilerUrl(bucket, object) if objectContentType == "" { diff --git a/weed/s3api/s3api_object_handlers_retention.go b/weed/s3api/s3api_object_handlers_retention.go index eaf73bd12..08224cc2f 100644 --- a/weed/s3api/s3api_object_handlers_retention.go +++ b/weed/s3api/s3api_object_handlers_retention.go @@ -17,6 +17,17 @@ func (s3a *S3ApiServer) PutObjectRetentionHandler(w http.ResponseWriter, r *http bucket, object := s3_constants.GetBucketAndObject(r) glog.V(3).Infof("PutObjectRetentionHandler %s %s", bucket, object) + // Check if Object Lock is available for this bucket (requires versioning) + if err := s3a.isObjectLockAvailable(bucket); err != nil { + glog.Errorf("PutObjectRetentionHandler: object lock not available for bucket %s: %v", bucket, err) + if strings.Contains(err.Error(), "bucket not found") { + s3err.WriteErrorResponse(w, r, s3err.ErrNoSuchBucket) + } else { + s3err.WriteErrorResponse(w, r, s3err.ErrInvalidRequest) + } + return + } + // Get version ID from query parameters versionId := r.URL.Query().Get("versionId") @@ -71,6 +82,17 @@ func (s3a *S3ApiServer) GetObjectRetentionHandler(w http.ResponseWriter, r *http bucket, object := s3_constants.GetBucketAndObject(r) glog.V(3).Infof("GetObjectRetentionHandler %s %s", bucket, object) + // Check if Object Lock is available for this bucket (requires versioning) + if err := s3a.isObjectLockAvailable(bucket); err != nil { + glog.Errorf("GetObjectRetentionHandler: object lock not available for bucket %s: %v", bucket, err) + if strings.Contains(err.Error(), "bucket not found") { + s3err.WriteErrorResponse(w, r, s3err.ErrNoSuchBucket) + } else { + s3err.WriteErrorResponse(w, r, s3err.ErrInvalidRequest) + } + return + } + // Get version ID from query parameters versionId := r.URL.Query().Get("versionId") @@ -129,6 +151,17 @@ func (s3a *S3ApiServer) PutObjectLegalHoldHandler(w http.ResponseWriter, r *http bucket, object := s3_constants.GetBucketAndObject(r) glog.V(3).Infof("PutObjectLegalHoldHandler %s %s", bucket, object) + // Check if Object Lock is available for this bucket (requires versioning) + if err := s3a.isObjectLockAvailable(bucket); err != nil { + glog.Errorf("PutObjectLegalHoldHandler: object lock not available for bucket %s: %v", bucket, err) + if strings.Contains(err.Error(), "bucket not found") { + s3err.WriteErrorResponse(w, r, s3err.ErrNoSuchBucket) + } else { + s3err.WriteErrorResponse(w, r, s3err.ErrInvalidRequest) + } + return + } + // Get version ID from query parameters versionId := r.URL.Query().Get("versionId") @@ -175,6 +208,17 @@ func (s3a *S3ApiServer) GetObjectLegalHoldHandler(w http.ResponseWriter, r *http bucket, object := s3_constants.GetBucketAndObject(r) glog.V(3).Infof("GetObjectLegalHoldHandler %s %s", bucket, object) + // Check if Object Lock is available for this bucket (requires versioning) + if err := s3a.isObjectLockAvailable(bucket); err != nil { + glog.Errorf("GetObjectLegalHoldHandler: object lock not available for bucket %s: %v", bucket, err) + if strings.Contains(err.Error(), "bucket not found") { + s3err.WriteErrorResponse(w, r, s3err.ErrNoSuchBucket) + } else { + s3err.WriteErrorResponse(w, r, s3err.ErrInvalidRequest) + } + return + } + // Get version ID from query parameters versionId := r.URL.Query().Get("versionId") @@ -242,8 +286,8 @@ func (s3a *S3ApiServer) PutObjectLockConfigurationHandler(w http.ResponseWriter, } // Validate object lock configuration - if config.ObjectLockEnabled != "" && config.ObjectLockEnabled != s3_constants.ObjectLockEnabled { - glog.Errorf("PutObjectLockConfigurationHandler: invalid object lock enabled value: %s", config.ObjectLockEnabled) + if err := validateObjectLockConfiguration(config); err != nil { + glog.Errorf("PutObjectLockConfigurationHandler: invalid object lock config: %v", err) s3err.WriteErrorResponse(w, r, s3err.ErrInvalidRequest) return } diff --git a/weed/s3api/s3api_object_retention.go b/weed/s3api/s3api_object_retention.go index b23806c35..32307e0e7 100644 --- a/weed/s3api/s3api_object_retention.go +++ b/weed/s3api/s3api_object_retention.go @@ -149,6 +149,56 @@ func validateLegalHold(legalHold *ObjectLegalHold) error { return nil } +// validateObjectLockConfiguration validates object lock configuration +func validateObjectLockConfiguration(config *ObjectLockConfiguration) error { + // Validate ObjectLockEnabled if present + if config.ObjectLockEnabled != "" && config.ObjectLockEnabled != s3_constants.ObjectLockEnabled { + return fmt.Errorf("invalid object lock enabled value: %s", config.ObjectLockEnabled) + } + + // Validate Rule if present + if config.Rule != nil { + if config.Rule.DefaultRetention != nil { + return validateDefaultRetention(config.Rule.DefaultRetention) + } + } + + return nil +} + +// validateDefaultRetention validates default retention configuration +func validateDefaultRetention(retention *DefaultRetention) error { + // Mode is required + if retention.Mode == "" { + return fmt.Errorf("default retention must specify Mode") + } + + // Mode must be valid + if retention.Mode != s3_constants.RetentionModeGovernance && retention.Mode != s3_constants.RetentionModeCompliance { + return fmt.Errorf("invalid default retention mode: %s", retention.Mode) + } + + // Exactly one of Days or Years must be specified + if retention.Days == 0 && retention.Years == 0 { + return fmt.Errorf("default retention must specify either Days or Years") + } + + if retention.Days > 0 && retention.Years > 0 { + return fmt.Errorf("default retention cannot specify both Days and Years") + } + + // Validate ranges + if retention.Days < 0 || retention.Days > 36500 { + return fmt.Errorf("default retention days must be between 0 and 36500") + } + + if retention.Years < 0 || retention.Years > 100 { + return fmt.Errorf("default retention years must be between 0 and 100") + } + + return nil +} + // getObjectRetention retrieves retention configuration from object metadata func (s3a *S3ApiServer) getObjectRetention(bucket, object, versionId string) (*ObjectRetention, error) { var entry *filer_pb.Entry @@ -448,3 +498,37 @@ func (s3a *S3ApiServer) checkObjectLockPermissions(bucket, object, versionId str return nil } + +// isObjectLockAvailable checks if Object Lock features are available for the bucket +// Object Lock requires versioning to be enabled (AWS S3 requirement) +func (s3a *S3ApiServer) isObjectLockAvailable(bucket string) error { + versioningEnabled, err := s3a.isVersioningEnabled(bucket) + if err != nil { + if err == filer_pb.ErrNotFound { + return fmt.Errorf("bucket not found") + } + return fmt.Errorf("error checking versioning status: %v", err) + } + + if !versioningEnabled { + return fmt.Errorf("object lock requires versioning to be enabled") + } + + return nil +} + +// checkObjectLockPermissionsForPut checks object lock permissions for PUT operations +// This is a shared helper to avoid code duplication in PUT handlers +func (s3a *S3ApiServer) checkObjectLockPermissionsForPut(bucket, object string, bypassGovernance bool, versioningEnabled bool) error { + // Object Lock only applies to versioned buckets (AWS S3 requirement) + if !versioningEnabled { + return nil + } + + // For PUT operations, we check permissions on the current object (empty versionId) + if err := s3a.checkObjectLockPermissions(bucket, object, "", bypassGovernance); err != nil { + glog.V(2).Infof("checkObjectLockPermissionsForPut: object lock check failed for %s/%s: %v", bucket, object, err) + return err + } + return nil +} diff --git a/weed/s3api/s3api_object_retention_test.go b/weed/s3api/s3api_object_retention_test.go index ebe55fbfd..a495150b4 100644 --- a/weed/s3api/s3api_object_retention_test.go +++ b/weed/s3api/s3api_object_retention_test.go @@ -326,7 +326,7 @@ func TestParseXMLGeneric(t *testing.T) { xmlBody: ` GOVERNANCE - 2024-12-31T23:59:59Z + 2025-01-01T00:00:00Z `, expectError: false, }, @@ -334,11 +334,11 @@ func TestParseXMLGeneric(t *testing.T) { name: "Empty body", xmlBody: "", expectError: true, - errorMsg: "empty request body", + errorMsg: "error parsing XML", }, { name: "Invalid XML", - xmlBody: "", + xmlBody: "not xml", expectError: true, errorMsg: "error parsing XML", }, @@ -346,13 +346,8 @@ func TestParseXMLGeneric(t *testing.T) { for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - var req *http.Request - if tt.xmlBody == "" { - req = &http.Request{Body: nil} - } else { - req = &http.Request{ - Body: io.NopCloser(strings.NewReader(tt.xmlBody)), - } + req := &http.Request{ + Body: io.NopCloser(strings.NewReader(tt.xmlBody)), } var retention ObjectRetention @@ -362,11 +357,242 @@ func TestParseXMLGeneric(t *testing.T) { if err == nil { t.Errorf("Expected error but got none") } else if !strings.Contains(err.Error(), tt.errorMsg) { - t.Errorf("Expected error message to contain '%s', got '%s'", tt.errorMsg, err.Error()) + t.Errorf("Expected error message to contain '%s', got: %v", tt.errorMsg, err) } } else { if err != nil { - t.Errorf("Expected no error but got: %v", err) + t.Errorf("Unexpected error: %v", err) + } + } + }) + } +} + +func TestValidateObjectLockConfiguration(t *testing.T) { + tests := []struct { + name string + config *ObjectLockConfiguration + expectError bool + errorMsg string + }{ + { + name: "Valid config with ObjectLockEnabled only", + config: &ObjectLockConfiguration{ + ObjectLockEnabled: "Enabled", + }, + expectError: false, + }, + { + name: "Valid config with rule and days", + config: &ObjectLockConfiguration{ + ObjectLockEnabled: "Enabled", + Rule: &ObjectLockRule{ + DefaultRetention: &DefaultRetention{ + Mode: "GOVERNANCE", + Days: 30, + }, + }, + }, + expectError: false, + }, + { + name: "Valid config with rule and years", + config: &ObjectLockConfiguration{ + ObjectLockEnabled: "Enabled", + Rule: &ObjectLockRule{ + DefaultRetention: &DefaultRetention{ + Mode: "COMPLIANCE", + Years: 1, + }, + }, + }, + expectError: false, + }, + { + name: "Invalid ObjectLockEnabled value", + config: &ObjectLockConfiguration{ + ObjectLockEnabled: "InvalidValue", + }, + expectError: true, + errorMsg: "invalid object lock enabled value", + }, + { + name: "Invalid rule - missing mode", + config: &ObjectLockConfiguration{ + ObjectLockEnabled: "Enabled", + Rule: &ObjectLockRule{ + DefaultRetention: &DefaultRetention{ + Days: 30, + }, + }, + }, + expectError: true, + errorMsg: "default retention must specify Mode", + }, + { + name: "Invalid rule - both days and years", + config: &ObjectLockConfiguration{ + ObjectLockEnabled: "Enabled", + Rule: &ObjectLockRule{ + DefaultRetention: &DefaultRetention{ + Mode: "GOVERNANCE", + Days: 30, + Years: 1, + }, + }, + }, + expectError: true, + errorMsg: "default retention cannot specify both Days and Years", + }, + { + name: "Invalid rule - neither days nor years", + config: &ObjectLockConfiguration{ + ObjectLockEnabled: "Enabled", + Rule: &ObjectLockRule{ + DefaultRetention: &DefaultRetention{ + Mode: "GOVERNANCE", + }, + }, + }, + expectError: true, + errorMsg: "default retention must specify either Days or Years", + }, + { + name: "Invalid rule - invalid mode", + config: &ObjectLockConfiguration{ + ObjectLockEnabled: "Enabled", + Rule: &ObjectLockRule{ + DefaultRetention: &DefaultRetention{ + Mode: "INVALID_MODE", + Days: 30, + }, + }, + }, + expectError: true, + errorMsg: "invalid default retention mode", + }, + { + name: "Invalid rule - days out of range", + config: &ObjectLockConfiguration{ + ObjectLockEnabled: "Enabled", + Rule: &ObjectLockRule{ + DefaultRetention: &DefaultRetention{ + Mode: "GOVERNANCE", + Days: 50000, + }, + }, + }, + expectError: true, + errorMsg: "default retention days must be between 0 and 36500", + }, + { + name: "Invalid rule - years out of range", + config: &ObjectLockConfiguration{ + ObjectLockEnabled: "Enabled", + Rule: &ObjectLockRule{ + DefaultRetention: &DefaultRetention{ + Mode: "GOVERNANCE", + Years: 200, + }, + }, + }, + expectError: true, + errorMsg: "default retention years must be between 0 and 100", + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + err := validateObjectLockConfiguration(tt.config) + + if tt.expectError { + if err == nil { + t.Errorf("Expected error but got none") + } else if !strings.Contains(err.Error(), tt.errorMsg) { + t.Errorf("Expected error message to contain '%s', got: %v", tt.errorMsg, err) + } + } else { + if err != nil { + t.Errorf("Unexpected error: %v", err) + } + } + }) + } +} + +func TestValidateDefaultRetention(t *testing.T) { + tests := []struct { + name string + retention *DefaultRetention + expectError bool + errorMsg string + }{ + { + name: "Valid retention with days", + retention: &DefaultRetention{ + Mode: "GOVERNANCE", + Days: 30, + }, + expectError: false, + }, + { + name: "Valid retention with years", + retention: &DefaultRetention{ + Mode: "COMPLIANCE", + Years: 1, + }, + expectError: false, + }, + { + name: "Missing mode", + retention: &DefaultRetention{ + Days: 30, + }, + expectError: true, + errorMsg: "default retention must specify Mode", + }, + { + name: "Invalid mode", + retention: &DefaultRetention{ + Mode: "INVALID", + Days: 30, + }, + expectError: true, + errorMsg: "invalid default retention mode", + }, + { + name: "Both days and years specified", + retention: &DefaultRetention{ + Mode: "GOVERNANCE", + Days: 30, + Years: 1, + }, + expectError: true, + errorMsg: "default retention cannot specify both Days and Years", + }, + { + name: "Neither days nor years specified", + retention: &DefaultRetention{ + Mode: "GOVERNANCE", + }, + expectError: true, + errorMsg: "default retention must specify either Days or Years", + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + err := validateDefaultRetention(tt.retention) + + if tt.expectError { + if err == nil { + t.Errorf("Expected error but got none") + } else if !strings.Contains(err.Error(), tt.errorMsg) { + t.Errorf("Expected error message to contain '%s', got: %v", tt.errorMsg, err) + } + } else { + if err != nil { + t.Errorf("Unexpected error: %v", err) } } })