From 6f0271d60f785c11f962f8381505822550721d84 Mon Sep 17 00:00:00 2001 From: Taran Pelkey Date: Wed, 16 Sep 2026 15:53:58 -0500 Subject: [PATCH 1/4] Allow escaped policy literals --- policy/condition/stringfunc.go | 22 ++--- policy/condition/stringfunc_test.go | 42 ++++++++++ policy/condition/substitute.go | 119 ++++++++++++++++++++++++++++ policy/condition/substitute_test.go | 95 ++++++++++++++++++++++ policy/resource.go | 49 ++---------- policy/resource_test.go | 42 ++++++++++ wildcard/match.go | 75 ++++++++++++++++++ wildcard/match_equivalence_test.go | 23 ++++++ wildcard/match_test.go | 68 ++++++++++++++++ 9 files changed, 483 insertions(+), 52 deletions(-) create mode 100644 policy/condition/substitute.go create mode 100644 policy/condition/substitute_test.go diff --git a/policy/condition/stringfunc.go b/policy/condition/stringfunc.go index 9644804..89ec509 100644 --- a/policy/condition/stringfunc.go +++ b/policy/condition/stringfunc.go @@ -28,15 +28,19 @@ import ( "github.com/minio/pkg/v3/wildcard" ) +// substitute expands policy variables in a condition value that is compared as +// text. func substitute(values map[string][]string) func(string) string { return func(v string) string { - for _, key := range CommonKeys { - // Empty values are not supported for policy variables. - if rvalues, ok := values[key.Name()]; ok && rvalues[0] != "" { - v = strings.ReplaceAll(v, key.VarName(), rvalues[0]) - } - } - return v + return Substitute(v, values, false) + } +} + +// substitutePattern expands policy variables in a condition value that is +// wildcard matched. The result is a pattern for wildcard.MatchEscaped. +func substitutePattern(values map[string][]string) func(string) string { + return func(v string) string { + return Substitute(v, values, true) } } @@ -131,9 +135,9 @@ type stringLikeFunc struct { func (f stringLikeFunc) eval(values map[string][]string) bool { rvalues := getValuesByKey(values, f.k) - fvalues := f.values.ApplyFunc(substitute(values)) + fvalues := f.values.ApplyFunc(substitutePattern(values)) for _, v := range rvalues { - matched := !fvalues.FuncMatch(wildcard.Match, v).IsEmpty() + matched := !fvalues.FuncMatch(wildcard.MatchEscaped, v).IsEmpty() if f.n.qualifier == forAllValues { if !matched { return false diff --git a/policy/condition/stringfunc_test.go b/policy/condition/stringfunc_test.go index 8d602c5..9f558ce 100644 --- a/policy/condition/stringfunc_test.go +++ b/policy/condition/stringfunc_test.go @@ -817,3 +817,45 @@ func TestNewStringFuncError(t *testing.T) { t.Errorf("error expected") } } + +func TestStringFuncEscapes(t *testing.T) { + testCases := []struct { + condValue string + reqValue string + wantEqual bool + wantLike bool + }{ + // ${*} is a literal asterisk to both operators. Only StringLike reads + // an unescaped '*' as a wildcard. + {"prefix${*}", "prefix*", true, true}, + {"prefix${*}", "prefixfoo", false, false}, + {"prefix*", "prefixfoo", false, true}, + {"${?}", "?", true, true}, + {"${?}", "a", false, false}, + {"${$}{aws:username}", "${aws:username}", true, true}, + {"${$}{aws:username}", "david", false, false}, + {"${aws:username}", "david", true, true}, + } + + for _, tc := range testCases { + t.Run(tc.condValue+"|"+tc.reqValue, func(t *testing.T) { + reqValues := map[string][]string{"prefix": {tc.reqValue}, "username": {"david"}} + + eqFunc, err := NewStringEqualsFunc("", S3Prefix.ToKey(), tc.condValue) + if err != nil { + t.Fatalf("NewStringEqualsFunc: %v", err) + } + if got := eqFunc.evaluate(reqValues); got != tc.wantEqual { + t.Fatalf("StringEquals(%q).evaluate(%q) = %v, want %v", tc.condValue, tc.reqValue, got, tc.wantEqual) + } + + likeFunc, err := newStringLikeFunc(S3Prefix.ToKey(), NewValueSet(NewStringValue(tc.condValue)), "") + if err != nil { + t.Fatalf("newStringLikeFunc: %v", err) + } + if got := likeFunc.evaluate(reqValues); got != tc.wantLike { + t.Fatalf("StringLike(%q).evaluate(%q) = %v, want %v", tc.condValue, tc.reqValue, got, tc.wantLike) + } + }) + } +} diff --git a/policy/condition/substitute.go b/policy/condition/substitute.go new file mode 100644 index 0000000..7ae7304 --- /dev/null +++ b/policy/condition/substitute.go @@ -0,0 +1,119 @@ +// Copyright (c) 2015-2025 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 condition + +import ( + "bytes" + "strings" + "sync" +) + +// Predefined policy variables, each expanding to one literal character. A +// policy uses them where it needs the character itself rather than a wildcard +// or the start of a variable. +const ( + // VarAsterisk expands to a literal '*'. + VarAsterisk = "${*}" + + // VarQuestion expands to a literal '?'. + VarQuestion = "${?}" + + // VarDollar expands to a literal '$'. + VarDollar = "${$}" +) + +var substBufPool = sync.Pool{ + New: func() any { return &bytes.Buffer{} }, +} + +// Substitute expands the policy variables in pattern using the request's +// condition values. A variable that is unknown or carries no value is left as +// written. An expanded value is never scanned for further variables. +// +// The escapes ${*}, ${?} and ${$} expand to a literal character. Set escape to +// match the result with wildcard.MatchEscaped. Leave it unset to compare the +// result as text. +func Substitute(pattern string, values map[string][]string, escape bool) string { + idx := strings.IndexByte(pattern, '$') + if idx < 0 { + if !escape || !strings.Contains(pattern, `\`) { + return pattern + } + idx = len(pattern) + } + + buf := substBufPool.Get().(*bytes.Buffer) + defer substBufPool.Put(buf) + buf.Reset() + + writeText(buf, pattern[:idx], escape) + remain := pattern[idx:] + for len(remain) > 0 { + if remain[0] != '$' || len(remain) < 3 || remain[1] != '{' { + writeText(buf, remain[:1], escape) + remain = remain[1:] + continue + } + // No '}' in remain means none in any suffix of it either. Emit the + // rest and stop, instead of rescanning at every '${'. + keyEnds := strings.IndexByte(remain, '}') + if keyEnds < 0 { + writeText(buf, remain, escape) + break + } + + name := remain[2:keyEnds] + switch remain[:keyEnds+1] { + case VarAsterisk, VarQuestion, VarDollar: + if escape { + buf.WriteByte('\\') + } + buf.WriteString(name) + default: + ckey := KeyName(name) + // Only replace keys we know, and only when they carry a value. + if rvalues, ok := values[ckey.Name()]; CommonKeysMap[ckey] && ok && rvalues[0] != "" { + writeText(buf, rvalues[0], escape) + } else { + writeText(buf, remain[:keyEnds+1], escape) + } + } + remain = remain[keyEnds+1:] + } + + return buf.String() +} + +// writeText appends s to buf. With escape set, s is written so that it +// matches only itself. +func writeText(buf *bytes.Buffer, s string, escape bool) { + if !escape { + buf.WriteString(s) + return + } + for { + i := strings.IndexByte(s, '\\') + if i < 0 { + buf.WriteString(s) + return + } + buf.WriteString(s[:i]) + buf.WriteString(`\\`) + s = s[i+1:] + } +} diff --git a/policy/condition/substitute_test.go b/policy/condition/substitute_test.go new file mode 100644 index 0000000..64503d4 --- /dev/null +++ b/policy/condition/substitute_test.go @@ -0,0 +1,95 @@ +// Copyright (c) 2015-2025 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 condition + +import ( + "strings" + "testing" + "time" + + "github.com/minio/pkg/v3/wildcard" +) + +func TestSubstitute(t *testing.T) { + values := map[string][]string{ + "username": {"david"}, + "userid": {""}, + } + + // wantMatch, when set, is a string the escaped pattern must match. + testCases := []struct { + pattern string + wantLiteral string + wantEscaped string + wantMatch string + }{ + {"mybucket/foo", "mybucket/foo", "mybucket/foo", ""}, + {"mybucket/${aws:username}/*", "mybucket/david/*", "mybucket/david/*", ""}, + // A key with no value, or one that is not a known key, stays as written. + {"mybucket/${aws:userid}/*", "mybucket/${aws:userid}/*", "mybucket/${aws:userid}/*", ""}, + {"mybucket/${aws:nosuchkey}", "mybucket/${aws:nosuchkey}", "mybucket/${aws:nosuchkey}", ""}, + // The predefined escapes. + {"mybucket/${*}", "mybucket/*", `mybucket/\*`, "mybucket/*"}, + {"mybucket/${?}", "mybucket/?", `mybucket/\?`, "mybucket/?"}, + {"mybucket/${$}", "mybucket/$", `mybucket/\$`, "mybucket/$"}, + {"${*}${?}${$}", "*?$", `\*\?\$`, "*?$"}, + // An expansion is not rescanned, so ${$} does not build a variable. + {"${$}{aws:username}", "${aws:username}", `\${aws:username}`, "${aws:username}"}, + // A backslash the policy wrote stands for itself either way. + {`my\bucket/${*}`, `my\bucket/*`, `my\\bucket/\*`, `my\bucket/*`}, + {`my\bucket`, `my\bucket`, `my\\bucket`, `my\bucket`}, + // Incomplete variable syntax is emitted as written. + {"mybucket/$", "mybucket/$", "mybucket/$", ""}, + {"mybucket/${", "mybucket/${", "mybucket/${", ""}, + {"mybucket/${aws:username", "mybucket/${aws:username", "mybucket/${aws:username", ""}, + {"mybucket/${}", "mybucket/${}", "mybucket/${}", ""}, + {"mybucket/$${aws:username}", "mybucket/$david", "mybucket/$david", ""}, + } + + for _, tc := range testCases { + t.Run(tc.pattern, func(t *testing.T) { + if got := Substitute(tc.pattern, values, false); got != tc.wantLiteral { + t.Fatalf("Substitute(%q, false) = %q, want %q", tc.pattern, got, tc.wantLiteral) + } + if got := Substitute(tc.pattern, values, true); got != tc.wantEscaped { + t.Fatalf("Substitute(%q, true) = %q, want %q", tc.pattern, got, tc.wantEscaped) + } + if tc.wantMatch != "" && !wildcard.MatchEscaped(tc.wantEscaped, tc.wantMatch) { + t.Fatalf("escaped pattern %q does not match %q", tc.wantEscaped, tc.wantMatch) + } + }) + } +} + +// An unclosed '${' must not restart the scan for '}' one byte later. Doing so +// is quadratic in the pattern length. +func TestSubstituteUnterminatedVariablesAreLinear(t *testing.T) { + values := map[string][]string{"username": {"david"}} + + for _, n := range []int{1 << 10, 1 << 12, 1 << 14, 1 << 16} { + pattern := strings.Repeat("${", n) + start := time.Now() + got := Substitute(pattern, values, true) + if d := time.Since(start); d > 50*time.Millisecond { + t.Errorf("Substitute over %d bytes took %v, want well under 50ms", len(pattern), d) + } + if got != pattern { + t.Fatalf("Substitute(%d unterminated variables) rewrote the pattern", n) + } + } +} diff --git a/policy/resource.go b/policy/resource.go index 731d5fd..8b381f9 100644 --- a/policy/resource.go +++ b/policy/resource.go @@ -18,7 +18,6 @@ package policy import ( - "bytes" "encoding/json" "path" "strings" @@ -201,56 +200,20 @@ func (r Resource) Match(resource string, conditionValues map[string][]string) bo } } // Happy path, with no replacements - idx := strings.IndexByte(r.Pattern, '$') - if idx < 0 { + if strings.IndexByte(r.Pattern, '$') < 0 { if cp := path.Clean(resource); cp != "." && cp == r.Pattern { return true } return wildcard.Match(r.Pattern, resource) } - // Use a small buffer - pat := smallBufPool.Get().(*bytes.Buffer) - defer smallBufPool.Put(pat) - pat.Reset() - - // Do replacement of known keys. - pat.WriteString(r.Pattern[:idx]) - remain := r.Pattern[idx:] - for len(remain) > 0 { - val := remain[0] - if val != '$' || len(remain) < 3 { - pat.WriteByte(val) - remain = remain[1:] - continue - } - keyEnds := strings.IndexByte(remain, '}') - - // If no curly brackets, emit as-is. - if remain[1] != '{' || keyEnds < 0 { - pat.WriteByte('$') - remain = remain[1:] - continue - } - - ckey := condition.KeyName(remain[2:keyEnds]) - - // Only replace keys we know - if rvalues, ok := conditionValues[ckey.Name()]; condition.CommonKeysMap[ckey] && ok && rvalues[0] != "" { - pat.WriteString(rvalues[0]) - } else { - // Write without replacing... - pat.WriteString("${") - pat.WriteString(string(ckey)) - pat.WriteString("}") - } - remain = remain[keyEnds+1:] - } - pattern := pat.String() - if cp := path.Clean(resource); cp != "." && cp == pattern { + // A pattern can escape a literal '*', '?' or '$', so expand it and match + // the result as an escaped pattern. + pattern := condition.Substitute(r.Pattern, conditionValues, true) + if cp := path.Clean(resource); cp != "." && cp == wildcard.Unescape(pattern) { return true } - return wildcard.Match(pattern, resource) + return wildcard.MatchEscaped(pattern, resource) } // MarshalJSON - encodes Resource to JSON data. diff --git a/policy/resource_test.go b/policy/resource_test.go index e2a7758..72aba65 100644 --- a/policy/resource_test.go +++ b/policy/resource_test.go @@ -325,3 +325,45 @@ func TestResourceValidateBucket(t *testing.T) { } } } + +func TestResourceMatchEscapes(t *testing.T) { + conditionValues := map[string][]string{"username": {"david"}} + + testCases := []struct { + pattern string + resource string + want bool + }{ + // ${*} stands for a literal asterisk, not for the wildcard. + {"mybucket/${*}", "mybucket/*", true}, + {"mybucket/${*}", "mybucket/foo", false}, + {"mybucket/${*}/*", "mybucket/*/foo", true}, + {"mybucket/${*}/*", "mybucket/bar/foo", false}, + // ${?} stands for a literal question mark. + {"mybucket/${?}.txt", "mybucket/?.txt", true}, + {"mybucket/${?}.txt", "mybucket/a.txt", false}, + // ${$} stands for a literal dollar sign. What follows it is not read + // as a variable. + {"mybucket/${$}{aws:username}", "mybucket/${aws:username}", true}, + {"mybucket/${$}{aws:username}", "mybucket/david", false}, + {"mybucket/${$}100", "mybucket/$100", true}, + // Escapes and variables in the same pattern. + {"mybucket/${aws:username}/${*}", "mybucket/david/*", true}, + {"mybucket/${aws:username}/${*}", "mybucket/david/x", false}, + // An unescaped wildcard keeps its meaning alongside an escape. + {"mybucket/${*}-*", "mybucket/*-anything", true}, + {"mybucket/${*}-*", "mybucket/x-anything", false}, + // A backslash in the pattern is literal, not an escape of its own. + {`mybucket/a\b/${*}`, `mybucket/a\b/*`, true}, + {`mybucket/a\b/${*}`, `mybucket/a\bx`, false}, + } + + for _, tc := range testCases { + t.Run(tc.pattern+"|"+tc.resource, func(t *testing.T) { + r := NewResource(tc.pattern) + if got := r.Match(tc.resource, conditionValues); got != tc.want { + t.Fatalf("Resource(%q).Match(%q) = %v, want %v", tc.pattern, tc.resource, got, tc.want) + } + }) + } +} diff --git a/wildcard/match.go b/wildcard/match.go index 17e04ee..8f69885 100644 --- a/wildcard/match.go +++ b/wildcard/match.go @@ -156,3 +156,78 @@ func MatchAsPatternPrefix(pattern, text string) bool { } return len(text) <= len(pattern) } + +// MatchEscaped is Match with backslash escaping. A '\' in the pattern makes +// the next byte literal, so `\*` matches a single '*' rather than any run of +// characters. A '\' at the end of the pattern matches itself. +func MatchEscaped(pattern, name string) bool { + if pattern == "" { + return name == pattern + } + if pattern == "*" { + return true + } + return deepMatchEscaped(name, pattern) +} + +// Unescape drops the backslashes MatchEscaped reads as escapes. +func Unescape(pattern string) string { + idx := strings.IndexByte(pattern, '\\') + if idx < 0 { + return pattern + } + var sb strings.Builder + sb.Grow(len(pattern)) + sb.WriteString(pattern[:idx]) + for i := idx; i < len(pattern); i++ { + if pattern[i] == '\\' && i+1 < len(pattern) { + i++ + } + sb.WriteByte(pattern[i]) + } + return sb.String() +} + +// deepMatchEscaped matches str against pattern, treating a byte after a +// backslash as literal. +func deepMatchEscaped(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) { + c, width, literal := pattern[p], 1, false + if c == '\\' && p+1 < len(pattern) { + c, width, literal = pattern[p+1], 2, true + } + switch { + case c == '*' && !literal: + star, mark = p, s + p++ + continue + case c == '?' && !literal: + if s < len(str) { + s++ + p++ + continue + } + default: + if s < len(str) && c == str[s] { + s++ + p += width + continue + } + } + } + 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 true +} diff --git a/wildcard/match_equivalence_test.go b/wildcard/match_equivalence_test.go index 1fbccb1..15df988 100644 --- a/wildcard/match_equivalence_test.go +++ b/wildcard/match_equivalence_test.go @@ -123,6 +123,23 @@ func oldMatchSimple(pattern, name string) bool { return oldDeepMatchRune(name, pattern, true) } +// MatchEscaped copies deepMatchRune's loop, so the two can drift apart. On a +// pattern with no backslash they must agree. +func TestMatchEscapedEquivalenceExhaustive(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 := MatchEscaped(p, name), Match(p, name); got != want { + t.Fatalf("MatchEscaped(%q, %q) = %v, Match = %v", p, name, got, want) + } + n++ + } + } + t.Logf("%d backslash-free combinations agree with Match", n) +} + // A pattern with many stars used to take time exponential in the star count. func TestMatchStarsAreLinear(t *testing.T) { name := "admin:ServerInfo" @@ -192,5 +209,11 @@ func FuzzDeepMatchEquivalence(f *testing.F) { if got, want := MatchSimple(pattern, name), oldMatchSimple(pattern, name); got != want { t.Fatalf("MatchSimple(%q, %q) = %v, old = %v", pattern, name, got, want) } + // The two agree only where the escape has nothing to act on. + if !strings.Contains(pattern, `\`) { + if got, want := MatchEscaped(pattern, name), Match(pattern, name); got != want { + t.Fatalf("MatchEscaped(%q, %q) = %v, Match = %v", pattern, name, got, want) + } + } }) } diff --git a/wildcard/match_test.go b/wildcard/match_test.go index 142bad7..f83e7cf 100644 --- a/wildcard/match_test.go +++ b/wildcard/match_test.go @@ -829,3 +829,71 @@ func TestMatchAsPatternPrefix(t *testing.T) { } } } + +func TestMatchEscaped(t *testing.T) { + testCases := []struct { + pattern string + name string + want bool + }{ + {`a\*b`, "a*b", true}, + {`a\*b`, "axb", false}, + {`a\*b`, "axxxb", false}, + {`a\?b`, "a?b", true}, + {`a\?b`, "axb", false}, + {`a\$b`, "a$b", true}, + {`a\\b`, `a\b`, true}, + {`a\\b`, "ab", false}, + {`\*`, "*", true}, + {`\**`, "*anything", true}, + {`\**`, "anything", false}, + {`*\*`, "anything*", true}, + {`*\*`, "anything", false}, + // A backslash with nothing after it stands for itself. + {`ab\`, `ab\`, true}, + // A '*' or '?' that is not escaped keeps its wildcard meaning. + {`a*c\?`, "abbbc?", true}, + {`a*c\?`, "abbbcd", false}, + {`?\?`, "a?", true}, + {`?\?`, "ab", false}, + // Backtracking still has to skip over an escape. + {`*\*z`, "a*b*z", true}, + {`*\*z`, "a*b*y", false}, + // Patterns without escapes behave exactly as under Match. + {"", "", true}, + {"", "a", false}, + {"*", "anything", true}, + {"a?c", "abc", true}, + {"a?c", "ac", false}, + } + + for _, tc := range testCases { + t.Run(tc.pattern+"|"+tc.name, func(t *testing.T) { + if got := MatchEscaped(tc.pattern, tc.name); got != tc.want { + t.Fatalf("MatchEscaped(%q, %q) = %v, want %v", tc.pattern, tc.name, got, tc.want) + } + }) + } +} + +func TestUnescape(t *testing.T) { + testCases := []struct { + pattern string + want string + }{ + {"abc", "abc"}, + {`a\*b`, "a*b"}, + {`a\?b\$c`, "a?b$c"}, + {`a\\b`, `a\b`}, + {`ab\`, `ab\`}, + {`\*\*`, "**"}, + } + + for _, tc := range testCases { + t.Run(tc.pattern, func(t *testing.T) { + if got := Unescape(tc.pattern); got != tc.want { + t.Fatalf("Unescape(%q) = %q, want %q", tc.pattern, got, tc.want) + } + }) + } +} From 9bead8d273fabbb260727a2d1ee9ad667849f203 Mon Sep 17 00:00:00 2001 From: Taran Pelkey Date: Tue, 22 Sep 2026 19:56:29 -0500 Subject: [PATCH 2/4] coderabbit --- policy/condition/substitute.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/policy/condition/substitute.go b/policy/condition/substitute.go index 7ae7304..38031d7 100644 --- a/policy/condition/substitute.go +++ b/policy/condition/substitute.go @@ -87,7 +87,7 @@ func Substitute(pattern string, values map[string][]string, escape bool) string default: ckey := KeyName(name) // Only replace keys we know, and only when they carry a value. - if rvalues, ok := values[ckey.Name()]; CommonKeysMap[ckey] && ok && rvalues[0] != "" { + if rvalues, ok := values[ckey.Name()]; CommonKeysMap[ckey] && ok && len(rvalues) > 0 && rvalues[0] != "" { writeText(buf, rvalues[0], escape) } else { writeText(buf, remain[:keyEnds+1], escape) From c5d466b9f1f4718a9ed9004689d100928f613bc0 Mon Sep 17 00:00:00 2001 From: Taran Pelkey Date: Wed, 23 Sep 2026 11:01:46 -0500 Subject: [PATCH 3/4] benchmark --- policy/condition/stringfunc_bench_test.go | 67 +++++++++++++++++++ policy/condition/substitute.go | 78 ++++++++++------------- policy/resource.go | 6 +- policy/resource_bench_test.go | 41 ++++++++++++ wildcard/match.go | 4 ++ 5 files changed, 151 insertions(+), 45 deletions(-) create mode 100644 policy/condition/stringfunc_bench_test.go create mode 100644 policy/resource_bench_test.go diff --git a/policy/condition/stringfunc_bench_test.go b/policy/condition/stringfunc_bench_test.go new file mode 100644 index 0000000..410f342 --- /dev/null +++ b/policy/condition/stringfunc_bench_test.go @@ -0,0 +1,67 @@ +// 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 condition + +import "testing" + +func BenchmarkStringLikeFuncEvaluate(b *testing.B) { + values := map[string][]string{ + "prefix": {"home/alice/docs/"}, + "username": {"alice"}, + } + benchCases := []struct { + name string + patterns []string + }{ + {"NoVariable", []string{"home/*", "public/*", "shared/*"}}, + {"Variable", []string{"home/${aws:username}/*", "public/*", "shared/*"}}, + {"Escape", []string{"home/${aws:username}/${*}", "public/*", "shared/*"}}, + } + for _, bc := range benchCases { + function, err := NewStringLikeFunc("", S3Prefix.ToKey(), bc.patterns...) + if err != nil { + b.Fatal(err) + } + b.Run(bc.name, func(b *testing.B) { + b.ReportAllocs() + for b.Loop() { + function.evaluate(values) + } + }) + } +} + +func BenchmarkSubstitute(b *testing.B) { + values := map[string][]string{"username": {"alice"}} + benchCases := []struct { + name, pattern string + }{ + {"NoVariable", "test-bucket/home/alice/*"}, + {"Variable", "test-bucket/home/${aws:username}/*"}, + {"UnknownVariable", "test-bucket/home/${aws:nosuchkey}/*"}, + {"Escape", `test-bucket/home/${aws:username}/${*}\file`}, + } + for _, bc := range benchCases { + b.Run(bc.name, func(b *testing.B) { + b.ReportAllocs() + for b.Loop() { + Substitute(bc.pattern, values, true) + } + }) + } +} diff --git a/policy/condition/substitute.go b/policy/condition/substitute.go index 38031d7..d8959ef 100644 --- a/policy/condition/substitute.go +++ b/policy/condition/substitute.go @@ -18,9 +18,7 @@ package condition import ( - "bytes" "strings" - "sync" ) // Predefined policy variables, each expanding to one literal character. A @@ -37,10 +35,6 @@ const ( VarDollar = "${$}" ) -var substBufPool = sync.Pool{ - New: func() any { return &bytes.Buffer{} }, -} - // Substitute expands the policy variables in pattern using the request's // condition values. A variable that is unknown or carries no value is left as // written. An expanded value is never scanned for further variables. @@ -49,71 +43,69 @@ var substBufPool = sync.Pool{ // match the result with wildcard.MatchEscaped. Leave it unset to compare the // result as text. func Substitute(pattern string, values map[string][]string, escape bool) string { - idx := strings.IndexByte(pattern, '$') - if idx < 0 { - if !escape || !strings.Contains(pattern, `\`) { - return pattern - } - idx = len(pattern) + if strings.IndexByte(pattern, '$') < 0 && (!escape || strings.IndexByte(pattern, '\\') < 0) { + return pattern } + var buf [128]byte + return string(AppendSubstitute(buf[:0], pattern, values, escape)) +} - buf := substBufPool.Get().(*bytes.Buffer) - defer substBufPool.Put(buf) - buf.Reset() - - writeText(buf, pattern[:idx], escape) - remain := pattern[idx:] - for len(remain) > 0 { - if remain[0] != '$' || len(remain) < 3 || remain[1] != '{' { - writeText(buf, remain[:1], escape) - remain = remain[1:] +// AppendSubstitute is Substitute that appends the result to dst, so a caller +// whose result stays local can expand without allocating. +func AppendSubstitute(dst []byte, pattern string, values map[string][]string, escape bool) []byte { + for len(pattern) > 0 { + idx := strings.IndexByte(pattern, '$') + if idx < 0 { + return appendText(dst, pattern, escape) + } + dst = appendText(dst, pattern[:idx], escape) + pattern = pattern[idx:] + if len(pattern) < 3 || pattern[1] != '{' { + dst = append(dst, '$') + pattern = pattern[1:] continue } - // No '}' in remain means none in any suffix of it either. Emit the + // No '}' in pattern means none in any suffix of it either. Emit the // rest and stop, instead of rescanning at every '${'. - keyEnds := strings.IndexByte(remain, '}') + keyEnds := strings.IndexByte(pattern, '}') if keyEnds < 0 { - writeText(buf, remain, escape) - break + return appendText(dst, pattern, escape) } - name := remain[2:keyEnds] - switch remain[:keyEnds+1] { + name := pattern[2:keyEnds] + switch pattern[:keyEnds+1] { case VarAsterisk, VarQuestion, VarDollar: if escape { - buf.WriteByte('\\') + dst = append(dst, '\\') } - buf.WriteString(name) + dst = append(dst, name...) default: ckey := KeyName(name) // Only replace keys we know, and only when they carry a value. if rvalues, ok := values[ckey.Name()]; CommonKeysMap[ckey] && ok && len(rvalues) > 0 && rvalues[0] != "" { - writeText(buf, rvalues[0], escape) + dst = appendText(dst, rvalues[0], escape) } else { - writeText(buf, remain[:keyEnds+1], escape) + dst = appendText(dst, pattern[:keyEnds+1], escape) } } - remain = remain[keyEnds+1:] + pattern = pattern[keyEnds+1:] } - - return buf.String() + return dst } -// writeText appends s to buf. With escape set, s is written so that it +// appendText appends s to dst. With escape set, s is written so that it // matches only itself. -func writeText(buf *bytes.Buffer, s string, escape bool) { +func appendText(dst []byte, s string, escape bool) []byte { if !escape { - buf.WriteString(s) - return + return append(dst, s...) } for { i := strings.IndexByte(s, '\\') if i < 0 { - buf.WriteString(s) - return + return append(dst, s...) } - buf.WriteString(s[:i]) - buf.WriteString(`\\`) + dst = append(dst, s[:i]...) + dst = append(dst, '\\', '\\') s = s[i+1:] } } diff --git a/policy/resource.go b/policy/resource.go index 8b381f9..f0ee131 100644 --- a/policy/resource.go +++ b/policy/resource.go @@ -208,8 +208,10 @@ func (r Resource) Match(resource string, conditionValues map[string][]string) bo } // A pattern can escape a literal '*', '?' or '$', so expand it and match - // the result as an escaped pattern. - pattern := condition.Substitute(r.Pattern, conditionValues, true) + // the result as an escaped pattern. Not Substitute: its returned string + // always escapes to the heap, while this one stays local to Match. + var buf [128]byte + pattern := string(condition.AppendSubstitute(buf[:0], r.Pattern, conditionValues, true)) if cp := path.Clean(resource); cp != "." && cp == wildcard.Unescape(pattern) { return true } diff --git a/policy/resource_bench_test.go b/policy/resource_bench_test.go new file mode 100644 index 0000000..c2feb09 --- /dev/null +++ b/policy/resource_bench_test.go @@ -0,0 +1,41 @@ +// 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 "testing" + +func BenchmarkResourceMatch(b *testing.B) { + conditionValues := map[string][]string{"username": {"alice"}} + benchCases := []struct { + name, pattern, resource string + }{ + {"NoVariable", "test-bucket/home/*", "test-bucket/home/alice/file.txt"}, + {"Variable", "test-bucket/home/${aws:username}/*", "test-bucket/home/alice/file.txt"}, + {"VariableLong", "test-bucket/some/deeper/prefix/path/home/${aws:username}/sub/*", "test-bucket/some/deeper/prefix/path/home/alice/sub/dir/file.txt"}, + {"Escape", "test-bucket/home/${aws:username}/${*}", "test-bucket/home/alice/*"}, + } + for _, bc := range benchCases { + r := NewResource(bc.pattern) + b.Run(bc.name, func(b *testing.B) { + b.ReportAllocs() + for b.Loop() { + r.Match(bc.resource, conditionValues) + } + }) + } +} diff --git a/wildcard/match.go b/wildcard/match.go index 8f69885..1e2db3a 100644 --- a/wildcard/match.go +++ b/wildcard/match.go @@ -167,6 +167,10 @@ func MatchEscaped(pattern, name string) bool { if pattern == "*" { return true } + // Most patterns escape nothing, and the plain matcher is cheaper per byte. + if strings.IndexByte(pattern, '\\') < 0 { + return deepMatchRune(name, pattern) + } return deepMatchEscaped(name, pattern) } From 08f4fd91d9222d9fa6586f76a13b14649feacf66 Mon Sep 17 00:00:00 2001 From: Taran Pelkey Date: Wed, 23 Sep 2026 11:52:49 -0500 Subject: [PATCH 4/4] Add more benchmarks --- policy/condition/stringfunc_bench_test.go | 18 +++++++++++------- policy/resource.go | 3 +-- policy/resource_bench_test.go | 5 ++++- 3 files changed, 16 insertions(+), 10 deletions(-) diff --git a/policy/condition/stringfunc_bench_test.go b/policy/condition/stringfunc_bench_test.go index 410f342..c0982a8 100644 --- a/policy/condition/stringfunc_bench_test.go +++ b/policy/condition/stringfunc_bench_test.go @@ -20,23 +20,27 @@ package condition import "testing" func BenchmarkStringLikeFuncEvaluate(b *testing.B) { - values := map[string][]string{ - "prefix": {"home/alice/docs/"}, - "username": {"alice"}, - } benchCases := []struct { name string + prefix string patterns []string }{ - {"NoVariable", []string{"home/*", "public/*", "shared/*"}}, - {"Variable", []string{"home/${aws:username}/*", "public/*", "shared/*"}}, - {"Escape", []string{"home/${aws:username}/${*}", "public/*", "shared/*"}}, + {"NoVariable", "home/alice/docs/", []string{"home/*", "public/*", "shared/*"}}, + {"Variable", "home/alice/docs/", []string{"home/${aws:username}/*", "public/*", "shared/*"}}, + {"Escape", "home/alice/*/docs/", []string{"home/${aws:username}/${*}/*", "public/*", "shared/*"}}, } for _, bc := range benchCases { function, err := NewStringLikeFunc("", S3Prefix.ToKey(), bc.patterns...) if err != nil { b.Fatal(err) } + values := map[string][]string{ + "prefix": {bc.prefix}, + "username": {"alice"}, + } + if !function.evaluate(values) { + b.Fatalf("%s: want a match", bc.name) + } b.Run(bc.name, func(b *testing.B) { b.ReportAllocs() for b.Loop() { diff --git a/policy/resource.go b/policy/resource.go index f0ee131..a4d51be 100644 --- a/policy/resource.go +++ b/policy/resource.go @@ -208,8 +208,7 @@ func (r Resource) Match(resource string, conditionValues map[string][]string) bo } // A pattern can escape a literal '*', '?' or '$', so expand it and match - // the result as an escaped pattern. Not Substitute: its returned string - // always escapes to the heap, while this one stays local to Match. + // the result as an escaped pattern. Uses AppendSubstitute for performance. var buf [128]byte pattern := string(condition.AppendSubstitute(buf[:0], r.Pattern, conditionValues, true)) if cp := path.Clean(resource); cp != "." && cp == wildcard.Unescape(pattern) { diff --git a/policy/resource_bench_test.go b/policy/resource_bench_test.go index c2feb09..480fab7 100644 --- a/policy/resource_bench_test.go +++ b/policy/resource_bench_test.go @@ -27,10 +27,13 @@ func BenchmarkResourceMatch(b *testing.B) { {"NoVariable", "test-bucket/home/*", "test-bucket/home/alice/file.txt"}, {"Variable", "test-bucket/home/${aws:username}/*", "test-bucket/home/alice/file.txt"}, {"VariableLong", "test-bucket/some/deeper/prefix/path/home/${aws:username}/sub/*", "test-bucket/some/deeper/prefix/path/home/alice/sub/dir/file.txt"}, - {"Escape", "test-bucket/home/${aws:username}/${*}", "test-bucket/home/alice/*"}, + {"Escape", "test-bucket/home/${aws:username}/${*}/*", "test-bucket/home/alice/*/file.txt"}, } for _, bc := range benchCases { r := NewResource(bc.pattern) + if !r.Match(bc.resource, conditionValues) { + b.Fatalf("%s: want a match", bc.name) + } b.Run(bc.name, func(b *testing.B) { b.ReportAllocs() for b.Loop() {