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()) + } + }) + } + } + }}) +}