diff --git a/auth/access-control.go b/auth/access-control.go index 578d1f46..69573eed 100644 --- a/auth/access-control.go +++ b/auth/access-control.go @@ -116,7 +116,7 @@ func VerifyAccess(ctx fiber.Ctx, be backend.Backend, opts AccessOptions) error { return err } - errs, err := objectsAccessErrors(ctx.RequestCtx(), be, opts, []string{opts.Object}, requestConditionContext(ctx)) + errs, err := objectsAccessErrors(ctx.RequestCtx(), be, opts, []string{opts.Object}, requestConditionContext(ctx, opts.Actions)) if err != nil { return err } @@ -163,7 +163,9 @@ func VerifyObjectsAccess(ctx fiber.Ctx, be backend.Backend, opts AccessOptions, } rctx := ctx.RequestCtx() - condCtx := requestConditionContext(ctx) + // A DeleteObjects batch reads no If-Match/If-None-Match, so no + // conditional-write key applies to any object in it. + condCtx := requestConditionContext(ctx, nil) keys := make([]string, len(objects)) for i, obj := range objects { @@ -528,7 +530,7 @@ func VerifyPublicAccess(ctx fiber.Ctx, be backend.Backend, action Action, permis return err } if err == nil { - err = VerifyPublicBucketPolicy(policy, bucket, object, requestConditionContext(ctx), be.NormalizeObjectKey, action) + err = VerifyPublicBucketPolicy(policy, bucket, object, requestConditionContext(ctx, []Action{action}), be.NormalizeObjectKey, action) if errors.Is(err, errExplicitDeny) { // Explicit public-policy Deny has higher precedence than any // public ACL grant, so do not continue to ACL fallback. @@ -619,7 +621,7 @@ func verifyIdentityOnlyAccess(ctx fiber.Ctx, pe PolicyEvaluator, acc Account, ac Acc: acc, Bucket: resource, Actions: []Action{action}, - }, []string{""}, nil, requestConditionContext(ctx)) + }, []string{""}, nil, requestConditionContext(ctx, []Action{action})) if err != nil { return err } diff --git a/auth/bucket_policy_condition.go b/auth/bucket_policy_condition.go index 70a2d00f..b2a0ce0d 100644 --- a/auth/bucket_policy_condition.go +++ b/auth/bucket_policy_condition.go @@ -69,6 +69,28 @@ func isVersionedAction(a Action) bool { } } +// isConditionalWriteAction is s3:if-match's applicable-action set: +// s3:PutObject — which also covers CompleteMultipartUpload, authorized as +// s3:PutObject — and s3:DeleteObject, S3's conditional delete. Note that +// s3:DeleteObjectVersion is not in the set: a versioned delete names the +// version to remove rather than overwriting the current one. +// +// addConditionalWriteKeys gates the runtime key on the same predicate, so +// narrowing or widening this set moves both ends at once. +func isConditionalWriteAction(a Action) bool { + return a == PutObjectAction || a == DeleteObjectAction +} + +// isConditionalCreateAction is s3:if-none-match's applicable-action set: +// s3:PutObject alone. If-None-Match asserts the object doesn't exist yet, +// which only an upload can require — S3 rejects the header on DeleteObject. +// +// addConditionalWriteKeys gates the runtime key on the same predicate, so +// narrowing or widening this set moves both ends at once. +func isConditionalCreateAction(a Action) bool { + return a == PutObjectAction +} + // bucketPolicyConditionKeys is the fixed catalogue of condition keys this // gateway's S3 bucket-policy Condition support recognizes, each mapped to // the actions it may be used with. Keys are looked up case-insensitively @@ -81,10 +103,8 @@ func isVersionedAction(a Action) bool { // s3:RequestObjectTagKeys), object-lock keys, s3:x-amz-server-side-encryption // (the gateway never reads that header, so enforcing it would be // misleading), and aws:MultiFactorAuthAge (no MFA concept here) are out of -// scope. A Condition naming one of those is still accepted at write time — -// the key just never appears in the runtime context, so any Condition -// depending on it simply never matches, the same as any other key this -// package doesn't populate. +// scope: a Condition naming one of those is rejected at write time, the +// same as any other key absent from this catalogue. var bucketPolicyConditionKeys = map[string]conditionKeyRule{ // Generic keys: AWS accepts these with any action. "aws:sourceip": {appliesTo: anyAction, ipSemantic: true}, @@ -104,6 +124,11 @@ var bucketPolicyConditionKeys = map[string]conditionKeyRule{ "s3:max-keys": {appliesTo: isListAction}, "s3:x-amz-acl": {appliesTo: isAclPutAction}, "s3:versionid": {appliesTo: isVersionedAction}, + + // Conditional-write keys, carrying the request's If-Match / + // If-None-Match header value. + "s3:if-match": {appliesTo: isConditionalWriteAction}, + "s3:if-none-match": {appliesTo: isConditionalCreateAction}, } // lookupConditionKeyRule finds key's rule case-insensitively. diff --git a/auth/bucket_policy_condition_test.go b/auth/bucket_policy_condition_test.go index 58b716c6..2b7100cf 100644 --- a/auth/bucket_policy_condition_test.go +++ b/auth/bucket_policy_condition_test.go @@ -137,6 +137,70 @@ func TestValidateBucketPolicyCondition(t *testing.T) { raw: `{"StringEquals":{"s3:x-amz-acl":"public-read"}}`, actions: actionSet(AllActions), }, + { + name: "s3:if-match accepted for PutObject", + raw: `{"StringEquals":{"s3:if-match":"abc123"}}`, + actions: actionSet(PutObjectAction), + }, + { + // S3's conditional delete takes If-Match, so the key applies + // to DeleteObject as well as PutObject. + name: "s3:if-match accepted for DeleteObject", + raw: `{"StringEquals":{"s3:if-match":"abc123"}}`, + actions: actionSet(DeleteObjectAction), + }, + { + name: "s3:if-match accepted for PutObject and DeleteObject together", + raw: `{"StringEquals":{"s3:if-match":"abc123"}}`, + actions: actionSet(PutObjectAction, DeleteObjectAction), + }, + { + name: "s3:if-match rejected for GetObject", + raw: `{"StringEquals":{"s3:if-match":"abc123"}}`, + actions: actionSet(GetObjectAction), + wantErr: policyErrConditionActionMismatch, + }, + { + // A versioned delete names the version to remove and takes no + // If-Match, so the key doesn't extend to it. + name: "s3:if-match rejected for DeleteObjectVersion", + raw: `{"StringEquals":{"s3:if-match":"abc123"}}`, + actions: actionSet(DeleteObjectVersionAction), + wantErr: policyErrConditionActionMismatch, + }, + { + name: "s3:if-none-match accepted for PutObject", + raw: `{"Null":{"s3:if-none-match":"false"}}`, + actions: actionSet(PutObjectAction), + }, + { + // If-None-Match asserts the object doesn't exist yet, which + // only an upload can require - unlike s3:if-match, DeleteObject + // doesn't take it. + name: "s3:if-none-match rejected for DeleteObject", + raw: `{"Null":{"s3:if-none-match":"false"}}`, + actions: actionSet(DeleteObjectAction), + wantErr: policyErrConditionActionMismatch, + }, + { + name: "s3:if-none-match rejected for PutObject and DeleteObject together", + raw: `{"Null":{"s3:if-none-match":"false"}}`, + actions: actionSet(PutObjectAction, DeleteObjectAction), + wantErr: policyErrConditionActionMismatch, + }, + { + name: "conditional-write keys recognized case-insensitively", + raw: `{"StringEquals":{"S3:IF-MATCH":"abc123"},"Null":{"s3:If-None-Match":"false"}}`, + actions: actionSet(PutObjectAction), + }, + { + // The hyphens and the s3: prefix are part of the key name; + // dropping either names a key that doesn't exist. + name: "s3:ifmatch is not a recognized key", + raw: `{"StringEquals":{"s3:ifmatch":"abc123"}}`, + actions: actionSet(PutObjectAction), + wantErr: policyErrInvalidConditionKey, + }, { name: "aws:SourceIp with a valid CIDR", raw: `{"IpAddress":{"aws:SourceIp":"10.0.0.0/8"}}`, diff --git a/auth/condition_context.go b/auth/condition_context.go index 7d1e5cc5..e30cfbcc 100644 --- a/auth/condition_context.go +++ b/auth/condition_context.go @@ -15,6 +15,7 @@ package auth import ( + "slices" "strconv" "time" @@ -28,7 +29,11 @@ import ( // aws:username, aws:PrincipalTag/*, …) are deliberately absent: the S3 // gateway has no way to know them, so the IAM service fills them in itself // when it evaluates an identity policy. -func requestConditionContext(ctx fiber.Ctx) map[string][]string { +// +// actions is the action set this request is being authorized under, used by +// the keys whose applicability AWS defines per action. Pass nil where no +// such key can apply. +func requestConditionContext(ctx fiber.Ctx, actions []Action) map[string][]string { now := time.Now().UTC() condCtx := map[string][]string{ "aws:CurrentTime": {now.Format(time.RFC3339)}, @@ -63,6 +68,117 @@ func requestConditionContext(ctx fiber.Ctx) map[string][]string { if versionID := ctx.Query("versionId"); versionID != "" { condCtx["s3:VersionId"] = []string{versionID} } + addConditionalWriteKeys(ctx, actions, condCtx) return condCtx } + +// addConditionalWriteKeys populates s3:if-match and s3:if-none-match from +// the request's If-Match/If-None-Match headers, each only on a request +// whose action the key applies to. The applicability rules are the ones +// PutBucketPolicy validates a Condition against, so a key can never reach +// the request context on an action a policy isn't allowed to name it on. +// The value is the header with its surrounding ETag quotes removed, so a +// policy compares against the bare ETag whichever form the client sent. +func addConditionalWriteKeys(ctx fiber.Ctx, actions []Action, condCtx map[string][]string) { + action := conditionalWriteAction(ctx, actions) + if isConditionalWriteAction(action) { + if ifMatch := trimETagQuotes(ctx.Get("If-Match")); ifMatch != "" { + condCtx["s3:if-match"] = []string{ifMatch} + } + } + if isConditionalCreateAction(action) { + if ifNoneMatch := trimETagQuotes(ctx.Get("If-None-Match")); ifNoneMatch != "" { + condCtx["s3:if-none-match"] = []string{ifNoneMatch} + } + } +} + +// nonConditionalWriteSubresources names the query parameters that route an +// object PUT or DELETE to a handler other than PutObject/DeleteObject: +// tagging, retention, legal-hold and ACL writes, plus UploadPart and +// AbortMultipartUpload. UploadPart and UploadPartCopy are authorized as +// s3:PutObject just like PutObject itself, so only the route tells them +// apart. +var nonConditionalWriteSubresources = []string{"acl", "tagging", "retention", "legal-hold", "uploadId"} + +// conditionalWriteAction reports the action ctx is authorized under, for +// the requests whose If-Match/If-None-Match the gateway enforces: PutObject +// and CompleteMultipartUpload, both authorized as s3:PutObject, and +// DeleteObject, which a versionId turns into s3:DeleteObjectVersion exactly +// as the handler does. It returns the empty action for everything else. +// +// Everything else ignores those headers, and a policy must never grant on a +// precondition that won't be checked — otherwise a form upload, a +// DeleteObjects batch, a copy or an upload part could satisfy a statement +// demanding a conditional write by sending a header that changes nothing. +// Reads are the same case: GET and HEAD take these headers as ordinary HTTP +// cache preconditions. Excluding a request leaves both keys absent, which +// denies it under such a policy rather than letting it through. +// +// The request shape alone doesn't identify an object write: a bucket +// sub-resource write is a PUT or DELETE carrying none of the object +// sub-resources, so it has to be recognized by what it is authorized as. +// actions is that set, and the shape's action must be in it — the two +// disagree exactly when the request routes somewhere else, as +// PutBucketVersioning, CreateBucket and DeleteBucket all do. +func conditionalWriteAction(ctx fiber.Ctx, actions []Action) Action { + action := conditionalWriteRouteAction(ctx) + if action == "" || !slices.Contains(actions, action) { + return "" + } + return action +} + +// conditionalWriteRouteAction is conditionalWriteAction's request-shape +// half: the action this method, query and copy-source header would route +// to, before checking what the request is actually authorized as. +func conditionalWriteRouteAction(ctx fiber.Ctx) Action { + // A copy carries its preconditions in the X-Amz-Copy-Source-If-* + // headers, which name the source object and populate neither key. Both + // a copy and a plain upload are authorized as s3:PutObject, so only + // this header tells them apart. + if ctx.Get("X-Amz-Copy-Source") != "" { + return "" + } + query := ctx.Request().URI().QueryArgs() + switch string(ctx.Request().Header.Method()) { + case fiber.MethodPut: + if slices.ContainsFunc(nonConditionalWriteSubresources, query.Has) { + return "" + } + return PutObjectAction + case fiber.MethodDelete: + if slices.ContainsFunc(nonConditionalWriteSubresources, query.Has) { + return "" + } + // A delete naming a version removes that version rather than + // overwriting the current one, so it is authorized as + // s3:DeleteObjectVersion — an action neither key applies to. The + // gateway still enforces the precondition against the named + // version; a policy simply has no vocabulary to require it there, + // and the key stays absent so such a statement denies instead. + if query.Has("versionId") { + return DeleteObjectVersionAction + } + return DeleteObjectAction + case fiber.MethodPost: + // CompleteMultipartUpload is the only POST that enforces them. + if query.Has("uploadId") { + return PutObjectAction + } + return "" + default: + return "" + } +} + +// trimETagQuotes strips one leading and one trailing double quote from an +// ETag-valued header, leaving any other value (notably If-None-Match's "*") +// untouched. +func trimETagQuotes(s string) string { + if len(s) >= 2 && s[0] == '"' && s[len(s)-1] == '"' { + return s[1 : len(s)-1] + } + return s +} diff --git a/auth/condition_context_test.go b/auth/condition_context_test.go new file mode 100644 index 00000000..d84c9736 --- /dev/null +++ b/auth/condition_context_test.go @@ -0,0 +1,304 @@ +// Copyright 2026 Versity Software +// This file is licensed under the Apache License, Version 2.0 +// (the "License"); you may not use this file except in compliance +// with the License. You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, +// software distributed under the License is distributed on an +// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +// KIND, either express or implied. See the License for the +// specific language governing permissions and limitations +// under the License. + +package auth + +import ( + "testing" + + "github.com/gofiber/fiber/v3" + "github.com/stretchr/testify/assert" + "github.com/valyala/fasthttp" +) + +func TestRequestConditionContextConditionalWriteKeys(t *testing.T) { + tests := []struct { + name string + method string + ifMatch string + ifNoneMatch string + copySource string + // uri defaults to an object path when empty. + uri string + query string + actions []Action + want map[string][]string + }{ + { + name: "no conditional headers", + method: fiber.MethodPut, + actions: []Action{PutObjectAction}, + want: map[string][]string{}, + }, + { + name: "If-Match quotes are stripped", + method: fiber.MethodPut, + ifMatch: `"abc123"`, + actions: []Action{PutObjectAction}, + want: map[string][]string{"s3:if-match": {"abc123"}}, + }, + { + // Both wire forms land on the same context value, so a policy + // always compares against the bare ETag. + name: "unquoted If-Match is taken as-is", + method: fiber.MethodPut, + ifMatch: "abc123", + actions: []Action{PutObjectAction}, + want: map[string][]string{"s3:if-match": {"abc123"}}, + }, + { + // A multi-ETag If-Match is one value, not a list: the gateway + // compares the whole header against the object's ETag, and the + // key carries exactly what that comparison sees. + name: "a multi-ETag If-Match is one value", + method: fiber.MethodPut, + ifMatch: `"abc123", "def456"`, + actions: []Action{PutObjectAction}, + want: map[string][]string{"s3:if-match": {`abc123", "def456`}}, + }, + { + name: "If-None-Match wildcard", + method: fiber.MethodPut, + ifNoneMatch: "*", + actions: []Action{PutObjectAction}, + want: map[string][]string{"s3:if-none-match": {"*"}}, + }, + { + name: "both headers on one request", + method: fiber.MethodPut, + ifMatch: `"abc123"`, + ifNoneMatch: "*", + actions: []Action{PutObjectAction}, + want: map[string][]string{ + "s3:if-match": {"abc123"}, + "s3:if-none-match": {"*"}, + }, + }, + { + // An upload carrying tagging or lock headers is authorized + // under those actions too; s3:PutObject is still among them. + name: "an upload authorized under several actions", + method: fiber.MethodPut, + ifMatch: `"abc123"`, + actions: []Action{PutObjectAction, PutObjectTaggingAction, PutObjectRetentionAction}, + want: map[string][]string{"s3:if-match": {"abc123"}}, + }, + { + name: "CompleteMultipartUpload populates the keys", + method: fiber.MethodPost, + query: "uploadId=abc", + ifMatch: `"abc123"`, + actions: []Action{PutObjectAction}, + want: map[string][]string{"s3:if-match": {"abc123"}}, + }, + { + name: "DeleteObject populates s3:if-match", + method: fiber.MethodDelete, + ifMatch: `"abc123"`, + actions: []Action{DeleteObjectAction}, + want: map[string][]string{"s3:if-match": {"abc123"}}, + }, + { + // A delete is authorized as s3:DeleteObject, which + // s3:if-none-match doesn't apply to — DeleteObject reads no + // If-None-Match, so the header changes nothing. + name: "a delete populates no s3:if-none-match", + method: fiber.MethodDelete, + ifMatch: `"abc123"`, + ifNoneMatch: "*", + actions: []Action{DeleteObjectAction}, + want: map[string][]string{"s3:if-match": {"abc123"}}, + }, + { + // A versioned delete is authorized as s3:DeleteObjectVersion, + // which neither key applies to, even though the gateway does + // check the precondition against the named version. + name: "a versioned delete populates neither key", + method: fiber.MethodDelete, + query: "versionId=v1", + ifMatch: `"abc123"`, + ifNoneMatch: "*", + actions: []Action{DeleteObjectVersionAction}, + want: map[string][]string{}, + }, + { + // A bucket sub-resource write is a bare PUT carrying none of + // the object sub-resources, so only the action it authorizes + // under separates it from an upload. + name: "PutBucketVersioning populates neither key", + method: fiber.MethodPut, + uri: "/bucket", + query: "versioning=", + ifMatch: `"abc123"`, + actions: []Action{PutBucketVersioningAction}, + want: map[string][]string{}, + }, + { + name: "PutBucketPolicy populates neither key", + method: fiber.MethodPut, + uri: "/bucket", + query: "policy=", + ifMatch: `"abc123"`, + ifNoneMatch: "*", + actions: []Action{PutBucketPolicyAction}, + want: map[string][]string{}, + }, + { + name: "CreateBucket populates neither key", + method: fiber.MethodPut, + uri: "/bucket", + ifMatch: `"abc123"`, + ifNoneMatch: "*", + actions: []Action{CreateBucketAction}, + want: map[string][]string{}, + }, + { + name: "DeleteBucket populates neither key", + method: fiber.MethodDelete, + uri: "/bucket", + ifMatch: `"abc123"`, + actions: []Action{DeleteBucketAction}, + want: map[string][]string{}, + }, + { + // The object-lock paths authorize s3:BypassGovernanceRetention, + // an action neither key applies to, on the very requests that + // do carry a precondition. + name: "a governance bypass populates neither key", + method: fiber.MethodPut, + ifMatch: `"abc123"`, + ifNoneMatch: "*", + actions: []Action{BypassGovernanceRetentionAction}, + want: map[string][]string{}, + }, + { + name: "no actions populates neither key", + method: fiber.MethodPut, + ifMatch: `"abc123"`, + ifNoneMatch: "*", + want: map[string][]string{}, + }, + { + // A form upload and a DeleteObjects batch are POSTs that never + // look at the headers. + name: "a form upload populates neither key", + method: fiber.MethodPost, + ifMatch: `"abc123"`, + ifNoneMatch: "*", + actions: []Action{PutObjectAction}, + want: map[string][]string{}, + }, + { + name: "a DeleteObjects batch populates neither key", + method: fiber.MethodPost, + uri: "/bucket", + query: "delete=", + ifMatch: `"abc123"`, + actions: []Action{DeleteObjectAction}, + want: map[string][]string{}, + }, + { + name: "an upload part populates neither key", + method: fiber.MethodPut, + query: "partNumber=1&uploadId=abc", + ifMatch: `"abc123"`, + actions: []Action{PutObjectAction}, + want: map[string][]string{}, + }, + { + name: "a tagging write populates neither key", + method: fiber.MethodPut, + query: "tagging=", + ifMatch: `"abc123"`, + actions: []Action{PutObjectTaggingAction}, + want: map[string][]string{}, + }, + { + name: "an ACL write populates neither key", + method: fiber.MethodPut, + query: "acl=", + ifMatch: `"abc123"`, + actions: []Action{PutObjectAclAction}, + want: map[string][]string{}, + }, + { + // On a read the same headers are ordinary HTTP cache + // preconditions, not conditional writes. + name: "GET populates neither key", + method: fiber.MethodGet, + ifMatch: `"abc123"`, + ifNoneMatch: `"abc123"`, + actions: []Action{GetObjectAction}, + want: map[string][]string{}, + }, + { + name: "HEAD populates neither key", + method: fiber.MethodHead, + ifMatch: `"abc123"`, + ifNoneMatch: `"abc123"`, + actions: []Action{GetObjectAction}, + want: map[string][]string{}, + }, + { + // A copy takes its preconditions from the + // X-Amz-Copy-Source-If-* headers, so a plain one is ignored. + // It is authorized as s3:PutObject like any other upload, so + // only the copy-source header separates the two. + name: "a copy populates neither key", + method: fiber.MethodPut, + ifMatch: `"abc123"`, + ifNoneMatch: "*", + copySource: "/src-bucket/src-key", + actions: []Action{PutObjectAction}, + want: map[string][]string{}, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + app := fiber.New() + fctx := &fasthttp.RequestCtx{} + fctx.Request.Header.SetMethod(tt.method) + uri := tt.uri + if uri == "" { + uri = "/bucket/object" + } + if tt.query != "" { + uri += "?" + tt.query + } + fctx.Request.SetRequestURI(uri) + if tt.ifMatch != "" { + fctx.Request.Header.Set("If-Match", tt.ifMatch) + } + if tt.ifNoneMatch != "" { + fctx.Request.Header.Set("If-None-Match", tt.ifNoneMatch) + } + if tt.copySource != "" { + fctx.Request.Header.Set("X-Amz-Copy-Source", tt.copySource) + } + + ctx := app.AcquireCtx(fctx) + defer app.ReleaseCtx(ctx) + + condCtx := requestConditionContext(ctx, tt.actions) + for _, key := range []string{"s3:if-match", "s3:if-none-match"} { + if want, ok := tt.want[key]; ok { + assert.Equal(t, want, condCtx[key]) + continue + } + assert.NotContains(t, condCtx, key) + } + }) + } +} diff --git a/auth/object_lock.go b/auth/object_lock.go index 826510f5..ca8dcb25 100644 --- a/auth/object_lock.go +++ b/auth/object_lock.go @@ -234,7 +234,7 @@ func IsObjectLockRetentionPutAllowed(ctx fiber.Ctx, be backend.Backend, iam IAMS // or switching it to COMPLIANCE — with the bypass header. That needs // s3:BypassGovernanceRetention, via the bucket policy and/or (when // configured) the IAM identity policy. - if err := verifyBypassGovernancePermission(ctx.RequestCtx(), be, iam, acc, bucket, object, BypassRequested, false, requestConditionContext(ctx)); err != nil { + if err := verifyBypassGovernancePermission(ctx.RequestCtx(), be, iam, acc, bucket, object, BypassRequested, false, requestConditionContext(ctx, []Action{BypassGovernanceRetentionAction})); err != nil { debuglogger.Logf("the user is missing 's3:BypassGovernanceRetention' permission: %v", err) return err } @@ -393,7 +393,7 @@ func CheckObjectAccess(ctx fiber.Ctx, bucket string, acc Account, objects []type return err } - condCtx := requestConditionContext(ctx) + condCtx := requestConditionContext(ctx, []Action{BypassGovernanceRetentionAction}) for _, obj := range objects { if err := state.checkObject(rctx, be, iam, acc, bucket, obj, bypass, isBucketPublic, condCtx); err != nil { return err diff --git a/tests/integration/Access_Control.go b/tests/integration/Access_Control.go index ec4474a3..0c600806 100644 --- a/tests/integration/Access_Control.go +++ b/tests/integration/Access_Control.go @@ -1677,3 +1677,327 @@ func AccessControl_bucket_policy_condition_not_ip_address_deny(s *S3Conf) error return checkApiErr(err, s3err.GetAPIError(s3err.ErrAccessDenied)) }) } + +// AccessControl_bucket_policy_condition_if_none_match_required is AWS's +// documented "enforce conditional writes" pattern: an unconditional Allow +// paired with a Deny that fires whenever the key is absent, forcing every +// upload to carry If-None-Match. +func AccessControl_bucket_policy_condition_if_none_match_required(s *S3Conf) error { + testName := "AccessControl_bucket_policy_condition_if_none_match_required" + return actionHandler(s, testName, func(s3client *s3.Client, bucket string) error { + testuser := getUser("user") + if err := createUsers(s, []user{testuser}); err != nil { + return err + } + userClient := s.getUserClient(testuser) + + if err := putBucketPolicyDoc(s, bucket, + bucketStatement{ + Effect: "Allow", + Principal: testuser.access, + Action: "s3:PutObject", + Resource: fmt.Sprintf("arn:aws:s3:::%s/*", bucket), + }, + bucketStatement{ + Effect: "Deny", + Principal: testuser.access, + Action: "s3:PutObject", + Resource: fmt.Sprintf("arn:aws:s3:::%s/*", bucket), + Condition: json.RawMessage(`{"Null":{"s3:if-none-match":"true"}}`), + }, + ); err != nil { + return err + } + + ctx, cancel := context.WithTimeout(context.Background(), shortTimeout) + _, err := userClient.PutObject(ctx, &s3.PutObjectInput{ + Bucket: &bucket, + Key: getPtr("unconditional"), + Body: bytes.NewReader([]byte("data")), + }) + cancel() + if err := checkApiErr(err, s3err.GetExplicitDenyAccessErr(testuser.access, "s3:PutObject", + fmt.Sprintf("arn:aws:s3:::%s/unconditional", bucket), "a resource-based policy")); err != nil { + return fmt.Errorf("an upload without If-None-Match must be denied: %w", err) + } + + ctx, cancel = context.WithTimeout(context.Background(), shortTimeout) + _, err = userClient.PutObject(ctx, &s3.PutObjectInput{ + Bucket: &bucket, + Key: getPtr("conditional"), + Body: bytes.NewReader([]byte("data")), + IfNoneMatch: getPtr("*"), + }) + cancel() + if err != nil { + return fmt.Errorf("an upload carrying If-None-Match must be allowed: %w", err) + } + + // The same request once the key exists is still authorized; it now + // fails on the precondition itself, which the policy has no say in. + ctx, cancel = context.WithTimeout(context.Background(), shortTimeout) + _, err = userClient.PutObject(ctx, &s3.PutObjectInput{ + Bucket: &bucket, + Key: getPtr("conditional"), + Body: bytes.NewReader([]byte("data")), + IfNoneMatch: getPtr("*"), + }) + cancel() + return checkApiErr(err, s3err.GetAPIError(s3err.ErrPreconditionFailed)) + }) +} + +// AccessControl_bucket_policy_condition_if_none_match_value covers the +// key's value rather than its presence: the only value S3 accepts in an +// If-None-Match write is "*", and that is what the key carries verbatim - +// under StringEquals "*" is a literal, not a wildcard. +func AccessControl_bucket_policy_condition_if_none_match_value(s *S3Conf) error { + testName := "AccessControl_bucket_policy_condition_if_none_match_value" + return actionHandler(s, testName, func(s3client *s3.Client, bucket string) error { + testuser := getUser("user") + if err := createUsers(s, []user{testuser}); err != nil { + return err + } + userClient := s.getUserClient(testuser) + + for i, tc := range []struct { + name string + condition string + wantAllow bool + }{ + {"StringEquals on the literal wildcard matches", `{"StringEquals":{"s3:if-none-match":"*"}}`, true}, + {"StringEquals on any other value does not", `{"StringEquals":{"s3:if-none-match":"abc123"}}`, false}, + {"Null:false matches a present key", `{"Null":{"s3:if-none-match":"false"}}`, true}, + {"Null:true does not match a present key", `{"Null":{"s3:if-none-match":"true"}}`, false}, + {"StringLike wildcard matches", `{"StringLike":{"s3:if-none-match":"*"}}`, true}, + } { + if err := putBucketPolicyDoc(s, bucket, bucketStatement{ + Effect: "Allow", + Principal: testuser.access, + Action: "s3:PutObject", + Resource: fmt.Sprintf("arn:aws:s3:::%s/*", bucket), + Condition: json.RawMessage(tc.condition), + }); err != nil { + return fmt.Errorf("%s: %w", tc.name, err) + } + + ctx, cancel := context.WithTimeout(context.Background(), shortTimeout) + _, err := userClient.PutObject(ctx, &s3.PutObjectInput{ + Bucket: &bucket, + Key: getPtr(fmt.Sprintf("obj-%v", i)), + Body: bytes.NewReader([]byte("data")), + IfNoneMatch: getPtr("*"), + }) + cancel() + + if tc.wantAllow { + if err != nil { + return fmt.Errorf("%s: expected success, got %w", tc.name, err) + } + continue + } + if err := checkApiErr(err, s3err.GetAPIError(s3err.ErrAccessDenied)); err != nil { + return fmt.Errorf("%s: %w", tc.name, err) + } + } + return nil + }) +} + +// AccessControl_bucket_policy_condition_if_match_value covers +// s3:if-match's value shape: the key carries the ETag with the surrounding +// quotes stripped, so a policy compares against the bare ETag whichever +// form the client puts on the wire. +func AccessControl_bucket_policy_condition_if_match_value(s *S3Conf) error { + testName := "AccessControl_bucket_policy_condition_if_match_value" + return actionHandler(s, testName, func(s3client *s3.Client, bucket string) error { + testuser := getUser("user") + if err := createUsers(s, []user{testuser}); err != nil { + return err + } + userClient := s.getUserClient(testuser) + + etag, err := putObjectAndGetETag(s3client, bucket, "obj") + if err != nil { + return err + } + bare := strings.Trim(etag, `"`) + + for _, tc := range []struct { + name string + condition func() string + ifMatch *string + wantAllow bool + azureUnsupp bool + }{ + {"quoted header matches a bare policy value", + func() string { return fmt.Sprintf(`{"StringEquals":{"s3:if-match":%q}}`, bare) }, &etag, true, false}, + {"unquoted header matches the same bare policy value", + func() string { return fmt.Sprintf(`{"StringEquals":{"s3:if-match":%q}}`, bare) }, &bare, true, false}, + {"a policy value carrying the quotes never matches", + func() string { return fmt.Sprintf(`{"StringEquals":{"s3:if-match":%q}}`, etag) }, &etag, false, true}, + {"a different ETag does not match", + func() string { return `{"StringEquals":{"s3:if-match":"0123456789abcdef0123456789abcdef"}}` }, &etag, false, false}, + {"StringLike wildcard matches", + func() string { return `{"StringLike":{"s3:if-match":"*"}}` }, &etag, true, false}, + {"Null:false matches a present key", + func() string { return `{"Null":{"s3:if-match":"false"}}` }, &etag, true, false}, + } { + if tc.azureUnsupp && s.azureTests { + continue + } + if err := putBucketPolicyDoc(s, bucket, bucketStatement{ + Effect: "Allow", + Principal: testuser.access, + Action: "s3:PutObject", + Resource: fmt.Sprintf("arn:aws:s3:::%s/*", bucket), + Condition: json.RawMessage(tc.condition()), + }); err != nil { + return fmt.Errorf("%s: %w", tc.name, err) + } + + ctx, cancel := context.WithTimeout(context.Background(), shortTimeout) + _, err := userClient.PutObject(ctx, &s3.PutObjectInput{ + Bucket: &bucket, + Key: getPtr("obj"), + Body: bytes.NewReader([]byte("data")), + IfMatch: tc.ifMatch, + }) + cancel() + + if tc.wantAllow { + if err != nil { + return fmt.Errorf("%s: expected success, got %w", tc.name, err) + } + // The overwrite may have changed the ETag, so every + // subsequent case has to condition on the current one. + if etag, err = headObjectETag(s3client, bucket, "obj"); err != nil { + return err + } + bare = strings.Trim(etag, `"`) + continue + } + if err := checkApiErr(err, s3err.GetAPIError(s3err.ErrAccessDenied)); err != nil { + return fmt.Errorf("%s: %w", tc.name, err) + } + } + return nil + }) +} + +// AccessControl_bucket_policy_condition_if_match_delete_object covers the +// half of s3:if-match that isn't an upload: S3's conditional delete, which +// is the only other action a bucket policy may name the key on. +func AccessControl_bucket_policy_condition_if_match_delete_object(s *S3Conf) error { + testName := "AccessControl_bucket_policy_condition_if_match_delete_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 + } + userClient := s.getUserClient(testuser) + + etag, err := putObjectAndGetETag(s3client, bucket, "obj") + if err != nil { + return err + } + + // A delete that doesn't name the object's ETag is not authorized + // at all, so it never reaches the precondition check. + if err := putBucketPolicyDoc(s, bucket, bucketStatement{ + Effect: "Allow", + Principal: testuser.access, + Action: "s3:DeleteObject", + Resource: fmt.Sprintf("arn:aws:s3:::%s/*", bucket), + Condition: json.RawMessage(fmt.Sprintf(`{"StringEquals":{"s3:if-match":%q}}`, strings.Trim(etag, `"`))), + }); err != nil { + return err + } + + ctx, cancel := context.WithTimeout(context.Background(), shortTimeout) + _, err = userClient.DeleteObject(ctx, &s3.DeleteObjectInput{ + Bucket: &bucket, + Key: getPtr("obj"), + }) + cancel() + if err := checkApiErr(err, s3err.GetAPIError(s3err.ErrAccessDenied)); err != nil { + return fmt.Errorf("an unconditional delete must be denied: %w", err) + } + + ctx, cancel = context.WithTimeout(context.Background(), shortTimeout) + _, err = userClient.DeleteObject(ctx, &s3.DeleteObjectInput{ + Bucket: &bucket, + Key: getPtr("obj"), + IfMatch: &etag, + }) + cancel() + if err != nil { + return fmt.Errorf("a delete naming the object's ETag must be allowed: %w", err) + } + return nil + }) +} + +// AccessControl_bucket_policy_condition_if_match_versioned_delete pins the +// edge of s3:if-match's action set. A delete naming a version removes that +// version rather than overwriting the current one, so it is authorized as +// s3:DeleteObjectVersion — an action the key doesn't apply to, which is why +// PutBucketPolicy rejects a statement pairing the two. The key has to stay +// absent from the request context to match, or a wildcard-action statement +// would grant on a precondition the policy language can't name. +func AccessControl_bucket_policy_condition_if_match_versioned_delete(s *S3Conf) error { + testName := "AccessControl_bucket_policy_condition_if_match_versioned_delete" + return actionHandler(s, testName, func(s3client *s3.Client, bucket string) error { + testuser := getUser("user") + if err := createUsers(s, []user{testuser}); err != nil { + return err + } + userClient := s.getUserClient(testuser) + + etag, err := putObjectAndGetETag(s3client, bucket, "obj") + if err != nil { + return err + } + + // s3:* is the only action a bucket policy can name s3:if-match on + // and still reach a versioned delete: a wildcard is exempt from + // the applicability check an explicit s3:DeleteObjectVersion fails. + if err := putBucketPolicyDoc(s, bucket, bucketStatement{ + Effect: "Allow", + Principal: testuser.access, + Action: "s3:*", + Resource: fmt.Sprintf("arn:aws:s3:::%s/*", bucket), + Condition: json.RawMessage(`{"Null":{"s3:if-match":"false"}}`), + }); err != nil { + return err + } + + // "null" is the version id every object carries until versioning is + // enabled, so this is a versioned delete on any backend. + ctx, cancel := context.WithTimeout(context.Background(), shortTimeout) + _, err = userClient.DeleteObject(ctx, &s3.DeleteObjectInput{ + Bucket: &bucket, + Key: getPtr("obj"), + VersionId: getPtr("null"), + IfMatch: &etag, + }) + cancel() + if err := checkApiErr(err, s3err.GetAPIError(s3err.ErrAccessDenied)); err != nil { + return fmt.Errorf("a versioned delete must leave s3:if-match absent: %w", err) + } + + // The same header on the same object, minus the version, is a plain + // s3:DeleteObject and does populate the key. + ctx, cancel = context.WithTimeout(context.Background(), shortTimeout) + _, err = userClient.DeleteObject(ctx, &s3.DeleteObjectInput{ + Bucket: &bucket, + Key: getPtr("obj"), + IfMatch: &etag, + }) + cancel() + if err != nil { + return fmt.Errorf("an unversioned delete carrying If-Match must be allowed: %w", err) + } + return nil + }) +} diff --git a/tests/integration/PutBucketPolicy.go b/tests/integration/PutBucketPolicy.go index a9d4ecaa..612f9daa 100644 --- a/tests/integration/PutBucketPolicy.go +++ b/tests/integration/PutBucketPolicy.go @@ -807,6 +807,19 @@ func PutBucketPolicy_condition_action_mismatch(s *S3Conf) error { // an explicit multi-action list requires every action to // support the key, even though s3:PutObject alone would. {`["s3:GetObject","s3:PutObject"]`, `{"StringEquals":{"s3:x-amz-acl":"public-read"}}`}, + // s3:if-match applies to the conditional-write actions only, + // so a read action is rejected... + {`"s3:GetObject"`, `{"StringEquals":{"s3:if-match":"abc123"}}`}, + // ... and so is a versioned delete, which names the version to + // remove rather than taking an If-Match. + {`"s3:DeleteObjectVersion"`, `{"StringEquals":{"s3:if-match":"abc123"}}`}, + // ... as is a bucket-level write, which reads no If-Match at + // all. + {`"s3:PutBucketVersioning"`, `{"StringEquals":{"s3:if-match":"abc123"}}`}, + // s3:if-none-match is narrower still: only s3:PutObject can + // require that the object not already exist. + {`"s3:DeleteObject"`, `{"Null":{"s3:if-none-match":"false"}}`}, + {`["s3:PutObject","s3:DeleteObject"]`, `{"Null":{"s3:if-none-match":"false"}}`}, } { doc := fmt.Sprintf(`{"Statement":[{"Effect":"Allow","Principal":"*","Action":%s, "Resource":"arn:aws:s3:::%s/*","Condition":%s}]}`, tc.action, bucket, tc.condition) @@ -826,6 +839,43 @@ func PutBucketPolicy_condition_action_mismatch(s *S3Conf) error { }) } +func PutBucketPolicy_condition_conditional_write_keys(s *S3Conf) error { + testName := "PutBucketPolicy_condition_conditional_write_keys" + return actionHandler(s, testName, func(s3client *s3.Client, bucket string) error { + for _, tc := range []struct { + action string + condition string + }{ + {`"s3:PutObject"`, `{"StringEquals":{"s3:if-match":"abc123"}}`}, + {`"s3:PutObject"`, `{"Null":{"s3:if-none-match":"false"}}`}, + // s3:if-match also covers S3's conditional delete. + {`"s3:DeleteObject"`, `{"StringEquals":{"s3:if-match":"abc123"}}`}, + {`["s3:PutObject","s3:DeleteObject"]`, `{"StringEquals":{"s3:if-match":"abc123"}}`}, + // a wildcard action is exempt from the applicability check + {`"s3:*"`, `{"Null":{"s3:if-none-match":"true"}}`}, + // key names are case-insensitive + {`"s3:PutObject"`, `{"StringEquals":{"S3:IF-MATCH":"abc123"}}`}, + {`"s3:PutObject"`, `{"Null":{"s3:If-None-Match":"false"}}`}, + // both keys in one statement + {`"s3:PutObject"`, `{"Null":{"s3:if-match":"true","s3:if-none-match":"false"}}`}, + } { + doc := fmt.Sprintf(`{"Statement":[{"Effect":"Allow","Principal":"*","Action":%s, + "Resource":"arn:aws:s3:::%s/*","Condition":%s}]}`, tc.action, bucket, tc.condition) + + ctx, cancel := context.WithTimeout(context.Background(), shortTimeout) + _, err := s3client.PutBucketPolicy(ctx, &s3.PutBucketPolicyInput{ + Bucket: &bucket, + Policy: &doc, + }) + cancel() + if err != nil { + return fmt.Errorf("action %s with condition %s: %w", tc.action, tc.condition, err) + } + } + return nil + }) +} + func PutBucketPolicy_condition_invalid_ip(s *S3Conf) error { testName := "PutBucketPolicy_condition_invalid_ip" return actionHandler(s, testName, func(s3client *s3.Client, bucket string) error { diff --git a/tests/integration/group-tests.go b/tests/integration/group-tests.go index d132cf8e..6b652ba2 100644 --- a/tests/integration/group-tests.go +++ b/tests/integration/group-tests.go @@ -633,6 +633,7 @@ func TestPutBucketPolicy(ts *TestState) { ts.Run(PutBucketPolicy_condition_invalid_operator) ts.Run(PutBucketPolicy_condition_invalid_key) ts.Run(PutBucketPolicy_condition_action_mismatch) + ts.Run(PutBucketPolicy_condition_conditional_write_keys) ts.Run(PutBucketPolicy_condition_invalid_ip) } @@ -1744,6 +1745,14 @@ func TestS3IAMAccessControl(ts *TestState) { ts.Run(S3IAMAccessControl_condition_principal_tag) ts.Run(S3IAMAccessControl_condition_on_deny_statement) ts.Run(S3IAMAccessControl_condition_multiple_keys_anded) + ts.Run(S3IAMAccessControl_condition_if_none_match_required) + ts.Run(S3IAMAccessControl_condition_conditional_write_values) + ts.Run(S3IAMAccessControl_condition_if_match_delete_object) + ts.Run(S3IAMAccessControl_condition_if_match_versioned_delete) + ts.Run(S3IAMAccessControl_condition_conditional_write_keys_ignore_reads) + ts.Run(S3IAMAccessControl_condition_conditional_write_keys_ignore_copies) + ts.Run(S3IAMAccessControl_condition_if_match_bucket_level_write) + ts.Run(S3IAMAccessControl_condition_if_match_ignores_delete_objects) ts.Run(S3IAMAccessControl_inactive_and_deleted_credentials) ts.Run(S3IAMAccessControl_access_key_last_used_records_s3) ts.Run(S3IAMPrincipal_accepted_forms) @@ -1875,6 +1884,11 @@ func TestAccessControl(ts *TestState) { ts.Run(AccessControl_bucket_policy_condition_bool_operator) ts.Run(AccessControl_bucket_policy_condition_binary_operator) ts.Run(AccessControl_bucket_policy_condition_null_operator) + ts.Run(AccessControl_bucket_policy_condition_if_none_match_required) + ts.Run(AccessControl_bucket_policy_condition_if_none_match_value) + ts.Run(AccessControl_bucket_policy_condition_if_match_value) + ts.Run(AccessControl_bucket_policy_condition_if_match_delete_object) + ts.Run(AccessControl_bucket_policy_condition_if_match_versioned_delete) } func TestPublicBuckets(ts *TestState) { @@ -2201,6 +2215,14 @@ func GetIntTests() IntTests { "S3IAMAccessControl_condition_principal_tag": S3IAMAccessControl_condition_principal_tag, "S3IAMAccessControl_condition_on_deny_statement": S3IAMAccessControl_condition_on_deny_statement, "S3IAMAccessControl_condition_multiple_keys_anded": S3IAMAccessControl_condition_multiple_keys_anded, + "S3IAMAccessControl_condition_if_none_match_required": S3IAMAccessControl_condition_if_none_match_required, + "S3IAMAccessControl_condition_conditional_write_values": S3IAMAccessControl_condition_conditional_write_values, + "S3IAMAccessControl_condition_if_match_delete_object": S3IAMAccessControl_condition_if_match_delete_object, + "S3IAMAccessControl_condition_if_match_versioned_delete": S3IAMAccessControl_condition_if_match_versioned_delete, + "S3IAMAccessControl_condition_conditional_write_keys_ignore_reads": S3IAMAccessControl_condition_conditional_write_keys_ignore_reads, + "S3IAMAccessControl_condition_conditional_write_keys_ignore_copies": S3IAMAccessControl_condition_conditional_write_keys_ignore_copies, + "S3IAMAccessControl_condition_if_match_bucket_level_write": S3IAMAccessControl_condition_if_match_bucket_level_write, + "S3IAMAccessControl_condition_if_match_ignores_delete_objects": S3IAMAccessControl_condition_if_match_ignores_delete_objects, "S3IAMAccessControl_inactive_and_deleted_credentials": S3IAMAccessControl_inactive_and_deleted_credentials, "S3IAMSession_bucket_policy_role_arn_covers_every_session": S3IAMSession_bucket_policy_role_arn_covers_every_session, "S3IAMSession_bucket_policy_names_one_session": S3IAMSession_bucket_policy_names_one_session, @@ -3167,6 +3189,7 @@ func GetIntTests() IntTests { "PutBucketPolicy_condition_invalid_operator": PutBucketPolicy_condition_invalid_operator, "PutBucketPolicy_condition_invalid_key": PutBucketPolicy_condition_invalid_key, "PutBucketPolicy_condition_action_mismatch": PutBucketPolicy_condition_action_mismatch, + "PutBucketPolicy_condition_conditional_write_keys": PutBucketPolicy_condition_conditional_write_keys, "PutBucketPolicy_condition_invalid_ip": PutBucketPolicy_condition_invalid_ip, "GetBucketPolicy_non_existing_bucket": GetBucketPolicy_non_existing_bucket, "GetBucketPolicy_not_set": GetBucketPolicy_not_set, @@ -3388,6 +3411,11 @@ func GetIntTests() IntTests { "AccessControl_bucket_policy_condition_bool_operator": AccessControl_bucket_policy_condition_bool_operator, "AccessControl_bucket_policy_condition_binary_operator": AccessControl_bucket_policy_condition_binary_operator, "AccessControl_bucket_policy_condition_null_operator": AccessControl_bucket_policy_condition_null_operator, + "AccessControl_bucket_policy_condition_if_none_match_required": AccessControl_bucket_policy_condition_if_none_match_required, + "AccessControl_bucket_policy_condition_if_none_match_value": AccessControl_bucket_policy_condition_if_none_match_value, + "AccessControl_bucket_policy_condition_if_match_value": AccessControl_bucket_policy_condition_if_match_value, + "AccessControl_bucket_policy_condition_if_match_delete_object": AccessControl_bucket_policy_condition_if_match_delete_object, + "AccessControl_bucket_policy_condition_if_match_versioned_delete": AccessControl_bucket_policy_condition_if_match_versioned_delete, "PublicBucket_default_private_bucket": PublicBucket_default_private_bucket, "PublicBucket_public_bucket_policy": PublicBucket_public_bucket_policy, "PublicBucket_public_object_policy": PublicBucket_public_object_policy, diff --git a/tests/integration/s3_iam_access_control.go b/tests/integration/s3_iam_access_control.go index a7d19e60..5db50930 100644 --- a/tests/integration/s3_iam_access_control.go +++ b/tests/integration/s3_iam_access_control.go @@ -15,7 +15,9 @@ package integration import ( + "bytes" "context" + "encoding/json" "fmt" "strings" "time" @@ -2103,6 +2105,515 @@ func S3IAMAccessControl_access_key_last_used_records_s3(s *S3Conf) error { }) } +// S3IAMAccessControl_condition_if_none_match_required is AWS's documented +// "enforce conditional writes" pattern expressed as an identity policy: an +// unconditional Allow paired with a Deny that fires whenever the key is +// absent. +func S3IAMAccessControl_condition_if_none_match_required(s *S3Conf) error { + testName := "S3IAMAccessControl_condition_if_none_match_required" + return s3IAMActionHandler(s, testName, func(root *iam.Client, bucket string) error { + user, cleanup, err := newS3IAMUser(root, s, map[string]string{ + "p": policyDoc( + accessStatement{Effect: "Allow", Action: actS3PutObject, Resource: objectsArn(bucket)}, + accessStatement{ + Effect: "Deny", Action: actS3PutObject, Resource: objectsArn(bucket), + Condition: cond("Null", "s3:if-none-match", "true"), + }, + ), + }) + if err != nil { + return err + } + defer cleanup() + + ctx, cancel := context.WithTimeout(context.Background(), shortTimeout) + _, err = user.client.PutObject(ctx, &s3.PutObjectInput{ + Bucket: &bucket, + Key: getPtr("unconditional"), + Body: bytes.NewReader([]byte("data")), + }) + cancel() + if err := checkApiErr(err, wantExplicitIdentityDeny(user.arn, actS3PutObject, objectArn(bucket, "unconditional"))); err != nil { + return fmt.Errorf("an upload without If-None-Match must be denied: %w", err) + } + + ctx, cancel = context.WithTimeout(context.Background(), shortTimeout) + _, err = user.client.PutObject(ctx, &s3.PutObjectInput{ + Bucket: &bucket, + Key: getPtr("conditional"), + Body: bytes.NewReader([]byte("data")), + IfNoneMatch: getPtr("*"), + }) + cancel() + if err != nil { + return fmt.Errorf("an upload carrying If-None-Match must be allowed: %w", err) + } + + // Still authorized once the key exists; only the precondition + // itself fails now, which the policy has no say in. + ctx, cancel = context.WithTimeout(context.Background(), shortTimeout) + _, err = user.client.PutObject(ctx, &s3.PutObjectInput{ + Bucket: &bucket, + Key: getPtr("conditional"), + Body: bytes.NewReader([]byte("data")), + IfNoneMatch: getPtr("*"), + }) + cancel() + return checkApiErr(err, s3err.GetAPIError(s3err.ErrPreconditionFailed)) + }) +} + +// S3IAMAccessControl_condition_conditional_write_values covers both keys' +// values on an upload: s3:if-match carries the ETag with its surrounding +// quotes stripped, and s3:if-none-match carries the only value S3 accepts +// on a write, the literal "*". +func S3IAMAccessControl_condition_conditional_write_values(s *S3Conf) error { + testName := "S3IAMAccessControl_condition_conditional_write_values" + return s3IAMActionHandler(s, testName, func(root *iam.Client, bucket string) error { + etag, err := putObjectAndGetETag(s.GetClient(), bucket, "obj") + if err != nil { + return err + } + bare := strings.Trim(etag, `"`) + + user, cleanup, err := newS3IAMUser(root, s, nil) + if err != nil { + return err + } + defer cleanup() + + for i, tc := range []struct { + name string + condition json.RawMessage + // ifMatch and ifNoneMatch are the headers the client sends; + // S3 rejects a request carrying both. + ifMatch *string + ifNoneMatch *string + wantAllow bool + }{ + { + name: "quoted If-Match header matches a bare policy value", + condition: cond("StringEquals", "s3:if-match", bare), + ifMatch: &etag, + wantAllow: true, + }, + { + name: "unquoted If-Match header matches the same bare policy value", + condition: cond("StringEquals", "s3:if-match", bare), + ifMatch: &bare, + wantAllow: true, + }, + { + name: "a policy value carrying the quotes never matches", + condition: cond("StringEquals", "s3:if-match", etag), + ifMatch: &etag, + }, + { + name: "a different ETag does not match", + condition: cond("StringEquals", "s3:if-match", "0123456789abcdef0123456789abcdef"), + ifMatch: &etag, + }, + { + name: "s3:if-match is absent without the header", + condition: cond("Null", "s3:if-match", "true"), + wantAllow: true, + }, + { + name: "an absent s3:if-match cannot satisfy StringEquals", + condition: cond("StringEquals", "s3:if-match", bare), + }, + { + // Key names are case-insensitive, values are not. + name: "key name case is ignored", + condition: cond("StringEquals", "S3:IF-MATCH", bare), + ifMatch: &etag, + wantAllow: true, + }, + { + name: "s3:if-none-match carries the literal wildcard", + condition: cond("StringEquals", "s3:if-none-match", "*"), + ifNoneMatch: getPtr("*"), + wantAllow: true, + }, + { + // "*" is a literal under StringEquals, so it cannot stand + // in for an arbitrary value. + name: "s3:if-none-match is absent without the header", + condition: cond("StringEquals", "s3:if-none-match", "*"), + }, + { + name: "one key is absent while the other is present", + condition: cond("Null", "s3:if-match", "true"), + ifNoneMatch: getPtr("*"), + wantAllow: true, + }, + } { + // An If-None-Match write has to target a key that doesn't exist + // yet, or it is authorized and then fails the precondition - + // every other case conditions on "obj"'s own ETag. + key := "obj" + if tc.ifNoneMatch != nil { + key = fmt.Sprintf("absent-%v", i) + } + + if err := func() error { + if err := putS3IAMUserPolicy(root, user, "p", policyDoc(accessStatement{ + Effect: "Allow", Action: actS3PutObject, Resource: objectsArn(bucket), + Condition: tc.condition, + })); err != nil { + return err + } + + ctx, cancel := context.WithTimeout(context.Background(), shortTimeout) + _, err := user.client.PutObject(ctx, &s3.PutObjectInput{ + Bucket: &bucket, + Key: &key, + Body: bytes.NewReader([]byte("data")), + IfMatch: tc.ifMatch, + IfNoneMatch: tc.ifNoneMatch, + }) + cancel() + + if !tc.wantAllow { + return checkApiErr(err, wantImplicitDeny(user.arn, actS3PutObject, objectArn(bucket, key))) + } + if err != nil { + return fmt.Errorf("expected the request to be allowed: %w", err) + } + // An overwrite may have changed the ETag, so the remaining + // cases have to condition on the current one. + if etag, err = headObjectETag(s.GetClient(), bucket, "obj"); err != nil { + return err + } + bare = strings.Trim(etag, `"`) + return nil + }(); err != nil { + return fmt.Errorf("%s: %w", tc.name, err) + } + } + return nil + }) +} + +// S3IAMAccessControl_condition_if_match_delete_object covers the half of +// s3:if-match that isn't an upload: S3's conditional delete. +func S3IAMAccessControl_condition_if_match_delete_object(s *S3Conf) error { + testName := "S3IAMAccessControl_condition_if_match_delete_object" + return s3IAMActionHandler(s, testName, func(root *iam.Client, bucket string) error { + etag, err := putObjectAndGetETag(s.GetClient(), bucket, "obj") + if err != nil { + return err + } + + user, cleanup, err := newS3IAMUser(root, s, map[string]string{ + "p": policyDoc(accessStatement{ + Effect: "Allow", Action: actS3DeleteObject, Resource: objectsArn(bucket), + Condition: cond("StringEquals", "s3:if-match", strings.Trim(etag, `"`)), + }), + }) + if err != nil { + return err + } + defer cleanup() + + ctx, cancel := context.WithTimeout(context.Background(), shortTimeout) + _, err = user.client.DeleteObject(ctx, &s3.DeleteObjectInput{Bucket: &bucket, Key: getPtr("obj")}) + cancel() + if err := checkApiErr(err, wantImplicitDeny(user.arn, actS3DeleteObject, objectArn(bucket, "obj"))); err != nil { + return fmt.Errorf("an unconditional delete must be denied: %w", err) + } + + ctx, cancel = context.WithTimeout(context.Background(), shortTimeout) + _, err = user.client.DeleteObject(ctx, &s3.DeleteObjectInput{ + Bucket: &bucket, + Key: getPtr("obj"), + IfMatch: &etag, + }) + cancel() + if err != nil { + return fmt.Errorf("a delete naming the object's ETag must be allowed: %w", err) + } + return nil + }) +} + +// S3IAMAccessControl_condition_if_match_versioned_delete pins the edge of +// s3:if-match's action set on the identity-policy side, where nothing +// validates a Condition's key against the action it names. A delete naming +// a version is authorized as s3:DeleteObjectVersion, which the key doesn't +// apply to, so it stays absent and a statement demanding it can never be +// satisfied +func S3IAMAccessControl_condition_if_match_versioned_delete(s *S3Conf) error { + testName := "S3IAMAccessControl_condition_if_match_versioned_delete" + return s3IAMActionHandler(s, testName, func(root *iam.Client, bucket string) error { + etag, err := putObjectAndGetETag(s.GetClient(), bucket, "obj") + if err != nil { + return err + } + + user, cleanup, err := newS3IAMUser(root, s, map[string]string{ + "p": policyDoc(accessStatement{ + Effect: "Allow", Action: actS3DeleteObjectVersion, Resource: objectsArn(bucket), + Condition: cond("StringEquals", "s3:if-match", strings.Trim(etag, `"`)), + }), + }) + if err != nil { + return err + } + defer cleanup() + + // "null" is the version id every object carries until versioning is + // enabled, so this is a versioned delete on any backend. + deleteVersion := func(ifMatch string) error { + ctx, cancel := context.WithTimeout(context.Background(), shortTimeout) + defer cancel() + _, err := user.client.DeleteObject(ctx, &s3.DeleteObjectInput{ + Bucket: &bucket, + Key: getPtr("obj"), + VersionId: getPtr("null"), + IfMatch: &ifMatch, + }) + return err + } + + if err := checkApiErr(deleteVersion(etag), + wantImplicitDeny(user.arn, actS3DeleteObjectVersion, objectArn(bucket, "obj"))); err != nil { + return fmt.Errorf("a versioned delete must leave s3:if-match absent: %w", err) + } + + // Absent, not merely different: the same request satisfies a + // statement requiring the key to be absent. + if err := putS3IAMUserPolicy(root, user, "p", policyDoc(accessStatement{ + Effect: "Allow", Action: actS3DeleteObjectVersion, Resource: objectsArn(bucket), + Condition: cond("Null", "s3:if-match", "true"), + })); err != nil { + return err + } + + // Authorized now, and the header still decides the delete: a + // mismatch fails the precondition the policy had no say in. + if err := checkApiErr(deleteVersion("0123456789abcdef0123456789abcdef"), + s3err.GetAPIError(s3err.ErrPreconditionFailed)); err != nil { + return fmt.Errorf("the precondition itself must still be enforced: %w", err) + } + if err := deleteVersion(etag); err != nil { + return fmt.Errorf("a versioned delete naming the object's ETag must be allowed: %w", err) + } + return nil + }) +} + +// S3IAMAccessControl_condition_if_match_bucket_level_write pins the last +// shape a bare PUT can take. A bucket sub-resource write carries none of +// the object sub-resources, so the request alone looks exactly like an +// upload; only the action it is authorized under separates the two. The +// gateway reads no If-Match there, so the key has to stay absent rather +// than satisfy a statement demanding a conditional write. +func S3IAMAccessControl_condition_if_match_bucket_level_write(s *S3Conf) error { + testName := "S3IAMAccessControl_condition_if_match_bucket_level_write" + return s3IAMActionHandler(s, testName, func(root *iam.Client, bucket string) error { + etag, err := putObjectAndGetETag(s.GetClient(), bucket, "obj") + if err != nil { + return err + } + bareETag := strings.Trim(etag, `"`) + + user, cleanup, err := newS3IAMUser(root, s, map[string]string{ + "p": policyDoc(accessStatement{ + Effect: "Allow", Action: actS3PutBucketOwnershipControls, Resource: bucketArn(bucket), + Condition: cond("StringEquals", "s3:if-match", bareETag), + }), + }) + if err != nil { + return err + } + defer cleanup() + + putOwnership := func() error { + ctx, cancel := context.WithTimeout(context.Background(), shortTimeout) + defer cancel() + _, err := user.client.PutBucketOwnershipControls(ctx, &s3.PutBucketOwnershipControlsInput{ + Bucket: &bucket, + OwnershipControls: &types.OwnershipControls{ + Rules: []types.OwnershipControlsRule{ + {ObjectOwnership: types.ObjectOwnershipBucketOwnerPreferred}, + }, + }, + }, withRequestHeader("If-Match", etag)) + return err + } + + if err := checkApiErr(putOwnership(), + wantImplicitDeny(user.arn, actS3PutBucketOwnershipControls, bucketArn(bucket))); err != nil { + return fmt.Errorf("a bucket-level write must leave s3:if-match absent: %w", err) + } + + // Absent, not merely different: the same request satisfies a + // statement requiring the key to be absent. + if err := putS3IAMUserPolicy(root, user, "p", policyDoc(accessStatement{ + Effect: "Allow", Action: actS3PutBucketOwnershipControls, Resource: bucketArn(bucket), + Condition: cond("Null", "s3:if-match", "true"), + })); err != nil { + return err + } + if err := putOwnership(); err != nil { + return fmt.Errorf("a bucket-level write must be allowed once the statement stops demanding the key: %w", err) + } + return nil + }) +} + +// S3IAMAccessControl_condition_conditional_write_keys_ignore_copies pins +// the other half of that rule. A copy takes its preconditions from the +// x-amz-copy-source-if-* headers, so the gateway ignores a plain If-Match or +// If-None-Match on one — and a policy demanding a conditional write must +// therefore keep denying copies rather than be satisfied by a header that +// changes nothing. +func S3IAMAccessControl_condition_conditional_write_keys_ignore_copies(s *S3Conf) error { + testName := "S3IAMAccessControl_condition_conditional_write_keys_ignore_copies" + return s3IAMActionHandler(s, testName, func(root *iam.Client, bucket string) error { + if _, err := putObjectAndGetETag(s.GetClient(), bucket, "src"); err != nil { + return err + } + + user, cleanup, err := newS3IAMUser(root, s, map[string]string{ + "p": policyDoc( + accessStatement{Effect: "Allow", Action: actS3GetObject, Resource: objectsArn(bucket)}, + accessStatement{Effect: "Allow", Action: actS3PutObject, Resource: objectsArn(bucket)}, + accessStatement{ + Effect: "Deny", Action: actS3PutObject, Resource: objectsArn(bucket), + Condition: cond("Null", "s3:if-none-match", "true"), + }, + ), + }) + if err != nil { + return err + } + defer cleanup() + + for _, tc := range []struct { + name string + ifNoneMatch *string + }{ + {"a copy without the header", nil}, + {"a copy carrying the header", getPtr("*")}, + } { + ctx, cancel := context.WithTimeout(context.Background(), shortTimeout) + _, err := user.client.CopyObject(ctx, &s3.CopyObjectInput{ + Bucket: &bucket, + Key: getPtr("dst"), + CopySource: getPtr(bucket + "/src"), + IfNoneMatch: tc.ifNoneMatch, + }) + cancel() + if err := checkApiErr(err, wantExplicitIdentityDeny(user.arn, actS3PutObject, objectArn(bucket, "dst"))); err != nil { + return fmt.Errorf("%s: %w", tc.name, err) + } + } + return nil + }) +} + +// S3IAMAccessControl_condition_if_match_ignores_delete_objects covers the +// batch delete. DeleteObjects carries one request-level header for the +// whole batch and the gateway never applies it to any key, so letting it +// populate s3:if-match would authorize deleting every object in the batch +// against an ETag nothing checks. +func S3IAMAccessControl_condition_if_match_ignores_delete_objects(s *S3Conf) error { + testName := "S3IAMAccessControl_condition_if_match_ignores_delete_objects" + return s3IAMActionHandler(s, testName, func(root *iam.Client, bucket string) error { + etag, err := putObjectAndGetETag(s.GetClient(), bucket, "obj") + if err != nil { + return err + } + + user, cleanup, err := newS3IAMUser(root, s, map[string]string{ + "p": policyDoc(accessStatement{ + Effect: "Allow", Action: actS3DeleteObject, Resource: objectsArn(bucket), + Condition: cond("StringEquals", "s3:if-match", strings.Trim(etag, `"`)), + }), + }) + if err != nil { + return err + } + defer cleanup() + + ctx, cancel := context.WithTimeout(context.Background(), shortTimeout) + res, err := user.client.DeleteObjects(ctx, &s3.DeleteObjectsInput{ + Bucket: &bucket, + Delete: &types.Delete{Objects: objectIdentifiers("obj")}, + }) + cancel() + if err != nil { + return err + } + if len(res.Errors) != 1 || getString(res.Errors[0].Code) != "AccessDenied" { + return fmt.Errorf("expected the batch delete to be denied, instead got %+v", res) + } + return nil + }) +} + +// S3IAMAccessControl_condition_conditional_write_keys_ignore_reads pins the +// keys to writes. GET and HEAD take If-Match/If-None-Match too, as ordinary +// HTTP cache preconditions, and neither may populate the condition context +// there — otherwise a browser revalidating its cache would decide whether a +// read is authorized. +func S3IAMAccessControl_condition_conditional_write_keys_ignore_reads(s *S3Conf) error { + testName := "S3IAMAccessControl_condition_conditional_write_keys_ignore_reads" + return s3IAMActionHandler(s, testName, func(root *iam.Client, bucket string) error { + etag, err := putObjectAndGetETag(s.GetClient(), bucket, "obj") + if err != nil { + return err + } + + user, cleanup, err := newS3IAMUser(root, s, nil) + if err != nil { + return err + } + defer cleanup() + + for _, tc := range []struct { + name string + condition json.RawMessage + wantAllow bool + }{ + {"s3:if-match stays absent on a conditional read", cond("Null", "s3:if-match", "true"), true}, + {"s3:if-none-match stays absent on a conditional read", cond("Null", "s3:if-none-match", "true"), true}, + {"a read can never satisfy a present-key condition", cond("Null", "s3:if-match", "false"), false}, + {"nor an equality against the ETag it sent", cond("StringEquals", "s3:if-match", strings.Trim(etag, `"`)), false}, + } { + if err := func() error { + if err := putS3IAMUserPolicy(root, user, "p", policyDoc(accessStatement{ + Effect: "Allow", Action: actS3GetObject, Resource: objectsArn(bucket), + Condition: tc.condition, + })); err != nil { + return err + } + + ctx, cancel := context.WithTimeout(context.Background(), shortTimeout) + _, err := user.client.GetObject(ctx, &s3.GetObjectInput{ + Bucket: &bucket, + Key: getPtr("obj"), + IfMatch: &etag, + }) + cancel() + + if !tc.wantAllow { + return checkApiErr(err, wantImplicitDeny(user.arn, actS3GetObject, objectArn(bucket, "obj"))) + } + if err != nil { + return fmt.Errorf("expected the request to be allowed: %w", err) + } + return nil + }(); err != nil { + return fmt.Errorf("%s: %w", tc.name, err) + } + } + return nil + }) +} + // containsBucket reports whether buckets names bucket, so a listing can be // asserted without depending on what else other tests left behind. func containsBucket(buckets []types.Bucket, bucket string) bool { diff --git a/tests/integration/s3_iam_utils.go b/tests/integration/s3_iam_utils.go index ee8b6979..645caefb 100644 --- a/tests/integration/s3_iam_utils.go +++ b/tests/integration/s3_iam_utils.go @@ -38,14 +38,15 @@ import ( ) const ( - actS3GetObject = "s3:GetObject" - actS3PutObject = "s3:PutObject" - actS3DeleteObject = "s3:DeleteObject" - actS3DeleteObjectVersion = "s3:DeleteObjectVersion" - actS3ListBucket = "s3:ListBucket" - actS3CreateBucket = "s3:CreateBucket" - actS3ListAllMyBuckets = "s3:ListAllMyBuckets" - actS3BypassGovernance = "s3:BypassGovernanceRetention" + actS3GetObject = "s3:GetObject" + actS3PutObject = "s3:PutObject" + actS3DeleteObject = "s3:DeleteObject" + actS3DeleteObjectVersion = "s3:DeleteObjectVersion" + actS3ListBucket = "s3:ListBucket" + actS3CreateBucket = "s3:CreateBucket" + actS3ListAllMyBuckets = "s3:ListAllMyBuckets" + actS3BypassGovernance = "s3:BypassGovernanceRetention" + actS3PutBucketOwnershipControls = "s3:PutBucketOwnershipControls" ) const ( diff --git a/tests/integration/utils.go b/tests/integration/utils.go index 695a7f2e..6100f766 100644 --- a/tests/integration/utils.go +++ b/tests/integration/utils.go @@ -1017,6 +1017,61 @@ func putObjects(client *s3.Client, objs []string, bucket string) ([]types.Object return contents, nil } +// putObjectAndGetETag uploads an object and returns its ETag as the wire +// carries it, quotes included, for the conditional-write tests that have to +// name a real ETag in an If-Match header or a policy condition. +func putObjectAndGetETag(client *s3.Client, bucket, key string) (string, error) { + ctx, cancel := context.WithTimeout(context.Background(), shortTimeout) + defer cancel() + + res, err := client.PutObject(ctx, &s3.PutObjectInput{ + Bucket: &bucket, + Key: &key, + Body: bytes.NewReader([]byte("data")), + }) + if err != nil { + return "", err + } + return getString(res.ETag), nil +} + +// withRequestHeader sets a raw header on an SDK request, for the cases no +// input field reaches — an If-Match on a bucket-level write, say. The +// middleware runs right before signing so the header is signed like any +// other. +func withRequestHeader(key, value string) func(*s3.Options) { + return func(o *s3.Options) { + o.APIOptions = append(o.APIOptions, func(stack *middleware.Stack) error { + return stack.Finalize.Insert( + middleware.FinalizeMiddlewareFunc("SetRequestHeader", + func(ctx context.Context, in middleware.FinalizeInput, next middleware.FinalizeHandler) ( + out middleware.FinalizeOutput, md middleware.Metadata, err error, + ) { + if req, ok := in.Request.(*smithyhttp.Request); ok { + req.Header.Set(key, value) + } + return next.HandleFinalize(ctx, in) + }), + "Signing", + middleware.Before, + ) + }) + } +} + +// headObjectETag re-reads an object's current ETag, which an overwrite may +// have changed. +func headObjectETag(client *s3.Client, bucket, key string) (string, error) { + ctx, cancel := context.WithTimeout(context.Background(), shortTimeout) + defer cancel() + + res, err := client.HeadObject(ctx, &s3.HeadObjectInput{Bucket: &bucket, Key: &key}) + if err != nil { + return "", err + } + return getString(res.ETag), nil +} + func listObjects(client *s3.Client, bucket, prefix, delimiter string, maxKeys int32) ([]types.Object, []types.CommonPrefix, error) { var contents []types.Object var commonPrefixes []types.CommonPrefix