From 1d866206510f0759ff4749ca6b4c4a7e7e185622 Mon Sep 17 00:00:00 2001 From: Harshavardhana Date: Fri, 25 Sep 2026 19:49:28 -0700 Subject: [PATCH 1/4] policy: add admin:PolicyName condition key Policy admin actions (admin:CreatePolicy, admin:DeletePolicy, admin:GetPolicy) take no resource, so a principal that may manage policies may manage every policy, including its own, and can widen itself. The admin:PolicyName condition key names the policy the action works on, so a statement can limit those actions to a set of policies: "Condition": {"StringLike": {"admin:PolicyName": ["app-*"]}} The server fills PolicyName in for the policy admin handlers. --- policy/admin_policy_name_test.go | 96 ++++++++++++++++++++++++++++++++ policy/condition/keyname.go | 9 +++ 2 files changed, 105 insertions(+) create mode 100644 policy/admin_policy_name_test.go diff --git a/policy/admin_policy_name_test.go b/policy/admin_policy_name_test.go new file mode 100644 index 0000000..f0d5316 --- /dev/null +++ b/policy/admin_policy_name_test.go @@ -0,0 +1,96 @@ +// 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" +) + +// A policy admin can be limited to a set of policies by name. +func TestAdminPolicyNameCondition(t *testing.T) { + doc := `{ + "Version": "2012-10-17", + "Statement": [{ + "Effect": "Allow", + "Action": ["admin:CreatePolicy", "admin:DeletePolicy", "admin:GetPolicy"], + "Condition": {"StringLike": {"admin:PolicyName": ["app-*", "app-system"]}} + }] +}` + p, err := ParseConfig(strings.NewReader(doc)) + if err != nil { + t.Fatalf("a policy naming admin:PolicyName must parse: %v", err) + } + + cases := []struct { + action AdminAction + policy []string + allowed bool + }{ + {CreatePolicyAdminAction, []string{"app-42"}, true}, + {DeletePolicyAdminAction, []string{"app-system"}, true}, + {GetPolicyAdminAction, []string{"app-7"}, true}, + {CreatePolicyAdminAction, []string{"consoleAdmin"}, false}, + {CreatePolicyAdminAction, []string{"app-"}, true}, + {CreatePolicyAdminAction, nil, false}, + {CreateUserAdminAction, []string{"app-42"}, false}, + } + for _, tc := range cases { + values := map[string][]string{} + if tc.policy != nil { + values["PolicyName"] = tc.policy + } + got := p.IsAllowed(Args{ + AccountName: "orb", + Action: Action(tc.action), + ConditionValues: values, + }) + if got != tc.allowed { + t.Errorf("%s on %v: allowed=%v, want %v", tc.action, tc.policy, got, tc.allowed) + } + } +} + +// A Deny scoped by name overrides a wider Allow. +func TestAdminPolicyNameConditionDeny(t *testing.T) { + doc := `{ + "Version": "2012-10-17", + "Statement": [ + {"Effect": "Allow", "Action": ["admin:CreatePolicy"]}, + {"Effect": "Deny", "Action": ["admin:CreatePolicy"], + "Condition": {"StringEquals": {"admin:PolicyName": ["consoleAdmin"]}}} + ] +}` + p, err := ParseConfig(strings.NewReader(doc)) + if err != nil { + t.Fatal(err) + } + allowed := func(name string) bool { + return p.IsAllowed(Args{ + AccountName: "orb", + Action: Action(CreatePolicyAdminAction), + ConditionValues: map[string][]string{"PolicyName": {name}}, + }) + } + if allowed("consoleAdmin") { + t.Error("the Deny must win for consoleAdmin") + } + if !allowed("app-1") { + t.Error("the Allow must still hold for other names") + } +} diff --git a/policy/condition/keyname.go b/policy/condition/keyname.go index 120d661..5c7f381 100644 --- a/policy/condition/keyname.go +++ b/policy/condition/keyname.go @@ -37,6 +37,7 @@ var toTrim = map[string]bool{ "s3": true, "s3tables": true, "memory": true, + "admin": true, } // Name - returns the key name with its service prefix stripped, so a key reads @@ -274,6 +275,12 @@ const ( // SVCDurationSeconds - Duration seconds condition for Admin policy SVCDurationSeconds KeyName = "svc:DurationSeconds" + + // AdminPolicyName - the name of the policy a policy admin action + // (admin:CreatePolicy, admin:DeletePolicy, admin:GetPolicy) works on, so a + // statement can grant those actions on a set of policies, for example + // StringLike {"admin:PolicyName": ["app-*"]}. + AdminPolicyName KeyName = "admin:PolicyName" ) // JWTKeys - Supported JWT keys, non-exhaustive list please @@ -377,6 +384,7 @@ var AllSupportedKeys = []KeyName{ JWTClientID, STSDurationSeconds, SVCDurationSeconds, + AdminPolicyName, } // CommonKeys - is list of all common condition keys. @@ -428,6 +436,7 @@ var AllSupportedAdminKeys = append([]KeyName{ LDAPUsername, LDAPGroups, SVCDurationSeconds, + AdminPolicyName, // Add new supported condition keys. }, JWTKeys...) From 360a546ef00f8bbfb536eb61ec88964e49f29926 Mon Sep 17 00:00:00 2001 From: Harshavardhana Date: Sat, 26 Sep 2026 01:24:16 -0700 Subject: [PATCH 2/4] policy: refuse admin condition keys on S3 actions admin:PolicyName describes an admin API request, so it must only validate on admin actions. It has to be in AllSupportedKeys to parse, which also put it in the s3:* key set; leave admin: keys out of that set. --- policy/action.go | 6 ++++++ policy/admin_policy_name_test.go | 12 ++++++++++++ 2 files changed, 18 insertions(+) diff --git a/policy/action.go b/policy/action.go index 420e90c..3c0d0ee 100644 --- a/policy/action.go +++ b/policy/action.go @@ -18,6 +18,8 @@ package policy import ( + "strings" + "github.com/minio/pkg/v3/policy/condition" "github.com/minio/pkg/v3/wildcard" ) @@ -417,6 +419,10 @@ func createActionConditionKeyMap() ActionConditionKeyMap { allSupportedKeys := []condition.Key{} for _, keyName := range condition.AllSupportedKeys { + // Admin keys describe admin API requests; an S3 request never carries them. + if strings.HasPrefix(string(keyName), "admin:") { + continue + } allSupportedKeys = append(allSupportedKeys, keyName.ToKey()) } diff --git a/policy/admin_policy_name_test.go b/policy/admin_policy_name_test.go index f0d5316..008024a 100644 --- a/policy/admin_policy_name_test.go +++ b/policy/admin_policy_name_test.go @@ -94,3 +94,15 @@ func TestAdminPolicyNameConditionDeny(t *testing.T) { t.Error("the Allow must still hold for other names") } } + +// admin:PolicyName describes an admin API request, so no S3 statement may use it. +func TestAdminPolicyNameRefusedOnS3Actions(t *testing.T) { + for _, action := range []string{"s3:*", "s3:GetObject", "s3:PutObject"} { + doc := `{"Version": "2012-10-17", "Statement": [{"Effect": "Allow", "Action": ["` + action + `"], + "Resource": ["arn:aws:s3:::bucket/*"], + "Condition": {"StringLike": {"admin:PolicyName": ["app-*"]}}}]}` + if _, err := ParseConfig(strings.NewReader(doc)); err == nil { + t.Errorf("%s with admin:PolicyName must be refused", action) + } + } +} From cf3851ded7b3b4a01fb35b8f1c6921b17a8a668c Mon Sep 17 00:00:00 2001 From: Harshavardhana Date: Sat, 26 Sep 2026 08:44:45 -0700 Subject: [PATCH 3/4] policy: never read admin condition keys from a request header A request header reaches condition values under its canonical form, and getValuesByKey falls back to that form. For admin:PolicyName that let a caller name the policy itself (a PolicyName header read as Policyname) on any admin handler that does not set the value, so an identity scoped to some policies could act on others. Admin keys now read only the value the server set. Name the admin prefix as a constant with KeyName.IsAdmin, and add adversarial tests: header forms, lookalike names, misspelled keys, the key on every non-admin action family, mixed statements, and NotAction. --- policy/action.go | 4 +- policy/admin_policy_name_test.go | 109 +++++++++++++++++++++++++++++++ policy/condition/keyname.go | 29 +++++--- policy/condition/value.go | 2 +- 4 files changed, 131 insertions(+), 13 deletions(-) diff --git a/policy/action.go b/policy/action.go index 3c0d0ee..15d5178 100644 --- a/policy/action.go +++ b/policy/action.go @@ -18,8 +18,6 @@ package policy import ( - "strings" - "github.com/minio/pkg/v3/policy/condition" "github.com/minio/pkg/v3/wildcard" ) @@ -420,7 +418,7 @@ func createActionConditionKeyMap() ActionConditionKeyMap { allSupportedKeys := []condition.Key{} for _, keyName := range condition.AllSupportedKeys { // Admin keys describe admin API requests; an S3 request never carries them. - if strings.HasPrefix(string(keyName), "admin:") { + if keyName.IsAdmin() { continue } allSupportedKeys = append(allSupportedKeys, keyName.ToKey()) diff --git a/policy/admin_policy_name_test.go b/policy/admin_policy_name_test.go index 008024a..3f62480 100644 --- a/policy/admin_policy_name_test.go +++ b/policy/admin_policy_name_test.go @@ -106,3 +106,112 @@ func TestAdminPolicyNameRefusedOnS3Actions(t *testing.T) { } } } + +// scopedPolicyAdmin is an identity allowed to manage the app-* policies only. +const scopedPolicyAdmin = `{ + "Version": "2012-10-17", + "Statement": [{ + "Effect": "Allow", + "Action": ["admin:CreatePolicy", "admin:DeletePolicy", "admin:GetPolicy"], + "Condition": {"StringLike": {"admin:PolicyName": ["app-*"]}} + }] +}` + +func allowedPolicyAdmin(t *testing.T, doc string, action AdminAction, values map[string][]string) bool { + t.Helper() + p, err := ParseConfig(strings.NewReader(doc)) + if err != nil { + t.Fatal(err) + } + return p.IsAllowed(Args{AccountName: "orb", Action: Action(action), ConditionValues: values}) +} + +// Only the server sets admin:PolicyName. A request header reaches condition +// values under its canonical form, Policyname, which other keys fall back to; +// an admin key never does, so a header cannot name a policy. +func TestAdminPolicyNameIgnoresHeaderForm(t *testing.T) { + for _, values := range []map[string][]string{ + {"Policyname": {"app-1"}}, + {"policyname": {"app-1"}}, + {"POLICYNAME": {"app-1"}}, + {"Policy-Name": {"app-1"}}, + } { + if allowedPolicyAdmin(t, scopedPolicyAdmin, CreatePolicyAdminAction, values) { + t.Errorf("%v must not satisfy admin:PolicyName", values) + } + } + // The value the server set wins over a header form alongside it. + values := map[string][]string{"PolicyName": {"consoleAdmin"}, "Policyname": {"app-1"}} + if allowedPolicyAdmin(t, scopedPolicyAdmin, CreatePolicyAdminAction, values) { + t.Error("a header form must not override the server's PolicyName") + } +} + +// Names that only resemble a granted one do not match it. +func TestAdminPolicyNameLookalikes(t *testing.T) { + for _, name := range []string{ + "APP-1", // case differs + "App-1", // case differs + "xapp-1", // prefix before the pattern + " app-1", // leading space + "*", // a wildcard is a name here, not a pattern + "app", // shorter than the pattern's literal part + "consoleAdmin", // unrelated + "", // empty + "ａｐｐ-1", // full-width letters + } { + if allowedPolicyAdmin(t, scopedPolicyAdmin, CreatePolicyAdminAction, map[string][]string{"PolicyName": {name}}) { + t.Errorf("PolicyName %q must not match app-*", name) + } + } +} + +// A condition key spelled any other way is not admin:PolicyName: the policy +// is refused rather than parsed into a condition nothing satisfies or, worse, +// one something else satisfies. +func TestAdminPolicyNameMisspellingsRefused(t *testing.T) { + for _, key := range []string{"Admin:PolicyName", "admin:policyname", "admin:Policyname", "ADMIN:POLICYNAME", "admin:PolicyName ", "aws:PolicyName", "s3:PolicyName"} { + doc := `{"Version": "2012-10-17", "Statement": [{"Effect": "Allow", + "Action": ["admin:CreatePolicy"], + "Condition": {"StringLike": {"` + key + `": ["app-*"]}}}]}` + if _, err := ParseConfig(strings.NewReader(doc)); err == nil { + t.Errorf("condition key %q must be refused", key) + } + } +} + +// admin:PolicyName is refused on every non-admin action family, and a +// statement may not mix admin actions with others to carry it along. +func TestAdminPolicyNameRefusedOutsideAdmin(t *testing.T) { + for _, actions := range []string{ + `"s3tables:*"`, + `"s3tables:CreateTable"`, + `"sts:AssumeRole"`, + `"admin:CreatePolicy", "s3:GetObject"`, + } { + doc := `{"Version": "2012-10-17", "Statement": [{"Effect": "Allow", "Action": [` + actions + `], + "Resource": ["*"], + "Condition": {"StringLike": {"admin:PolicyName": ["app-*"]}}}]}` + if _, err := ParseConfig(strings.NewReader(doc)); err == nil { + t.Errorf("actions %s with admin:PolicyName must be refused", actions) + } + } +} + +// NotAction cannot widen the scoped grant: a statement allowing every admin +// action but CreateUser, under the same condition, still leaves other policies +// out of reach. +func TestAdminPolicyNameNotAction(t *testing.T) { + doc := `{"Version": "2012-10-17", "Statement": [{"Effect": "Allow", + "NotAction": ["admin:CreateUser"], + "Condition": {"StringLike": {"admin:PolicyName": ["app-*"]}}}]}` + p, err := ParseConfig(strings.NewReader(doc)) + if err != nil { + return + } + for _, name := range []string{"consoleAdmin", "readwrite"} { + if p.IsAllowed(Args{AccountName: "orb", Action: Action(CreatePolicyAdminAction), ConditionValues: map[string][]string{"PolicyName": {name}}}) { + t.Errorf("NotAction must not allow creating %s", name) + } + } +} diff --git a/policy/condition/keyname.go b/policy/condition/keyname.go index 5c7f381..efc90d2 100644 --- a/policy/condition/keyname.go +++ b/policy/condition/keyname.go @@ -27,17 +27,23 @@ import ( // for more information about available condition keys. type KeyName string +// adminKeyPrefix is the service prefix of the keys that describe an admin API +// request, such as admin:PolicyName. The server sets their values itself, so a +// policy may use them on admin actions only, and no request header supplies +// them. +const adminKeyPrefix = "admin" + // Prefixes to trim from key names. var toTrim = map[string]bool{ - "aws": true, - "jwt": true, - "ldap": true, - "sts": true, - "svc": true, - "s3": true, - "s3tables": true, - "memory": true, - "admin": true, + "aws": true, + "jwt": true, + "ldap": true, + "sts": true, + "svc": true, + "s3": true, + "s3tables": true, + "memory": true, + adminKeyPrefix: true, } // Name - returns the key name with its service prefix stripped, so a key reads @@ -52,6 +58,11 @@ func (key KeyName) Name() string { return string(key[idx+1:]) } +// IsAdmin reports whether key describes an admin API request. +func (key KeyName) IsAdmin() bool { + return strings.HasPrefix(string(key), adminKeyPrefix+":") +} + // VarName - returns variable key name, such as "${aws:username}" func (key KeyName) VarName() string { return fmt.Sprintf("${%s}", key) diff --git a/policy/condition/value.go b/policy/condition/value.go index d292931..dddfd79 100644 --- a/policy/condition/value.go +++ b/policy/condition/value.go @@ -28,7 +28,7 @@ import ( func getValuesByKey(m map[string][]string, key Key) []string { name := key.Name() - if values, found := m[name]; found { + if values, found := m[name]; found || key.name.IsAdmin() { return values } return m[http.CanonicalHeaderKey(name)] From 9ba936d09e5a413a17021e8be8f197685e9961ce Mon Sep 17 00:00:00 2001 From: Harshavardhana Date: Sat, 26 Sep 2026 09:15:40 -0700 Subject: [PATCH 4/4] policy: make the NotAction test evaluate NotAction The statement had no Resource, so ParseConfig refused it and the test returned before checking anything. Give it a Resource, fail on a parse error, and assert the granted name is still allowed. --- policy/admin_policy_name_test.go | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/policy/admin_policy_name_test.go b/policy/admin_policy_name_test.go index 3f62480..7d3aac5 100644 --- a/policy/admin_policy_name_test.go +++ b/policy/admin_policy_name_test.go @@ -204,10 +204,14 @@ func TestAdminPolicyNameRefusedOutsideAdmin(t *testing.T) { func TestAdminPolicyNameNotAction(t *testing.T) { doc := `{"Version": "2012-10-17", "Statement": [{"Effect": "Allow", "NotAction": ["admin:CreateUser"], + "Resource": ["arn:aws:s3:::*"], "Condition": {"StringLike": {"admin:PolicyName": ["app-*"]}}}]}` p, err := ParseConfig(strings.NewReader(doc)) if err != nil { - return + t.Fatal(err) + } + if !p.IsAllowed(Args{AccountName: "orb", Action: Action(CreatePolicyAdminAction), ConditionValues: map[string][]string{"PolicyName": {"app-1"}}}) { + t.Error("NotAction must still allow the granted app-1") } for _, name := range []string{"consoleAdmin", "readwrite"} { if p.IsAllowed(Args{AccountName: "orb", Action: Action(CreatePolicyAdminAction), ConditionValues: map[string][]string{"PolicyName": {name}}}) {