fix(auth): authorize every tagging proxy destination

Read and authorize every selected remote version before any tagging mutation, and retain the inspected clients for the write phase. Count only attempted writes so disabled targets cannot mask remote failures. Preserve remote errors and missing-version behavior. Cover PUT and DELETE across both erasure backends with divergent tags, invalid credentials, anonymous requests, unavailable reads, missing versions and skipped targets. Refs #337. Signed-off-by: Feng Ruohang <rh@vonng.com>
2026-11-11 10:05:34 +03:00 · 2026-11-11 11:47:56 +08:00
parent 62b8784428
commit 67a90dfef4
3 changed files with 222 additions and 85 deletions
@@ -2775,70 +2775,81 @@ func scheduleReplication(ctx context.Context, oi ObjectInfo, o ObjectLayer, dsc

 // proxyTaggingToRepTarget proxies tagging requests to remote targets for
 // active-active replicated setups
-func proxyTaggingToRepTarget(ctx context.Context, bucket, object string, tags *tags.Tags, opts ObjectOptions, proxyTargets *madmin.BucketTargets) (proxy proxyResult) {
-	// this option is set when active-active replication is in place between site A -> B,
-	// and request hits site B that does not have the object yet.
-	if opts.ProxyRequest || (opts.ProxyHeaderSet && !opts.ProxyRequest) { // true only when site B sets MinIOSourceProxyRequest header
-		return proxy
+func proxyTaggingToRepTarget(ctx context.Context, bucket, object string, tags *tags.Tags, opts ObjectOptions, proxyTargets *madmin.BucketTargets, authorize func(string, error) APIErrorCode) (proxy proxyResult, authErr APIErrorCode) {
+	if opts.ProxyRequest || (opts.ProxyHeaderSet && !opts.ProxyRequest) {
+		return proxy, ErrNone
 	}
+	// Keep the exact clients whose addressed versions we read. A second
+	// discovery could include a newly available, unauthorized destination.
+	var targets []*TargetClient
+	for _, target := range proxyTargets.Targets {
+		tgt := globalBucketTargetSys.GetRemoteTargetClient(bucket, target.Arn)
+		if tgt == nil || globalBucketTargetSys.isOffline(tgt.EndpointURL()) || tgt.disableProxy {
+			continue
+		}
+		targets = append(targets, tgt)
+	}
+	if len(targets) == 0 {
+		err := RemoteTargetConnectionErr{Bucket: bucket, Err: errors.New("no available remote replication target")}
+		return proxyResult{Err: err}, authorize("", err)
+	}
+
 	var wg sync.WaitGroup
-	errs := make([]error, len(proxyTargets.Targets))
-	for idx, t := range proxyTargets.Targets {
-		tgt := globalBucketTargetSys.GetRemoteTargetClient(bucket, t.Arn)
-		if tgt == nil || globalBucketTargetSys.isOffline(tgt.EndpointURL()) {
-			continue
-		}
-		// if proxying explicitly disabled on remote target
-		if tgt.disableProxy {
-			continue
-		}
-		idx := idx
-		wg.Add(1)
-		go func(idx int, tgt *TargetClient) {
-			defer wg.Done()
-			var err error
-			if tags != nil {
-				popts := minio.PutObjectTaggingOptions{
-					VersionID: opts.VersionID,
-					Internal: minio.AdvancedObjectTaggingOptions{
-						ReplicationProxyRequest: "true",
-					},
-				}
-				err = tgt.PutObjectTagging(ctx, tgt.Bucket, object, tags, popts)
-			} else {
-				dopts := minio.RemoveObjectTaggingOptions{
-					VersionID: opts.VersionID,
-					Internal: minio.AdvancedObjectTaggingOptions{
-						ReplicationProxyRequest: "true",
-					},
-				}
-				err = tgt.RemoveObjectTagging(ctx, tgt.Bucket, object, dopts)
+	errs := make([]error, len(targets))
+	existingTags := make([]string, len(targets))
+	for i, tgt := range targets {
+		wg.Go(func() {
+			remoteTags, err := tgt.GetObjectTagging(ctx, tgt.Bucket, object, minio.GetObjectTaggingOptions{
+				VersionID: opts.VersionID,
+				Internal:  minio.AdvancedObjectTaggingOptions{ReplicationProxyRequest: "true"},
+			})
+			errs[i] = err
+			if remoteTags != nil {
+				existingTags[i] = remoteTags.String()
 			}
-			if err != nil {
-				errs[idx] = err
-			}
-		}(idx, tgt)
+		})
 	}
 	wg.Wait()
+	// Authorize every contacted target before any mutation. A tag-based deny
+	// on one copy must not be hidden by a more permissive copy on another.
+	var readErr error
+	for i, err := range errs {
+		if code := authorize(existingTags[i], err); code != ErrNone {
+			return proxy, code
+		}
+		if err != nil && !isRemoteTaggingNotFound(err) && readErr == nil {
+			readErr = err
+		}
+	}
+	if readErr != nil {
+		return proxyResult{Err: readErr}, ErrNone
+	}

-	var (
-		terr        error
-		taggedCount int
-	)
+	for i, tgt := range targets {
+		wg.Go(func() {
+			if tags != nil {
+				errs[i] = tgt.PutObjectTagging(ctx, tgt.Bucket, object, tags, minio.PutObjectTaggingOptions{
+					VersionID: opts.VersionID,
+					Internal:  minio.AdvancedObjectTaggingOptions{ReplicationProxyRequest: "true"},
+				})
+			} else {
+				errs[i] = tgt.RemoveObjectTagging(ctx, tgt.Bucket, object, minio.RemoveObjectTaggingOptions{
+					VersionID: opts.VersionID,
+					Internal:  minio.AdvancedObjectTaggingOptions{ReplicationProxyRequest: "true"},
+				})
+			}
+		})
+	}
+	wg.Wait()
+	// Preserve partial success, counting only targets actually contacted.
+	var lastErr error
 	for _, err := range errs {
 		if err == nil {
-			taggedCount++
-			continue
-		}
-		if err != nil {
-			terr = err
+			return proxy, ErrNone
 		}
+		lastErr = err
 	}
-	// don't return error if at least one target was tagged successfully
-	if taggedCount == 0 && terr != nil {
-		proxy.Err = terr
-	}
-	return proxy
+	return proxyResult{Err: lastErr}, ErrNone
 }

 // proxyGetTaggingToRepTarget proxies get tagging requests to remote targets for
@@ -3841,26 +3841,17 @@ func (api objectAPIHandlers) PutObjectTaggingHandler(w http.ResponseWriter, r *h
 		if isErrObjectNotFound(err) || isErrVersionNotFound(err) {
 			proxytgts := getProxyTargets(ctx, bucket, object, opts)
 			if !proxytgts.Empty() {
-				// The local miss does not authorize a remote mutation. Read
-				// the addressed remote version first, then evaluate the
-				// caller against its actual ExistingObjectTag values.
-				remoteTags, readProxy := proxyGetTaggingToRepTarget(ctx, bucket, object, opts, proxytgts)
-				existingTags := ""
-				if remoteTags != nil {
-					existingTags = remoteTags.String()
-				}
-				if s3Error := authorizeRequestWithExistingTags(ctx, r, policy.PutObjectTaggingAction, existingTags); s3Error != ErrNone {
+				perr, s3Error := proxyTaggingToRepTarget(ctx, bucket, object, tags, opts, proxytgts, func(existingTags string, readErr error) APIErrorCode {
+					if readErr != nil && !isRemoteTaggingNotFound(readErr) {
+						return authorizeRequestWithUnreadTags(ctx, r, policy.PutObjectTaggingAction, readErr)
+					}
+					return authorizeRequestWithExistingTags(ctx, r, policy.PutObjectTaggingAction, existingTags)
+				})
+				if s3Error != ErrNone {
 					writeErrorResponse(ctx, w, errorCodes.ToAPIErr(s3Error), r.URL)
 					return
 				}
-				if readProxy.Err != nil && !isRemoteTaggingNotFound(readProxy.Err) {
-					globalReplicationStats.Load().incProxy(bucket, putObjectTaggingAPI, true)
-					writeErrorResponse(ctx, w, toAPIError(ctx, readProxy.Err), r.URL)
-					return
-				}
 				globalReplicationStats.Load().incProxy(bucket, putObjectTaggingAPI, false)
-				// proxy to replication target if site replication is in place.
-				perr := proxyTaggingToRepTarget(ctx, bucket, object, tags, opts, proxytgts)
 				if perr.Err != nil {
 					globalReplicationStats.Load().incProxy(bucket, putObjectTaggingAPI, true)
 					writeErrorResponse(ctx, w, toAPIError(ctx, perr.Err), r.URL)
@@ -3978,25 +3969,17 @@ func (api objectAPIHandlers) DeleteObjectTaggingHandler(w http.ResponseWriter, r
 		if isErrObjectNotFound(err) || isErrVersionNotFound(err) {
 			proxytgts := getProxyTargets(ctx, bucket, object, opts)
 			if !proxytgts.Empty() {
-				// Authenticate above and authorize against the remote version's
-				// tags before sending a destructive proxy request.
-				remoteTags, readProxy := proxyGetTaggingToRepTarget(ctx, bucket, object, opts, proxytgts)
-				existingTags := ""
-				if remoteTags != nil {
-					existingTags = remoteTags.String()
-				}
-				if s3Error := authorizeRequestWithExistingTags(ctx, r, policy.DeleteObjectTaggingAction, existingTags); s3Error != ErrNone {
+				perr, s3Error := proxyTaggingToRepTarget(ctx, bucket, object, nil, opts, proxytgts, func(existingTags string, readErr error) APIErrorCode {
+					if readErr != nil && !isRemoteTaggingNotFound(readErr) {
+						return authorizeRequestWithUnreadTags(ctx, r, policy.DeleteObjectTaggingAction, readErr)
+					}
+					return authorizeRequestWithExistingTags(ctx, r, policy.DeleteObjectTaggingAction, existingTags)
+				})
+				if s3Error != ErrNone {
 					writeErrorResponse(ctx, w, errorCodes.ToAPIErr(s3Error), r.URL)
 					return
 				}
-				if readProxy.Err != nil && !isRemoteTaggingNotFound(readProxy.Err) {
-					globalReplicationStats.Load().incProxy(bucket, removeObjectTaggingAPI, true)
-					writeErrorResponse(ctx, w, toAPIError(ctx, readProxy.Err), r.URL)
-					return
-				}
 				globalReplicationStats.Load().incProxy(bucket, removeObjectTaggingAPI, false)
-				// proxy to replication target if active-active replication is in place.
-				perr := proxyTaggingToRepTarget(ctx, bucket, object, nil, opts, proxytgts)
 				if perr.Err != nil {
 					globalReplicationStats.Load().incProxy(bucket, removeObjectTaggingAPI, true)
 					writeErrorResponse(ctx, w, toAPIError(ctx, perr.Err), r.URL)
@@ -0,0 +1,143 @@
+// Copyright (c) 2026 Feng Ruohang
+// SPDX-License-Identifier: AGPL-3.0-or-later
+//
+// This program is free software: you can redistribute it and/or modify
+// it under the terms of the GNU Affero General Public License as published by
+// the Free Software Foundation, either version 3 of the License, or
+// (at your option) any later version.
+//
+// This program is distributed in the hope that it will be useful,
+// but WITHOUT ANY WARRANTY; without even the implied warranty of
+// MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+// GNU Affero General Public License for more details.
+//
+// You should have received a copy of the GNU Affero General Public License
+// along with this program. If not, see <http://www.gnu.org/licenses/>.
+
+package cmd
+
+import (
+	"fmt"
+	"net/http"
+	"net/http/httptest"
+	"strings"
+	"sync/atomic"
+	"testing"
+
+	"github.com/minio/madmin-go/v3"
+	"github.com/minio/minio/internal/auth"
+	"github.com/minio/minio/internal/bucket/replication"
+	xhttp "github.com/minio/minio/internal/http"
+)
+
+func TestAPITaggingProxyAuthorization(t *testing.T) {
+	ExecObjectLayerAPITest(ExecObjectLayerAPITestArgs{t: t, objAPITest: func(obj ObjectLayer, backend, bucket string, router http.Handler, root auth.Credentials, t *testing.T) {
+		conditional := newDeleteAuthzPolicyUser(t, backend, bucket, fmt.Sprintf(`[
+			{"Effect":"Allow","Action":["s3:PutObjectTagging","s3:DeleteObjectTagging"],"Resource":["arn:aws:s3:::%s/*"]},
+			{"Effect":"Deny","Action":["s3:PutObjectTagging","s3:DeleteObjectTagging"],"Resource":["arn:aws:s3:::%s/*"],"Condition":{"StringEquals":{"s3:ExistingObjectTag/secret":"true"}}}
+		]`, bucket, bucket))
+		versionID := mustGetUUID()
+		for _, method := range []string{http.MethodPut, http.MethodDelete} {
+			for _, tc := range []struct {
+				name                                string
+				tags                                []string
+				cred                                auth.Credentials
+				readStatus, writeStatus, wantStatus int
+				wantWrites                          int32
+				disabledFirst                       bool
+			}{
+				{name: "deny-second-target", tags: []string{"false", "true"}, cred: conditional, wantStatus: http.StatusForbidden},
+				{name: "deny-first-target", tags: []string{"true", "false"}, cred: conditional, wantStatus: http.StatusForbidden},
+				{name: "all-authorized", tags: []string{"false", "false"}, cred: conditional, wantWrites: 2},
+				{name: "anonymous", tags: []string{"false"}, wantStatus: http.StatusForbidden},
+				{name: "invalid-credentials", tags: []string{"false"}, cred: auth.Credentials{AccessKey: root.AccessKey, SecretKey: "wrong-secret-key"}, wantStatus: http.StatusForbidden},
+				{name: "unreadable", tags: []string{"false"}, cred: root, readStatus: http.StatusServiceUnavailable, wantStatus: http.StatusServiceUnavailable},
+				{name: "missing-version", tags: []string{"false"}, cred: root, readStatus: http.StatusNotFound, writeStatus: http.StatusNotFound, wantStatus: http.StatusNotFound, wantWrites: 1},
+				{name: "disabled-target", tags: []string{"true", "false"}, cred: conditional, disabledFirst: true, wantWrites: 1},
+				{name: "skipped-target-cannot-mask-failure", tags: []string{"false", "false"}, cred: root, disabledFirst: true, writeStatus: http.StatusServiceUnavailable, wantStatus: http.StatusServiceUnavailable, wantWrites: 1},
+			} {
+				t.Run(backend+"/"+method+"/"+tc.name, func(t *testing.T) {
+					var reads, writes atomic.Int32
+					globalBucketTargetSys.Lock()
+					globalBucketTargetSys.targetsMap[bucket] = nil
+					globalBucketTargetSys.Unlock()
+					cfg := replication.Config{}
+					for i, value := range tc.tags {
+						target := newILMTestTarget(t)
+						registerILMTestTarget(t, obj, bucket, target, func(w http.ResponseWriter, r *http.Request) {
+							if r.URL.Query().Get("versionId") != versionID || !r.URL.Query().Has("tagging") || r.Header.Get(xhttp.MinIOSourceProxyRequest) != "true" {
+								t.Errorf("remote request lost addressed version or proxy marker: %s", r.URL)
+							}
+							status := tc.writeStatus
+							if r.Method == http.MethodGet {
+								reads.Add(1)
+								status = tc.readStatus
+							} else {
+								writes.Add(1)
+							}
+							w.Header().Set("Content-Type", "application/xml")
+							if status != 0 {
+								w.WriteHeader(status)
+								code := "ServiceUnavailable"
+								if status == http.StatusNotFound {
+									code = "NoSuchVersion"
+								}
+								fmt.Fprintf(w, "<Error><Code>%s</Code><Message>injected remote result</Message></Error>", code)
+								return
+							}
+							switch r.Method {
+							case http.MethodGet:
+								fmt.Fprintf(w, "<Tagging><TagSet><Tag><Key>secret</Key><Value>%s</Value></Tag></TagSet></Tagging>", value)
+							case http.MethodDelete:
+								w.WriteHeader(http.StatusNoContent)
+							}
+						})
+						globalBucketTargetSys.Lock()
+						globalBucketTargetSys.targetsMap[bucket][i].Credentials = &madmin.Credentials{AccessKey: "proxy-test", SecretKey: "proxy-test-secret"}
+						globalBucketTargetSys.arnRemotesMap[target.arn].Client.disableProxy = tc.disabledFirst && i == 0
+						globalBucketTargetSys.Unlock()
+						rule := configs[0].Rules[0]
+						rule.Priority = i + 1
+						rule.Destination = replication.Destination{ARN: target.arn, Bucket: target.bucket}
+						cfg.Rules = append(cfg.Rules, rule)
+					}
+					meta, err := globalBucketMetadataSys.Get(bucket)
+					if err != nil {
+						t.Fatal(err)
+					}
+					meta.replicationConfig = &cfg
+					globalBucketMetadataSys.Set(bucket, meta)
+					body := ""
+					if method == http.MethodPut {
+						body = "<Tagging><TagSet><Tag><Key>new</Key><Value>value</Value></Tag></TagSet></Tagging>"
+					}
+					path := "/" + bucket + "/remote-only?tagging&versionId=" + versionID
+					var req *http.Request
+					if tc.cred.AccessKey == "" {
+						req = httptest.NewRequest(method, path, strings.NewReader(body))
+					} else {
+						req, err = newTestSignedRequestV4(method, path, int64(len(body)), strings.NewReader(body), tc.cred.AccessKey, tc.cred.SecretKey, nil)
+						if err != nil {
+							t.Fatal(err)
+						}
+					}
+					rec := httptest.NewRecorder()
+					router.ServeHTTP(rec, req)
+					wantStatus := tc.wantStatus
+					if wantStatus == 0 {
+						wantStatus = http.StatusOK
+						if method == http.MethodDelete {
+							wantStatus = http.StatusNoContent
+						}
+					}
+					if rec.Code != wantStatus || writes.Load() != tc.wantWrites {
+						t.Errorf("status=%d writes=%d, want status=%d writes=%d: %s", rec.Code, writes.Load(), wantStatus, tc.wantWrites, rec.Body.String())
+					}
+					if tc.name == "invalid-credentials" && reads.Load() != 0 {
+						t.Errorf("invalid credentials reached %d remote reads", reads.Load())
+					}
+				})
+			}
+		}
+	}})
+}