From 55bd906d30e91c81cf58781d2aa89ed5cd78c10a Mon Sep 17 00:00:00 2001 From: Denis Babchuk Date: Sat, 7 Nov 2026 11:43:29 +0300 Subject: [PATCH 1/6] test(auth): cover reads allowed by s3:ExistingObjectTag when the object cannot be read Signed-off-by: Denis Babchuk (cherry picked from commit afa35d16e9395594d20d0599b072b9656e575e8d) Signed-off-by: Feng Ruohang --- cmd/auth-handler-unread-tags_test.go | 246 +++++++++++++++++++++++++++ 1 file changed, 246 insertions(+) create mode 100644 cmd/auth-handler-unread-tags_test.go diff --git a/cmd/auth-handler-unread-tags_test.go b/cmd/auth-handler-unread-tags_test.go new file mode 100644 index 000000000..16a45dc66 --- /dev/null +++ b/cmd/auth-handler-unread-tags_test.go @@ -0,0 +1,246 @@ +// Copyright (c) 2026 Denis Babchuk +// +// This file is part of Silo Object Storage stack +// +// 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 ( + "bytes" + "context" + "encoding/xml" + "fmt" + "io" + "net/http" + "net/http/httptest" + "net/url" + "strings" + "testing" + + "github.com/minio/minio/internal/auth" + xhttp "github.com/minio/minio/internal/http" + "github.com/pgsty/silo-pkg/v3/policy" +) + +// objectReadFault takes the first n drives of the set that holds object +// offline for the metadata reads of that object only, so that IAM and bucket +// metadata stay readable. It returns a function that restores the drives. +func objectReadFault(z *erasureServerPools, bucket, object string, n int) func() { + pool := z.serverPools[0] + setIndex := pool.getHashedSetIndex(object) + pool.erasureDisksMu.Lock() + defer pool.erasureDisksMu.Unlock() + previous := pool.erasureDisks[setIndex] + disks := append([]StorageAPI(nil), previous...) + for i := range n { + disks[i] = consistencyReadFaultDisk{StorageAPI: disks[i], bucket: bucket, object: object} + } + pool.erasureDisks[setIndex] = disks + return func() { + pool.erasureDisksMu.Lock() + pool.erasureDisks[setIndex] = previous + pool.erasureDisksMu.Unlock() + } +} + +// A caller allowed to read an object only through s3:ExistingObjectTag must +// get the storage error, which clients retry, and not AccessDenied, when the +// object's tags cannot be read because read quorum is lost. A caller that no +// tags would let through must still get AccessDenied, and nothing changes +// while the tags can be read. +func TestExistingObjectTagAuthReadQuorumLost(t *testing.T) { + ctx, cancel := context.WithCancel(t.Context()) + defer cancel() + obj, dirs, err := prepareErasure16(ctx) + if err != nil { + t.Fatal(err) + } + z := obj.(*erasureServerPools) + previous := newObjectLayerFn() + setObjectLayer(z) + defer func() { + z.Shutdown(context.Background()) + removeRoots(dirs) + setObjectLayer(previous) + }() + bucket, router, err := initAPIHandlerTest(ctx, z, nil, MakeBucketOptions{}) + if err != nil { + t.Fatal(err) + } + + statement := func(effect, resource, conditions string) string { + st := fmt.Sprintf(`{"Effect": %q, "Action": ["s3:GetObject", "s3:PutObjectTagging"], "Resource": ["arn:aws:s3:::%s/%s"]`, + effect, bucket, resource) + if conditions != "" { + st += `, "Condition": ` + conditions + } + return st + "}" + } + tagStatement := func(effect, resource, value string) string { + return statement(effect, resource, fmt.Sprintf(`{"StringEquals": {"s3:ExistingObjectTag/k": %q}}`, value)) + } + newUser := func(statements ...string) auth.Credentials { + return newDeleteAuthzPolicyUser(t, "", bucket, "["+strings.Join(statements, ",")+"]") + } + newServiceAccount := func(parent auth.Credentials, statements string) auth.Credentials { + sessionPolicy, err := policy.ParseConfig(strings.NewReader(`{"Version": "2012-10-17", "Statement": [` + statements + `]}`)) + if err != nil { + t.Fatal(err) + } + cred, _, err := globalIAMSys.NewServiceAccount(ctx, parent.AccessKey, nil, newServiceAccountOpts{sessionPolicy: sessionPolicy}) + if err != nil { + t.Fatal(err) + } + return cred + } + + type user struct { + name string + creds auth.Credentials + // The status with the object's tags readable and with read quorum + // lost: 200 and 503 for a caller that some tags let through, 403 + // and 403 for a caller that none do. + healthy, quorumLost int + } + allowedByTags := func(name string, creds auth.Credentials) user { + return user{name, creds, http.StatusOK, http.StatusServiceUnavailable} + } + deniedByAnyTags := func(name string, creds auth.Credentials) user { + return user{name, creds, http.StatusForbidden, http.StatusForbidden} + } + // Each case below that tests a tag key of its own uses a key no other + // policy asks for. + ownerCondition := `{"StringEquals": {"s3:ExistingObjectTag/owner": "${aws:username}"}}` + owner := newUser(statement("Allow", "*", ownerCondition)) + users := []user{ + // Allowed only while the object carries k=v. + allowedByTags("tagged", newUser(tagStatement("Allow", "*", "v"))), + // Allowed while the object carries a tag np, whatever its value. + allowedByTags("nullFalse", newUser(statement("Allow", "*", `{"Null": {"s3:ExistingObjectTag/np": "false"}}`))), + // Allowed while a tag ne is there and is not w. + allowedByTags("notEquals", newUser(statement("Allow", "*", + `{"Null": {"s3:ExistingObjectTag/ne": "false"}, "StringNotEquals": {"s3:ExistingObjectTag/ne": "w"}}`))), + // Allowed while every value of tag fa differs from w; that needs a tag fa. + allowedByTags("forAllNotEquals", newUser(statement("Allow", "*", `{"ForAllValues:StringNotEquals": {"s3:ExistingObjectTag/fa": "w"}}`))), + // Allowed while the object is tagged with the caller's name. + allowedByTags("owner", owner), + // Allowed, but denied unless the object carries du=1. + allowedByTags("denyUnlessTagged", newUser(statement("Allow", "*", ""), + statement("Deny", "*", `{"StringNotEquals": {"s3:ExistingObjectTag/du": "1"}}`))), + // A service account whose parent asks for k=v and whose own policy + // asks for j=w: only an object carrying both lets it through. + allowedByTags("serviceAccount", newServiceAccount(newUser(tagStatement("Allow", "*", "v")), + statement("Allow", "*", `{"StringEquals": {"s3:ExistingObjectTag/j": "w"}}`))), + + // Allowed only for k=w: the decision still depends on the tags. + {"otherValue", newUser(tagStatement("Allow", "*", "w")), http.StatusForbidden, http.StatusServiceUnavailable}, + + // Allowed for k=v under another prefix only. + deniedByAnyTags("otherPrefix", newUser(tagStatement("Allow", "other/*", "v"))), + // Allowed and denied for k=v alike. + deniedByAnyTags("allowDeny", newUser(tagStatement("Allow", "*", "v"), tagStatement("Deny", "*", "v"))), + // Allowed only without a tag nc that is 1. + deniedByAnyTags("nullConflict", newUser(statement("Allow", "*", + `{"Null": {"s3:ExistingObjectTag/nc": "true"}, "StringEquals": {"s3:ExistingObjectTag/nc": "1"}}`))), + // Tagged with the caller's name, under another prefix only. + deniedByAnyTags("ownerPrefix", newUser(statement("Allow", "other/*", ownerCondition))), + // Denied unless the object carries db=1, and denied if it does. + deniedByAnyTags("denyBoth", newUser(statement("Allow", "*", ""), + statement("Deny", "*", `{"StringNotEquals": {"s3:ExistingObjectTag/db": "1"}}`), + statement("Deny", "*", `{"StringEquals": {"s3:ExistingObjectTag/db": "1"}}`))), + // The parent asks for k=v and the service account for k=x. + deniedByAnyTags("serviceAccountConflict", newServiceAccount(newUser(tagStatement("Allow", "*", "v")), + tagStatement("Allow", "*", "x"))), + } + + const object, missing = "tagged", "missing" + if _, err := z.PutObject(ctx, bucket, object, mustGetPutObjReader(t, bytes.NewReader([]byte("data")), 4, "", ""), + ObjectOptions{UserDefined: map[string]string{xhttp.AmzObjectTagging: "k=v&j=w&np=1&ne=1&fa=1&du=1&owner=" + owner.AccessKey}}); err != nil { + t.Fatal(err) + } + // Anonymous reads are allowed by the bucket policy while the object carries k=v. + if _, err := globalBucketMetadataSys.Update(ctx, bucket, bucketPolicyConfig, []byte(`{"Version": "2012-10-17", "Statement": [ + {"Effect": "Allow", "Principal": {"AWS": ["*"]}, "Action": ["s3:GetObject"], "Resource": ["arn:aws:s3:::`+bucket+`/*"], + "Condition": {"StringEquals": {"s3:ExistingObjectTag/k": "v"}}}]}`)); err != nil { + t.Fatal(err) + } + + type request struct { + name string + method, target string + header map[string]string + body []byte + } + objectURL := getGetObjectURL("", bucket, object) + get := request{"GET", http.MethodGet, objectURL, nil, nil} + head := request{"HEAD", http.MethodHead, objectURL, nil, nil} + taggingURL := makeTestTargetURL("", bucket, object, url.Values{"tagging": []string{""}}) + taggingBody := []byte(`kv`) + putTagging := request{"PutObjectTagging", http.MethodPut, taggingURL, nil, taggingBody} + check := func(stage string, creds auth.Credentials, name string, req request, want int) { + t.Helper() + var rec *httptest.ResponseRecorder + if creds.AccessKey == "" { + anonymous, err := newTestRequest(req.method, req.target, 0, nil) + if err != nil { + t.Fatal(err) + } + rec = httptest.NewRecorder() + router.ServeHTTP(rec, anonymous) + } else { + var body io.ReadSeeker + if req.body != nil { + body = bytes.NewReader(req.body) + } + signed, err := newTestSignedRequestV4(req.method, req.target, int64(len(req.body)), body, creds.AccessKey, creds.SecretKey, req.header) + if err != nil { + t.Fatal(err) + } + rec = httptest.NewRecorder() + router.ServeHTTP(rec, signed) + } + if rec.Code != want { + var apiErr APIErrorResponse + _ = xml.Unmarshal(rec.Body.Bytes(), &apiErr) + t.Errorf("%s: %s by %s: status %d %s, want %d", stage, req.name, name, rec.Code, apiErr.Code, want) + } + } + + var anonymous auth.Credentials + for _, u := range users { + check("healthy", u.creds, u.name, get, u.healthy) + } + check("healthy", anonymous, "anonymous", get, http.StatusOK) + // The tags were read and allow the request: the range error is the answer. + check("healthy", users[0].creds, users[0].name, + request{"GET bytes=100-200", http.MethodGet, objectURL, map[string]string{xhttp.Range: "bytes=100-200"}, nil}, + http.StatusRequestedRangeNotSatisfiable) + // Without an object there are no tags: AccessDenied, as before. + missingURL := getGetObjectURL("", bucket, missing) + check("missing key", users[0].creds, users[0].name, request{"GET", http.MethodGet, missingURL, nil, nil}, http.StatusForbidden) + check("missing key", users[0].creds, users[0].name, request{"HEAD", http.MethodHead, missingURL, nil, nil}, http.StatusForbidden) + + defer objectReadFault(z, bucket, object, 10)() + if _, err := z.GetObjectInfo(ctx, bucket, object, ObjectOptions{}); !isErrReadQuorum(err) { + t.Fatalf("GetObjectInfo with 10 of 16 drives failing: %v, want InsufficientReadQuorum", err) + } + for _, req := range []request{get, head, putTagging} { + for _, u := range users { + check("read quorum lost", u.creds, u.name, req, u.quorumLost) + } + } + check("read quorum lost", anonymous, "anonymous", get, http.StatusServiceUnavailable) + check("read quorum lost", anonymous, "anonymous", head, http.StatusServiceUnavailable) +} From 89028732ab2dc69d852ca8eb37cbd50fe84d9ce4 Mon Sep 17 00:00:00 2001 From: Denis Babchuk Date: Sat, 7 Nov 2026 11:43:30 +0300 Subject: [PATCH 2/6] fix(auth): keep the server error when a read allowed by object tags cannot read the tags Signed-off-by: Denis Babchuk (cherry picked from commit 3c4a6296f95e8b722a57feee6d16cb88a3108e47) Signed-off-by: Feng Ruohang --- cmd/auth-handler-unread-tags.go | 289 +++++++++++++++++++++++++++ cmd/auth-handler-unread-tags_test.go | 59 ++++++ cmd/object-handlers.go | 31 ++- 3 files changed, 372 insertions(+), 7 deletions(-) create mode 100644 cmd/auth-handler-unread-tags.go diff --git a/cmd/auth-handler-unread-tags.go b/cmd/auth-handler-unread-tags.go new file mode 100644 index 000000000..d595d2bf2 --- /dev/null +++ b/cmd/auth-handler-unread-tags.go @@ -0,0 +1,289 @@ +// Copyright (c) 2026 Denis Babchuk +// +// This file is part of Silo Object Storage stack +// +// 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 ( + "context" + "encoding/json" + "maps" + "net/http" + "slices" + "strings" + + "github.com/minio/minio-go/v7/pkg/set" + "github.com/minio/minio-go/v7/pkg/tags" + "github.com/minio/minio/internal/auth" + "github.com/minio/minio/internal/logger" + "github.com/pgsty/silo-pkg/v3/policy" + "github.com/pgsty/silo-pkg/v3/policy/condition" +) + +// authorizeRequestWithUnreadTags authorizes action on an object whose stored +// tags could not be read because reading the object failed with readErr. +// +// Without the tags, an Allow statement that tests s3:ExistingObjectTag cannot +// match, so a caller allowed only by such a statement would get AccessDenied, +// which clients do not retry, for a transient server error such as lost read +// quorum. For a server error the request is therefore also authorized as if +// the object carried tags that the policies ask for: a caller that some tags +// let through gets the error, a caller that no tags let through still gets +// AccessDenied. For any other error, NoSuchKey among them, the tags stay empty. +func authorizeRequestWithUnreadTags(ctx context.Context, r *http.Request, action policy.Action, readErr error) APIErrorCode { + s3Err := authorizeRequest(ctx, r, action) + if s3Err == ErrNone || toAPIError(ctx, readErr).HTTPStatusCode < http.StatusInternalServerError { + return s3Err + } + for _, existingTags := range policyExistingObjectTags(ctx, r, action) { + if authorizeRequestWithExistingTags(ctx, r, action, existingTags) == ErrNone { + return ErrNone + } + } + return s3Err +} + +// maxExistingTagCandidates bounds the tag sets tried for one request; each +// one costs an authorization. +const maxExistingTagCandidates = 64 + +// policyExistingObjectTags returns tag sets under which the policies that +// authorize the request may let action through on its object: the bucket +// policy for an anonymous request, else the cached IAM policies and, as a +// second policy set, the session policy of the credential. A tag set passes +// the s3:ExistingObjectTag tests of one Allow statement of each policy set and, +// as far as the same tags allow, keeps the Deny statements that test tags from +// matching. The tag sets are only candidates; the regular authorization +// decides what they allow. +func policyExistingObjectTags(ctx context.Context, r *http.Request, action policy.Action) []string { + reqInfo := logger.GetReqInfo(ctx) + if reqInfo == nil { + return nil + } + bucket, object := reqInfo.BucketName, reqInfo.ObjectName + var ( + allows [][]map[string]string // per policy set, the tags its Allow statements ask for + denies []map[string]string // tags that keep a Deny statement from matching + ) + // add files the tags of a statement that applies to action on the + // object, whatever its conditions. + add := func(effect policy.Effect, conditions condition.Functions, values map[string][]string, fromSet *[]map[string]string) { + if tagMap, ok := existingObjectTagsFor(effect, conditions, values); ok { + if effect == policy.Allow { + *fromSet = append(*fromSet, tagMap) + } else { + denies = append(denies, tagMap) + } + } + } + switch { + case reqInfo.Cred.AccessKey == "": + p, err := globalPolicySys.Get(bucket) + if err != nil { + return nil + } + values := getConditionValues(r, reqInfo.Region, auth.AnonymousCredentials) + var fromSet []map[string]string + for _, st := range p.Statements { + match := st + match.Effect, match.Conditions = policy.Allow, nil + if match.IsAllowed(policy.BucketPolicyArgs{Action: action, BucketName: bucket, ObjectName: object, ConditionValues: values}) { + add(st.Effect, st.Conditions, values, &fromSet) + } + } + allows = append(allows, fromSet) + case newGlobalAuthZPluginFn() == nil && globalIAMSys.Initialized(): + values := getConditionValues(r, "", reqInfo.Cred) + // From the cache only: reading the drives may be what just failed. + docs, _ := globalIAMSys.store.listPolicyDocs(ctx, "") + policySets := [][]policy.Policy{nil} + for _, name := range slices.Sorted(maps.Keys(docs)) { + policySets[0] = append(policySets[0], docs[name].Policy) + } + if sp, ok := reqInfo.Cred.Claims[sessionPolicyNameExtracted].(string); ok { + if p, err := policy.ParseConfig(strings.NewReader(sp)); err == nil { + policySets = append(policySets, []policy.Policy{*p}) + } + } + for _, policies := range policySets { + var fromSet []map[string]string + for _, p := range policies { + for _, st := range p.Statements { + match := st + match.Effect, match.Conditions = policy.Allow, nil + if match.IsAllowed(policy.Args{Action: action, BucketName: bucket, ObjectName: object, ConditionValues: values}) { + add(st.Effect, st.Conditions, values, &fromSet) + } + } + } + allows = append(allows, fromSet) + } + } + return combineExistingObjectTags(allows, denies) +} + +// combineExistingObjectTags merges the tags of one Allow statement of each +// policy set, or of none for a set that may allow without tags, and tries each +// result also with the tags that keep the Deny statements from matching. The +// first tags win a conflict; the authorization of the result settles it. +func combineExistingObjectTags(allows [][]map[string]string, denies []map[string]string) []string { + merge := func(parts ...map[string]string) map[string]string { + merged := make(map[string]string) + for _, part := range parts { + for k, v := range part { + if _, ok := merged[k]; !ok { + merged[k] = v + } + } + } + return merged + } + combos := [][]map[string]string{nil} + for _, fromSet := range allows { + // Many statements ask for the same tags. + seen := set.NewStringSet() + fromSet = slices.DeleteFunc(slices.Clone(fromSet), func(tagMap map[string]string) bool { + t, err := tags.MapToObjectTags(tagMap) + if err != nil || seen.Contains(t.String()) { + return true + } + seen.Add(t.String()) + return false + }) + next := make([][]map[string]string, 0, len(combos)*(len(fromSet)+1)) + for _, combo := range combos { + next = append(next, combo) + for _, tagMap := range fromSet { + next = append(next, append(slices.Clip(combo), tagMap)) + } + } + combos = next[:min(len(next), maxExistingTagCandidates)] + } + deny := merge(denies...) + found := set.NewStringSet() + var candidates []string + for _, combo := range combos { + for _, tagMap := range []map[string]string{merge(combo...), merge(append(slices.Clip(combo), deny)...)} { + t, err := tags.MapToObjectTags(tagMap) + if len(tagMap) == 0 || err != nil || found.Contains(t.String()) { + continue + } + found.Add(t.String()) + if candidates = append(candidates, t.String()); len(candidates) == maxExistingTagCandidates { + return candidates + } + } + } + return candidates +} + +// existingObjectTagsFor returns tags under which the s3:ExistingObjectTag tests +// of conditions all pass, for an Allow statement, or one of them fails, for a +// Deny statement; a tag left out is absent. It returns false if the conditions +// test no tag, or if no such tags were found among those tried for each tag: +// none, the values the tests name, with policy variables taken from values and +// wildcards filled, and the fillers "" and "_". The policy engine itself +// evaluates the tests for each of them. +func existingObjectTagsFor(effect policy.Effect, conditions condition.Functions, values map[string][]string) (map[string]string, bool) { + // Each function tests one key, so the tests of one tag pass or fail + // whatever the other tags are. + byKey := make(map[string]condition.Functions) + for _, f := range conditions { + for key := range condition.NewFunctions(f).Keys() { + if key.Is(condition.ExistingObjectTag) { + byKey[key.Name()] = append(byKey[key.Name()], f) + } + } + } + if len(byKey) == 0 { + return nil, false + } + tagMap := make(map[string]string) + for _, key := range slices.Sorted(maps.Keys(byKey)) { + tagKey := strings.TrimPrefix(key, condition.ExistingObjectTag.Name()+"/") + value, present, found := existingObjectTagValue(key, tagKey, byKey[key], values, effect == policy.Allow) + switch { + case effect == policy.Deny && found: + if present { + tagMap[tagKey] = value + } + return tagMap, true + case effect == policy.Allow && !found: + return nil, false + case present: + tagMap[tagKey] = value + } + } + if effect == policy.Deny { + return nil, false + } + return tagMap, true +} + +// existingObjectTagValue looks for a value of tag tagKey, or its absence, +// under which tests, the functions that test the condition key key, evaluate +// to want. +func existingObjectTagValue(key, tagKey string, tests condition.Functions, values map[string][]string, want bool) (value string, present, found bool) { + defer delete(values, key) + delete(values, key) + if tests.Evaluate(values) == want { + return "", false, true + } + // The functions keep their values unexported; JSON exposes them. + buf, err := json.Marshal(tests) + if err != nil { + return "", false, false + } + var byName map[string]map[string]condition.ValueSet + if err = json.Unmarshal(buf, &byName); err != nil { + return "", false, false + } + var tried []string + for name, byKey := range byName { + if name == "Null" { + continue // its operand says whether the tag is there, not a value + } + for _, valueSet := range byKey { + for _, v := range valueSet.ToSlice() { + s := substituteConditionVariables(v.String(), values) + tried = append(tried, s, strings.NewReplacer("*", "", "?", "_").Replace(s)) + } + } + } + slices.Sort(tried) + for _, v := range append(slices.Compact(tried), "", "_") { + if _, err := tags.MapToObjectTags(map[string]string{tagKey: v}); err != nil { + continue + } + values[key] = []string{v} + if tests.Evaluate(values) == want { + return v, true, true + } + } + return "", false, false +} + +// substituteConditionVariables resolves the policy variables in v from the +// condition values as the policy engine does for the values of a condition; +// silo-pkg keeps its own function for this unexported. +func substituteConditionVariables(v string, values map[string][]string) string { + for _, key := range condition.CommonKeys { + if rvalues, ok := values[key.Name()]; ok && len(rvalues) > 0 && rvalues[0] != "" { + v = strings.ReplaceAll(v, key.VarName(), rvalues[0]) + } + } + return v +} diff --git a/cmd/auth-handler-unread-tags_test.go b/cmd/auth-handler-unread-tags_test.go index 16a45dc66..a42b04574 100644 --- a/cmd/auth-handler-unread-tags_test.go +++ b/cmd/auth-handler-unread-tags_test.go @@ -20,6 +20,7 @@ package cmd import ( "bytes" "context" + "encoding/json" "encoding/xml" "fmt" "io" @@ -29,9 +30,11 @@ import ( "strings" "testing" + "github.com/minio/minio-go/v7/pkg/tags" "github.com/minio/minio/internal/auth" xhttp "github.com/minio/minio/internal/http" "github.com/pgsty/silo-pkg/v3/policy" + "github.com/pgsty/silo-pkg/v3/policy/condition" ) // objectReadFault takes the first n drives of the set that holds object @@ -244,3 +247,59 @@ func TestExistingObjectTagAuthReadQuorumLost(t *testing.T) { check("read quorum lost", anonymous, "anonymous", get, http.StatusServiceUnavailable) check("read quorum lost", anonymous, "anonymous", head, http.StatusServiceUnavailable) } + +func TestExistingObjectTagsFor(t *testing.T) { + values := map[string][]string{"username": {"alice"}, "sub": {"u1"}} + for _, tc := range []struct { + effect policy.Effect + conditions string + want string // the tags to set, encoded as in x-amz-tagging + ok bool // false: no tags for this statement + }{ + {policy.Allow, `{"StringEquals": {"s3:ExistingObjectTag/k": "v"}}`, "k=v", true}, + {policy.Allow, `{"StringEqualsIgnoreCase": {"s3:ExistingObjectTag/k": "V"}, "IpAddress": {"aws:SourceIp": "10.0.0.0/8"}}`, "k=V", true}, + {policy.Allow, `{"ForAnyValue:StringEquals": {"s3:ExistingObjectTag/k": ["b", "a"]}}`, "k=a", true}, + {policy.Allow, `{"StringLike": {"s3:ExistingObjectTag/k": "a?c*"}}`, "k=a_c", true}, + {policy.Allow, `{"StringEquals": {"s3:ExistingObjectTag/b": "2", "s3:ExistingObjectTag/a": "1"}}`, "a=1&b=2", true}, + // Passed without the tag. + {policy.Allow, `{"StringNotEquals": {"s3:ExistingObjectTag/k": "v"}}`, "", true}, + {policy.Allow, `{"Null": {"s3:ExistingObjectTag/k": "true"}}`, "", true}, + // Null "false" asks for the tag, with any value. + {policy.Allow, `{"Null": {"s3:ExistingObjectTag/k": "false"}}`, "k=", true}, + {policy.Allow, `{"Null": {"s3:ExistingObjectTag/k": "false"}, "StringNotEquals": {"s3:ExistingObjectTag/k": ["", "w"]}}`, "k=_", true}, + {policy.Allow, `{"Null": {"s3:ExistingObjectTag/k": "false"}, "StringNotLike": {"s3:ExistingObjectTag/k": "*w"}}`, "k=", true}, + {policy.Allow, `{"ForAllValues:StringNotEquals": {"s3:ExistingObjectTag/k": "w"}}`, "k=", true}, + {policy.Allow, `{"Null": {"s3:ExistingObjectTag/k": "true"}, "StringEquals": {"s3:ExistingObjectTag/k": "v"}}`, "", false}, + // Policy variables take the values of the request. + {policy.Allow, `{"StringEquals": {"s3:ExistingObjectTag/owner": "${aws:username}"}}`, "owner=alice", true}, + {policy.Allow, `{"StringLike": {"s3:ExistingObjectTag/owner": "${jwt:sub}-*"}}`, "owner=u1-", true}, + {policy.Allow, `{"StringEquals": {"s3:ExistingObjectTag/owner": "${ldap:user}"}}`, "", false}, + // A Deny is kept from matching. + {policy.Deny, `{"StringNotEquals": {"s3:ExistingObjectTag/k": "v"}}`, "k=v", true}, + {policy.Deny, `{"Null": {"s3:ExistingObjectTag/k": "true"}}`, "k=", true}, + {policy.Deny, `{"StringEquals": {"s3:ExistingObjectTag/k": "v"}}`, "", true}, + {policy.Deny, `{"StringEquals": {"s3:prefix": "v"}}`, "", false}, + // No tag tests. + {policy.Allow, `{"StringEquals": {"s3:prefix": "v"}}`, "", false}, + {policy.Allow, "", "", false}, + } { + var conditions condition.Functions // nil: a statement without a Condition + if tc.conditions != "" { + if err := json.Unmarshal([]byte(tc.conditions), &conditions); err != nil { + t.Fatalf("%s: %v", tc.conditions, err) + } + } + tagMap, ok := existingObjectTagsFor(tc.effect, conditions, values) + got := "" + if len(tagMap) > 0 { + encoded, err := tags.MapToObjectTags(tagMap) + if err != nil { + t.Fatalf("%s: %v", tc.conditions, err) + } + got = encoded.String() + } + if got != tc.want || ok != tc.ok { + t.Errorf("%s %s: got %q %v, want %q %v", tc.effect, tc.conditions, got, ok, tc.want, tc.ok) + } + } +} diff --git a/cmd/object-handlers.go b/cmd/object-handlers.go index 3d3c3626a..553254e14 100644 --- a/cmd/object-handlers.go +++ b/cmd/object-handlers.go @@ -395,12 +395,14 @@ func (api objectAPIHandlers) getObjectHandler(ctx context.Context, objectAPI Obj } // Validate pre-conditions if any. + var storedTags *string // set once the object has been read far enough opts.CheckPrecondFn = func(oi ObjectInfo) bool { if _, err := DecryptObjectInfo(&oi, r); err != nil { writeErrorResponse(ctx, w, toAPIError(ctx, err), r.URL) return true } + storedTags = &oi.UserTags if s3Error := authorizeRequestWithExistingTags(ctx, r, policy.GetObjectAction, oi.UserTags); s3Error != ErrNone { writeErrorResponse(ctx, w, errorCodes.ToAPIErr(s3Error), r.URL) return true @@ -445,8 +447,14 @@ func (api objectAPIHandlers) getObjectHandler(ctx context.Context, objectAPI Obj return } // validate if the request indeed was authorized, if it wasn't we need to return "ErrAccessDenied" - // instead of any namespace related error. - if s3Error := authorizeRequest(ctx, r, policy.GetObjectAction); s3Error != ErrNone { + // instead of any namespace related error. Use the stored tags if the callback read them. + var s3Error APIErrorCode + if storedTags != nil { + s3Error = authorizeRequestWithExistingTags(ctx, r, policy.GetObjectAction, *storedTags) + } else { + s3Error = authorizeRequestWithUnreadTags(ctx, r, policy.GetObjectAction, err) + } + if s3Error != ErrNone { writeErrorResponse(ctx, w, errorCodes.ToAPIErr(s3Error), r.URL) return } @@ -925,7 +933,14 @@ func (api objectAPIHandlers) headObjectHandler(ctx context.Context, objectAPI Ob } } - if s3Error := authorizeRequestWithExistingTags(ctx, r, policy.GetObjectAction, objInfo.UserTags); s3Error != ErrNone { + // objInfo carries the stored tags unless reading the object failed. + var s3Error APIErrorCode + if err != nil && !proxy.Proxy { + s3Error = authorizeRequestWithUnreadTags(ctx, r, policy.GetObjectAction, err) + } else { + s3Error = authorizeRequestWithExistingTags(ctx, r, policy.GetObjectAction, objInfo.UserTags) + } + if s3Error != ErrNone { writeErrorResponseHeadersOnly(w, errorCodes.ToAPIErr(s3Error)) return } @@ -3783,11 +3798,13 @@ func (api objectAPIHandlers) PutObjectTaggingHandler(w http.ResponseWriter, r *h } objInfo, err := objAPI.GetObjectInfo(ctx, bucket, object, opts) - existingTags := "" - if err == nil { - existingTags = objInfo.UserTags + var s3Error APIErrorCode + if err != nil { + s3Error = authorizeRequestWithUnreadTags(ctx, r, policy.PutObjectTaggingAction, err) + } else { + s3Error = authorizeRequestWithExistingTags(ctx, r, policy.PutObjectTaggingAction, objInfo.UserTags) } - if s3Error := authorizeRequestWithExistingTags(ctx, r, policy.PutObjectTaggingAction, existingTags); s3Error != ErrNone { + if s3Error != ErrNone { writeErrorResponse(ctx, w, errorCodes.ToAPIErr(s3Error), r.URL) return } From 80dd3e3ce11bb2500f264a760a8cf37c60d4f852 Mon Sep 17 00:00:00 2001 From: Denis Babchuk Date: Sat, 7 Nov 2026 11:43:31 +0300 Subject: [PATCH 3/6] test(api): cover GetObjectAttributes errors with and without s3:ListBucket Signed-off-by: Denis Babchuk (cherry picked from commit 67bc32f401befa09d17ac1b0b870d3b4a49bf106) Signed-off-by: Feng Ruohang --- cmd/object-attributes-read-error_test.go | 91 ++++++++++++++++++++++++ 1 file changed, 91 insertions(+) create mode 100644 cmd/object-attributes-read-error_test.go diff --git a/cmd/object-attributes-read-error_test.go b/cmd/object-attributes-read-error_test.go new file mode 100644 index 000000000..7b00495fa --- /dev/null +++ b/cmd/object-attributes-read-error_test.go @@ -0,0 +1,91 @@ +// Copyright (c) 2026 Denis Babchuk +// +// This file is part of Silo Object Storage stack +// +// 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 ( + "bytes" + "context" + "encoding/xml" + "net/http" + "net/http/httptest" + "net/url" + "testing" + + "github.com/minio/minio/internal/auth" + xhttp "github.com/minio/minio/internal/http" +) + +// GetObjectAttributes answers NoSuchKey with AccessDenied to a caller without +// s3:ListBucket on the bucket, as S3 does, so that a missing object cannot be +// told apart from a forbidden one. A server error, such as lost read quorum, +// must reach a caller allowed to read the object as the error it is. +func TestGetObjectAttributesReadQuorumLost(t *testing.T) { + ctx, cancel := context.WithCancel(t.Context()) + defer cancel() + obj, dirs, err := prepareErasure16(ctx) + if err != nil { + t.Fatal(err) + } + z := obj.(*erasureServerPools) + previous := newObjectLayerFn() + setObjectLayer(z) + defer func() { + z.Shutdown(context.Background()) + removeRoots(dirs) + setObjectLayer(previous) + }() + bucket, router, err := initAPIHandlerTest(ctx, z, nil, MakeBucketOptions{}) + if err != nil { + t.Fatal(err) + } + const object, missing = "object", "missing" + if _, err := z.PutObject(ctx, bucket, object, mustGetPutObjReader(t, bytes.NewReader([]byte("data")), 4, "", ""), ObjectOptions{}); err != nil { + t.Fatal(err) + } + + read := `{"Effect": "Allow", "Action": ["s3:GetObject", "s3:GetObjectAttributes"], "Resource": ["arn:aws:s3:::` + bucket + `/*"]}` + withoutList := newDeleteAuthzPolicyUser(t, "", bucket, "["+read+"]") + withList := newDeleteAuthzPolicyUser(t, "", bucket, "["+read+ + `, {"Effect": "Allow", "Action": ["s3:ListBucket"], "Resource": ["arn:aws:s3:::`+bucket+`"]}]`) + + check := func(stage, user string, creds auth.Credentials, key string, want int) { + t.Helper() + target := makeTestTargetURL("", bucket, key, url.Values{"attributes": []string{""}}) + req, err := newTestSignedRequestV4(http.MethodGet, target, 0, nil, creds.AccessKey, creds.SecretKey, + map[string]string{xhttp.AmzObjectAttributes: "ETag"}) + if err != nil { + t.Fatal(err) + } + rec := httptest.NewRecorder() + router.ServeHTTP(rec, req) + if rec.Code != want { + var apiErr APIErrorResponse + _ = xml.Unmarshal(rec.Body.Bytes(), &apiErr) + t.Errorf("%s: GetObjectAttributes %s by %s: status %d %s, want %d", stage, key, user, rec.Code, apiErr.Code, want) + } + } + + check("healthy", "withoutList", withoutList, object, http.StatusOK) + check("healthy", "withList", withList, object, http.StatusOK) + check("healthy", "withoutList", withoutList, missing, http.StatusForbidden) + check("healthy", "withList", withList, missing, http.StatusNotFound) + + defer objectReadFault(z, bucket, object, 10)() + check("read quorum lost", "withoutList", withoutList, object, http.StatusServiceUnavailable) + check("read quorum lost", "withList", withList, object, http.StatusServiceUnavailable) +} From c6949a38c2a5c08b17c3a0baf3bee60be7c4754b Mon Sep 17 00:00:00 2001 From: Denis Babchuk Date: Sat, 7 Nov 2026 11:43:31 +0300 Subject: [PATCH 4/6] fix(api): keep GetObjectAttributes server errors and check s3:ListBucket on the bucket Signed-off-by: Denis Babchuk (cherry picked from commit a2ff3c2ecde8e71a5442340a82053273737acbaa) Signed-off-by: Feng Ruohang --- cmd/auth-handler.go | 13 +++++++++++++ cmd/object-handlers.go | 13 ++++++++----- 2 files changed, 21 insertions(+), 5 deletions(-) diff --git a/cmd/auth-handler.go b/cmd/auth-handler.go index a20a74505..700126b47 100644 --- a/cmd/auth-handler.go +++ b/cmd/auth-handler.go @@ -425,6 +425,19 @@ func authorizeRequest(ctx context.Context, r *http.Request, action policy.Action return authorizeRequestWithExistingTags(ctx, r, action, "") } +// authorizeBucketRequest authorizes a bucket action, such as s3:ListBucket, +// on the bucket of a request made for one of its objects. +func authorizeBucketRequest(ctx context.Context, r *http.Request, action policy.Action) APIErrorCode { + reqInfo := logger.GetReqInfo(ctx) + if reqInfo == nil { + return ErrAccessDenied + } + objectName := reqInfo.ObjectName + reqInfo.ObjectName = "" + defer func() { reqInfo.ObjectName = objectName }() + return authorizeRequest(ctx, r, action) +} + func deleteObjectAction(versionID string) policy.Action { if versionID != "" { return policy.DeleteObjectVersionAction diff --git a/cmd/object-handlers.go b/cmd/object-handlers.go index 553254e14..a8f7d70d9 100644 --- a/cmd/object-handlers.go +++ b/cmd/object-handlers.go @@ -631,12 +631,15 @@ func (api objectAPIHandlers) getObjectAttributesHandler(ctx context.Context, obj objInfo, err := objectAPI.GetObjectInfo(ctx, bucket, object, opts) if err != nil { - s3Error = authorizeRequest(ctx, r, policy.ListBucketAction) - if s3Error == ErrNone { - writeErrorResponse(ctx, w, toAPIError(ctx, err), r.URL) - return + // As with S3, a caller without s3:ListBucket on the bucket gets + // AccessDenied in place of NoSuchKey. A server error, such as lost + // read quorum, tells nothing about the object and reaches the caller, + // who may read the object, as it is. + apiErr := toAPIError(ctx, err) + if apiErr.HTTPStatusCode < http.StatusInternalServerError && authorizeBucketRequest(ctx, r, policy.ListBucketAction) != ErrNone { + apiErr = errorCodes.ToAPIErr(ErrAccessDenied) } - writeErrorResponse(ctx, w, errorCodes.ToAPIErr(ErrAccessDenied), r.URL) + writeErrorResponse(ctx, w, apiErr, r.URL) return } From 65acda0114fffb2d9e66366af2341a8973e581f6 Mon Sep 17 00:00:00 2001 From: Feng Ruohang Date: Tue, 10 Nov 2026 23:22:57 +0800 Subject: [PATCH 5/6] fix(auth): scope unread tag candidates to effective policies Select cached user, group, role and session policies without lazy IAM reads. Keep alternative tag values and Deny keys when combining candidates, and bound intermediate combinations as well as authorization attempts. Cover unrelated cached policies, intersecting tag conditions, temporary credentials, root-derived credentials and cache-only lookup regressions for issue #293. Signed-off-by: Feng Ruohang --- cmd/auth-handler-unread-tags.go | 292 +++++++++++++++++---------- cmd/auth-handler-unread-tags_test.go | 144 ++++++++++++- 2 files changed, 325 insertions(+), 111 deletions(-) diff --git a/cmd/auth-handler-unread-tags.go b/cmd/auth-handler-unread-tags.go index d595d2bf2..a2db1bc8b 100644 --- a/cmd/auth-handler-unread-tags.go +++ b/cmd/auth-handler-unread-tags.go @@ -27,22 +27,24 @@ import ( "github.com/minio/minio-go/v7/pkg/set" "github.com/minio/minio-go/v7/pkg/tags" + "github.com/minio/minio/internal/arn" "github.com/minio/minio/internal/auth" "github.com/minio/minio/internal/logger" "github.com/pgsty/silo-pkg/v3/policy" "github.com/pgsty/silo-pkg/v3/policy/condition" + "github.com/puzpuzpuz/xsync/v3" ) -// authorizeRequestWithUnreadTags authorizes action on an object whose stored -// tags could not be read because reading the object failed with readErr. +// authorizeRequestWithUnreadTags decides whether to disclose readErr for an +// object whose stored tags could not be read. It must not grant object access. // // Without the tags, an Allow statement that tests s3:ExistingObjectTag cannot // match, so a caller allowed only by such a statement would get AccessDenied, // which clients do not retry, for a transient server error such as lost read -// quorum. For a server error the request is therefore also authorized as if -// the object carried tags that the policies ask for: a caller that some tags -// let through gets the error, a caller that no tags let through still gets -// AccessDenied. For any other error, NoSuchKey among them, the tags stay empty. +// quorum. For a server error, try a bounded set of tags from the caller's +// policies and return the storage error if normal authorization accepts one. +// If no candidate passes, keep AccessDenied. This is not an exhaustive policy +// solver. For any other error, NoSuchKey among them, the tags stay empty. func authorizeRequestWithUnreadTags(ctx context.Context, r *http.Request, action policy.Action, readErr error) APIErrorCode { s3Err := authorizeRequest(ctx, r, action) if s3Err == ErrNone || toAPIError(ctx, readErr).HTTPStatusCode < http.StatusInternalServerError { @@ -76,17 +78,16 @@ func policyExistingObjectTags(ctx context.Context, r *http.Request, action polic bucket, object := reqInfo.BucketName, reqInfo.ObjectName var ( allows [][]map[string]string // per policy set, the tags its Allow statements ask for - denies []map[string]string // tags that keep a Deny statement from matching + denies [][]map[string]string // per Deny statement, alternative tags that keep it from matching ) - // add files the tags of a statement that applies to action on the + // add collects the tags of a statement that applies to action on the // object, whatever its conditions. add := func(effect policy.Effect, conditions condition.Functions, values map[string][]string, fromSet *[]map[string]string) { - if tagMap, ok := existingObjectTagsFor(effect, conditions, values); ok { - if effect == policy.Allow { - *fromSet = append(*fromSet, tagMap) - } else { - denies = append(denies, tagMap) - } + candidates := existingObjectTagsCandidatesFor(effect, conditions, values) + if effect == policy.Allow { + *fromSet = append(*fromSet, candidates...) + } else if len(candidates) > 0 { + denies = append(denies, candidates) } } switch { @@ -107,17 +108,7 @@ func policyExistingObjectTags(ctx context.Context, r *http.Request, action polic allows = append(allows, fromSet) case newGlobalAuthZPluginFn() == nil && globalIAMSys.Initialized(): values := getConditionValues(r, "", reqInfo.Cred) - // From the cache only: reading the drives may be what just failed. - docs, _ := globalIAMSys.store.listPolicyDocs(ctx, "") - policySets := [][]policy.Policy{nil} - for _, name := range slices.Sorted(maps.Keys(docs)) { - policySets[0] = append(policySets[0], docs[name].Policy) - } - if sp, ok := reqInfo.Cred.Claims[sessionPolicyNameExtracted].(string); ok { - if p, err := policy.ParseConfig(strings.NewReader(sp)); err == nil { - policySets = append(policySets, []policy.Policy{*p}) - } - } + policySets := globalIAMSys.unreadTagPolicySets(reqInfo.Cred) for _, policies := range policySets { var fromSet []map[string]string for _, p := range policies { @@ -132,72 +123,131 @@ func policyExistingObjectTags(ctx context.Context, r *http.Request, action polic allows = append(allows, fromSet) } } - return combineExistingObjectTags(allows, denies) + return combineExistingObjectTags(append(allows, denies...)) } -// combineExistingObjectTags merges the tags of one Allow statement of each -// policy set, or of none for a set that may allow without tags, and tries each -// result also with the tags that keep the Deny statements from matching. The -// first tags win a conflict; the authorization of the result settles it. -func combineExistingObjectTags(allows [][]map[string]string, denies []map[string]string) []string { - merge := func(parts ...map[string]string) map[string]string { - merged := make(map[string]string) - for _, part := range parts { - for k, v := range part { - if _, ok := merged[k]; !ok { - merged[k] = v +// unreadTagMappings reuses the normal user/group mapping lookup without lazy +// storage reads. Missing cache entries contribute no candidates; the regular +// authorization still decides whether any candidate is allowed. +type unreadTagMappings struct { + *IAMStoreSys +} + +func (unreadTagMappings) loadMappedPolicy(context.Context, string, IAMUserType, bool, *xsync.MapOf[string, MappedPolicy]) error { + return errNoSuchPolicy +} + +func (unreadTagMappings) loadGroup(context.Context, string, map[string]GroupInfo) error { + return errNoSuchGroup +} + +// unreadTagPolicySets selects only this credential's cached policies. Base +// and session policies remain separate because both must allow the request. +// This only supplies candidates, so identity, claims and policy validity are +// left to the normal authorization on each candidate. +func (sys *IAMSys) unreadTagPolicySets(cred auth.Credentials) [][]policy.Policy { + derived := cred.IsTemp() || cred.IsServiceAccount() + name := cred.AccessKey + if derived { + name = cred.ParentUser + } + cache := sys.store.rlock() + defer sys.store.runlock() + var names []string + roleARN := (policy.Args{Claims: cred.Claims}).GetRoleArn() + switch { + case name == globalActiveCred.AccessKey: + case derived && roleARN != "": + parsed, err := arn.Parse(roleARN) + if err != nil { + return nil + } + names = newMappedPolicy(sys.rolesMap[parsed]).toSlice() + default: + loader := unreadTagMappings{sys.store} + var err error + names, _, err = cache.policyDBGet(loader, name, false, false) + if err != nil { + return nil + } + groups, err := cache.policyDBGetGroups(loader, len(names) > 0, cred.Groups...) + if err != nil { + return nil + } + names = append(names, groups...) + if derived && len(names) == 0 { + claims, _ := policy.GetPoliciesFromClaims(cred.Claims, iamPolicyClaimNameOpenID()) + names = claims.ToSlice() + } + } + + sets := [][]policy.Policy{nil} + slices.Sort(names) + for _, name := range slices.Compact(names) { + if doc, ok := cache.iamPolicyDocsMap[name]; ok { + sets[0] = append(sets[0], doc.Policy) + } + } + if derived && (cred.IsTemp() || cred.Claims[iamPolicyClaimNameSA()] != inheritedPolicyType) { + if sp, ok := cred.Claims[sessionPolicyNameExtracted].(string); ok { + if p, err := policy.ParseConfig(strings.NewReader(sp)); err == nil { + sets = append(sets, []policy.Policy{*p}) + } + } + } + return sets +} + +// combineExistingObjectTags combines alternatives from each base/session +// Allow set and each tag-dependent Deny. A set may also contribute no tags: +// another statement or another set may already supply what it needs. First +// values win conflicts; the normal authorization decides whether they work. +func combineExistingObjectTags(sets [][]map[string]string) []string { + combos := []map[string]string{{}} + for _, fromSet := range sets { + fromSet = append(uniqueExistingObjectTagMaps(fromSet), nil) + var next []map[string]string + seen := set.NewStringSet() + combinations: + for _, combo := range combos { + for _, addition := range fromSet { + merged := maps.Clone(combo) + for k, v := range addition { + if _, ok := merged[k]; !ok { + merged[k] = v + } + } + t, err := tags.MapToObjectTags(merged) + if err != nil || seen.Contains(t.String()) { + continue + } + seen.Add(t.String()) + next = append(next, merged) + if len(next) == maxExistingTagCandidates { + break combinations } } } - return merged + combos = next } - combos := [][]map[string]string{nil} - for _, fromSet := range allows { - // Many statements ask for the same tags. - seen := set.NewStringSet() - fromSet = slices.DeleteFunc(slices.Clone(fromSet), func(tagMap map[string]string) bool { - t, err := tags.MapToObjectTags(tagMap) - if err != nil || seen.Contains(t.String()) { - return true - } - seen.Add(t.String()) - return false - }) - next := make([][]map[string]string, 0, len(combos)*(len(fromSet)+1)) - for _, combo := range combos { - next = append(next, combo) - for _, tagMap := range fromSet { - next = append(next, append(slices.Clip(combo), tagMap)) - } - } - combos = next[:min(len(next), maxExistingTagCandidates)] - } - deny := merge(denies...) - found := set.NewStringSet() var candidates []string for _, combo := range combos { - for _, tagMap := range []map[string]string{merge(combo...), merge(append(slices.Clip(combo), deny)...)} { - t, err := tags.MapToObjectTags(tagMap) - if len(tagMap) == 0 || err != nil || found.Contains(t.String()) { - continue - } - found.Add(t.String()) - if candidates = append(candidates, t.String()); len(candidates) == maxExistingTagCandidates { - return candidates - } + if len(combo) != 0 { // empty tags were already authorized + t, _ := tags.MapToObjectTags(combo) + candidates = append(candidates, t.String()) } } return candidates } -// existingObjectTagsFor returns tags under which the s3:ExistingObjectTag tests -// of conditions all pass, for an Allow statement, or one of them fails, for a -// Deny statement; a tag left out is absent. It returns false if the conditions -// test no tag, or if no such tags were found among those tried for each tag: -// none, the values the tests name, with policy variables taken from values and -// wildcards filled, and the fillers "" and "_". The policy engine itself -// evaluates the tests for each of them. -func existingObjectTagsFor(effect policy.Effect, conditions condition.Functions, values map[string][]string) (map[string]string, bool) { +// existingObjectTagsCandidatesFor returns tag sets under which the +// s3:ExistingObjectTag tests of conditions all pass, for an Allow statement, +// or one of them fails, for a Deny statement; a tag left out is absent. For an +// Allow, values for different tag keys are combined, while a Deny needs only +// one failing key. It tries the values named in the conditions, with policy +// variables resolved, wildcard fillers, and absence, then lets the policy +// engine decide whether each candidate really satisfies the conditions. +func existingObjectTagsCandidatesFor(effect policy.Effect, conditions condition.Functions, values map[string][]string) []map[string]string { // Each function tests one key, so the tests of one tag pass or fail // whatever the other tags are. byKey := make(map[string]condition.Functions) @@ -209,47 +259,74 @@ func existingObjectTagsFor(effect policy.Effect, conditions condition.Functions, } } if len(byKey) == 0 { - return nil, false - } - tagMap := make(map[string]string) - for _, key := range slices.Sorted(maps.Keys(byKey)) { - tagKey := strings.TrimPrefix(key, condition.ExistingObjectTag.Name()+"/") - value, present, found := existingObjectTagValue(key, tagKey, byKey[key], values, effect == policy.Allow) - switch { - case effect == policy.Deny && found: - if present { - tagMap[tagKey] = value - } - return tagMap, true - case effect == policy.Allow && !found: - return nil, false - case present: - tagMap[tagKey] = value - } + return nil } + + keys := slices.Sorted(maps.Keys(byKey)) if effect == policy.Deny { - return nil, false + var candidates []map[string]string + for _, key := range keys { + candidates = append(candidates, existingObjectTagValues(key, strings.TrimPrefix(key, condition.ExistingObjectTag.Name()+"/"), byKey[key], values, false)...) + } + return uniqueExistingObjectTagMaps(candidates) } - return tagMap, true + + candidates := []map[string]string{{}} + for _, key := range keys { + keyCandidates := existingObjectTagValues(key, strings.TrimPrefix(key, condition.ExistingObjectTag.Name()+"/"), byKey[key], values, true) + if len(keyCandidates) == 0 { + return nil + } + next := make([]map[string]string, 0, min(maxExistingTagCandidates, len(candidates)*len(keyCandidates))) + for _, base := range candidates { + for _, keyCandidate := range keyCandidates { + merged := maps.Clone(base) + maps.Copy(merged, keyCandidate) + next = append(next, merged) + if len(next) == maxExistingTagCandidates { + break + } + } + if len(next) == maxExistingTagCandidates { + break + } + } + candidates = next + } + return uniqueExistingObjectTagMaps(candidates) } -// existingObjectTagValue looks for a value of tag tagKey, or its absence, -// under which tests, the functions that test the condition key key, evaluate -// to want. -func existingObjectTagValue(key, tagKey string, tests condition.Functions, values map[string][]string, want bool) (value string, present, found bool) { +func uniqueExistingObjectTagMaps(candidates []map[string]string) []map[string]string { + seen := set.NewStringSet() + unique := make([]map[string]string, 0, len(candidates)) + for _, candidate := range candidates { + t, err := tags.MapToObjectTags(candidate) + if err != nil || seen.Contains(t.String()) { + continue + } + seen.Add(t.String()) + unique = append(unique, candidate) + } + return unique +} + +// existingObjectTagValues looks for values of tag tagKey, or its absence, +// under which the functions that test the condition key key evaluate to want. +func existingObjectTagValues(key, tagKey string, tests condition.Functions, values map[string][]string, want bool) []map[string]string { defer delete(values, key) delete(values, key) + var candidates []map[string]string if tests.Evaluate(values) == want { - return "", false, true + candidates = append(candidates, map[string]string{}) } // The functions keep their values unexported; JSON exposes them. buf, err := json.Marshal(tests) if err != nil { - return "", false, false + return candidates } var byName map[string]map[string]condition.ValueSet if err = json.Unmarshal(buf, &byName); err != nil { - return "", false, false + return candidates } var tried []string for name, byKey := range byName { @@ -270,10 +347,13 @@ func existingObjectTagValue(key, tagKey string, tests condition.Functions, value } values[key] = []string{v} if tests.Evaluate(values) == want { - return v, true, true + candidates = append(candidates, map[string]string{tagKey: v}) + if len(candidates) == maxExistingTagCandidates { + break + } } } - return "", false, false + return uniqueExistingObjectTagMaps(candidates) } // substituteConditionVariables resolves the policy variables in v from the diff --git a/cmd/auth-handler-unread-tags_test.go b/cmd/auth-handler-unread-tags_test.go index a42b04574..aa3f1e822 100644 --- a/cmd/auth-handler-unread-tags_test.go +++ b/cmd/auth-handler-unread-tags_test.go @@ -20,6 +20,7 @@ package cmd import ( "bytes" "context" + "encoding/base64" "encoding/json" "encoding/xml" "fmt" @@ -29,8 +30,10 @@ import ( "net/url" "strings" "testing" + "time" "github.com/minio/minio-go/v7/pkg/tags" + "github.com/minio/minio/internal/arn" "github.com/minio/minio/internal/auth" xhttp "github.com/minio/minio/internal/http" "github.com/pgsty/silo-pkg/v3/policy" @@ -108,6 +111,45 @@ func TestExistingObjectTagAuthReadQuorumLost(t *testing.T) { } return cred } + newTempUser := func(parent auth.Credentials, statements string) auth.Credentials { + session := `{"Version": "2012-10-17", "Statement": [` + statements + `]}` + secret, err := getTokenSigningKey() + if err != nil { + t.Fatal(err) + } + cred, err := auth.GetNewCredentialsWithMetadata(map[string]any{ + parentClaim: parent.AccessKey, "exp": UTCNow().Add(time.Hour).Unix(), + policy.SessionPolicyName: base64.StdEncoding.EncodeToString([]byte(session)), + }, secret) + if err != nil { + t.Fatal(err) + } + cred.ParentUser = parent.AccessKey + if _, err := globalIAMSys.SetTempUser(ctx, cred.AccessKey, cred, ""); err != nil { + t.Fatal(err) + } + return cred + } + // An unrelated cached policy must not consume the candidate limit. The + // target user's policy is created below and sorts after this policy. + fillerStatements := make([]string, 0, maxExistingTagCandidates) + for i := range maxExistingTagCandidates { + fillerStatements = append(fillerStatements, statement("Allow", "*", + fmt.Sprintf(`{"StringEquals": {"s3:ExistingObjectTag/fill%02d": "v"}}`, i))) + } + fillerPolicy, err := policy.ParseConfig(strings.NewReader(`{"Version": "2012-10-17", "Statement": [` + strings.Join(fillerStatements, ",") + `]}`)) + if err != nil { + t.Fatal(err) + } + fillerPolicyName := "000-unread-tags-filler-" + mustGetUUID() + if _, err = globalIAMSys.SetPolicy(ctx, fillerPolicyName, *fillerPolicy); err != nil { + t.Fatal(err) + } + defer func() { + if err := globalIAMSys.DeletePolicy(ctx, fillerPolicyName, false); err != nil { + t.Logf("delete filler policy: %v", err) + } + }() type user struct { name string @@ -146,6 +188,35 @@ func TestExistingObjectTagAuthReadQuorumLost(t *testing.T) { // asks for j=w: only an object carrying both lets it through. allowedByTags("serviceAccount", newServiceAccount(newUser(tagStatement("Allow", "*", "v")), statement("Allow", "*", `{"StringEquals": {"s3:ExistingObjectTag/j": "w"}}`))), + allowedByTags("temporary", newTempUser(newUser(tagStatement("Allow", "*", "v")), + statement("Allow", "*", `{"StringEquals": {"s3:ExistingObjectTag/j": "w"}}`))), + allowedByTags("rootTemporary", newTempUser(globalActiveCred, tagStatement("Allow", "*", "v"))), + allowedByTags("rootServiceAccount", newServiceAccount(globalActiveCred, tagStatement("Allow", "*", "v"))), + // The effective policy must be selected before applying the candidate + // limit; the unrelated cached policy above has 64 other tag statements. + allowedByTags("unrelatedPolicies", newUser(statement("Allow", "*", + `{"StringEquals": {"s3:ExistingObjectTag/target": "ok"}}`))), + // An alternative allowed value must survive a Deny that excludes the + // first value. With unread tags, k=v is the only valid candidate. + allowedByTags("alternativeAllowedValue", newUser( + statement("Allow", "*", `{"StringEquals": {"s3:ExistingObjectTag/k": ["a", "v"]}}`), + tagStatement("Deny", "*", "a"))), + allowedByTags("alternativeValuesAcrossKeys", newUser( + statement("Allow", "*", `{"StringEquals": {"s3:ExistingObjectTag/k": ["a", "v"], "s3:ExistingObjectTag/j": ["b", "w"]}}`), + tagStatement("Deny", "*", "a"), + statement("Deny", "*", `{"StringEquals": {"s3:ExistingObjectTag/j": "b"}}`))), + // Both Deny statements must be avoided. The first candidate (k="") + // avoids the Null Deny but still matches the second Deny. + allowedByTags("multipleDenies", newUser(statement("Allow", "*", ""), + statement("Deny", "*", `{"Null": {"s3:ExistingObjectTag/k": "true"}}`), + tagStatement("Deny", "*", ""))), + // The Deny can be avoided by k=v OR j=w. Only j=w is compatible + // with this Allow, so stopping at the first Deny key loses it. + { + "alternativeDenyKey", newUser(tagStatement("Allow", "*", "a"), + statement("Deny", "*", `{"StringNotEquals": {"s3:ExistingObjectTag/k": "v", "s3:ExistingObjectTag/j": "w"}}`)), + http.StatusForbidden, http.StatusServiceUnavailable, + }, // Allowed only for k=w: the decision still depends on the tags. {"otherValue", newUser(tagStatement("Allow", "*", "w")), http.StatusForbidden, http.StatusServiceUnavailable}, @@ -170,7 +241,7 @@ func TestExistingObjectTagAuthReadQuorumLost(t *testing.T) { const object, missing = "tagged", "missing" if _, err := z.PutObject(ctx, bucket, object, mustGetPutObjReader(t, bytes.NewReader([]byte("data")), 4, "", ""), - ObjectOptions{UserDefined: map[string]string{xhttp.AmzObjectTagging: "k=v&j=w&np=1&ne=1&fa=1&du=1&owner=" + owner.AccessKey}}); err != nil { + ObjectOptions{UserDefined: map[string]string{xhttp.AmzObjectTagging: "k=v&j=w&np=1&ne=1&fa=1&du=1&target=ok&owner=" + owner.AccessKey}}); err != nil { t.Fatal(err) } // Anonymous reads are allowed by the bucket policy while the object carries k=v. @@ -207,7 +278,14 @@ func TestExistingObjectTagAuthReadQuorumLost(t *testing.T) { if req.body != nil { body = bytes.NewReader(req.body) } - signed, err := newTestSignedRequestV4(req.method, req.target, int64(len(req.body)), body, creds.AccessKey, creds.SecretKey, req.header) + header := make(map[string]string) + for k, v := range req.header { + header[k] = v + } + if creds.IsTemp() { + header[xhttp.AmzSecurityToken] = creds.SessionToken + } + signed, err := newTestSignedRequestV4(req.method, req.target, int64(len(req.body)), body, creds.AccessKey, creds.SecretKey, header) if err != nil { t.Fatal(err) } @@ -289,10 +367,11 @@ func TestExistingObjectTagsFor(t *testing.T) { t.Fatalf("%s: %v", tc.conditions, err) } } - tagMap, ok := existingObjectTagsFor(tc.effect, conditions, values) + candidates := existingObjectTagsCandidatesFor(tc.effect, conditions, values) + ok := len(candidates) > 0 got := "" - if len(tagMap) > 0 { - encoded, err := tags.MapToObjectTags(tagMap) + if ok { + encoded, err := tags.MapToObjectTags(candidates[0]) if err != nil { t.Fatalf("%s: %v", tc.conditions, err) } @@ -303,3 +382,58 @@ func TestExistingObjectTagsFor(t *testing.T) { } } } + +// Group-only and external identities need mapping lookups that normally +// fall back to storage. Candidate collection must stay within the cache even +// for those identities, while still finding their group/claim/role policy. +func TestUnreadTagPolicySetsCacheOnly(t *testing.T) { + ctx, sys, _ := prepareIAMRevisionFixture(t) + const policyName = "unread-tag-policy" + batchAuthzPolicy(ctx, t, sys, policyName, `{"Effect":"Allow","Action":["s3:GetObject"],"Resource":["arn:aws:s3:::bucket/*"],"Condition":{"StringEquals":{"s3:ExistingObjectTag/k":"v"}}}`) + member := iamAdminUser(ctx, t, sys, "unread-group-member", "") + _, err := sys.AddUsersToGroup(ctx, "unread-group", []string{member.AccessKey}) + mustIAM(t, err) + _, err = sys.PolicyDBSet(ctx, "unread-group", policyName, regUser, true) + mustIAM(t, err) + role, err := arn.NewIAMRoleARN("unread-role", "") + mustIAM(t, err) + sys.rolesMap = map[arn.ARN]string{role: policyName} + claimUser := auth.Credentials{ + AccessKey: "unread-sts", ParentUser: "external-parent", SessionToken: "token", + Expiration: UTCNow().Add(time.Hour), Claims: map[string]any{iamPolicyClaimNameOpenID(): policyName}, + } + roleUser := claimUser + roleUser.Claims = map[string]any{roleArnClaim: role.String()} + original := sys.store.IAMStorageAPI + defer func() { sys.store.IAMStorageAPI = original }() + for _, tc := range []struct { + name string + cred auth.Credentials + }{ + {"group", member}, + {"claims", claimUser}, + {"role", roleUser}, + } { + t.Run(tc.name, func(t *testing.T) { + name := tc.cred.AccessKey + if tc.cred.ParentUser != "" { + name = tc.cred.ParentUser + } + probe := &batchBlockedIAMStore{ + IAMStorageAPI: original, mapping: name, doc: policyName, + started: make(chan struct{}), release: make(chan struct{}), + } + close(probe.release) // report an accidental read without blocking the test + sys.store.IAMStorageAPI = probe + sets := sys.unreadTagPolicySets(tc.cred) + select { + case <-probe.started: + t.Error("candidate collection attempted a storage read") + default: + } + if len(sets) != 1 || len(sets[0]) != 1 { + t.Fatalf("expected the cached policy through %s, got %v", tc.name, sets) + } + }) + } +} From 322cd8d2c1ac0574aae9534473e58620ea3666e2 Mon Sep 17 00:00:00 2001 From: Feng Ruohang Date: Wed, 11 Nov 2026 00:08:07 +0800 Subject: [PATCH 6/6] fix(decom): count lifecycle removals as scan progress Reuse the source-version lookup to count versions removed by ILM before migration cleanup so nested keys are discovered on the next pass. Keep delete-marker-only expiry from reporting physical progress, and cover both the decommission regression and the removal-count boundary. Signed-off-by: Feng Ruohang --- cmd/erasure-server-pool-consistency.go | 13 +++++- ...asure-server-pool-decom-keys-below_test.go | 38 +++++++++++++++++- cmd/erasure-server-pool-migration-ilm_test.go | 40 +++++++++++++++++++ 3 files changed, 88 insertions(+), 3 deletions(-) diff --git a/cmd/erasure-server-pool-consistency.go b/cmd/erasure-server-pool-consistency.go index 79ba0f01c..1d1414023 100644 --- a/cmd/erasure-server-pool-consistency.go +++ b/cmd/erasure-server-pool-consistency.go @@ -272,7 +272,7 @@ func (z *erasureServerPools) applyMigrationExpiry(ctx context.Context, bucket, o // owns replication state and tier cleanup, and it re-evaluates the current // lifecycle rule. A migration cleanup must never turn an asynchronous ILM // decision into an unconditional physical purge. It returns how many source -// versions it deleted. +// versions were removed, including versions deleted by ILM before cleanup. func (z *erasureServerPools) removeMovedVersions(ctx context.Context, bucket, object string, source int, versions []FileInfo, expired []migrationExpiredVersion) (removed int, err error) { if err := z.applyMigrationExpiry(ctx, bucket, object, source, expired); err != nil { return 0, err @@ -290,8 +290,14 @@ func (z *erasureServerPools) removeMovedVersions(ctx context.Context, bucket, ob if vid == "" { vid = nullVersionID } + expiredVersion := isExpiredVersion(expired, version) copies, err := z.objectPoolInfos(ctx, bucket, object, ObjectOptions{VersionID: vid, NoAuditLog: true}) if isErrObjectNotFound(err) || isErrVersionNotFound(err) { + // ILM can remove the parent key before cleanup. Count that as + // progress so the next walk discovers keys below it. + if expiredVersion { + removed++ + } continue } if err != nil { @@ -307,12 +313,15 @@ func (z *erasureServerPools) removeMovedVersions(ctx context.Context, bucket, ob } } if src == nil { + if expiredVersion { + removed++ + } continue } if !src.ModTime.Equal(version.ModTime) || src.DeleteMarker != version.Deleted { return removed, PreConditionFailed{} } - if isExpiredVersion(expired, version) { + if expiredVersion { // The logical expiry path owns this version. Keeping the source is // also the retry point if that path failed or the pool check races it. continue diff --git a/cmd/erasure-server-pool-decom-keys-below_test.go b/cmd/erasure-server-pool-decom-keys-below_test.go index 7435bd6a4..1538511a0 100644 --- a/cmd/erasure-server-pool-decom-keys-below_test.go +++ b/cmd/erasure-server-pool-decom-keys-below_test.go @@ -17,7 +17,10 @@ package cmd -import "testing" +import ( + "testing" + "time" +) // A key below another key ("k/b" under "k") is a directory inside the // directory of that key. The listing treats the directory of an object as a @@ -64,3 +67,36 @@ func TestDecommissionMovesKeysBelowAKey(t *testing.T) { } } } + +// An ILM expiry can remove the parent key during a decommission walk. That +// removal must count as progress, or the next key below the parent remains +// hidden from the listing and the source pool is left non-empty. +func TestDecommissionContinuesAfterLifecycleRemovesParent(t *testing.T) { + z, _ := migrationTestPools(t) + const bucket = "decommission-ilm-keys-below" + if err := z.MakeBucket(t.Context(), bucket, MakeBucketOptions{}); err != nil { + t.Fatal(err) + } + meta, err := globalBucketMetadataSys.Get(bucket) + if err != nil { + t.Fatal(err) + } + meta.LifecycleConfigXML = []byte(`expire-oldEnabled1`) + if err := meta.Save(t.Context(), z); err != nil { + t.Fatal(err) + } + globalBucketMetadataSys.Set(bucket, meta) + + putConsistencyObject(t, z, bucket, "k", 0, "parent", ObjectOptions{MTime: UTCNow().Add(-72 * time.Hour)}) + putConsistencyObject(t, z, bucket, "k/b", 0, "child", ObjectOptions{MTime: UTCNow()}) + + if info := keysBelowDecommission(t, z); !info.Complete || info.Failed { + t.Fatalf("decommission: complete %v, failed %v; want complete", info.Complete, info.Failed) + } + if _, err := z.serverPools[0].GetObjectInfo(t.Context(), bucket, "k/b", ObjectOptions{}); !isErrObjectNotFound(err) { + t.Fatalf("child still in source pool: %v", err) + } + if body := migrationTestBody(t, z.GetObjectNInfo, bucket, "k/b", ""); body != "child" { + t.Fatalf("child reads %q", body) + } +} diff --git a/cmd/erasure-server-pool-migration-ilm_test.go b/cmd/erasure-server-pool-migration-ilm_test.go index da14d2df9..ddd049611 100644 --- a/cmd/erasure-server-pool-migration-ilm_test.go +++ b/cmd/erasure-server-pool-migration-ilm_test.go @@ -414,6 +414,46 @@ func testMigrationLifecycleExpiry(t *testing.T, kind string) { }) } +// Physical expiry advances the walk; creating a delete marker does not remove +// the addressed version and must not make a walk repeat without progress. +func TestPoolsMigrationLifecycleRemovalProgress(t *testing.T) { + for _, versioned := range []bool{false, true} { + status := versioning.State("") + name, wantRemoved := "physical-delete", 1 + if versioned { + status = versioning.Enabled + name, wantRemoved = "delete-marker-only", 0 + } + t.Run(name, func(t *testing.T) { + m := newMigrationILM(t, status, false, migrationILMExpireCurrent()) + oi := m.put("obj-01", "v1", ObjectOptions{Versioned: versioned, MTime: UTCNow().Add(-72 * time.Hour)}) + _, updatedAt, err := globalBucketMetadataSys.GetLifecycleConfig(m.bucket) + if err != nil { + t.Fatal(err) + } + version := FileInfo{Name: oi.Name, VersionID: oi.VersionID, ModTime: oi.ModTime} + removed, err := m.z.removeMovedVersions(t.Context(), m.bucket, oi.Name, 0, []FileInfo{version}, []migrationExpiredVersion{{ + version: version, + event: lifecycle.Event{Action: lifecycle.DeleteAction}, + lifecycleUpdatedAt: updatedAt, + source: lcEventSrc_Decom, + }}) + if err != nil || removed != wantRemoved { + t.Fatalf("removed %d versions, err %v; want %d, nil", removed, err, wantRemoved) + } + if versioned { + if body := migrationTestBody(t, m.z.GetObjectNInfo, m.bucket, oi.Name, oi.VersionID); body != "obj-01 v1" { + t.Fatalf("noncurrent version reads %q", body) + } + } + current, err := m.z.GetObjectInfo(t.Context(), m.bucket, oi.Name, ObjectOptions{}) + if !isErrObjectNotFound(err) || current.DeleteMarker != versioned { + t.Fatalf("after expiry: delete marker %t, err %v", current.DeleteMarker, err) + } + }) + } +} + // A lifecycle action found during the migration walk must not be applied // after the bucket's lifecycle configuration changes. The source copy stays // available for a fresh scan under the new policy.