From 67a90dfef458d3e5228e849711118bc162abed30 Mon Sep 17 00:00:00 2001 From: Feng Ruohang Date: Wed, 11 Nov 2026 11:47:56 +0800 Subject: [PATCH] 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 --- cmd/bucket-replication.go | 119 ++++++++++++++------------ cmd/object-handlers.go | 45 ++++------ cmd/tagging-proxy-authz_test.go | 143 ++++++++++++++++++++++++++++++++ 3 files changed, 222 insertions(+), 85 deletions(-) create mode 100644 cmd/tagging-proxy-authz_test.go diff --git a/cmd/bucket-replication.go b/cmd/bucket-replication.go index f55a45372..81099f504 100644 --- a/cmd/bucket-replication.go +++ b/cmd/bucket-replication.go @@ -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 diff --git a/cmd/object-handlers.go b/cmd/object-handlers.go index bac280fb7..13c122f62 100644 --- a/cmd/object-handlers.go +++ b/cmd/object-handlers.go @@ -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) diff --git a/cmd/tagging-proxy-authz_test.go b/cmd/tagging-proxy-authz_test.go new file mode 100644 index 000000000..0e0f0eb77 --- /dev/null +++ b/cmd/tagging-proxy-authz_test.go @@ -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 . + +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, "%sinjected remote result", code) + return + } + switch r.Method { + case http.MethodGet: + fmt.Fprintf(w, "secret%s", 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 = "newvalue" + } + 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()) + } + }) + } + } + }}) +}