From 5c084b8452a04a3a13cbaaa3122c07268e404877 Mon Sep 17 00:00:00 2001 From: niksis02 Date: Thu, 9 Oct 2025 01:01:18 +0400 Subject: [PATCH 1/2] fix: fixes locked objects overwrite in versioning-enabled buckets Fixes #1574 When versioning is enabled at the gateway level and object lock is enabled for a bucket, any overwrite request on a locked object should succeed since it results in the creation of a new object version. This PR fixes the logic by adding a bucket versioning status check in `CheckObjectAccess`. --- auth/object_lock.go | 11 +- s3api/controllers/bucket-post.go | 2 +- s3api/controllers/object-delete.go | 1 + s3api/controllers/object-put.go | 2 +- s3api/controllers/object-put_test.go | 3 + tests/integration/group-tests.go | 6 + tests/integration/tests.go | 249 +++++++++++++++++++++++++++ 7 files changed, 271 insertions(+), 3 deletions(-) diff --git a/auth/object_lock.go b/auth/object_lock.go index a46aed03..7e1a834e 100644 --- a/auth/object_lock.go +++ b/auth/object_lock.go @@ -210,7 +210,16 @@ func ParseObjectLegalHoldOutput(status *bool) *s3response.GetObjectLegalHoldResu } } -func CheckObjectAccess(ctx context.Context, bucket, userAccess string, objects []types.ObjectIdentifier, bypass, isBucketPublic bool, be backend.Backend) error { +func CheckObjectAccess(ctx context.Context, bucket, userAccess string, objects []types.ObjectIdentifier, bypass, isBucketPublic bool, be backend.Backend, isOverwrite bool) error { + if isOverwrite { + // if bucket versioning is enabled, any overwrite request + // should be enabled, as it leads to a new object version + // creation + res, err := be.GetBucketVersioning(ctx, bucket) + if err == nil && res.Status != nil && *res.Status == types.BucketVersioningStatusEnabled { + return nil + } + } data, err := be.GetObjectLockConfiguration(ctx, bucket) if err != nil { if errors.Is(err, s3err.GetAPIError(s3err.ErrObjectLockConfigurationNotFound)) { diff --git a/s3api/controllers/bucket-post.go b/s3api/controllers/bucket-post.go index bc692f48..be3d3bd3 100644 --- a/s3api/controllers/bucket-post.go +++ b/s3api/controllers/bucket-post.go @@ -67,7 +67,7 @@ func (c S3ApiController) DeleteObjects(ctx *fiber.Ctx) (*Response, error) { }, s3err.GetAPIError(s3err.ErrInvalidRequest) } - err = auth.CheckObjectAccess(ctx.Context(), bucket, acct.Access, dObj.Objects, bypass, IsBucketPublic, c.be) + err = auth.CheckObjectAccess(ctx.Context(), bucket, acct.Access, dObj.Objects, bypass, IsBucketPublic, c.be, false) if err != nil { return &Response{ MetaOpts: &MetaOptions{ diff --git a/s3api/controllers/object-delete.go b/s3api/controllers/object-delete.go index bfb5367d..4e41bd5c 100644 --- a/s3api/controllers/object-delete.go +++ b/s3api/controllers/object-delete.go @@ -159,6 +159,7 @@ func (c S3ApiController) DeleteObject(ctx *fiber.Ctx) (*Response, error) { bypass, isBucketPublic, c.be, + false, ) if err != nil { return &Response{ diff --git a/s3api/controllers/object-put.go b/s3api/controllers/object-put.go index af5bd1ce..55afcff7 100644 --- a/s3api/controllers/object-put.go +++ b/s3api/controllers/object-put.go @@ -629,7 +629,7 @@ func (c S3ApiController) PutObject(ctx *fiber.Ctx) (*Response, error) { }, err } - err = auth.CheckObjectAccess(ctx.Context(), bucket, acct.Access, []types.ObjectIdentifier{{Key: &key}}, true, IsBucketPublic, c.be) + err = auth.CheckObjectAccess(ctx.Context(), bucket, acct.Access, []types.ObjectIdentifier{{Key: &key}}, true, IsBucketPublic, c.be, true) if err != nil { return &Response{ MetaOpts: &MetaOptions{ diff --git a/s3api/controllers/object-put_test.go b/s3api/controllers/object-put_test.go index f586818f..928062a3 100644 --- a/s3api/controllers/object-put_test.go +++ b/s3api/controllers/object-put_test.go @@ -1174,6 +1174,9 @@ func TestS3ApiController_PutObject(t *testing.T) { GetObjectLockConfigurationFunc: func(contextMoqParam context.Context, bucket string) ([]byte, error) { return nil, tt.input.extraMockErr }, + GetBucketVersioningFunc: func(contextMoqParam context.Context, bucket string) (s3response.GetBucketVersioningOutput, error) { + return s3response.GetBucketVersioningOutput{}, s3err.GetAPIError(s3err.ErrNotImplemented) + }, } ctrl := S3ApiController{ diff --git a/tests/integration/group-tests.go b/tests/integration/group-tests.go index 2871feb2..a3d39218 100644 --- a/tests/integration/group-tests.go +++ b/tests/integration/group-tests.go @@ -1017,6 +1017,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_PutObject_overwrite_locked_object) + ts.Run(Versioning_WORM_CopyObject_overwrite_locked_object) + ts.Run(Versioning_WORM_CompleteMultipartUpload_overwrite_locked_object) // Concurrent requests // Versioninig_concurrent_upload_object ts.Run(Versioning_AccessControl_GetObjectVersion) @@ -1612,6 +1615,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_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_AccessControl_GetObjectVersion": Versioning_AccessControl_GetObjectVersion, "Versioning_AccessControl_HeadObjectVersion": Versioning_AccessControl_HeadObjectVersion, "Versioning_concurrent_upload_object": Versioning_concurrent_upload_object, diff --git a/tests/integration/tests.go b/tests/integration/tests.go index 3e88d76f..823a664c 100644 --- a/tests/integration/tests.go +++ b/tests/integration/tests.go @@ -24060,6 +24060,255 @@ func Versioning_WORM_obj_version_locked_with_compliance_retention(s *S3Conf) err }, withLock(), withVersioning(types.BucketVersioningStatusEnabled)) } +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 { + obj := "my-obj" + versions, err := createObjVersions(s3client, bucket, obj, 1) + if err != nil { + return err + } + + v := versions[0] + v.IsLatest = getPtr(false) + + // lock the object with legal hold + 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 + } + + dataLen := int64(10) + + // overwrite the locked object with a new version + r, err := putObjectWithData(dataLen, &s3.PutObjectInput{ + Bucket: &bucket, + Key: &obj, + }, s3client) + if err != nil { + return err + } + + version := types.ObjectVersion{ + ETag: r.res.ETag, + IsLatest: getPtr(true), + Key: &obj, + Size: &dataLen, + VersionId: r.res.VersionId, + StorageClass: types.ObjectVersionStorageClassStandard, + ChecksumType: r.res.ChecksumType, + } + + result := []types.ObjectVersion{version, v} + + ctx, cancel = context.WithTimeout(context.Background(), shortTimeout) + out, err := s3client.ListObjectVersions(ctx, &s3.ListObjectVersionsInput{ + Bucket: &bucket, + }) + cancel() + if err != nil { + return err + } + + if !compareVersions(result, out.Versions) { + return fmt.Errorf("expected the object versions to be %v, instead got %v", result, out.Versions) + } + + return cleanupLockedObjects(s3client, bucket, []objToDelete{ + { + key: obj, + versionId: getString(v.VersionId), + removeOnlyLeglHold: true, + }, + }) + }, withLock()) +} + +func Versioning_WORM_CopyObject_overwrite_locked_object(s *S3Conf) error { + testName := "Versioning_WORM_CopyObject_overwrite_locked_object" + return actionHandler(s, testName, func(s3client *s3.Client, bucket string) error { + obj := "my-obj" + versions, err := createObjVersions(s3client, bucket, obj, 1) + if err != nil { + return err + } + + v := versions[0] + v.IsLatest = getPtr(false) + + // lock the object with legal hold + 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 + } + + // create a source object version + srcObj := "source-object" + srcVersions, err := createObjVersions(s3client, bucket, srcObj, 1) + if err != nil { + return err + } + + srcVersion := srcVersions[0] + + // overwrite the locked object with a new version with CopyObject + ctx, cancel = context.WithTimeout(context.Background(), shortTimeout) + copyResult, err := s3client.CopyObject(ctx, &s3.CopyObjectInput{ + Bucket: &bucket, + Key: &obj, + CopySource: getPtr(fmt.Sprintf("%s/%s", bucket, srcObj)), + }) + cancel() + if err != nil { + return err + } + + version := types.ObjectVersion{ + ETag: copyResult.CopyObjectResult.ETag, + IsLatest: getPtr(true), + Key: &obj, + Size: srcVersion.Size, + VersionId: copyResult.VersionId, + StorageClass: types.ObjectVersionStorageClassStandard, + ChecksumType: copyResult.CopyObjectResult.ChecksumType, + } + + result := []types.ObjectVersion{version, v, srcVersion} + + ctx, cancel = context.WithTimeout(context.Background(), shortTimeout) + out, err := s3client.ListObjectVersions(ctx, &s3.ListObjectVersionsInput{ + Bucket: &bucket, + }) + cancel() + if err != nil { + return err + } + + if !compareVersions(result, out.Versions) { + return fmt.Errorf("expected the object versions to be %v, instead got %v", result, out.Versions) + } + + return cleanupLockedObjects(s3client, bucket, []objToDelete{ + { + key: obj, + versionId: getString(v.VersionId), + removeOnlyLeglHold: true, + }, + }) + }, withLock()) +} + +func Versioning_WORM_CompleteMultipartUpload_overwrite_locked_object(s *S3Conf) error { + testName := "Versioning_WORM_CompleteMultipartUpload_overwrite_locked_object" + return actionHandler(s, testName, func(s3client *s3.Client, bucket string) error { + obj := "my-obj" + versions, err := createObjVersions(s3client, bucket, obj, 1) + if err != nil { + return err + } + + v := versions[0] + v.IsLatest = getPtr(false) + + // lock the object with legal hold + 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 + } + + dataLen := int64(5 * 1024 * 1024) + + // overwrite the locked object with a new version + mp, err := createMp(s3client, bucket, obj) + if err != nil { + return err + } + + parts, _, err := uploadParts(s3client, dataLen, 1, bucket, obj, *mp.UploadId) + if err != nil { + return err + } + part := parts[0] + + ctx, cancel = context.WithTimeout(context.Background(), shortTimeout) + res, err := s3client.CompleteMultipartUpload(ctx, &s3.CompleteMultipartUploadInput{ + Bucket: &bucket, + Key: &obj, + MultipartUpload: &types.CompletedMultipartUpload{ + Parts: []types.CompletedPart{ + { + ETag: part.ETag, + PartNumber: part.PartNumber, + ChecksumCRC64NVME: part.ChecksumCRC64NVME, + }, + }, + }, + UploadId: mp.UploadId, + }) + cancel() + if err != nil { + return err + } + + version := types.ObjectVersion{ + ETag: res.ETag, + IsLatest: getPtr(true), + Key: &obj, + Size: &dataLen, + VersionId: res.VersionId, + StorageClass: types.ObjectVersionStorageClassStandard, + ChecksumType: res.ChecksumType, + } + + result := []types.ObjectVersion{version, v} + + ctx, cancel = context.WithTimeout(context.Background(), shortTimeout) + out, err := s3client.ListObjectVersions(ctx, &s3.ListObjectVersionsInput{ + Bucket: &bucket, + }) + cancel() + if err != nil { + return err + } + + if !compareVersions(result, out.Versions) { + return fmt.Errorf("expected the object versions to be %v, instead got %v", result, out.Versions) + } + + return cleanupLockedObjects(s3client, bucket, []objToDelete{ + { + key: obj, + versionId: getString(v.VersionId), + removeOnlyLeglHold: true, + }, + }) + }, withLock()) +} + func Versioning_AccessControl_GetObjectVersion(s *S3Conf) error { testName := "Versioning_AccessControl_GetObjectVersion" return actionHandler(s, testName, func(s3client *s3.Client, bucket string) error { From c158dfeb0d96dc87b6f113f7d2cf66a9a1740a97 Mon Sep 17 00:00:00 2001 From: Luke McCrone Date: Wed, 8 Oct 2025 18:52:04 -0300 Subject: [PATCH 2/2] test: temp fixes for legal-hold/retention tests --- tests/test_s3api_root_inner.sh | 15 ++++----------- 1 file changed, 4 insertions(+), 11 deletions(-) diff --git a/tests/test_s3api_root_inner.sh b/tests/test_s3api_root_inner.sh index 25bf899c..1f51e7ee 100755 --- a/tests/test_s3api_root_inner.sh +++ b/tests/test_s3api_root_inner.sh @@ -127,13 +127,10 @@ test_get_put_object_legal_hold_s3api_root() { echo "fdkljafajkfs" > "$TEST_FILE_FOLDER/$bucket_file" run put_object_with_user "s3api" "$TEST_FILE_FOLDER/$bucket_file" "$BUCKET_ONE_NAME" "$bucket_file" "$username" "$password" - assert_failure - assert_output --partial "object protected by object lock" + assert_success run delete_object_with_user "s3api" "$BUCKET_ONE_NAME" "$bucket_file" "$username" "$password" - assert_failure 1 - # shellcheck disable=SC2154 - assert_output --partial "object protected by object lock" + assert_success run put_object_legal_hold "s3api" "$BUCKET_ONE_NAME" "$bucket_file" "OFF" assert_success @@ -171,14 +168,10 @@ test_get_put_object_retention_s3api_root() { echo "fdkljafajkfs" > "$TEST_FILE_FOLDER/$bucket_file" run put_object_with_user "s3api" "$TEST_FILE_FOLDER/$bucket_file" "$BUCKET_ONE_NAME" "$bucket_file" "$username" "$secret_key" - assert_failure - # shellcheck disable=SC2154 - assert_output --partial "object protected by object lock" + assert_success run delete_object_with_user "s3api" "$BUCKET_ONE_NAME" "$bucket_file" "$username" "$secret_key" - assert_failure - # shellcheck disable=SC2154 - assert_output --partial "object protected by object lock" + assert_success } test_retention_bypass_s3api_root() {