From 483dd4b12e9940d368dee3a585d0ebfb5aba3dc7 Mon Sep 17 00:00:00 2001 From: zhao-yc <43179751+zhao-yc@users.noreply.github.com> Date: Mon, 5 Oct 2026 09:13:19 +0800 Subject: [PATCH] s3api: persist ACLs on PutObject uploads (#11592) * s3api: persist ACLs on PutObject uploads Signed-off-by: zhaoyuchen <43179751+zhao-yc@users.noreply.github.com> * s3api: fix PutObject ACL edge cases found in review - Only enforce BucketOwnerEnforced when explicitly configured; buckets without a stored ownership control keep accepting upload ACLs - Ignore ACL query parameters on SigV2 requests, which do not sign them - Mirror signed-query ACL values into headers after authentication so grant parsing and resolveFileMode agree on presigned uploads - Validate only caller-supplied grantees against the account registry; default grants now work for accounts outside the local registry - Reject unknown grantee keys and accept comma-separated grantee lists without spaces in ParseCustomAclHeader - Guard against identities without an account * s3api: harden upload ACL parsing and authorization Signed-off-by: zhaoyuchen <43179751+zhao-yc@users.noreply.github.com> * s3api: evaluate upload ACL grantees individually in policies A comma-joined grant header or a signed query parameter reached policy conditions as one value, so a deny on a later grantee did not fire. Split grant headers into per-grantee values for policy evaluation and share the grantee pair parser with ParseCustomAclHeader. * s3api: keep raw grant header values visible to policy conditions Exact-match conditions written against the signed header value stopped matching once grantees were split for evaluation. Preserve the original wire values alongside the per-grantee values so deny policies fire on either granularity. * s3api: evaluate upload ACL grants as one canonical list in policies Conditions on s3:x-amz-grant-* now see a single comma-separated canonical grant list identical for a single line, repeated header lines, or a signed query parameter. This keeps StringEquals allows and exact-list or allowlist (StringNotEquals) denies accurate regardless of wire encoding. * s3api: preserve upload ACL denies and align policy checks Signed-off-by: zhaoyuchen <43179751+zhao-yc@users.noreply.github.com> * s3api: retain upload owner grants and literal policy values Signed-off-by: zhaoyuchen <43179751+zhao-yc@users.noreply.github.com> --------- Signed-off-by: zhaoyuchen <43179751+zhao-yc@users.noreply.github.com> Co-authored-by: Chris Lu --- weed/s3api/auth_credentials.go | 10 +- weed/s3api/policy_engine/conditions.go | 12 + weed/s3api/policy_engine/engine.go | 4 + .../policy_engine/request_grant_conditions.go | 46 ++ .../request_grant_conditions_test.go | 63 ++ weed/s3api/policy_engine/types.go | 3 + weed/s3api/s3_action_resolver.go | 4 +- weed/s3api/s3api_acl_header_parser_test.go | 61 ++ weed/s3api/s3api_acl_helper.go | 101 +-- weed/s3api/s3api_bucket_policy_engine.go | 1 + weed/s3api/s3api_object_handlers_put.go | 25 +- weed/s3api/s3api_object_upload_acl.go | 265 +++++++ weed/s3api/s3api_object_upload_acl_test.go | 706 ++++++++++++++++++ weed/s3api/s3api_server.go | 7 + weed/s3api/s3err/s3api_errors.go | 6 + 15 files changed, 1266 insertions(+), 48 deletions(-) create mode 100644 weed/s3api/policy_engine/request_grant_conditions.go create mode 100644 weed/s3api/policy_engine/request_grant_conditions_test.go create mode 100644 weed/s3api/s3api_acl_header_parser_test.go create mode 100644 weed/s3api/s3api_object_upload_acl.go create mode 100644 weed/s3api/s3api_object_upload_acl_test.go diff --git a/weed/s3api/auth_credentials.go b/weed/s3api/auth_credentials.go index afbf303ba..aff58e713 100644 --- a/weed/s3api/auth_credentials.go +++ b/weed/s3api/auth_credentials.go @@ -1778,6 +1778,13 @@ func (iam *IdentityAccessManagement) authRequestWithAuthType(r *http.Request, ac } bucket, object := s3_constants.GetBucketAndObject(r) + // Verify the original signature first, then evaluate policies against the + // effective PUT ACL, including signed query parameters hoisted by presigners. + originalRequest := r + r, s3Err = putObjectACLPolicyRequest(r, action, bucket, object) + if s3Err != s3err.ErrNone { + return identity, s3Err, reqAuthType + } prefix := s3_constants.GetPrefix(r) // For bucket listings, use prefix for permission checking if available: @@ -1873,7 +1880,7 @@ func (iam *IdentityAccessManagement) authRequestWithAuthType(r *http.Request, ac } } - r.Header.Set(s3_constants.AmzAccountId, identity.Account.Id) + originalRequest.Header.Set(s3_constants.AmzAccountId, identity.Account.Id) return identity, s3err.ErrNone, reqAuthType @@ -2558,6 +2565,7 @@ func (iam *IdentityAccessManagement) evaluateAttachedIAMPolicies(r *http.Request Conditions: conditions, Claims: identity.Claims, } + evalArgs.OriginalGrantConditions = policy_engine.OriginalGrantConditionsFromRequest(r) // Evaluate user's own policies for _, policyName := range identity.PolicyNames { diff --git a/weed/s3api/policy_engine/conditions.go b/weed/s3api/policy_engine/conditions.go index c5ac36fd6..becf8836d 100644 --- a/weed/s3api/policy_engine/conditions.go +++ b/weed/s3api/policy_engine/conditions.go @@ -756,6 +756,12 @@ func getConditionContextValue(key string, contextValues map[string][]string, obj // objectEntry is the object's metadata from entry.Extended (can be nil) // claims are JWT claims for jwt:* policy variables (can be nil) func EvaluateConditions(conditions PolicyConditions, contextValues map[string][]string, objectEntry map[string][]byte, claims map[string]interface{}) bool { + return evaluateConditions(conditions, contextValues, objectEntry, claims, nil) +} + +// evaluateConditions supplements positive string grant conditions in explicit denies with the original complete list. +// Select values per operator so whitespace or escapes cannot change negative conditions on the same key. +func evaluateConditions(conditions PolicyConditions, contextValues map[string][]string, objectEntry map[string][]byte, claims map[string]interface{}, originalGrants map[string][]string) bool { if len(conditions) == 0 { return true // No conditions means always true } @@ -769,6 +775,12 @@ func EvaluateConditions(conditions PolicyConditions, contextValues map[string][] for key, value := range conditionMap { contextVals := getConditionContextValue(key, contextValues, objectEntry) + if original := originalGrants[key]; len(original) != 0 && isGrantConditionKey(key) { + switch operator { + case "StringEquals", "StringEqualsIgnoreCase", "StringLike", "ArnEquals", "ArnLike": + contextVals = append(append([]string(nil), contextVals...), original...) + } + } // Substitute variables in expected values expectedValues := value.Strings() diff --git a/weed/s3api/policy_engine/engine.go b/weed/s3api/policy_engine/engine.go index b3a3048eb..933ed1d96 100644 --- a/weed/s3api/policy_engine/engine.go +++ b/weed/s3api/policy_engine/engine.go @@ -246,6 +246,10 @@ func (engine *PolicyEngine) evaluateStatement(stmt *CompiledStatement, args *Pol condCtx = injectSSEForMultipart(args.Conditions, args.InheritedSSEAlgorithm) } match := EvaluateConditions(stmt.Statement.Condition, condCtx, args.ObjectEntry, args.Claims) + // Preserve positive string denies on the original complete list; allows, negative conditions, and variables remain canonical. + if !match && stmt.Statement.Effect == PolicyEffectDeny && len(args.OriginalGrantConditions) != 0 { + match = evaluateConditions(stmt.Statement.Condition, condCtx, args.ObjectEntry, args.Claims, args.OriginalGrantConditions) + } if !match { return false } diff --git a/weed/s3api/policy_engine/request_grant_conditions.go b/weed/s3api/policy_engine/request_grant_conditions.go new file mode 100644 index 000000000..14fc88408 --- /dev/null +++ b/weed/s3api/policy_engine/request_grant_conditions.go @@ -0,0 +1,46 @@ +package policy_engine + +import ( + "context" + "net/http" +) + +// originalGrantConditionsKey prevents clients from forging original grant values through request headers. +type originalGrantConditionsKey struct{} + +// isGrantConditionKey restricts original-value checks to the five upload grant condition keys. +func isGrantConditionKey(key string) bool { + switch key { + case "s3:x-amz-grant-read", "s3:x-amz-grant-write", "s3:x-amz-grant-read-acp", "s3:x-amz-grant-write-acp", "s3:x-amz-grant-full-control": + return true + } + return false +} + +// cloneGrantConditions isolates inputs and results so later mutations cannot change the original complete grant representation. +func cloneGrantConditions(values map[string][]string) map[string][]string { + if len(values) == 0 { + return nil + } + cloned := make(map[string][]string, len(values)) + for key, grants := range values { + if isGrantConditionKey(key) { + cloned[key] = append([]string(nil), grants...) + } + } + return cloned +} + +// WithOriginalGrantConditions saves the complete list before upload normalization to supplement explicit deny checks only. +func WithOriginalGrantConditions(r *http.Request, values map[string][]string) *http.Request { + return r.WithContext(context.WithValue(r.Context(), originalGrantConditionsKey{}, cloneGrantConditions(values))) +} + +// OriginalGrantConditionsFromRequest reads the internal snapshot; other operations have no such context. +func OriginalGrantConditionsFromRequest(r *http.Request) map[string][]string { + if r == nil { + return nil + } + values, _ := r.Context().Value(originalGrantConditionsKey{}).(map[string][]string) + return cloneGrantConditions(values) +} diff --git a/weed/s3api/policy_engine/request_grant_conditions_test.go b/weed/s3api/policy_engine/request_grant_conditions_test.go new file mode 100644 index 000000000..bd017af71 --- /dev/null +++ b/weed/s3api/policy_engine/request_grant_conditions_test.go @@ -0,0 +1,63 @@ +package policy_engine + +import ( + "fmt" + "net/http/httptest" + "testing" + + "github.com/stretchr/testify/require" +) + +// TestOriginalGrantConditionsDeny verifies original positive denies without expanding allows or negative conditions. +func TestOriginalGrantConditionsDeny(t *testing.T) { + const key = "s3:x-amz-grant-read" + const canonical, original = `id="bucket-owner"`, `id = "bucket-\u006fwner"` + tests := []struct { + name, effect, conditions string + result PolicyEvaluationResult + }{ + {"original exact deny", "Deny", fmt.Sprintf(`{"StringEquals":{%q:%q}}`, key, original), PolicyResultDeny}, + {"original wildcard deny", "Deny", fmt.Sprintf(`{"StringLike":{%q:%q}}`, key, `id = *`), PolicyResultDeny}, + {"canonical deny unchanged", "Deny", fmt.Sprintf(`{"StringEquals":{%q:%q}}`, key, canonical), PolicyResultDeny}, + {"approved negative condition not denied", "Deny", fmt.Sprintf(`{"StringNotEquals":{%q:%q}}`, key, canonical), PolicyResultIndeterminate}, + {"negative deny unchanged", "Deny", fmt.Sprintf(`{"StringNotEquals":{%q:%q}}`, key, `id="other"`), PolicyResultDeny}, + {"original value cannot allow", "Allow", fmt.Sprintf(`{"StringEquals":{%q:%q}}`, key, original), PolicyResultIndeterminate}, + {"negative allow not expanded", "Allow", fmt.Sprintf(`{"StringNotEquals":{%q:%q}}`, key, canonical), PolicyResultIndeterminate}, + {"complete list allow unchanged", "Allow", fmt.Sprintf(`{"StringEquals":{%q:%q}}`, key, canonical), PolicyResultAllow}, + {"positive and negative conditions evaluated separately", "Deny", fmt.Sprintf(`{"StringEquals":{%q:%q},"StringNotEquals":{%q:%q}}`, key, original, key, canonical), PolicyResultIndeterminate}, + {"other conditions must still match", "Deny", fmt.Sprintf(`{"StringEquals":{%q:%q,"aws:username":"other"}}`, key, original), PolicyResultIndeterminate}, + {"variables remain canonical", "Deny", fmt.Sprintf(`{"StringEquals":{%q:"${s3:x-amz-grant-read}"}}`, key), PolicyResultDeny}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + engine := NewPolicyEngine() + policy := fmt.Sprintf(`{"Version":"2012-10-17","Statement":[{"Effect":%q,"Principal":"*","Action":"s3:PutObjectAcl","Resource":"arn:aws:s3:::bucket/object","Condition":%s}]}`, tt.effect, tt.conditions) + require.NoError(t, engine.SetBucketPolicy("bucket", policy)) + req := WithOriginalGrantConditions(httptest.NewRequest("PUT", "/bucket/object", nil), map[string][]string{key: {original}}) + args := &PolicyEvaluationArgs{ + Action: "s3:PutObjectAcl", Resource: "arn:aws:s3:::bucket/object", Principal: "upload-writer", + Conditions: map[string][]string{key: {canonical}, "aws:username": {"upload-writer"}}, + OriginalGrantConditions: OriginalGrantConditionsFromRequest(req), + } + require.Equal(t, tt.result, engine.EvaluatePolicy("bucket", args)) + }) + } +} + +// TestOriginalGrantConditionsSnapshot verifies original values cannot be forged through headers or later mutations. +func TestOriginalGrantConditionsSnapshot(t *testing.T) { + const key = "s3:x-amz-grant-read" + values := map[string][]string{key: {`id = "bucket-owner"`}, "aws:username": {"forged"}} + original := httptest.NewRequest("PUT", "/bucket/object", nil) + original.Header.Set("X-Amz-Original-Grant-Read", `id="attacker"`) + require.Nil(t, OriginalGrantConditionsFromRequest(original)) + require.Nil(t, OriginalGrantConditionsFromRequest(nil)) + req := WithOriginalGrantConditions(original, values) + values[key][0] = `id="attacker"` + delete(values, key) + first := OriginalGrantConditionsFromRequest(req) + require.Equal(t, `id = "bucket-owner"`, first[key][0]) + require.NotContains(t, first, "aws:username") + first[key][0] = `id="attacker"` + require.Equal(t, `id = "bucket-owner"`, OriginalGrantConditionsFromRequest(req)[key][0]) +} diff --git a/weed/s3api/policy_engine/types.go b/weed/s3api/policy_engine/types.go index fe61b0263..2979e21c4 100644 --- a/weed/s3api/policy_engine/types.go +++ b/weed/s3api/policy_engine/types.go @@ -315,6 +315,9 @@ type PolicyEvaluationArgs struct { // inherited from the CreateMultipartUpload request for UploadPart and // UploadPartCopy actions. The empty string means no SSE was used. InheritedSSEAlgorithm string + + // Original complete grant values supplement only positive string conditions in explicit denies, never allows or negative conditions. + OriginalGrantConditions map[string][]string } // PolicyCache for caching compiled policies diff --git a/weed/s3api/s3_action_resolver.go b/weed/s3api/s3_action_resolver.go index 8f86eb8f1..04dd8769a 100644 --- a/weed/s3api/s3_action_resolver.go +++ b/weed/s3api/s3_action_resolver.go @@ -141,7 +141,9 @@ func resolveFromQueryParameters(query url.Values, method string, hasObject bool) if hasObject && query.Has("uploadId") { switch method { case http.MethodPut: - if query.Has("partNumber") { + // Match the multipart route's [0-9]+ pattern; invalid part parameters fall through to regular uploads. + partNumber := query.Get("partNumber") + if partNumber != "" && strings.Trim(partNumber, "0123456789") == "" { return s3_constants.S3_ACTION_UPLOAD_PART } case http.MethodPost: diff --git a/weed/s3api/s3api_acl_header_parser_test.go b/weed/s3api/s3api_acl_header_parser_test.go new file mode 100644 index 000000000..2b3932dfc --- /dev/null +++ b/weed/s3api/s3api_acl_header_parser_test.go @@ -0,0 +1,61 @@ +package s3api + +import ( + "testing" + + "github.com/aws/aws-sdk-go/aws" + "github.com/aws/aws-sdk-go/service/s3" + "github.com/seaweedfs/seaweedfs/weed/s3api/s3_constants" + "github.com/seaweedfs/seaweedfs/weed/s3api/s3err" + "github.com/stretchr/testify/require" +) + +// TestParseCustomAclHeaderList covers the wire syntax separately from account +// resolution, including quoted delimiters and rejection without partial grants. +func TestParseCustomAclHeaderList(t *testing.T) { + tests := []struct { + name, input string + values []string + invalid bool + }{ + {name: "absent"}, + {name: "single", input: `id="alice"`, values: []string{"alice"}}, + {name: "comma without space", input: `id="alice",id="bob"`, values: []string{"alice", "bob"}}, + {name: "comma with space", input: `id="alice", id="bob"`, values: []string{"alice", "bob"}}, + {name: "optional whitespace", input: " id = \"alice\" ,\t id=\"bob\" ", values: []string{"alice", "bob"}}, + {name: "quoted comma and equals", input: `id="a,b=c",id="bob"`, values: []string{"a,b=c", "bob"}}, + {name: "escaped quote", input: `id="a\"b",id="bob"`, values: []string{`a"b`, "bob"}}, + {name: "email", input: `emailAddress="a=b@example.com"`, values: []string{"a=b@example.com"}}, + {name: "group", input: `uri="http://acs.amazonaws.com/groups/global/AllUsers"`, values: []string{s3_constants.GranteeGroupAllUsers}}, + {name: "unknown type", input: `account="alice"`, invalid: true}, + {name: "mixed unknown type", input: `id="alice",principal="bob"`, invalid: true}, + {name: "empty grantee", input: `id=""`, invalid: true}, + {name: "unquoted", input: `id=alice`, invalid: true}, + {name: "unterminated", input: `id="alice`, invalid: true}, + {name: "trailing comma", input: `id="alice",`, invalid: true}, + {name: "empty element", input: `id="alice",,id="bob"`, invalid: true}, + {name: "missing comma", input: `id="alice" id="bob"`, invalid: true}, + {name: "invalid escape", input: `id="a\q"`, invalid: true}, + {name: "whitespace only", input: " ", invalid: true}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + original := &s3.Grant{Permission: aws.String(s3_constants.PermissionFullControl)} + grants := []*s3.Grant{original} + code := ParseCustomAclHeader(tt.input, s3_constants.PermissionRead, &grants) + if tt.invalid { + require.Equal(t, s3err.ErrInvalidRequest, code) + require.Equal(t, []*s3.Grant{original}, grants, "invalid lists must not leave partial grants") + return + } + require.Equal(t, s3err.ErrNone, code) + require.Len(t, grants, 1+len(tt.values)) + for i, value := range tt.values { + grant := grants[i+1] + actual := aws.StringValue(grant.Grantee.ID) + aws.StringValue(grant.Grantee.EmailAddress) + aws.StringValue(grant.Grantee.URI) + require.Equal(t, value, actual) + require.Equal(t, s3_constants.PermissionRead, aws.StringValue(grant.Permission)) + } + }) + } +} diff --git a/weed/s3api/s3api_acl_helper.go b/weed/s3api/s3api_acl_helper.go index 4ba382bd6..7b9a51007 100644 --- a/weed/s3api/s3api_acl_helper.go +++ b/weed/s3api/s3api_acl_helper.go @@ -130,7 +130,7 @@ func ParseCustomAclHeaders(r *http.Request, grants *[]*s3.Grant) s3err.ErrorCode customAclHeaders := []string{s3_constants.AmzAclFullControl, s3_constants.AmzAclRead, s3_constants.AmzAclReadAcp, s3_constants.AmzAclWrite, s3_constants.AmzAclWriteAcp} var errCode s3err.ErrorCode for _, customAclHeader := range customAclHeaders { - headerValue := r.Header.Get(customAclHeader) + headerValue := strings.Join(r.Header.Values(customAclHeader), ",") switch customAclHeader { case s3_constants.AmzAclRead: errCode = ParseCustomAclHeader(headerValue, s3_constants.PermissionRead, grants) @@ -150,51 +150,64 @@ func ParseCustomAclHeaders(r *http.Request, grants *[]*s3.Grant) s3err.ErrorCode return s3err.ErrNone } -func ParseCustomAclHeader(headerValue, permission string, grants *[]*s3.Grant) s3err.ErrorCode { - if len(headerValue) > 0 { - split := strings.Split(headerValue, ", ") - for _, grantStr := range split { - kv := strings.Split(grantStr, "=") - if len(kv) != 2 { - return s3err.ErrInvalidRequest - } - - switch kv[0] { - case "id": - var accountId string - _ = json.Unmarshal([]byte(kv[1]), &accountId) - *grants = append(*grants, &s3.Grant{ - Grantee: &s3.Grantee{ - Type: &s3_constants.GrantTypeCanonicalUser, - ID: &accountId, - }, - Permission: &permission, - }) - case "emailAddress": - var emailAddress string - _ = json.Unmarshal([]byte(kv[1]), &emailAddress) - *grants = append(*grants, &s3.Grant{ - Grantee: &s3.Grantee{ - Type: &s3_constants.GrantTypeAmazonCustomerByEmail, - EmailAddress: &emailAddress, - }, - Permission: &permission, - }) - case "uri": - var groupName string - _ = json.Unmarshal([]byte(kv[1]), &groupName) - *grants = append(*grants, &s3.Grant{ - Grantee: &s3.Grantee{ - Type: &s3_constants.GrantTypeGroup, - URI: &groupName, - }, - Permission: &permission, - }) - } - } +// parseAclGranteePairs decodes a comma-separated list of quoted grantees into +// key/value pairs. Decoding each value before splitting keeps commas and equals +// signs inside quotes intact. +func parseAclGranteePairs(headerValue string) (pairs [][2]string, errCode s3err.ErrorCode) { + if headerValue == "" { + return nil, s3err.ErrNone } - return s3err.ErrNone + remaining := strings.TrimSpace(headerValue) + for { + key, encoded, ok := strings.Cut(remaining, "=") + if !ok { + return nil, s3err.ErrInvalidRequest + } + decoder := json.NewDecoder(strings.NewReader(encoded)) + var value string + if decoder.Decode(&value) != nil || value == "" { + return nil, s3err.ErrInvalidRequest + } + key = strings.TrimSpace(key) + switch key { + case "id", "emailAddress", "uri": + default: + return nil, s3err.ErrInvalidRequest + } + pairs = append(pairs, [2]string{key, value}) + remaining = strings.TrimSpace(encoded[decoder.InputOffset():]) + if remaining == "" { + break + } + if remaining[0] != ',' { + return nil, s3err.ErrInvalidRequest + } + remaining = strings.TrimSpace(remaining[1:]) + } + return pairs, s3err.ErrNone +} +func ParseCustomAclHeader(headerValue, permission string, grants *[]*s3.Grant) s3err.ErrorCode { + pairs, errCode := parseAclGranteePairs(headerValue) + if errCode != s3err.ErrNone { + return errCode + } + var parsed []*s3.Grant + for i := range pairs { + grantee := &s3.Grantee{} + switch pairs[i][0] { + case "id": + grantee.Type, grantee.ID = &s3_constants.GrantTypeCanonicalUser, &pairs[i][1] + case "emailAddress": + grantee.Type, grantee.EmailAddress = &s3_constants.GrantTypeAmazonCustomerByEmail, &pairs[i][1] + case "uri": + grantee.Type, grantee.URI = &s3_constants.GrantTypeGroup, &pairs[i][1] + } + parsed = append(parsed, &s3.Grant{Grantee: grantee, Permission: &permission}) + } + // Do not leave partially parsed grants behind when any list element fails. + *grants = append(*grants, parsed...) + return s3err.ErrNone } func ParseCannedAclHeader(bucketOwnership, bucketOwnerId, accountId, cannedAcl string, putAcl bool) (ownerId string, grants []*s3.Grant, err s3err.ErrorCode) { diff --git a/weed/s3api/s3api_bucket_policy_engine.go b/weed/s3api/s3api_bucket_policy_engine.go index cfc5bdf5c..8552ccc3a 100644 --- a/weed/s3api/s3api_bucket_policy_engine.go +++ b/weed/s3api/s3api_bucket_policy_engine.go @@ -145,6 +145,7 @@ func (bpe *BucketPolicyEngine) EvaluatePolicy(bucket, object, action, principal // Extract conditions and claims from request if available if r != nil { + args.OriginalGrantConditions = policy_engine.OriginalGrantConditionsFromRequest(r) args.Conditions = bpe.engine.ExtractConditionValuesFromRequest(r) // Extract principal-related variables (aws:username, etc.) from principal ARN diff --git a/weed/s3api/s3api_object_handlers_put.go b/weed/s3api/s3api_object_handlers_put.go index 45388a0ba..b92d5b095 100644 --- a/weed/s3api/s3api_object_handlers_put.go +++ b/weed/s3api/s3api_object_handlers_put.go @@ -147,6 +147,13 @@ func (s3a *S3ApiServer) PutObjectHandler(w http.ResponseWriter, r *http.Request) return } + var aclCode s3err.ErrorCode + r, aclCode = s3a.preparePutObjectACL(r, bucket) + if aclCode != s3err.ErrNone { + s3err.WriteErrorResponse(w, r, aclCode) + return + } + objectLockEnabled, lockErr := s3a.isObjectLockEnabled(bucket) if lockErr != nil && !errors.Is(lockErr, filer_pb.ErrNotFound) { glog.Errorf("PutObjectHandler: failed to check object lock for bucket %s: %v", bucket, lockErr) @@ -230,6 +237,7 @@ func (s3a *S3ApiServer) PutObjectHandler(w http.ResponseWriter, r *http.Request) // Set object owner for directory objects (same as regular objects) s3a.setObjectOwnerFromRequest(r, bucket, entry) + applyPutObjectACL(r, entry) if lockErr := s3a.extractObjectLockMetadataFromRequest(r, entry); lockErr != nil { glog.Errorf("PutObjectHandler: failed to extract object lock metadata for %s/%s: %v", bucket, object, lockErr) @@ -268,6 +276,13 @@ func (s3a *S3ApiServer) PutObjectHandler(w http.ResponseWriter, r *http.Request) } } + var aclCode s3err.ErrorCode + r, aclCode = s3a.preparePutObjectACL(r, bucket) + if aclCode != s3err.ErrNone { + s3err.WriteErrorResponse(w, r, aclCode) + return + } + versioningEnabled := (versioningState == s3_constants.VersioningEnabled) versioningConfigured := (versioningState != "") @@ -796,6 +811,7 @@ func (s3a *S3ApiServer) putToFiler(r *http.Request, filePath string, dataReader // Set object owner according to bucket ownership settings. s3a.setObjectOwnerFromRequest(r, bucket, entry) + applyPutObjectACL(r, entry) // Set version ID if present. It is later used as a filer path segment, so a // value carrying "/", "\\" or ".." must never be stored. @@ -1355,9 +1371,14 @@ func detectRequestedChecksumAlgorithmQ(r *http.Request, query url.Values) (Check const defaultFileMode = uint32(0660) // resolveFileMode determines the file permission mode for an S3 upload. -// Priority: per-object X-Amz-Acl header > server default > defaultFileMode. +// Priority: validated PUT ACL > X-Amz-Acl header > server default > defaultFileMode. func (s3a *S3ApiServer) resolveFileMode(r *http.Request) uint32 { - if cannedAcl := r.Header.Get(s3_constants.AmzCannedAcl); cannedAcl != "" { + cannedAcl := r.Header.Get(s3_constants.AmzCannedAcl) + if metadata, ok := r.Context().Value(putObjectACLContextKey{}).(putObjectACLMetadata); ok { + // Signed query ACLs must resolve identically to signed ACL headers. + cannedAcl = metadata.canned + } + if cannedAcl != "" { switch cannedAcl { case s3_constants.CannedAclPublicRead, s3_constants.CannedAclAuthenticatedRead, s3_constants.CannedAclBucketOwnerRead: diff --git a/weed/s3api/s3api_object_upload_acl.go b/weed/s3api/s3api_object_upload_acl.go new file mode 100644 index 000000000..a87977e3e --- /dev/null +++ b/weed/s3api/s3api_object_upload_acl.go @@ -0,0 +1,265 @@ +package s3api + +import ( + "context" + "encoding/json" + "net/http" + "net/url" + "strings" + + "github.com/aws/aws-sdk-go/service/s3" + "github.com/seaweedfs/seaweedfs/weed/pb/filer_pb" + "github.com/seaweedfs/seaweedfs/weed/s3api/policy_engine" + "github.com/seaweedfs/seaweedfs/weed/s3api/s3_constants" + "github.com/seaweedfs/seaweedfs/weed/s3api/s3err" +) + +// putObjectACLContextKey carries validated ACL metadata to every PutObject write +// path without exposing an internal header that a client could forge. +type putObjectACLContextKey struct{} + +type putObjectACLMetadata struct { + extended map[string][]byte + canned string +} + +// putObjectACLValue ignores unsigned V2 query ACLs and rejects ambiguity. +// Headers stay untouched because authentication verifies the original request. +func putObjectACLValue(r *http.Request, query url.Values, header string) (string, s3err.ErrorCode) { + // Preserve SigV2's header-only ACL behavior: arbitrary query parameters + // are not in its canonical resource and must have no effect on grants. + switch getRequestAuthType(r) { + case authTypeSignedV2, authTypePresignedV2: + query = nil + } + var queryValues []string + queryPresent := false + for key, values := range query { + if strings.EqualFold(key, header) { + queryPresent = true + queryValues = append(queryValues, values...) + } + } + if queryPresent { + if len(queryValues) != 1 { + // V4 sorts duplicate values when signing. Choosing the first value + // would let reordering change the effective ACL without resigning. + return "", s3err.ErrInvalidRequest + } + } + values := r.Header.Values(header) + if header == s3_constants.AmzCannedAcl && len(values) > 1 { + return "", s3err.ErrInvalidRequest + } + value := strings.Join(values, ",") + if queryPresent { + if len(values) > 0 && value != queryValues[0] { + return "", s3err.ErrInvalidRequest + } + value = queryValues[0] + } + return value, s3err.ErrNone +} + +// putObjectACLPolicyRequest exposes effective PUT ACLs to policy conditions only +// after authentication. Other operations keep their original request semantics. +func putObjectACLPolicyRequest(r *http.Request, action Action, bucket, object string) (*http.Request, s3err.ErrorCode) { + // Copy routes match any repeated header value, so checking only the first line can misclassify a copy as a regular upload. + copyRequest := false + for _, copySource := range r.Header.Values("X-Amz-Copy-Source") { + if strings.Contains(copySource, "/") || strings.Contains(strings.ToLower(copySource), "%2f") { + copyRequest = true + break + } + } + if (action != s3_constants.ACTION_WRITE && action != s3_constants.ACTION_WRITE_ACP) || + r.Method != http.MethodPut || object == "" || object == "/" || + copyRequest || + ResolveS3Action(r, string(s3_constants.ACTION_WRITE), bucket, object) != s3_constants.S3_ACTION_PUT_OBJECT { + return r, s3err.ErrNone + } + // Rechecks reuse the normalized internal request, preserving signed original values without false query conflicts. + if len(policy_engine.OriginalGrantConditionsFromRequest(r)) != 0 { + return r, s3err.ErrNone + } + policyRequest := r.Clone(r.Context()) + query := parseRequestQuery(r) + originalGrants := make(map[string][]string) + for _, header := range []string{s3_constants.AmzCannedAcl, s3_constants.AmzAclFullControl, s3_constants.AmzAclRead, s3_constants.AmzAclReadAcp, s3_constants.AmzAclWrite, s3_constants.AmzAclWriteAcp} { + value, code := putObjectACLValue(r, query, header) + if code != s3err.ErrNone { + return r, code + } + if value == "" { + continue + } + if header == s3_constants.AmzCannedAcl { + policyRequest.Header.Set(header, value) + continue + } + // Preserve only the complete effective list for original-string denies, not individual header lines as separate lists. + originalGrants["s3:"+strings.ToLower(header)] = []string{value} + // Policy conditions see the canonical grant list: one comma-separated + // value covering every persisted grantee, identical for a single line, + // repeated lines, or a signed query parameter. Sneaking an extra grantee + // past a StringEquals allow or a StringNotEquals allowlist deny requires + // changing this value, which a signed request cannot do. + pairs, pairCode := parseAclGranteePairs(value) + if pairCode != s3err.ErrNone { + return r, pairCode + } + var tokens []string + for _, pair := range pairs { + // Grant conditions use JSON quoting without HTML escaping, so valid + // literal characters in an account or email still match the policy. + var encoded strings.Builder + encoder := json.NewEncoder(&encoded) + encoder.SetEscapeHTML(false) + if err := encoder.Encode(pair[1]); err != nil { + return r, s3err.ErrInvalidRequest + } + tokens = append(tokens, pair[0]+"="+strings.TrimSuffix(encoded.String(), "\n")) + } + policyRequest.Header.Set(header, strings.Join(tokens, ",")) + } + if len(originalGrants) != 0 { + policyRequest = policy_engine.WithOriginalGrantConditions(policyRequest, originalGrants) + } + return policyRequest, s3err.ErrNone +} + +// preparePutObjectACL validates and authorizes ACLs before the upload body is +// consumed. The resulting metadata is committed in the same entry as the object. +func (s3a *S3ApiServer) preparePutObjectACL(r *http.Request, bucket string) (*http.Request, s3err.ErrorCode) { + metadata, code := s3a.getBucketConfig(bucket) + if code != s3err.ErrNone { + return r, code + } + if metadata == nil || s3a.iam == nil { + return r, s3err.ErrInternalError + } + + // Presigners can hoist ACL headers into the signed query string. Normalize a + // separate request for parsing, preserving the original for signature checks. + aclRequest := r.Clone(r.Context()) + query := parseRequestQuery(r) + custom := false + for _, header := range []string{s3_constants.AmzAclFullControl, s3_constants.AmzAclRead, s3_constants.AmzAclReadAcp, s3_constants.AmzAclWrite, s3_constants.AmzAclWriteAcp} { + value, code := putObjectACLValue(r, query, header) + if code != s3err.ErrNone { + return r, code + } + if value != "" { + custom = true + aclRequest.Header.Set(header, value) + } + } + canned, code := putObjectACLValue(r, query, s3_constants.AmzCannedAcl) + if code != s3err.ErrNone { + return r, code + } + aclRequest.Header.Set(s3_constants.AmzCannedAcl, canned) + explicit := canned != "" || custom + accountID := r.Header.Get(s3_constants.AmzAccountId) + if !s3a.iam.isEnabled() { + accountID = AccountAdmin.Id + } else if explicit { + // Setting an ACL during PutObject also requires s3:PutObjectAcl. Use the + // unified authorization path so bucket-policy allows and explicit denies + // retain the same semantics as standalone ACL requests. + identity, authCode := s3a.iam.authRequest(r.Clone(r.Context()), s3_constants.ACTION_WRITE_ACP) + if authCode != s3err.ErrNone { + return r, authCode + } + if identity == nil || identity.Account == nil { + return r, s3err.ErrAccessDenied + } + accountID = identity.Account.Id + } + if explicit && !s3a.iam.isEnabled() { + _, object := s3_constants.GetBucketAndObject(r) + policyRequest, policyCode := putObjectACLPolicyRequest(r, s3_constants.ACTION_WRITE, bucket, object) + if policyCode != s3err.ErrNone { + return r, policyCode + } + for _, action := range []Action{s3_constants.ACTION_WRITE, s3_constants.ACTION_WRITE_ACP} { + if policyCode, _ := s3a.checkPolicyWithEntry(policyRequest, bucket, object, string(action), "", nil); policyCode != s3err.ErrNone { + return r, policyCode + } + } + } + if accountID == "" { + return r, s3err.ErrAccessDenied + } + if canned != "" && custom { + return r, s3err.ErrInvalidRequest + } + + bucketOwner := metadata.Owner + if bucketOwner == "" { + // Buckets created outside S3 can have no recorded owner, matching the + // bucket registry's existing admin fallback for these entries. + bucketOwner = AccountAdmin.Id + } + ownership := s3_constants.EffectiveOwnership(metadata.Ownership) + if ownership == s3_constants.OwnershipBucketOwnerEnforced { + if metadata.Ownership == s3_constants.OwnershipBucketOwnerEnforced { + // Keep legacy buckets without recorded ownership controls accepting + // ACLs; only an explicitly configured enforced control disables them. + if custom || (canned != "" && canned != s3_constants.CannedAclBucketOwnerFullControl) { + return r, s3err.ErrAccessControlListNotSupported + } + aclRequest.Header.Set(s3_constants.AmzCannedAcl, s3_constants.CannedAclPrivate) + } + accountID = bucketOwner + } + if aclRequest.Header.Get(s3_constants.AmzCannedAcl) == "" && !custom { + aclRequest.Header.Set(s3_constants.AmzCannedAcl, s3_constants.CannedAclPrivate) + } + // Canned grants contain only authenticated writer and recorded bucket-owner + // IDs. Dynamic IAM/JWT accounts need not exist in the static account directory. + // Client-supplied custom grantees must still pass directory validation. + owner, grants, code := ParseAclHeaders(aclRequest, ownership, bucketOwner, accountID, false) + if code == s3err.ErrNone && custom { + grants, code = ValidateAndTransferGrants(s3a.iam, grants) + } + if code != s3err.ErrNone { + return r, code + } + if custom { + // Custom upload grants supplement the owner's default full control. + // Check after email resolution to avoid duplicating an explicit owner + // grant; the authenticated owner need not be in the static directory. + ownerFullControl := false + for _, grant := range grants { + if grant.Grantee != nil && grant.Grantee.Type != nil && + *grant.Grantee.Type == s3_constants.GrantTypeCanonicalUser && + grant.Grantee.ID != nil && *grant.Grantee.ID == owner && + grant.Permission != nil && *grant.Permission == s3_constants.PermissionFullControl { + ownerFullControl = true + break + } + } + if !ownerFullControl { + grants = append(grants, &s3.Grant{ + Grantee: &s3.Grantee{Type: &s3_constants.GrantTypeCanonicalUser, ID: &owner}, + Permission: &s3_constants.PermissionFullControl, + }) + } + } + entry := &filer_pb.Entry{} + if code = AssembleEntryWithAcp(entry, owner, grants); code != s3err.ErrNone { + return r, code + } + prepared := putObjectACLMetadata{extended: entry.Extended, canned: canned} + return r.WithContext(context.WithValue(r.Context(), putObjectACLContextKey{}, prepared)), s3err.ErrNone +} + +// applyPutObjectACL adds prevalidated ownership and grants before CreateEntry. +// Multipart parts and POST form uploads do not carry this PutObject context. +func applyPutObjectACL(r *http.Request, entry *filer_pb.Entry) { + metadata, _ := r.Context().Value(putObjectACLContextKey{}).(putObjectACLMetadata) + for key, value := range metadata.extended { + entry.Extended[key] = value + } +} diff --git a/weed/s3api/s3api_object_upload_acl_test.go b/weed/s3api/s3api_object_upload_acl_test.go new file mode 100644 index 000000000..2d6bcf93f --- /dev/null +++ b/weed/s3api/s3api_object_upload_acl_test.go @@ -0,0 +1,706 @@ +package s3api + +import ( + "crypto/md5" + "encoding/base64" + "encoding/xml" + "fmt" + "hash/crc32" + "net/http" + "net/http/httptest" + "net/url" + "strings" + "testing" + "time" + + "github.com/aws/aws-sdk-go/aws" + "github.com/aws/aws-sdk-go/aws/credentials" + v4 "github.com/aws/aws-sdk-go/aws/signer/v4" + "github.com/gorilla/mux" + "github.com/seaweedfs/seaweedfs/weed/pb/filer_pb" + "github.com/seaweedfs/seaweedfs/weed/s3api/s3_constants" + "github.com/seaweedfs/seaweedfs/weed/s3api/s3err" + "github.com/stretchr/testify/require" + "google.golang.org/protobuf/proto" +) + +// TestPutObjectUploadACL exercises signed uploads and inspects the actual filer +// entry, including rejection before any volume allocation or object replacement. +func TestPutObjectUploadACL(t *testing.T) { + const bucket, object, writer, bucketOwner = "acl-bucket", "allowed/image.png", "upload-writer", "bucket-owner" + type uploadACLTest struct { + name, acl, grantHeader, grant, ownership, policy, versioning, errorCode string + writeOnly, wrongScope, marker, presigned, overwrite, unsigned, streaming bool + status int + signature string + query, afterSigning url.Values + grantees []string + repeatedGrant string + conditionValue string + conditionOperator string + defaultMode uint32 + unregisteredAccounts bool + policyOnly bool + route bool + copySource string + grantAccount, grantEmail string + unregisteredWriter bool + } + tests := []uploadACLTest{ + {name: "default private", status: 200}, + {name: "explicit private", acl: "private", status: 200}, + {name: "public read", acl: "public-read", status: 200}, + {name: "public read write", acl: "public-read-write", status: 200}, + {name: "authenticated read", acl: "authenticated-read", status: 200}, + {name: "custom read", grantHeader: s3_constants.AmzAclRead, grant: `id="bucket-owner"`, status: 200}, + {name: "custom write", grantHeader: s3_constants.AmzAclWrite, grant: `id="bucket-owner"`, status: 200}, + {name: "custom read acp", grantHeader: s3_constants.AmzAclReadAcp, grant: `id="bucket-owner"`, status: 200}, + {name: "custom write acp", grantHeader: s3_constants.AmzAclWriteAcp, grant: `id="bucket-owner"`, status: 200}, + {name: "custom full control", grantHeader: s3_constants.AmzAclFullControl, grant: `id="bucket-owner"`, status: 200}, + {name: "custom owner full control is not duplicated", grantHeader: s3_constants.AmzAclFullControl, grant: `id="upload-writer"`, grantees: []string{writer}, status: 200}, + {name: "custom owner email full control is not duplicated", grantHeader: s3_constants.AmzAclFullControl, grant: `emailAddress="writer@example.com"`, grantEmail: "writer@example.com", grantAccount: writer, grantees: []string{writer}, status: 200}, + {name: "custom owner read retains full control", grantHeader: s3_constants.AmzAclRead, grant: `id="upload-writer"`, grantees: []string{writer}, status: 200}, + {name: "custom dynamic writer keeps full control", grantHeader: s3_constants.AmzAclRead, grant: `id="bucket-owner"`, unregisteredWriter: true, status: 200}, + {name: "unknown grantee", grantHeader: s3_constants.AmzAclRead, grant: `id="unknown"`, status: 400, errorCode: "InvalidRequest"}, + {name: "unknown canned acl", acl: "invalid", status: 400, errorCode: "InvalidRequest"}, + {name: "conflicting acl headers", acl: "public-read", grantHeader: s3_constants.AmzAclRead, grant: `id="bucket-owner"`, status: 400, errorCode: "InvalidRequest"}, + {name: "bucket owner read", acl: "bucket-owner-read", status: 200}, + {name: "bucket owner full control", acl: "bucket-owner-full-control", status: 200}, + {name: "preferred ownership", acl: "bucket-owner-full-control", ownership: s3_constants.OwnershipBucketOwnerPreferred, status: 200}, + {name: "preferred default private", ownership: s3_constants.OwnershipBucketOwnerPreferred, status: 200}, + {name: "enforced default", ownership: s3_constants.OwnershipBucketOwnerEnforced, status: 200}, + {name: "enforced full control", acl: "bucket-owner-full-control", ownership: s3_constants.OwnershipBucketOwnerEnforced, status: 200}, + {name: "enforced rejects private", acl: "private", ownership: s3_constants.OwnershipBucketOwnerEnforced, status: 400, errorCode: "AccessControlListNotSupported"}, + {name: "enforced rejects public", acl: "public-read", ownership: s3_constants.OwnershipBucketOwnerEnforced, status: 400, errorCode: "AccessControlListNotSupported"}, + {name: "enforced rejects grants", grantHeader: s3_constants.AmzAclRead, grant: `id="bucket-owner"`, ownership: s3_constants.OwnershipBucketOwnerEnforced, status: 400, errorCode: "AccessControlListNotSupported"}, + {name: "write only default", writeOnly: true, status: 200}, + {name: "write only rejects explicit private", acl: "private", writeOnly: true, status: 403, errorCode: "AccessDenied"}, + {name: "write only rejects public", acl: "public-read", writeOnly: true, status: 403, errorCode: "AccessDenied"}, + {name: "write only rejects grants", grantHeader: s3_constants.AmzAclRead, grant: `id="bucket-owner"`, writeOnly: true, status: 403, errorCode: "AccessDenied"}, + {name: "acl permission outside prefix", acl: "public-read", wrongScope: true, status: 403, errorCode: "AccessDenied"}, + {name: "iam allows acl", acl: "public-read", writeOnly: true, policy: "iam-allow", status: 200}, + {name: "iam denies acl", acl: "public-read", policy: "iam-deny", status: 403, errorCode: "AccessDenied"}, + {name: "bucket allows acl", acl: "public-read", writeOnly: true, policy: "bucket-allow", status: 200}, + {name: "bucket denies acl", acl: "public-read", policy: "bucket-deny", status: 403, errorCode: "AccessDenied"}, + {name: "presigned public read", acl: "public-read", presigned: true, status: 200}, + {name: "presigned requires acl permission", acl: "public-read", presigned: true, writeOnly: true, status: 403, errorCode: "AccessDenied"}, + {name: "presigned custom read", grantHeader: s3_constants.AmzAclRead, grant: `id="bucket-owner"`, presigned: true, status: 200}, + {name: "unsigned with authentication disabled", acl: "public-read", unsigned: true, status: 200}, + {name: "streaming unsigned payload", acl: "public-read", streaming: true, status: 200}, + {name: "directory marker", acl: "public-read", marker: true, status: 200}, + {name: "directory marker rejects acl", acl: "public-read", marker: true, writeOnly: true, status: 403, errorCode: "AccessDenied"}, + {name: "suspended version", acl: "public-read", versioning: s3_constants.VersioningSuspended, status: 200}, + {name: "enabled version", acl: "public-read", versioning: s3_constants.VersioningEnabled, status: 200}, + {name: "overwrite resets private", overwrite: true, status: 200}, + {name: "rejected overwrite preserves acl", overwrite: true, acl: "invalid", status: 400, errorCode: "InvalidRequest"}, + // Preserve the upstream review fixes and legacy ownership behavior. + {name: "multi grantee without space", grantHeader: s3_constants.AmzAclRead, grant: `id="bucket-owner",id="upload-writer"`, grantees: []string{bucketOwner, writer}, status: 200}, + {name: "unknown grantee key", grantHeader: s3_constants.AmzAclRead, grant: `account="bucket-owner"`, status: 400, errorCode: "InvalidRequest"}, + {name: "absent ownership default", ownership: "absent", status: 200}, + {name: "absent ownership public read", acl: "public-read", ownership: "absent", status: 200}, + {name: "absent ownership grants", grantHeader: s3_constants.AmzAclRead, grant: `id="bucket-owner"`, ownership: "absent", status: 200}, + {name: "sigv2 ignores unsigned query acl", signature: "v2", afterSigning: url.Values{"X-Amz-Acl": {"public-read"}}, status: 200}, + {name: "external account uploader", acl: "public-read", unregisteredAccounts: true, status: 200}, + {name: "unknown grant type", grantHeader: s3_constants.AmzAclRead, grant: `account="bucket-owner"`, status: 400, errorCode: "InvalidRequest"}, + {name: "mixed unknown grant type", grantHeader: s3_constants.AmzAclRead, grant: `id="bucket-owner",principal="upload-writer"`, status: 400, errorCode: "InvalidRequest"}, + {name: "multiple grants without spaces", grantHeader: s3_constants.AmzAclRead, grant: `id="bucket-owner",id="upload-writer"`, grantees: []string{bucketOwner, writer}, status: 200}, + {name: "repeated grant headers", grantHeader: s3_constants.AmzAclRead, grant: `id="bucket-owner"`, repeatedGrant: `id="upload-writer"`, grantees: []string{bucketOwner, writer}, status: 200}, + {name: "presigned multiple grants", grantHeader: s3_constants.AmzAclRead, grant: `id="bucket-owner",id="upload-writer"`, grantees: []string{bucketOwner, writer}, presigned: true, status: 200}, + {name: "presigned public read write", acl: "public-read-write", presigned: true, status: 200}, + {name: "presigned public version", acl: "public-read", presigned: true, versioning: s3_constants.VersioningEnabled, status: 200}, + {name: "default server mode", defaultMode: 0600, status: 200}, + {name: "enforced default server mode", ownership: s3_constants.OwnershipBucketOwnerEnforced, defaultMode: 0600, status: 200}, + {name: "v2 signed header", signature: "v2", acl: "public-read", status: 200}, + {name: "v2 presigned default", signature: "v2", presigned: true, status: 200}, + {name: "v2 presigned signed header", signature: "v2-header", presigned: true, acl: "public-read", status: 200}, + {name: "v2 unsigned canned query", signature: "v2", presigned: true, acl: "public-read", status: 200}, + {name: "v2 unsigned grant query", signature: "v2", presigned: true, grantHeader: s3_constants.AmzAclRead, grant: `id="bucket-owner"`, status: 200}, + {name: "v2 tampered mixed case query", signature: "v2", afterSigning: url.Values{"x-AMZ-aCl": {"public-read"}}, status: 200}, + {name: "v2 tampered presigned query", signature: "v2", presigned: true, afterSigning: url.Values{"x-amz-acl": {"public-read"}}, status: 200}, + {name: "v2 empty acl query", signature: "v2", afterSigning: url.Values{"x-amz-acl": {""}}, status: 200}, + {name: "v2 header cannot hide unsigned query", signature: "v2", acl: "private", afterSigning: url.Values{"x-amz-acl": {"public-read"}}, status: 200}, + {name: "v2 fake v4 marker cannot bypass", signature: "v2", afterSigning: url.Values{"x-amz-acl": {"public-read"}, "X-Amz-Credential": {"fake"}}, status: 200}, + {name: "v4 duplicate canned query", presigned: true, query: url.Values{"X-Amz-Acl": {"private", "public-read"}}, afterSigning: url.Values{"X-Amz-Acl": {"public-read", "private"}}, status: 400, errorCode: "InvalidRequest"}, + {name: "v4 case alias query", presigned: true, query: url.Values{"X-Amz-Acl": {"private"}, "x-amz-acl": {"public-read"}}, status: 400, errorCode: "InvalidRequest"}, + {name: "v4 conflicting header query", acl: "private", query: url.Values{"X-Amz-Acl": {"public-read"}}, status: 400, errorCode: "InvalidRequest"}, + {name: "v4 tampered signed query", presigned: true, acl: "private", afterSigning: url.Values{"X-Amz-Acl": {"public-read"}}, status: 403, errorCode: "SignatureDoesNotMatch"}, + {name: "dynamic default writer", unregisteredAccounts: true, status: 200}, + {name: "dynamic public writer", unregisteredAccounts: true, acl: "public-read", status: 200}, + {name: "dynamic bucket owner", unregisteredAccounts: true, acl: "bucket-owner-full-control", status: 200}, + {name: "dynamic preferred owner", unregisteredAccounts: true, acl: "bucket-owner-full-control", ownership: s3_constants.OwnershipBucketOwnerPreferred, status: 200}, + {name: "dynamic enforced default", unregisteredAccounts: true, ownership: s3_constants.OwnershipBucketOwnerEnforced, status: 200}, + {name: "dynamic enforced full control", unregisteredAccounts: true, acl: "bucket-owner-full-control", ownership: s3_constants.OwnershipBucketOwnerEnforced, status: 200}, + {name: "dynamic does not bypass custom validation", unregisteredAccounts: true, grantHeader: s3_constants.AmzAclRead, grant: `id="bucket-owner"`, status: 400, errorCode: "InvalidRequest"}, + {name: "disabled authentication bucket denies acl", unsigned: true, acl: "public-read", policy: "bucket-deny", status: 403, errorCode: "AccessDenied"}, + {name: "header bucket condition denies acl", acl: "public-read", policy: "bucket-condition-deny", status: 403, errorCode: "AccessDenied"}, + {name: "query bucket condition denies acl", acl: "public-read", presigned: true, policy: "bucket-condition-deny", status: 403, errorCode: "AccessDenied"}, + {name: "query bucket condition denies upload", acl: "public-read", presigned: true, policy: "bucket-put-condition-deny", status: 403, errorCode: "AccessDenied"}, + {name: "query iam condition denies acl", acl: "public-read", presigned: true, policy: "iam-condition-deny", status: 403, errorCode: "AccessDenied"}, + {name: "query iam condition denies upload", acl: "public-read", presigned: true, policy: "iam-put-condition-deny", status: 403, errorCode: "AccessDenied"}, + {name: "query bucket condition allows acl", acl: "public-read", presigned: true, writeOnly: true, policy: "bucket-condition-allow", status: 200}, + {name: "query bucket condition supplies all permissions", acl: "public-read", presigned: true, policyOnly: true, policy: "bucket-all-condition-allow", status: 200}, + {name: "query iam condition supplies all permissions", acl: "public-read", presigned: true, policyOnly: true, policy: "iam-all-condition-allow", status: 200}, + {name: "query grant bucket condition denies", grantHeader: s3_constants.AmzAclRead, grant: `id="bucket-owner"`, presigned: true, policy: "bucket-condition-deny", status: 403, errorCode: "AccessDenied"}, + {name: "query grant iam condition denies", grantHeader: s3_constants.AmzAclRead, grant: `id="bucket-owner"`, presigned: true, policy: "iam-condition-deny", status: 403, errorCode: "AccessDenied"}, + {name: "disabled authentication query condition denies acl", unsigned: true, presigned: true, acl: "public-read", policy: "bucket-condition-deny", status: 403, errorCode: "AccessDenied"}, + {name: "disabled authentication query condition denies upload", unsigned: true, presigned: true, acl: "public-read", policy: "bucket-put-condition-deny", status: 403, errorCode: "AccessDenied"}, + // Policy conditions compare the canonical grant list as a whole, so a + // deny on the exact list fires identically for repeated header lines, a + // single comma-joined line, or a signed query parameter. + {name: "repeated grants preserve condition deny", grantHeader: s3_constants.AmzAclRead, grant: `id="bucket-owner"`, repeatedGrant: `id="upload-writer"`, policy: "bucket-condition-deny", conditionValue: `id="bucket-owner",id="upload-writer"`, status: 403, errorCode: "AccessDenied"}, + {name: "single line grants preserve condition deny", grantHeader: s3_constants.AmzAclRead, grant: `id="bucket-owner",id="upload-writer"`, policy: "bucket-condition-deny", conditionValue: `id="bucket-owner",id="upload-writer"`, status: 403, errorCode: "AccessDenied"}, + {name: "presigned grants preserve condition deny", grantHeader: s3_constants.AmzAclRead, grant: `id="bucket-owner",id="upload-writer"`, presigned: true, policy: "bucket-condition-deny", conditionValue: `id="bucket-owner",id="upload-writer"`, status: 403, errorCode: "AccessDenied"}, + {name: "extra grantee defeats allow condition", grantHeader: s3_constants.AmzAclRead, grant: `id="bucket-owner"`, repeatedGrant: `id="upload-writer"`, writeOnly: true, policy: "bucket-condition-allow", conditionValue: `id="bucket-owner"`, status: 403, errorCode: "AccessDenied"}, + } + // Invalid multipart parameters or copy headers must not bypass policy normalization on the actual regular-upload route. + for _, policy := range []string{"bucket", "iam"} { + for _, shape := range []string{"upload id only", "invalid part number", "invalid copy source"} { + query, copySource := url.Values{}, "" + switch shape { + case "upload id only": + query.Set("uploadId", "opaque") + case "invalid part number": + query.Set("uploadId", "opaque") + query.Set("partNumber", "abc") + case "invalid copy source": + copySource = "bogus" + } + tests = append(tests, uploadACLTest{ + name: "routed extra grant denied " + policy + " " + shape, route: true, query: query, copySource: copySource, + grantHeader: s3_constants.AmzAclRead, grant: `id="bucket-owner"`, repeatedGrant: `id="upload-writer"`, + conditionValue: `id="bucket-owner"`, policy: policy + "-all-condition-allow", policyOnly: true, + status: 403, errorCode: "AccessDenied", + }) + } + for _, presigned := range []bool{false, true} { + tests = append(tests, uploadACLTest{ + name: fmt.Sprintf("put action approved negative deny %s presigned %t", policy, presigned), + grantHeader: s3_constants.AmzAclRead, grant: `id = "bucket-owner" , id = "upload-writer"`, + conditionValue: `id="bucket-owner",id="upload-writer"`, conditionOperator: "StringNotEquals", + policy: policy + "-put-condition-deny", presigned: presigned, grantees: []string{bucketOwner, writer}, status: 200, + }) + } + } + // Every grant header must pass the same policy boundary, not just read grants. + for _, header := range []string{s3_constants.AmzAclRead, s3_constants.AmzAclWrite, s3_constants.AmzAclReadAcp, s3_constants.AmzAclWriteAcp, s3_constants.AmzAclFullControl} { + for _, policy := range []string{"bucket", "iam"} { + for _, presigned := range []bool{false, true} { + tests = append(tests, uploadACLTest{ + name: fmt.Sprintf("raw grant header deny %s %s presigned %t", header, policy, presigned), + grantHeader: header, grant: `id = "bucket-owner"`, policy: policy + "-condition-deny", presigned: presigned, + status: 403, errorCode: "AccessDenied", + }) + } + } + } + for _, presigned := range []bool{false, true} { + tests = append(tests, uploadACLTest{ + name: fmt.Sprintf("disabled authentication raw deny presigned %t", presigned), + grantHeader: s3_constants.AmzAclRead, grant: `id = "bucket-owner"`, unsigned: true, presigned: presigned, + policy: "bucket-condition-deny", status: 403, errorCode: "AccessDenied", + }) + for _, policy := range []string{"bucket", "iam"} { + for _, repeated := range []bool{false, true} { + grant, extra := `id="bucket-owner",id="upload-writer"`, "" + if repeated { + grant, extra = `id="bucket-owner"`, `id="upload-writer"` + } + tests = append(tests, uploadACLTest{ + name: fmt.Sprintf("whole list deny preserves extra grantee %s presigned %t repeated %t", policy, presigned, repeated), + grantHeader: s3_constants.AmzAclRead, grant: grant, repeatedGrant: extra, + conditionValue: `id="bucket-owner"`, policy: policy + "-condition-deny", presigned: presigned, + grantees: []string{bucketOwner, writer}, status: 200, + }, uploadACLTest{ + name: fmt.Sprintf("whole list allow rejects extra grantee %s presigned %t repeated %t", policy, presigned, repeated), + grantHeader: s3_constants.AmzAclRead, grant: grant, repeatedGrant: extra, + conditionValue: `id="bucket-owner"`, policy: policy + "-all-condition-allow", presigned: presigned, + policyOnly: true, status: 403, errorCode: "AccessDenied", + }) + } + tests = append(tests, uploadACLTest{ + name: fmt.Sprintf("canonical approved allow with whitespace %s presigned %t", policy, presigned), + grantHeader: s3_constants.AmzAclRead, grant: `id = "bucket-owner" , id = "upload-writer"`, + conditionValue: `id="bucket-owner",id="upload-writer"`, policy: policy + "-all-condition-allow", presigned: presigned, + policyOnly: true, grantees: []string{bucketOwner, writer}, status: 200, + }) + } + } + // Explicit denies on original complete grant values must survive whitespace and escape normalization. + for _, policy := range []string{"bucket", "iam"} { + for _, presigned := range []bool{false, true} { + for _, grant := range []string{ + `id="bucket-owner",id="upload-writer"`, + `id = "bucket-owner" , id = "upload-writer"`, + `id="bucket-\u006fwner",id="upload-writer"`, + } { + for _, operator := range []string{"StringEquals", "StringEqualsIgnoreCase", "StringLike"} { + tests = append(tests, uploadACLTest{ + name: fmt.Sprintf("raw grant deny %s %s presigned %t %s", policy, operator, presigned, grant), + grantHeader: s3_constants.AmzAclRead, grant: grant, presigned: presigned, + conditionOperator: operator, policy: policy + "-condition-deny", status: 403, errorCode: "AccessDenied", + }) + } + } + // Repeated headers and presigned queries must check all grants; an approved value cannot hide an added grantee. + tests = append(tests, uploadACLTest{ + name: fmt.Sprintf("joined repeated raw deny %s presigned %t", policy, presigned), + grantHeader: s3_constants.AmzAclRead, grant: `id="bucket-owner"`, repeatedGrant: `id="upload-writer"`, + conditionValue: `id="bucket-owner",id="upload-writer"`, policy: policy + "-condition-deny", presigned: presigned, + status: 403, errorCode: "AccessDenied", + }, uploadACLTest{ + name: fmt.Sprintf("original repeated line deny %s presigned %t", policy, presigned), + grantHeader: s3_constants.AmzAclRead, grant: `id = "bucket-owner"`, repeatedGrant: `id="upload-writer"`, + conditionValue: `*id = "bucket-owner"*`, conditionOperator: "StringLike", policy: policy + "-condition-deny", presigned: presigned, + status: 403, errorCode: "AccessDenied", + }) + // Negative conditions still compare the canonical complete list, preserving equivalent encodings of approved lists. + for _, operator := range []string{"StringNotEquals", "StringNotLike", "StringNotEqualsIgnoreCase"} { + for _, grant := range []string{`id="bucket-owner",id="upload-writer"`, `id = "bucket-owner" , id = "upload-writer"`} { + tests = append(tests, uploadACLTest{ + name: fmt.Sprintf("approved negative deny %s %s presigned %t %s", policy, operator, presigned, grant), + grantHeader: s3_constants.AmzAclRead, grant: grant, conditionOperator: operator, + conditionValue: `id="bucket-owner",id="upload-writer"`, policy: policy + "-condition-deny", presigned: presigned, + grantees: []string{bucketOwner, writer}, status: 200, + }, uploadACLTest{ + name: fmt.Sprintf("negative allow unchanged %s %s presigned %t %s", policy, operator, presigned, grant), + grantHeader: s3_constants.AmzAclRead, grant: grant, conditionOperator: operator, + conditionValue: `id="bucket-owner",id="upload-writer"`, policy: policy + "-all-condition-allow", presigned: presigned, + policyOnly: true, status: 403, errorCode: "AccessDenied", + }) + } + tests = append(tests, uploadACLTest{ + name: fmt.Sprintf("extra repeated grantee denied %s %s presigned %t", policy, operator, presigned), + grantHeader: s3_constants.AmzAclRead, grant: `id="bucket-owner"`, repeatedGrant: `id="upload-writer"`, + conditionOperator: operator, conditionValue: `id="bucket-owner"`, policy: policy + "-condition-deny", presigned: presigned, + status: 403, errorCode: "AccessDenied", + }) + } + } + } + // Special characters must remain literal in canonical policy values, while + // authorization still checks both upload permissions and complete grant lists. + for _, grantee := range []struct{ key, value, account string }{ + {"emailAddress", "a&b@example.com", "email-reader"}, + {"id", "readeraccount", "reader>account"}, + } { + grant := fmt.Sprintf("%s=%q", grantee.key, grantee.value) + for _, policy := range []string{"bucket", "iam"} { + for _, presigned := range []bool{false, true} { + for _, rule := range []struct { + name, operator, policy string + status int + }{ + {"allow", "StringEquals", "-all-condition-allow", 200}, + {"approved negative deny", "StringNotEquals", "-condition-deny", 200}, + {"positive deny", "StringEquals", "-condition-deny", 403}, + } { + tt := uploadACLTest{ + name: fmt.Sprintf("html grant %s %s %s presigned %t", grantee.value, policy, rule.name, presigned), + grantHeader: s3_constants.AmzAclRead, grant: grant, grantAccount: grantee.account, + conditionValue: grant, conditionOperator: rule.operator, policy: policy + rule.policy, + presigned: presigned, policyOnly: rule.name == "allow", grantees: []string{grantee.account}, status: rule.status, + } + if grantee.key == "emailAddress" { + tt.grantEmail = grantee.value + } + if rule.status == 403 { + tt.errorCode = "AccessDenied" + } + tests = append(tests, tt) + } + } + } + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + key := object + if tt.marker { + key = "allowed/folder/" + } + volume := startFakeVolumeServer(t) + filer := &ambiguousPutFiler{volume: volume, apply: true, entries: map[string]*filer_pb.Entry{}} + s3a := newPutTestServer(t, startFakeFiler(t, filer)) + s3a.option.DefaultFileMode = tt.defaultMode + s3a.iam = NewIdentityAccessManagementWithStore(s3a.option, nil, "memory") + t.Cleanup(s3a.iam.Shutdown) + s3a.iam.isAuthEnabled = !tt.unsigned + account := &Account{Id: writer, DisplayName: writer} + identity := &Identity{Name: "upload-acl-test", Account: account, IsStatic: true, + Actions: []Action{"Write:acl-bucket/allowed/*"}, + Credentials: []*Credential{{AccessKey: routingTestAccessKey, SecretKey: routingTestSecretKey}}} + if !tt.writeOnly { + scope := "WriteAcp:acl-bucket/allowed/*" + if tt.wrongScope { + scope = "WriteAcp:acl-bucket/other/*" + } + identity.Actions = append(identity.Actions, Action(scope)) + } + if tt.policyOnly { + identity.Actions = nil + } + s3a.iam.accessKeyIdent[routingTestAccessKey] = identity + s3a.iam.nameToIdentity[identity.Name] = identity + s3a.iam.accounts[writer] = account + s3a.iam.accounts[bucketOwner] = &Account{Id: bucketOwner, DisplayName: bucketOwner} + if tt.grantAccount != "" { + grantee := &Account{Id: tt.grantAccount, DisplayName: tt.grantAccount, EmailAddress: tt.grantEmail} + s3a.iam.accounts[tt.grantAccount] = grantee + if tt.grantEmail != "" { + s3a.iam.emailAccount[tt.grantEmail] = grantee + } + } + if tt.unregisteredWriter { + delete(s3a.iam.accounts, writer) + } + if tt.unregisteredAccounts { + // JWT/STS authentication supplies trusted accounts dynamically; + // their IDs are not registered in the static grantee directory. + delete(s3a.iam.accounts, writer) + delete(s3a.iam.accounts, bucketOwner) + } + ownership := tt.ownership + if ownership == "" { + ownership = s3_constants.OwnershipObjectWriter + } + bucketEntry := &filer_pb.Entry{Name: bucket, IsDirectory: true, Attributes: &filer_pb.FuseAttributes{}, + Extended: map[string][]byte{s3_constants.ExtAmzOwnerKey: []byte(bucketOwner)}} + storedOwnership := ownership + if ownership == "absent" { + storedOwnership = "" + } else { + bucketEntry.Extended[s3_constants.ExtOwnershipKey] = []byte(ownership) + } + filer.entries["/buckets/"+bucket] = bucketEntry + s3a.bucketConfigCache = NewBucketConfigCache(time.Minute) + s3a.bucketConfigCache.Set(bucket, &BucketConfig{Name: bucket, Owner: bucketOwner, Ownership: storedOwnership, Versioning: tt.versioning}) + s3a.bucketRegistry = NewBucketRegistry(s3a) + s3a.bucketRegistry.LoadBucketMetadata(bucketEntry) + if tt.versioning == s3_constants.VersioningEnabled { + filer.entries["/buckets/"+bucket+"/"+key+s3_constants.VersionsFolder] = &filer_pb.Entry{Name: "image.png.versions", IsDirectory: true, Attributes: &filer_pb.FuseAttributes{}} + } + var original *filer_pb.Entry + if tt.overwrite { + original = &filer_pb.Entry{Name: "image.png", Attributes: &filer_pb.FuseAttributes{}, Extended: map[string][]byte{s3_constants.ExtAmzOwnerKey: []byte(bucketOwner), s3_constants.ExtAmzAclKey: []byte("old-acl")}} + filer.entries["/buckets/"+bucket+"/"+key] = proto.Clone(original).(*filer_pb.Entry) + } + if tt.policy != "" { + effect := "Allow" + if strings.HasSuffix(tt.policy, "deny") { + effect = "Deny" + } + statement := fmt.Sprintf(`{"Effect":%q,"Action":"s3:PutObjectAcl","Resource":"arn:aws:s3:::acl-bucket/allowed/*"}`, effect) + if strings.Contains(tt.policy, "put-") { + statement = strings.Replace(statement, `"s3:PutObjectAcl"`, `"s3:PutObject"`, 1) + } else if strings.Contains(tt.policy, "all-") { + statement = strings.Replace(statement, `"s3:PutObjectAcl"`, `["s3:PutObject","s3:PutObjectAcl"]`, 1) + } + if strings.Contains(tt.policy, "condition") { + header, value := s3_constants.AmzCannedAcl, tt.acl + if tt.grantHeader != "" { + header, value = tt.grantHeader, tt.grant + } + if tt.conditionValue != "" { + value = tt.conditionValue + } + operator := tt.conditionOperator + if operator == "" { + operator = "StringEquals" + } + condition := fmt.Sprintf(`,"Condition":{%q:{%q:%q}}}`, operator, "s3:"+strings.ToLower(header), value) + statement = strings.TrimSuffix(statement, "}") + condition + } + if strings.HasPrefix(tt.policy, "iam") { + statements := statement + if !tt.policyOnly { + allowActions := `"s3:PutObject"` + if strings.Contains(tt.policy, "condition") && effect == "Deny" { + allowActions = `["s3:PutObject","s3:PutObjectAcl"]` + } + statements = `{"Effect":"Allow","Action":` + allowActions + `,"Resource":"arn:aws:s3:::acl-bucket/allowed/*"},` + statement + } + require.NoError(t, s3a.iam.PutPolicy("upload-acl-policy", `{"Version":"2012-10-17","Statement":[`+statements+`]}`)) + identity.PolicyNames = []string{"upload-acl-policy"} + } else { + s3a.policyEngine = NewBucketPolicyEngine() + s3a.iam.policyEngine = s3a.policyEngine + statement = strings.Replace(statement, `{"Effect":`, `{"Principal":"*","Effect":`, 1) + require.NoError(t, s3a.policyEngine.engine.SetBucketPolicy(bucket, `{"Version":"2012-10-17","Statement":[`+statement+`]}`)) + } + } + body := "uploaded content" + wireBody := body + if tt.streaming { + checksum := crc32.NewIEEE() + _, err := checksum.Write([]byte(body)) + require.NoError(t, err) + wireBody = fmt.Sprintf("%x\r\n%s\r\n0\r\n\r\nx-amz-checksum-crc32:%s\r\n\r\n", len(body), body, base64.StdEncoding.EncodeToString(checksum.Sum(nil))) + } + req := httptest.NewRequest(http.MethodPut, "http://s3/"+bucket+"/"+key, strings.NewReader(wireBody)) + req = mux.SetURLVars(req, map[string]string{"bucket": bucket, "object": key}) + req.Header.Set("Content-Type", "text/plain") + if tt.copySource != "" { + req.Header.Set("X-Amz-Copy-Source", tt.copySource) + } + if tt.acl != "" { + req.Header.Set(s3_constants.AmzCannedAcl, tt.acl) + } + if tt.grantHeader != "" { + req.Header.Set(tt.grantHeader, tt.grant) + if tt.repeatedGrant != "" { + req.Header.Add(tt.grantHeader, tt.repeatedGrant) + } + } + req.URL.RawQuery = tt.query.Encode() + if tt.streaming { + req.Header.Set("X-Amz-Content-Sha256", streamingUnsignedPayload) + req.Header.Set("X-Amz-Trailer", "x-amz-checksum-crc32") + req.Header.Set("X-Amz-Decoded-Content-Length", fmt.Sprint(len(body))) + req.Header.Set("Content-Encoding", "aws-chunked") + } + if tt.presigned && tt.signature != "v2-header" { + // Exercise ACLs in the signed query rather than relying on a + // particular SDK version's automatic header-hoisting behavior. + query := req.URL.Query() + if tt.acl != "" { + query.Set(s3_constants.AmzCannedAcl, tt.acl) + req.Header.Del(s3_constants.AmzCannedAcl) + } + if tt.grantHeader != "" { + query.Set(tt.grantHeader, strings.Join(req.Header.Values(tt.grantHeader), ",")) + req.Header.Del(tt.grantHeader) + } + req.URL.RawQuery = query.Encode() + } + if tt.unsigned { + // Disabled authentication uses the admin account, not a caller's + // forged internal account header. + req.Header.Set(s3_constants.AmzAccountId, "forged-account") + } + if tt.signature != "" { + cred := &Credential{AccessKey: routingTestAccessKey, SecretKey: routingTestSecretKey} + if tt.presigned { + query := req.URL.Query() + expires := fmt.Sprint(time.Now().Add(time.Minute).Unix()) + query.Set("AWSAccessKeyId", routingTestAccessKey) + query.Set("Expires", expires) + query.Set("Signature", preSignatureV2(cred, req.Method, req.URL.EscapedPath(), query.Encode(), req.Header, expires)) + req.URL.RawQuery = query.Encode() + } else { + req.Header.Set("Date", time.Now().UTC().Format(http.TimeFormat)) + req.Header.Set("Authorization", signatureV2(cred, req.Method, req.URL.EscapedPath(), req.URL.RawQuery, req.Header)) + } + } else if tt.presigned && !tt.unsigned { + signer := v4.NewSigner(credentials.NewStaticCredentials(routingTestAccessKey, routingTestSecretKey, "")) + _, err := signer.Presign(req, strings.NewReader(wireBody), "s3", "us-east-1", time.Minute, time.Now()) + require.NoError(t, err) + } else if !tt.unsigned { + signRoutingTestRequest(t, req, wireBody, "s3") + } + if tt.afterSigning != nil { + query := req.URL.Query() + for key, values := range tt.afterSigning { + query[key] = values + } + req.URL.RawQuery = query.Encode() + if tt.errorCode != "SignatureDoesNotMatch" { + // These attacks preserve a valid signature. Unsigned V2 ACLs + // must be ignored; ambiguous signed V4 ACLs must be rejected. + _, code := s3a.iam.AuthenticateRequest(req.Clone(req.Context())) + require.Equal(t, s3err.ErrNone, code) + } + } + rr := httptest.NewRecorder() + if tt.route { + s3a.cb = &CircuitBreaker{s3a: s3a} + router := mux.NewRouter() + s3a.registerRouter(router) + router.ServeHTTP(rr, req) + } else { + s3a.iam.Auth(s3a.PutObjectHandler, s3_constants.ACTION_WRITE)(rr, req) + } + require.Equal(t, tt.status, rr.Code, rr.Body.String()) + // Snapshot the committed entry under the fixture lock, then release it + // before GetObjectAcl makes another RPC to the fake filer. + var allocatedChunks uint64 + stored := func() *filer_pb.Entry { + filer.mu.Lock() + defer filer.mu.Unlock() + allocatedChunks = filer.nextKey + entry := filer.entries["/buckets/"+bucket+"/"+strings.TrimSuffix(key, "/")] + if tt.status == http.StatusOK && tt.versioning == s3_constants.VersioningEnabled { + versionID := rr.Header().Get("x-amz-version-id") + require.NotEmpty(t, versionID) + entry = nil + for _, candidate := range filer.entries { + if string(candidate.Extended[s3_constants.ExtVersionIdKey]) == versionID { + entry = candidate + break + } + } + } + if entry == nil { + return nil + } + return proto.Clone(entry).(*filer_pb.Entry) + }() + if tt.status != http.StatusOK { + require.Contains(t, rr.Body.String(), ""+tt.errorCode+"") + require.Zero(t, allocatedChunks, "rejected ACLs must not allocate chunks") + require.True(t, proto.Equal(original, stored), "rejected uploads must not replace the object") + return + } + wantACL, wantGrantHeader := tt.acl, tt.grantHeader + if strings.HasPrefix(tt.signature, "v2") && tt.presigned && tt.signature != "v2-header" { + wantACL, wantGrantHeader = "", "" + } + require.NotNil(t, stored) + if !tt.marker { + mode := defaultFileMode + if tt.defaultMode != 0 && wantACL == "" { + mode = tt.defaultMode + } + switch wantACL { + case "public-read", "authenticated-read", "bucket-owner-read": + mode = 0644 + case "public-read-write": + mode = 0666 + } + require.Equal(t, mode, stored.Attributes.FileMode, "header and signed query ACLs must use the same file mode") + } + bodyMD5 := md5.Sum([]byte(body)) + require.Equal(t, bodyMD5[:], stored.Attributes.Md5, "ACL parsing must not consume or alter the upload body") + wantOwner := writer + if tt.unsigned { + wantOwner = AccountAdmin.Id + } + if s3_constants.EffectiveOwnership(storedOwnership) == s3_constants.OwnershipBucketOwnerEnforced || (ownership == s3_constants.OwnershipBucketOwnerPreferred && wantACL == "bucket-owner-full-control") { + wantOwner = bucketOwner + } + require.Equal(t, wantOwner, string(stored.Extended[s3_constants.ExtAmzOwnerKey])) + grants := GetAcpGrants(stored.Extended) + require.NotEmpty(t, grants, "ACL must be persisted in the object create") + if wantGrantHeader != "" { + wantGrantees := tt.grantees + if wantGrantees == nil { + wantGrantees = []string{bucketOwner} + } + wantPermission := map[string]string{ + s3_constants.AmzAclRead: s3_constants.PermissionRead, + s3_constants.AmzAclWrite: s3_constants.PermissionWrite, + s3_constants.AmzAclReadAcp: s3_constants.PermissionReadAcp, + s3_constants.AmzAclWriteAcp: s3_constants.PermissionWriteAcp, + s3_constants.AmzAclFullControl: s3_constants.PermissionFullControl, + }[wantGrantHeader] + ownerFullControl := false + for _, grantee := range wantGrantees { + ownerFullControl = ownerFullControl || (grantee == wantOwner && wantPermission == s3_constants.PermissionFullControl) + } + wantCount := len(wantGrantees) + if !ownerFullControl { + wantCount++ + } + require.Len(t, grants, wantCount, "custom uploads must retain the owner's full control") + for i, grantee := range wantGrantees { + require.Equal(t, grantee, aws.StringValue(grants[i].Grantee.ID)) + require.Equal(t, wantPermission, aws.StringValue(grants[i].Permission)) + } + if !ownerFullControl { + ownerGrant := grants[len(grants)-1] + require.Equal(t, wantOwner, aws.StringValue(ownerGrant.Grantee.ID)) + require.Equal(t, s3_constants.GrantTypeCanonicalUser, aws.StringValue(ownerGrant.Grantee.Type)) + require.Equal(t, s3_constants.PermissionFullControl, aws.StringValue(ownerGrant.Permission)) + } + } else { + require.Equal(t, wantOwner, aws.StringValue(grants[0].Grantee.ID)) + require.Equal(t, s3_constants.PermissionFullControl, aws.StringValue(grants[0].Permission)) + if wantACL == "public-read" || wantACL == "public-read-write" || wantACL == "authenticated-read" { + wantGrants := 2 + if wantACL == "public-read-write" { + wantGrants = 3 + } + require.Len(t, grants, wantGrants) + wantGroup := s3_constants.GranteeGroupAllUsers + if wantACL == "authenticated-read" { + wantGroup = s3_constants.GranteeGroupAuthenticatedUsers + } + require.Equal(t, wantGroup, aws.StringValue(grants[1].Grantee.URI)) + require.Equal(t, s3_constants.PermissionRead, aws.StringValue(grants[1].Permission)) + if wantACL == "public-read-write" { + require.Equal(t, s3_constants.GranteeGroupAllUsers, aws.StringValue(grants[2].Grantee.URI)) + require.Equal(t, s3_constants.PermissionWrite, aws.StringValue(grants[2].Permission)) + } + } else if ownership == s3_constants.OwnershipObjectWriter && strings.HasPrefix(wantACL, "bucket-owner-") { + require.Len(t, grants, 2) + require.Equal(t, bucketOwner, aws.StringValue(grants[1].Grantee.ID)) + wantPermission := s3_constants.PermissionRead + if wantACL == "bucket-owner-full-control" { + wantPermission = s3_constants.PermissionFullControl + } + require.Equal(t, wantPermission, aws.StringValue(grants[1].Permission)) + } else { + require.Len(t, grants, 1) + } + } + if wantGrantHeader != "" { + aclRequest := httptest.NewRequest(http.MethodGet, "http://s3/"+bucket+"/"+key+"?acl", nil) + aclRequest = mux.SetURLVars(aclRequest, map[string]string{"bucket": bucket, "object": key}) + aclRequest.Header.Set(s3_constants.AmzAccountId, wantOwner) + aclResponse := httptest.NewRecorder() + s3a.GetObjectAclHandler(aclResponse, aclRequest) + require.Equal(t, http.StatusOK, aclResponse.Code, aclResponse.Body.String()) + var acl AccessControlPolicy + require.NoError(t, xml.Unmarshal(aclResponse.Body.Bytes(), &acl)) + require.Equal(t, wantOwner, acl.Owner.ID) + require.Len(t, acl.AccessControlList.Grant, len(grants)) + for i, grant := range acl.AccessControlList.Grant { + require.Equal(t, aws.StringValue(grants[i].Grantee.ID), grant.Grantee.ID) + require.Equal(t, Permission(aws.StringValue(grants[i].Permission)), grant.Permission) + } + } + }) + } +} + +// TestPutObjectACLPolicyScope ensures query normalization cannot alter other +// operations or the original signed request passed to the upload handler. +func TestPutObjectACLPolicyScope(t *testing.T) { + tests := []struct { + name, method, object, subresource string + action Action + copy bool + repeatedCopy bool + wantACL string + }{ + {name: "upload", method: http.MethodPut, object: "key", action: s3_constants.ACTION_WRITE, wantACL: "public-read"}, + {name: "upload acl authorization", method: http.MethodPut, object: "key", action: s3_constants.ACTION_WRITE_ACP, wantACL: "public-read"}, + {name: "bucket", method: http.MethodPut, action: s3_constants.ACTION_WRITE}, + {name: "post form", method: http.MethodPost, object: "key", action: s3_constants.ACTION_WRITE}, + {name: "copy", method: http.MethodPut, object: "key", action: s3_constants.ACTION_WRITE, copy: true}, + {name: "repeated copy source", method: http.MethodPut, object: "key", action: s3_constants.ACTION_WRITE, repeatedCopy: true}, + {name: "multipart part", method: http.MethodPut, object: "key", subresource: "uploadId=upload&partNumber=1", action: s3_constants.ACTION_WRITE}, + {name: "multipart leading zero", method: http.MethodPut, object: "key", subresource: "uploadId=upload&partNumber=01", action: s3_constants.ACTION_WRITE}, + {name: "upload id only", method: http.MethodPut, object: "key", subresource: "uploadId=upload", action: s3_constants.ACTION_WRITE, wantACL: "public-read"}, + {name: "invalid part number", method: http.MethodPut, object: "key", subresource: "uploadId=upload&partNumber=abc", action: s3_constants.ACTION_WRITE, wantACL: "public-read"}, + {name: "standalone acl", method: http.MethodPut, object: "key", subresource: "acl=", action: s3_constants.ACTION_WRITE_ACP}, + {name: "tagging", method: http.MethodPut, object: "key", subresource: "tagging=", action: s3_constants.ACTION_WRITE}, + {name: "retention", method: http.MethodPut, object: "key", subresource: "retention=", action: s3_constants.ACTION_WRITE}, + {name: "other service", method: http.MethodPut, object: "key", action: "iam:CreateUser"}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + req := httptest.NewRequest(tt.method, "http://s3/bucket/"+tt.object+"?x-amz-acl=public-read&"+tt.subresource, nil) + if tt.copy { + req.Header.Set("X-Amz-Copy-Source", "/source/key") + } + if tt.repeatedCopy { + req.Header.Set("X-Amz-Copy-Source", "bogus") + req.Header.Add("X-Amz-Copy-Source", "%2fsource%2fkey") + } + policyRequest, code := putObjectACLPolicyRequest(req, tt.action, "bucket", tt.object) + require.Equal(t, s3err.ErrNone, code) + require.Equal(t, tt.wantACL, policyRequest.Header.Get(s3_constants.AmzCannedAcl)) + require.Empty(t, req.Header.Get(s3_constants.AmzCannedAcl), "normalization must preserve signed headers") + }) + } +} diff --git a/weed/s3api/s3api_server.go b/weed/s3api/s3api_server.go index 48886f199..b0dae8207 100644 --- a/weed/s3api/s3api_server.go +++ b/weed/s3api/s3api_server.go @@ -608,6 +608,13 @@ func (s3a *S3ApiServer) checkPolicyWithEntry(r *http.Request, bucket, object, ac return s3err.ErrNone, false } + // Upload handler rechecks use the same effective ACL conditions as authentication without changing the signed request. + policyRequest, policyCode := putObjectACLPolicyRequest(r, Action(action), bucket, object) + if policyCode != s3err.ErrNone { + return policyCode, true + } + r = policyRequest + identityRaw := GetIdentityFromContext(r) var identity *Identity if identityRaw != nil { diff --git a/weed/s3api/s3err/s3api_errors.go b/weed/s3api/s3err/s3api_errors.go index f0bd3354e..302bb0665 100644 --- a/weed/s3api/s3err/s3api_errors.go +++ b/weed/s3api/s3err/s3api_errors.go @@ -171,6 +171,7 @@ const ( ErrInvalidRenameSource ErrRenameDestinationSameAsSource ErrIdempotentParameterMismatch + ErrAccessControlListNotSupported ) // Error message constants for checksum validation @@ -565,6 +566,11 @@ var errorCodeResponse = map[ErrorCode]APIError{ Description: "Invalid Request", HTTPStatusCode: http.StatusBadRequest, }, + ErrAccessControlListNotSupported: { + Code: "AccessControlListNotSupported", + Description: "The bucket does not allow ACLs", + HTTPStatusCode: http.StatusBadRequest, + }, ErrInvalidRange: { Code: "InvalidRange", Description: "The requested range is not satisfiable",