mirror of
https://github.com/versity/versitygw.git
synced 2026-09-25 09:24:22 +00:00
fix: require s3:PutObjectTagging for tagged browser-based POST object uploads
`POSTObject` authorized only `s3:PutObject`, even when the form's `tagging` field set tags on the uploaded object. `PutObject`, `CopyObject` and `CreateMultipartUpload` also require `s3:PutObjectTagging` when tags are sent, so a caller without that permission could tag objects through `POST` in a way `PutObject` refuses. S3 enforces the permission for `POST` too: a user granted only `s3:PutObject` gets `AccessDenied` for `s3:PutObjectTagging` on the object ARN. `POSTObject` now adds `s3:PutObjectTagging` to the `verifyAccess` actions when the parsed tag set is not empty, in the same single check as `s3:PutObject`, matching `PutObject`. A `tagging` field with an empty `TagSet` still needs only `s3:PutObject`, as on S3. The access check now runs after the form is validated, instead of before the `POST` policy is parsed. S3 validates the policy and its conditions, the tagging, the checksum fields, the metadata size and the website redirect location before it authorizes the upload, so a caller missing a permission now gets the validation error for an invalid form instead of `AccessDenied`.
This commit is contained in:
@@ -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{
|
||||
|
||||
@@ -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")),
|
||||
},
|
||||
},
|
||||
},
|
||||
|
||||
@@ -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 := `<Tagging><TagSet><Tag><Key>env</Key><Value>test</Value></Tag></TagSet></Tagging>`
|
||||
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 := `<Tagging><TagSet><Tag><Key>env</Key><Value>test</Value></Tag></TagSet></Tagging>`
|
||||
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 := `<Tagging><TagSet></TagSet></Tagging>`
|
||||
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 {
|
||||
|
||||
@@ -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,
|
||||
|
||||
@@ -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 := `<Tagging><TagSet><Tag><Key>env</Key><Value>test</Value></Tag></TagSet></Tagging>`
|
||||
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 {
|
||||
|
||||
@@ -40,6 +40,7 @@ import (
|
||||
const (
|
||||
actS3GetObject = "s3:GetObject"
|
||||
actS3PutObject = "s3:PutObject"
|
||||
actS3PutObjectTagging = "s3:PutObjectTagging"
|
||||
actS3DeleteObject = "s3:DeleteObject"
|
||||
actS3DeleteObjectVersion = "s3:DeleteObjectVersion"
|
||||
actS3ListBucket = "s3:ListBucket"
|
||||
|
||||
Reference in New Issue
Block a user