From 744a9dcd71c2ba740937ea7c3527f6a04f1b8fe0 Mon Sep 17 00:00:00 2001 From: Feng Ruohang Date: Mon, 3 Aug 2026 23:30:55 +0800 Subject: [PATCH] fix: bind s3:versionid conditions to the effective object version A bucket policy that allows s3:DeleteObject only when s3:versionid is null -- Condition {"Null": {"s3:versionid": "true"}}, the idiom for "let clients delete current objects but not roll back versions" -- denied every delete, including the version-less ones it was meant to permit (upstream issue minio/minio#21735). getConditionValues wrote "versionid": {""} unconditionally. The condition engine decides Null by slice length (nullfunc.evaluate), so a present-but- empty value reads as "key present": Null:true never matched and Null:false always did. Absent and empty were indistinguishable. Writing the key only when the request names a version fixes the reported case but, alone, opens a worse one. DeleteObjects carries each object's version in the XML body, which getConditionValues -- reading only r.Form -- never sees. A body version would then vanish from the map, read as null, and a policy meant to protect old versions would authorize deleting a specific one. So authorization also rebinds versionid to the effective, server-resolved reqInfo.VersionID for DeleteObjectAction: the per-entry body value that checkRequestAuthTypeWithVID already sets in the DeleteObjects loop, deleting the key when that value is empty. A query-level ?versionId on a DeleteObjects POST no longer leaks into any entry's decision. Finally, trim the version the condition builder reads. newContext and getOpts both TrimSpace it before the object layer acts, so an untrimmed value here let a padded ?versionId=V%20 present a different s3:versionid than the version actually operated on, sidestepping a Deny keyed on StringEquals s3:versionid. DeleteObjectAction was already immune via the trimmed reqInfo value; this covers GetObject, tagging, retention, and the copy-source read. Tests: an end-to-end DeleteObjects against a Null:{s3:versionid:true} policy over versioned objects (with a decoy query versionId proving the per-entry body value wins), and a unit test asserting key presence, trimming, and the copy-source fallback. Co-authored-by: ChatGPT Co-authored-by: Claude --- cmd/auth-handler.go | 23 +++-- cmd/bucket-handlers_test.go | 127 +++++++++++++++++++++++++++ cmd/bucket-policy.go | 14 ++- cmd/bucket-policy_test.go | 96 ++++++++++++++++++++ docs/iam/access-management-plugin.md | 3 - 5 files changed, 252 insertions(+), 11 deletions(-) diff --git a/cmd/auth-handler.go b/cmd/auth-handler.go index e1e54d650..d5df28624 100644 --- a/cmd/auth-handler.go +++ b/cmd/auth-handler.go @@ -455,6 +455,19 @@ func authorizeRequestWithTags(ctx context.Context, r *http.Request, action polic bucket := reqInfo.BucketName object := reqInfo.ObjectName versionID := reqInfo.VersionID + conditionValuesForAuth := func(locationConstraint string, credentials auth.Credentials) map[string][]string { + values := getConditionValuesWithTags(r, locationConstraint, credentials, existingTags, requestTags) + if action == policy.DeleteObjectAction { + // DeleteObjects carries the effective version ID in each XML object, + // not in the request query. Keep authorization scoped to that entry. + if versionID == "" { + delete(values, "versionid") + } else { + values["versionid"] = []string{versionID} + } + } + return values + } if action != policy.ListAllMyBucketsAction && cred.AccessKey == "" { // Anonymous checks are not meant for ListAllBuckets action @@ -463,7 +476,7 @@ func authorizeRequestWithTags(ctx context.Context, r *http.Request, action polic Groups: cred.Groups, Action: action, BucketName: bucket, - ConditionValues: getConditionValuesWithTags(r, region, auth.AnonymousCredentials, existingTags, requestTags), + ConditionValues: conditionValuesForAuth(region, auth.AnonymousCredentials), IsOwner: false, ObjectName: object, }) { @@ -479,7 +492,7 @@ func authorizeRequestWithTags(ctx context.Context, r *http.Request, action polic Groups: cred.Groups, Action: policy.ListBucketAction, BucketName: bucket, - ConditionValues: getConditionValuesWithTags(r, region, auth.AnonymousCredentials, existingTags, requestTags), + ConditionValues: conditionValuesForAuth(region, auth.AnonymousCredentials), IsOwner: false, ObjectName: object, }) { @@ -496,7 +509,7 @@ func authorizeRequestWithTags(ctx context.Context, r *http.Request, action polic Groups: cred.Groups, Action: policy.Action(policy.DeleteObjectVersionAction), BucketName: bucket, - ConditionValues: getConditionValuesWithTags(r, "", cred, existingTags, requestTags), + ConditionValues: conditionValuesForAuth("", cred), ObjectName: object, IsOwner: owner, Claims: cred.Claims, @@ -510,7 +523,7 @@ func authorizeRequestWithTags(ctx context.Context, r *http.Request, action polic Groups: cred.Groups, Action: action, BucketName: bucket, - ConditionValues: getConditionValuesWithTags(r, "", cred, existingTags, requestTags), + ConditionValues: conditionValuesForAuth("", cred), ObjectName: object, IsOwner: owner, Claims: cred.Claims, @@ -527,7 +540,7 @@ func authorizeRequestWithTags(ctx context.Context, r *http.Request, action polic Groups: cred.Groups, Action: policy.ListBucketAction, BucketName: bucket, - ConditionValues: getConditionValuesWithTags(r, "", cred, existingTags, requestTags), + ConditionValues: conditionValuesForAuth("", cred), ObjectName: object, IsOwner: owner, Claims: cred.Claims, diff --git a/cmd/bucket-handlers_test.go b/cmd/bucket-handlers_test.go index 0adb9b8f1..c8972508f 100644 --- a/cmd/bucket-handlers_test.go +++ b/cmd/bucket-handlers_test.go @@ -944,3 +944,130 @@ func testAPIDeleteMultipleObjectsHandler(obj ObjectLayer, instanceType, bucketNa // `ExecObjectLayerAPINilTest` manages the operation. ExecObjectLayerAPINilTest(t, nilBucket, nilObject, instanceType, apiRouter, nilReq) } + +func TestAPIDeleteMultipleObjectsVersionIDNullCondition(t *testing.T) { + ExecObjectLayerAPITest(ExecObjectLayerAPITestArgs{ + t: t, + objAPITest: testAPIDeleteMultipleObjectsVersionIDNullCondition, + endpoints: []string{"DeleteMultipleObjects", "PutBucketPolicy"}, + makeBucketOptions: MakeBucketOptions{VersioningEnabled: true}, + }) +} + +func testAPIDeleteMultipleObjectsVersionIDNullCondition(obj ObjectLayer, instanceType, bucketName string, apiRouter http.Handler, + credentials auth.Credentials, t *testing.T, +) { + versionIDs := make(map[string]string, 4) + for _, objectName := range []string{ + "without-version-id-before", + "with-version-id", + "without-version-id-after", + "with-null-version-id", + } { + data := []byte(objectName) + info, err := obj.PutObject(t.Context(), bucketName, objectName, + mustGetPutObjReader(t, bytes.NewReader(data), int64(len(data)), "", ""), ObjectOptions{Versioned: true}) + if err != nil { + t.Fatalf("%s: put %q: %v", instanceType, objectName, err) + } + if info.VersionID == "" { + t.Fatalf("%s: put %q did not create a version ID", instanceType, objectName) + } + versionIDs[objectName] = info.VersionID + } + + policyBytes := fmt.Appendf(nil, `{ + "Version":"2012-10-17", + "Statement":[{ + "Effect":"Allow", + "Principal":"*", + "Action":"s3:DeleteObject", + "Resource":"arn:aws:s3:::%s/*", + "Condition":{"Null":{"s3:versionid":"true"}} + }] + }`, bucketName) + policyReq, err := newTestSignedRequestV4(http.MethodPut, getPutPolicyURL("", bucketName), int64(len(policyBytes)), + bytes.NewReader(policyBytes), credentials.AccessKey, credentials.SecretKey, nil) + if err != nil { + t.Fatal(err) + } + policyRec := httptest.NewRecorder() + apiRouter.ServeHTTP(policyRec, policyReq) + if policyRec.Code != http.StatusNoContent { + t.Fatalf("%s: put policy returned %d: %s", instanceType, policyRec.Code, policyRec.Body.String()) + } + + deleteBody := encodeResponse(DeleteObjectsRequest{Objects: []ObjectToDelete{ + {ObjectV: ObjectV{ObjectName: "without-version-id-before"}}, + {ObjectV: ObjectV{ObjectName: "with-version-id", VersionID: versionIDs["with-version-id"]}}, + {ObjectV: ObjectV{ObjectName: "without-version-id-after"}}, + {ObjectV: ObjectV{ObjectName: "with-null-version-id", VersionID: nullVersionID}}, + }}) + // A query-level versionId is not the version of every XML entry. Each + // object's optional VersionId remains the effective authorization value. + deleteURL := getDeleteMultipleObjectsURL("", bucketName) + "&versionId=query-level-decoy" + deleteReq, err := newTestRequest(http.MethodPost, deleteURL, + int64(len(deleteBody)), bytes.NewReader(deleteBody)) + if err != nil { + t.Fatal(err) + } + deleteRec := httptest.NewRecorder() + apiRouter.ServeHTTP(deleteRec, deleteReq) + if deleteRec.Code != http.StatusOK { + t.Fatalf("%s: delete returned %d: %s", instanceType, deleteRec.Code, deleteRec.Body.String()) + } + + var response DeleteObjectsResponse + if err = xml.Unmarshal(deleteRec.Body.Bytes(), &response); err != nil { + t.Fatalf("%s: decode response: %v: %s", instanceType, err, deleteRec.Body.String()) + } + deleted := make(map[string]DeletedObject, len(response.DeletedObjects)) + for _, object := range response.DeletedObjects { + deleted[object.ObjectName] = object + } + for _, objectName := range []string{"without-version-id-before", "without-version-id-after"} { + object, ok := deleted[objectName] + if !ok || !object.DeleteMarker || object.DeleteMarkerVersionID == "" { + t.Errorf("%s: %q was not a successful delete-marker creation: %+v", instanceType, objectName, response.DeletedObjects) + } + } + if len(deleted) != 2 { + t.Errorf("%s: unexpected deleted objects: %+v", instanceType, response.DeletedObjects) + } + errorsByKey := make(map[string]DeleteError, len(response.Errors)) + for _, deleteErr := range response.Errors { + errorsByKey[deleteErr.Key] = deleteErr + } + for objectName, versionID := range map[string]string{ + "with-version-id": versionIDs["with-version-id"], + "with-null-version-id": nullVersionID, + } { + deleteErr, ok := errorsByKey[objectName] + if !ok || deleteErr.VersionID != versionID || deleteErr.Code != errorCodes[ErrAccessDenied].Code { + t.Errorf("%s: %q did not return AccessDenied for version %q: %+v", instanceType, objectName, versionID, response.Errors) + } + } + if len(errorsByKey) != 2 { + t.Errorf("%s: unexpected delete errors: %+v", instanceType, response.Errors) + } + + // A simple delete adds a marker and keeps the old version. The explicitly + // named version must also remain because its policy condition did not match. + for objectName, versionID := range versionIDs { + if _, err = obj.GetObjectInfo(t.Context(), bucketName, objectName, ObjectOptions{VersionID: versionID}); err != nil { + t.Errorf("%s: version %s of %q was not preserved: %v", instanceType, versionID, objectName, err) + } + } + for _, objectName := range []string{"without-version-id-before", "without-version-id-after"} { + if _, err = obj.GetObjectInfo(t.Context(), bucketName, objectName, ObjectOptions{}); !isErrObjectNotFound(err) { + t.Errorf("%s: simple delete of %q did not hide the latest object behind a delete marker: %v", instanceType, objectName, err) + } + } + for _, objectName := range []string{"with-version-id", "with-null-version-id"} { + if info, err := obj.GetObjectInfo(t.Context(), bucketName, objectName, ObjectOptions{}); err != nil { + t.Errorf("%s: denied version delete removed latest %q: %v", instanceType, objectName, err) + } else if info.VersionID != versionIDs[objectName] { + t.Errorf("%s: latest version of %q changed from %s to %s", instanceType, objectName, versionIDs[objectName], info.VersionID) + } + } +} diff --git a/cmd/bucket-policy.go b/cmd/bucket-policy.go index d096705b2..84c15e54a 100644 --- a/cmd/bucket-policy.go +++ b/cmd/bucket-policy.go @@ -198,10 +198,13 @@ func getConditionValuesWithTags(r *http.Request, lc string, cred auth.Credential } } - vid := r.Form.Get(xhttp.VersionID) + // Match the version the object layer will act on: newContext and getOpts both + // TrimSpace this value, so leaving it untrimmed here would let a padded + // ?versionId=V%20 present a different s3:versionid than the effective version. + vid := strings.TrimSpace(r.Form.Get(xhttp.VersionID)) if vid == "" { if u, err := url.Parse(r.Header.Get(xhttp.AmzCopySource)); err == nil { - vid = u.Query().Get(xhttp.VersionID) + vid = strings.TrimSpace(u.Query().Get(xhttp.VersionID)) } } @@ -234,11 +237,16 @@ func getConditionValuesWithTags(r *http.Request, lc string, cred auth.Credential "principaltype": {principalType}, "userid": {username}, "username": {username}, - "versionid": {vid}, "signatureversion": {signatureVersion}, "authType": {authtype}, } + // Null conditions distinguish an absent key from a present key with an + // empty value. Only expose s3:versionid when the request names a version. + if vid != "" { + args["versionid"] = []string{vid} + } + if lc != "" { args["LocationConstraint"] = []string{lc} } diff --git a/cmd/bucket-policy_test.go b/cmd/bucket-policy_test.go index 4e24d3e6c..b7801c102 100644 --- a/cmd/bucket-policy_test.go +++ b/cmd/bucket-policy_test.go @@ -26,6 +26,7 @@ import ( "testing" "github.com/minio/minio/internal/auth" + "github.com/minio/minio/internal/handlers" xhttp "github.com/minio/minio/internal/http" "github.com/minio/pkg/v3/policy" "github.com/minio/pkg/v3/policy/condition" @@ -161,6 +162,65 @@ func TestGetConditionValuesUsesActualRequestSource(t *testing.T) { } } +func TestGetConditionValuesVersionIDPresence(t *testing.T) { + nullVersionID, err := condition.NewNullFunc(condition.S3VersionID.ToKey(), true) + if err != nil { + t.Fatal(err) + } + + withoutVersionID := condValuesForRequest(t, "http://minio.local/bkt/obj", nil) + if _, ok := withoutVersionID["versionid"]; ok { + t.Fatalf("an absent versionId was exposed to policy evaluation as %v", withoutVersionID["versionid"]) + } + if !condition.NewFunctions(nullVersionID).Evaluate(withoutVersionID) { + t.Fatal("Null s3:versionid=true did not match a request without versionId") + } + + const versionID = "7f4b6b5f-bf25-4e98-95df-90cba8070dd8" + withVersionID := condValuesForRequest(t, + "http://minio.local/bkt/obj?"+url.Values{xhttp.VersionID: {versionID}}.Encode(), nil) + if got := withVersionID["versionid"]; !slices.Equal(got, []string{versionID}) { + t.Fatalf("expected versionId %q, got %v", versionID, got) + } + if condition.NewFunctions(nullVersionID).Evaluate(withVersionID) { + t.Fatal("Null s3:versionid=true matched a request with versionId") + } + + copySourceVersion := condValuesForRequest(t, "http://minio.local/bkt/copied", map[string]string{ + xhttp.AmzCopySource: "/source-bucket/source-object?" + url.Values{xhttp.VersionID: {versionID}}.Encode(), + }) + if got := copySourceVersion["versionid"]; !slices.Equal(got, []string{versionID}) { + t.Fatalf("copy source versionId was lost: got %v", got) + } + + // The object layer trims the version before acting on it; the condition value + // must be the same effective string, or a padded ?versionId=V%20 would let a + // StringEquals/Deny on s3:versionid see a different value than the one deleted. + paddedVersion := condValuesForRequest(t, + "http://minio.local/bkt/obj?"+url.Values{xhttp.VersionID: {versionID + " "}}.Encode(), nil) + if got := paddedVersion["versionid"]; !slices.Equal(got, []string{versionID}) { + t.Fatalf("a padded versionId was not trimmed to the effective value: got %v", got) + } + + paddedCopySource := condValuesForRequest(t, "http://minio.local/bkt/copied", map[string]string{ + xhttp.AmzCopySource: "/source-bucket/source-object?" + url.Values{xhttp.VersionID: {versionID + " "}}.Encode(), + }) + if got := paddedCopySource["versionid"]; !slices.Equal(got, []string{versionID}) { + t.Fatalf("a padded copy source versionId was not trimmed: got %v", got) + } + + // A whitespace-only versionId names no version once trimmed, exactly as the + // object layer treats it, so the key must be absent and Null:true must match. + blankVersion := condValuesForRequest(t, + "http://minio.local/bkt/obj?"+url.Values{xhttp.VersionID: {" "}}.Encode(), nil) + if _, ok := blankVersion["versionid"]; ok { + t.Fatalf("a whitespace-only versionId was exposed to policy evaluation as %v", blankVersion["versionid"]) + } + if !condition.NewFunctions(nullVersionID).Evaluate(blankVersion) { + t.Fatal("Null s3:versionid=true did not match a request whose versionId was only whitespace") + } +} + func TestGetConditionValuesUsesEffectiveRequestTags(t *testing.T) { rawURL := "http://minio.local/bkt/obj?" + url.Values{ strings.ToLower(xhttp.AmzObjectTagging): {"security=public&virus=true"}, @@ -257,6 +317,42 @@ func TestBucketPolicySourceIPCannotBeForged(t *testing.T) { } } +// aws:SourceIp must be whatever the hardened resolver decided and nothing else. +// The resolver is where the forwarded-header trust policy is enforced and where +// its three modes are tested (internal/handlers/proxy_test.go); this pins the +// join, so the condition value cannot drift onto some other derivation that the +// policy would not cover. +// +// It also records the default-mode contract: with no trust policy configured, +// each of the three forwarded headers still sets aws:SourceIp, and so an +// IpAddress condition is only as good as the network path to the API port. +// Enforcing such a condition against a client with direct access requires +// MINIO_API_TRUSTED_PROXIES or _MINIO_API_XFF_HEADER=off. +func TestGetConditionValuesSourceIPMatchesResolver(t *testing.T) { + for _, header := range []map[string]string{ + nil, + {"X-Forwarded-For": "10.1.2.3"}, + {"X-Real-IP": "10.1.2.3"}, + {"Forwarded": "for=10.1.2.3"}, + {"X-Forwarded-For": "10.1.2.3, 198.51.100.9"}, + } { + r, err := http.NewRequest(http.MethodGet, "http://minio.local/bkt/obj", nil) + if err != nil { + t.Fatal(err) + } + r.RemoteAddr = testCondRemoteILP + for k, v := range header { + r.Header.Set(k, v) + } + + got := resolvedConditionValues(condValuesForRequest(t, "http://minio.local/bkt/obj", header), condition.AWSSourceIP.ToKey().Name()) + want := handlers.GetSourceIPRaw(r) + if len(got) != 1 || got[0] != want { + t.Errorf("headers %v: aws:SourceIp = %v, resolver returned %q", header, got, want) + } + } +} + // "Deny unless the connection is TLS" is the usual hardening statement, and // aws:SecureTransport is computed from r.TLS. func TestBucketPolicySecureTransportCannotBeForged(t *testing.T) { diff --git a/docs/iam/access-management-plugin.md b/docs/iam/access-management-plugin.md index f18b1e11c..873eabdde 100644 --- a/docs/iam/access-management-plugin.md +++ b/docs/iam/access-management-plugin.md @@ -125,9 +125,6 @@ The JSON body structure can be seen from this sample: ], "username": [ "minio" - ], - "versionid": [ - "" ] }, "owner": true,