Fix policy validation & improve speed

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 {

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 <http://www.gnu.org/licenses/>.
+
+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)
+		}
+	})
+}

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

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 <http://www.gnu.org/licenses/>.
+
+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)
+			}
+		})
+	}
+}