fix: fixes delete markers access for some actions

Fixes #1766
Fixes #1750

This PR focuses on two bug fixes:

First, it blocks access to delete `DeleteMarkers` for the following operations by returning a `MethodNotAllowed` error: `PutObjectTagging`, `GetObjectTagging`, `DeleteObjectTagging`, `PutObjectLegalHold`, `GetObjectLegalHold`, `PutObjectRetention`, and `GetObjectRetention`.

Second, it removes the access check that previously prevented deleting a delete marker locked by a bucket default retention rule. A delete marker should always be allowed to be deleted.

auth/object_lock.go

@@ -278,6 +278,11 @@ func CheckObjectAccess(ctx context.Context, bucket, userAccess string, objects [
 		if errors.Is(err, s3err.GetAPIError(s3err.ErrNoSuchKey)) {
 			continue
 		}
+		// the object is a delete marker, if a `MethodNotAllowed` error is returned
+		// no object lock check is needed
+		if errors.Is(err, s3err.GetAPIError(s3err.ErrMethodNotAllowed)) {
+			continue
+		}
 		if errors.Is(err, s3err.GetAPIError(s3err.ErrNoSuchObjectLockConfiguration)) {
 			checkRetention = false
 		}

backend/posix/posix.go

@@ -871,6 +871,30 @@ func getBoolPtr(b bool) *bool {
 	return &b
 }
 
+// ensureNotDeleteMarker return a `MethodNotAllowd` error
+// if the provided object(version) is a delete marker
+func (p *Posix) ensureNotDeleteMarker(bucket, object, versionId string) error {
+	if !p.versioningEnabled() {
+		return nil
+	}
+
+	_, err := p.meta.RetrieveAttribute(nil, bucket, object, deleteMarkerKey)
+	if errors.Is(err, fs.ErrNotExist) || errors.Is(err, syscall.ENOTDIR) {
+		if versionId != "" {
+			return s3err.GetAPIError(s3err.ErrNoSuchVersion)
+		}
+		return s3err.GetAPIError(s3err.ErrNoSuchKey)
+	}
+	if errors.Is(err, meta.ErrNoSuchKey) {
+		return nil
+	}
+	if err != nil {
+		return fmt.Errorf("get delete marker attr: %w", err)
+	}
+
+	return s3err.GetAPIError(s3err.ErrMethodNotAllowed)
+}
+
 // Check if the given object is a delete marker
 func (p *Posix) isObjDeleteMarker(bucket, object string) (bool, error) {
 	_, err := p.meta.RetrieveAttribute(nil, bucket, object, deleteMarkerKey)
@@ -4890,6 +4914,11 @@ func (p *Posix) GetObjectTagging(_ context.Context, bucket, object, versionId st
 		}
 	}
 
+	err = p.ensureNotDeleteMarker(bucket, object, versionId)
+	if err != nil {
+		return nil, err
+	}
+
 	return p.getAttrTags(bucket, object, versionId)
 }
 
@@ -4952,6 +4981,11 @@ func (p *Posix) PutObjectTagging(_ context.Context, bucket, object, versionId st
 		}
 	}
 
+	err = p.ensureNotDeleteMarker(bucket, object, versionId)
+	if err != nil {
+		return err
+	}
+
 	if tags == nil {
 		err = p.meta.DeleteAttribute(bucket, object, tagHdr)
 		if errors.Is(err, fs.ErrNotExist) || errors.Is(err, syscall.ENOTDIR) {
@@ -5243,6 +5277,11 @@ func (p *Posix) PutObjectLegalHold(_ context.Context, bucket, object, versionId
 		}
 	}
 
+	err = p.ensureNotDeleteMarker(bucket, object, versionId)
+	if err != nil {
+		return err
+	}
+
 	err = p.meta.StoreAttribute(nil, bucket, object, objectLegalHoldKey, statusData)
 	if errors.Is(err, fs.ErrNotExist) || errors.Is(err, syscall.ENOTDIR) {
 		if versionId != "" {
@@ -5289,6 +5328,11 @@ func (p *Posix) GetObjectLegalHold(_ context.Context, bucket, object, versionId
 		}
 	}
 
+	err = p.ensureNotDeleteMarker(bucket, object, versionId)
+	if err != nil {
+		return nil, err
+	}
+
 	data, err := p.meta.RetrieveAttribute(nil, bucket, object, objectLegalHoldKey)
 	if errors.Is(err, fs.ErrNotExist) || errors.Is(err, syscall.ENOTDIR) {
 		if versionId != "" {
@@ -5340,6 +5384,11 @@ func (p *Posix) PutObjectRetention(_ context.Context, bucket, object, versionId
 		}
 	}
 
+	err = p.ensureNotDeleteMarker(bucket, object, versionId)
+	if err != nil {
+		return err
+	}
+
 	err = p.meta.StoreAttribute(nil, bucket, object, objectRetentionKey, retention)
 	if err != nil {
 		return fmt.Errorf("set object lock config: %w", err)
@@ -5380,6 +5429,11 @@ func (p *Posix) GetObjectRetention(_ context.Context, bucket, object, versionId
 		}
 	}
 
+	err = p.ensureNotDeleteMarker(bucket, object, versionId)
+	if err != nil {
+		return nil, err
+	}
+
 	data, err := p.meta.RetrieveAttribute(nil, bucket, object, objectRetentionKey)
 	if errors.Is(err, fs.ErrNotExist) || errors.Is(err, syscall.ENOTDIR) {
 		if versionId != "" {

tests/integration/group-tests.go

@@ -1024,6 +1024,7 @@ func TestVersioning(ts *TestState) {
 	// object tagging actions
 	ts.Run(Versioning_PutObjectTagging_invalid_versionId)
 	ts.Run(Versioning_PutObjectTagging_non_existing_object_version)
+	ts.Run(Versioning_PutGetDeleteObjectTagging_delete_marker)
 	ts.Run(Versioning_GetObjectTagging_invalid_versionId)
 	ts.Run(Versioning_GetObjectTagging_non_existing_object_version)
 	ts.Run(Versioning_DeleteObjectTagging_invalid_versionId)
@@ -1068,12 +1069,14 @@ func TestVersioning(ts *TestState) {
 	ts.Run(Versioning_PutObjectRetention_non_existing_object_version)
 	ts.Run(Versioning_GetObjectRetention_invalid_versionId)
 	ts.Run(Versioning_GetObjectRetention_non_existing_object_version)
+	ts.Run(Versioning_Put_GetObjectRetention_delete_marker)
 	ts.Run(Versioning_Put_GetObjectRetention_success)
 	// Object-Lock Legal hold
 	ts.Run(Versioning_PutObjectLegalHold_invalid_versionId)
 	ts.Run(Versioning_PutObjectLegalHold_non_existing_object_version)
 	ts.Run(Versioning_GetObjectLegalHold_invalid_versionId)
 	ts.Run(Versioning_GetObjectLegalHold_non_existing_object_version)
+	ts.Run(Versioning_PutGetObjectLegalHold_delete_marker)
 	ts.Run(Versioning_Put_GetObjectLegalHold_success)
 	// WORM protection
 	ts.Run(Versioning_WORM_obj_version_locked_with_legal_hold)
@@ -1085,6 +1088,7 @@ func TestVersioning(ts *TestState) {
 	ts.Run(Versioning_WORM_PutObject_overwrite_locked_object)
 	ts.Run(Versioning_WORM_CopyObject_overwrite_locked_object)
 	ts.Run(Versioning_WORM_CompleteMultipartUpload_overwrite_locked_object)
+	ts.Run(Versioning_WORM_remove_delete_marker_under_bucket_default_retention)
 	// Concurrent requests
 	// Versioninig_concurrent_upload_object
 	ts.Run(Versioning_AccessControl_GetObjectVersion)
@@ -1736,6 +1740,7 @@ func GetIntTests() IntTests {
 		"Versioning_GetObject_null_versionId_obj":                                  Versioning_GetObject_null_versionId_obj,
 		"Versioning_PutObjectTagging_invalid_versionId":                            Versioning_PutObjectTagging_invalid_versionId,
 		"Versioning_PutObjectTagging_non_existing_object_version":                  Versioning_PutObjectTagging_non_existing_object_version,
+		"Versioning_PutGetDeleteObjectTagging_delete_marker":                       Versioning_PutGetDeleteObjectTagging_delete_marker,
 		"Versioning_GetObjectTagging_invalid_versionId":                            Versioning_GetObjectTagging_invalid_versionId,
 		"Versioning_GetObjectTagging_non_existing_object_version":                  Versioning_GetObjectTagging_non_existing_object_version,
 		"Versioning_DeleteObjectTagging_invalid_versionId":                         Versioning_DeleteObjectTagging_invalid_versionId,
@@ -1774,11 +1779,13 @@ func GetIntTests() IntTests {
 		"Versioning_PutObjectRetention_non_existing_object_version":                Versioning_PutObjectRetention_non_existing_object_version,
 		"Versioning_GetObjectRetention_invalid_versionId":                          Versioning_GetObjectRetention_invalid_versionId,
 		"Versioning_GetObjectRetention_non_existing_object_version":                Versioning_GetObjectRetention_non_existing_object_version,
+		"Versioning_Put_GetObjectRetention_delete_marker":                          Versioning_Put_GetObjectRetention_delete_marker,
 		"Versioning_Put_GetObjectRetention_success":                                Versioning_Put_GetObjectRetention_success,
 		"Versioning_PutObjectLegalHold_invalid_versionId":                          Versioning_PutObjectLegalHold_invalid_versionId,
 		"Versioning_PutObjectLegalHold_non_existing_object_version":                Versioning_PutObjectLegalHold_non_existing_object_version,
 		"Versioning_GetObjectLegalHold_invalid_versionId":                          Versioning_GetObjectLegalHold_invalid_versionId,
 		"Versioning_GetObjectLegalHold_non_existing_object_version":                Versioning_GetObjectLegalHold_non_existing_object_version,
+		"Versioning_PutGetObjectLegalHold_delete_marker":                           Versioning_PutGetObjectLegalHold_delete_marker,
 		"Versioning_Put_GetObjectLegalHold_success":                                Versioning_Put_GetObjectLegalHold_success,
 		"Versioning_WORM_obj_version_locked_with_legal_hold":                       Versioning_WORM_obj_version_locked_with_legal_hold,
 		"Versioning_WORM_obj_version_locked_with_governance_retention":             Versioning_WORM_obj_version_locked_with_governance_retention,
@@ -1789,6 +1796,7 @@ func GetIntTests() IntTests {
 		"Versioning_WORM_PutObject_overwrite_locked_object":                        Versioning_WORM_PutObject_overwrite_locked_object,
 		"Versioning_WORM_CopyObject_overwrite_locked_object":                       Versioning_WORM_CopyObject_overwrite_locked_object,
 		"Versioning_WORM_CompleteMultipartUpload_overwrite_locked_object":          Versioning_WORM_CompleteMultipartUpload_overwrite_locked_object,
+		"Versioning_WORM_remove_delete_marker_under_bucket_default_retention":      Versioning_WORM_remove_delete_marker_under_bucket_default_retention,
 		"Versioning_AccessControl_GetObjectVersion":                                Versioning_AccessControl_GetObjectVersion,
 		"Versioning_AccessControl_HeadObjectVersion":                               Versioning_AccessControl_HeadObjectVersion,
 		"Versioning_AccessControl_object_tagging_policy":                           Versioning_AccessControl_object_tagging_policy,

tests/integration/utils.go

@@ -118,17 +118,25 @@ func teardown(s *S3Conf, bucket string) error {
 	s3client := s.GetClient()
 
 	deleteObject := func(bucket, key, versionId *string) error {
-		ctx, cancel := context.WithTimeout(context.Background(), shortTimeout)
-		_, err := s3client.DeleteObject(ctx, &s3.DeleteObjectInput{
-			Bucket:    bucket,
-			Key:       key,
-			VersionId: versionId,
-		})
-		cancel()
-		if err != nil {
-			return fmt.Errorf("failed to delete object %v: %w", *key, err)
+		var attempts int
+		var err error
+		for attempts < maxRetryAttempts {
+			ctx, cancel := context.WithTimeout(context.Background(), shortTimeout)
+			_, err = s3client.DeleteObject(ctx, &s3.DeleteObjectInput{
+				Bucket:    bucket,
+				Key:       key,
+				VersionId: versionId,
+			})
+			cancel()
+			if err == nil {
+				return nil
+			}
+
+			attempts++
+			time.Sleep(time.Second)
 		}
-		return nil
+
+		return fmt.Errorf("delete object %s: %w", *key, err)
 	}
 
 	if s.versioningEnabled {
@@ -1915,6 +1923,7 @@ func cleanupLockedObjects(client *s3.Client, bucket string, objs []objToDelete)
 			if errors.Is(err, s3err.GetAPIError(s3err.ErrNoSuchKey)) {
 				return nil
 			}
+
 			if err != nil {
 				return err
 			}
@@ -1922,29 +1931,8 @@ func cleanupLockedObjects(client *s3.Client, bucket string, objs []objToDelete)
 			// Wait until retention lock expires before attempting delete
 			time.Sleep(lockWaitTime)
 
-			// Attempt deletion with retries
-			attempts := 0
-			for attempts != maxRetryAttempts {
-				ctx, cancel := context.WithTimeout(context.Background(), shortTimeout)
-				_, err = client.DeleteObject(ctx, &s3.DeleteObjectInput{
-					Bucket:    &bucket,
-					Key:       &obj.key,
-					VersionId: getPtr(obj.versionId),
-				})
-				cancel()
-				if err != nil {
-					// Retry after a short delay if delete fails
-					time.Sleep(time.Second)
-					attempts++
-					continue
-				}
-
-				// Success, no more retries needed
-				return nil
-			}
-
 			// Return last error if all retries failed
-			return err
+			return nil
 		})
 	}
 

tests/integration/versioning.go

@@ -2176,6 +2176,57 @@ func Versioning_GetObjectRetention_non_existing_object_version(s *S3Conf) error
 	}, withLock(), withVersioning(types.BucketVersioningStatusEnabled))
 }
 
+func Versioning_Put_GetObjectRetention_delete_marker(s *S3Conf) error {
+	testName := "Versioning_Put_GetObjectRetention_delete_marker"
+	return actionHandler(s, testName, func(s3client *s3.Client, bucket string) error {
+		obj := "my-object"
+		_, err := putObjectWithData(10, &s3.PutObjectInput{
+			Bucket: &bucket,
+			Key:    &obj,
+		}, s3client)
+		if err != nil {
+			return err
+		}
+
+		ctx, cancel := context.WithTimeout(context.Background(), shortTimeout)
+		out, err := s3client.DeleteObject(ctx, &s3.DeleteObjectInput{
+			Bucket: &bucket,
+			Key:    &obj,
+		})
+		cancel()
+		if err != nil {
+			return err
+		}
+
+		// PutObjectRetention
+		ctx, cancel = context.WithTimeout(context.Background(), shortTimeout)
+		_, err = s3client.PutObjectRetention(ctx, &s3.PutObjectRetentionInput{
+			Bucket:    &bucket,
+			Key:       &obj,
+			VersionId: out.VersionId,
+			Retention: &types.ObjectLockRetention{
+				Mode:            types.ObjectLockRetentionModeCompliance,
+				RetainUntilDate: getPtr(time.Now().AddDate(1, 0, 0)),
+			},
+		})
+		cancel()
+		if err := checkApiErr(err, s3err.GetAPIError(s3err.ErrMethodNotAllowed)); err != nil {
+			return err
+		}
+
+		// GetObjectRetention
+		ctx, cancel = context.WithTimeout(context.Background(), shortTimeout)
+		_, err = s3client.GetObjectRetention(ctx, &s3.GetObjectRetentionInput{
+			Bucket:    &bucket,
+			Key:       &obj,
+			VersionId: out.VersionId,
+		})
+		cancel()
+
+		return checkApiErr(err, s3err.GetAPIError(s3err.ErrMethodNotAllowed))
+	}, withLock())
+}
+
 func Versioning_Put_GetObjectRetention_success(s *S3Conf) error {
 	testName := "Versioning_Put_GetObjectRetention_success"
 	return actionHandler(s, testName, func(s3client *s3.Client, bucket string) error {
@@ -2316,6 +2367,56 @@ func Versioning_GetObjectLegalHold_non_existing_object_version(s *S3Conf) error
 	}, withLock(), withVersioning(types.BucketVersioningStatusEnabled))
 }
 
+func Versioning_PutGetObjectLegalHold_delete_marker(s *S3Conf) error {
+	testName := "Versioning_PutGetObjectLegalHold_delete_marker"
+	return actionHandler(s, testName, func(s3client *s3.Client, bucket string) error {
+		obj := "my-object"
+		_, err := putObjectWithData(10, &s3.PutObjectInput{
+			Bucket: &bucket,
+			Key:    &obj,
+		}, s3client)
+		if err != nil {
+			return err
+		}
+
+		ctx, cancel := context.WithTimeout(context.Background(), shortTimeout)
+		out, err := s3client.DeleteObject(ctx, &s3.DeleteObjectInput{
+			Bucket: &bucket,
+			Key:    &obj,
+		})
+		cancel()
+		if err != nil {
+			return err
+		}
+
+		// PutObjectLegalHold
+		ctx, cancel = context.WithTimeout(context.Background(), shortTimeout)
+		_, err = s3client.PutObjectLegalHold(ctx, &s3.PutObjectLegalHoldInput{
+			Bucket:    &bucket,
+			Key:       &obj,
+			VersionId: out.VersionId,
+			LegalHold: &types.ObjectLockLegalHold{
+				Status: types.ObjectLockLegalHoldStatusOn,
+			},
+		})
+		cancel()
+		if err := checkApiErr(err, s3err.GetAPIError(s3err.ErrMethodNotAllowed)); err != nil {
+			return err
+		}
+
+		// GetObjectLegalHold
+		ctx, cancel = context.WithTimeout(context.Background(), shortTimeout)
+		_, err = s3client.GetObjectLegalHold(ctx, &s3.GetObjectLegalHoldInput{
+			Bucket:    &bucket,
+			Key:       &obj,
+			VersionId: out.VersionId,
+		})
+		cancel()
+
+		return checkApiErr(err, s3err.GetAPIError(s3err.ErrMethodNotAllowed))
+	}, withLock())
+}
+
 func Versioning_Put_GetObjectLegalHold_success(s *S3Conf) error {
 	testName := "Versioning_Put_GetObjectLegalHold_success"
 	return actionHandler(s, testName, func(s3client *s3.Client, bucket string) error {
@@ -2957,6 +3058,85 @@ func Versioning_WORM_CompleteMultipartUpload_overwrite_locked_object(s *S3Conf)
 	}, withLock())
 }
 
+func Versioning_WORM_remove_delete_marker_under_bucket_default_retention(s *S3Conf) error {
+	testName := "Versioning_WORM_remove_delete_marker_under_bucket_default_retention"
+	return actionHandler(s, testName, func(s3client *s3.Client, bucket string) error {
+		ctx, cancel := context.WithTimeout(context.Background(), shortTimeout)
+		_, err := s3client.PutObjectLockConfiguration(ctx, &s3.PutObjectLockConfigurationInput{
+			Bucket: &bucket,
+			ObjectLockConfiguration: &types.ObjectLockConfiguration{
+				ObjectLockEnabled: types.ObjectLockEnabledEnabled,
+				Rule: &types.ObjectLockRule{
+					DefaultRetention: &types.DefaultRetention{
+						Mode: types.ObjectLockRetentionModeGovernance,
+						Days: getPtr(int32(5)),
+					},
+				},
+			},
+		})
+		cancel()
+		if err != nil {
+			return err
+		}
+
+		obj := "my-object"
+		versions, err := createObjVersions(s3client, bucket, obj, 3)
+		if err != nil {
+			return err
+		}
+
+		// Create a delete marker
+		ctx, cancel = context.WithTimeout(context.Background(), shortTimeout)
+		out, err := s3client.DeleteObject(ctx, &s3.DeleteObjectInput{
+			Bucket: &bucket,
+			Key:    &obj,
+		})
+		cancel()
+		if err != nil {
+			return err
+		}
+
+		// Delete the delete marker
+		ctx, cancel = context.WithTimeout(context.Background(), shortTimeout)
+		_, err = s3client.DeleteObject(ctx, &s3.DeleteObjectInput{
+			Bucket:    &bucket,
+			Key:       &obj,
+			VersionId: out.VersionId,
+		})
+		cancel()
+		if err != nil {
+			return err
+		}
+
+		ctx, cancel = context.WithTimeout(context.Background(), shortTimeout)
+		resp, err := s3client.ListObjectVersions(ctx, &s3.ListObjectVersionsInput{
+			Bucket: &bucket,
+		})
+		cancel()
+		if err != nil {
+			return err
+		}
+
+		if !compareVersions(versions, resp.Versions) {
+			return fmt.Errorf("expected the object vresions to be %v, instead got %v", versions, resp.Versions)
+		}
+		if len(resp.DeleteMarkers) != 0 {
+			return fmt.Errorf("expected empty delete markers list, instead got %v", resp.DeleteMarkers)
+		}
+
+		//
+		lockedVersions := make([]objToDelete, 0, len(versions))
+		for _, v := range versions {
+			lockedVersions = append(lockedVersions, objToDelete{
+				key:          obj,
+				versionId:    getString(v.VersionId),
+				isCompliance: false,
+			})
+		}
+		return cleanupLockedObjects(s3client, bucket, lockedVersions)
+	}, withLock())
+}
+
 func Versioning_AccessControl_GetObjectVersion(s *S3Conf) error {
 	testName := "Versioning_AccessControl_GetObjectVersion"
 	return actionHandler(s, testName, func(s3client *s3.Client, bucket string) error {
@@ -3516,6 +3696,68 @@ func Versioning_PutObjectTagging_non_existing_object_version(s *S3Conf) error {
 	}, withVersioning(types.BucketVersioningStatusEnabled))
 }
 
+func Versioning_PutGetDeleteObjectTagging_delete_marker(s *S3Conf) error {
+	testName := "Versioning_PutGetDeleteObjectTagging_delete_marker"
+	return actionHandler(s, testName, func(s3client *s3.Client, bucket string) error {
+		obj := "my-object"
+		_, err := putObjectWithData(10, &s3.PutObjectInput{
+			Bucket: &bucket,
+			Key:    &obj,
+		}, s3client)
+		if err != nil {
+			return err
+		}
+
+		ctx, cancel := context.WithTimeout(context.Background(), shortTimeout)
+		out, err := s3client.DeleteObject(ctx, &s3.DeleteObjectInput{
+			Bucket: &bucket,
+			Key:    &obj,
+		})
+		cancel()
+		if err != nil {
+			return err
+		}
+
+		// PutObjectTagging
+		ctx, cancel = context.WithTimeout(context.Background(), shortTimeout)
+		_, err = s3client.PutObjectTagging(ctx, &s3.PutObjectTaggingInput{
+			Bucket:    &bucket,
+			Key:       &obj,
+			VersionId: out.VersionId,
+			Tagging: &types.Tagging{
+				TagSet: []types.Tag{{Key: getPtr("key"), Value: getPtr("value")}},
+			},
+		})
+		cancel()
+		if err := checkApiErr(err, s3err.GetAPIError(s3err.ErrMethodNotAllowed)); err != nil {
+			return err
+		}
+
+		// GetObjectTagging
+		ctx, cancel = context.WithTimeout(context.Background(), shortTimeout)
+		_, err = s3client.GetObjectTagging(ctx, &s3.GetObjectTaggingInput{
+			Bucket:    &bucket,
+			Key:       &obj,
+			VersionId: out.VersionId,
+		})
+		cancel()
+		if err := checkApiErr(err, s3err.GetAPIError(s3err.ErrMethodNotAllowed)); err != nil {
+			return err
+		}
+
+		// DeleteObjectTagging
+		ctx, cancel = context.WithTimeout(context.Background(), shortTimeout)
+		_, err = s3client.DeleteObjectTagging(ctx, &s3.DeleteObjectTaggingInput{
+			Bucket:    &bucket,
+			Key:       &obj,
+			VersionId: out.VersionId,
+		})
+		cancel()
+
+		return checkApiErr(err, s3err.GetAPIError(s3err.ErrMethodNotAllowed))
+	}, withLock())
+}
+
 func Versioning_PutObjectTagging_invalid_versionId(s *S3Conf) error {
 	testName := "Versioning_PutObjectTagging_invalid_versionId"
 	return actionHandler(s, testName, func(s3client *s3.Client, bucket string) error {