From e340894a3950342d2775eabbcfd74ac4854f5749 Mon Sep 17 00:00:00 2001 From: SSushmitha8 Date: Wed, 5 Aug 2026 00:04:22 -0700 Subject: [PATCH] A policy reports which resources and values it covers --- policy/condition/func.go | 30 ++++ policy/condition/func_test.go | 76 ++++++++ policy/condition/name.go | 9 + policy/condition/stringfunc_test.go | 9 +- policy/condition_values_test.go | 268 ++++++++++++++++++++++++++++ policy/policy.go | 146 +++++++++++++++ policy/table-action.go | 7 + policy/table-action_test.go | 4 +- 8 files changed, 544 insertions(+), 5 deletions(-) create mode 100644 policy/condition_values_test.go diff --git a/policy/condition/func.go b/policy/condition/func.go index 59e93ced..43d6754c 100644 --- a/policy/condition/func.go +++ b/policy/condition/func.go @@ -81,6 +81,36 @@ func (functions Functions) Keys() KeySet { return keySet } +// ValuesByKey returns the literal values every function constrains key to, +// keyed by condition name (for example "StringEquals"). Callers deriving which +// resources a policy permits use it to read the allowed set; +func (functions Functions) ValuesByKey(key Key) map[string][]string { + var byName map[string][]string + for _, f := range functions { + if f.key() != key { + continue + } + values, ok := f.toMap()[key] + if !ok { + continue + } + fname := f.name().String() + for _, v := range values.ToSlice() { + s, err := v.GetString() + if err != nil { + // Non-string values cannot name a resource; skip them so the + // caller sees no constraint rather than a bogus one. + continue + } + if byName == nil { + byName = make(map[string][]string) + } + byName[fname] = append(byName[fname], s) + } + } + return byName +} + // Clone clones Functions structure func (functions Functions) Clone() Functions { funcs := []Function{} diff --git a/policy/condition/func_test.go b/policy/condition/func_test.go index 758dff3a..7d369587 100644 --- a/policy/condition/func_test.go +++ b/policy/condition/func_test.go @@ -21,6 +21,7 @@ import ( "encoding/json" "fmt" "reflect" + "slices" "testing" ) @@ -123,6 +124,81 @@ func TestFunctionsKeys(t *testing.T) { } } +func TestFunctionsValuesByKey(t *testing.T) { + equalsFunc, err := newStringEqualsFunc(S3TablesNamespace.ToKey(), NewValueSet(NewStringValue("ns1"), NewStringValue("ns2")), "") + if err != nil { + t.Fatalf("unexpected error. %v\n", err) + } + + likeFunc, err := newStringLikeFunc(S3TablesNamespace.ToKey(), NewValueSet(NewStringValue("ns3*")), "") + if err != nil { + t.Fatalf("unexpected error. %v\n", err) + } + + // A function on another key must not contribute to the queried key. + otherKeyFunc, err := newStringEqualsFunc(S3XAmzCopySource.ToKey(), NewValueSet(NewStringValue("mybucket/myobject")), "") + if err != nil { + t.Fatalf("unexpected error. %v\n", err) + } + + // A non-string value names no resource, so it is skipped rather than + // reported as a constraint the caller could narrow on. + boolFunc, err := newNullFunc(S3TablesNamespace.ToKey(), NewValueSet(NewBoolValue(true)), "") + if err != nil { + t.Fatalf("unexpected error. %v\n", err) + } + + testCases := []struct { + name string + functions Functions + key Key + expectedResult map[string][]string + }{ + { + name: "values group under their condition name", + functions: NewFunctions(equalsFunc, likeFunc), + key: S3TablesNamespace.ToKey(), + expectedResult: map[string][]string{stringEquals: {"ns1", "ns2"}, stringLike: {"ns3*"}}, + }, + { + name: "another key contributes nothing", + functions: NewFunctions(otherKeyFunc), + key: S3TablesNamespace.ToKey(), + expectedResult: nil, + }, + { + name: "a key no function names yields nothing", + functions: NewFunctions(equalsFunc), + key: AWSSourceIP.ToKey(), + expectedResult: nil, + }, + { + name: "non-string values are skipped", + functions: NewFunctions(boolFunc), + key: S3TablesNamespace.ToKey(), + expectedResult: nil, + }, + { + name: "no functions yields nothing", + functions: NewFunctions(), + key: S3TablesNamespace.ToKey(), + expectedResult: nil, + }, + } + + for _, testCase := range testCases { + t.Run(testCase.name, func(t *testing.T) { + result := testCase.functions.ValuesByKey(testCase.key) + for _, values := range result { + slices.Sort(values) + } + if !reflect.DeepEqual(result, testCase.expectedResult) { + t.Fatalf("expected: %v, got: %v\n", testCase.expectedResult, result) + } + }) + } +} + func TestFunctionsMarshalJSON(t *testing.T) { func1, err := newStringLikeFunc(S3XAmzMetadataDirective.ToKey(), NewValueSet(NewStringValue("REPL*")), "") if err != nil { diff --git a/policy/condition/name.go b/policy/condition/name.go index 4a844acf..335d9b11 100644 --- a/policy/condition/name.go +++ b/policy/condition/name.go @@ -56,6 +56,15 @@ const ( forAnyValue = "ForAnyValue" ) +// IsAllowList reports whether a condition name constrains its key to a set of +// permitted values, as StringEquals and StringLike do. A caller deriving which +// resources a policy reaches can read those values as the reachable set; every +// other form (a negation, a numeric or date comparison) excludes or bounds +// rather than enumerating, so nothing can be derived from it. +func IsAllowList(name string) bool { + return name == stringEquals || name == stringLike +} + // Names - list of all supported condition names. var Names = map[string]struct{}{ stringEquals: {}, diff --git a/policy/condition/stringfunc_test.go b/policy/condition/stringfunc_test.go index 8d602c56..06d7d674 100644 --- a/policy/condition/stringfunc_test.go +++ b/policy/condition/stringfunc_test.go @@ -305,8 +305,10 @@ func TestBinaryEqualsFuncEvaluate(t *testing.T) { case4Function, err := newBinaryEqualsFunc( JWTGroups.ToKey(), - NewValueSet(NewStringValue( - base64.StdEncoding.EncodeToString([]byte("prod"))), + NewValueSet( + NewStringValue( + base64.StdEncoding.EncodeToString([]byte("prod")), + ), NewStringValue(base64.StdEncoding.EncodeToString([]byte("art"))), ), forAnyValue, @@ -573,7 +575,8 @@ func TestStringEqualsFuncToMap(t *testing.T) { S3XAmzCopySource.ToKey(): NewValueSet(NewStringValue("mybucket/myobject")), } - case2Function, err := newStringEqualsFunc(S3XAmzCopySource.ToKey(), + case2Function, err := newStringEqualsFunc( + S3XAmzCopySource.ToKey(), NewValueSet( NewStringValue("mybucket/myobject"), NewStringValue("yourbucket/myobject"), diff --git a/policy/condition_values_test.go b/policy/condition_values_test.go new file mode 100644 index 00000000..fc66469c --- /dev/null +++ b/policy/condition_values_test.go @@ -0,0 +1,268 @@ +// Copyright (c) 2015-2024 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 ( + "slices" + "strings" + "testing" + + "github.com/minio/pkg/v3/policy/condition" +) + +func TestConditionValues(t *testing.T) { + const ( + resource = "bucket/wh1" + action = Action("s3tables:ListTables") + ) + nsKey := condition.NewKey(condition.S3TablesNamespace, "") + + tests := []struct { + name string + policy string + wantAllow []string + wantDeny []string + wantAllAll bool + wantDenyAll bool + }{ + { + name: "StringEquals values are permitted", + policy: `{"Version":"2012-10-17","Statement":[ + {"Effect":"Allow","Action":["s3tables:ListTables"],"Resource":["arn:aws:s3tables:::bucket/wh1"], + "Condition":{"StringEquals":{"s3tables:namespace":["ns1","ns2.ns3","n4"]}}}]}`, + wantAllow: []string{"n4", "ns1", "ns2.ns3"}, + }, + { + // Empty Resources matches every resource, the polarity IsAllowed uses. + name: "a NotResource-only statement still contributes its values", + policy: `{"Version":"2012-10-17","Statement":[ + {"Effect":"Allow","Action":["s3tables:ListTables"], + "NotResource":["arn:aws:s3tables:::bucket/private/*"], + "Condition":{"StringEquals":{"s3tables:namespace":["sales"]}}}]}`, + wantAllow: []string{"sales"}, + }, + { + // ForAllValues holds when the request carries no value for the key, so + // the listed values do not bound what the grant reaches. + name: "a set qualifier is unconstrained", + policy: `{"Version":"2012-10-17","Statement":[ + {"Effect":"Allow","Action":["s3tables:ListTables"],"Resource":["arn:aws:s3tables:::bucket/wh1"], + "Condition":{"ForAllValues:StringEquals":{"s3tables:namespace":["ns1"]}}}]}`, + wantAllAll: true, + }, + { + name: "an uninterpreted operator is unconstrained", + policy: `{"Version":"2012-10-17","Statement":[ + {"Effect":"Allow","Action":["s3tables:ListTables"],"Resource":["arn:aws:s3tables:::bucket/wh1"], + "Condition":{"StringEqualsIgnoreCase":{"s3tables:namespace":["ns1"]}}}]}`, + wantAllAll: true, + }, + { + name: "no condition on the key is unconstrained", + policy: `{"Version":"2012-10-17","Statement":[ + {"Effect":"Allow","Action":["s3tables:ListTables"],"Resource":["arn:aws:s3tables:::bucket/wh1"]}]}`, + wantAllAll: true, + }, + { + name: "deny values are reported separately", + policy: `{"Version":"2012-10-17","Statement":[ + {"Effect":"Allow","Action":["s3tables:ListTables"],"Resource":["arn:aws:s3tables:::bucket/wh1"], + "Condition":{"StringEquals":{"s3tables:namespace":["ns1","ns2","ns3"]}}}, + {"Effect":"Deny","Action":["s3tables:ListTables"],"Resource":["arn:aws:s3tables:::bucket/wh1"], + "Condition":{"StringEquals":{"s3tables:namespace":["ns2"]}}}]}`, + wantAllow: []string{"ns1", "ns2", "ns3"}, + wantDeny: []string{"ns2"}, + }, + { + name: "unconditional deny reports denyAll", + policy: `{"Version":"2012-10-17","Statement":[ + {"Effect":"Allow","Action":["s3tables:ListTables"],"Resource":["arn:aws:s3tables:::bucket/wh1"], + "Condition":{"StringEquals":{"s3tables:namespace":["ns1"]}}}, + {"Effect":"Deny","Action":["s3tables:ListTables"],"Resource":["arn:aws:s3tables:::bucket/wh1"]}]}`, + wantAllow: []string{"ns1"}, + wantDenyAll: true, + }, + { + name: "another resource does not contribute", + policy: `{"Version":"2012-10-17","Statement":[ + {"Effect":"Allow","Action":["s3tables:ListTables"],"Resource":["arn:aws:s3tables:::bucket/wh2"], + "Condition":{"StringEquals":{"s3tables:namespace":["ns9"]}}}]}`, + }, + { + name: "another action does not contribute", + policy: `{"Version":"2012-10-17","Statement":[ + {"Effect":"Allow","Action":["s3tables:DeleteTable"],"Resource":["arn:aws:s3tables:::bucket/wh1"], + "Condition":{"StringEquals":{"s3tables:namespace":["ns9"]}}}]}`, + }, + { + name: "StringLike values are permitted", + policy: `{"Version":"2012-10-17","Statement":[ + {"Effect":"Allow","Action":["s3tables:ListTables"],"Resource":["arn:aws:s3tables:::bucket/wh1"], + "Condition":{"StringLike":{"s3tables:namespace":["ns1.*"]}}}]}`, + wantAllow: []string{"ns1.*"}, + }, + { + // An uninterpretable condition form must not narrow the caller's view. + name: "StringNotEquals is reported as allowAll", + policy: `{"Version":"2012-10-17","Statement":[ + {"Effect":"Allow","Action":["s3tables:ListTables"],"Resource":["arn:aws:s3tables:::bucket/wh1"], + "Condition":{"StringNotEquals":{"s3tables:namespace":["ns9"]}}}]}`, + wantAllAll: true, + }, + { + name: "wildcard resource contributes", + policy: `{"Version":"2012-10-17","Statement":[ + {"Effect":"Allow","Action":["s3tables:ListTables"],"Resource":["arn:aws:s3tables:::bucket/*"], + "Condition":{"StringEquals":{"s3tables:namespace":["ns1"]}}}]}`, + wantAllow: []string{"ns1"}, + }, + } + + for _, test := range tests { + t.Run(test.name, func(t *testing.T) { + p, err := ParseConfig(strings.NewReader(test.policy)) + if err != nil { + t.Fatalf("ParseConfig: %v", err) + } + + values := p.ConditionValues(resource, []Action{action}, nsKey)[action] + var allow, deny []string + var allowAll, denyAll bool + if values != nil { + allow, deny = values.Allow.ToSlice(), values.Deny.ToSlice() + allowAll, denyAll = values.AllowAll, values.DenyAll + } + if allowAll != test.wantAllAll { + t.Fatalf("allowAll = %v, want %v", allowAll, test.wantAllAll) + } + if denyAll != test.wantDenyAll { + t.Fatalf("denyAll = %v, want %v", denyAll, test.wantDenyAll) + } + slices.Sort(allow) + slices.Sort(deny) + if !slices.Equal(allow, test.wantAllow) { + t.Fatalf("allow = %v, want %v", allow, test.wantAllow) + } + if !slices.Equal(deny, test.wantDeny) { + t.Fatalf("deny = %v, want %v", deny, test.wantDeny) + } + }) + } +} + +func TestTableResourcePatterns(t *testing.T) { + listTables := Action("s3tables:ListTables") + deleteNamespace := Action("s3tables:DeleteNamespace") + actions := []Action{listTables, deleteNamespace} + + tests := []struct { + name string + policy string + wantAllow map[Action][]string + wantDeny map[Action][]string + wantNoData bool + }{ + { + name: "a literal warehouse is named for the granted action only", + policy: `{"Version":"2012-10-17","Statement":[ + {"Effect":"Allow","Action":["s3tables:ListTables"],"Resource":["arn:aws:s3tables:::bucket/wh1"]}]}`, + wantAllow: map[Action][]string{listTables: {"bucket/wh1"}}, + }, + { + name: "a wildcard action credits every requested action", + policy: `{"Version":"2012-10-17","Statement":[ + {"Effect":"Allow","Action":["s3tables:*"],"Resource":["arn:aws:s3tables:::bucket/wh1"]}]}`, + wantAllow: map[Action][]string{ + listTables: {"bucket/wh1"}, + deleteNamespace: {"bucket/wh1"}, + }, + }, + { + name: "a deny on one action leaves the other action's grant intact", + policy: `{"Version":"2012-10-17","Statement":[ + {"Effect":"Allow","Action":["s3tables:*"],"Resource":["arn:aws:s3tables:::bucket/wh1"]}, + {"Effect":"Deny","Action":["s3tables:DeleteNamespace"],"Resource":["arn:aws:s3tables:::bucket/wh1"]}]}`, + wantAllow: map[Action][]string{ + listTables: {"bucket/wh1"}, + deleteNamespace: {"bucket/wh1"}, + }, + wantDeny: map[Action][]string{deleteNamespace: {"bucket/wh1"}}, + }, + { + name: "a wildcard warehouse is reported verbatim", + policy: `{"Version":"2012-10-17","Statement":[ + {"Effect":"Allow","Action":["s3tables:ListTables"],"Resource":["arn:aws:s3tables:::bucket/*"]}]}`, + wantAllow: map[Action][]string{listTables: {"bucket/*"}}, + }, + { + // The statement reaches every warehouse but one, so it must name them + // all rather than none; naming none would hide a permitted warehouse. + name: "a NotResource-only statement names every resource", + policy: `{"Version":"2012-10-17","Statement":[ + {"Effect":"Allow","Action":["s3tables:ListTables"], + "NotResource":["arn:aws:s3tables:::bucket/private/*"]}]}`, + wantAllow: map[Action][]string{listTables: {"*"}}, + }, + { + name: "a plain S3 resource names no table pattern", + policy: `{"Version":"2012-10-17","Statement":[ + {"Effect":"Allow","Action":["s3:GetObject"],"Resource":["arn:aws:s3:::mybucket/*"]}]}`, + wantNoData: true, + }, + } + + for _, test := range tests { + t.Run(test.name, func(t *testing.T) { + parsed, err := ParseConfig(strings.NewReader(test.policy)) + if err != nil { + t.Fatalf("ParseConfig: %v", err) + } + got := parsed.TableResourcePatterns(actions) + if test.wantNoData { + if len(got) != 0 { + t.Fatalf("expected no patterns, got %v", got) + } + return + } + for action, want := range test.wantAllow { + entry := got[action] + if entry == nil { + t.Fatalf("%s: no entry", action) + } + allow := entry.Allow.ToSlice() + slices.Sort(allow) + slices.Sort(want) + if !slices.Equal(allow, want) { + t.Errorf("%s allow = %v, want %v", action, allow, want) + } + } + for action, entry := range got { + deny := entry.Deny.ToSlice() + slices.Sort(deny) + want := test.wantDeny[action] + slices.Sort(want) + if len(deny) == 0 && len(want) == 0 { + continue + } + if !slices.Equal(deny, want) { + t.Errorf("%s deny = %v, want %v", action, deny, want) + } + } + }) + } +} diff --git a/policy/policy.go b/policy/policy.go index d865f646..56e02a32 100644 --- a/policy/policy.go +++ b/policy/policy.go @@ -28,6 +28,7 @@ import ( "sync" "github.com/minio/minio-go/v7/pkg/set" + "github.com/minio/pkg/v3/policy/condition" "github.com/minio/pkg/v3/wildcard" ) @@ -140,6 +141,151 @@ func (iamp Policy) MatchResource(resource string) bool { return false } +// ConditionValues reports the values of key that this policy names for each of +// actions on resource,so callers can compose Allow and Deny +// across several policies. +// +// Only StringEquals and StringLike contribute; any other form on key sets AllowAll +// or DenyAll so callers never narrow on a condition they cannot interpret. +func (iamp Policy) ConditionValues(resource string, actions []Action, key condition.Key) map[Action]*ConditionValueSet { + var byAction map[Action]*ConditionValueSet + + for _, statement := range iamp.Statements { + if len(statement.Resources) > 0 && !statement.Resources.MatchResource(resource) { + continue + } + if statement.NotResources.MatchResource(resource) { + continue + } + + values, constrained := interpretableValues(statement.Conditions, key) + + for _, action := range actions { + if len(statement.Actions) > 0 && !statement.Actions.Match(action) { + continue + } + if statement.NotActions.Match(action) { + continue + } + if byAction == nil { + byAction = make(map[Action]*ConditionValueSet, len(actions)) + } + entry := byAction[action] + if entry == nil { + entry = &ConditionValueSet{Allow: set.NewStringSet(), Deny: set.NewStringSet()} + byAction[action] = entry + } + + target, all := entry.Allow, &entry.AllowAll + if statement.Effect != Allow { + target, all = entry.Deny, &entry.DenyAll + } + if !constrained { + *all = true + continue + } + for _, v := range values { + target.Add(v) + } + } + } + + return byAction +} + +// TableResourcePatterns reports the S3 Tables resource patterns this policy names +// for each of actions, as ARN suffixes such as "bucket/wh1" or "bucket/*". Keying +// by action keeps a Deny on one action from withdrawing another's grant. +func (iamp Policy) TableResourcePatterns(actions []Action) map[Action]*ResourcePatternSet { + var byAction map[Action]*ResourcePatternSet + + for _, statement := range iamp.Statements { + patterns := make([]string, 0, len(statement.Resources)) + for resource := range statement.Resources { + if resource.isTable() { + patterns = append(patterns, resource.Pattern) + } + } + // A statement naming only NotResource reaches every resource it does not + // exclude, so it names them all rather than none. + if len(statement.Resources) == 0 && excludesTableResource(statement.NotResources) { + patterns = append(patterns, ResourceARNAll.String()) + } + if len(patterns) == 0 { + continue + } + + for _, action := range actions { + if len(statement.Actions) > 0 && !statement.Actions.Match(action) { + continue + } + if statement.NotActions.Match(action) { + continue + } + if byAction == nil { + byAction = make(map[Action]*ResourcePatternSet, len(actions)) + } + entry := byAction[action] + if entry == nil { + entry = &ResourcePatternSet{Allow: set.NewStringSet(), Deny: set.NewStringSet()} + byAction[action] = entry + } + target := entry.Allow + if statement.Effect != Allow { + target = entry.Deny + } + for _, pattern := range patterns { + target.Add(pattern) + } + } + } + + return byAction +} + +// excludesTableResource reports whether a NotResource set excludes any S3 Tables +// resource, meaning the statement reaches the remaining ones. +func excludesTableResource(notResources ResourceSet) bool { + for resource := range notResources { + if resource.isTable() { + return true + } + } + return false +} + +// ResourcePatternSet holds the resource patterns one action's statements name. +type ResourcePatternSet struct { + Allow set.StringSet + Deny set.StringSet +} + +// ConditionValueSet holds the values one action's statements name for a condition +// key. AllowAll or DenyAll mean the effect applies without constraining it. +type ConditionValueSet struct { + Allow set.StringSet + Deny set.StringSet + AllowAll bool + DenyAll bool +} + +// interpretableValues returns the literal values functions constrain key to. +func interpretableValues(functions condition.Functions, key condition.Key) (values []string, constrained bool) { + for name, vs := range functions.ValuesByKey(key) { + // A set qualifier such as "ForAllValues:" holds when the request carries no + // value for the key, so the listed values are not the reachable set. + if strings.ContainsRune(name, ':') { + continue + } + if !condition.IsAllowList(name) { + continue + } + constrained = true + values = append(values, vs...) + } + return values, constrained +} + // IsAllowedActions returns all supported actions for this policy. func (iamp Policy) IsAllowedActions(bucketName, objectName string, conditionValues map[string][]string) ActionSet { actionSet := make(ActionSet) diff --git a/policy/table-action.go b/policy/table-action.go index 71cc7308..311149e8 100644 --- a/policy/table-action.go +++ b/policy/table-action.go @@ -475,3 +475,10 @@ func createTableActionConditionKeyMap() map[Action]condition.KeySet { // tableActionConditionKeyMap - holds mapping of supported condition key for a table action. var tableActionConditionKeyMap = createTableActionConditionKeyMap() + +// TableActionConditionKeys returns the condition keys action supports. The keys +// describe the scope an action can be narrowed to, so callers can tell a +// namespace- or table-scoped action from a warehouse-level one. +func TableActionConditionKeys(action TableAction) condition.KeySet { + return tableActionConditionKeyMap[Action(action)] +} diff --git a/policy/table-action_test.go b/policy/table-action_test.go index 70c31bb8..f11570f9 100644 --- a/policy/table-action_test.go +++ b/policy/table-action_test.go @@ -69,8 +69,8 @@ func TestTableActionConditionKeys(t *testing.T) { } for i, testCase := range testCases { - keySet, ok := tableActionConditionKeyMap[Action(testCase.action)] - if !ok { + keySet := TableActionConditionKeys(testCase.action) + if len(keySet) == 0 { t.Fatalf("case %v: action %v: no condition key set registered", i+1, testCase.action) } for _, key := range testCase.expectedKeys {