From 06f4f0ac159bc73be61ca1baa9cc891931d77756 Mon Sep 17 00:00:00 2001 From: niksis02 Date: Tue, 13 Jan 2026 16:50:54 +0400 Subject: [PATCH] fix: skips object lock check in DeleteObject without versionId. Fixes #1741 An object delete request without a `versionId` results in the creation of a new delete marker in versioning-enabled buckets. Even if the latest object version is locked, a new delete marker must still be created. This implementation skips the object lock check for delete requests in versioning-enabled buckets when the `versionId` is missing, allowing the delete marker to be created as expected. Additionally, it introduces a flag in the `createObjVersion` method in POSIX to remove unnecessary xattr attributes from an object after creating a new object version. A delete marker must not carry object-specific attributes such as tagging, legal hold, or retention. Currently, the cleanup is limited to legal hold and retention attributes, but this list will be expanded after fixing issue #1751. --- auth/object_lock.go | 11 ++ backend/posix/posix.go | 25 +++- tests/integration/group-tests.go | 6 + tests/integration/versioning.go | 206 +++++++++++++++++++++++++++++++ 4 files changed, 244 insertions(+), 4 deletions(-) diff --git a/auth/object_lock.go b/auth/object_lock.go index 7e1a834e..5ff0432d 100644 --- a/auth/object_lock.go +++ b/auth/object_lock.go @@ -254,6 +254,12 @@ func CheckObjectAccess(ctx context.Context, bucket, userAccess string, objects [ } } + var versioningEnabled bool + vers, err := be.GetBucketVersioning(ctx, bucket) + if err == nil && vers.Status != nil { + versioningEnabled = *vers.Status == types.BucketVersioningStatusEnabled + } + for _, obj := range objects { var key, versionId string if obj.Key != nil { @@ -262,6 +268,11 @@ func CheckObjectAccess(ctx context.Context, bucket, userAccess string, objects [ if obj.VersionId != nil { versionId = *obj.VersionId } + // if bucket versioning is enabled and versionId isn't provided + // no lock check is needed, as it leads to a new delete marker creation + if versioningEnabled && versionId == "" { + continue + } checkRetention := true retentionData, err := be.GetObjectRetention(ctx, bucket, key, versionId) if errors.Is(err, s3err.GetAPIError(s3err.ErrNoSuchKey)) { diff --git a/backend/posix/posix.go b/backend/posix/posix.go index 060dac7c..ed5933c0 100644 --- a/backend/posix/posix.go +++ b/backend/posix/posix.go @@ -728,8 +728,17 @@ func (p *Posix) deleteNullVersionIdObject(bucket, key string) error { return err } +func isRemovableAttr(attr string) bool { + switch attr { + case objectLegalHoldKey, objectRetentionKey: + return true + default: + return false + } +} + // Creates a new copy(version) of an object in the versioning directory -func (p *Posix) createObjVersion(bucket, key string, size int64, acc auth.Account) (versionPath string, err error) { +func (p *Posix) createObjVersion(bucket, key string, size int64, acc auth.Account, removeAttributes bool) (versionPath string, err error) { sf, err := os.Open(filepath.Join(bucket, key)) if err != nil { return "", err @@ -785,6 +794,14 @@ func (p *Posix) createObjVersion(bucket, key string, size int64, acc auth.Accoun if err != nil { return versionPath, fmt.Errorf("store %v attribute: %w", attr, err) } + + // remove object lock attributes in delete marker + if removeAttributes && isRemovableAttr(attr) { + err := p.meta.DeleteAttribute(bucket, key, attr) + if err != nil { + return versionPath, fmt.Errorf("remove %s attribute: %w", attr, err) + } + } } if err := f.link(); err != nil { @@ -1699,7 +1716,7 @@ func (p *Posix) CompleteMultipartUploadWithCopy(ctx context.Context, input *s3.C // if the versioninng is enabled first create the file object version if p.versioningEnabled() && vEnabled && err == nil && !d.IsDir() { - _, err := p.createObjVersion(bucket, object, d.Size(), acct) + _, err := p.createObjVersion(bucket, object, d.Size(), acct, false) if err != nil { return res, "", fmt.Errorf("create object version: %w", err) } @@ -2995,7 +3012,7 @@ func (p *Posix) PutObjectWithPostFunc(ctx context.Context, po s3response.PutObje isVersionIdMissing = len(vIdBytes) == 0 } if !isVersionIdMissing { - _, err := p.createObjVersion(*po.Bucket, *po.Key, d.Size(), acct) + _, err := p.createObjVersion(*po.Bucket, *po.Key, d.Size(), acct, false) if err != nil { return s3response.PutObjectOutput{}, fmt.Errorf("create object version: %w", err) } @@ -3343,7 +3360,7 @@ func (p *Posix) DeleteObject(ctx context.Context, input *s3.DeleteObjectInput) ( // Creates a new object version in the versioning directory if p.isBucketVersioningEnabled(vStatus) || string(vId) != nullVersionId { - _, err = p.createObjVersion(bucket, object, fi.Size(), acct) + _, err = p.createObjVersion(bucket, object, fi.Size(), acct, true) if err != nil { return nil, err } diff --git a/tests/integration/group-tests.go b/tests/integration/group-tests.go index 92cef368..f5c6c541 100644 --- a/tests/integration/group-tests.go +++ b/tests/integration/group-tests.go @@ -1076,6 +1076,9 @@ func TestVersioning(ts *TestState) { ts.Run(Versioning_WORM_obj_version_locked_with_legal_hold) ts.Run(Versioning_WORM_obj_version_locked_with_governance_retention) ts.Run(Versioning_WORM_obj_version_locked_with_compliance_retention) + ts.Run(Versioning_WORM_delete_marker_locked_object_legal_hold) + ts.Run(Versioning_WORM_delete_marker_locked_object_governance_retention) + ts.Run(Versioning_WORM_delete_marker_locked_object_compliance_retention) ts.Run(Versioning_WORM_PutObject_overwrite_locked_object) ts.Run(Versioning_WORM_CopyObject_overwrite_locked_object) ts.Run(Versioning_WORM_CompleteMultipartUpload_overwrite_locked_object) @@ -1774,6 +1777,9 @@ func GetIntTests() IntTests { "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, "Versioning_WORM_obj_version_locked_with_compliance_retention": Versioning_WORM_obj_version_locked_with_compliance_retention, + "Versioning_WORM_delete_marker_locked_object_legal_hold": Versioning_WORM_delete_marker_locked_object_legal_hold, + "Versioning_WORM_delete_marker_locked_object_governance_retention": Versioning_WORM_delete_marker_locked_object_governance_retention, + "Versioning_WORM_delete_marker_locked_object_compliance_retention": Versioning_WORM_delete_marker_locked_object_compliance_retention, "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, diff --git a/tests/integration/versioning.go b/tests/integration/versioning.go index 261d8d8c..8fdfcb19 100644 --- a/tests/integration/versioning.go +++ b/tests/integration/versioning.go @@ -2445,6 +2445,212 @@ func Versioning_WORM_obj_version_locked_with_compliance_retention(s *S3Conf) err }, withLock(), withVersioning(types.BucketVersioningStatusEnabled)) } +func Versioning_WORM_delete_marker_locked_object_legal_hold(s *S3Conf) error { + testName := "Versioning_WORM_delete_marker_locked_object_legal_hold" + return actionHandler(s, testName, func(s3client *s3.Client, bucket string) error { + obj := "my-obj" + objVersions, err := createObjVersions(s3client, bucket, obj, 1) + if err != nil { + return err + } + version := objVersions[0] + objVersions[0].IsLatest = getPtr(false) + + ctx, cancel := context.WithTimeout(context.Background(), shortTimeout) + _, err = s3client.PutObjectLegalHold(ctx, &s3.PutObjectLegalHoldInput{ + Bucket: &bucket, + Key: &obj, + LegalHold: &types.ObjectLockLegalHold{ + Status: types.ObjectLockLegalHoldStatusOn, + }, + }) + cancel() + 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 + } + + delMarkers := []types.DeleteMarkerEntry{ + { + IsLatest: getPtr(true), + Key: &obj, + VersionId: out.VersionId, + }, + } + + ctx, cancel = context.WithTimeout(context.Background(), shortTimeout) + resp, err := s3client.ListObjectVersions(ctx, &s3.ListObjectVersionsInput{ + Bucket: &bucket, + }) + cancel() + if err != nil { + return err + } + + if !compareVersions(objVersions, resp.Versions) { + return fmt.Errorf("expected the object versions to be %v, instead got %v", objVersions, resp.Versions) + } + if !compareDelMarkers(delMarkers, resp.DeleteMarkers) { + return fmt.Errorf("expected the object delete markers to be %v, instead got %v", delMarkers, resp.DeleteMarkers) + } + + return cleanupLockedObjects(s3client, bucket, []objToDelete{ + { + key: obj, + versionId: getString(version.VersionId), + removeOnlyLeglHold: true, + }, + }) + }, withLock()) +} + +func Versioning_WORM_delete_marker_locked_object_governance_retention(s *S3Conf) error { + testName := "Versioning_WORM_delete_marker_locked_object_governance_retention" + return actionHandler(s, testName, func(s3client *s3.Client, bucket string) error { + obj := "my-obj" + objVersions, err := createObjVersions(s3client, bucket, obj, 1) + if err != nil { + return err + } + version := objVersions[0] + objVersions[0].IsLatest = getPtr(false) + + ctx, cancel := context.WithTimeout(context.Background(), shortTimeout) + _, err = s3client.PutObjectRetention(ctx, &s3.PutObjectRetentionInput{ + Bucket: &bucket, + Key: &obj, + Retention: &types.ObjectLockRetention{ + Mode: types.ObjectLockRetentionModeGovernance, + RetainUntilDate: getPtr(time.Now().AddDate(1, 0, 0)), + }, + }) + cancel() + 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 + } + + delMarkers := []types.DeleteMarkerEntry{ + { + IsLatest: getPtr(true), + Key: &obj, + VersionId: out.VersionId, + }, + } + + ctx, cancel = context.WithTimeout(context.Background(), shortTimeout) + resp, err := s3client.ListObjectVersions(ctx, &s3.ListObjectVersionsInput{ + Bucket: &bucket, + }) + cancel() + if err != nil { + return err + } + + if !compareVersions(objVersions, resp.Versions) { + return fmt.Errorf("expected the object versions to be %v, instead got %v", objVersions, resp.Versions) + } + if !compareDelMarkers(delMarkers, resp.DeleteMarkers) { + return fmt.Errorf("expected the object delete markers to be %v, instead got %v", delMarkers, resp.DeleteMarkers) + } + + return cleanupLockedObjects(s3client, bucket, []objToDelete{ + { + key: obj, + versionId: getString(version.VersionId), + isCompliance: false, + }, + }) + }, withLock()) +} + +func Versioning_WORM_delete_marker_locked_object_compliance_retention(s *S3Conf) error { + testName := "Versioning_WORM_delete_marker_locked_object_compliance_retention" + return actionHandler(s, testName, func(s3client *s3.Client, bucket string) error { + obj := "my-obj" + objVersions, err := createObjVersions(s3client, bucket, obj, 1) + if err != nil { + return err + } + version := objVersions[0] + objVersions[0].IsLatest = getPtr(false) + + ctx, cancel := context.WithTimeout(context.Background(), shortTimeout) + _, err = s3client.PutObjectRetention(ctx, &s3.PutObjectRetentionInput{ + Bucket: &bucket, + Key: &obj, + Retention: &types.ObjectLockRetention{ + Mode: types.ObjectLockRetentionModeCompliance, + RetainUntilDate: getPtr(time.Now().AddDate(1, 0, 0)), + }, + }) + cancel() + 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 + } + + delMarkers := []types.DeleteMarkerEntry{ + { + IsLatest: getPtr(true), + Key: &obj, + VersionId: out.VersionId, + }, + } + + ctx, cancel = context.WithTimeout(context.Background(), shortTimeout) + resp, err := s3client.ListObjectVersions(ctx, &s3.ListObjectVersionsInput{ + Bucket: &bucket, + }) + cancel() + if err != nil { + return err + } + + if !compareVersions(objVersions, resp.Versions) { + return fmt.Errorf("expected the object versions to be %v, instead got %v", objVersions, resp.Versions) + } + if !compareDelMarkers(delMarkers, resp.DeleteMarkers) { + return fmt.Errorf("expected the object delete markers to be %v, instead got %v", delMarkers, resp.DeleteMarkers) + } + + return cleanupLockedObjects(s3client, bucket, []objToDelete{ + { + key: obj, + versionId: getString(version.VersionId), + isCompliance: true, + }, + }) + }, withLock()) +} + func Versioning_WORM_PutObject_overwrite_locked_object(s *S3Conf) error { testName := "Versioning_WORM_PutObject_overwrite_locked_object" return actionHandler(s, testName, func(s3client *s3.Client, bucket string) error {