Skip to content

Commit fd2cb71

Browse files
Vonngklauspost
andcommitted
fix(policy): detect denies in directly constructed policies
Scan the statements when the cached Deny flag is unset. Keep the cached true path and avoid mutating policies during inspection. Cover literal policies, indexed policies and the built-in readonly definition. Adapt the fallback from minio/pkg commit 911bb0d. Co-authored-by: Klaus Post <klauspost@gmail.com> Signed-off-by: Feng Ruohang <rh@vonng.com>
1 parent c8bc102 commit fd2cb71

2 files changed

Lines changed: 49 additions & 1 deletion

File tree

policy/policy.go

Lines changed: 10 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -127,7 +127,16 @@ type Policy struct {
127127

128128
// HasDenyStatement returns if the policy has a deny statement.
129129
func (iamp *Policy) HasDenyStatement() bool {
130-
return iamp.hasDeny
130+
if iamp.hasDeny {
131+
return true
132+
}
133+
// Directly constructed policies have not populated the cached flag.
134+
for i := range iamp.Statements {
135+
if iamp.Statements[i].Effect == Deny {
136+
return true
137+
}
138+
}
139+
return false
131140
}
132141

133142
// MatchResource matches resource with match resource patterns

policy/policy_regression_test.go

Lines changed: 39 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -75,3 +75,42 @@ func TestMergePoliciesPreservesNotResourceDenies(t *testing.T) {
7575
}
7676
}
7777
}
78+
79+
func TestHasDenyStatementWithoutParsing(t *testing.T) {
80+
for _, tt := range []struct {
81+
name string
82+
effects []Effect
83+
want bool
84+
}{
85+
{"empty", nil, false},
86+
{"allow", []Effect{Allow}, false},
87+
{"deny", []Effect{Deny}, true},
88+
{"allow-then-deny", []Effect{Allow, Deny}, true},
89+
} {
90+
t.Run(tt.name, func(t *testing.T) {
91+
p := Policy{Version: DefaultVersion}
92+
for _, effect := range tt.effects {
93+
p.Statements = append(p.Statements, Statement{
94+
Effect: effect, Actions: NewActionSet(GetObjectAction),
95+
Resources: NewResourceSet(NewResource("*")),
96+
})
97+
}
98+
if got := p.HasDenyStatement(); got != tt.want {
99+
t.Errorf("HasDenyStatement() = %v, want %v", got, tt.want)
100+
}
101+
p.updateActionIndex()
102+
if got := p.HasDenyStatement(); got != tt.want {
103+
t.Errorf("indexed HasDenyStatement() = %v, want %v", got, tt.want)
104+
}
105+
})
106+
}
107+
for _, p := range DefaultPolicies {
108+
if p.Name == "readonly" {
109+
if !p.Definition.HasDenyStatement() {
110+
t.Error("readonly's explicit Deny must be reported before parsing")
111+
}
112+
return
113+
}
114+
}
115+
t.Fatal("readonly policy not found")
116+
}

0 commit comments

Comments
 (0)