fix: authorize explicit version deletes with DeleteObjectVersion

buildscripts/rebrand-guard/compat-baseline.json

@@ -763,6 +763,7 @@
     "/debug/go",
     "/del-config-kv",
     "/delete-service-account",
+    "/deny/*",
     "/describe-job",
     "/dev/0",
     "/dev/1",

cmd/auth-handler.go

@@ -363,17 +363,6 @@ func checkRequestAuthTypeWithRequestTags(ctx context.Context, r *http.Request, a
 	return authorizeRequestWithTags(ctx, r, action, "", requestTags)
 }
 
-// checkRequestAuthTypeWithVID is similar to checkRequestAuthType
-// passes versionID additionally.
-func checkRequestAuthTypeWithVID(ctx context.Context, r *http.Request, action policy.Action, bucketName, objectName, versionID string) (s3Err APIErrorCode) {
-	logger.GetReqInfo(ctx).BucketName = bucketName
-	logger.GetReqInfo(ctx).ObjectName = objectName
-	logger.GetReqInfo(ctx).VersionID = versionID
-
-	_, _, s3Err = checkRequestAuthTypeCredential(ctx, r, action)
-	return s3Err
-}
-
 func authenticateRequest(ctx context.Context, r *http.Request, action policy.Action) (s3Err APIErrorCode) {
 	if logger.GetReqInfo(ctx) == nil {
 		bugLogIf(ctx, errors.New("unexpected context.Context does not have a logger.ReqInfo"), logger.ErrorKind)
@@ -439,6 +428,23 @@ func authorizeRequest(ctx context.Context, r *http.Request, action policy.Action
 	return authorizeRequestWithExistingTags(ctx, r, action, "")
 }
 
+func deleteObjectAction(versionID string) policy.Action {
+	if versionID != "" {
+		return policy.DeleteObjectVersionAction
+	}
+	return policy.DeleteObjectAction
+}
+
+func actionUsesObjectVersion(action policy.Action) bool {
+	switch action {
+	case policy.DeleteObjectAction, policy.DeleteObjectVersionAction,
+		policy.ReplicateDeleteAction, policy.BypassGovernanceRetentionAction:
+		return true
+	default:
+		return false
+	}
+}
+
 func authorizeRequestWithExistingTags(ctx context.Context, r *http.Request, action policy.Action, existingTags string) (s3Err APIErrorCode) {
 	return authorizeRequestWithTags(ctx, r, action, existingTags, nil)
 }
@@ -457,7 +463,7 @@ func authorizeRequestWithTags(ctx context.Context, r *http.Request, action polic
 	versionID := reqInfo.VersionID
 	conditionValuesForAuth := func(locationConstraint string, credentials auth.Credentials) map[string][]string {
 		values := getConditionValuesWithTags(r, locationConstraint, credentials, existingTags, requestTags)
-		if action == policy.DeleteObjectAction {
+		if actionUsesObjectVersion(action) {
 			// DeleteObjects carries the effective version ID in each XML object,
 			// not in the request query. Keep authorization scoped to that entry.
 			if versionID == "" {
@@ -503,21 +509,6 @@ func authorizeRequestWithTags(ctx context.Context, r *http.Request, action polic
 
 		return ErrAccessDenied
 	}
-	if action == policy.DeleteObjectAction && versionID != "" {
-		if !globalIAMSys.IsAllowed(policy.Args{
-			AccountName:     cred.AccessKey,
-			Groups:          cred.Groups,
-			Action:          policy.Action(policy.DeleteObjectVersionAction),
-			BucketName:      bucket,
-			ConditionValues: conditionValuesForAuth("", cred),
-			ObjectName:      object,
-			IsOwner:         owner,
-			Claims:          cred.Claims,
-			DenyOnly:        true,
-		}) { // Request is not allowed if Deny action on DeleteObjectVersionAction
-			return ErrAccessDenied
-		}
-	}
 	if globalIAMSys.IsAllowed(policy.Args{
 		AccountName:     cred.AccessKey,
 		Groups:          cred.Groups,
@@ -553,6 +544,40 @@ func authorizeRequestWithTags(ctx context.Context, r *http.Request, action polic
 	return ErrAccessDenied
 }
 
+// authorizeReplicationDelete preserves the established target-credential
+// contract for trusted replication: DeleteObject and ReplicateDelete must be
+// allowed, while an explicit DeleteObjectVersion deny still blocks a named
+// version. Ordinary S3 requests never use this compatibility path.
+func authorizeReplicationDelete(ctx context.Context, r *http.Request) APIErrorCode {
+	if s3Err := authorizeRequest(ctx, r, policy.DeleteObjectAction); s3Err != ErrNone {
+		return s3Err
+	}
+	reqInfo := logger.GetReqInfo(ctx)
+	if reqInfo == nil {
+		return ErrAccessDenied
+	}
+	if reqInfo.VersionID == "" {
+		return ErrNone
+	}
+	cred := reqInfo.Cred
+	values := getConditionValuesWithTags(r, "", cred, "", nil)
+	values["versionid"] = []string{reqInfo.VersionID}
+	if !globalIAMSys.IsAllowed(policy.Args{
+		AccountName:     cred.AccessKey,
+		Groups:          cred.Groups,
+		Action:          policy.DeleteObjectVersionAction,
+		BucketName:      reqInfo.BucketName,
+		ConditionValues: values,
+		ObjectName:      reqInfo.ObjectName,
+		IsOwner:         reqInfo.Owner,
+		Claims:          cred.Claims,
+		DenyOnly:        true,
+	}) {
+		return ErrAccessDenied
+	}
+	return ErrNone
+}
+
 // Check request auth type verifies the incoming http request
 //   - validates the request signature
 //   - validates the policy action if anonymous tests bucket policies if any,

cmd/bucket-handlers.go

@@ -463,13 +463,20 @@ func (api objectAPIHandlers) DeleteMultipleObjectsHandler(w http.ResponseWriter,
 	// Make sure to update context to print ObjectNames for multi objects.
 	ctx = updateReqContext(ctx, objects...)
 
-	// Call checkRequestAuthType to populate ReqInfo.AccessKey before GetBucketInfo()
-	// Ignore errors here to preserve the S3 error behavior of GetBucketInfo()
-	checkRequestAuthType(ctx, r, policy.DeleteObjectAction, bucket, "")
-
 	deleteObjectsFn := objectAPI.DeleteObjects
 
-	// Return Malformed XML as S3 spec if the number of objects is empty
+	reqInfo := logger.GetReqInfo(ctx)
+	if reqInfo == nil {
+		writeErrorResponse(ctx, w, errorCodes.ToAPIErr(ErrAccessDenied), r.URL)
+		return
+	}
+	reqInfo.BucketName = bucket
+	reqInfo.ObjectName = ""
+	if s3Err := authenticateRequest(ctx, r, policy.DeleteObjectAction); s3Err != ErrNone {
+		writeErrorResponse(ctx, w, errorCodes.ToAPIErr(s3Err), r.URL)
+		return
+	}
+	// Return Malformed XML as S3 spec if the number of objects is empty.
 	if len(deleteObjectsReq.Objects) == 0 || len(deleteObjectsReq.Objects) > maxDeleteList {
 		writeErrorResponse(ctx, w, errorCodes.ToAPIErr(ErrMalformedXML), r.URL)
 		return
@@ -499,7 +506,9 @@ func (api objectAPIHandlers) DeleteMultipleObjectsHandler(w http.ResponseWriter,
 	vc, _ := globalBucketVersioningSys.Get(bucket)
 	oss := make([]*objSweeper, len(deleteObjectsReq.Objects))
 	for index, object := range deleteObjectsReq.Objects {
-		if apiErrCode := checkRequestAuthTypeWithVID(ctx, r, policy.DeleteObjectAction, bucket, object.ObjectName, object.VersionID); apiErrCode != ErrNone {
+		reqInfo.ObjectName = object.ObjectName
+		reqInfo.VersionID = object.VersionID
+		if apiErrCode := authorizeRequest(ctx, r, deleteObjectAction(object.VersionID)); apiErrCode != ErrNone {
 			if apiErrCode == ErrSignatureDoesNotMatch || apiErrCode == ErrInvalidAccessKeyID {
 				writeErrorResponse(ctx, w, errorCodes.ToAPIErr(apiErrCode), r.URL)
 				return

cmd/bucket-handlers_test.go

@@ -1018,14 +1018,23 @@ func testAPIDeleteMultipleObjectsVersionIDNullCondition(obj ObjectLayer, instanc
 
 	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)
+		"Statement":[
+			{
+				"Effect":"Allow",
+				"Principal":"*",
+				"Action":"s3:DeleteObject",
+				"Resource":"arn:aws:s3:::%s/*",
+				"Condition":{"Null":{"s3:versionid":"true"}}
+			},
+			{
+				"Effect":"Allow",
+				"Principal":"*",
+				"Action":"s3:DeleteObjectVersion",
+				"Resource":"arn:aws:s3:::%s/*",
+				"Condition":{"StringEquals":{"s3:versionid":"%s"}}
+			}
+		]
+	}`, bucketName, bucketName, versionIDs["with-version-id"])
 	policyReq, err := newTestSignedRequestV4(http.MethodPut, getPutPolicyURL("", bucketName), int64(len(policyBytes)),
 		bytes.NewReader(policyBytes), credentials.AccessKey, credentials.SecretKey, nil)
 	if err != nil {
@@ -1071,29 +1080,30 @@ func testAPIDeleteMultipleObjectsVersionIDNullCondition(obj ObjectLayer, instanc
 			t.Errorf("%s: %q was not a successful delete-marker creation: %+v", instanceType, objectName, response.DeletedObjects)
 		}
 	}
-	if len(deleted) != 2 {
+	if object, ok := deleted["with-version-id"]; !ok || object.VersionID != versionIDs["with-version-id"] {
+		t.Errorf("%s: matching explicit version was not deleted: %+v", instanceType, response.DeletedObjects)
+	}
+	if len(deleted) != 3 {
 		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,
-	} {
+	for objectName, versionID := range map[string]string{"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 {
+	if len(errorsByKey) != 1 {
 		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 {
+	// A simple delete adds a marker and keeps the old version. The null-version
+	// delete remains denied because its per-entry condition does not match.
+	for _, objectName := range []string{"without-version-id-before", "without-version-id-after", "with-null-version-id"} {
+		versionID := versionIDs[objectName]
 		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)
 		}
@@ -1103,7 +1113,10 @@ func testAPIDeleteMultipleObjectsVersionIDNullCondition(obj ObjectLayer, instanc
 			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 _, err = obj.GetObjectInfo(t.Context(), bucketName, "with-version-id", ObjectOptions{VersionID: versionIDs["with-version-id"]}); !isErrVersionNotFound(err) && !isErrObjectNotFound(err) {
+		t.Errorf("%s: matching explicit version still exists: %v", instanceType, err)
+	}
+	for _, objectName := range []string{"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] {