diff --git a/policy/admin-action.go b/policy/admin-action.go
index c7c013bd..50152d90 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.
func (action AdminAction) IsValid() bool {
+ if _, ok := SupportedAdminActions[action]; ok {
+ return true
+ }
+ 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 {
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/constants.go b/policy/constants.go
index beab9ac4..bfd02b31 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.Reindex()
+ }
+}
+
// 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
new file mode 100644
index 00000000..72321b90
--- /dev/null
+++ b/policy/parse_dos_test.go
@@ -0,0 +1,66 @@
+// 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"
+ "fmt"
+ "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(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()
+ _, err := ParseConfig(bytes.NewReader([]byte(doc)))
+ if d := time.Since(start); 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(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()
+ _, err := ParseBucketPolicyConfig(bytes.NewReader([]byte(doc)), "b")
+ if d := time.Since(start); 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 d865f646..00b951fe 100644
--- a/policy/policy.go
+++ b/policy/policy.go
@@ -117,17 +117,37 @@ func (a Args) GetRoleArn() string {
}
// Policy - iam bucket iamp.
+//
+// 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
- 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. 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.
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
@@ -187,9 +207,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
}
@@ -232,6 +253,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)
@@ -247,8 +269,11 @@ func IsAllowedPar(policies []Policy, args Args) bool {
maxJ := min(i+numPoliciesPerWorker, len(policies))
res := NoDecision
+ // 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(&args)
+ decision := policies[j].decide(&a, resource)
if decision == DenyDecision {
res = DenyDecision
break
@@ -298,9 +323,17 @@ 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 {
+ 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 _, 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 +353,34 @@ 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 {
+ // 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.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
}
}
@@ -510,12 +558,25 @@ func (iamp Policy) ValidateStrict() error {
return nil
}
-// updateActionIndex with latest statements()
-// maintains a reverse map of Action -> []Statements
-// for faster lookup and short-circuit.
+// 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 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 {
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..4592dcb3
--- /dev/null
+++ b/policy/policy_regression_test.go
@@ -0,0 +1,193 @@
+// 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"
+ "fmt"
+ "maps"
+ "slices"
+ "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 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 {
+ 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 {
+ 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")
+ }
+}
+
+// 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
+// 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..8f96df94 100644
--- a/policy/statement.go
+++ b/policy/statement.go
@@ -36,6 +36,13 @@ 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 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
}
// smallBufPool should always return a non-nil *bytes.Buffer
@@ -48,28 +55,42 @@ func (statement Statement) IsAllowed(args Args) bool {
return statement.IsAllowedPtr(&args)
}
+// 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)
+ 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, buildRequestResource(args))
+}
+
+// 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 +108,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 +116,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 +139,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 +194,60 @@ 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 and deriving it on the spot otherwise — slower, never
+// wrong.
+func (statement Statement) classify() statementClass {
+ if statement.class != 0 {
+ return statement.class
+ }
+ return statement.computeClass()
+}
+
+// computeClass walks the action set once.
+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() {
@@ -561,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,
@@ -638,6 +710,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..17e04ee1 100644
--- a/wildcard/match.go
+++ b/wildcard/match.go
@@ -18,7 +18,6 @@
package wildcard
import (
- "cmp"
"strings"
)
@@ -33,8 +32,39 @@ 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.
+ 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] == '?' && (i > firstStar || i == len(name)) &&
+ deepMatchRune(name, pattern[:i]) {
+ return true
+ }
+ if pattern[i] != '*' {
+ lits++
+ if lits > len(name) {
+ break
+ }
+ }
+ }
+ return false
}
// Match - finds whether the text matches/satisfies the pattern string.
@@ -49,34 +79,53 @@ 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).
func Has(pattern string) bool {
- return cmp.Or(strings.Contains(pattern, "*"), strings.Contains(pattern, "?"))
+ return strings.ContainsAny(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 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.
+ 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..1fbccb11
--- /dev/null
+++ b/wildcard/match_equivalence_test.go
@@ -0,0 +1,196 @@
+// 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"
+ name = strings.Repeat("a", 128)
+ start := time.Now()
+ 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")
+ }
+ 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)
+ }
+ })
+}