diff --git a/s3api/controllers/bucket-post.go b/s3api/controllers/bucket-post.go index 72611ba0..1ca7dce7 100644 --- a/s3api/controllers/bucket-post.go +++ b/s3api/controllers/bucket-post.go @@ -144,28 +144,6 @@ func (c S3ApiController) POSTObject(ctx fiber.Ctx) (*Response, error) { key := parsed.Fields["key"] - // A POST upload is an s3:PutObject on the object named by the form's - // key field, so it is authorized against that object's ARN — the same - // resource PutObject is — not the bucket's. - err := c.verifyAccess(ctx, - auth.AccessOptions{ - Acl: parsedAcl, - AclPermission: auth.PermissionWrite, - IsRoot: isRoot, - Acc: acct, - Bucket: bucket, - Object: key, - Actions: []auth.Action{auth.PutObjectAction}, - IsPublicRequest: IsBucketPublic, - }) - if err != nil { - return &Response{ - MetaOpts: &MetaOptions{ - BucketOwner: parsedAcl.Owner, - }, - }, err - } - // parse POST policy — absent for anonymous uploads to public buckets if !IsBucketPublic { policyBase64 := parsed.Fields["policy"] @@ -198,6 +176,7 @@ func (c S3ApiController) POSTObject(ctx fiber.Ctx) (*Response, error) { // to pass PutObject, which expects the tagging to be a query string var tagging string if taggingXML, ok := parsed.Fields["tagging"]; ok { + var err error tagging, err = utils.ConvertTaggingXMLToQueryString([]byte(taggingXML)) if err != nil { return &Response{ @@ -237,6 +216,34 @@ func (c S3ApiController) POSTObject(ctx fiber.Ctx) (*Response, error) { }, err } + // A POST upload is an s3:PutObject on the object named by the form's + // key field, so it is authorized against that object's ARN — the same + // resource PutObject is — not the bucket's. Tagging the object also + // takes s3:PutObjectTagging, but only for a non-empty tag set + actions := []auth.Action{auth.PutObjectAction} + if tagging != "" { + actions = append(actions, auth.PutObjectTaggingAction) + } + + err = c.verifyAccess(ctx, + auth.AccessOptions{ + Acl: parsedAcl, + AclPermission: auth.PermissionWrite, + IsRoot: isRoot, + Acc: acct, + Bucket: bucket, + Object: key, + Actions: actions, + IsPublicRequest: IsBucketPublic, + }) + if err != nil { + return &Response{ + MetaOpts: &MetaOptions{ + BucketOwner: parsedAcl.Owner, + }, + }, err + } + err = auth.CheckObjectAccess(ctx, bucket, acct, []types.ObjectIdentifier{{Key: &key}}, auth.BypassOverwrite, IsBucketPublic, c.be, c.iam, true) if err != nil { return &Response{ diff --git a/s3api/controllers/bucket-post_test.go b/s3api/controllers/bucket-post_test.go index 14f84796..4e305eb2 100644 --- a/s3api/controllers/bucket-post_test.go +++ b/s3api/controllers/bucket-post_test.go @@ -421,9 +421,9 @@ func TestS3ApiController_POSTObject(t *testing.T) { }, utils.ContextKeyRegion: "us-east-1", utils.ContextKeyObjectPostResult: middlewares.PostObjectResult{ - Fields: map[string]string{ - "key": "key", - }, + Fields: baseFields, + FileRdr: newMockFileReader("payload"), + ContentLength: int64(len("payload")), }, }, }, diff --git a/tests/integration/PostObject.go b/tests/integration/PostObject.go index e8609f94..b047db2b 100644 --- a/tests/integration/PostObject.go +++ b/tests/integration/PostObject.go @@ -428,6 +428,238 @@ func PostObject_bucket_policy_explicit_deny(s *S3Conf) error { }) } +func PostObject_tagging_requires_put_object_tagging(s *S3Conf) error { + testName := "PostObject_tagging_requires_put_object_tagging" + return actionHandler(s, testName, func(s3client *s3.Client, bucket string) error { + testuser := getUser("user") + if err := createUsers(s, []user{testuser}); err != nil { + return err + } + + key := "my-obj" + taggingXML := `envtest` + post := func() (*http.Response, error) { + return sendPostObject(PostRequestConfig{ + bucket: bucket, + key: key, + access: testuser.access, + secret: testuser.secret, + s3Conf: s, + fileContent: []byte("data"), + policyConditions: []any{ + []any{"eq", "$tagging", taggingXML}, + }, + extraFields: map[string]string{ + "tagging": taggingXML, + }, + }) + } + + if err := putBucketPolicyDoc(s, bucket, bucketStatement{ + Effect: "Allow", + Principal: testuser.access, + Action: "s3:PutObject", + Resource: fmt.Sprintf("arn:aws:s3:::%s/*", bucket), + }); err != nil { + return err + } + + resp, err := post() + if err != nil { + return err + } + if err := checkHTTPResponseApiErr(resp, s3err.GetAPIError(s3err.ErrAccessDenied)); err != nil { + return fmt.Errorf("POST with s3:PutObject only: %w", err) + } + + ctx, cancel := context.WithTimeout(context.Background(), shortTimeout) + _, err = s3client.HeadObject(ctx, &s3.HeadObjectInput{ + Bucket: &bucket, + Key: &key, + }) + cancel() + if err := checkSdkApiErr(err, "NotFound"); err != nil { + return fmt.Errorf("expected the denied POST not to create %s: %w", key, err) + } + + if err := putBucketPolicyDoc(s, bucket, bucketStatement{ + Effect: "Allow", + Principal: testuser.access, + Action: []string{"s3:PutObject", "s3:PutObjectTagging"}, + Resource: fmt.Sprintf("arn:aws:s3:::%s/*", bucket), + }); err != nil { + return err + } + + resp, err = post() + if err != nil { + return err + } + if err := checkPostObjectSuccess(resp); err != nil { + return fmt.Errorf("POST with s3:PutObject and s3:PutObjectTagging: %w", err) + } + + ctx, cancel = context.WithTimeout(context.Background(), shortTimeout) + tagging, err := s3client.GetObjectTagging(ctx, &s3.GetObjectTaggingInput{ + Bucket: &bucket, + Key: &key, + }) + cancel() + if err != nil { + return err + } + + expectedTagging := []types.Tag{{Key: getPtr("env"), Value: getPtr("test")}} + if !areTagsSame(expectedTagging, tagging.TagSet) { + return fmt.Errorf("expected %v tagging, instead got %v", expectedTagging, tagging.TagSet) + } + + return nil + }) +} + +// PostObject_tagging_bucket_policy_explicit_deny covers a Deny on +// s3:PutObjectTagging scoped to a key prefix. It refuses a tagged POST +// upload to a key under that prefix although a bucket-wide Allow grants +// both actions, and leaves an untagged upload there, which needs only +// s3:PutObject, alone. +func PostObject_tagging_bucket_policy_explicit_deny(s *S3Conf) error { + testName := "PostObject_tagging_bucket_policy_explicit_deny" + return actionHandler(s, testName, func(s3client *s3.Client, bucket string) error { + testuser := getUser("user") + if err := createUsers(s, []user{testuser}); err != nil { + return err + } + + if err := putBucketPolicyDoc(s, bucket, + bucketStatement{ + Effect: "Allow", + Principal: testuser.access, + Action: []string{"s3:PutObject", "s3:PutObjectTagging"}, + Resource: fmt.Sprintf("arn:aws:s3:::%s/*", bucket), + }, + bucketStatement{ + Effect: "Deny", + Principal: testuser.access, + Action: "s3:PutObjectTagging", + Resource: fmt.Sprintf("arn:aws:s3:::%s/private/*", bucket), + }, + ); err != nil { + return err + } + + cfg := func(key string) PostRequestConfig { + return PostRequestConfig{ + bucket: bucket, + key: key, + access: testuser.access, + secret: testuser.secret, + s3Conf: s, + fileContent: []byte("data"), + } + } + taggingXML := `envtest` + tagged := func(key string) PostRequestConfig { + c := cfg(key) + c.policyConditions = []any{ + []any{"eq", "$tagging", taggingXML}, + } + c.extraFields = map[string]string{ + "tagging": taggingXML, + } + return c + } + + allowedKey := "public/my-obj" + resp, err := sendPostObject(tagged(allowedKey)) + if err != nil { + return err + } + if err := checkPostObjectSuccess(resp); err != nil { + return fmt.Errorf("tagged POST %s: %w", allowedKey, err) + } + + deniedKey := "private/my-obj" + resp, err = sendPostObject(tagged(deniedKey)) + if err != nil { + return err + } + if err := checkHTTPResponseApiErr(resp, s3err.GetExplicitDenyAccessErr(testuser.access, "s3:PutObjectTagging", + fmt.Sprintf("arn:aws:s3:::%s/%s", bucket, deniedKey), "a resource-based policy")); err != nil { + return fmt.Errorf("tagged POST %s: %w", deniedKey, err) + } + + resp, err = sendPostObject(cfg(deniedKey)) + if err != nil { + return err + } + if err := checkPostObjectSuccess(resp); err != nil { + return fmt.Errorf("untagged POST %s: %w", deniedKey, err) + } + return nil + }) +} + +// PostObject_empty_tag_set_requires_only_put_object covers a tagging field +// whose tag set is empty. It tags nothing, so the upload needs only +// s3:PutObject, as it does on S3. +func PostObject_empty_tag_set_requires_only_put_object(s *S3Conf) error { + testName := "PostObject_empty_tag_set_requires_only_put_object" + return actionHandler(s, testName, func(s3client *s3.Client, bucket string) error { + testuser := getUser("user") + if err := createUsers(s, []user{testuser}); err != nil { + return err + } + + if err := putBucketPolicyDoc(s, bucket, bucketStatement{ + Effect: "Allow", + Principal: testuser.access, + Action: "s3:PutObject", + Resource: fmt.Sprintf("arn:aws:s3:::%s/*", bucket), + }); err != nil { + return err + } + + key := "my-obj" + taggingXML := `` + resp, err := sendPostObject(PostRequestConfig{ + bucket: bucket, + key: key, + access: testuser.access, + secret: testuser.secret, + s3Conf: s, + fileContent: []byte("data"), + policyConditions: []any{ + []any{"eq", "$tagging", taggingXML}, + }, + extraFields: map[string]string{ + "tagging": taggingXML, + }, + }) + if err != nil { + return err + } + if err := checkPostObjectSuccess(resp); err != nil { + return err + } + + ctx, cancel := context.WithTimeout(context.Background(), shortTimeout) + tagging, err := s3client.GetObjectTagging(ctx, &s3.GetObjectTaggingInput{ + Bucket: &bucket, + Key: &key, + }) + cancel() + if err != nil { + return err + } + if len(tagging.TagSet) != 0 { + return fmt.Errorf("expected no tags, instead got %v", tagging.TagSet) + } + + return nil + }) +} + func PostObject_invalid_object_names(s *S3Conf) error { testName := "PostObject_invalid_object_names" return actionHandler(s, testName, func(s3client *s3.Client, bucket string) error { diff --git a/tests/integration/group-tests.go b/tests/integration/group-tests.go index 3ca71dd5..426970f1 100644 --- a/tests/integration/group-tests.go +++ b/tests/integration/group-tests.go @@ -1751,6 +1751,7 @@ func TestS3IAMAccessControl(ts *TestState) { ts.Run(S3IAMAccessControl_identity_policy_resource_scoping) ts.Run(S3IAMAccessControl_identity_policy_bucket_vs_object_arn) ts.Run(S3IAMAccessControl_post_object_identity_policy_resource_scoping) + ts.Run(S3IAMAccessControl_post_object_tagging_identity_policy) ts.Run(S3IAMAccessControl_identity_policy_not_action_and_not_resource) ts.Run(S3IAMAccessControl_identity_policy_explicit_deny_wins) ts.Run(S3IAMAccessControl_multiple_inline_policies_combine) @@ -2131,6 +2132,9 @@ func TestPostObject(ts *TestState) { ts.Run(PostObject_access_denied) ts.Run(PostObject_bucket_policy_object_resource) ts.Run(PostObject_bucket_policy_explicit_deny) + ts.Run(PostObject_tagging_requires_put_object_tagging) + ts.Run(PostObject_tagging_bucket_policy_explicit_deny) + ts.Run(PostObject_empty_tag_set_requires_only_put_object) ts.Run(PostObject_invalid_object_names) ts.Run(PostObject_policy_access_control) ts.Run(PostObject_policy_expired) @@ -2262,6 +2266,7 @@ func GetIntTests() IntTests { "S3IAMAccessControl_identity_policy_resource_scoping": S3IAMAccessControl_identity_policy_resource_scoping, "S3IAMAccessControl_identity_policy_bucket_vs_object_arn": S3IAMAccessControl_identity_policy_bucket_vs_object_arn, "S3IAMAccessControl_post_object_identity_policy_resource_scoping": S3IAMAccessControl_post_object_identity_policy_resource_scoping, + "S3IAMAccessControl_post_object_tagging_identity_policy": S3IAMAccessControl_post_object_tagging_identity_policy, "S3IAMAccessControl_identity_policy_not_action_and_not_resource": S3IAMAccessControl_identity_policy_not_action_and_not_resource, "S3IAMAccessControl_identity_policy_explicit_deny_wins": S3IAMAccessControl_identity_policy_explicit_deny_wins, "S3IAMAccessControl_multiple_inline_policies_combine": S3IAMAccessControl_multiple_inline_policies_combine, @@ -3699,6 +3704,9 @@ func GetIntTests() IntTests { "PostObject_access_denied": PostObject_access_denied, "PostObject_bucket_policy_object_resource": PostObject_bucket_policy_object_resource, "PostObject_bucket_policy_explicit_deny": PostObject_bucket_policy_explicit_deny, + "PostObject_tagging_requires_put_object_tagging": PostObject_tagging_requires_put_object_tagging, + "PostObject_tagging_bucket_policy_explicit_deny": PostObject_tagging_bucket_policy_explicit_deny, + "PostObject_empty_tag_set_requires_only_put_object": PostObject_empty_tag_set_requires_only_put_object, "PostObject_invalid_object_names": PostObject_invalid_object_names, "PostObject_policy_access_control": PostObject_policy_access_control, "PostObject_policy_expired": PostObject_policy_expired, diff --git a/tests/integration/s3_iam_access_control.go b/tests/integration/s3_iam_access_control.go index 9ca1232f..050e23b5 100644 --- a/tests/integration/s3_iam_access_control.go +++ b/tests/integration/s3_iam_access_control.go @@ -284,6 +284,83 @@ func S3IAMAccessControl_post_object_identity_policy_resource_scoping(s *S3Conf) }) } +// S3IAMAccessControl_post_object_tagging_identity_policy verifies a POST +// upload that tags its object needs s3:PutObjectTagging on the object in +// addition to s3:PutObject, as PutObject does. With only s3:PutObject +// granted, the tagged upload is denied naming s3:PutObjectTagging, and an +// explicit Deny on s3:PutObjectTagging refuses it even under s3:*. The +// untagged upload succeeds in both cases. +func S3IAMAccessControl_post_object_tagging_identity_policy(s *S3Conf) error { + testName := "S3IAMAccessControl_post_object_tagging_identity_policy" + return s3IAMActionHandler(s, testName, func(root *iam.Client, bucket string) error { + cases := []struct { + name string + policy string + want func(principal, resourceArn string) s3err.S3Error + }{ + { + name: "s3:PutObject only", + policy: policyDoc(accessStatement{ + Effect: "Allow", Action: actS3PutObject, Resource: objectsArn(bucket), + }), + want: func(principal, resourceArn string) s3err.S3Error { + return wantImplicitDeny(principal, actS3PutObjectTagging, resourceArn) + }, + }, + { + name: "s3:PutObjectTagging explicitly denied", + policy: policyDoc( + accessStatement{Effect: "Allow", Action: "s3:*", Resource: objectsArn(bucket)}, + accessStatement{Effect: "Deny", Action: actS3PutObjectTagging, Resource: objectsArn(bucket)}, + ), + want: func(principal, resourceArn string) s3err.S3Error { + return wantExplicitIdentityDeny(principal, actS3PutObjectTagging, resourceArn) + }, + }, + } + for _, tc := range cases { + if err := func() error { + user, cleanup, err := newS3IAMUser(root, s, map[string]string{"p": tc.policy}) + if err != nil { + return err + } + defer cleanup() + + cfg := PostRequestConfig{ + bucket: bucket, + key: "obj", + s3Conf: &user.conf, + fileContent: []byte("data"), + } + + resp, err := sendPostObject(cfg) + if err != nil { + return err + } + if err := checkPostObjectSuccess(resp); err != nil { + return fmt.Errorf("expected the untagged POST to be allowed: %w", err) + } + + taggingXML := `envtest` + cfg.policyConditions = []any{ + []any{"eq", "$tagging", taggingXML}, + } + cfg.extraFields = map[string]string{ + "tagging": taggingXML, + } + resp, err = sendPostObject(cfg) + if err != nil { + return err + } + return checkHTTPResponseApiErr(resp, tc.want(user.arn, objectArn(bucket, "obj"))) + }(); err != nil { + return fmt.Errorf("%s: %w", tc.name, err) + } + } + return nil + }) +} + // S3IAMAccessControl_identity_policy_not_action_and_not_resource verifies // NotAction and NotResource grant everything *except* what they name. func S3IAMAccessControl_identity_policy_not_action_and_not_resource(s *S3Conf) error { diff --git a/tests/integration/s3_iam_utils.go b/tests/integration/s3_iam_utils.go index 645caefb..535f239f 100644 --- a/tests/integration/s3_iam_utils.go +++ b/tests/integration/s3_iam_utils.go @@ -40,6 +40,7 @@ import ( const ( actS3GetObject = "s3:GetObject" actS3PutObject = "s3:PutObject" + actS3PutObjectTagging = "s3:PutObjectTagging" actS3DeleteObject = "s3:DeleteObject" actS3DeleteObjectVersion = "s3:DeleteObjectVersion" actS3ListBucket = "s3:ListBucket"