From 84653da8c68dee3a2406b7d0b56e94ea64de7f5d Mon Sep 17 00:00:00 2001 From: Klaus Post Date: Thu, 20 Aug 2026 15:51:59 +0200 Subject: [PATCH 1/6] Fix policy validation & improve speed MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit PR 1 — security ## fix(policy,wildcard): bound wildcard matching and close two Deny-dropping paths Three independent correctness bugs on the authorization path. No API changes. ### 1. `wildcard.Match` was exponential in the number of `*` `deepMatchRune` recursed on both alternatives at every `*`, so its cost grew exponentially with the star count. The pattern is caller-supplied — statement validation classifies every action against every namespace, and `Resource.Match` runs untrusted patterns — so a tiny document could consume unbounded CPU: | input | before | after | | --------------------------------------------------------- | ------- | ------ | | `ParseConfig`, action `"*********x"` | 59.3 s | < 1 ms | | `ParseBucketPolicyConfig`, same action | 18.8 s | < 1 ms | | `AdminAction.IsValid`, 8 stars | 4.5 s | < 1 ms | Scaling was ~4x per added star: 6 stars 165 ms, 8 stars 4.5 s, 10 stars > 100 s. `MatchSimple` was equally affected, and it backs bucket-policy principal matching and CORS origin matching. Replaced with the standard greedy two-pointer matcher: one backtrack point, no recursion, no allocation, O(n*m) worst case. `MatchSimple`'s documented "a trailing `?` is optional" behaviour does not survive greedy backtracking — a first attempt that folded it into the matcher diverged on `MatchSimple("*?*a", "a")`. It is now expressed explicitly in `MatchSimple`: if the pattern does not match outright, any prefix of it ending at a `?` that consumes the whole name also matches. The matcher itself is now plain glob semantics with no mode flag. ### 2. `Statement.hash` omitted `NotResources` `dropDuplicateStatements` switches to hashing above 10 statements. Two statements differing *only* in `NotResource` hashed identically, so one was discarded at parse time. When those are `Deny` statements, a restriction silently disappears from a policy that was written correctly. Reproduced: an 11-statement policy carrying two distinct `Deny ... NotResource` statements loses one; the same policy with 4 statements (below the threshold, exact-comparison path) keeps both. ### 3. `HasDenyStatement` trusted a field that struct-literal policies never set `hasDeny` is only assigned in `updateActionIndex`, which runs from `UnmarshalJSON` and `MergePolicies`. A `Policy` built as a struct literal never reaches it — including this package's own `readonly` and `consolereadonly` defaults in `constants.go`, both of which carry `Deny admin:CreateUser`. `HasDenyStatement()` reported `false` for them. That matters to callers that use the answer to route policies: AIStor sorts them into a "no deny statements" bucket, evaluates that bucket with an early return on the first `Allow`, and never reaches the `Deny`. A principal holding `readonly` plus any policy allowing `admin:CreateUser` was granted `admin:CreateUser`. `HasDenyStatement` now falls back to scanning statements. It runs once per policy per merge, not per request. ### Compatibility Fix 2 changes which statements survive parsing. A policy that was accidentally relying on the collapse regains a `Deny`, so a request that previously succeeded may start returning 403. That is the intended direction, but it belongs in release notes. Fixes 1 and 3 are behaviour-preserving for any correctly-built input. ### Testing - `wildcard/match_equivalence_test.go` keeps the previous recursive matcher as a reference implementation and diffs against it: 254,512 exhaustive pattern/name/mode combinations (patterns over `{a,b,*,?}` to length 5, `{a,:,/}` names) plus a differential fuzz target. 101M fuzz executions, no divergence. - `TestMatchStarsAreLinear` fails if a single `Match` with 8/16/64/256 stars, or 32 interleaved `*a` pairs, exceeds 50 ms. - `policy/parse_dos_test.go` asserts `ParseConfig` and `ParseBucketPolicyConfig` stay under 2 s for star-heavy actions. - `TestDropDuplicateStatementsKeepsNotResources` covers both the exact-comparison and hashed paths. - `TestHasDenyStatementOnStructLiteralPolicy` walks the built-in defaults. `go test ./...` is green. PR 2 — performance ## perf(policy): cut authorization cost for large policy collections A production profile of a 24-node cluster found 61–73% of all CPU on client-facing nodes inside `Policy.Decide`, and 51–61% in `AdminAction.IsValid` alone. The credential resolved to more than 4000 policies. Six changes, no API changes, no behaviour changes. ### `AdminAction.IsValid` / `HasResource` — O(1) fast path Both scanned their whole action map calling `wildcard.Match` per entry, with the receiver as the pattern. `Statement.isAdmin` calls `IsValid` for every action of every statement it evaluates, so a pure-S3 statement paid `len(Actions) x 101` wildcard matches to learn nothing. Now: exact map lookup; then bail if the pattern holds no metacharacter (a literal can only match by being in the map); then bail if the literal head before the first metacharacter is prefix-incompatible with `admin:`. | | before | after | | ---------------------- | ------- | ------- | | `isAdmin`, 1 action | 690 ns | 34.9 ns | | `isAdmin`, 3 actions | 2012 ns | 63.9 ns | The head comparison is bidirectional on purpose: `adm*` and `a*d*m*i*n*` are both valid admin patterns, so a plain `strings.HasPrefix(head, "admin:")` would be a silent authorization change. ### `actionStatementIndex` was a pessimisation `Decide` consulted the index and then — hit or miss — fell through to an unconditional walk of every statement, re-evaluating everything the index had just evaluated. The index only ever helped when an indexed statement allowed. The walk now skips the positions the index already tried (`indexes` is ascending, and `isAllowedFor` is pure). ### One classification pass instead of five, cached at parse time `IsAllowedPtr` called `isTable`, `isKMS`, `isSTS`, `isAdmin` and `hasAdminResource`, each ranging over the action set separately — four map-iterator setups per statement per authorization. Collapsed into one `classify()` pass, and cached on the statement by `updateActionIndex`. The cached field's zero value means "not computed — derive it now", not "no namespace". A statement built as a struct literal, or cloned, is therefore slower rather than wrong, and `Statement.Clone` deliberately does not copy it. `classify` is a pure function of `Statement.Actions`, which nothing mutates after parse — the same invariant `actionStatementIndex` already relies on. ### The request resource string is built once per request `IsAllowedPtr` rendered `BucketName`/`ObjectName` into a pooled buffer and then called `Buffer.String()` — which copies — once or twice *per statement*. A memory profile of one authorization over 156 policies of 20 statements put 92.5% of all allocations on that one line: 1561 allocations and 75 KB of garbage per authorization. It depends only on the request, so it is now memoized on `Args` and passed to every statement. `Args` carries the fields it was built from, so a caller reusing an `Args` for a second object cannot read a stale value. Note: one `*Args` must not be shared across goroutines within a single evaluation. `IsAllowedPar` did share one; it now gives each worker its own copy. ### Measured `go test ./policy/ -run XXX -bench 'BenchmarkIsAllowed|BenchmarkSerialEvalVsParEval' -benchmem`, on an idle 32-thread box: | benchmark | before | after | | | ------------------------------------ | -------- | -------- | ----- | | `IsAllowed/SingleStatementAllow` | 1811 ns | 127 ns | 14.3x | | `IsAllowed/DenyRule` | 2737 ns | 224 ns | 12.2x | | `IsAllowed/WildcardMatching` | 919 ns | 121 ns | 7.6x | | `IsAllowed/MultipleStatements` | 6054 ns | 4069 ns | 1.5x | | `SerialEvalVsParEval/128p` serial | 1.62 ms | 252 us | 6.4x | | `SerialEvalVsParEval/1024p` serial | 12.6 ms | 2.07 ms | 6.1x | `MultipleStatements` gains least because its statements have non-matching action sets, so `Actions.Match` short-circuits before classification runs. `SerialEvalVsParEval` builds policies as struct literals and never calls `updateActionIndex`, so it does not exercise the cached classification and understates the result — measured through `ParseConfig`, as a server loads policies, the same shape at 4000 policies goes from 66.1 ms to 660 us per authorization with allocations dropping from 88,888 to 1. Allocation caveat: benchmarks with short bucket+key strings previously stack-allocated the per-statement copy and report 0 -> 1 allocs/op. Anything with a realistic object key allocated once per statement before and once per request now. ### Testing - `policy/admin-action_test.go` (new): trap table for the fast path (`"*"` -> true, `"a*d*m*i*n*"` -> true, `"admin:heal"` -> false, `"s3:*"` -> false, ...), the same for `HasResource`, an equivalence check against the previous linear scan over 964 patterns, and a differential fuzz target. - `TestAdminActionNamespacePrefix` pins the invariant the fast path rests on — every `SupportedAdminActions` key starts with `admin:`. Nothing enforced this before, and the analogous `s3:` invariant is *already* broken by `s3express:CreateSession` in `SupportedActions`, so it can rot into an authorization change. - `TestDecideReachesDenyOnlyAndIsOwnerWithNoStatements`: `Decide`'s `DenyOnly` and `IsOwner` returns sit below the deny loop, so a policy with nothing to match must still reach them. An earlier draft of this change added an early return for the empty case and silently turned both from allow into deny. Any future work narrowing which statements `Decide` walks needs this test. --- policy/admin-action.go | 41 ++++++ policy/admin-action_test.go | 201 +++++++++++++++++++++++++++++ policy/parse_dos_test.go | 57 ++++++++ policy/policy.go | 63 +++++++-- policy/policy_regression_test.go | 111 ++++++++++++++++ policy/statement.go | 126 ++++++++++++++---- wildcard/match.go | 71 +++++++--- wildcard/match_equivalence_test.go | 165 +++++++++++++++++++++++ 8 files changed, 782 insertions(+), 53 deletions(-) create mode 100644 policy/admin-action_test.go create mode 100644 policy/parse_dos_test.go create mode 100644 policy/policy_regression_test.go create mode 100644 wildcard/match_equivalence_test.go diff --git a/policy/admin-action.go b/policy/admin-action.go index c7c013bd..8cd7a28e 100644 --- a/policy/admin-action.go +++ b/policy/admin-action.go @@ -18,6 +18,8 @@ package policy import ( + "strings" + "github.com/minio/pkg/v3/policy/condition" "github.com/minio/pkg/v3/wildcard" ) @@ -451,6 +453,13 @@ var AdminActionsWithResource = map[AdminAction]struct{}{ // HasResource reports whether this admin action operates on a bucket resource. func (action AdminAction) HasResource() bool { + if _, ok := AdminActionsWithResource[action]; ok { + return true + } + if !wildcard.Has(string(action)) { + // A literal action can only match by being in the set. + return false + } for a := range AdminActionsWithResource { if action.Match(a) { return true @@ -465,7 +474,25 @@ func (action AdminAction) Match(a AdminAction) bool { } // IsValid - checks if action is valid or not. +// +// The receiver is the pattern, so this asks whether the pattern matches any +// supported admin action. Statement.isAdmin calls it for every action of every +// statement it evaluates, so the two cases that can be answered without +// touching SupportedAdminActions are answered first: a literal action can only +// match by being in the set, and a pattern is only worth scanning when its +// literal head is prefix-compatible with the admin namespace. func (action AdminAction) IsValid() bool { + if _, ok := SupportedAdminActions[action]; ok { + return true + } + s := string(action) + star := strings.IndexAny(s, "*?") + if star < 0 { + return false + } + if !canMatchPrefix(s[:star], adminActionPrefix) { + return false + } for supAction := range SupportedAdminActions { if action.Match(supAction) { return true @@ -474,6 +501,20 @@ func (action AdminAction) IsValid() bool { return false } +// adminActionPrefix is the namespace every supported admin action carries. +const adminActionPrefix = "admin:" + +// canMatchPrefix reports whether a wildcard pattern whose literal head (the +// part before its first metacharacter) is head could match any string starting +// with prefix. Everything up to the first metacharacter has to match literally, +// so head and prefix must agree wherever they overlap. +func canMatchPrefix(head, prefix string) bool { + if len(head) > len(prefix) { + return strings.HasPrefix(head, prefix) + } + return strings.HasPrefix(prefix, head) +} + func createAdminActionConditionKeyMap() map[Action]condition.KeySet { allSupportedAdminKeys := []condition.Key{} for _, keyName := range condition.AllSupportedAdminKeys { diff --git a/policy/admin-action_test.go b/policy/admin-action_test.go new file mode 100644 index 00000000..768dc2a6 --- /dev/null +++ b/policy/admin-action_test.go @@ -0,0 +1,201 @@ +// Copyright (c) 2015-2026 MinIO, Inc. +// +// This file is part of MinIO 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 policy + +import ( + "strings" + "testing" +) + +// isValidScan is the linear scan AdminAction.IsValid replaced. The fast path +// must agree with it on every input, so it stays here as the reference. +func isValidScan(action AdminAction) bool { + for supAction := range SupportedAdminActions { + if action.Match(supAction) { + return true + } + } + return false +} + +func hasResourceScan(action AdminAction) bool { + for a := range AdminActionsWithResource { + if action.Match(a) { + return true + } + } + return false +} + +// TestAdminActionNamespacePrefix pins the invariant AdminAction.IsValid's fast +// path rests on: an action that cannot start with "admin:" cannot be an admin +// action. SupportedActions has already lost the analogous s3: invariant to +// s3express:CreateSession, so this is not hypothetical. +func TestAdminActionNamespacePrefix(t *testing.T) { + for action := range SupportedAdminActions { + if !strings.HasPrefix(string(action), adminActionPrefix) { + t.Errorf("SupportedAdminActions contains %q, which lacks the %q prefix that IsValid's fast path assumes", action, adminActionPrefix) + } + } + for action := range AdminActionsWithResource { + if !strings.HasPrefix(string(action), adminActionPrefix) { + t.Errorf("AdminActionsWithResource contains %q, which lacks the %q prefix", action, adminActionPrefix) + } + } +} + +func TestAdminActionIsValid(t *testing.T) { + tests := []struct { + action AdminAction + want bool + }{ + {"admin:Heal", true}, + {"admin:*", true}, + {"*", true}, + {"**", true}, + {"*:*", true}, + {"adm*", true}, + {"ad?in:Heal", true}, + {"a*d*m*i*n*", true}, // a plain HasPrefix on the literal head gets this wrong + {"admin:Hea*", true}, + {"?dmin:Heal", true}, + {"", false}, + {"?", false}, + {"admin", false}, + {"admin:", false}, + {"admin:NotAThing", false}, + {"admin:heal", false}, + {"Admin:*", false}, + {"adminx*", false}, + {"s3:*", false}, + {"s3:GetObject", false}, + {"s3tables:*", false}, + {"sts:*", false}, + {"kms:*", false}, + {"s3vectors:*", false}, + {"memory:*", false}, + } + for _, tt := range tests { + if got := tt.action.IsValid(); got != tt.want { + t.Errorf("AdminAction(%q).IsValid() = %v, want %v", tt.action, got, tt.want) + } + if want := isValidScan(tt.action); tt.want != want { + t.Errorf("test table disagrees with the reference scan for %q: table %v, scan %v", tt.action, tt.want, want) + } + } +} + +func TestAdminActionHasResourceFastPath(t *testing.T) { + tests := []struct { + action AdminAction + want bool + }{ + {"admin:SetBucketQuota", true}, + {"admin:Heal", true}, + {"admin:*", true}, // a key of SupportedAdminActions but not of AdminActionsWithResource + {"*", true}, + {"admin:SetBucket*", true}, + {"admin:ServerInfo", false}, + {"admin:CreateUser", false}, + {"", false}, + {"s3:*", false}, + } + for _, tt := range tests { + if got := tt.action.HasResource(); got != tt.want { + t.Errorf("AdminAction(%q).HasResource() = %v, want %v", tt.action, got, tt.want) + } + if want := hasResourceScan(tt.action); tt.want != want { + t.Errorf("test table disagrees with the reference scan for %q: table %v, scan %v", tt.action, tt.want, want) + } + } +} + +// TestAdminActionIsValidEquivalence checks the fast path against the reference +// scan over every action the package knows plus adversarial patterns. +func TestAdminActionIsValidEquivalence(t *testing.T) { + var pats []string + add := func(s string) { pats = append(pats, s) } + for k := range SupportedAdminActions { + add(string(k)) + } + for k := range SupportedActions { + add(string(k)) + } + for k := range SupportedTableActions { + add(string(k)) + } + for k := range SupportedVectorsActions { + add(string(k)) + } + for k := range SupportedMemoryActions { + add(string(k)) + } + for k := range supportedKMSActions { + add(string(k)) + } + for k := range supportedSTSActions { + add(string(k)) + } + // Every prefix of "admin:Heal", with and without a trailing metacharacter. + const sample = "admin:Heal" + for i := range len(sample) + 1 { + add(sample[:i]) + add(sample[:i] + "*") + add(sample[:i] + "?") + add("*" + sample[i:]) + add("?" + sample[i:]) + } + for _, x := range []string{"", "*", "?", "a", "ad", "admin", "admin:", ":", "Heal", "s3"} { + for _, y := range []string{"", "*", "?", "a", "ad", "admin", "admin:", ":", "Heal", "s3"} { + add(x + y) + for _, z := range []string{"", "*", "?", ":", "Heal"} { + add(x + y + z) + } + } + } + for _, p := range pats { + a := AdminAction(p) + if got, want := a.IsValid(), isValidScan(a); got != want { + t.Errorf("AdminAction(%q).IsValid() = %v, reference scan = %v", p, got, want) + } + if got, want := a.HasResource(), hasResourceScan(a); got != want { + t.Errorf("AdminAction(%q).HasResource() = %v, reference scan = %v", p, got, want) + } + } + t.Logf("checked %d patterns", len(pats)) +} + +func FuzzAdminActionIsValid(f *testing.F) { + for _, s := range []string{"", "*", "admin:Heal", "adm*", "s3:GetObject", "?", "a*d", "admin:*"} { + f.Add(s) + } + f.Fuzz(func(t *testing.T, s string) { + // The reference scan is exponential in the star count; bound it so the + // fuzzer compares answers rather than hanging on that separate bug. + if strings.Count(s, "*") > 4 || len(s) > 32 { + t.Skip() + } + a := AdminAction(s) + if got, want := a.IsValid(), isValidScan(a); got != want { + t.Fatalf("AdminAction(%q).IsValid() = %v, reference scan = %v", s, got, want) + } + if got, want := a.HasResource(), hasResourceScan(a); got != want { + t.Fatalf("AdminAction(%q).HasResource() = %v, reference scan = %v", s, got, want) + } + }) +} diff --git a/policy/parse_dos_test.go b/policy/parse_dos_test.go new file mode 100644 index 00000000..486b52e7 --- /dev/null +++ b/policy/parse_dos_test.go @@ -0,0 +1,57 @@ +// Copyright (c) 2015-2026 MinIO, Inc. +// +// This file is part of MinIO 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 policy + +import ( + "bytes" + "strings" + "testing" + "time" +) + +// Validating a statement classifies every action against each namespace, and a +// star-heavy action pattern used to make that cost time exponential in the star +// count: "*********x" held ParseConfig for a minute and the same action in a +// bucket policy held ParseBucketPolicyConfig for nineteen seconds. Both are +// caller-supplied, so parsing has to stay bounded. +func TestParseStarHeavyActionIsBounded(t *testing.T) { + const budget = 2 * time.Second + for _, stars := range []int{9, 16, 64} { + action := strings.Repeat("*", stars) + "x" + + t.Run("iam", func(t *testing.T) { + doc := `{"Version":"2012-10-17","Statement":[{"Effect":"Allow","Action":["` + + action + `"],"Resource":["arn:aws:s3:::b/*"]}]}` + start := time.Now() + ParseConfig(bytes.NewReader([]byte(doc))) + if d := time.Since(start); d > budget { + t.Errorf("ParseConfig with %d stars took %v, want under %v", stars, d, budget) + } + }) + + t.Run("bucket", func(t *testing.T) { + doc := `{"Version":"2012-10-17","Statement":[{"Effect":"Allow","Principal":{"AWS":["*"]},"Action":["` + + action + `"],"Resource":["arn:aws:s3:::b/*"]}]}` + start := time.Now() + ParseBucketPolicyConfig(bytes.NewReader([]byte(doc)), "b") + if d := time.Since(start); d > budget { + t.Errorf("ParseBucketPolicyConfig with %d stars took %v, want under %v", stars, d, budget) + } + }) + } +} diff --git a/policy/policy.go b/policy/policy.go index d865f646..1fecd205 100644 --- a/policy/policy.go +++ b/policy/policy.go @@ -46,6 +46,24 @@ type Args struct { ObjectName string `json:"object"` Claims map[string]any `json:"claims"` DenyOnly bool `json:"denyOnly"` // only applies deny + + // Memoized requestResource output, with the fields it was built from so a + // caller that reuses an Args for a second object cannot read a stale one. + // A single evaluation must not share one *Args across goroutines. + resource string + resourceFrom [2]string +} + +// requestResource returns the resource string this request names, building it +// at most once per Args. Every statement of every policy needs the same string, +// and rebuilding it per statement was the largest single source of allocation +// on the authorization path. +func (a *Args) requestResource() string { + if a.resource == "" || a.resourceFrom[0] != a.BucketName || a.resourceFrom[1] != a.ObjectName { + a.resource = buildRequestResource(a) + a.resourceFrom = [2]string{a.BucketName, a.ObjectName} + } + return a.resource } // GetValuesFromClaims returns the list of values for the input claimName. @@ -126,8 +144,21 @@ type Policy struct { } // HasDenyStatement returns if the policy has a deny statement. +// +// hasDeny is only populated by updateActionIndex, which a policy built as a +// struct literal never reaches — the built-in readonly policies are built that +// way and do carry a Deny — so the statements are the authority and the field +// is only a shortcut. func (iamp *Policy) HasDenyStatement() bool { - return iamp.hasDeny + if iamp.hasDeny { + return true + } + for i := range iamp.Statements { + if iamp.Statements[i].Effect == Deny { + return true + } + } + return false } // MatchResource matches resource with match resource patterns @@ -247,8 +278,10 @@ func IsAllowedPar(policies []Policy, args Args) bool { maxJ := min(i+numPoliciesPerWorker, len(policies)) res := NoDecision + // Args memoizes per-request state, so each worker needs its own. + a := args for j := i; j < maxJ; j++ { - decision := policies[j].Decide(&args) + decision := policies[j].Decide(&a) if decision == DenyDecision { res = DenyDecision break @@ -298,9 +331,12 @@ const ( // statement explicitly allows or denies the operation in the Args, it returns // `noDecision`. It is upto the caller to handle such cases. func (iamp *Policy) Decide(args *Args) Decision { + resource := args.requestResource() + // Check all deny statements. If any one statement denies, return false. - for _, statement := range iamp.Statements { - if statement.Effect == Deny && !statement.IsAllowedPtr(args) { + for i := range iamp.Statements { + statement := &iamp.Statements[i] + if statement.Effect == Deny && !statement.isAllowedFor(args, resource) { return DenyDecision } } @@ -320,19 +356,29 @@ func (iamp *Policy) Decide(args *Args) Decision { } // Check all allow statements. If any one statement allows, return true. + // The index only covers statements that name args.Action literally, so the + // full walk still has to happen; tried records what the index already + // evaluated so the walk does not evaluate it a second time. + var tried []int if len(iamp.actionStatementIndex) > 0 { if indexes, ok := iamp.actionStatementIndex[args.Action]; ok { for _, index := range indexes { - statement := iamp.Statements[index] - if statement.Effect == Allow && statement.IsAllowedPtr(args) { + statement := &iamp.Statements[index] + if statement.Effect == Allow && statement.isAllowedFor(args, resource) { return AllowDecision } } + tried = indexes } } - for _, statement := range iamp.Statements { - if statement.Effect == Allow && statement.IsAllowedPtr(args) { + for i := range iamp.Statements { + if len(tried) > 0 && tried[0] == i { + tried = tried[1:] + continue + } + statement := &iamp.Statements[i] + if statement.Effect == Allow && statement.isAllowedFor(args, resource) { return AllowDecision } } @@ -516,6 +562,7 @@ func (iamp Policy) ValidateStrict() error { func (iamp *Policy) updateActionIndex() { for i := range iamp.Statements { stmt := &iamp.Statements[i] + stmt.class = stmt.computeClass() if stmt.Effect == Deny { iamp.hasDeny = true continue diff --git a/policy/policy_regression_test.go b/policy/policy_regression_test.go new file mode 100644 index 00000000..3f174ebd --- /dev/null +++ b/policy/policy_regression_test.go @@ -0,0 +1,111 @@ +// Copyright (c) 2015-2026 MinIO, Inc. +// +// This file is part of MinIO 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 policy + +import ( + "fmt" + "testing" + + "github.com/minio/pkg/v3/policy/condition" +) + +// Statement.hash once omitted NotResources, so dropDuplicateStatements collapsed +// two Deny statements that differed only there -- silently discarding a Deny -- +// but only past the 10-statement threshold where the hash path takes over. +func TestDropDuplicateStatementsKeepsNotResources(t *testing.T) { + mk := func(n int) Policy { + sts := []Statement{ + NewStatementWithNotResource("", Deny, + NewActionSet(GetObjectAction), + NewResourceSet(NewResource("public/*")), + condition.NewFunctions()), + NewStatementWithNotResource("", Deny, + NewActionSet(GetObjectAction), + NewResourceSet(NewResource("other/*")), + condition.NewFunctions()), + } + // pad with distinct filler statements to cross the threshold + for i := len(sts); i < n; i++ { + sts = append(sts, NewStatement("", Allow, + NewActionSet(PutObjectAction), + NewResourceSet(NewResource(fmt.Sprintf("filler%d/*", i))), + condition.NewFunctions())) + } + return Policy{Version: DefaultVersion, Statements: sts} + } + for _, n := range []int{4, 11, 20} { + p := mk(n) + before := len(p.Statements) + p.dropDuplicateStatements() + path := "original(<=10)" + if n > 10 { + path = "hashed(>10)" + } + t.Logf("n=%2d %-15s statements %d -> %d", n, path, before, len(p.Statements)) + if len(p.Statements) != before { + t.Errorf("n=%d: LOST %d statement(s) that are not duplicates", n, before-len(p.Statements)) + } + } +} + +// Policy.hasDeny is only set by updateActionIndex, which a policy built as a +// struct literal never reaches. The built-in readonly policies are built that +// way and do carry a Deny, so HasDenyStatement must not trust the field alone. +func TestHasDenyStatementOnStructLiteralPolicy(t *testing.T) { + for _, name := range []string{"readonly", "consolereadonly", "diagnostics"} { + for _, dp := range DefaultPolicies { + if dp.Name != name { + continue + } + p := dp.Definition + hasDenyStmt := false + for _, s := range p.Statements { + if s.Effect == Deny { + hasDenyStmt = true + } + } + t.Logf("%-16s actual Deny statement=%v HasDenyStatement()=%v", name, hasDenyStmt, p.HasDenyStatement()) + if hasDenyStmt && !p.HasDenyStatement() { + t.Errorf("%s: carries a Deny but HasDenyStatement() reports false", name) + } + } + } +} + +// Decide's DenyOnly and IsOwner fallthroughs sit below the Deny loop, so a +// policy with nothing to match must still reach them. Short-circuiting on an +// empty statement list turns every STS login for such a credential into +// ErrSTSAccessDenied. Any future work that narrows which statements Decide +// walks has to keep this passing. +func TestDecideReachesDenyOnlyAndIsOwnerWithNoStatements(t *testing.T) { + p := Policy{Version: DefaultVersion} + tests := []struct { + name string + args Args + want Decision + }{ + {"DenyOnly", Args{Action: Action(AssumeRoleWithWebIdentityAction), DenyOnly: true}, AllowDecision}, + {"IsOwner", Args{Action: GetObjectAction, BucketName: "b", ObjectName: "o", IsOwner: true}, AllowDecision}, + {"neither", Args{Action: GetObjectAction, BucketName: "b", ObjectName: "o"}, NoDecision}, + } + for _, tt := range tests { + if got := p.Decide(&tt.args); got != tt.want { + t.Errorf("%s: Decide on a statement-less policy = %v, want %v", tt.name, got, tt.want) + } + } +} diff --git a/policy/statement.go b/policy/statement.go index 2c27e611..fcb2ccab 100644 --- a/policy/statement.go +++ b/policy/statement.go @@ -36,6 +36,12 @@ type Statement struct { Resources ResourceSet `json:"Resource,omitempty"` NotResources ResourceSet `json:"NotResource,omitempty"` Conditions condition.Functions `json:"Condition,omitempty"` + + // Namespace classification, filled in by Policy.updateActionIndex. Zero + // means "not classified yet" — classify() then derives it on the spot, so a + // statement built by a path that does not populate it is slower, never + // wrong. Statement.Clone deliberately leaves it zero. + class statementClass } // smallBufPool should always return a non-nil *bytes.Buffer @@ -48,28 +54,42 @@ func (statement Statement) IsAllowed(args Args) bool { return statement.IsAllowedPtr(&args) } +// buildRequestResource renders the resource string an Args names, in the form +// ResourceSet.Match expects. Callers go through Args.requestResource, which +// memoizes it. +func buildRequestResource(args *Args) string { + buf := smallBufPool.Get().(*bytes.Buffer) + defer smallBufPool.Put(buf) + buf.Reset() + buf.WriteString(args.BucketName) + if args.ObjectName != "" { + if !strings.HasPrefix(args.ObjectName, "/") { + buf.WriteByte('/') + } + buf.WriteString(args.ObjectName) + } else { + buf.WriteByte('/') + } + return buf.String() +} + // IsAllowedPtr - checks given policy args is allowed to continue the Rest API. func (statement Statement) IsAllowedPtr(args *Args) bool { + return statement.isAllowedFor(args, args.requestResource()) +} + +// isAllowedFor is IsAllowedPtr with the request resource string supplied by the +// caller. +func (statement Statement) isAllowedFor(args *Args, resource string) bool { check := func() bool { if (!statement.Actions.Match(args.Action) && !statement.Actions.IsEmpty()) || statement.NotActions.Match(args.Action) { return false } - resource := smallBufPool.Get().(*bytes.Buffer) - defer smallBufPool.Put(resource) - resource.Reset() - resource.WriteString(args.BucketName) - if args.ObjectName != "" { - if !strings.HasPrefix(args.ObjectName, "/") { - resource.WriteByte('/') - } - resource.WriteString(args.ObjectName) - } else { - resource.WriteByte('/') - } + class := statement.classify() - if statement.isTable() && !TableAction(args.Action).IsValid() { + if class.has(classTable) && !TableAction(args.Action).IsValid() { // When a tables policy statement (for example // "Action": ["s3tables:GetTableData"], // "Resource": ["arn:aws:s3tables:::bucket/wh/table/uuid"] @@ -87,7 +107,7 @@ func (statement Statement) IsAllowedPtr(args *Args) bool { // s3tables:GetTableData (or similar) is granted, normalize the // S3 data-path resource into the canonical tables form before // running the usual resource match. - if !isTableResourceString(resource.String()) { + if !isTableResourceString(resource) { if args.BucketName == "" || args.ObjectName == "" { return false } @@ -95,19 +115,15 @@ func (statement Statement) IsAllowedPtr(args *Args) bool { if idx := strings.IndexByte(objectName, '/'); idx >= 0 { objectName = objectName[:idx] } - resource.Reset() - resource.WriteString("bucket/") - resource.WriteString(args.BucketName) - resource.WriteString("/table/") - resource.WriteString(objectName) - if !isTableResourceString(resource.String()) { + resource = "bucket/" + args.BucketName + "/table/" + objectName + if !isTableResourceString(resource) { return false } } } - if statement.isKMS() { - if resource.Len() == 1 && resource.String() == "/" || len(statement.Resources) == 0 { + if class.has(classKMS) { + if resource == "/" || len(statement.Resources) == 0 { // In previous MinIO versions, KMS statements ignored Resources, so if len(statement.Resources) == 0, // allow backward compatibility by not trying to Match. @@ -122,13 +138,13 @@ func (statement Statement) IsAllowedPtr(args *Args) bool { // skip resource matching entirely. For the small set of // bucket-scoped admin actions (e.g. SetBucketQuota), // resource matching is enforced when Resources are present. - ignoreResourceMatch := statement.isSTS() || (statement.isAdmin() && !statement.hasAdminResource()) + ignoreResourceMatch := class.has(classSTS) || (class.has(classAdmin) && !class.has(classAdminResource)) - if !ignoreResourceMatch && len(statement.Resources) > 0 && !statement.Resources.Match(resource.String(), args.ConditionValues) { + if !ignoreResourceMatch && len(statement.Resources) > 0 && !statement.Resources.Match(resource, args.ConditionValues) { return false } - if !ignoreResourceMatch && len(statement.NotResources) > 0 && statement.NotResources.Match(resource.String(), args.ConditionValues) { + if !ignoreResourceMatch && len(statement.NotResources) > 0 && statement.NotResources.Match(resource, args.ConditionValues) { return false } @@ -177,6 +193,61 @@ func (statement Statement) validateActionTypes() error { return nil } +// statementClass records which action namespaces a statement's actions belong +// to. Statements are validated to carry actions from a single namespace, but +// this is a bit set so that a policy predating that rule classifies exactly as +// the individual predicates did. +type statementClass uint8 + +const ( + classAdmin statementClass = 1 << iota + classSTS + classKMS + classTable + // classAdminResource marks an admin action that is scoped to a bucket. + classAdminResource + // classKnown distinguishes a computed classification from the zero value. + // A plain S3 statement belongs to none of the namespaces above, so without + // it "no namespace" and "not computed" would be the same value. + classKnown +) + +func (c statementClass) has(f statementClass) bool { return c&f != 0 } + +// classify reports the statement's namespaces, using the value cached at parse +// time when there is one. +func (statement Statement) classify() statementClass { + if statement.class != 0 { + return statement.class + } + return statement.computeClass() +} + +// computeClass walks the action set once. The predicates it stands in for on +// the authorization path each walked it separately, so a statement paid four +// map iterator setups per evaluation to learn what one pass can tell it. +func (statement Statement) computeClass() statementClass { + c := classKnown + for action := range statement.Actions { + if AdminAction(action).IsValid() { + c |= classAdmin + } + if AdminAction(action).HasResource() { + c |= classAdminResource + } + if STSAction(action).IsValid() { + c |= classSTS + } + if KMSAction(action).IsValid() { + c |= classKMS + } + if TableAction(action).IsValid() { + c |= classTable + } + } + return c +} + func (statement Statement) isAdmin() bool { for action := range statement.Actions { if AdminAction(action).IsValid() { @@ -638,6 +709,11 @@ func (statement Statement) hash(seed uint64) [16]byte { xorTo(&h, xxh3.HashString128Seed(res.Pattern+res.Type.String(), seed+6)) } + xorInt(&h, len(statement.NotResources), seed+9) + for res := range statement.NotResources { + xorTo(&h, xxh3.HashString128Seed(res.Pattern+res.Type.String(), seed+10)) + } + xorInt(&h, len(statement.Conditions), seed+7) for _, cond := range statement.Conditions { xorTo(&h, xxh3.HashString128Seed(cond.String(), seed+8)) diff --git a/wildcard/match.go b/wildcard/match.go index 2a3f3993..d1072861 100644 --- a/wildcard/match.go +++ b/wildcard/match.go @@ -33,8 +33,18 @@ func MatchSimple(pattern, name string) bool { if pattern == "*" { return true } - // Do an extended wildcard '*' and '?' match. - return deepMatchRune(name, pattern, true) + if deepMatchRune(name, pattern) { + return true + } + // Reaching a '?' with name already exhausted succeeds, and succeeds for + // the whole pattern, so the pattern also matches whenever any prefix of it + // ending at a '?' consumes name exactly. + for i := range len(pattern) { + if pattern[i] == '?' && deepMatchRune(name, pattern[:i]) { + return true + } + } + return false } // Match - finds whether the text matches/satisfies the pattern string. @@ -49,7 +59,7 @@ func Match(pattern, name string) (matched bool) { return true } // Do an extended wildcard '*' and '?' match. - return deepMatchRune(name, pattern, false) + return deepMatchRune(name, pattern) } // Has returns true if the input pattern has a wildcard (pattern). @@ -57,26 +67,47 @@ func Has(pattern string) bool { return cmp.Or(strings.Contains(pattern, "*"), strings.Contains(pattern, "?")) } -func deepMatchRune(str, pattern string, simple bool) bool { - for len(pattern) > 0 { - switch pattern[0] { - default: - if len(str) == 0 || str[0] != pattern[0] { - return false +// deepMatchRune walks pattern against str one byte at a time, remembering only +// the most recent '*' to resume from. Trying both alternatives at every '*' +// instead — as a recursive matcher does — costs time exponential in the number +// of stars, which a caller-supplied pattern can trigger: a policy action of +// "*********x" kept AdminAction.IsValid busy for a minute. +func deepMatchRune(str, pattern string) bool { + var s, p int + // Position of the '*' to resume from, and how much of str it has consumed. + star, mark := -1, 0 + for s < len(str) || p < len(pattern) { + if p < len(pattern) { + switch pattern[p] { + case '*': + star, mark = p, s + p++ + continue + case '?': + if s < len(str) { + s++ + p++ + continue + } + default: + if s < len(str) && pattern[p] == str[s] { + s++ + p++ + continue + } } - case '?': - if len(str) == 0 { - return simple - } - case '*': - return len(pattern) == 1 || // Pattern ends with this star - deepMatchRune(str, pattern[1:], simple) || // Matches next part of pattern - (len(str) > 0 && deepMatchRune(str[1:], pattern, simple)) // Continue searching forward } - str = str[1:] - pattern = pattern[1:] + if star < 0 { + return false + } + // Let the last '*' swallow one more byte and retry from there. + mark++ + if mark > len(str) { + return false + } + s, p = mark, star+1 } - return len(str) == 0 && len(pattern) == 0 + return true } // MatchAsPatternPrefix matches text as a prefix of the given pattern. Examples: diff --git a/wildcard/match_equivalence_test.go b/wildcard/match_equivalence_test.go new file mode 100644 index 00000000..ac8cba5a --- /dev/null +++ b/wildcard/match_equivalence_test.go @@ -0,0 +1,165 @@ +// Copyright (c) 2015-2026 MinIO, Inc. +// +// This file is part of MinIO 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 wildcard + +import ( + "strings" + "testing" + "time" +) + +// oldDeepMatchRune is the recursive matcher deepMatchRune replaced, kept here so +// the rewrite can be proven equivalent rather than asserted to be. +func oldDeepMatchRune(str, pattern string, simple bool) bool { + for len(pattern) > 0 { + switch pattern[0] { + default: + if len(str) == 0 || str[0] != pattern[0] { + return false + } + case '?': + if len(str) == 0 { + return simple + } + case '*': + return len(pattern) == 1 || + oldDeepMatchRune(str, pattern[1:], simple) || + (len(str) > 0 && oldDeepMatchRune(str[1:], pattern, simple)) + } + str = str[1:] + pattern = pattern[1:] + } + return len(str) == 0 && len(pattern) == 0 +} + +func gen(alphabet string, maxLen int) []string { + out := []string{""} + cur := []string{""} + for range maxLen { + var next []string + for _, s := range cur { + for _, c := range alphabet { + next = append(next, s+string(c)) + } + } + out = append(out, next...) + cur = next + } + return out +} + +// Exhaustive over a small alphabet: every pattern up to length 5 against every +// name up to length 5, for both simple modes. +func TestDeepMatchEquivalenceExhaustive(t *testing.T) { + pats := gen("ab*?", 5) + names := gen("ab", 5) + var n int + for _, p := range pats { + for _, name := range names { + if got, want := Match(p, name), oldMatch(p, name); got != want { + t.Fatalf("Match(%q, %q) = %v, old = %v", p, name, got, want) + } + if got, want := MatchSimple(p, name), oldMatchSimple(p, name); got != want { + t.Fatalf("MatchSimple(%q, %q) = %v, old = %v", p, name, got, want) + } + n += 2 + } + } + t.Logf("%d pattern/name/mode combinations agree", n) +} + +// Same for the exported entry points, over a colon-bearing alphabet closer to +// policy actions and resource ARNs. +func TestMatchEquivalenceExhaustive(t *testing.T) { + pats := gen("a:*?", 4) + names := gen("a:/", 4) + var n int + for _, p := range pats { + for _, name := range names { + if got, want := Match(p, name), oldMatch(p, name); got != want { + t.Fatalf("Match(%q, %q) = %v, old = %v", p, name, got, want) + } + if got, want := MatchSimple(p, name), oldMatchSimple(p, name); got != want { + t.Fatalf("MatchSimple(%q, %q) = %v, old = %v", p, name, got, want) + } + n += 2 + } + } + t.Logf("%d exported-entry-point combinations agree", n) +} + +func oldMatch(pattern, name string) bool { + if pattern == "" { + return name == pattern + } + if pattern == "*" { + return true + } + return oldDeepMatchRune(name, pattern, false) +} + +func oldMatchSimple(pattern, name string) bool { + if pattern == "" { + return name == pattern + } + if pattern == "*" { + return true + } + return oldDeepMatchRune(name, pattern, true) +} + +// A pattern with many stars used to take time exponential in the star count. +func TestMatchStarsAreLinear(t *testing.T) { + name := "admin:ServerInfo" + for _, stars := range []int{8, 16, 64, 256} { + pattern := strings.Repeat("*", stars) + "X" + start := time.Now() + if Match(pattern, name) { + t.Fatalf("pattern %d stars + X should not match %q", stars, name) + } + if d := time.Since(start); d > 50*time.Millisecond { + t.Errorf("Match with %d stars took %v, want well under 50ms", stars, d) + } + } + // Interleaved stars are the harder shape. + pattern := strings.Repeat("*a", 32) + "X" + start := time.Now() + Match(pattern, strings.Repeat("a", 128)) + if d := time.Since(start); d > 50*time.Millisecond { + t.Errorf("Match with interleaved stars took %v, want well under 50ms", d) + } +} + +func FuzzDeepMatchEquivalence(f *testing.F) { + for _, s := range []string{"", "*", "?", "a*b", "admin:*", "**?", "a", "*a*a*b"} { + f.Add(s, "admin:Heal") + } + f.Fuzz(func(t *testing.T, pattern, name string) { + // Bound the old implementation's exponential blowup so the fuzzer + // compares results instead of timing out on the bug being fixed. + if strings.Count(pattern, "*") > 4 || len(pattern) > 24 || len(name) > 24 { + t.Skip() + } + if got, want := Match(pattern, name), oldMatch(pattern, name); got != want { + t.Fatalf("Match(%q, %q) = %v, old = %v", pattern, name, got, want) + } + if got, want := MatchSimple(pattern, name), oldMatchSimple(pattern, name); got != want { + t.Fatalf("MatchSimple(%q, %q) = %v, old = %v", pattern, name, got, want) + } + }) +} From 3c371036df22f528fb1388d9add3cbefdcbe48af Mon Sep 17 00:00:00 2001 From: Klaus Post Date: Fri, 21 Aug 2026 11:32:52 +0200 Subject: [PATCH 2/6] Address feedback --- policy/constants.go | 9 ++++++ policy/parse_dos_test.go | 21 +++++++++---- policy/policy.go | 50 ++++++++++++++---------------- policy/policy_regression_test.go | 30 +++++++++++++++--- policy/statement.go | 5 ++- wildcard/match.go | 16 ++++++++-- wildcard/match_equivalence_test.go | 33 +++++++++++++++++++- 7 files changed, 120 insertions(+), 44 deletions(-) diff --git a/policy/constants.go b/policy/constants.go index beab9ac4..84ca706f 100644 --- a/policy/constants.go +++ b/policy/constants.go @@ -27,6 +27,15 @@ const ( SessionPolicyName = "sessionPolicy" ) +// The canned policies are struct literals, so none of them has been through +// the parse path that classifies statements, indexes actions and records that +// a policy carries a Deny. Do it once here instead of on every evaluation. +func init() { + for i := range DefaultPolicies { + DefaultPolicies[i].Definition.updateActionIndex() + } +} + // DefaultPolicies - list of canned policies available in MinIO. var DefaultPolicies = []struct { Name string diff --git a/policy/parse_dos_test.go b/policy/parse_dos_test.go index 486b52e7..72321b90 100644 --- a/policy/parse_dos_test.go +++ b/policy/parse_dos_test.go @@ -19,6 +19,7 @@ package policy import ( "bytes" + "fmt" "strings" "testing" "time" @@ -34,23 +35,31 @@ func TestParseStarHeavyActionIsBounded(t *testing.T) { for _, stars := range []int{9, 16, 64} { action := strings.Repeat("*", stars) + "x" - t.Run("iam", func(t *testing.T) { + t.Run(fmt.Sprintf("iam-%d-stars", stars), func(t *testing.T) { doc := `{"Version":"2012-10-17","Statement":[{"Effect":"Allow","Action":["` + action + `"],"Resource":["arn:aws:s3:::b/*"]}]}` start := time.Now() - ParseConfig(bytes.NewReader([]byte(doc))) + _, err := ParseConfig(bytes.NewReader([]byte(doc))) if d := time.Since(start); d > budget { - t.Errorf("ParseConfig with %d stars took %v, want under %v", stars, d, budget) + t.Errorf("ParseConfig took %v, want under %v", d, budget) + } + // The action names nothing supported, so bounding the cost must not + // have come at the price of accepting it. + if err == nil { + t.Errorf("ParseConfig accepted unsupported action %q", action) } }) - t.Run("bucket", func(t *testing.T) { + t.Run(fmt.Sprintf("bucket-%d-stars", stars), func(t *testing.T) { doc := `{"Version":"2012-10-17","Statement":[{"Effect":"Allow","Principal":{"AWS":["*"]},"Action":["` + action + `"],"Resource":["arn:aws:s3:::b/*"]}]}` start := time.Now() - ParseBucketPolicyConfig(bytes.NewReader([]byte(doc)), "b") + _, err := ParseBucketPolicyConfig(bytes.NewReader([]byte(doc)), "b") if d := time.Since(start); d > budget { - t.Errorf("ParseBucketPolicyConfig with %d stars took %v, want under %v", stars, d, budget) + t.Errorf("ParseBucketPolicyConfig took %v, want under %v", d, budget) + } + if err == nil { + t.Errorf("ParseBucketPolicyConfig accepted unsupported action %q", action) } }) } diff --git a/policy/policy.go b/policy/policy.go index 1fecd205..d760f6ac 100644 --- a/policy/policy.go +++ b/policy/policy.go @@ -46,24 +46,6 @@ type Args struct { ObjectName string `json:"object"` Claims map[string]any `json:"claims"` DenyOnly bool `json:"denyOnly"` // only applies deny - - // Memoized requestResource output, with the fields it was built from so a - // caller that reuses an Args for a second object cannot read a stale one. - // A single evaluation must not share one *Args across goroutines. - resource string - resourceFrom [2]string -} - -// requestResource returns the resource string this request names, building it -// at most once per Args. Every statement of every policy needs the same string, -// and rebuilding it per statement was the largest single source of allocation -// on the authorization path. -func (a *Args) requestResource() string { - if a.resource == "" || a.resourceFrom[0] != a.BucketName || a.resourceFrom[1] != a.ObjectName { - a.resource = buildRequestResource(a) - a.resourceFrom = [2]string{a.BucketName, a.ObjectName} - } - return a.resource } // GetValuesFromClaims returns the list of values for the input claimName. @@ -145,10 +127,11 @@ type Policy struct { // HasDenyStatement returns if the policy has a deny statement. // -// hasDeny is only populated by updateActionIndex, which a policy built as a -// struct literal never reaches — the built-in readonly policies are built that -// way and do carry a Deny — so the statements are the authority and the field -// is only a shortcut. +// hasDeny is only populated by updateActionIndex, which every policy this +// package builds goes through, but which a policy a caller assembles as a +// struct literal never reaches. So the field is a shortcut and the statements +// stay the authority: trusting the field alone would silently under-report a +// Deny on such a policy. func (iamp *Policy) HasDenyStatement() bool { if iamp.hasDeny { return true @@ -218,9 +201,10 @@ func (iamp Policy) IsAllowedActions(bucketName, objectName string, conditionValu // // This is currently the fastest implementation for our basic benchmark. func IsAllowedSerial(policies []Policy, args Args) bool { + resource := buildRequestResource(&args) gotAllow := false for _, policy := range policies { - res := policy.Decide(&args) + res := policy.decide(&args, resource) if res == DenyDecision { return false } @@ -263,6 +247,7 @@ func IsAllowedPar(policies []Policy, args Args) bool { close(jobs) resultCh := make(chan Decision, len(policies)) + resource := buildRequestResource(&args) var wg sync.WaitGroup wg.Add(numWorkers) @@ -278,10 +263,11 @@ func IsAllowedPar(policies []Policy, args Args) bool { maxJ := min(i+numPoliciesPerWorker, len(policies)) res := NoDecision - // Args memoizes per-request state, so each worker needs its own. + // Each worker evaluates against its own Args value, so nothing + // on the evaluation path is shared between goroutines. a := args for j := i; j < maxJ; j++ { - decision := policies[j].Decide(&a) + decision := policies[j].decide(&a, resource) if decision == DenyDecision { res = DenyDecision break @@ -331,8 +317,13 @@ const ( // statement explicitly allows or denies the operation in the Args, it returns // `noDecision`. It is upto the caller to handle such cases. func (iamp *Policy) Decide(args *Args) Decision { - resource := args.requestResource() + return iamp.decide(args, buildRequestResource(args)) +} +// decide is Decide with the request resource string supplied by the caller. +// Every statement of every policy needs the same string, so a walk over a set +// of policies builds it once instead of once per statement. +func (iamp *Policy) decide(args *Args, resource string) Decision { // Check all deny statements. If any one statement denies, return false. for i := range iamp.Statements { statement := &iamp.Statements[i] @@ -361,7 +352,12 @@ func (iamp *Policy) Decide(args *Args) Decision { // evaluated so the walk does not evaluate it a second time. var tried []int if len(iamp.actionStatementIndex) > 0 { - if indexes, ok := iamp.actionStatementIndex[args.Action]; ok { + // Statements is exported, so a caller can shrink it after the index was + // built. Rather than index out of bounds - or worse, evaluate the wrong + // statement - fall back to the plain walk when the index is stale. + // Indexes are recorded in ascending order, so the last one bounds them all. + if indexes, ok := iamp.actionStatementIndex[args.Action]; ok && + len(indexes) > 0 && indexes[len(indexes)-1] < len(iamp.Statements) { for _, index := range indexes { statement := &iamp.Statements[index] if statement.Effect == Allow && statement.isAllowedFor(args, resource) { diff --git a/policy/policy_regression_test.go b/policy/policy_regression_test.go index 3f174ebd..79e2e16d 100644 --- a/policy/policy_regression_test.go +++ b/policy/policy_regression_test.go @@ -63,10 +63,24 @@ func TestDropDuplicateStatementsKeepsNotResources(t *testing.T) { } } -// Policy.hasDeny is only set by updateActionIndex, which a policy built as a -// struct literal never reaches. The built-in readonly policies are built that -// way and do carry a Deny, so HasDenyStatement must not trust the field alone. +// Policy.hasDeny is only set by updateActionIndex, which a policy assembled as +// a struct literal outside this package never reaches, so HasDenyStatement must +// not trust the field alone. func TestHasDenyStatementOnStructLiteralPolicy(t *testing.T) { + // A literal built here has been through no parse path at all, so it pins + // the behavior down regardless of what the canned policies contain. + literal := Policy{ + Version: DefaultVersion, + Statements: []Statement{ + NewStatement("", Deny, NewActionSet(GetObjectAction), + NewResourceSet(NewResource("*")), condition.NewFunctions()), + }, + } + if !literal.HasDenyStatement() { + t.Error("struct literal policy carries a Deny but HasDenyStatement() reports false") + } + + checked := 0 for _, name := range []string{"readonly", "consolereadonly", "diagnostics"} { for _, dp := range DefaultPolicies { if dp.Name != name { @@ -80,11 +94,17 @@ func TestHasDenyStatementOnStructLiteralPolicy(t *testing.T) { } } t.Logf("%-16s actual Deny statement=%v HasDenyStatement()=%v", name, hasDenyStmt, p.HasDenyStatement()) - if hasDenyStmt && !p.HasDenyStatement() { - t.Errorf("%s: carries a Deny but HasDenyStatement() reports false", name) + if hasDenyStmt { + checked++ + if !p.HasDenyStatement() { + t.Errorf("%s: carries a Deny but HasDenyStatement() reports false", name) + } } } } + if checked == 0 { + t.Error("none of the named canned policies carries a Deny; this test checked nothing") + } } // Decide's DenyOnly and IsOwner fallthroughs sit below the Deny loop, so a diff --git a/policy/statement.go b/policy/statement.go index fcb2ccab..fb59b0ef 100644 --- a/policy/statement.go +++ b/policy/statement.go @@ -55,8 +55,7 @@ func (statement Statement) IsAllowed(args Args) bool { } // buildRequestResource renders the resource string an Args names, in the form -// ResourceSet.Match expects. Callers go through Args.requestResource, which -// memoizes it. +// ResourceSet.Match expects. func buildRequestResource(args *Args) string { buf := smallBufPool.Get().(*bytes.Buffer) defer smallBufPool.Put(buf) @@ -75,7 +74,7 @@ func buildRequestResource(args *Args) string { // IsAllowedPtr - checks given policy args is allowed to continue the Rest API. func (statement Statement) IsAllowedPtr(args *Args) bool { - return statement.isAllowedFor(args, args.requestResource()) + return statement.isAllowedFor(args, buildRequestResource(args)) } // isAllowedFor is IsAllowedPtr with the request resource string supplied by the diff --git a/wildcard/match.go b/wildcard/match.go index d1072861..558dd94e 100644 --- a/wildcard/match.go +++ b/wildcard/match.go @@ -18,7 +18,6 @@ package wildcard import ( - "cmp" "strings" ) @@ -39,10 +38,23 @@ func MatchSimple(pattern, name string) bool { // Reaching a '?' with name already exhausted succeeds, and succeeds for // the whole pattern, so the pattern also matches whenever any prefix of it // ending at a '?' consumes name exactly. + if !strings.ContainsRune(pattern, '?') { + return false + } + // Every byte that is not a '*' consumes one byte of name, so once a prefix + // needs more of them than name has, that prefix and every longer one is + // too long to consume name exactly and the walk can stop. + lits := 0 for i := range len(pattern) { if pattern[i] == '?' && deepMatchRune(name, pattern[:i]) { return true } + if pattern[i] != '*' { + lits++ + if lits > len(name) { + break + } + } } return false } @@ -64,7 +76,7 @@ func Match(pattern, name string) (matched bool) { // Has returns true if the input pattern has a wildcard (pattern). func Has(pattern string) bool { - return cmp.Or(strings.Contains(pattern, "*"), strings.Contains(pattern, "?")) + return strings.ContainsAny(pattern, "*?") } // deepMatchRune walks pattern against str one byte at a time, remembering only diff --git a/wildcard/match_equivalence_test.go b/wildcard/match_equivalence_test.go index ac8cba5a..1fbccb11 100644 --- a/wildcard/match_equivalence_test.go +++ b/wildcard/match_equivalence_test.go @@ -138,13 +138,44 @@ func TestMatchStarsAreLinear(t *testing.T) { } // Interleaved stars are the harder shape. pattern := strings.Repeat("*a", 32) + "X" + name = strings.Repeat("a", 128) start := time.Now() - Match(pattern, strings.Repeat("a", 128)) + if Match(pattern, name) { + t.Errorf("pattern %q should not match %q", pattern, name) + } if d := time.Since(start); d > 50*time.Millisecond { t.Errorf("Match with interleaved stars took %v, want well under 50ms", d) } } +// Question-mark-heavy patterns take the MatchSimple prefix walk rather than a +// single deepMatchRune call, so they get their own before/after comparison. +// Star counts stay low: the old matcher is exponential in them. +func BenchmarkMatchSimpleQuestionMarks(b *testing.B) { + cases := []struct { + name string + pattern string + text string + }{ + {"star-free", strings.Repeat("a?", 16), strings.Repeat("aa", 16)}, + {"all-marks", strings.Repeat("?", 32), strings.Repeat("a", 16)}, + {"leading-star", "*" + strings.Repeat("a?", 8), strings.Repeat("a", 24)}, + {"no-mark", "arn:aws:s3:::*", "arn:aws:s3:::bucket/object"}, + } + for _, c := range cases { + b.Run("new/"+c.name, func(b *testing.B) { + for i := 0; i < b.N; i++ { + _ = MatchSimple(c.pattern, c.text) + } + }) + b.Run("old/"+c.name, func(b *testing.B) { + for i := 0; i < b.N; i++ { + _ = oldMatchSimple(c.pattern, c.text) + } + }) + } +} + func FuzzDeepMatchEquivalence(f *testing.F) { for _, s := range []string{"", "*", "?", "a*b", "admin:*", "**?", "a", "*a*a*b"} { f.Add(s, "admin:Heal") From 5700493069ff8a4e1d791d7bd59d0e7ce063794f Mon Sep 17 00:00:00 2001 From: Klaus Post Date: Fri, 21 Aug 2026 12:06:59 +0200 Subject: [PATCH 3/6] Avoid quadratic ? --- policy/statement.go | 2 +- wildcard/match.go | 10 +++++++++- 2 files changed, 10 insertions(+), 2 deletions(-) diff --git a/policy/statement.go b/policy/statement.go index fb59b0ef..94f0838c 100644 --- a/policy/statement.go +++ b/policy/statement.go @@ -54,7 +54,7 @@ func (statement Statement) IsAllowed(args Args) bool { return statement.IsAllowedPtr(&args) } -// buildRequestResource renders the resource string an Args names, in the form +// buildRequestResource renders the resource string an args names, in the form // ResourceSet.Match expects. func buildRequestResource(args *Args) string { buf := smallBufPool.Get().(*bytes.Buffer) diff --git a/wildcard/match.go b/wildcard/match.go index 558dd94e..ae580032 100644 --- a/wildcard/match.go +++ b/wildcard/match.go @@ -41,12 +41,20 @@ func MatchSimple(pattern, name string) bool { if !strings.ContainsRune(pattern, '?') { return false } + // A prefix holding no '*' consumes exactly one byte of name per byte of + // pattern, so only the prefix as long as name can consume it: a star-free + // pattern costs one deepMatchRune call however many '?' it carries. + firstStar := strings.IndexByte(pattern, '*') + if firstStar < 0 { + firstStar = len(pattern) + } // Every byte that is not a '*' consumes one byte of name, so once a prefix // needs more of them than name has, that prefix and every longer one is // too long to consume name exactly and the walk can stop. lits := 0 for i := range len(pattern) { - if pattern[i] == '?' && deepMatchRune(name, pattern[:i]) { + if pattern[i] == '?' && (i > firstStar || i == len(name)) && + deepMatchRune(name, pattern[:i]) { return true } if pattern[i] != '*' { From 641c50bf67e194bd8db967b278c2b2716b8e262e Mon Sep 17 00:00:00 2001 From: Klaus Post Date: Fri, 21 Aug 2026 12:48:27 +0200 Subject: [PATCH 4/6] Make explicit Reindex and update docs. --- policy/constants.go | 2 +- policy/policy.go | 44 +++++++++++++++++------ policy/policy_regression_test.go | 62 ++++++++++++++++++++++++++++++++ policy/statement.go | 11 ++++-- 4 files changed, 105 insertions(+), 14 deletions(-) diff --git a/policy/constants.go b/policy/constants.go index 84ca706f..bfd02b31 100644 --- a/policy/constants.go +++ b/policy/constants.go @@ -32,7 +32,7 @@ const ( // a policy carries a Deny. Do it once here instead of on every evaluation. func init() { for i := range DefaultPolicies { - DefaultPolicies[i].Definition.updateActionIndex() + DefaultPolicies[i].Definition.Reindex() } } diff --git a/policy/policy.go b/policy/policy.go index d760f6ac..8c07096e 100644 --- a/policy/policy.go +++ b/policy/policy.go @@ -117,21 +117,29 @@ func (a Args) GetRoleArn() string { } // Policy - iam bucket iamp. +// +// The fields below Statements are derived from it by updateActionIndex, so +// changing Statements on a parsed policy leaves them stale. Call Reindex after +// doing so. type Policy struct { - ID ID `json:"ID,omitempty"` - Version string - Statements []Statement `json:"Statement"` + ID ID `json:"ID,omitempty"` + Version string + Statements []Statement `json:"Statement"` + + // Statement indexes keyed by the actions they name literally, so a request + // action can skip statements that cannot match it. Decide falls back to the + // full walk when the highest index no longer addresses Statements. actionStatementIndex map[Action][]int - hasDeny bool + + // Whether any statement carries a Deny. See HasDenyStatement for why this + // is only a shortcut. + hasDeny bool } -// HasDenyStatement returns if the policy has a deny statement. -// -// hasDeny is only populated by updateActionIndex, which every policy this -// package builds goes through, but which a policy a caller assembles as a -// struct literal never reaches. So the field is a shortcut and the statements -// stay the authority: trusting the field alone would silently under-report a -// Deny on such a policy. +// HasDenyStatement returns if the policy has a deny statement. hasDeny is only +// a shortcut, filled in by updateActionIndex; the statements stay authoritative +// because a policy assembled directly never reaches that path, and trusting the +// field alone would silently under-report its Deny. func (iamp *Policy) HasDenyStatement() bool { if iamp.hasDeny { return true @@ -552,10 +560,24 @@ func (iamp Policy) ValidateStrict() error { return nil } +// Reindex reclassifies the statements and rebuilds the action index. Call it +// after changing Statements on a policy that has already been parsed: the +// derived state is what decides whether a statement's Resources are matched at +// all, so leaving it stale can skip that check. It also moves a policy +// assembled by hand onto the indexed evaluation path. +func (iamp *Policy) Reindex() { + iamp.updateActionIndex() +} + // updateActionIndex with latest statements() // maintains a reverse map of Action -> []Statements // for faster lookup and short-circuit. +// +// Everything it derives is discarded first, so calling it a second time +// replaces the previous result rather than accumulating onto it. func (iamp *Policy) updateActionIndex() { + iamp.actionStatementIndex = nil + iamp.hasDeny = false for i := range iamp.Statements { stmt := &iamp.Statements[i] stmt.class = stmt.computeClass() diff --git a/policy/policy_regression_test.go b/policy/policy_regression_test.go index 79e2e16d..4592dcb3 100644 --- a/policy/policy_regression_test.go +++ b/policy/policy_regression_test.go @@ -18,7 +18,10 @@ package policy import ( + "bytes" "fmt" + "maps" + "slices" "testing" "github.com/minio/pkg/v3/policy/condition" @@ -107,6 +110,65 @@ func TestHasDenyStatementOnStructLiteralPolicy(t *testing.T) { } } +// updateActionIndex appended to the action index and only ever set hasDeny to +// true, so a second pass accumulated onto the first instead of replacing it. +// Reindex is the exported way to re-derive that state, so repeating it has to +// land on the same answer as deriving it once. +func TestReindexIsIdempotent(t *testing.T) { + doc := `{"Version":"2012-10-17","Statement":[ + {"Effect":"Allow","Action":["s3:GetObject"],"Resource":["arn:aws:s3:::b/*"]}, + {"Effect":"Deny","Action":["s3:PutObject"],"Resource":["arn:aws:s3:::b/*"]}]}` + p, err := ParseConfig(bytes.NewReader([]byte(doc))) + if err != nil { + t.Fatal(err) + } + want := maps.Clone(p.actionStatementIndex) + for i := range 3 { + p.Reindex() + if !maps.EqualFunc(p.actionStatementIndex, want, slices.Equal) { + t.Fatalf("Reindex %d: index = %v, want %v", i+1, p.actionStatementIndex, want) + } + if !p.hasDeny { + t.Fatalf("Reindex %d: hasDeny cleared on a policy that carries a Deny", i+1) + } + } + + // Dropping the Deny has to clear the flag, not leave it latched on. + p.Statements = p.Statements[:1] + p.Reindex() + if p.hasDeny { + t.Error("hasDeny still set after the only Deny statement was removed") + } + if p.HasDenyStatement() { + t.Error("HasDenyStatement() true after the only Deny statement was removed") + } +} + +// Replacing Actions on a parsed policy leaves the cached statement class +// describing the old namespace, and that class decides whether Resources are +// matched at all -- an admin statement skips the check. So a statement switched +// from an admin action to an S3 one must stop being resource-exempt once +// Reindex has run. Without the Reindex call below, the first check fails. +func TestReindexRefreshesStatementClassAfterActionChange(t *testing.T) { + doc := `{"Version":"2012-10-17","Statement":[{"Effect":"Allow","Action":["admin:ServerInfo"],"Resource":[]}]}` + p, err := ParseConfig(bytes.NewReader([]byte(doc))) + if err != nil { + t.Fatal(err) + } + + // An S3 action confined to bucket "other" must not reach bucket "b". + p.Statements[0].Actions = NewActionSet(GetObjectAction) + p.Statements[0].Resources = NewResourceSet(NewResource("other/*")) + p.Reindex() + + if p.IsAllowed(Args{Action: GetObjectAction, BucketName: "b", ObjectName: "o"}) { + t.Error("GetObject on b/o allowed by a statement scoped to other/*: stale admin class skipped resource matching") + } + if !p.IsAllowed(Args{Action: GetObjectAction, BucketName: "other", ObjectName: "o"}) { + t.Error("GetObject on other/o denied by a statement scoped to other/*") + } +} + // Decide's DenyOnly and IsOwner fallthroughs sit below the Deny loop, so a // policy with nothing to match must still reach them. Short-circuiting on an // empty statement list turns every STS login for such a credential into diff --git a/policy/statement.go b/policy/statement.go index 94f0838c..d7af97e5 100644 --- a/policy/statement.go +++ b/policy/statement.go @@ -41,6 +41,12 @@ type Statement struct { // means "not classified yet" — classify() then derives it on the spot, so a // statement built by a path that does not populate it is slower, never // wrong. Statement.Clone deliberately leaves it zero. + // + // Replacing Actions on a statement already held by a parsed policy leaves + // this stale, and the namespace decides whether Resources are matched at + // all, so a stale one can skip that check. Call Policy.Reindex after such a + // change; recomputing per evaluation instead costs 60-160% on the + // authorization path, which is why the value is cached at all. class statementClass } @@ -54,8 +60,9 @@ func (statement Statement) IsAllowed(args Args) bool { return statement.IsAllowedPtr(&args) } -// buildRequestResource renders the resource string an args names, in the form -// ResourceSet.Match expects. +// buildRequestResource is called once per request and its result passed down, +// because every statement of every policy matches against the same string and +// rebuilding it per statement dominated allocation on the authorization path. func buildRequestResource(args *Args) string { buf := smallBufPool.Get().(*bytes.Buffer) defer smallBufPool.Put(buf) From bebd595d3868660bcc7a022015318d50db989025 Mon Sep 17 00:00:00 2001 From: Klaus Post Date: Fri, 21 Aug 2026 14:44:24 +0200 Subject: [PATCH 5/6] Update internal docs --- policy/policy.go | 9 +++------ 1 file changed, 3 insertions(+), 6 deletions(-) diff --git a/policy/policy.go b/policy/policy.go index 8c07096e..ed982728 100644 --- a/policy/policy.go +++ b/policy/policy.go @@ -569,12 +569,9 @@ func (iamp *Policy) Reindex() { iamp.updateActionIndex() } -// updateActionIndex with latest statements() -// maintains a reverse map of Action -> []Statements -// for faster lookup and short-circuit. -// -// Everything it derives is discarded first, so calling it a second time -// replaces the previous result rather than accumulating onto it. +// updateActionIndex fills the Action -> []Statements reverse lookup by +// appending, so it discards the derived state first: a repeat call has to +// replace the previous result rather than accumulate onto it. func (iamp *Policy) updateActionIndex() { iamp.actionStatementIndex = nil iamp.hasDeny = false From c4b8101308583b1fe0c444e5b60bf43e9ce1940d Mon Sep 17 00:00:00 2001 From: Klaus Post Date: Mon, 24 Aug 2026 15:39:11 +0200 Subject: [PATCH 6/6] Apply suggestions --- policy/admin-action.go | 10 +++++----- policy/policy.go | 23 +++++++++++------------ policy/statement.go | 25 ++++++++++--------------- wildcard/match.go | 8 +++----- 4 files changed, 29 insertions(+), 37 deletions(-) diff --git a/policy/admin-action.go b/policy/admin-action.go index 8cd7a28e..50152d90 100644 --- a/policy/admin-action.go +++ b/policy/admin-action.go @@ -476,11 +476,7 @@ func (action AdminAction) Match(a AdminAction) bool { // IsValid - checks if action is valid or not. // // The receiver is the pattern, so this asks whether the pattern matches any -// supported admin action. Statement.isAdmin calls it for every action of every -// statement it evaluates, so the two cases that can be answered without -// touching SupportedAdminActions are answered first: a literal action can only -// match by being in the set, and a pattern is only worth scanning when its -// literal head is prefix-compatible with the admin namespace. +// supported admin action. func (action AdminAction) IsValid() bool { if _, ok := SupportedAdminActions[action]; ok { return true @@ -488,9 +484,13 @@ func (action AdminAction) IsValid() bool { s := string(action) star := strings.IndexAny(s, "*?") if star < 0 { + // A literal action can only match by being in the set above. return false } if !canMatchPrefix(s[:star], adminActionPrefix) { + // computeClass runs this for every action of every statement it + // classifies, so a pattern is only scanned against the whole set when + // its literal head is prefix-compatible with the admin namespace. return false } for supAction := range SupportedAdminActions { diff --git a/policy/policy.go b/policy/policy.go index ed982728..00b951fe 100644 --- a/policy/policy.go +++ b/policy/policy.go @@ -118,9 +118,8 @@ func (a Args) GetRoleArn() string { // Policy - iam bucket iamp. // -// The fields below Statements are derived from it by updateActionIndex, so -// changing Statements on a parsed policy leaves them stale. Call Reindex after -// doing so. +// Reindex must be called after changing Statements on a policy that has already +// been parsed, because the state derived from them is otherwise left stale. type Policy struct { ID ID `json:"ID,omitempty"` Version string @@ -131,15 +130,14 @@ type Policy struct { // full walk when the highest index no longer addresses Statements. actionStatementIndex map[Action][]int - // Whether any statement carries a Deny. See HasDenyStatement for why this - // is only a shortcut. + // Whether any statement carries a Deny. Only a shortcut, filled in by + // updateActionIndex; the statements stay authoritative because a policy + // assembled directly never reaches that path, and trusting the field alone + // would silently under-report its Deny. hasDeny bool } -// HasDenyStatement returns if the policy has a deny statement. hasDeny is only -// a shortcut, filled in by updateActionIndex; the statements stay authoritative -// because a policy assembled directly never reaches that path, and trusting the -// field alone would silently under-report its Deny. +// HasDenyStatement returns if the policy has a deny statement. func (iamp *Policy) HasDenyStatement() bool { if iamp.hasDeny { return true @@ -569,10 +567,11 @@ func (iamp *Policy) Reindex() { iamp.updateActionIndex() } -// updateActionIndex fills the Action -> []Statements reverse lookup by -// appending, so it discards the derived state first: a repeat call has to -// replace the previous result rather than accumulate onto it. +// updateActionIndex fills the Action -> []Statements reverse lookup used to +// skip statements that cannot match a request action. func (iamp *Policy) updateActionIndex() { + // Both are appended to below, so a repeat call has to start from nothing + // rather than accumulate onto the previous result. iamp.actionStatementIndex = nil iamp.hasDeny = false for i := range iamp.Statements { diff --git a/policy/statement.go b/policy/statement.go index d7af97e5..8f96df94 100644 --- a/policy/statement.go +++ b/policy/statement.go @@ -37,16 +37,11 @@ type Statement struct { NotResources ResourceSet `json:"NotResource,omitempty"` Conditions condition.Functions `json:"Condition,omitempty"` - // Namespace classification, filled in by Policy.updateActionIndex. Zero - // means "not classified yet" — classify() then derives it on the spot, so a - // statement built by a path that does not populate it is slower, never - // wrong. Statement.Clone deliberately leaves it zero. - // - // Replacing Actions on a statement already held by a parsed policy leaves - // this stale, and the namespace decides whether Resources are matched at - // all, so a stale one can skip that check. Call Policy.Reindex after such a - // change; recomputing per evaluation instead costs 60-160% on the - // authorization path, which is why the value is cached at all. + // Namespace classification, filled in by Policy.updateActionIndex and + // cached because recomputing it per evaluation is expensive. Zero means + // "not classified yet". Replacing Actions on a statement already held by a + // parsed policy leaves this stale, and the namespace decides whether + // Resources are matched at all, so call Policy.Reindex after such a change. class statementClass } @@ -221,7 +216,8 @@ const ( func (c statementClass) has(f statementClass) bool { return c&f != 0 } // classify reports the statement's namespaces, using the value cached at parse -// time when there is one. +// time when there is one and deriving it on the spot otherwise — slower, never +// wrong. func (statement Statement) classify() statementClass { if statement.class != 0 { return statement.class @@ -229,9 +225,7 @@ func (statement Statement) classify() statementClass { return statement.computeClass() } -// computeClass walks the action set once. The predicates it stands in for on -// the authorization path each walked it separately, so a statement paid four -// map iterator setups per evaluation to learn what one pass can tell it. +// computeClass walks the action set once. func (statement Statement) computeClass() statementClass { c := classKnown for action := range statement.Actions { @@ -638,7 +632,8 @@ func (statement Statement) Equals(st Statement) bool { return true } -// Clone clones Statement structure +// Clone clones Statement structure. The clone carries no cached namespace +// classification. func (statement Statement) Clone() Statement { return Statement{ SID: statement.SID, diff --git a/wildcard/match.go b/wildcard/match.go index ae580032..17e04ee1 100644 --- a/wildcard/match.go +++ b/wildcard/match.go @@ -87,11 +87,9 @@ func Has(pattern string) bool { return strings.ContainsAny(pattern, "*?") } -// deepMatchRune walks pattern against str one byte at a time, remembering only -// the most recent '*' to resume from. Trying both alternatives at every '*' -// instead — as a recursive matcher does — costs time exponential in the number -// of stars, which a caller-supplied pattern can trigger: a policy action of -// "*********x" kept AdminAction.IsValid busy for a minute. +// deepMatchRune matches pattern against str in a single pass, backtracking to +// the most recent '*'. Recursing on both branches at each '*' is exponential in +// the star count, which a caller-supplied pattern can trigger. func deepMatchRune(str, pattern string) bool { var s, p int // Position of the '*' to resume from, and how much of str it has consumed.