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,