From 3d3199e5882645ea5daf49dd14d8bc0ceff7219d Mon Sep 17 00:00:00 2001 From: Denis Babchuk Date: Sat, 17 Oct 2026 19:48:49 +0300 Subject: [PATCH] perf(policy): add fast paths to AdminAction.IsValid and HasResource Co-authored-by: Klaus Post Signed-off-by: Denis Babchuk --- policy/admin-action.go | 36 +++++++ policy/admin-action_test.go | 193 ++++++++++++++++++++++++++++++++++++ policy/policy_bench_test.go | 23 +++++ 3 files changed, 252 insertions(+) create mode 100644 policy/admin-action_test.go diff --git a/policy/admin-action.go b/policy/admin-action.go index fcf4665d..bcaf01b0 100644 --- a/policy/admin-action.go +++ b/policy/admin-action.go @@ -18,6 +18,8 @@ package policy import ( + "strings" + "github.com/pgsty/silo-pkg/v3/policy/condition" "github.com/pgsty/silo-pkg/v3/wildcard" ) @@ -425,6 +427,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)) { + // No wildcards, so only an exact match counts. + return false + } for a := range AdminActionsWithResource { if action.Match(a) { return true @@ -440,6 +449,20 @@ func (action AdminAction) Match(a AdminAction) bool { // IsValid - checks if action is valid or not. func (action AdminAction) IsValid() bool { + if _, ok := SupportedAdminActions[action]; ok { + return true + } + s := string(action) + star := strings.IndexAny(s, "*?") + if star < 0 { + // No wildcards, so only an exact match counts. + return false + } + if !canMatchPrefix(s[:star], adminActionPrefix) { + // This runs on every request, so don't scan the whole set for + // patterns like "s3:*" that can never be admin actions. + return false + } for supAction := range SupportedAdminActions { if action.Match(supAction) { return true @@ -448,6 +471,19 @@ func (action AdminAction) IsValid() bool { return false } +// Every supported admin action starts with this. +const adminActionPrefix = "admin:" + +// canMatchPrefix reports whether a pattern whose text before the first +// wildcard is head can match a string starting with prefix. Either one may +// be longer: "adm*" and "admin:Heal*" both pass. +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..7fc544c7 --- /dev/null +++ b/policy/admin-action_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 ( + "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 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) { + 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/policy_bench_test.go b/policy/policy_bench_test.go index 4ca51e48..fec49e25 100644 --- a/policy/policy_bench_test.go +++ b/policy/policy_bench_test.go @@ -91,6 +91,29 @@ func BenchmarkIsAllowed(b *testing.B) { }()), args: args, }, + { + // Every statement matches the action but not the bucket. + name: "ManyBucketGrants", + policy: setupPolicy(func() []Statement { + stmts := make([]Statement, 100) + for i := range stmts { + stmts[i] = setupStatement( + []string{"s3:GetObject", "s3:PutObject"}, + []string{fmt.Sprintf("arn:aws:s3:::bucket%d/*", i)}, + "Allow", + nil, + ) + } + stmts = append(stmts, setupStatement( + []string{"s3:GetObject"}, + []string{"arn:aws:s3:::test-bucket/*"}, + "Allow", + nil, + )) + return stmts + }()), + args: args, + }, { name: "DenyRule", policy: setupPolicy([]Statement{