From 05f82255775e0f997a8e2776028a09d562c5a835 Mon Sep 17 00:00:00 2001 From: niksis02 Date: Wed, 12 Nov 2025 23:53:27 +0400 Subject: [PATCH] feat: adds missing versioning-related bucket policy actions MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Closes #1635 Some S3 actions have dedicated bucket policy actions and require explicit policy permissions when operating on object versions. These actions were missing in the gateway: `GetObjectVersionTagging`, `PutObjectVersionTagging`, `DeleteObjectVersionTagging`, `DeleteObjectVersion`, and `GetObjectVersionAttributes`. The logic for these actions is straightforward — if the incoming request includes the `versionId` query parameter, S3 enforces the corresponding bucket policy action that includes `version`. This PR adds support for these missing actions in the gateway. --- auth/bucket_policy_actions.go | 53 ++++-- s3api/controllers/object-delete.go | 14 +- s3api/controllers/object-get.go | 14 +- s3api/controllers/object-put.go | 7 +- tests/integration/group-tests.go | 6 + tests/integration/utils.go | 10 ++ tests/integration/versioning.go | 279 +++++++++++++++++++++++++++++ 7 files changed, 358 insertions(+), 25 deletions(-) diff --git a/auth/bucket_policy_actions.go b/auth/bucket_policy_actions.go index f1a2c9f..393677d 100644 --- a/auth/bucket_policy_actions.go +++ b/auth/bucket_policy_actions.go @@ -38,15 +38,20 @@ const ( GetObjectAction Action = "s3:GetObject" GetObjectVersionAction Action = "s3:GetObjectVersion" DeleteObjectAction Action = "s3:DeleteObject" + DeleteObjectVersionAction Action = "s3:DeleteObjectVersion" GetObjectAclAction Action = "s3:GetObjectAcl" GetObjectAttributesAction Action = "s3:GetObjectAttributes" + GetObjectVersionAttributesAction Action = "s3:GetObjectVersionAttributes" PutObjectAclAction Action = "s3:PutObjectAcl" RestoreObjectAction Action = "s3:RestoreObject" GetBucketTaggingAction Action = "s3:GetBucketTagging" PutBucketTaggingAction Action = "s3:PutBucketTagging" GetObjectTaggingAction Action = "s3:GetObjectTagging" + GetObjectVersionTaggingAction Action = "s3:GetObjectVersionTagging" PutObjectTaggingAction Action = "s3:PutObjectTagging" + PutObjectVersionTaggingAction Action = "s3:PutObjectVersionTagging" DeleteObjectTaggingAction Action = "s3:DeleteObjectTagging" + DeleteObjectVersionTaggingAction Action = "s3:DeleteObjectVersionTagging" ListBucketVersionsAction Action = "s3:ListBucketVersions" ListBucketAction Action = "s3:ListBucket" GetBucketObjectLockConfigurationAction Action = "s3:GetBucketObjectLockConfiguration" @@ -109,15 +114,20 @@ var supportedActionList = map[Action]struct{}{ GetObjectAction: {}, GetObjectVersionAction: {}, DeleteObjectAction: {}, + DeleteObjectVersionAction: {}, GetObjectAclAction: {}, GetObjectAttributesAction: {}, + GetObjectVersionAttributesAction: {}, PutObjectAclAction: {}, RestoreObjectAction: {}, GetBucketTaggingAction: {}, PutBucketTaggingAction: {}, GetObjectTaggingAction: {}, + GetObjectVersionTaggingAction: {}, PutObjectTaggingAction: {}, + PutObjectVersionTaggingAction: {}, DeleteObjectTaggingAction: {}, + DeleteObjectVersionTaggingAction: {}, ListBucketVersionsAction: {}, ListBucketAction: {}, GetBucketObjectLockConfigurationAction: {}, @@ -163,25 +173,30 @@ var supportedActionList = map[Action]struct{}{ } var supportedObjectActionList = map[Action]struct{}{ - AbortMultipartUploadAction: {}, - ListMultipartUploadPartsAction: {}, - PutObjectAction: {}, - GetObjectAction: {}, - GetObjectVersionAction: {}, - DeleteObjectAction: {}, - GetObjectAclAction: {}, - GetObjectAttributesAction: {}, - PutObjectAclAction: {}, - RestoreObjectAction: {}, - GetObjectTaggingAction: {}, - PutObjectTaggingAction: {}, - DeleteObjectTaggingAction: {}, - GetObjectLegalHoldAction: {}, - PutObjectLegalHoldAction: {}, - GetObjectRetentionAction: {}, - PutObjectRetentionAction: {}, - BypassGovernanceRetentionAction: {}, - AllActions: {}, + AbortMultipartUploadAction: {}, + ListMultipartUploadPartsAction: {}, + PutObjectAction: {}, + GetObjectAction: {}, + GetObjectVersionAction: {}, + DeleteObjectAction: {}, + DeleteObjectVersionAction: {}, + GetObjectAclAction: {}, + GetObjectAttributesAction: {}, + GetObjectVersionAttributesAction: {}, + PutObjectAclAction: {}, + RestoreObjectAction: {}, + GetObjectTaggingAction: {}, + GetObjectVersionTaggingAction: {}, + PutObjectTaggingAction: {}, + PutObjectVersionTaggingAction: {}, + DeleteObjectTaggingAction: {}, + DeleteObjectVersionTaggingAction: {}, + GetObjectLegalHoldAction: {}, + PutObjectLegalHoldAction: {}, + GetObjectRetentionAction: {}, + PutObjectRetentionAction: {}, + BypassGovernanceRetentionAction: {}, + AllActions: {}, } // Validates Action: it should either wildcard match with supported actions list or be in it diff --git a/s3api/controllers/object-delete.go b/s3api/controllers/object-delete.go index 59be267..59d5670 100644 --- a/s3api/controllers/object-delete.go +++ b/s3api/controllers/object-delete.go @@ -36,6 +36,11 @@ func (c S3ApiController) DeleteObjectTagging(ctx *fiber.Ctx) (*Response, error) isBucketPublic := utils.ContextKeyPublicBucket.IsSet(ctx) parsedAcl := utils.ContextKeyParsedAcl.Get(ctx).(auth.ACL) + action := auth.DeleteObjectTaggingAction + if versionId != "" { + action = auth.DeleteObjectVersionTaggingAction + } + err := auth.VerifyAccess(ctx.Context(), c.be, auth.AccessOptions{ Readonly: c.readonly, @@ -45,7 +50,7 @@ func (c S3ApiController) DeleteObjectTagging(ctx *fiber.Ctx) (*Response, error) Acc: acct, Bucket: bucket, Object: key, - Action: auth.DeleteObjectTaggingAction, + Action: action, IsPublicRequest: isBucketPublic, }) if err != nil { @@ -134,7 +139,10 @@ func (c S3ApiController) DeleteObject(ctx *fiber.Ctx) (*Response, error) { isBucketPublic := utils.ContextKeyPublicBucket.IsSet(ctx) parsedAcl := utils.ContextKeyParsedAcl.Get(ctx).(auth.ACL) - //TODO: check s3:DeleteObjectVersion policy in case a use tries to delete a version of an object + action := auth.DeleteObjectAction + if versionId != "" { + action = auth.DeleteObjectVersionAction + } err := auth.VerifyAccess(ctx.Context(), c.be, auth.AccessOptions{ @@ -145,7 +153,7 @@ func (c S3ApiController) DeleteObject(ctx *fiber.Ctx) (*Response, error) { Acc: acct, Bucket: bucket, Object: key, - Action: auth.DeleteObjectAction, + Action: action, IsPublicRequest: isBucketPublic, }) if err != nil { diff --git a/s3api/controllers/object-get.go b/s3api/controllers/object-get.go index ced8527..f8d938f 100644 --- a/s3api/controllers/object-get.go +++ b/s3api/controllers/object-get.go @@ -41,6 +41,11 @@ func (c S3ApiController) GetObjectTagging(ctx *fiber.Ctx) (*Response, error) { parsedAcl := utils.ContextKeyParsedAcl.Get(ctx).(auth.ACL) isPublicBucket := utils.ContextKeyPublicBucket.IsSet(ctx) + action := auth.GetObjectTaggingAction + if versionId != "" { + action = auth.GetObjectVersionTaggingAction + } + err := auth.VerifyAccess(ctx.Context(), c.be, auth.AccessOptions{ Readonly: c.readonly, Acl: parsedAcl, @@ -49,7 +54,7 @@ func (c S3ApiController) GetObjectTagging(ctx *fiber.Ctx) (*Response, error) { Acc: acct, Bucket: bucket, Object: key, - Action: auth.GetObjectTaggingAction, + Action: action, IsPublicRequest: isPublicBucket, }) if err != nil { @@ -321,6 +326,11 @@ func (c S3ApiController) GetObjectAttributes(ctx *fiber.Ctx) (*Response, error) parsedAcl := utils.ContextKeyParsedAcl.Get(ctx).(auth.ACL) isPublicBucket := utils.ContextKeyPublicBucket.IsSet(ctx) + action := auth.GetObjectAttributesAction + if versionId != "" { + action = auth.GetObjectVersionAttributesAction + } + err := auth.VerifyAccess(ctx.Context(), c.be, auth.AccessOptions{ Readonly: c.readonly, Acl: parsedAcl, @@ -329,7 +339,7 @@ func (c S3ApiController) GetObjectAttributes(ctx *fiber.Ctx) (*Response, error) Acc: acct, Bucket: bucket, Object: key, - Action: auth.GetObjectAttributesAction, + Action: action, IsPublicRequest: isPublicBucket, }) if err != nil { diff --git a/s3api/controllers/object-put.go b/s3api/controllers/object-put.go index 260544c..6578470 100644 --- a/s3api/controllers/object-put.go +++ b/s3api/controllers/object-put.go @@ -42,6 +42,11 @@ func (c S3ApiController) PutObjectTagging(ctx *fiber.Ctx) (*Response, error) { IsBucketPublic := utils.ContextKeyPublicBucket.IsSet(ctx) parsedAcl := utils.ContextKeyParsedAcl.Get(ctx).(auth.ACL) + action := auth.PutObjectTaggingAction + if versionId != "" { + action = auth.PutObjectVersionTaggingAction + } + err := auth.VerifyAccess(ctx.Context(), c.be, auth.AccessOptions{ Readonly: c.readonly, Acl: parsedAcl, @@ -50,7 +55,7 @@ func (c S3ApiController) PutObjectTagging(ctx *fiber.Ctx) (*Response, error) { Acc: acct, Bucket: bucket, Object: key, - Action: auth.PutObjectTaggingAction, + Action: action, IsPublicRequest: IsBucketPublic, }) if err != nil { diff --git a/tests/integration/group-tests.go b/tests/integration/group-tests.go index 0466f1d..9e41686 100644 --- a/tests/integration/group-tests.go +++ b/tests/integration/group-tests.go @@ -1071,6 +1071,9 @@ func TestVersioning(ts *TestState) { // Versioninig_concurrent_upload_object ts.Run(Versioning_AccessControl_GetObjectVersion) ts.Run(Versioning_AccessControl_HeadObjectVersion) + ts.Run(Versioning_AccessControl_object_tagging_policy) + ts.Run(Versioning_AccessControl_DeleteObject_policy) + ts.Run(Versioning_AccessControl_GetObjectAttributes_policy) } func TestVersioningDisabled(ts *TestState) { @@ -1710,6 +1713,9 @@ func GetIntTests() IntTests { "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_AccessControl_object_tagging_policy": Versioning_AccessControl_object_tagging_policy, + "Versioning_AccessControl_DeleteObject_policy": Versioning_AccessControl_DeleteObject_policy, + "Versioning_AccessControl_GetObjectAttributes_policy": Versioning_AccessControl_GetObjectAttributes_policy, "Versioning_concurrent_upload_object": Versioning_concurrent_upload_object, "RouterPutPartNumberWithoutUploadId": RouterPutPartNumberWithoutUploadId, "RouterPostRoot": RouterPostRoot, diff --git a/tests/integration/utils.go b/tests/integration/utils.go index d701db4..ba57a1d 100644 --- a/tests/integration/utils.go +++ b/tests/integration/utils.go @@ -2000,3 +2000,13 @@ type mpinfo struct { uploadId *string parts []types.CompletedPart } + +func putBucketPolicy(client *s3.Client, bucket, policy string) error { + ctx, cancel := context.WithTimeout(context.Background(), shortTimeout) + _, err := client.PutBucketPolicy(ctx, &s3.PutBucketPolicyInput{ + Bucket: &bucket, + Policy: &policy, + }) + cancel() + return err +} diff --git a/tests/integration/versioning.go b/tests/integration/versioning.go index f7171aa..bb70a08 100644 --- a/tests/integration/versioning.go +++ b/tests/integration/versioning.go @@ -2832,6 +2832,285 @@ func Versioning_AccessControl_HeadObjectVersion(s *S3Conf) error { }, withVersioning(types.BucketVersioningStatusEnabled)) } +func Versioning_AccessControl_object_tagging_policy(s *S3Conf) error { + testName := "Versioning_AccessControl_PutObjectTagging_policy" + return actionHandler(s, testName, func(s3client *s3.Client, bucket string) error { + object := "my-object" + res, err := putObjectWithData(10, &s3.PutObjectInput{ + Bucket: &bucket, + Key: &object, + }, s3client) + if err != nil { + return err + } + + testuser := getUser("user") + err = createUsers(s, []user{testuser}) + if err != nil { + return err + } + + userClient := s.getUserClient(testuser) + + putGetDeleteObjectTagging := func(versionId *string, denyAccess bool) error { + ctx, cancel := context.WithTimeout(context.Background(), shortTimeout) + _, err := userClient.PutObjectTagging(ctx, &s3.PutObjectTaggingInput{ + Bucket: &bucket, + Key: &object, + VersionId: versionId, + Tagging: &types.Tagging{ + TagSet: []types.Tag{ + {Key: getPtr("key"), Value: getPtr("value")}, + }, + }, + }) + cancel() + if denyAccess { + if err := checkApiErr(err, s3err.GetAPIError(s3err.ErrAccessDenied)); err != nil { + return err + } + } else { + if err != nil { + return err + } + } + + ctx, cancel = context.WithTimeout(context.Background(), shortTimeout) + _, err = userClient.GetObjectTagging(ctx, &s3.GetObjectTaggingInput{ + Bucket: &bucket, + Key: &object, + VersionId: versionId, + }) + cancel() + if denyAccess { + if err := checkApiErr(err, s3err.GetAPIError(s3err.ErrAccessDenied)); err != nil { + return err + } + } else { + if err != nil { + return err + } + } + + ctx, cancel = context.WithTimeout(context.Background(), shortTimeout) + _, err = userClient.DeleteObjectTagging(ctx, &s3.DeleteObjectTaggingInput{ + Bucket: &bucket, + Key: &object, + VersionId: versionId, + }) + cancel() + if denyAccess { + if err := checkApiErr(err, s3err.GetAPIError(s3err.ErrAccessDenied)); err != nil { + return err + } + } else { + if err != nil { + return err + } + } + + return nil + } + + policy := genPolicyDoc("Allow", fmt.Sprintf(`"%s"`, testuser.access), `["s3:PutObjectVersionTagging", "s3:GetObjectVersionTagging", "s3:DeleteObjectVersionTagging"]`, fmt.Sprintf(`"arn:aws:s3:::%s/*"`, bucket)) + err = putBucketPolicy(s3client, bucket, policy) + if err != nil { + return err + } + + // deny without versionId + err = putGetDeleteObjectTagging(nil, true) + if err != nil { + return err + } + + // allow with versionId + err = putGetDeleteObjectTagging(res.res.VersionId, false) + if err != nil { + return err + } + + policy = genPolicyDoc("Allow", fmt.Sprintf(`"%s"`, testuser.access), `["s3:PutObjectTagging", "s3:GetObjectTagging", "s3:DeleteObjectTagging"]`, fmt.Sprintf(`"arn:aws:s3:::%s/*"`, bucket)) + err = putBucketPolicy(s3client, bucket, policy) + if err != nil { + return err + } + + // allow without versionId + err = putGetDeleteObjectTagging(nil, false) + if err != nil { + return err + } + + // deny with versionId + err = putGetDeleteObjectTagging(res.res.VersionId, true) + if err != nil { + return err + } + + return nil + }, withVersioning(types.BucketVersioningStatusEnabled)) +} + +func Versioning_AccessControl_DeleteObject_policy(s *S3Conf) error { + testName := "Versioning_AccessControl_DeleteObject_policy" + return actionHandler(s, testName, func(s3client *s3.Client, bucket string) error { + obj := "my-object" + testuser := getUser("user") + err := createUsers(s, []user{testuser}) + if err != nil { + return err + } + + userClient := s.getUserClient(testuser) + + delObject := func(versionId *string, denyAccess bool) error { + ctx, cancel := context.WithTimeout(context.Background(), shortTimeout) + _, err := userClient.DeleteObject(ctx, &s3.DeleteObjectInput{ + Bucket: &bucket, + Key: &obj, + VersionId: versionId, + }) + cancel() + if denyAccess { + return checkApiErr(err, s3err.GetAPIError(s3err.ErrAccessDenied)) + } + + return err + } + + res, err := putObjectWithData(10, &s3.PutObjectInput{ + Bucket: &bucket, + Key: &obj, + }, s3client) + if err != nil { + return err + } + + policy := genPolicyDoc("Allow", fmt.Sprintf(`"%s"`, testuser.access), `"s3:DeleteObject"`, fmt.Sprintf(`"arn:aws:s3:::%s/*"`, bucket)) + err = putBucketPolicy(s3client, bucket, policy) + if err != nil { + return err + } + + // deny with versionId + err = delObject(res.res.VersionId, true) + if err != nil { + return err + } + + // allow without versionId + err = delObject(nil, false) + if err != nil { + return err + } + + policy = genPolicyDoc("Allow", fmt.Sprintf(`"%s"`, testuser.access), `"s3:DeleteObjectVersion"`, fmt.Sprintf(`"arn:aws:s3:::%s/*"`, bucket)) + err = putBucketPolicy(s3client, bucket, policy) + if err != nil { + return err + } + + // recreate the object + res, err = putObjectWithData(10, &s3.PutObjectInput{ + Bucket: &bucket, + Key: &obj, + }, s3client) + if err != nil { + return err + } + + // deny without versionId + err = delObject(nil, true) + if err != nil { + return err + } + + // allow with versionId + err = delObject(res.res.VersionId, false) + if err != nil { + return err + } + + return nil + }, withVersioning(types.BucketVersioningStatusEnabled)) +} + +func Versioning_AccessControl_GetObjectAttributes_policy(s *S3Conf) error { + testName := "Versioning_AccessControl_GetObjectAttributes_policy" + return actionHandler(s, testName, func(s3client *s3.Client, bucket string) error { + obj := "my-object" + res, err := putObjectWithData(10, &s3.PutObjectInput{ + Bucket: &bucket, + Key: &obj, + }, s3client) + if err != nil { + return err + } + + testuser := getUser("user") + err = createUsers(s, []user{testuser}) + if err != nil { + return err + } + userClient := s.getUserClient(testuser) + + getObjectAttr := func(versionId *string, denyAccess bool) error { + ctx, cancel := context.WithTimeout(context.Background(), shortTimeout) + _, err := userClient.GetObjectAttributes(ctx, &s3.GetObjectAttributesInput{ + Bucket: &bucket, + Key: &obj, + VersionId: versionId, + ObjectAttributes: types.ObjectAttributesChecksum.Values(), + }) + cancel() + if denyAccess { + return checkApiErr(err, s3err.GetAPIError(s3err.ErrAccessDenied)) + } + + return nil + } + + policy := genPolicyDoc("Allow", fmt.Sprintf(`"%s"`, testuser.access), `"s3:GetObjectAttributes"`, fmt.Sprintf(`"arn:aws:s3:::%s/*"`, bucket)) + err = putBucketPolicy(s3client, bucket, policy) + if err != nil { + return err + } + + // deny with versionId + err = getObjectAttr(res.res.VersionId, true) + if err != nil { + return err + } + + // allow without versionId + err = getObjectAttr(nil, false) + if err != nil { + return err + } + + policy = genPolicyDoc("Allow", fmt.Sprintf(`"%s"`, testuser.access), `"s3:GetObjectVersionAttributes"`, fmt.Sprintf(`"arn:aws:s3:::%s/*"`, bucket)) + err = putBucketPolicy(s3client, bucket, policy) + if err != nil { + return err + } + + // deny without versionId + err = getObjectAttr(nil, true) + if err != nil { + return err + } + + // allow with versionId + err = getObjectAttr(res.res.VersionId, false) + if err != nil { + return err + } + + return nil + }, withVersioning(types.BucketVersioningStatusEnabled)) +} + func VersioningDisabled_GetBucketVersioning_not_configured(s *S3Conf) error { testName := "VersioningDisabled_GetBucketVersioning_not_configured" return actionHandler(s, testName, func(s3client *s3.Client, bucket string) error {